Re: [PATCH v4 4/8] am, rebase--merge: do not overlook --skip'ed commits with post-rewrite

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Elijah,

On Mon, 21 Jan 2019, Elijah Newren wrote:

> On Mon, Jan 21, 2019 at 8:07 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 11 Dec 2018, Elijah Newren wrote:
> >
> > > The post-rewrite hook is supposed to be invoked for each rewritten
> > > commit.  The fact that a commit was selected and processed by the rebase
> > > operation (even though when we hit an error a user said it had no more
> > > useful changes), suggests we should write an entry for it.  In
> > > particular, let's treat it as an empty commit trivially squashed into
> > > its parent.
> > >
> > > This brings the rebase--am and rebase--merge backends in sync with the
> > > behavior of the interactive rebase backend.
> > >
> > > Signed-off-by: Elijah Newren <newren@gmail.com>
> >
> > This makes sense. I think, though, that we need to be more careful...
> >
> > > ---
> > >  builtin/am.c                 | 9 +++++++++
> > >  git-rebase--merge.sh         | 2 ++
> > >  t/t5407-post-rewrite-hook.sh | 3 +++
> > >  3 files changed, 14 insertions(+)
> > >
> > > diff --git a/builtin/am.c b/builtin/am.c
> > > index 8f27f3375b..af9d034838 100644
> > > --- a/builtin/am.c
> > > +++ b/builtin/am.c
> > > @@ -2000,6 +2000,15 @@ static void am_skip(struct am_state *state)
> > >       if (clean_index(&head, &head))
> > >               die(_("failed to clean index"));
> > >
> > > +     if (state->rebasing) {
> > > +             FILE *fp = xfopen(am_path(state, "rewritten"), "a");
> > > +
> > > +             assert(!is_null_oid(&state->orig_commit));
> > > +             fprintf(fp, "%s ", oid_to_hex(&state->orig_commit));
> >
> > ... here. What if `fp == NULL`? (Users do all kinds of interesting
> > things...)
> 
> This if-block is actually copy-pasted from the end of the do_commit()
> function, since the same logic was needed in both places.  The fact
> that a `fp == NULL` case never triggered for do_commit() suggests that
> the check has never been needed in the wild (or perhaps it just
> indicates a latent bug that no one has triggered yet).  However, it
> does suggest a code cleanup regardless.  I thought of it as such a
> small block that I didn't think to put it in a separate function, but
> perhaps I should so that someone spotting the possibility of a NULL fp
> could fix it for both callers in a single place.
> 
> Should I insert a preliminary change pulling this block out of
> do_commit into a separate function, and then modify this patch to just
> call this function?  Or perhaps given the length of time it has
> already been cooking (and Junio's rerere resolution of our two series
> that I don't want to mess up), just do it as a simple post-series
> fixup?

Speaking for myself: I am fine with either way.

Ciao,
Dscho