# [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs 11 messages from 2020-10-08 to 2020-10-09. Participants: Denton Liu, Jeff King, Junio C Hamano, brian m. carlson, Andreas Schwab. Thread: https://gitlist.dev/t/54369 ## Denton Liu, 2020-10-08 06:44 Subject: [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs Message-ID: <5f5f1763043e80cbae6f9d4286aa5ba1960d32e0.1602139448.git.liu.denton@gmail.com> URL: https://gitlist.dev/e/5f5f1763043e80cbae6f9d4286aa5ba1960d32e0.1602139448.git.liu.denton%40gmail.com In-Reply-To: ``` 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 --- contrib/git-resurrect.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh index 8c171dd959..57a77c03f9 100755 --- a/contrib/git-resurrect.sh +++ b/contrib/git-resurrect.sh @@ -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 () { -- 2.29.0.rc0.261.g7178c9af9c ``` ## Denton Liu, 2020-10-08 06:44 Subject: [PATCH 0/2] contrib/git-resurrect.sh: make it hash-agnostic Message-ID: URL: https://gitlist.dev/e/cover.1602139448.git.liu.denton%40gmail.com ``` 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(-) -- 2.29.0.rc0.261.g7178c9af9c ``` ## Denton Liu, 2020-10-08 06:44 Subject: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: <6fad1fc7fdad98c3dda1ec334a10a6a9e311fef8.1602139448.git.liu.denton@gmail.com> URL: https://gitlist.dev/e/6fad1fc7fdad98c3dda1ec334a10a6a9e311fef8.1602139448.git.liu.denton%40gmail.com In-Reply-To: ``` Since 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. Signed-off-by: Denton Liu --- contrib/git-resurrect.sh | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh index 57a77c03f9..d843df3afd 100755 --- a/contrib/git-resurrect.sh +++ b/contrib/git-resurrect.sh @@ -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 URL: https://gitlist.dev/e/20201008161345.GD2823778%40coredump.intra.peff.net In-Reply-To: <6fad1fc7fdad98c3dda1ec334a10a6a9e311fef8.1602139448.git.liu.denton@gmail.com> ``` On Wed, Oct 07, 2020 at 11:44:40PM -0700, Denton Liu wrote: > diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh > index 57a77c03f9..d843df3afd 100755 > --- a/contrib/git-resurrect.sh > +++ b/contrib/git-resurrect.sh > @@ -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 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} " > } 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 ``` ## Junio C Hamano, 2020-10-08 17:32 Subject: Re: [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs Message-ID: URL: https://gitlist.dev/e/xmqqa6wwy66x.fsf%40gitster.c.googlers.com In-Reply-To: <5f5f1763043e80cbae6f9d4286aa5ba1960d32e0.1602139448.git.liu.denton@gmail.com> ``` Denton Liu writes: > 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 > --- > contrib/git-resurrect.sh | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh > index 8c171dd959..57a77c03f9 100755 > --- a/contrib/git-resurrect.sh > +++ b/contrib/git-resurrect.sh > @@ -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. > 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 () { ``` ## Junio C Hamano, 2020-10-08 18:29 Subject: Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: URL: https://gitlist.dev/e/xmqqwo00woz5.fsf%40gitster.c.googlers.com In-Reply-To: <20201008161345.GD2823778@coredump.intra.peff.net> ``` Jeff King writes: > On Wed, Oct 07, 2020 at 11:44:40PM -0700, Denton Liu wrote: > >> diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh >> index 57a77c03f9..d843df3afd 100755 >> --- a/contrib/git-resurrect.sh >> +++ b/contrib/git-resurrect.sh >> @@ -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 ... > 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. > 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. ``` ## Junio C Hamano, 2020-10-08 18:48 Subject: Re: [PATCH 1/2] contrib/git-resurrect.sh: indent with tabs Message-ID: URL: https://gitlist.dev/e/xmqqsgaowo37.fsf%40gitster.c.googlers.com In-Reply-To: ``` Junio C Hamano writes: > Denton Liu writes: > >> 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 >> --- >> contrib/git-resurrect.sh | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh >> index 8c171dd959..57a77c03f9 100755 >> --- a/contrib/git-resurrect.sh >> +++ b/contrib/git-resurrect.sh >> @@ -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. > 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? >> 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 () { ``` ## Jeff King, 2020-10-08 19:53 Subject: Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: <20201008195309.GA2841047@coredump.intra.peff.net> URL: https://gitlist.dev/e/20201008195309.GA2841047%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Oct 08, 2020 at 11:29:34AM -0700, Junio C Hamano wrote: > > 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"? 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. > 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 " 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 ``` ## brian m. carlson, 2020-10-08 22:11 Subject: Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: <20201008221101.GO1392312@camp.crustytoothpaste.net> URL: https://gitlist.dev/e/20201008221101.GO1392312%40camp.crustytoothpaste.net In-Reply-To: <20201008161345.GD2823778@coredump.intra.peff.net> ``` On 2020-10-08 at 16:13:45, Jeff King wrote: > On Wed, Oct 07, 2020 at 11:44:40PM -0700, Denton Liu wrote: > > > diff --git a/contrib/git-resurrect.sh b/contrib/git-resurrect.sh > > index 57a77c03f9..d843df3afd 100755 > > --- a/contrib/git-resurrect.sh > > +++ b/contrib/git-resurrect.sh > > @@ -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 > 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. > > 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} " > > } > > 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. > 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. -- brian m. carlson: Houston, Texas, US ``` ## Andreas Schwab, 2020-10-09 07:55 Subject: Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: <874kn3rfxy.fsf@igel.home> URL: https://gitlist.dev/e/874kn3rfxy.fsf%40igel.home In-Reply-To: <20201008221101.GO1392312@camp.crustytoothpaste.net> ``` On Okt 08 2020, brian m. carlson wrote: > On 2020-10-08 at 16:13:45, Jeff King wrote: >> On Wed, Oct 07, 2020 at 11:44:40PM -0700, Denton Liu wrote: >> > 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} " >> > } >> >> 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 wonder why --pretty uses %s when it is filtered out again anyway? (There is also a duplicate --all.) Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different." ``` ## Jeff King, 2020-10-09 11:53 Subject: Re: [PATCH 2/2] contrib/git-resurrect.sh: use hash-agnostic OID pattern Message-ID: <20201009115335.GA2855114@coredump.intra.peff.net> URL: https://gitlist.dev/e/20201009115335.GA2855114%40coredump.intra.peff.net In-Reply-To: <874kn3rfxy.fsf@igel.home> ``` On Fri, Oct 09, 2020 at 09:55:53AM +0200, Andreas Schwab wrote: > >> > 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}" > [...] > 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 ```