Re: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev

From: Will Palmer <wmpalmer@gmail.com>
Date: Jul 28, 2010, 10:01 UTC
Message-ID: <1280311304.2378.64.camel@wpalmer.simply-domain>
In-Reply-To: <20100727210908.GA11317@burratino>

On Tue, 2010-07-27 at 16:09 -0500, Jonathan Nieder wrote:

Show 20 quoted lines

> Will Palmer wrote:
> 
> > the purpose of the patch was to respect --abbrev instead of always
> > abbreviating to a minimum of 7 characters. /Not/ to respect abbrev
> > "instead of always abbreviating".
> 
> Sure, though it had that added effect.
> 
> One goal of that series was to be able to write formats like this:
> 
> 	%C(commit)commit %H%Creset
> 	%M(Merge: %p
> 	)Author: %an <%ae>
> 	Date:   %ad
> 
> 	%w(0,4,4)%B
> 
> to replicate the effect of --format=medium.  With diff-tree (and
> rev-list before v1.7.0.6~1^2) that is not possible if %p abbreviates
> by default.

Not sure I understand what you're saying here.

However, just to note and throw a little unfinished code around: while the series it came from was indeed intended to allow user-defined formats which exactly match the output of built-in formats, it was one of the few "useful enough on its own" patches which got sent along prior to the main work being finished. The patch to allow user-defined formats which match built-ins exactly is much more complicated (probably much more complicated than it needs to be) and leaves plenty of wiggle-room for minor differences in formats.

It's currently stalled (mostly due to lack of time to think about git, combined with most of my git-related time being spent thinking about git-remote-svn) but the very-unfinished very-broken very-doesn't-do-enough-to-justify-itself proof-of-concept can be found here:

http://repo.or.cz/w/git/wpalmer.git/shortlog/refs/heads/pretty/parse-format-poc

or (specific commit) here: http://repo.or.cz/w/git/wpalmer.git/commit/eac1527aaf7a839bb7b60ed66a7da502b890e8b0

Show 5 quoted lines

> 
> Of course, v1.7.0.6~1^2 illustrates that no one seems to have been
> relying on the format of Merge: lines, anyway, so I am not saying that
> to make diff-tree --format=medium abbreviate by default would be a bad
> change.

I expect that no one should be relying in scripts on the format of anything log produces which is not specified explicitly. For example, I'd expect any script which wanted the information --format=medium provides to do so by listing out an explicit format-line such as the one you gave.

Show 7 quoted lines

> 
> > Perhaps armed with that phrasing, a
> > more general solution, such as equating "0" with "DEFAULT_ABBREV" rather
> > than "no abbrev", could be applied?
> 
> Maybe.  If so, one would have to deal with the other callers that
> explicitly set abbrev to 0.

Doing a simple grep for "abbrev" shows many places using "0" to mean "no abbreviation" while many other places use "40" to mean "no abbreviation". That seems bad enough, especially considering --abbrev=0 will wind up setting abbrev to MINIMUM_ABBREV.

Here's what I propose:
 - #define NO_ABBREV 40
 - replace all instances of revs->abbrev = 40 and revs->abbrev = 0 with
revs->abbrev = NO_ABBREV

That will at least make it explicit and consistent.

Meanwhile, what does abbrev = 0 actually mean? Once abbrev = 0 has never been explicitly set, its meaning becomes obvious: undefined. And an undefined value should (I think obviously) be interpreted as DEFAULT_ABBREV, since that's what the word "DEFAULT" actually comes from. I think it's safe to assume that no code-paths which explicitly want an unabbreviated value (eg: %H) will actually bother to call find_unique_abbrev, especially without explicitly setting either revs->abbrev = 0 (that would be revs->abbrev = NO_ABBREV) or revs->abbrev = 40 (same here)

> Hope that helps,
> Jonathan

-- Will