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

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

On Tue, Feb 01 2022, Elijah Newren wrote:

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

Thanks :)

>>
>> > +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.

Sorry to add to the frustration. I really didn't mean that as a
suggestion for a thing to be addressed, I think this is way past good
enough. It was just something I found curious, didn't quite know how it
worked, and thought I'd ask if you knew offhand. Thanks!