Re: [PATCH v4 02/10] log: clean unneeded objects during `log --remerge-diff`
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-01 09:45:36
On Fri, Jan 21 2022, Elijah Newren via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
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.
+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?