Re: [PATCH v2 3/6] diff: diff_aligned_abbrev: remove ellipsis after abbreviated SHA-1 value

[Junio C Hamano <gitster@pobox.com>]

Ann T Ropea <bedhanger@gmx.de> writes:

> Neither Git nor the user are in need of this (visual) aid anymore, but
> we must offer a transition period.
>
> Also, fix a typo: "abbbreviated" ---> "abbreviated".
>
> Signed-off-by: Ann T Ropea <bedhanger@gmx.de>
> ---
> v2: rename patch series & focus on removal of ellipses
>  diff.c | 69 +++++++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 39 insertions(+), 30 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index 0763e89263ef..9709dc37c6d7 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4902,41 +4902,50 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)
>  	int abblen;
>  	const char *abbrev;
>  
> +	/* Do we want all 40 hex characters?
> +	 */

The oldest parts of the codebase (including diff.c) deviate from the
rule in places, but in our multi-line comment, /* and */ occupy a
line on their own.  In this case, a single-line comment should be
sufficient, though.

>  	if (len == GIT_SHA1_HEXSZ)
>  		return oid_to_hex(oid);
>  
> -	abbrev = diff_abbrev_oid(oid, len);
> -	abblen = strlen(abbrev);
> -
> -	/*
> -	 * In well-behaved cases, where the abbbreviated result is the
> -	 * same as the requested length, append three dots after the
> -	 * abbreviation (hence the whole logic is limited to the case
> -	 * where abblen < 37); when the actual abbreviated result is a
> -	 * bit longer than the requested length, we reduce the number
> -	 * of dots so that they match the well-behaved ones.  However,
> -	 * if the actual abbreviation is longer than the requested
> -	 * length by more than three, we give up on aligning, and add
> -	 * three dots anyway, to indicate that the output is not the
> -	 * full object name.  Yes, this may be suboptimal, but this
> -	 * appears only in "diff --raw --abbrev" output and it is not
> -	 * worth the effort to change it now.  Note that this would
> -	 * likely to work fine when the automatic sizing of default
> -	 * abbreviation length is used--we would be fed -1 in "len" in
> -	 * that case, and will end up always appending three-dots, but
> -	 * the automatic sizing is supposed to give abblen that ensures
> -	 * uniqueness across all objects (statistically speaking).
> +	/* An abbreviated value is fine, possibly followed by an
> +	 * ellipsis.
>  	 */

Ditto.

> -	if (abblen < GIT_SHA1_HEXSZ - 3) {
> -		static char hex[GIT_MAX_HEXSZ + 1];
> -		if (len < abblen && abblen <= len + 2)
> -			xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
> -		else
> -			xsnprintf(hex, sizeof(hex), "%s...", abbrev);
> -		return hex;
> -	}
> +	if (print_sha1_ellipsis) {
> +		abbrev = diff_abbrev_oid(oid, len);
> +		abblen = strlen(abbrev);
> +
> +		/*
> +		 * In well-behaved cases, where the abbreviated result is the
> +		 * same as the requested length, append three dots after the
> +		 * abbreviation (hence the whole logic is limited to the case
> +		 * where abblen < 37); when the actual abbreviated result is a
> +		 * bit longer than the requested length, we reduce the number
> +		 * of dots so that they match the well-behaved ones.  However,
> +		 * if the actual abbreviation is longer than the requested
> +		 * length by more than three, we give up on aligning, and add
> +		 * three dots anyway, to indicate that the output is not the
> +		 * full object name.  Yes, this may be suboptimal, but this
> +		 * appears only in "diff --raw --abbrev" output and it is not
> +		 * worth the effort to change it now.  Note that this would
> +		 * likely to work fine when the automatic sizing of default
> +		 * abbreviation length is used--we would be fed -1 in "len" in
> +		 * that case, and will end up always appending three-dots, but
> +		 * the automatic sizing is supposed to give abblen that ensures
> +		 * uniqueness across all objects (statistically speaking).
> +		 */
> +		if (abblen < GIT_SHA1_HEXSZ - 3) {
> +			static char hex[GIT_MAX_HEXSZ + 1];
> +			if (len < abblen && abblen <= len + 2)
> +				xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
> +			else
> +				xsnprintf(hex, sizeof(hex), "%s...", abbrev);
> +			return hex;
> +		}
>  
> -	return oid_to_hex(oid);
> +		return oid_to_hex(oid);
> +	} else {
> +		return diff_abbrev_oid(oid, len);
> +	}
>  }

If I were writing this, I would have called diff_abbrev_oid() before
checking print_sha1_ellipsis, returned it if it is not set.  That
way the indentation level of the code would not have to change.

HOWEVER.

Notice the name of the function.  We no longer even attempt to align
the output, and in general the output column length of each line
would be shorter than the original.  I am wondering if the change
would be of less impact if we try to abbreviate to len+3 and then
chomp the result at the right hand side to len+3 (only if the result
is unique) when print_sha1_ellipsis is false.  Of course, once we go
that path, the code structure this patch introduces (not the one I
mentioned in the previous paragraph) would be necessary.  Essentially
you would be enhancing the "else" clause.


>  
>  static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)