Re: [PATCH 1/3] pack-write: use more idiomatic strbuf usage for packname construction

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Jeff King <peff@peff.net>
Subject: Re: [PATCH 1/3] pack-write: use more idiomatic strbuf usage for packname construction
Date: Wed, 08 Sep 2021 01:22:05 +0200	[thread overview]
Message-ID: <87pmtkj7b8.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <YTfl7U93ttUQu2jc@nand.local>

On Tue, Sep 07 2021, Taylor Blau wrote:

> On Tue, Sep 07, 2021 at 09:42:36PM +0200, Ævar Arnfjörð Bjarmason wrote:
>> Change code added in 5889271114a (finish_tmp_packfile():use strbuf for
>
> s/Change code/Code/ ?

That would make it:

    Change code added in X (...) to do strbuf_reset() instead...

Instead of:

    Code added in X (...) to do strbuf_reset() instead...

> (I wondered also if the missing space in 5889271114a's subject line was
> intentional, but it does appear in the original commit.)

*Nod*, it was automatically generated.

> Reading this patch, I'm not sure I agree that this makes the later
> changes any easier. To be honest, replacing things like
>
>>  	if (rev_tmp_name) {
>> -		strbuf_addf(name_buffer, "%s.rev", hash_to_hex(hash));
>> -		if (rename(rev_tmp_name, name_buffer->buf))
>> +		strbuf_addf(&sb, "%s%s.rev", tmp_basename->buf,
>> +			    hash_to_hex(hash));
>> +		if (rename(rev_tmp_name, sb.buf))
>>  			die_errno("unable to rename temporary reverse-index file");
>> -
>> -		strbuf_setlen(name_buffer, basename_len);
>> +		strbuf_reset(&sb);
>
> Does not much help or hurt the readability, at least in my opinion. One
> advantage of the pre-image is that we're doing less copying, but that's
> probably splitting hairs at this point.

> So, I would probably be just as happy without this patch. You mentioned
> that it makes the later changes easier, but I couldn't come up with why.
> I may be missing something, in which case it may be helpful to know what
> that is and how it makes this change necessary.

It's not that continually appending/trimming the strbuf is per-se less
readable than having a "prefix" and copying/appending to it, and as you
point out this moves us towards more allocations, but in this case
that's not the bottleneck.

It's that if I retain the current pattern while splitting up these
functions I'd need to pass a "basename_len" owned by the caller between
the two, and we'd end up juggling the "tmpname" as the "if
(write_bitmap_index)" codepath is moved around.

So just having each site get the prefix and add its own suffix seemed
much easier to deal with,.