On Wed, Dec 21, 2011 at 11:32:43AM -0800, Alexey Neyman wrote:
> Hi Stefan,
>
> Thanks for a review. A few questions:
>
> On Wednesday, December 21, 2011 03:44:07 am Stefan Sperling wrote:
> > I'd prefer keeping --no-diff-properties and add a --patch option later
> > which implies --no-diff-properties and maybe other options.
>
> I was explicitly asked by Julian Foad to avoid adding more low-level options
> (such as --no-diff-properties) and add "interface-level" options instead. I am
> fine with either approach.
I'd prefer having both low-level and high-level options available.
That way it is easier to tell what each individual option does.
Higher-level options can simply explain themselves as being equivalent
to some set of lower-level options.
But feel free to let us argue about it instead of getting involved in
the bikeshedding. We can apply your patch as-is and change the option
name later. It's no big deal. Before doing so I'll wait a bit to see
what the others have to say.
> > I'm not sure I like the name --patch.
> > Maybe call it --patch-compatible or something?
>
> Again, the option name was suggested on the list by C.Michael Pilato. I guess
> it's to be in line with the existing --git (which is not --git-compatible).
There is no existing 'git' subcommand in svn. But there is a 'patch'
subcommand. Having an option with the same name as a subcommand can
cause confusion.
I notice your --patch option description mentions GNU patch.
GNU patch is not the only patch implementation in use.
E.g. there is Larry Wall's original BSD-licensed patch implementation
on which GNU patch is based. I would rather have the --patch-compat
description say "compatibility to third-party patch programs" or
something like that.
> > > @@ -3035,6 +3037,7 @@
> > >
> > > svn_boolean_t no_diff_deleted,
> > > svn_boolean_t show_copies_as_adds,
> > > svn_boolean_t ignore_content_type,
> > > + svn_boolean_t ignore_prop_diff,
> >
> > We cannot add new arguments to already released, stable, APIs.
> > You'll need to create a new version of svn_client_diff6 called
> > svn_client_diff7 and re-implement svn_client_diff6 as a wrapper around
> > svn_client_diff7 which passes FALSE for ignore_prop_diff.
>
> I am a bit confused here. The comment for svn_client_diff6 says "New in 1.8",
> and 1.8 is not released yes, is it? I thought that non-released API is subject
> to change. If it is not the case, I'll create another wrapper.
Oh, you are correct. In that case it's fine. Sorry, I couldn't tell from
the context of the patch whether this was the case. I should've checked
the file...
Received on 2011-12-21 20:54:06 CET