Possible bug with `export-subst' attribute

I have a file with:

  (define archive-id "$Format:%ct|%h|a$")

and an `export-subst' attribute -- and it looks like the "%h" results in a full sha1 instead of the abbreviated one when used with `git archive'. This is with 1.7..2 -- I'm not sure, but I think that it worked fine with 1.7.1.

On Sun, Jul 25, 2010 at 05:08:12AM -0400, Eli Barzilay wrote:

 
I remember seeing similar stuff. It isn't just archive, I also rember seeing
commit printing full hashes in that informational line it prints when it has
made the commit (IIRC, normally that hash is abbrevated).

-Ilari

Ilari Liusvaara wrote:

My bad.  Would something like this fix it?

-- 8< --
Subject: archive, commit: use --abbrev by default again

v1.7.1.1~17^2~3 (pretty: Respect --abbrev option, 2010-05-03) taught git log --format=%h to respect the --abbrev option instead of always abbreviating, with the side-effect that we have to pay attention to the abbrev setting now.

For example, the "git archive" export-subst feature and the informational line printed by "git commit" are using unabbreviated object names now, the former because full object names are the low-level default, the latter because it was first written to imitate plumbing.

Fix them. While at it, remove a similar confusing assignment of 0 to rev.abbrev in "git checkout" which had no effect.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---

On Jul 25, Jonathan Nieder wrote:

In my case (using archive), this fixes it -- thanks!

Jonathan Nieder <jrnieder@gmail.com> writes:

The ones to archive and checkout I understand, but what effect does the one to commit.c::print_summary() have?

Junio C Hamano wrote:

Currently commit.c::print_summary() does this:

	struct strbuf format = STRBUF_INIT;
	...

	strbuf_addstr(&format, "format:%h] %s");
	[+ other bits for the commit notice]

	rev.abbrev = 0;
	rev.diff = 1;
	...
	get_commit_format(format.buf, &rev)
	...

	printf("[%s%s ",
			[branch name " (root-commit)"]);

	if (!log_tree_commit(&rev, commit)) {
		...

In other words, it imbues rev with a format including %h and uses that to print a commit summary.

That code is as old as builtin commit (v1.5.4-rc0~78^2~30, 2007-11-08) and was meant to imitate a diff-tree invocation (which is plumbing).

-- %< --
Subject: examples/commit: use --abbrev for commit summary

After v1.7.1.1~17^2~3 (pretty: Respect --abbrev option, 2010-05-03), plumbing users do not abbreviate %h hashes by default any more.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 If this seems to be a problem elsewhere, we will have to decouple
 the remembered --abbrev setting for %h from that for --raw output.

Jonathan Nieder <jrnieder@gmail.com> writes:

Sorry, but I think I understood that part.

But the thing is, we do not seem to show non-abbreviated string there with or without your patch, because inside log_tree_diff_flush() -> show_log() callchain we use opt->diffopt.abbrev to decide what is done for that %h token:

	ctx.abbrev = opt->diffopt.abbrev;

so just like the confusing assignment in builtin/checkout.c, isn't this one in builtin/commit.c also a confusing no-op?

Perhaps I am missing something obvious?

Junio C Hamano wrote:

I see. When setup_revisions is called (which initializes diffopt), rev.abbrev still equals DEFAULT_ABBREV.

I missed v1.7.1.1~17^2 (commit::print_summary(): don't use format_commit_message(), 2010-06-12) and did not notice that the bug had gone away. Sorry for the confusion.

Here’s a rerolled series.

Jonathan Nieder (3):
  archive: abbreviate substituted commit ids again
  checkout, commit: remove confusing assignments to rev.abbrev
  examples/commit: use --abbrev for commit summary

 archive.c                      |    1 +
 builtin/checkout.c             |    1 -
 builtin/commit.c               |    1 -
 contrib/examples/git-commit.sh |    2 +-
 t/t5001-archive-attr.sh        |    2 +-
 5 files changed, 3 insertions(+), 4 deletions(-)

Given a file with:

  (define archive-id "$Format:%ct|%h|a$")

and an export-subst attribute, the "%h" results in an full 40-digit object name instead of the expected 7-digit one.

The export-subst feature requests unabbreviated object names because that is the low-level default. The effect was not observable until v1.7.1.1~17^2~3 (2010-05-03), which taught log --format=%h to respect the --abbrev option.

Reported-by: Eli Barzilay <eli@barzilay.org>
Tested-by: Eli Barzilay <eli@barzilay.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I carried over the tested-by; I hope that’s okay.  Well, I’ve tested
the new patch myself, at least. :)

 archive.c               |    1 +
 t/t5001-archive-attr.sh |    2 +-
 2 files changed, 2 insertions(+), 1 deletions(-)

Since they do not precede setup_revisions, these assignments of 0 to rev.abbrev have no effect.

v1.7.1.1~17^2~3 (2010-05-03) taught the log --format=%h machinery to respect --abbrev instead of always abbreviating, so we have to pay attention to the abbrev setting now.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/checkout.c |    1 -
 builtin/commit.c   |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

On Tue, 2010-07-27 at 13:37 -0500, Jonathan Nieder wrote: ...

...

I've been seeing this phrasing throughout this discussion, and at first I thought it was merely a poor choice of words, but now I feel I must ensure it's clear: 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". Perhaps armed with that phrasing, a more general solution, such as equating "0" with "DEFAULT_ABBREV" rather than "no abbrev", could be applied?

Will Palmer wrote:

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.

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.

Maybe. If so, one would have to deal with the other callers that explicitly set abbrev to 0.

 probably just confusing no-ops:

 - bisect.c::bisect_rev_setup
 - bisect.c::show_diff_tree (to imitate diff-tree: probably a no-op
   because there is no setup_revisions call)

 means FULL_SHA1:

 - diff-files.c::cmd_diff_files
 - diff-index.c::cmd_diff_index
 - diff-tree.c:cmd_diff_tree
 - revision.c::handle_revision_opt

Hope that helps, Jonathan

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

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

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.

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)

-- Will

Will Palmer <wmpalmer@gmail.com> writes:

That is a good idea. I think abbrev == 0 in the early days used to mean "use the compiled-in default, whatever it is" but somehow some codepaths mistakenly used it to mean "please do not abbreviate" (my fault).

We would probably need to be a bit careful here. By default plumbing commands do not abbreviate, while we do want the default abbreviation in our Porcelains.

After v1.7.1.1~17^2~3 (pretty: Respect --abbrev option, 2010-05-03), plumbing users do not abbreviate %h hashes by default any more.

Noticed while investigating the bug fixed by v1.7.1.1~17^2 (commit::print_summary(): don't use format_commit_message(), 2010-06-12).

Cc: Will Palmer <wmpalmer@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Again, sorry for the trouble.

Maybe diff-tree should always abbreviate the format %h. That would make the Parents: line and %p format produce different result when the --abbrev option is not supplied.

I could go either way.

 contrib/examples/git-commit.sh |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)