Re: [PATCH v4 02/10] log: clean unneeded objects during `log --remerge-diff`

On Tue, Feb 01 2022, Elijah Newren wrote:

On Tue, Feb 1, 2022 at 1:45 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Fri, Jan 21 2022, Elijah Newren via GitGitGadget wrote:

quoted

From: Elijah Newren <redacted>
[...]

@@ -944,7 +945,12 @@ static int do_remerge_diff(struct rev_info *opt,
      strbuf_release(&parent1_desc);
      strbuf_release(&parent2_desc);
      merge_finalize(&o, &res);
-     /* TODO: clean up the temporary object directory */
+
+     /* Clean up the contents of the temporary object directory */
+     if (opt->remerge_objdir)
+             tmp_objdir_discard_objects(opt->remerge_objdir);
+     else
+             BUG("unable to remove temporary object directory");

Re the die in 1/10 I don't think this will ever trigger the way this bug
suggests.

If we didn't manage to remove the directory that'll be signalled with
the return code of tmp_objdir_discard_objects() which you're adding
here, but which doesn't have a meaningful return value.

So shouldn't it first of all be returning the "int" like the
remove_dir_recursively() user in tmp_objdir_destroy_1() makes use of?

What this bug is really about is:

    BUG("our juggling of opt->remerge_objdir between here and builtin/log.c is screwy")

Or something, because if we failed to remove the director(ies) we'll
just ignore that here.

Yeah, I think I'm suffering from leftover bits from earlier versions
since this patch series has been waiting for 17 months now.  I
switched it to

    BUG("did a remerge diff without remerge_objdir?!?");

Thanks :)

quoted

quoted

+void tmp_objdir_discard_objects(struct tmp_objdir *t)
+{
+     remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
+}

I skimmed remove_dir_recurse() a bit, but didn't test this, does this
remove just the "de/eadbeef..." in "de/eadbeef..." or also "de/",
i.e. do we (and do we want) to keep the fanned-out 256 loose top-level
directories throughout the operation?

It will remove everything below t->path, but leave t->path.  As such,
it'll nuke any of the 256 loose top-level directories that exist.

If someone wants to come along later and measure performance and
determine if leaving those 256 loose top-level directories around
improves things, I think that's fine, but I'm not going to look at it
as part of this series.  I'm more curious about where tmp_objdir
creates the temporary directory; when the intent is to migrate the
objects into the main directory, it should probably be created on the
same filesystem.  When the intent is scratch space, like it is for
--remerge-diff, the tmp_objdir should probably be shoved in /dev/shm
or something like that.  But again, that's outside of this series.
This series already has had a long list of things keeping it from the
light of day; there's no need to add frills to it as part of the
initial submission.

Sorry to add to the frustration. I really didn't mean that as a
suggestion for a thing to be addressed, I think this is way past good
enough. It was just something I found curious, didn't quite know how it
worked, and thought I'd ask if you knew offhand. Thanks!