From: René Scharfe <l.s.r@web.de>
Date: Fri, 07 Oct 2016 20:45:12 GMT
Subject: Re: [PATCH 2/2] use strbuf_add_unique_abbrev() for adding short hashes, part 2
Message-ID: <baff7c23-1c0b-3df5-bb4e-661bf0383fe4@web.de>
In-Reply-To: <20161007004635.xbuaxvc6eyfkyyma@sigill.intra.peff.net>

Am 07.10.2016 um 02:46 schrieb Jeff King:
> On Tue, Sep 27, 2016 at 09:11:58PM +0200, René Scharfe wrote:
>
>> 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. :)

Thank you for paying attention. :)  In general I agree that the 
surrounding code of such changes should be checked; the issue at hand 
could be part of a bigger problem.

> 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 still think the automatically generated patch is a net win, but we 
shouldn't stop there.

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

OK, then I'll wait for that series to land.

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

Yes, and there are some more.  Will take a look.

I don't know how to crack printf-style formats using semantic patches. 
It's easy for fixed formats (silly example):

	- strbuf_addf(sb, "%s%s", a, b);
	+ strbuf_addf(sb, "%s", a);
	+ strbuf_addf(sb, "%s", b);

But how to do that for arbitrary formats?  Probably not worth it..

René