Re: [PATCH v2 09/18] maintenance: add loose-objects task
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Jul 29, 2020, 22:21 UTC
- Message-ID
- <20200729222126.GC2965447@google.com>
- In-Reply-To
- <83648f48655ba68126110018d81c1d2e2bcc7a6f.1595527000.git.gitgitgadget@gmail.com>
On Thu, Jul 23, 2020 at 05:56:31PM +0000, Derrick Stolee via GitGitGadget wrote:
Show 7 quoted lines
> +loose-objects:: > + The `loose-objects` job cleans up loose objects and places them into > + pack-files. In order to prevent race conditions with concurrent Git > + commands, it follows a two-step process. First, it deletes any loose > + objects that already exist in a pack-file; concurrent Git processes > + will examine the pack-file for the object data instead of the loose > + object. Second, it creates a new pack-file (starting with "loose-")
[jonathan tan + jonathan nieder] If you are going to document this, probably it should also be tested, so the documentation does not become stale. Or, just don't document it.
Show 15 quoted lines
> +static int pack_loose(void)
> +{
> + struct repository *r = the_repository;
> + int result = 0;
> + struct write_loose_object_data data;
> + struct strbuf prefix = STRBUF_INIT;
> + struct child_process *pack_proc;
> +
> + /*
> + * Do not start pack-objects process
> + * if there are no loose objects.
> + */
> + if (!for_each_loose_file_in_objdir(r->objects->odb->path,
> + loose_object_exists,
> + NULL, NULL, NULL))[emily] To me, this is unintuitive - but upon inspection, it's exiting the foreach early if any loose object is found, so this is cheaper than actually counting. Maybe a comment would help to understand? Or we could name the function differently, like "bail_if_loose()" or something?
Show 7 quoted lines
> +test_expect_success 'loose-objects task' ' > + # Repack everything so we know the state of the object dir > + git repack -adk && > + > + # Hack to stop maintenance from running during "git commit" > + echo in use >.git/objects/maintenance.lock && > + test_commit create-loose-object &&
[jonathan nieder] Does it make sense to use a different git command which is guaranteed to make a loose object? Is 'git commit' futureproof, if we decide commits should directly create packs in the future? 'git unpack-objects' is guaranteed to make a loose object, although it is clumsy because it needs a packfile to begin with...
[jonathan tan] But, using 'git commit' is easier to understand in this context. Maybe commenting to say that we assume 'git commit' makes 1 or more loose objects will be enough to futureproof - then we have a signal to whoever made a change to make this fail, and that person knows how to fix this test.