# [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse 7 messages from 2019-01-21 to 2019-01-25. Participants: Sven van Haastregt, Johannes Schindelin, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/50271 ## Sven van Haastregt, 2019-01-20 20:46 Subject: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: <20190120204653.3224-1-svenvh@gmail.com> URL: https://gitlist.dev/e/20190120204653.3224-1-svenvh%40gmail.com ``` Until now, `git submodule summary` was always emitting 7-character SHA-1s that have a higher chance of being ambiguous for larger repositories. Use `git rev-parse --short` instead, which will determine suitable short SHA-1 lengths. Signed-off-by: Sven van Haastregt --- git-submodule.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-submodule.sh b/git-submodule.sh index 5e608f8bad..a422b0728d 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -850,8 +850,8 @@ cmd_summary() { ;; esac - sha1_abbr_src=$(echo $sha1_src | cut -c1-7) - sha1_abbr_dst=$(echo $sha1_dst | cut -c1-7) + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) + sha1_abbr_dst=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_dst) if test $status = T then blob="$(gettext "blob")" -- 2.20.1.dirty ``` ## Johannes Schindelin, 2019-01-22 15:24 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1901221623370.41%40tvgsbejvaqbjf.bet In-Reply-To: <20190120204653.3224-1-svenvh@gmail.com> ``` Hi Sven, On Sun, 20 Jan 2019, Sven van Haastregt wrote: > Until now, `git submodule summary` was always emitting 7-character > SHA-1s that have a higher chance of being ambiguous for larger > repositories. Use `git rev-parse --short` instead, which will > determine suitable short SHA-1 lengths. Good point. Just one suggestion: > diff --git a/git-submodule.sh b/git-submodule.sh > index 5e608f8bad..a422b0728d 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -850,8 +850,8 @@ cmd_summary() { > ;; > esac > > - sha1_abbr_src=$(echo $sha1_src | cut -c1-7) > - sha1_abbr_dst=$(echo $sha1_dst | cut -c1-7) > + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) How about `git -C "$name" rev-parse --short`? That would less likely run over 80 columns/line, either. Ciao, Johannes > + sha1_abbr_dst=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_dst) > if test $status = T > then > blob="$(gettext "blob")" > -- > 2.20.1.dirty > > ``` ## Junio C Hamano, 2019-01-22 20:22 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: URL: https://gitlist.dev/e/xmqqfttkfnmy.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190120204653.3224-1-svenvh@gmail.com> ``` Sven van Haastregt writes: > Until now, `git submodule summary` was always emitting 7-character > SHA-1s that have a higher chance of being ambiguous for larger > repositories. Use `git rev-parse --short` instead, which will > determine suitable short SHA-1 lengths. In general I think it is a good idea to scale as the repository grows by asking "rev-parse --short" to do the job. One thing it is not clear to me is that this codepath is prepared to handle sha1_src and sha1_dst referring to an object that does not exist (i.e. $missing_(src|dst)=t); the original code will still give us 7 hexdigit to show on the headline, but does the updated code that uses "rev-parse --short" give us a reasonable output? > > Signed-off-by: Sven van Haastregt > --- > git-submodule.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/git-submodule.sh b/git-submodule.sh > index 5e608f8bad..a422b0728d 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -850,8 +850,8 @@ cmd_summary() { > ;; > esac > > - sha1_abbr_src=$(echo $sha1_src | cut -c1-7) > - sha1_abbr_dst=$(echo $sha1_dst | cut -c1-7) > + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) > + sha1_abbr_dst=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_dst) > if test $status = T > then > blob="$(gettext "blob")" ``` ## Junio C Hamano, 2019-01-22 20:23 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: URL: https://gitlist.dev/e/xmqqbm48fnl0.fsf%40gitster-ct.c.googlers.com In-Reply-To: ``` Johannes Schindelin writes: >> + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) > > How about `git -C "$name" rev-parse --short`? That would less likely run > over 80 columns/line, either. That would be a separate patch, either as a preliminary or a follow-up. The existing code has too many of the same construct. ``` ## Sven van Haastregt, 2019-01-22 21:38 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: <5C478D44.2070405@gmail.com> URL: https://gitlist.dev/e/5C478D44.2070405%40gmail.com In-Reply-To: ``` On 22/01/19 20:22, Junio C Hamano wrote: > One thing it is not clear to me is that this codepath is prepared to > handle sha1_src and sha1_dst referring to an object that does not > exist (i.e. $missing_(src|dst)=t); the original code will still give > us 7 hexdigit to show on the headline, but does the updated code > that uses "rev-parse --short" give us a reasonable output? Good point, I expect it will output an error message in that case. I suppose we can fallback to the old echo and cut if the object does not exist; I'll update the patch. >> diff --git a/git-submodule.sh b/git-submodule.sh >> index 5e608f8bad..a422b0728d 100755 >> --- a/git-submodule.sh >> +++ b/git-submodule.sh >> @@ -850,8 +850,8 @@ cmd_summary() { >> ;; >> esac >> >> - sha1_abbr_src=$(echo $sha1_src | cut -c1-7) >> - sha1_abbr_dst=$(echo $sha1_dst | cut -c1-7) >> + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) >> + sha1_abbr_dst=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_dst) >> if test $status = T >> then >> blob="$(gettext "blob")" > ``` ## Jeff King, 2019-01-24 21:49 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: <20190124214953.GA19668@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190124214953.GA19668%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Jan 22, 2019 at 12:23:55PM -0800, Junio C Hamano wrote: > Johannes Schindelin writes: > > >> + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) > > > > How about `git -C "$name" rev-parse --short`? That would less likely run > > over 80 columns/line, either. > > That would be a separate patch, either as a preliminary or a > follow-up. The existing code has too many of the same construct. It's also not the same; if $GIT_DIR is already set in the environment, that will override auto-discovery done after the chdir() triggered by "-C". It would need to be "git --git-dir". Sincerely, Somebody who has been bit by the distinction many times ``` ## Johannes Schindelin, 2019-01-25 13:05 Subject: Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse Message-ID: URL: https://gitlist.dev/e/nycvar.QRO.7.76.6.1901251405010.41%40tvgsbejvaqbjf.bet In-Reply-To: <20190124214953.GA19668@sigill.intra.peff.net> ``` Hi Peff, On Thu, 24 Jan 2019, Jeff King wrote: > On Tue, Jan 22, 2019 at 12:23:55PM -0800, Junio C Hamano wrote: > > > Johannes Schindelin writes: > > > > >> + sha1_abbr_src=$(GIT_DIR="$name/.git" git rev-parse --short $sha1_src) > > > > > > How about `git -C "$name" rev-parse --short`? That would less likely run > > > over 80 columns/line, either. > > > > That would be a separate patch, either as a preliminary or a > > follow-up. The existing code has too many of the same construct. > > It's also not the same; if $GIT_DIR is already set in the environment, > that will override auto-discovery done after the chdir() triggered by > "-C". It would need to be "git --git-dir". > > Sincerely, > Somebody who has been bit by the distinction many times Ouch. Good point. Thanks, Dscho ```