# Possible bug with `export-subst' attribute 15 messages from 2010-07-25 to 2010-07-28. Participants: Eli Barzilay, Ilari Liusvaara, Jonathan Nieder, Junio C Hamano, Will Palmer. Thread: https://gitlist.dev/t/24506 ## Eli Barzilay, 2010-07-25 09:08 Subject: Possible bug with `export-subst' attribute Message-ID: <19531.65276.394443.190317@winooski.ccs.neu.edu> URL: https://gitlist.dev/e/19531.65276.394443.190317%40winooski.ccs.neu.edu ``` 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! ``` ## Ilari Liusvaara, 2010-07-25 13:09 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <20100725130935.GA22083@LK-Perkele-V2.elisa-laajakaista.fi> URL: https://gitlist.dev/e/20100725130935.GA22083%40LK-Perkele-V2.elisa-laajakaista.fi In-Reply-To: <19531.65276.394443.190317@winooski.ccs.neu.edu> ``` 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 ``` ## Jonathan Nieder, 2010-07-25 22:15 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <20100725221539.GA21813@burratino> URL: https://gitlist.dev/e/20100725221539.GA21813%40burratino In-Reply-To: <20100725130935.GA22083@LK-Perkele-V2.elisa-laajakaista.fi> ``` 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 --- 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; -- ``` ## Eli Barzilay, 2010-07-25 22:41 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <19532.48534.761164.164123@winooski.ccs.neu.edu> URL: https://gitlist.dev/e/19532.48534.761164.164123%40winooski.ccs.neu.edu In-Reply-To: <20100725221539.GA21813@burratino> ``` 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! ``` ## Junio C Hamano, 2010-07-26 06:01 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <7vbp9uaii2.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vbp9uaii2.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100725221539.GA21813@burratino> ``` Jonathan Nieder 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 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; ``` ## Jonathan Nieder, 2010-07-26 19:04 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <20100726190448.GA32367@burratino> URL: https://gitlist.dev/e/20100726190448.GA32367%40burratino In-Reply-To: <7vbp9uaii2.fsf@alter.siamese.dyndns.org> ``` 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 --- 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 -- ``` ## Junio C Hamano, 2010-07-27 17:35 Subject: Re: Possible bug with `export-subst' attribute Message-ID: <7vzkxc7rpn.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vzkxc7rpn.fsf%40alter.siamese.dyndns.org In-Reply-To: <20100726190448.GA32367@burratino> ``` Jonathan Nieder 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? ``` ## Jonathan Nieder, 2010-07-27 18:29 Subject: [PATCH 0/3] archive: abbreviate substituted commit ids again Message-ID: <20100727182942.GB5578@burratino> URL: https://gitlist.dev/e/20100727182942.GB5578%40burratino In-Reply-To: <7vzkxc7rpn.fsf@alter.siamese.dyndns.org> ``` 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 ``` ## Jonathan Nieder, 2010-07-27 18:32 Subject: [PATCH 1/3] archive: abbreviate substituted commit ids again Message-ID: <20100727183236.GC5578@burratino> URL: https://gitlist.dev/e/20100727183236.GC5578%40burratino In-Reply-To: <20100727182942.GB5578@burratino> ``` 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 Tested-by: Eli Barzilay Signed-off-by: Jonathan Nieder --- 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 ``` ## Jonathan Nieder, 2010-07-27 18:37 Subject: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev Message-ID: <20100727183706.GD5578@burratino> URL: https://gitlist.dev/e/20100727183706.GD5578%40burratino In-Reply-To: <20100727182942.GB5578@burratino> ``` 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 Signed-off-by: Jonathan Nieder --- 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 ``` ## Jonathan Nieder, 2010-07-27 18:44 Subject: [PATCH 3/3] examples/commit: use --abbrev for commit summary Message-ID: <20100727184450.GE5578@burratino> URL: https://gitlist.dev/e/20100727184450.GE5578%40burratino In-Reply-To: <20100727182942.GB5578@burratino> ``` 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 Signed-off-by: Jonathan Nieder --- 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 ``` ## Will Palmer, 2010-07-27 20:18 Subject: Re: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev Message-ID: <1280261936.4462.6.camel@walleee> URL: https://gitlist.dev/e/1280261936.4462.6.camel%40walleee In-Reply-To: <20100727183706.GD5578@burratino> ``` 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 ``` ## Jonathan Nieder, 2010-07-27 21:09 Subject: Re: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev Message-ID: <20100727210908.GA11317@burratino> URL: https://gitlist.dev/e/20100727210908.GA11317%40burratino In-Reply-To: <1280261936.4462.6.camel@walleee> ``` 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 ``` ## Will Palmer, 2010-07-28 10:01 Subject: Re: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev Message-ID: <1280311304.2378.64.camel@wpalmer.simply-domain> URL: https://gitlist.dev/e/1280311304.2378.64.camel%40wpalmer.simply-domain In-Reply-To: <20100727210908.GA11317@burratino> ``` 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 ``` ## Junio C Hamano, 2010-07-28 17:23 Subject: Re: [PATCH 2/3] checkout, commit: remove confusing assignments to rev.abbrev Message-ID: <7vhbjj5xmw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhbjj5xmw.fsf%40alter.siamese.dyndns.org In-Reply-To: <1280311304.2378.64.camel@wpalmer.simply-domain> ``` Will Palmer 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. ```