From: Jeff King <firstname.lastname@example.org> To: Junio C Hamano <email@example.com> Cc: Taylor Blau <firstname.lastname@example.org>, email@example.com Subject: Re: [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way Date: Mon, 16 Nov 2020 19:02:52 -0500 [thread overview] Message-ID: <20201117000252.GB12322@coredump.intra.peff.net> (raw) In-Reply-To: <firstname.lastname@example.org> On Mon, Nov 16, 2020 at 03:29:05PM -0800, Junio C Hamano wrote: > > t7700.14 ensures > > that 'git repack' will, for example, remove existing bitmaps even if the > > pack written is verbatim the same (when repacking without '-b' in a > > repository unchanged from the time 'git repack -b' was run). So, we have > > to handle metadata that we didn't write, by unlinking any existing > > metadata that our invocation of pack-objects didn't generate itself. > > Hmph, t7700.14 wants it that way because? > > If we were told to generate a packfile, and we ended up regenerating > the exactly the same one, it appears to me that we can just pretend > nothing happened and leave things as they were? Puzzled... We definitely could, and the outcome would be correct in the sense that the bitmap would still work. I'm not sure that the test in t7700 cares particularly about this "making the same pack" case. It only wants to know that if we disable bitmaps and repack, we end up without a bitmap. And normally, if you added in even a single extra object, you'd get a new pack and that would happen. But of course the test just repacks back-to-back, so it does trigger the "making the same pack" case. I think you could make an argument either way: - we have an existing bitmap for free, and bitmaps make things faster, so why not keep it? - the user did not ask for bitmaps, so we should make the outcome consistent whether a pack of the exact name existed before or not The second one seems less surprising to me. But I think if we did the first, the code would be shorter (it would not need any of this "keep track of what pack-objects generated" stuff at all, but would just copy whatever files we see into place). > > if (rename(fname_old, fname)) > > die_errno(_("renaming '%s' failed"), fname_old); > > OK, so this is where the previous step matters. We do the same as > before (i.e. stat+chmod and rename) only for paths we have created. > > We don't reuse the old one because we have already written a new > file so we won't save anything by doing so, and checking if the > target of rename exists already before deciding not to rename cannot > be made atomic, so just relying on rename() to do the right thing is > a good idea anyway. Even though the pack is byte-for-byte identical, the new .idx, etc, might not be. And those could be affected by options. E.g.: git -c pack.writeBitmapHashCache=false repack -adb git -c pack.writeBitmapHashCache=true repack -adb should probably end up with a bitmap file that contains a hash cache. -Peff
next prev parent reply other threads:[~2020-11-17 0:04 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-16 18:41 [PATCH 0/3] repack: " Taylor Blau 2020-11-16 18:41 ` [PATCH 1/3] repack: make "exts" array available outside cmd_repack() Taylor Blau 2020-11-16 18:41 ` [PATCH 2/3] builtin/repack.c: keep track of what pack-objects wrote Taylor Blau 2020-11-16 18:41 ` [PATCH 3/3] builtin/repack.c: don't move existing packs out of the way Taylor Blau 2020-11-16 23:29 ` Junio C Hamano 2020-11-17 0:02 ` Jeff King [this message] 2020-11-17 0:26 ` Taylor Blau 2020-11-17 0:25 ` Taylor Blau 2020-11-17 0:46 ` Junio C Hamano 2020-11-17 20:15 ` Taylor Blau 2020-11-17 21:28 ` Junio C Hamano
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20201117000252.GB12322@coredump.intra.peff.net \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --subject='Re: [PATCH 3/3] builtin/repack.c: don'\''t move existing packs out of the way' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).