Re: Problems with ra/rebase-i-more-options - should we revert it?

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

Hi Phillip,

On Fri, 17 Jan 2020, Phillip Wood wrote:

> On 12/01/2020 18:41, Johannes Schindelin wrote:
> >
> > On Sun, 12 Jan 2020, Phillip Wood wrote:
> >
> > > On 12/01/2020 16:12, Phillip Wood wrote:
> > > > I'm concerned that there are some bugs in this series and think it
> > > > may be best to revert it before releasing 2.25.0. Jonathan Nieder
> > > > posted a bug report on Friday [1] which I think is caused by this
> > > > series. While trying to reproduce Jonathan's bug I came up with
> > > > the test below which fails, but not in the same way.
> >
> > Thank you so much for your thoughts and your work on this. For what
> > it's worth, I totally agree with your assessment and your suggestion
> > to revert those patches _before_ releasing v2.25.0. (I seem to
> > remember vaguely that there were repeated requests for better test
> > coverage and that those requests went unaddressed, so I would not be
> > surprised if there were more unfortunate surprises waiting for us.)
>
> Yes there were more surprises - when we fork `git merge`
> --committer-date-is-author-date is broken. That was tested but with a
> commit where the author date was the current time so it did not detect
> the failure.

Thanks for confirming.

> > [...]
> > > --- >8 ---
> > > diff --git a/sequencer.c b/sequencer.c
> > > index 763ccbbc45..22a38de47b 100644
> > > --- a/sequencer.c
> > > +++ b/sequencer.c
> > > @@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,
> > >                  if (!date)
> > >                          return -1;
> > >
> > > -               strbuf_addf(&datebuf, "@%s", date);
> > > +               strbuf_addf(&datebuf, "%s", date);
> >
> > I have to admit that I have not analyzed the code before this hunk (it
> > would be much easier to increase the context in a non-static reviewing
> > environment, e.g. on GitHub, but the mailing list does not allow for
> > that), so I do not know just _how_ likely our `date` here is going to
> > change or remain prefixed by a `@`. Therefore, this suggestion might be
> > totally stupid: `"@%s", date + (*date == '@')`
>
> The date was read from the author-script so I think we should leave it as is
> in case the user has edited it and is using a different date format. Having
> said that I'm keen to make a bigger change to Rohit's implementation and just
> get the author date out of the argv_array holding the child's environment as
> this avoids re-reading the author-script file. It has taken a bit longer than
> I planned so it'll be next week before I post the fixes.

I look forward to it!
Dscho