On Tue, 2008-06-10 at 10:59 -0400, C. Michael Pilato wrote:
> I seem to be doing alot of talking to myself at the moment, but that's cool.
I'm listening. Sorry that you didn't read my mind and know that's what I
was doing. :-)
> For the record, I think the correct-er of the fixes is the second patch.
Your patch 1 (send user_args==NULL rather than uninitialised) is
essential to fix this regression.
Your patch 2 ((svn_io_run_diff): Don't check user_args for NULL-ness;
consult the num_user_args instead) is bad: it will break this public API
for clients that were relying on it working the way it did work, and
initialising only 'user_args' but not 'num_user_args'. (Note also that
the test for null occurs twice in the function and your patch changes
only one occurrence.)
FWIW, it looks to me like this bug was introduced in r30053 in the
calling code. The svn_io_run_diff() API has been stable for ages.
The correct fix should include:
* Your patch 1.
* Update the doc string for svn_io_run_diff() to match the
implementation, which is to say that user_args must be NULL to
indicate no user args specified and thus get the default "-u".
(Presently it's ambiguous on whether user_args need be NULL
and/or num_user_args need be 0 to get that behaviour.)
> But the super-duper-bestest fix would be to rev the svn_io_run_diff()
> interface and accept apr_array_header_t *user_args instead of a C memory
> array and length indicator. Doing so allows us to support three codepaths:
>
> a. NULL array: provide the default '-u' parameter to the external diff
> b. empty array: provide no parameters
> c. non-empty array: provide the caller's specified parameters
Yes, that's goodness, but something to consider for later and perhaps
hardly worth doing until we re-vamp this external diff support properly.
> We can, of course, do this today, but folks wishing to use codepath (b) are
> forced to pass some icky non-NULL value for the C array parameter. That's
> nasty, so my second patch nipped that mess in the bud.
> C. Michael Pilato wrote:
> > By the way, I haven't looked closely, but I'm betting that the problem
> > is the difference between a NULL user_args array and an empty one
> > getting passed into svn_io_run_diff().
> >
> > Attached are two untested patches, each of which probably fixes the
> > problem, but in different ways.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-06-10 18:10:06 CEST