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

From: Jeff King <peff@peff.net>
Date: Sep 30, 2016, 18:09 UTC
Message-ID: <20160930180957.xj4jqoslbtevhqpb@sigill.intra.peff.net>
In-Reply-To: <xmqqvaxd9ssy.fsf@gitster.mtv.corp.google.com>

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

Show 7 quoted lines

> 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