contrib/git-resurrect.sh: make it hash-agnostic
cover contrib/git-resurrect.sh: make it hash-agnostic
Since Git supports hashes other than SHA-1, the hardcoded $_x40 used to match OIDs is no longer correct. Replace it with a hash-agnostic version.
While we're at it fix some indent issues.
There are some other style issues in the script but I'll leave those as out-of-scope.
Denton Liu (2):
contrib/git-resurrect.sh: indent with tabs contrib/git-resurrect.sh: use hash-agnostic OID pattern
contrib/git-resurrect.sh | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
Signature · 1 line
2.29.0.rc0.261.g7178c9af9c
PATCH 1/2 contrib/git-resurrect.sh: indent with tabs
In the git-resurrect script, there are a few lines that are mistakenly indented with spaces. Replace these lines with tabs.
--- contrib/git-resurrect.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
contrib/git-resurrect.sh +4 -4
@@ -27,7 +27,7 @@ n,dry-run don't recreate the branch" search_reflog () { sed -ne 's~^\([^ ]*\) .* checkout: moving from '"$1"' .*~\1~p' \ - < "$GIT_DIR"/logs/HEAD + < "$GIT_DIR"/logs/HEAD } search_reflog_merges () {
@@ -41,9 +41,9 @@ _x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" search_merges () { - git rev-list --all --grep="Merge branch '$1'" \ - --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + git rev-list --all --grep="Merge branch '$1'" \ + --pretty=tformat:"%P %s" | + sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" } search_merge_targets () {
Signature · 1 line
2.29.0.rc0.261.g7178c9af9c
Re: [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs
Quoted 13 lines · Denton Liu
In the git-resurrect script, there are a few lines that are mistakenly indented with spaces. Replace these lines with tabs.
Signed-off-by Denton Liu <[email protected]>--- contrib/git-resurrect.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)contrib/git-resurrect.sh +0
-0
contrib/git-resurrect.sh n,dry-run don't recreate the branch" · line 31
Quoted 6 lines
contrib/git-resurrect.sh +1
-1@@ -27,7 +27,7 @@ n,dry-run don't recreate the branch" search_reflog () { sed -ne 's~^\([^ ]*\) .* checkout: moving from '"$1"' .*~\1~p' \ - < "$GIT_DIR"/logs/HEAD + < "$GIT_DIR"/logs/HEAD }
Unless this is done in preparation for a more meaningful change (like not reading from on-disk logs/HEAD and instead reading from "git reflog" output in order to help our future migration to the reftable backend), I do not think the reviewers' time is worth it for cosmetic changes on this script.
Quoted 14 lines
contrib/git-resurrect.sh +3
-3search_reflog_merges () {@@ -41,9 +41,9 @@ _x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" search_merges () { - git rev-list --all --grep="Merge branch '$1'" \ - --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + git rev-list --all --grep="Merge branch '$1'" \ + --pretty=tformat:"%P %s" | + sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" } search_merge_targets () {
Re: [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs
Quoted 16 lines · Junio C Hamano
Quoted 14 lines · Denton Liu
In the git-resurrect script, there are a few lines that are mistakenly indented with spaces. Replace these lines with tabs.
Signed-off-by Denton Liu <[email protected]>--- contrib/git-resurrect.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)contrib/git-resurrect.sh +0
-0@@ -27,7 +27,7 @@ n,dry-run don't recreate the branch"
Quoted 12 lines
contrib/git-resurrect.sh n,dry-run don't recreate the branch" · line 31
Quoted 5 lines
contrib/git-resurrect.sh +1
-1search_reflog () { sed -ne 's~^\([^ ]*\) .* checkout: moving from '"$1"' .*~\1~p' \ - < "$GIT_DIR"/logs/HEAD + < "$GIT_DIR"/logs/HEAD }Unless this is done in preparation for a more meaningful change (like not reading from on-disk logs/HEAD and instead reading from "git reflog" output in order to help our future migration to the reftable backend), I do not think the reviewers' time is worth it for cosmetic changes on this script.
OK, so the "meaningful" part is to prepare for SHA-256 world. That sort-of makes sense.
But don't we want to remove the SP after the redirection operator on the same line, while we are looking at it?
Quoted 14 lines
Quoted 14 lines
contrib/git-resurrect.sh +3
-3search_reflog_merges () {@@ -41,9 +41,9 @@ _x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" search_merges () { - git rev-list --all --grep="Merge branch '$1'" \ - --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + git rev-list --all --grep="Merge branch '$1'" \ + --pretty=tformat:"%P %s" | + sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" } search_merge_targets () {
PATCH 2/2 contrib/git-resurrect.sh: use hash-agnostic OID pattern
In reply to Denton LiuSince Git now supports hashes other than SHA-1, the hash length isn't guaranteed to be 40 characters. Replace $_x40 with a hash-agnostic OID pattern.
--- contrib/git-resurrect.sh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
contrib/git-resurrect.sh +3 -4
@@ -37,19 +37,18 @@ search_reflog_merges () { ) } -_x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" -_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" +oid_pattern=$(git hash-object --stdin </dev/null | sed -e 's/./[0-9a-f]/g') search_merges () { git rev-list --all --grep="Merge branch '$1'" \ --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + sed -ne "/^$oid_pattern \($oid_pattern\) Merge .*/ {s//\1/p;$early_exit}" } search_merge_targets () { git rev-list --all --grep="Merge branch '[^']*' into $branch\$" \ --pretty=tformat:"%H %s" --all | - sed -ne "/^\($_x40\) Merge .*/ {s//\1/p;$early_exit} " + sed -ne "/^\($oid_pattern\) Merge .*/ {s//\1/p;$early_exit} " } dry_run=
Signature · 1 line
2.29.0.rc0.261.g7178c9af9c
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
contrib/git-resurrect.sh search_reflog_merges () { · line 40
Quoted 11 lines · Denton Liu
contrib/git-resurrect.sh +1
-2@@ -37,19 +37,18 @@ search_reflog_merges () { ) } -_x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" -_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" +oid_pattern=$(git hash-object --stdin </dev/null | sed -e 's/./[0-9a-f]/g')
This looks correct, although...
contrib/git-resurrect.sh search_reflog_merges () { · line 52
Quoted 13 lines
contrib/git-resurrect.sh +2
-2search_merges () { git rev-list --all --grep="Merge branch '$1'" \ --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + sed -ne "/^$oid_pattern \($oid_pattern\) Merge .*/ {s//\1/p;$early_exit}" } search_merge_targets () { git rev-list --all --grep="Merge branch '[^']*' into $branch\$" \ --pretty=tformat:"%H %s" --all | - sed -ne "/^\($_x40\) Merge .*/ {s//\1/p;$early_exit} " + sed -ne "/^\($oid_pattern\) Merge .*/ {s//\1/p;$early_exit} " }
in both cases we are matching output we asked for, so we really matching [0-9a-f]\+ would be correct and sufficient. That's a little simpler. I don't feel too strongly either way, though.
Side note: It's a shame that there is no way to convince rev-list not to print the "commit ..." header, which is really what we're avoiding here. We probably should have suppressed it with user-formats when they were introduced, but it's too late to make that change. I wonder if it would be worth adding a command-line option, though. I've often had to hack around this when parsing rev-list output (and sometimes even resort to using git-log if it's a one-off).
-Peff
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
Quoted 10 lines · Jeff King
Quoted 8 lines · Denton Liu
contrib/git-resurrect.sh +0
-0@@ -37,19 +37,18 @@ search_reflog_merges () { ) }
Quoted 7 lines
contrib/git-resurrect.sh search_reflog_merges () { · line 40
Quoted 3 lines
contrib/git-resurrect.sh +1
-2-_x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" -_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" +oid_pattern=$(git hash-object --stdin </dev/null | sed -e 's/./[0-9a-f]/g')...
in both cases we are matching output we asked for, so we really matching [0-9a-f]\+ would be correct and sufficient. That's a little simpler. I don't feel too strongly either way, though.
Yes. I think use of _x40 is overrated.
Quoted 7 lines
Side note: It's a shame that there is no way to convince rev-list not to print the "commit ..." header, which is really what we're avoiding here. We probably should have suppressed it with user-formats when they were introduced, but it's too late to make that change. I wonder if it would be worth adding a command-line option, though. I've often had to hack around this when parsing rev-list output (and sometimes even resort to using git-log if it's a one-off).
Or make "git log" without frills as fast as rev-list, perhaps?
What extra things do we do that makes "log" inherently slower than "rev-list"?
I do not mind a new option (e.g. --no-header) to "rev-list", though.
Thanks.
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
Quoted 8 lines · Junio C Hamano
Quoted 7 lines
Side note: It's a shame that there is no way to convince rev-list not to print the "commit ..." header, which is really what we're avoiding here. We probably should have suppressed it with user-formats when they were introduced, but it's too late to make that change. I wonder if it would be worth adding a command-line option, though. I've often had to hack around this when parsing rev-list output (and sometimes even resort to using git-log if it's a one-off).
Quoted 4 lines
Or make "git log" without frills as fast as rev-list, perhaps?
What extra things do we do that makes "log" inherently slower than "rev-list"?
It's not the speed of log that is a problem, but just that I usually try to use plumbing when scripting. So I often reach for rev-list first.
I do think for just listing commit hashes that log is slower, though. One reason is that when there's a commit-graph, it's not as good at avoiding reading the commit objects. E.g.:
$ time git rev-list HEAD >/dev/null real 0m0.031s user 0m0.027s sys 0m0.004s
$ time git -c core.commitgraph=false rev-list HEAD >/dev/null real 0m0.362s user 0m0.345s sys 0m0.016s
$ time git log --format=%H HEAD >/dev/null real 0m0.371s user 0m0.355s sys 0m0.016s
So running "git log" takes about the same time as rev-list if we disable the commit-graph. Which makes sense. The pretty-print code is aggressive about loading the object contents, even if we end up not needing it (because really, everything _except_ userformat does need it, and even userformat usually needs it).
I think it would be nice to make the formatting code smarter about reporting exactly which parts it needs.
Quoted 1 line
I do not mind a new option (e.g. --no-header) to "rev-list", though.
I took a brief look at this earlier today and it was more awkward than I expected. The "commit <oid>" header might also have other stuff attached to it (revision marks, parents, and who even knew we had a "--timestamp" option?). It's not clear where those things should go if we suppress the header (for oneline, they just get stuck in front of the oneline; would that be OK for userformats, too?).
-Peff
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
In reply to Jeff KingQuoted 10 lines · Jeff King
Quoted 8 lines · Denton Liu
contrib/git-resurrect.sh +0
-0@@ -37,19 +37,18 @@ search_reflog_merges () { ) }
Quoted 5 lines
contrib/git-resurrect.sh search_reflog_merges () { · line 40
Quoted 3 lines
contrib/git-resurrect.sh +1
-2-_x40="[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]" -_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" +oid_pattern=$(git hash-object --stdin </dev/null | sed -e 's/./[0-9a-f]/g')This looks correct, although...
We could write this more simply as this:
oid_pattern=$(git hash-object /dev/null | sed -e 's/./[0-9a-f]/g')
I'm almost certain that works just fine, even on Windows, and I think the hashing code may be able to optimize better if we avoid --stdin (since it knows the size ahead of time). The performance benefit, if any, won't be that significant, though, since we're only hashing 7 bytes.
This is, of course, not worth a reroll in itself.
Quoted 7 lines
Quoted 7 lines
contrib/git-resurrect.sh +1
-1search_merges () { git rev-list --all --grep="Merge branch '$1'" \ --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + sed -ne "/^$oid_pattern \($oid_pattern\) Merge .*/ {s//\1/p;$early_exit}" }
Quoted 10 lines
contrib/git-resurrect.sh search_reflog_merges () { · line 52
Quoted 6 lines
contrib/git-resurrect.sh +1
-1search_merge_targets () { git rev-list --all --grep="Merge branch '[^']*' into $branch\$" \ --pretty=tformat:"%H %s" --all | - sed -ne "/^\($_x40\) Merge .*/ {s//\1/p;$early_exit} " + sed -ne "/^\($oid_pattern\) Merge .*/ {s//\1/p;$early_exit} " }in both cases we are matching output we asked for, so we really matching [0-9a-f]\+ would be correct and sufficient. That's a little simpler. I don't feel too strongly either way, though.
The problem here is that we'd need to write [0-9a-f][0-9a-f]* because this is a BRE and a backslashed + here is a GNU extension.
I'm fine with this patch (and patch 1) as it stands or with that change. I don't think this can be ambiguous, although if we think it can be (even with silly user behavior), then we should adopt this solution.
Quoted 7 lines
Side note: It's a shame that there is no way to convince rev-list not to print the "commit ..." header, which is really what we're avoiding here. We probably should have suppressed it with user-formats when they were introduced, but it's too late to make that change. I wonder if it would be worth adding a command-line option, though. I've often had to hack around this when parsing rev-list output (and sometimes even resort to using git-log if it's a one-off).
Yeah, I use git log for scripting more often than I'd like because rev-list can't remove the header. That would indeed be a welcome feature.
Signature · 1 line
brian m. carlson: Houston, Texas, US
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
Quoted 20 lines · brian m. carlson
Quoted 8 lines · Jeff King
Quoted 7 lines · Denton Liu
search_merges() {contrib/git-resurrect.sh +1
-1git rev-list --all --grep="Merge branch '$1'" \ --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + sed -ne "/^$oid_pattern \($oid_pattern\) Merge .*/ {s//\1/p;$early_exit}" }Quoted 10 lines
contrib/git-resurrect.sh search_reflog_merges () { · line 52
Quoted 6 lines
search_merge_targets() {contrib/git-resurrect.sh +1
-1git rev-list --all --grep="Merge branch '[^']*' into $branch\$" \ --pretty=tformat:"%H %s" --all | - sed -ne "/^\($_x40\) Merge .*/ {s//\1/p;$early_exit} " + sed -ne "/^\($oid_pattern\) Merge .*/ {s//\1/p;$early_exit} " }in both cases we are matching output we asked for, so we really matching [0-9a-f]\+ would be correct and sufficient. That's a little simpler. I don't feel too strongly either way, though.
Quoted 2 lines
The problem here is that we'd need to write [0-9a-f][0-9a-f]* because this is a BRE and a backslashed + here is a GNU extension.
I wonder why --pretty uses %s when it is filtered out again anyway? (There is also a duplicate --all.)
Andreas.
Signature · 3 lines
Andreas Schwab, [email protected] GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."
Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern
Quoted 8 lines · Andreas Schwab
Quoted 5 lines
Quoted 5 lines
contrib/git-resurrect.sh search_reflog_merges () { · line 45
Quoted 5 lines
search_merges() {contrib/git-resurrect.sh +1
-1git rev-list --all --grep="Merge branch '$1'" \ --pretty=tformat:"%P %s" | - sed -ne "/^$_x40 \($_x40\) Merge .*/ {s//\1/p;$early_exit}" + sed -ne "/^$oid_pattern \($oid_pattern\) Merge .*/ {s//\1/p;$early_exit}"[...]
I wonder why --pretty uses %s when it is filtered out again anyway? (There is also a duplicate --all.)
It does confirm that the commit in question is a (likely) merge commit, and not one that happens to have "Merge branch 'foo'" in the body, which the earlier --grep would have hit. But it doesn't actually check for 'foo' in the sed match, so it would be fooled by a commit message like:
Merge branch 'bar'
* bar:
Merge branch 'foo'
If we wanted to tighten that up, then sed should match the branch name. If we're willing to accept the looseness, the whole thing could probably be:
git rev-list --all --grep="Merge branch '$1'" \
--min-parents 2 --parents ${early_exit:-"-1"} |
awk '{print $3}'I'm happy either way, but I'm not sure anybody overly cares. Let's not derail Denton's actual fix to make the script work in a sha256 world.
-Peff