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

From: Taylor Blau <[email-masked]>
Date: Tue, 7 Sep 2021 18:21:33 -0400

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/ ?

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

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.

Thanks,
Taylor