Repository navigation
Conversation
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, |
hediet
added a commit
to hediet/git-repack-keep-race-repro
that referenced
this pull request
Sep 10, 2026
Unpatched master b8242b0 loses the parent commit, tree and blob under the injection; patched a18e354 keeps every object across all 27 cases on the same runner with the same harness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/allow |
|
User qeesung is now allowed to use GitGitGadget. |
Author
|
/submit |
|
Submitted as pull.2219.git.1789385483.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| #include "pack.h" | ||
| #include "packfile.h" | ||
| #include "path.h" | ||
| #include "read-cache-ll.h" |
There was a problem hiding this comment.
Justin Tobler wrote on the Git mailing list (how to reply to this email):
On 26/09/14 11:31AM, Qin ShiCheng via GitGitGadget wrote:
>From: Qin ShiCheng <qeesung@live.com>
>
>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.
Yup, when the ".keep" file gets registered as a tempfile, it needs to
know where it will eventually be located post-migration. That way it can
be deleted after the references have been updated or if the process
exits early. This is a bit awkward, but its a result of use relying on
git-index-pack(1) to create the ".keep" file for us and it gets written
to the quaratine directory.
>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.
Interesting, for a pair of identical concurrent pushes, if one exits
early it could end of deleting the other processes packfile out from
under it. Really the process should probably only delete a ".keep" file
that itself created.
Something worth noting, if there are two concurrent identical pushes,
both will generate the same ".keep", but the keep message contained will
differ. In such cases, when the quarantined files are migrated to the
ODB, the ".keep" file that gets migrated first "wins" and the other push
will fail because the competing ".keep" fails the collision check and
consequently the push fails. I mention this because the current behavior
for how Git handles concurrent identical pushes is to reject one of
them. So if a process encounters an already existing ".keep" file in the
main ODB, it may be sufficient to abort early anyways.
>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.
Right, registering the temporary ".keep" files doesn't really need to
happen prior to the ODB transaction commit anyways. In fact, we could go
a step further and stop using git-index-pack(1) to prematurely create
".keep" files altogether in favor of letting the commit phase of the ODB
transaction create it explicitly. This has a couple of benefits:
- It avoids the already awkward tracking of ".keep" files in ODB
transaction pre-commit.
- It would also make fixing the issue in question a bit easier
by allowing us to simply try to create the ".keep" file and if
it already exists, unregister the tempfile and abort early.
Completely unrelated to this bug as part of another series I'm working
on locally, I've already have some patches that start creating ".keep"
files explicitly during the ODB commit phase in the "files" backend. I
would be happy to pick these patches out and send them upstream with
some small adjustments to also fix the issue here in your first patch.
Just let me know what you would perfer. :)
Thanks,
-JustinThere was a problem hiding this comment.
Qin ShiCheng wrote on the Git mailing list (how to reply to this email):
On 26/09/15 01:25PM, Justin Tobler wrote:
> Something worth noting, if there are two concurrent identical pushes,
> both will generate the same ".keep", but the keep message contained will
> differ. In such cases, when the quarantined files are migrated to the
> ODB, the ".keep" file that gets migrated first "wins" and the other push
> will fail because the competing ".keep" fails the collision check and
> consequently the push fails. I mention this because the current behavior
> for how Git handles concurrent identical pushes is to reject one of
> them. So if a process encounters an already existing ".keep" file in the
> main ODB, it may be sufficient to abort early anyways.
Agreed, aborting is fine there. The two pushes that bit us were seconds
apart rather than concurrent: the first had already finished and removed
its ".keep", so the second found nothing at that path and linked its own
".keep" onto the first push's pack. What the patch is after is narrower
than handling the collision: whatever happens on the way out, we should
only remove a ".keep" we created ourselves. Today finalize unlinks the
path unconditionally, even when pre-receive rejected the push and
nothing was migrated at all, which is what the first t5547 test pins.
> Right, registering the temporary ".keep" files doesn't really need to
> happen prior to the ODB transaction commit anyways. In fact, we could go
> a step further and stop using git-index-pack(1) to prematurely create
> ".keep" files altogether in favor of letting the commit phase of the ODB
> transaction create it explicitly.
[...]
> Completely unrelated to this bug as part of another series I'm working
> on locally, I've already have some patches that start creating ".keep"
> files explicitly during the ODB commit phase in the "files" backend. I
> would be happy to pick these patches out and send them upstream with
> some small adjustments to also fix the issue here in your first patch.
> Just let me know what you would perfer. :)
That is the better shape for it, please do. Creating the file at commit
time and registering it right there gives the two guarantees this patch
gets by reading the files back: nothing foreign is removed, and a signal
after our ".keep" is in place still cleans it up. (v1 registers before
the migration for the latter: the ".keep" is migrated first, and for a
duplicate of a large pack the migration then spends a while in
check_collision().) One case worth keeping in mind is a migration that
fails partway with our ".keep" already installed, e.g. the ".idx"
colliding because pack.indexVersion changed between the two pushes; the
second t5547 test covers that.
I will drop 1/6 from v2 so the series is only the repack side; 2/6-6/6
do not depend on it. Feel free to take the two t5547 tests if they are
of use to your series.
Thanks,
Qin
qeesung
force-pushed
the
repack-kept-packs-snapshot
branch
from
September 17, 2026 06:38
a18e354 to
4684fd8
Compare
Author
|
/submit |
|
Submitted as pull.2219.v2.git.1789700615.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
| stop_progress(&progress_state); | ||
| } | ||
|
|
||
| static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs) |
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Qin ShiCheng via GitGitGadget" <gitgitgadget@gmail.com> writes:
> @@ -4301,10 +4302,17 @@ static void enumerate_and_traverse_cruft_objects(struct string_list *fresh_packs
> /*
> * Re-mark only the fresh packs as kept so that objects in
> * unknown packs do not halt the reachability traversal early.
> + * The kept-pack cache was built while those packs were still
> + * marked, so drop it too.
> */
> repo_for_each_pack(the_repository, p)
> p->pack_keep_in_core = 0;
> mark_pack_kept_in_core(fresh_packs, 1);
> + for (source = the_repository->objects->sources; source;
> + source = source->next) {
> + struct odb_source_files *files = odb_source_files_downcast(source);
> + packfile_store_invalidate_kept_pack_cache(files->packed);
> + }
This question is primarily meant for folks who are pushing different
ODB backends, but I am not sure this is safe in the long term.
When downcasting finds that 'source' is not from the files backend,
we immediately hit BUG(). Is checking the type of 'source' first
and calling packfile_store_invalidate_kept_pack_cache() only when
it is from the files backend a sensible workaround? That sounds
like a blatant layering violation.
One of the recent design decisions, unrelated to this, was to make
the concept of "alternate object store" an implementation detail of
the files backend, if I recall correctly. Do we need a similar
rearchitecting of the code here, pushing details like packfile
management down to the files backend layer, before we can properly
fix this?
Of course, until an ODB backend other than files materializes, all
of the above is merely academic and the proposed change might be
sufficient. However, relying on an unchecked downcast feels like
laying mines for our future selves.There was a problem hiding this comment.
Qin ShiCheng wrote on the Git mailing list (how to reply to this email):
Junio C Hamano <gitster@pobox.com> writes:
> When downcasting finds that 'source' is not from the files backend,
> we immediately hit BUG(). Is checking the type of 'source' first
> and calling packfile_store_invalidate_kept_pack_cache() only when
> it is from the files backend a sensible workaround? That sounds
> like a blatant layering violation.
Agreed, and I would rather not have pack-objects look at the type of
a source at all.
The assumption is already made two lines above the new loop, though:
repo_for_each_pack() downcasts every source in the same way, and so
does has_object_kept_pack(), which is what reads this cache in the
first place. So the loop is not wrong so much as in the wrong place.
It belongs next to its reader in packfile.c, not in the builtin.
For v3 I have this instead:
void repo_invalidate_kept_pack_caches(struct repository *r)
{
struct odb_source *source;
for (source = r->objects->sources; source; source = source->next) {
struct odb_source_files *files = odb_source_files_downcast(source);
invalidate_kept_pack_cache(files->packed);
}
}
with the per-store function made static again, and the caller in
pack-objects reduced to
mark_pack_kept_in_core(fresh_packs, 1);
repo_invalidate_kept_pack_caches(the_repository);
This does not make the code work with another backend -- nothing
around it would either -- but pack-objects no longer gains a new
dependency on the files backend, and the downcast sits with the
others that will have to move together.
> Do we need a similar
> rearchitecting of the code here, pushing details like packfile
> management down to the files backend layer, before we can properly
> fix this?
I hope not. Without this patch, a cruft repack with an expiration
drops objects that are only reachable through a pack pack-objects was
not told about; the new test in t5329 shows it happening today. When
packfile management does move down to the files backend, this
function should go along with has_object_kept_pack(), and nothing in
the fix depends on where they end up. Patrick may well know better
how that is meant to look.
Thanks,
QinThere was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
Qin ShiCheng <qeesung@live.com> writes:
> This does not make the code work with another backend -- nothing
> around it would either -- but pack-objects no longer gains a new
> dependency on the files backend, and the downcast sits with the
> others that will have to move together.
OK.
>> Do we need a similar
>> rearchitecting of the code here, pushing details like packfile
>> management down to the files backend layer, before we can properly
>> fix this?
>
> I hope not. Without this patch, a cruft repack with an expiration
> drops objects ...
Ah, I think you misunderstood.
By fix "this" I meant fixing "the layering violation" and not what
your topic originally wanted to achieve. And as we agreed above,
these downcasts that sit together with existing ones need to move in
order to avoid layering violation, which is what I meant by
"rearchitecting". Until that happens, layering violation is left
unfixed, but addressing the kept pack cache issue with layering
violation can be better than not addressing the issue at all.
In any case, my original question to experts
>> This question is primarily meant for folks who are pushing different
>> ODB backends, but I am not sure this is safe in the long term.
still stands. I think we between two of us agreed the answer is "no
it is not safe in the long term", but others may have ideas to solve
it more cleanly, hopefully.
Thanks.
qeesung
force-pushed
the
repack-kept-packs-snapshot
branch
from
October 8, 2026 07:33
4684fd8 to
8f3b9d9
Compare
Author
|
/submit |
|
Submitted as pull.2219.v3.git.1791453141.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "qeesung via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Interaction with topics in seen:
>
> * ps/odb-files-alternates turns the list of object sources into a single
> files source with a list of object directories, so
> repo_invalidate_kept_pack_caches() from 2/5 needs to walk those instead.
> The textual merge is clean, but the build breaks; this resolution follows
> has_object_kept_pack() on that topic:
>
> void repo_invalidate_kept_pack_caches(struct repository *r) { struct
> odb_source_files *files = odb_source_files_downcast(r->objects->source);
> for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
> invalidate_kept_pack_cache(dir->packed); }
I do not know if you meant to cram a function on four lines this
way, but I suspect that it may be easier for everybody to stop and
wait until the other topic solidifies a bit more, and then create a
synthetic base that merges the other topic into the tip of 'master'
and rebase these five patches on top of the resulting merge.
I wonder how close ps/odb-files-alternates topic is to the finish
line? Karthik did read through the initial round and then gave a
thumbs up on the current iteration. We would benefit from a
different set of eyes on the series [*].
Thanks.
[Reference]
* <CAOLa=ZTjrzNbuvZ-kr6k5TZSMyGGMFTb4iar6DZwdnCDvUrH9Q@mail.gmail.com> |
|
Qin ShiCheng wrote on the Git mailing list (how to reply to this email): Junio C Hamano <gitster@pobox.com> writes:
> I do not know if you meant to cram a function on four lines this
> way,
I did not; GitGitGadget reflowed the indented block in the PR
description. It was meant to read
void repo_invalidate_kept_pack_caches(struct repository *r)
{
struct odb_source_files *files = odb_source_files_downcast(r->objects->source);
for (struct odb_files_dir *dir = files->dirs; dir; dir = dir->next)
invalidate_kept_pack_cache(dir->packed);
}
> but I suspect that it may be easier for everybody to stop and
> wait until the other topic solidifies a bit more, and then create a
> synthetic base that merges the other topic into the tip of 'master'
> and rebase these five patches on top of the resulting merge.
Makes sense. I will hold off until ps/odb-files-alternates settles,
then send v4 on top of master with that topic merged in, and say so
in the cover letter.
Thanks,
Qin |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Qin ShiCheng <qeesung@live.com> writes:
> Makes sense. I will hold off until ps/odb-files-alternates settles,
> then send v4 on top of master with that topic merged in, and say so
> in the cover letter.
Thanks. In the meantime you can participate in reviewing Patrick's
topic, of course, and that may help it move forward a bit faster ;-) |
|
This branch is now known as |
|
This patch series was integrated into seen via git@b37b6f9. |
|
There was a status update in the "New Topics" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.com> |
|
There was a status update in the "Cooking" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.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 left out of the cruft pack, to go when that pack is deleted. Whether that happens depends on whether the question changes along the way, which "--honor-pack-keep" does whenever there is a ".keep" file around. Drop the cache after unmarking. Only packfile.c is to touch it, so add repo_invalidate_kept_pack_caches() there, which drops it for every object directory. With the cache gone, the walk goes through every pack but the listed fresh ones, as was intended, including packs borrowed from an alternate. It has to: an object there may refer to a local one, for example when two repositories borrow from each other, or when "git repack --filter-to" has moved some objects to an alternate and left the ones they refer to behind. Without "--local", whatever the walk finds there is copied into the cruft pack, as it already is for loose objects of an alternate; with "--local", nothing is. Either way, the walk may now go through a history far bigger than the repository's own, as it already did whenever there was a ".keep" file around. It also includes packs given with "--keep-pack", whose objects are to be left out. Turn those into "open" kept packs, as "--stdin-packs=follow" does for its "!" packs: the walk goes through them, and their objects are still left out. This is how an unlisted pack with a ".keep" file is treated under "--honor-pack-keep". With "--keep-pack", the walk and the check that leaves the objects of such a pack out ask about different kinds of kept packs, so the cache is rebuilt each time they take turns, as already happens under "--honor-pack-keep" whenever there is a ".keep" file around. repack never gets here with "--keep-pack": it lists the packs it keeps as fresh ones instead. The tests walk from a recent commit to an expired one through a pack pack-objects is not told about, through one given with "--keep-pack", and through one of an alternate holding trees whose blobs "git repack --filter-to" left behind, with and without "--local". Signed-off-by: Qin ShiCheng <qeesung@live.com>
repack decides which packs to delete before it runs pack-objects, but pack-objects looks for ".keep" files on its own when it opens the packs. A ".keep" that appears in between, for example from a push or fetch whose index-pack wrote a pack identical to an existing one, makes pack-objects treat as kept a pack that repack is about to delete. Its objects are then left out of the new pack, or not loosened, or not kept as unreachable, and they are gone once repack deletes the pack. Not all of that goes through "--honor-pack-keep": loosening objects for "--unpack-unreachable", and collecting them for "--keep-unreachable" and "--cruft", skip packs with a ".keep" file regardless of it. Let the caller name such packs. "--ignore-pack-keep-from=<file>" reads names of local packs, one per line, given and matched as for "--keep-pack", and treats them as if they had no ".keep" file. All of the above go by p->pack_keep, which is read from the disk once, when a pack is first seen, so clearing it for the named packs before anything looks at it covers every one of them. Other packs, including any that show up later, keep whatever their ".keep" files say. The names come from a file because there can be more of them than fit on a command line (on Windows, the whole command line is limited to 32767 characters), and the standard input is already taken in every mode repack uses. Signed-off-by: Qin ShiCheng <qeesung@live.com>
repack works out which packs are redundant by looking for ".keep" files when it starts, then runs pack-objects, which looks for them all over again when it opens the packs. 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 to itself: its objects are not copied into the new pack, not loosened for "-A", not kept for "-k" or "--cruft". The parent deletes the pack regardless, as 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 names a pack after its contents and writes its ".keep" before it renames the packfile into place, so a "git fetch", or a push being migrated out of its quarantine, that brings in a pack identical to one we already have puts a ".keep" next to that pack. 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. When we may delete packs, that is with "-d" and either "-a", "-A", "--cruft" or "--geometric", write the names of the packs we may delete, the non-kept and cruft packs, to a file next to the refs snapshot we already write for "git multi-pack-index write", and pass it to every pack-objects we spawn with "--ignore-pack-keep-from". pack-objects then never treats those packs as kept, whatever appears on the disk. Every other pack, whether kept when we started or showing up later, still goes by its ".keep" file, so "--honor-pack-keep" stays. Otherwise nothing gets deleted and nothing is passed. The file has one name per line. A name with a newline in it gets split there, which at worst makes some other pack not count as kept: its objects may be copied, but none are lost. Unreachable objects of such a pack are still dropped by "-a -d", or by "--cruft-expiration" and "--unpack-unreachable" once they are old enough, as these drop unreachable objects by design, and a pack written again under the same name does not make them any younger; what changes is that the objects that are to be kept no longer go with them. The tests have a ".keep" appear on the pack about to be deleted as pack-objects starts, through a "git" wrapper found via "--exec-path", for "-a", "-A", "-k", "--cruft" with and without an expiration, "--pack-kept-objects", "--filter" and "--geometric". Signed-off-by: Qin ShiCheng <qeesung@live.com>
qeesung
force-pushed
the
repack-kept-packs-snapshot
branch
from
October 23, 2026 09:03
8f3b9d9 to
0018754
Compare
|
There was a status update in the "Cooking" section about the branch The behavior of repack and pack-objects has been updated to avoid losing objects to a '.keep' file that appears mid-run. Instead of pack-objects looking for '.keep' files independently, repack now hands it a snapshot of kept packs via a new --keep-pack-from-file option. Expecting a reroll. cf. <SJ0PR84MB29933F55E298B09B37F2778EDD922@SJ0PR84MB2993.NAMPRD84.PROD.OUTLOOK.COM> source: <pull.2219.v3.git.1791453141.gitgitgadget@gmail.com> |
Author
|
/submit |
|
Submitted as pull.2219.v4.git.1793005620.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
"git repack -d" could lose objects when a ".keep" file appeared on a
pack it had already decided to delete. repack now tells pack-objects
which packs it may delete, and pack-objects ignores ".keep" files on
those packs.
We hit this in production (a ref pointing at a missing commit, on git
2.43), and it reproduces on master:
pack-objects scans again when it opens the packs;
those, and a ".keep" ends up next to it;
or keep them for -A, -k and --cruft, and repack deletes the pack
anyway.
v3 passed pack-objects the kept packs repack saw at startup instead of
--honor-pack-keep. That missed -A, -k and --cruft, which skip packs with
a ".keep" file regardless of that option. v4 goes the other way:
master, but under --honor-pack-keep any ".keep" file happens to hide
it, including the one the race leaves; 3/3 takes that one away, so
this comes first.
the packs named in the file as having no ".keep".
Each fix has a test that fails without it; the race is reproduced in
t7700 and t7703. The full suite passes.
Known limitations:
and by the expiration options, as before. A pack written again under
the same name keeps its old mtime, so objects a push brings back that
way can still expire. Freshening the pack on such a collision would
help, but that is for a separate series.
must (see 1/3). On a "clone --shared" fork of git.git this took the
walk in "repack -d -l --cruft" from about 2ms to about 3.3s, which is
what master already does when a ".keep" file is around.
This is based on master, now that ps/odb-files-alternates is in. It
merges cleanly with next and seen (with v3 reverted), and the repack
tests pass there. jt/odb-keep-lockfile-fix, Justin's fix for the
receive-pack side that v1 had a patch for, leaves the race as it is.
Changes since v3:
nothing passes --keep-pack any more; 2/3 and 3/3 are new.
still leaving their objects out, and its message explains why the
walk goes through alternates. There are tests for both.
CC: Patrick Steinhardt ps@pks.im, Taylor Blau ttaylorr@openai.com, Junio C Hamano gitster@pobox.com, Justin Tobler jltobler@gmail.com