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

Hi Phillip,

On Fri, 17 Jan 2020, Phillip Wood wrote:

On 12/01/2020 18:41, Johannes Schindelin wrote:

quoted

On Sun, 12 Jan 2020, Phillip Wood wrote:

quoted

On 12/01/2020 16:12, Phillip Wood wrote:

quoted

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.

quoted

[...]

quoted

--- >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