Hi Elijah,
On Mon, 21 Jan 2019, Elijah Newren wrote:
> On Mon, Jan 21, 2019 at 8:07 AM Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> 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@xxxxxxxxx>
> >
> > 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
