Re: [PATCHv3] git apply: option to ignore whitespace differences

Giuseppe Bilotta [off-list ref] writes:

Before I go on with the next revision of the patch, I would like to
have some kind of agreed convention to implement.

My suggestion would be the following:
(1) implement options --ignore-space-change, --ignore-all-space
mirroring their 'git diff' meaning.
(2) add --ignore-whitespace as a synonym to --ignore-space-change, for
consistency with 'patch'
(3) apply.ignore-whitespace accepts values
     * false,no,none,0 to mean no whitespace ignoring
     * true,yes,change,1 to mean ignore whitespace change
     * all,2 to mean ignore all whitespace

I'd suggest taking one small bite at a time.  You do not have code to do
the ignore-all-space semantics that has been reviewed, and you neither
have the configuration variables.  So my preference would be to do in the
first round the first half of (1) with (2), docs, tests and nothing else.

A later follow-up patch would complete your (1) and (2) by supporting
ignore-all-space.  And perhaps you would give it --ignore-all-whitespace
synonym perhaps?  You may want to ask "GNU patch" people if they are
interested in ignoring all whitespaces, and if so what their plan is to
name that option, so that you can use the same name.

As to configuration:

 - I think the naming convention (I know, there are existing violators,
   but that is not an excuse to add new ones) is without dashes (again, I
   know I personally do not like CamelCase but that is what we have);

 - I personally think 0/1/2 would be more cluttering than they are useful.

 - I'd say "no (never, none)", "change", "all".  You could throw in
   "false" if you want, as declining this option is quite boolean-ish, but
   activating it is _not_ boolean decision and I would suggest actively
   keeping "true" or "yes" out of the choices.  Otherwise anybody who is
   tempted to answer "yes" has to ask himself "Ok, I want to say 'yes' but
   which semantics does it mean?  What would I be ignoring?"

That non-booleanness of "true" is why I am somewhat negative on your (2)
above.  It would only be there for people who know "GNU patch" because
they would understand it.

On Fri, Jul 31, 2009 at 7:17 PM, Junio C Hamano[off-list ref] wrote:

Giuseppe Bilotta [off-list ref] writes:

quoted

Before I go on with the next revision of the patch, I would like to
have some kind of agreed convention to implement.

My suggestion would be the following:
(1) implement options --ignore-space-change, --ignore-all-space
mirroring their 'git diff' meaning.
(2) add --ignore-whitespace as a synonym to --ignore-space-change, for
consistency with 'patch'
(3) apply.ignore-whitespace accepts values
     * false,no,none,0 to mean no whitespace ignoring
     * true,yes,change,1 to mean ignore whitespace change
     * all,2 to mean ignore all whitespace

I'd suggest taking one small bite at a time.

Sure.

A later follow-up patch would complete your (1) and (2) by supporting
ignore-all-space.  And perhaps you would give it --ignore-all-whitespace
synonym perhaps?  You may want to ask "GNU patch" people if they are
interested in ignoring all whitespaces, and if so what their plan is to
name that option, so that you can use the same name.

I was thinking about making --ignore-whitespace (I'd rather keep the
-- because that's what patch goes for) accept an optional argument,
allowing e.g. --ignore-whitespace=no to overrule the config option. In
this case we would have --ignore-whitespace=all as synonym to
-ignore-all-space. It would also make the --ignore-whitespace option
more in line with the ignorewhitespace config

-- 
Giuseppe "Oblomov" Bilotta