repack: don't lose objects to a ".keep" that appears mid-run - #2219
repack: don't lose objects to a ".keep" that appears mid-run#2219qeesung wants to merge 6 commits into
Conversation
receive-pack runs index-pack with "--keep" over the quarantine, which writes a "pack-XXX.keep" there. The path we register as a tempfile is a different one: where that ".keep" will land once the quarantine is migrated into the main object database. Nothing of ours is at that path yet, and something else may be. Two pushes of identical content produce identical thin packs, index-pack names a pack after its contents, and so both want the same ".keep" in the main object database. If the other push still holds it, that file is what keeps its pack from being repacked away, and we remove it at exit regardless -- even when pre-receive rejected our push and nothing was migrated at all. Register the path right before the migration instead, and once the migration has returned, read the files back. index-pack wrote the message we handed it; a file that says something else was not written for us, so let go of it without removing it. tempfile gains unregister_tempfile() for that. Registering only after the migration would leave a window: the ".keep" is the first thing migrated, and for a push that duplicates a large pack the migration then spends a while comparing the two packfiles. A signal in between would leave our ".keep" behind, with our message in it, and every later push of the same content would fail to migrate over it. Registering first keeps that window closed, as it is today. Reading the files back also covers a migration that fails partway through with our ".keep" already in place: we go by what is there, not by whether the migration succeeded, and still remove it. Signed-off-by: Qin ShiCheng <qeesung@live.com>
"--stdin-packs=follow" distinguishes excluded packs that are closed
under reachability ("^") from those that are not ("!"). The traversal
stops at objects in the former, and goes on through the latter to
rescue whatever they depend on that would otherwise be left out.
A pack named with "--keep-pack" gets the same in-core flag as a "^"
pack, so the traversal stops at it too. Nothing warrants that: the
caller said not to repack it, not that it is self-contained. When it
holds a commit but not that commit's tree, the tree is never rescued,
and writing a bitmap over the result fails for lack of closure.
In follow mode, mark such a pack as kept-open instead, the way repack
already lists the packs it cannot vouch for as "!" on stdin. Its
objects stay out of the result, and the traversal can go through it.
This matters more once repack names its ".keep" packs this way instead
of passing "--honor-pack-keep": on-disk kept packs never were a
boundary, and they should not become one.
Signed-off-by: Qin ShiCheng <qeesung@live.com>
When writing a cruft pack with an expiration, pack-objects first collects the recent objects and then walks from them to rescue whatever they reach, expired or not. A pack the caller did not list is marked kept while collecting, so that its objects are not copied into the cruft pack, and unmarked before the walk, so that the walk can go through it. The walk does not see the unmarking. Whether an object sits in a kept pack is answered from a cache that is built on first use and only dropped when asked about a different kind of kept pack. Collecting builds it while the unlisted pack is still marked, the walk asks the same kind of question, and so the unlisted pack stays in it: the walk stops there, and whatever lies beyond it in an expired pack is lost. This went unnoticed because of "--honor-pack-keep". repack passes it, and when there is a ".keep" file it makes the collecting side ask about on-disk and in-core kept packs together while the walk asks about in-core ones alone; the cache is rebuilt each time the question changes, and by accident the walk sees the current marks. Take the ".keep" file away and the objects are lost today. A later commit stops repack from passing "--honor-pack-keep" at all, so fix this first. Expose the invalidation packfile.c already has and call it after re-marking. The test builds an unreachable chain whose middle commit sits in a pack pack-objects is not told about and whose oldest objects have expired; without the fix the cruft pack holds only the recent tip. Signed-off-by: Qin ShiCheng <qeesung@live.com>
add_extra_kept_packs() scans the whole "--keep-pack" list once per pack in the repository. That is fine for the handful of names it gets today, but the next commit lets a caller name every kept pack in the repository, and with thousands of them the scan dominates: matching 20,000 kept packs against 20,000 names takes 11 seconds here, against under a second with "--honor-pack-keep". Sort the list once and look each pack up in it. The comparison stays fspathcmp(), so what matches does not change. Signed-off-by: Qin ShiCheng <qeesung@live.com>
"--keep-pack" names one pack per occurrence, and there is only so much room on the command line: ARG_MAX is shared with the environment, and on Windows the whole line is capped at 32,767 characters, which a few hundred pack names fill. Past that the spawn fails before pack-objects has started. fetch-pack grew "--stdin" in 078b895 (fetch-pack: new --stdin option to read refs from stdin, 2012-04-02) for the same reason. stdin is taken here: every mode repack drives pack-objects in already uses it, for the revision list under "-a", object names for the promisor pack, and pack lists for "--stdin-packs" and "--cruft". So read the names from a file instead, one per line, skipping empty lines. They go into the same list as the "--keep-pack" names and are treated exactly alike: matched against local packs, ignored when they match nothing, and kept open under "--stdin-packs=follow". A relative path is resolved against the directory the user ran from, as "--refs-snapshot" of "git multi-pack-index write" is. The list now holds strings from two sources, so let it own its copies. repack is about to use this to hand pack-objects its own snapshot of the packs that have a ".keep" file. Signed-off-by: Qin ShiCheng <qeesung@live.com>
repack works out which packs are redundant by looking for ".keep" files when it starts, then passes "--honor-pack-keep" to the pack-objects it spawns, which looks for them all over again. Two scans of the same directory, seconds apart, with nothing holding them together. A ".keep" that turns up in between loses objects. The parent did not see it, so the pack is on its list to delete. The child does see it, so it leaves that pack's objects out of the replacement. The parent deletes the pack regardless: repack_remove_redundant_pack() passes force_delete, which skips the ".keep" check in unlink_pack_path(). The objects are gone and repack exits successfully. The gap is easy to land in. index-pack writes its ".keep" before it renames the packfile into place, so a "git fetch" or a push being migrated out of its quarantine will do it. Checking for the ".keep" once more right before deleting would not help: a push holds it for a fraction of a second, and it may well be gone again by the time pack-objects has finished. Hand pack-objects the kept packs we collected at startup and drop "--honor-pack-keep". Both processes then work from one snapshot, and a ".keep" appearing or disappearing while we run cannot make them disagree. An earlier commit made sure a pack kept this way is no more of a boundary to the traversal than a ".keep" file was. The list goes into a file next to the refs snapshot we already write for "git multi-pack-index write", and is passed with "--keep-pack-from-file" to every pack-objects we spawn when "--pack-kept-objects" is not in effect, which is when "--honor-pack-keep" used to be. The cruft pack-objects already has the kept packs on its stdin; the file is redundant there, but it sees the same list as everybody else. With nothing to keep, no file is written and nothing is passed, which is what "--honor-pack-keep" came down to when it found no ".keep". The names go one per line, so a name with a newline in it cannot be passed. "--stdin-packs" and "--cruft" have the same limit and die on a name they cannot find, but "--keep-pack" ignores such a name, and the two halves of a garbled one could go on to exclude some other pack; refuse it up front instead. The user's own "--keep-pack" arguments keep being forwarded, since they apply either way. write_filtered_pack() had a loop passing the kept packs too, but without the ".pack" suffix pack-objects compares against; it goes. Kept packs borrowed from an alternate object directory were covered by "--honor-pack-keep" and are not by the snapshot, which only ever held local packs; repack never deletes those, so their objects now get packed rather than skipped, which costs room but cannot lose anything. Signed-off-by: Qin ShiCheng <qeesung@live.com>
Welcome to GitGitGadgetHi @qeesung, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
1 participant
A concurrent push can make "git repack -d" delete a pack whose objects
were never copied anywhere, and exit 0. We hit this in production: a
ref pointing at a commit that no longer exists, on git 2.43, and it
reproduces on master.
What happens:
then spawns pack-objects with --honor-pack-keep, which scans again;
migrating its quarantine. Its pack is a duplicate and is dropped,
but its ".keep" is linked into place, onto the old pack;
repack deletes the pack by its earlier list, with force_delete.
Two things are wrong, and each is fixed on its own:
it linked onto somebody else's pack, or a foreign one when its own
push was rejected before any migration. Only remove a ".keep" that
carries our own message.
pack-objects the snapshot repack took at startup instead.
Patches 2-5 are what 6/6 needs to be safe:
traversal like a "^" pack; on-disk ".keep" packs never did.
--honor-pack-keep happened to mask. Pre-existing, reproducible today.
packs than fit on a command line (32K characters on Windows).
Every fix comes with a test that fails without it; the race itself is
reproduced in t7703 by having a ".keep" appear as pack-objects starts.
The full suite passes, and the series merges cleanly into next and
seen.
CC: Patrick Steinhardt ps@pks.im, Taylor Blau ttaylorr@openai.com, Junio C Hamano gitster@pobox.com, Justin Tobler jltobler@gmail.com