On Tue, Feb 1, 2022 at 1:45 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> 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.
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?!?");
>
> > +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.
