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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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

> From: Elijah Newren <newren@gmail.com>
> [...]
> @@ -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?