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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 25, 2009, 19:30 UTC
Message-ID: <alpine.DEB.1.00.0901252016590.14855@racer>
In-Reply-To: <alpine.GSO.2.00.0901251033160.12651@kiwi.cs.ucla.edu>

Hi,

On Sun, 25 Jan 2009, Keith Cascio wrote:

Show 8 quoted lines

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

Show 5 quoted lines

> 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