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...