threads / patch / 50271

patchgit-submodule.sh: shorten submodule SHA-1s using rev-parse

Subject: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

## tl;dr

7 messages between Jan 21, 2019 and Jan 25, 2019. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Sven van Haastregt· Jan 20, 2019, 20:46 UTC · lore

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(-)

Show changes to git-submodule.sh +2 −2

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· Jan 22, 2019, 15:24 UTC · re: Sven van Haastregt · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

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:

Show 11 quoted lines

> 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

Show 8 quoted lines

> +		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· Jan 22, 2019, 20:23 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

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.

Jeff King· Jan 24, 2019, 21:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

On Tue, Jan 22, 2019 at 12:23:55PM -0800, Junio C Hamano wrote:

Show 9 quoted lines

> 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

Johannes Schindelin· Jan 25, 2019, 13:05 UTC · re: Jeff King · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

Hi Peff,

On Thu, 24 Jan 2019, Jeff King wrote:

Show 18 quoted lines

> 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

Junio C Hamano· Jan 22, 2019, 20:22 UTC · re: Sven van Haastregt · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

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?

Show 21 quoted lines

>
> 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")"

Sven van Haastregt· Jan 22, 2019, 21:38 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-submodule.sh: shorten submodule SHA-1s using rev-parse

On 22/01/19 20:22, Junio C Hamano wrote:

Show 5 quoted lines

> 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.

Show 16 quoted lines

>> 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")"
>