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

From: Keith Cascio <keith@cs.ucla.edu>
Date: Jan 25, 2009, 20:14 UTC
Message-ID: <alpine.GSO.2.00.0901251149030.12651@kiwi.cs.ucla.edu>
In-Reply-To: <alpine.DEB.1.00.0901252016590.14855@racer>

On Sun, 25 Jan 2009, Johannes Schindelin wrote:

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

Someone who already wrote local scripts probably won't use the diff.primer feature, or if he does, he'll be thoughtful about the consequences and modify his scripts. diff.primer has no effect unless you use it. It is totally personal.

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?

Show 6 quoted lines

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

I fixed up every single place where gitk and git-gui internally call diff. Before I ever got there, Shawn already protected himself by passing --no-color to "git diff" (git-gui/lib/diff.tcl line 279). My changes are in a good spirit because they enhance the declarativeness and explicitness of the code. I added meaningful information.

Show 15 quoted lines

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

You are right, I should mimic the existing conventions. If we reach consensus about the functionality of the patch, I would be happy to redo all the white space to match what's already there. In case you are curious, I put whitespace in lines in the name of readability, to make tokens line up with each other. Again, it is right to mimic local convention and I have no problem making that revision.

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

I appreciate that you noticed this little detail. You are extremely thorough and I take your criticism very seriously. The reason for that little if() is that, with the enhanced declarativeness of the code in diff.h, calling the cpp macro DIFF_OPT_SET() is now more meaningful that just flipping a bit. It is now the equivalent of saying "I intend to set this bit, and I want my intention recorded and honored later." The purpose of the piece of code where I introduced the if() is only to setup the right defaults, not to declare user intention. Therefore, IOW, if the default is already in place, do nothing.

c is only one of many languages I write. Designers of newer languages emphasize meangingfulness, explicitness, declarativeness of code, e.g. the DRY principle, etc. c is very flexible, so it does not prevent us from following these beneficial practices in c. After all, the first c++ compilers simply emitted c and then called a c compiler, right?

With thanks, Keith