Re: [PATCH 2/3] pack-write: split up finish_tmp_packfile() function

[Taylor Blau <me@ttaylorr.com>]

On Tue, Sep 07, 2021 at 09:42:37PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Split up the finish_tmp_packfile() function and use the split-up
> version in pack-objects.c. This change should not change any
> functionality, but sets up code flow for a bug fix where we'll be able
> to move the *.idx in-place after the *.bitmap is written.

Seems like a logical step to make, and all makes sense to me. One
thought for future clean-up...

> diff --git a/pack-write.c b/pack-write.c
> index 57b9fc11423..ba5c4767dc8 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -468,16 +468,33 @@ void finish_tmp_packfile(const struct strbuf *tmp_basename,
>  			 uint32_t nr_written,
>  			 struct pack_idx_option *pack_idx_opts,
>  			 unsigned char hash[])
> +{
> +	char *idx_tmp_name = NULL;
> +
> +	stage_tmp_packfile(tmp_basename, pack_tmp_name, written_list,
> +			   nr_written, pack_idx_opts, hash, &idx_tmp_name);
> +	rename_tmp_packfile_idx(tmp_basename, hash, &idx_tmp_name);
> +
> +	free(idx_tmp_name);
> +}
> +

I was surprised that you did not replace the caller in write_pack_file
with this (and instead open-coded the implementation). But that makes
sense, since the very next commit is going to move the bitmap into place
between staging and renaming the .idx.

The only other caller of finish_tmp_packfile is in the bulk-checkin
code. So I might suggest that we get rid of this function altogether and
instead call stage_tmp_packfile() and rename_tmp_packfile_idx() directly
in bulk-checkin.c:finish_bulk_checkin().

That would allow us to slim down the header, but more importantly would
make it clear that something like finish_tmp_packfile() shouldn't be
used blindly in case there is other work to be down between staging and
remaing into place.

> -	idx_tmp_name = write_idx_file(NULL, written_list, nr_written,
> -				      pack_idx_opts, hash);
> -	if (adjust_shared_perm(idx_tmp_name))
> +	*idx_tmp_name = (char *)write_idx_file(NULL, written_list, nr_written,
> +					       pack_idx_opts, hash);

Yuck (and unavoidable!)

Thanks,
Taylor