Re: [PATCH 8/9] show, log: provide a --remerge-diff capability

From: Elijah Newren <newren@gmail.com>
Date: Dec 21, 2021, 22:18 UTC
Message-ID: <CABPp-BFuZbLKLf8VnNxLAuF0NaSg=2Nut++YxhHsCxr3n1jwJg@mail.gmail.com>
In-Reply-To: <211221.864k71r6kz.gmgdl@evledraar.gmail.com>

On Tue, Dec 21, 2021 at 1:28 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 16 quoted lines

>
>
> On Tue, Dec 21 2021, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@gmail.com>
>
> > +     if (rev->remerge_diff) {
> > +             rev->remerge_objdir = tmp_objdir_create("remerge-diff");
> > +             if (!rev->remerge_objdir)
> > +                     die(_("unable to create temporary object directory"));
>
> It looks like the tmp_objdir_create() API is rather bad about mixing
> errors that would come with an errno with others, but shouldn't this be
> die_errno() in the case where it would fail due to a syscall? Even
> better would be passing a "gentle" to it and have it emit the
> appropriate errors.

I can switch to die_errno().

Show 5 quoted lines

>
> > +     if (rev.remerge_diff)
> > +             die(_("--remerge_diff does not make sense"));
>
> s/_/-/

Indeed, thanks.

Show 5 quoted lines

> > +     struct merge_options o;
> > +     struct commit_list *bases;
> > +     struct merge_result res;
>
> nit: could use "= { 0 }" instead of memset below.

Sure, I can make that change.

Show 7 quoted lines

> > +     /* Re-merge the parents */
> > +     merge_incore_recursive(&o,
> > +                            bases, parents->item, parents->next->item,
> > +                            &res);
>
> style: odd not to have arguments that fit on the line on
> the line, i.e. "&o, bases, ...".

Yes, but this groups all the ancestors so nicely as opposed to the typical happenstance of whatever fits on a line. ;-)

> > +     /* Clean up the temporary object directory */
> > +     if (opt->remerge_objdir != NULL)
>
> style: if (!x) not if (x != NULL)

Ok, will change.