Re: Possible bug with `export-subst' attribute

Jonathan Nieder [off-list ref] writes:

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

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

Junio C Hamano wrote:

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

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 <redacted>
---
 If this seems to be a problem elsewhere, we will have to decouple
 the remembered --abbrev setting for %h from that for --raw output.

diff --git i/contrib/examples/git-commit.sh w/contrib/examples/git-commit.sh
index 5c72f65..23ffb02 100755
--- i/contrib/examples/git-commit.sh
+++ w/contrib/examples/git-commit.sh

@@ -631,7 +631,7 @@ then
 	if test -z "$quiet"
 	then
 		commit=`git diff-tree --always --shortstat --pretty="format:%h: %s"\
-		       --summary --root HEAD --`
+		       --abbrev --summary --root HEAD --`
 		echo "Created${initial_commit:+ initial} commit $commit"
 	fi
 fi

-- 

Jonathan Nieder [off-list ref] writes:

Junio C Hamano wrote:

quoted

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

Currently commit.c::print_summary() does this:
...
	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.

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:

isn't
this one in builtin/commit.c also a confusing no-op?

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

-- 
1.7.2.21.g04ff

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 <redacted>
Tested-by: Eli Barzilay <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

diff --git a/archive.c b/archive.c
index d700af3..edd6853 100644
--- a/archive.c
+++ b/archive.c

@@ -33,6 +33,7 @@ static void format_subst(const struct commit *commit,
 	struct strbuf fmt = STRBUF_INIT;
 	struct pretty_print_context ctx = {0};
 	ctx.date_mode = DATE_NORMAL;
+	ctx.abbrev = DEFAULT_ABBREV;
 
 	if (src == buf->buf)
 		to_free = strbuf_detach(buf, NULL);

diff --git a/t/t5001-archive-attr.sh b/t/t5001-archive-attr.sh
index 426b319..02d4d22 100755
--- a/t/t5001-archive-attr.sh
+++ b/t/t5001-archive-attr.sh

@@ -4,7 +4,7 @@ test_description='git archive attribute tests'
 
 . ./test-lib.sh
 
-SUBSTFORMAT=%H%n
+SUBSTFORMAT='%H (%h)%n'
 
 test_expect_exists() {
 	test_expect_success " $1 exists" "test -e $1"

-- 
1.7.2.21.g04ff

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 [off-list ref]
Signed-off-by: Jonathan Nieder <redacted>
---
 builtin/checkout.c |    1 -
 builtin/commit.c   |    1 -
 2 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 1994be9..eef2b48 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c

@@ -279,7 +279,6 @@ static void show_local_changes(struct object *head)
 	struct rev_info rev;
 	/* I think we want full paths, even if we're in a subdirectory. */
 	init_revisions(&rev, NULL);
-	rev.abbrev = 0;
 	rev.diffopt.output_format |= DIFF_FORMAT_NAME_STATUS;
 	if (diff_setup_done(&rev.diffopt) < 0)
 		die("diff_setup_done failed");

diff --git a/builtin/commit.c b/builtin/commit.c
index a78dbd8..279cfc1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1163,7 +1163,6 @@ static void print_summary(const char *prefix, const unsigned char *sha1)
 	init_revisions(&rev, prefix);
 	setup_revisions(0, NULL, &rev, NULL);
 
-	rev.abbrev = 0;
 	rev.diff = 1;
 	rev.diffopt.output_format =
 		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;

-- 
1.7.2.21.g04ff

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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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(-)

diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
index 5c72f65..23ffb02 100755
--- a/contrib/examples/git-commit.sh
+++ b/contrib/examples/git-commit.sh

@@ -631,7 +631,7 @@ then
 	if test -z "$quiet"
 	then
 		commit=`git diff-tree --always --shortstat --pretty="format:%h: %s"\
-		       --summary --root HEAD --`
+		       --abbrev --summary --root HEAD --`
 		echo "Created${initial_commit:+ initial} commit $commit"
 	fi
 fi

-- 
1.7.2.21.g04ff

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

v1.7.1.1~17^2~3 (2010-05-03) taught the log --format=%h machinery
to respect --abbrev instead of always abbreviating ...

...

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

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.

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.

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.

 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:

Will Palmer wrote:

quoted

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.

quoted

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

Will Palmer [off-list ref] writes:

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.

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

... And an
undefined value should (I think obviously) be interpreted as
DEFAULT_ABBREV, since that's what the word "DEFAULT" actually comes
from.

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.