From 66833f0e70c473ca6c4e6a79d34e879d8b40ba9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 9 Sep 2021 19:24:37 -0400 Subject: pack-write: refactor renaming in finish_tmp_packfile() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactor the renaming in finish_tmp_packfile() into a helper function. The callers are now expected to pass a "name_buffer" ending in "pack-OID." instead of the previous "pack-", we then append "pack", "idx" or "rev" to it. By doing the strbuf_setlen() in rename_tmp_packfile() we reuse the buffer and avoid the repeated allocations we'd get if that function had its own temporary "struct strbuf". This approach of reusing the buffer does make the last user in pack-object.c's write_pack_file() slightly awkward, since we needlessly do a strbuf_setlen() before calling strbuf_release() for consistency. In subsequent changes we'll move that bitmap writing code around, so let's not skip the strbuf_setlen() now. The previous strbuf_reset() idiom originated with 5889271114a (finish_tmp_packfile():use strbuf for pathname construction, 2014-03-03), which in turn was a minimal adjustment of pre-strbuf code added in 0e990530ae (finish_tmp_packfile(): a helper function, 2011-10-28). Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- pack-write.c | 37 ++++++++++++++++--------------------- 1 file changed, 16 insertions(+), 21 deletions(-) (limited to 'pack-write.c') diff --git a/pack-write.c b/pack-write.c index f1fc3ecafa..ca94e2b106 100644 --- a/pack-write.c +++ b/pack-write.c @@ -462,6 +462,18 @@ struct hashfile *create_tmp_packfile(char **pack_tmp_name) return hashfd(fd, *pack_tmp_name); } +static void rename_tmp_packfile(struct strbuf *name_prefix, const char *source, + const char *ext) +{ + size_t name_prefix_len = name_prefix->len; + + strbuf_addstr(name_prefix, ext); + if (rename(source, name_prefix->buf)) + die_errno("unable to rename temporary file to '%s'", + name_prefix->buf); + strbuf_setlen(name_prefix, name_prefix_len); +} + void finish_tmp_packfile(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, @@ -470,7 +482,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"); @@ -483,26 +494,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); - strbuf_addf(name_buffer, "%s.pack", hash_to_hex(hash)); - - if (rename(pack_tmp_name, name_buffer->buf)) - die_errno("unable to rename temporary pack file"); - - strbuf_setlen(name_buffer, basename_len); - - strbuf_addf(name_buffer, "%s.idx", hash_to_hex(hash)); - if (rename(idx_tmp_name, name_buffer->buf)) - die_errno("unable to rename temporary index file"); - - strbuf_setlen(name_buffer, basename_len); - - if (rev_tmp_name) { - strbuf_addf(name_buffer, "%s.rev", hash_to_hex(hash)); - if (rename(rev_tmp_name, name_buffer->buf)) - die_errno("unable to rename temporary reverse-index file"); - } - - strbuf_setlen(name_buffer, basename_len); + 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); } -- cgit v1.2.3 From 16a86907bca0ae7ed9f1dfaa6261234215151396 Mon Sep 17 00:00:00 2001 From: Taylor Blau Date: Thu, 9 Sep 2021 19:24:41 -0400 Subject: pack-write.c: rename `.idx` files after `*.rev` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We treat the presence of an `.idx` file as the indicator of whether or not it's safe to use a packfile. But `finish_tmp_packfile()` (which is responsible for writing the pack and moving the temporary versions of all of its auxiliary files into place) is inconsistent about the write order. Specifically, it moves the `.rev` file into place after the `.idx`, leaving open the possibility to open a pack which looks "ready" (because the `.idx` file exists and is readable) but appears momentarily to not have a `.rev` file. This causes Git to fall back to generating the pack's reverse index in memory. Though racy, this amounts to an unnecessary slow-down at worst, and doesn't affect the correctness of the resulting reverse index. Close this race by moving the .rev file into place before moving the .idx file into place. This still leaves the issue of `.idx` files being renamed into place before the auxiliary `.bitmap` file is renamed when in pack-object.c's write_pack_file() "write_bitmap_index" is true. That race will be addressed in subsequent commits. Signed-off-by: Taylor Blau Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- pack-write.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'pack-write.c') diff --git a/pack-write.c b/pack-write.c index ca94e2b106..51157916f5 100644 --- a/pack-write.c +++ b/pack-write.c @@ -495,9 +495,9 @@ void finish_tmp_packfile(struct strbuf *name_buffer, pack_idx_opts->flags); 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"); + rename_tmp_packfile(name_buffer, idx_tmp_name, "idx"); free((void *)idx_tmp_name); } -- cgit v1.2.3 From 2ec02dd5a8261bc837b961ef36788081ded5c2bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 9 Sep 2021 19:24:56 -0400 Subject: pack-write: split up finish_tmp_packfile() function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split up the finish_tmp_packfile() function and use the split-up version in pack-objects.c in preparation for moving the step of renaming the *.idx file later as part of a function change. Since the only other caller of finish_tmp_packfile() was in bulk-checkin.c, and it won't be needing a change to its *.idx renaming, provide a thin wrapper for the old function as a static function in that file. If other callers end up needing the simpler version it could be moved back to "pack-write.c" and "pack.h". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Taylor Blau Signed-off-by: Junio C Hamano --- pack-write.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-) (limited to 'pack-write.c') diff --git a/pack-write.c b/pack-write.c index 51157916f5..32ebf0cdf7 100644 --- a/pack-write.c +++ b/pack-write.c @@ -474,21 +474,28 @@ static void rename_tmp_packfile(struct strbuf *name_prefix, const char *source, strbuf_setlen(name_prefix, name_prefix_len); } -void finish_tmp_packfile(struct strbuf *name_buffer, +void rename_tmp_packfile_idx(struct strbuf *name_buffer, + char **idx_tmp_name) +{ + rename_tmp_packfile(name_buffer, *idx_tmp_name, "idx"); +} + +void stage_tmp_packfiles(struct strbuf *name_buffer, const char *pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, struct pack_idx_option *pack_idx_opts, - unsigned char hash[]) + unsigned char hash[], + char **idx_tmp_name) { - const char *idx_tmp_name, *rev_tmp_name = NULL; + const char *rev_tmp_name = NULL; if (adjust_shared_perm(pack_tmp_name)) die_errno("unable to make temporary pack file readable"); - 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); + if (adjust_shared_perm(*idx_tmp_name)) die_errno("unable to make temporary index file readable"); rev_tmp_name = write_rev_file(NULL, written_list, nr_written, hash, @@ -497,9 +504,6 @@ void finish_tmp_packfile(struct strbuf *name_buffer, rename_tmp_packfile(name_buffer, pack_tmp_name, "pack"); if (rev_tmp_name) rename_tmp_packfile(name_buffer, rev_tmp_name, "rev"); - rename_tmp_packfile(name_buffer, idx_tmp_name, "idx"); - - free((void *)idx_tmp_name); } void write_promisor_file(const char *promisor_name, struct ref **sought, int nr_sought) -- cgit v1.2.3