Re: [PATCH 3/9] pack-write: refactor renaming in finish_tmp_packfile()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 9, 2021, 19:29 UTC
- Message-ID
- <xmqqk0jpzgxu.fsf@gitster.g>
- In-Reply-To
- <35052ef494dbc55119614f3e22742d8d814b21b1.1631157880.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 19 quoted lines
> @@ -1250,8 +1251,9 @@ static void write_pack_file(void)
> &pack_idx_opts, hash);
>
> if (write_bitmap_index) {
> - strbuf_addf(&tmpname, "%s.bitmap", hash_to_hex(hash));
> + size_t tmpname_len = tmpname.len;
>
> + strbuf_addstr(&tmpname, "bitmap");
> stop_progress(&progress_state);
>
> bitmap_writer_show_progress(progress);
> @@ -1260,6 +1262,7 @@ static void write_pack_file(void)
> bitmap_writer_finish(written_list, nr_written,
> tmpname.buf, write_bitmap_options);
> write_bitmap_index = 0;
> + strbuf_setlen(&tmpname, tmpname_len);
> }
>
> strbuf_release(&tmpname);This runs setlen on tmpname strbuf and immediately releases (the close brace before release closes the "if (write_bitmap_index)", not any loop. If we plan to insert more code to use tmpname where the blank line we see above is in the future, it may make sense, but even in such a case, adding setlen() only when it starts to matter may make it easier to follow.
In any case, the above correctly adjusts tmpname to have a <hash> plus '.' at the end of tmpname to call finish_tmp_packfile().
Show 14 quoted lines
> diff --git a/bulk-checkin.c b/bulk-checkin.c > index 6283bc8bd9..c19d471f0b 100644 > --- a/bulk-checkin.c > +++ b/bulk-checkin.c > @@ -46,7 +46,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state *state) > close(fd); > } > > - strbuf_addf(&packname, "%s/pack/pack-", get_object_directory()); > + strbuf_addf(&packname, "%s/pack/pack-%s.", get_object_directory(), > + hash_to_hex(hash)); > finish_tmp_packfile(&packname, state->pack_tmp_name, > state->written, state->nr_written, > &state->pack_idx_opts, hash);
OK.
Show 17 quoted lines
> diff --git a/pack-write.c b/pack-write.c
> index 2767b78619..95b063be94 100644
> --- a/pack-write.c
> +++ b/pack-write.c
> @@ -458,6 +458,18 @@ struct hashfile *create_tmp_packfile(char **pack_tmp_name)
> return hashfd(fd, *pack_tmp_name);
> }
>
> +static void rename_tmp_packfile(struct strbuf *nb, const char *source,
> + const char *ext)
> +{
> + size_t nb_len = nb->len;
> +
> + strbuf_addstr(nb, ext);
> + if (rename(source, nb->buf))
> + die_errno("unable to rename temporary '*.%s' file to '%s'",
> + ext, nb->buf);I do not like '*.%s' here. Without '*' it is clear enough, and because nb->buf has already the same ext information,
unable to rename temporary file to '%s'.
would be even simpler without losing any information than this rewrite does.
The original explicitly used the more human-facing terms like "pack file", so we are losing information by this refactoring, but the loss is not too bad. In one case, namely ".idx" files, it is even better, as the original says "temporary index file" to refer to the new .idx file, which makes it ambiguous with _the_ index file.
> + strbuf_setlen(nb, nb_len); > +}
In the longer term if we were to add more auxiliary files next to each of the .pack file, it makes perfect sense that the common prefix is fed to rename_tmp_packfile() and the function reverts contents of the prefix buffer back when it is done with it. But it would be more friendly to those adding more calls to this function in the future to document the convention in a comment before the function.
Also, the name "nb" would need rethinking. As far as the callers are concerned, that is not a full name, but they are supplying the common prefix to the function. Perhaps "struct strbuf *name_prefix" or soemthing might be better? I dunno.
Show 14 quoted lines
> void finish_tmp_packfile(struct strbuf *name_buffer,
> const char *pack_tmp_name,
> struct pack_idx_entry **written_list,
> @@ -466,7 +478,6 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
> unsigned char hash[])
> {
> const char *idx_tmp_name, *rev_tmp_name = NULL;
> - int basename_len = name_buffer->len;
>
> if (adjust_shared_perm(pack_tmp_name))
> die_errno("unable to make temporary pack file readable");
> @@ -479,26 +490,10 @@ void finish_tmp_packfile(struct strbuf *name_buffer,
> rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash,
> pack_idx_opts->flags);It is unfortunate that write_idx_file() and write_rev_file() take hash and come up with the temporary filename on their own (which means their resulting filenames may not share the same prefix as .pack and .idx files), but it is just the naming of temporaries and inconsistencies among them is, eh, temporary, so it is OK.
Show 7 quoted lines
> + rename_tmp_packfile(name_buffer, pack_tmp_name, "pack"); > + rename_tmp_packfile(name_buffer, idx_tmp_name, "idx"); > + if (rev_tmp_name) > + rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); > > free((void *)idx_tmp_name); > }