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

[Elijah Newren <newren@gmail.com>]

Hi Dscho,

On Mon, Jan 21, 2019 at 8:07 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi,
>
> 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?


Elijah