[PATCH] diff_unique_abbrev(): document its assumtion and limitation

This function is used to add "..." to displayed object names in
"diff --raw --abbrev[=<n>]" output.  It bases its behaviour on an
untold assumption that the abbreviation length requested by the
caller is "reasonble", i.e. most of the objects will abbreviate
within the requested length and the resulting length would never
exceed it by more than a few hexdigits (otherwise the resulting
columns would not align).  Explain that in a comment.

Signed-off-by: Junio C Hamano <redacted>
---

 * I had to scratch my head wondering what impact Linus's
   auto-abbrev change will have on this code, which I wrote many
   years ago in 47dd0d59 ("diff: --abbrev option", 2005-12-13).

 diff.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index cefc13eb8e..428ed4f4c9 100644
--- a/diff.c
+++ b/diff.c

@@ -4108,7 +4108,8 @@ void diff_free_filepair(struct diff_filepair *p)
 	free(p);
 }
 
-/* This is different from find_unique_abbrev() in that
+/*
+ * This is different from find_unique_abbrev() in that
  * it stuffs the result with dots for alignment.
  */
 const char *diff_unique_abbrev(const unsigned char *sha1, int len)

@@ -4120,6 +4121,26 @@ const char *diff_unique_abbrev(const unsigned char *sha1, int len)
 
 	abbrev = find_unique_abbrev(sha1, 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).
+	 */
 	if (abblen < 37) {
 		static char hex[41];
 		if (len < abblen && abblen <= len + 2)

-- 
2.10.0-612-g22341905f2

On Fri, Sep 30, 2016 at 10:54:53AM -0700, Junio C Hamano wrote:

This function is used to add "..." to displayed object names in
"diff --raw --abbrev[=<n>]" output.  It bases its behaviour on an
untold assumption that the abbreviation length requested by the
caller is "reasonble", i.e. most of the objects will abbreviate
within the requested length and the resulting length would never
exceed it by more than a few hexdigits (otherwise the resulting
columns would not align).  Explain that in a comment.

Heh, I have actually have a similar patch that renames it to
diff_aligned_abbrev(). Because I wanted to add another function:

  static const char *diff_abbrev_oid(const struct object_id *oid,
                                     int abbrev)
  {
        if (startup_info->have-repository)
                return find_unique_abbrev(oid->hash, abbrev);
        else {
                char *hex = oid_to_hex(oid);
                if (abbrev < 0) || abbrev > GIT_SHA1_HEXSZ)
                        die("BUG: oid abbreviation out of range: %d", abbrev);
                hex[abbrev] = '\0';
                return hex;
        }
  }

and I didn't want people to confuse the two. Now that function _would_
want to be updated as a result of the other conversation (it would need
to do something sensible with "-1", like turning it into "7", or
whatever else is deemed reasonable outside of a repository).

Anyway. I just wonder if you want to give it a better name while you are
at it.

-Peff