Re: [PATCH v2 2/4] pack-write: refactor renaming in finish_tmp_packfile()

[Taylor Blau <me@ttaylorr.com>]

On Wed, Sep 08, 2021 at 02:38:26AM +0200, Ævar Arnfjörð Bjarmason wrote:
> -void finish_tmp_packfile(struct strbuf *name_buffer,
> +static void rename_tmp_packfile(const char *source,
> +				 const struct strbuf *basename,
> +				 const unsigned char hash[],
> +				 const char *ext)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	strbuf_addf(&sb, "%s%s.%s", basename->buf, hash_to_hex(hash), ext);
> +	if (rename(source, sb.buf))
> +		die_errno("unable to rename temporary '*.%s' file to '%s'",
> +			  ext, sb.buf);
> +	strbuf_release(&sb);
> +}

At the end of the day, I really do not mind the extra copying of
basename. But if you were interested in a potential alternative, I'd
suggest making basename non-const and doing instead:

    static void rename_tmp_packfile(struct strbuf *basename, ...)
    {
      size_t basename_len = basename->len;
      strbuf_addf(basename, ".%s", ext);
      if (rename(source, basename.buf))
        die_errno(...);

      strbuf_setlen(basename, basename_len);
    }

where the contract of this function is "I will modify the buffer you
gave me (so do not read or write to it concurrently) but will return it
to you in the same state as it was provided".

That would be an improvement in readability (because of your idea to
extract rename_tmp_packfile()) but would result in no new copying, which
would be nice to avoid if we can.

But I do not feel that strongly, it just seems like an unnecessary
introduction of copying where we don't otherwise need it.

Thanks,
Taylor