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
