Re: [PATCH v1 1/3] Introduce config variable "diff.primer"

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

On Sun, 25 Jan 2009, Keith Cascio wrote:

> On Sun, 25 Jan 2009, Johannes Schindelin wrote:
> 
> > That would break existing scripts using "git diff" rather badly.  We 
> > already did not allow something like "git config alias.diff ..." from 
> > changing the behavior of "git diff", so I cannot find a reason why we 
> > should let diff.primer (a misnomer BTW) override the behavior.
> 
> I took special care to protect all core scripts from the effects.

What about my scripts I have here locally?  Do you want to change them, 
too?

> I fact, by introducing the cpp macro DIFF_MACHINE_FRIENDLY() and the 
> command-line options "--machine-friendly" and "--no-primer", I made such 
> protection declarative.  Don't you find it preferable that existing 
> programs and scripts would explicitly declare their desire for 
> machine-friendly output?

No.  We made a promise long time ago that plumbing (and "git diff" is 
pretty much plumbing, except for the configurable colorness) would not 
change behind scripts' backs.

And since Shawn uses plumbing for that very reason, your diff.primer patch 
would not be allowed to make a difference.  Ever.

Now, if you would have changed only the UI diff things (i.e. git diff, but 
not git diff-files), I could have accepted the diff.primer patch for 
different applications than "git gui", but from cursory reading of your 
patch it does not appear so.

Speaking of appearance (or for that matter, explaining why it was only a 
cursory reading): did it not occur to you that your coding style is 
utterly different from the surrounding code?

Just to number a few things that would definitely prohibit this patch from 
being applied:

- space instead of tabs,

- horrible lengths of spaces within the line,

- no space after if, but after the parenthesis.

Now, this could be good explanation why you need the patch (to ignore 
white-space), but that is not a reason of letting us suffer, too.

Besides, it seems you did a lot of "fixes" on the side that I do not like 
at all.  Simple example: if the original code cleared the 
DIRSTAT_CUMULATIVE flag, it is not acceptable for you to introduce an 
unnecessary if(), testing if the CUMULATIVE flag was set to begin with.

Ciao,
Dscho