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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 1, 2022, 09:35 UTC
Message-ID: <220201.86v8xyapj9.gmgdl@evledraar.gmail.com>
In-Reply-To: <f06de6c1b2fbd5c5a23b6755197a3683c7d18d2f.1642792341.git.gitgitgadget@gmail.com>

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

Show 13 quoted lines

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