Re: [PATCH v2 09/18] maintenance: add loose-objects task

On 7/29/2020 6:21 PM, Emily Shaffer wrote:

On Thu, Jul 23, 2020 at 05:56:31PM +0000, Derrick Stolee via GitGitGadget wrote:

quoted

+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.

Adding a condition to the test is a good idea.

quoted

+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?

Sure. "bail_on_loose()" makes more sense to me.

quoted

+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...

"unpack-objects" also has the problem that it won't write the loose
object if it exists as a packed object, so it requires moving the
pack-file out of the object directory first. (That's also required
for testing that the loose objects get packed by the maintenance
task, but I'd rather not need to remove the multi-pack-index, too.)

[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.

I'll add the comment for now. Thanks.

-Stolee