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

[Will Palmer <wmpalmer@gmail.com>]

On Tue, 2010-07-27 at 16:09 -0500, Jonathan Nieder wrote:
> 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

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

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