Re: [PATCH 2/3] pack-write: split up finish_tmp_packfile() function
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 7, 2021, 22:28 UTC
- Message-ID
- <YTfncHIiljW/E5Lz@nand.local>
- In-Reply-To
- <patch-2.3-42f83774fe8-20210907T193600Z-avarab@gmail.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...
Show 18 quoted lines
> 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.
Show 5 quoted lines
> - 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