| Message ID | 20190120204653.3224-1-svenvh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-submodule.sh: shorten submodule SHA-1s using rev-parse | expand |
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 > >
Sven van Haastregt <svenvh@gmail.com> 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 <svenvh@gmail.com> > --- > 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")"
Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
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")" >
On Tue, Jan 22, 2019 at 12:23:55PM -0800, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> 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
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 <Johannes.Schindelin@gmx.de> 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
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")"
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 <svenvh@gmail.com> --- git-submodule.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)