Possible bug with `export-subst' attribute

Possible bug with `export-subst' attribute

[Eli Barzilay]

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.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: Possible bug with `export-subst' attribute

[Ilari Liusvaara]

On Sun, Jul 25, 2010 at 05:08:12AM -0400, Eli Barzilay wrote:
> 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.
 
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

Re: Possible bug with `export-subst' attribute

[Jonathan Nieder]

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

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>
---
diff --git i/archive.c w/archive.c
index d700af3..edd6853 100644
--- i/archive.c
+++ w/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 i/builtin/checkout.c w/builtin/checkout.c
index 1994be9..eef2b48 100644
--- i/builtin/checkout.c
+++ w/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 i/builtin/commit.c w/builtin/commit.c
index a78dbd8..ae4831e 100644
--- i/builtin/commit.c
+++ w/builtin/commit.c
@@ -1163,7 +1163,7 @@ 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.abbrev = DEFAULT_ABBREV;
 	rev.diff = 1;
 	rev.diffopt.output_format =
 		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;
-- 

Re: Possible bug with `export-subst' attribute

[Eli Barzilay]

On Jul 25, Jonathan Nieder wrote:
> Ilari Liusvaara 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).
> 
> My bad.  Would something like this fix it?

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

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

Re: Possible bug with `export-subst' attribute

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> 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 <jrnieder@gmail.com>

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

> diff --git i/builtin/commit.c w/builtin/commit.c
> index a78dbd8..ae4831e 100644
> --- i/builtin/commit.c
> +++ w/builtin/commit.c
> @@ -1163,7 +1163,7 @@ 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.abbrev = DEFAULT_ABBREV;
>  	rev.diff = 1;
>  	rev.diffopt.output_format =
>  		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;

Re: Possible bug with `export-subst' attribute

[Jonathan Nieder]

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

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

Re: Possible bug with `export-subst' attribute

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

[PATCH 0/3] archive: abbreviate substituted commit ids again

[Jonathan Nieder]

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

[PATCH 1/3] archive: abbreviate substituted commit ids again

[Jonathan Nieder]

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

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

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

[Jonathan Nieder]

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

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

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

[Will Palmer]

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

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

[Jonathan Nieder]

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

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

[Will Palmer]

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

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

[Junio C Hamano]

Will Palmer <wmpalmer@gmail.com> 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.

[PATCH 3/3] examples/commit: use --abbrev for commit summary

[Jonathan Nieder]

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

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