Re: [PATCH 6/7] show, log: provide a --remerge-diff capability
- From
Elijah Newren <newren@gmail.com>
- Date
- Sep 29, 2021, 04:00 UTC
- Message-ID
- <CABPp-BGwunfsThNEPAKfXMUZKpHQTCdRU7V8=3J88XThN6=2Kg@mail.gmail.com>
- In-Reply-To
- <87mtnxxgz3.fsf@evledraar.gmail.com>
On Tue, Sep 28, 2021 at 1:05 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 42 quoted lines
>
> On Tue, Aug 31 2021, Elijah Newren via GitGitGadget wrote:
>
> > static int decoration_given;
> > static int use_mailmap_config = 1;
> > +static struct tmp_objdir *tmp_objdir;
> > static const char *fmt_patch_subject_prefix = "PATCH";
> > static int fmt_patch_name_max = FORMAT_PATCH_NAME_MAX_DEFAULT;
> > static const char *fmt_pretty;
>
> So here we make this static file-level etc...
>
> > @@ -407,6 +410,17 @@ static int cmd_log_walk(struct rev_info *rev)
> > int saved_nrl = 0;
> > int saved_dcctc = 0;
> >
> > + if (rev->remerge_diff) {
> > + tmp_objdir = tmp_objdir_create();
> > + if (!tmp_objdir)
> > + die(_("unable to create temporary object directory"));
> > + tmp_objdir_make_primary(the_repository, tmp_objdir);
> > +
> > + strbuf_init(&rev->remerge_objdir_location, 0);
> > + strbuf_addstr(&rev->remerge_objdir_location,
> > + tmp_objdir_path(tmp_objdir));
> > + }
> > +
> > if (rev->early_output)
> > setup_early_output();
> >
> > @@ -449,6 +463,13 @@ static int cmd_log_walk(struct rev_info *rev)
> > rev->diffopt.no_free = 0;
> > diff_free(&rev->diffopt);
> >
> > + if (rev->remerge_diff) {
> > + strbuf_release(&rev->remerge_objdir_location);
> > + tmp_objdir_remove_as_primary(the_repository, tmp_objdir);
> > + tmp_objdir_destroy(tmp_objdir);
> > + tmp_objdir = NULL;
>
> ...but all of the "tmp_objdir" usage is in one function, can't the
> variable be declared here instead?That's a very good point.
> We need to hand the "remerge_objdir_location" off to the "rev_info" > struct, but that seems separate from its lifetime.
Given Peff's suggestion elsewhere, though, to destroy the tmp_objdir after each merge and create a new one, I wonder if I should actually be passing a tmp_objdir** to rev_info (allowing log-tree to do the work of destroying and creating a new one after each merge, instead of using the "remerge_objdir_location" to run a recursive delete of files). That'd still work with your idea to remove the statically scoped variable, though.
> Re my [1] & [2] I like Neeraj's "atexit cleanup" approach better, > perhaps that makes your cleanup in log-tree.c redundant or easier?
Having an atexit cleanup as a safety measure seems fine. However, I don't like avoiding the manual cleanup step and relying on atexit cleanup; I'd go so far as to say I think that'd be a bug, at least for my usage. It presumes one-shot usage, whereas I'd rather move git to being more library-like.
However, fully destroying the tmp_objdir probably makes the cleanup in log-tree.c easier.
Show 6 quoted lines
> Per [2] it looks like you need to "hand off" the > "remerge_objdir_location", so having the struct live in tmp-objdir.h as > I suggested in [2] might make that work... > > 1. https://lore.kernel.org/git/87v92lxhh4.fsf@evledraar.gmail.com/ > 2. https://lore.kernel.org/git/87r1d9xh71.fsf@evledraar.gmail.com/