| Message ID | patch-3.3-78976fcb7b2-20210907T193600Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rename *.idx file into place last (also after *.bitmap) | expand |
On Tue, Sep 07, 2021 at 09:42:38PM +0200, Ævar Arnfjörð Bjarmason wrote: > + /* > + * We must write the *.idx last, so that anything that expects > + * an accompanying *.rev, *.bitmap etc. can count on it being > + * present. > + * > + * See also corresponding logic in the "exts" > + * struct in builtin/repack.c > + */ TBH, I'm not sure the cycle between comments helps much, and it would probably suffice to say: /* * write the .idx last, so everything else can count on its existence */ In both places. But I don't think that it makes much of a difference, really, so either outcome is fine with me. Thanks, Taylor
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c index 9fa4c6a9be8..fad3fe5651d 100644 --- a/builtin/pack-objects.c +++ b/builtin/pack-objects.c @@ -1249,7 +1249,6 @@ static void write_pack_file(void) 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); if (write_bitmap_index) { struct strbuf sb = STRBUF_INIT; @@ -1269,6 +1268,16 @@ static void write_pack_file(void) strbuf_release(&sb); } + /* + * We must write the *.idx last, so that anything that expects + * an accompanying *.rev, *.bitmap etc. can count on it being + * present. + * + * See also corresponding logic in the "exts" + * struct in builtin/repack.c + */ + rename_tmp_packfile_idx(&tmp_basename, hash, &idx_tmp_name); + free(idx_tmp_name); strbuf_release(&tmp_basename); free(pack_tmp_name); diff --git a/builtin/repack.c b/builtin/repack.c index c3e47716093..dd438ebb3ee 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -211,6 +211,14 @@ static struct { {".rev", 1}, {".bitmap", 1}, {".promisor", 1}, + /* + * We must write the *.idx last, so that anything that expects + * an accompanying *.rev, *.bitmap etc. can count on it being + * present. + * + * See also corresponding logic in write_pack_file() in + * builtin/pack-objects.c + */ {".idx"}, };
Follow-up a preceding commit (pack-write.c: rename `.idx` file into place last, 2021-08-16)[1] and rename the *.idx file in-place after we write the *.bitmap. The preceding commit fixed the issue of *.idx being written before *.rev files, but did not do so for *.idx files. See 7cc8f971085 (pack-objects: implement bitmap writing, 2013-12-21) for commentary at the time when *.bitmap was implemented about how those files are written out, nothing in that commit contradicts what's being done here. While we're at it let's add cross-commentary to both builtin/repack.c and builtin/pack-objects.c to point out the two places where we write out these sets of files in sequence. 1. https://lore.kernel.org/git/a6a4d2154e83d41c10986c5f455279ab249a910c.1630461918.git.me@ttaylorr.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/pack-objects.c | 11 ++++++++++- builtin/repack.c | 8 ++++++++ 2 files changed, 18 insertions(+), 1 deletion(-)