Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2

From: Jeff King <peff@peff.net>
Date: Oct 7, 2016, 00:46 UTC
Message-ID: <20161007004635.xbuaxvc6eyfkyyma@sigill.intra.peff.net>
In-Reply-To: <29e75b7b-6dd0-8c52-e444-cad1ba613cd0@web.de>

On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:

Show 17 quoted lines

> Call strbuf_add_unique_abbrev() to add abbreviated hashes to strbufs
> instead of taking detours through find_unique_abbrev() and its static
> buffer.  This is shorter and a bit more efficient.
> [...]
> diff --git a/diff.c b/diff.c
> index a178ed3..be11e4e 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3109,7 +3109,7 @@ static void fill_metainfo(struct strbuf *msg,
>  		}
>  		strbuf_addf(msg, "%s%sindex %s..", line_prefix, set,
>  			    find_unique_abbrev(one->oid.hash, abbrev));
> -		strbuf_addstr(msg, find_unique_abbrev(two->oid.hash, abbrev));
> +		strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);
>  		if (one->mode == two->mode)
>  			strbuf_addf(msg, " %06o", one->mode);
>  		strbuf_addf(msg, "%s\n", reset);

This one is an interesting case, and maybe a good example of why blind coccinelle usage can have some pitfalls. :)

We get rid of the strbuf_addstr(), but notice that we leave untouched the find_unique_abbrev() call immediately above. There was a symmetry to the two that has been lost.

Probably either:

  strbuf_addf(msg, "%s%sindex %s..%s", line_prefix, set
	find_unique_abbrev(one->oid.hash, abbrev),
	find_unique_abbrev(two->oid.hash, abbrev));

or:

  strbuf_addf(msg, "%s%sindex ", line_prefix, set);
  strbuf_add_unique_abbrev(msg, one->oid.hash, abbrev);
  strbuf_addstr(msg, "..");
  strbuf_add_unique_abbrev(msg, two->oid.hash, abbrev);

would be a more appropriate refactoring. The problem is in the original patch (which also lacks symmetry; either this predates the multi-buffer find_unique_abbrev, or the original author didn't know about it), but I think your refactor makes it slightly worse.

I noticed because I have another series which touches these lines, and it wants to symmetrically swap out find_unique_abbrev for something else. :) I don't think it's a big enough deal to switch now (and I've already rebased my series which will touch these lines), but I wanted to mention it as a thing to watch out for as we do more of these kinds of automated transformations.

Show 8 quoted lines

> --- a/submodule.c
> +++ b/submodule.c
> @@ -396,7 +396,7 @@ static void show_submodule_header(FILE *f, const char *path,
>  			find_unique_abbrev(one->hash, DEFAULT_ABBREV));
>  	if (!fast_backward && !fast_forward)
>  		strbuf_addch(&sb, '.');
> -	strbuf_addstr(&sb, find_unique_abbrev(two->hash, DEFAULT_ABBREV));
> +	strbuf_add_unique_abbrev(&sb->hash, two, DEFAULT_ABBREV);

This one is a similar situation, I think.

-Peff