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

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

[Sven van Haastregt]

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

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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

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

[Sven van Haastregt]

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