On Thu, Jul 23, 2020 at 05:56:31PM +0000, Derrick Stolee via GitGitGadget wrote:
> +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.
> +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?
> +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.
