On Fri, Jan 21 2022, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@xxxxxxxxx>
> [...]
> @@ -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?
