| Message ID | f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Problems with ra/rebase-i-more-options - should we revert it? | expand |
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. Doh I forgot to add --committer-date-is-author-date to the rebase command line in that test. It passes with that added - how embarrassing. However it does appear that it prefixes the date in GIT_COMMITTER_DATE with @@ rather than @. I think (though am not completely certain yet) the reason the test still passes is that the date has more than 8 digits so although match_object_header_date() fails because of the '@@' match_digit() succeeds once the loop in parse_date_basic() strips that prefix. Jonathan's test date only has 7 digits so match_digit() does not treat it as a number of seconds since the start of the epoch and fails to parse it. The fix for the @@ is quite simple, the date we read from the author script already has an @ so we don't need to add another. The diff below shows a basic fix but we should get rid of datebuf altogether as we don't need it. I need a break now I'll try and put a patch together later in the week if no one else has by then. Best Wishes Phillip --- >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); res = setenv("GIT_COMMITTER_DATE", opts->ignore_date ? "" : datebuf.buf, 1); > The > test coverage of this series has always been pretty poor and I > think it needs improving for us to have confidence in it. I'm > also concerned that at least one of the > tests ('--committer-date-is-author-date works with rebase -r') > does not detect failures properly in the code below > > while read HASH > do > git show $HASH --pretty="format:%ai" >authortime > git show $HASH --pretty="format:%ci" >committertime > test_cmp authortime committertime > done <rev_list > > > Best Wishes > > Phillip > > [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/ > > --- >8 --- > diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh > index 5166f158dd..c81e1d7167 100755 > --- a/t/t3433-rebase-options-compatibility.sh > +++ b/t/t3433-rebase-options-compatibility.sh > @@ -6,6 +6,7 @@ > test_description='tests to ensure compatibility between am and interactive backends' > > . ./test-lib.sh > +. "$TEST_DIRECTORY"/lib-rebase.sh > > GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30" > export GIT_AUTHOR_DATE > @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' ' > done <rev_list > ' > > +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' ' > + git checkout commit2 && > + ( > + set_fake_editor && > + FAKE_LINES=2 && > + export FAKE_LINES && > + test_must_fail git rebase -i HEAD^^ > + ) && > + echo resolved > foo && > + git add foo && > + git rebase --continue && > + git log -1 --format=%at commit2 >expect && > + git log -1 --format=%ct HEAD >actual && > + test_cmp expect actual > +' > + > # Checking for +0000 in author time is enough since default > # timezone is UTC, but the timezone used while committing > # sets to +0530. >
Hi Phillip, 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.) > Doh I forgot to add --committer-date-is-author-date to the rebase > command line in that test. It passes with that added - how > embarrassing. However it does appear that it prefixes the date in > GIT_COMMITTER_DATE with @@ rather than @. I think (though am not > completely certain yet) the reason the test still passes is that > the date has more than 8 digits so although > match_object_header_date() fails because of the '@@' > match_digit() succeeds once the loop in parse_date_basic() strips > that prefix. Jonathan's test date only has 7 digits so > match_digit() does not treat it as a number of seconds since the > start of the epoch and fails to parse it. The fix for the @@ is > quite simple, the date we read from the author script already has > an @ so we don't need to add another. The diff below shows a > basic fix but we should get rid of datebuf altogether as we don't > need it. I need a break now I'll try and put a patch together > later in the week if no one else has by then. Thank you so much! > > Best Wishes > > Phillip > > --- >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 == '@')` Thanks again, Dscho > res = setenv("GIT_COMMITTER_DATE", > opts->ignore_date ? "" : datebuf.buf, 1); > > > The > > test coverage of this series has always been pretty poor and I > > think it needs improving for us to have confidence in it. I'm > > also concerned that at least one of the > > tests ('--committer-date-is-author-date works with rebase -r') > > does not detect failures properly in the code below > > > > while read HASH > > do > > git show $HASH --pretty="format:%ai" >authortime > > git show $HASH --pretty="format:%ci" >committertime > > test_cmp authortime committertime > > done <rev_list > > > > > > Best Wishes > > > > Phillip > > > > [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/ > > > > --- >8 --- > > diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh > > index 5166f158dd..c81e1d7167 100755 > > --- a/t/t3433-rebase-options-compatibility.sh > > +++ b/t/t3433-rebase-options-compatibility.sh > > @@ -6,6 +6,7 @@ > > test_description='tests to ensure compatibility between am and interactive backends' > > > > . ./test-lib.sh > > +. "$TEST_DIRECTORY"/lib-rebase.sh > > > > GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30" > > export GIT_AUTHOR_DATE > > @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' ' > > done <rev_list > > ' > > > > +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' ' > > + git checkout commit2 && > > + ( > > + set_fake_editor && > > + FAKE_LINES=2 && > > + export FAKE_LINES && > > + test_must_fail git rebase -i HEAD^^ > > + ) && > > + echo resolved > foo && > > + git add foo && > > + git rebase --continue && > > + git log -1 --format=%at commit2 >expect && > > + git log -1 --format=%ct HEAD >actual && > > + test_cmp expect actual > > +' > > + > > # Checking for +0000 in author time is enough since default > > # timezone is UTC, but the timezone used while committing > > # sets to +0530. > > >
Phillip Wood <phillip.wood123@gmail.com> writes: > 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. Let's do that. >> 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. > > Doh I forgot to add --committer-date-is-author-date to the rebase > command line in that test. It passes with that added - how > embarrassing. However it does appear that it prefixes the date in > GIT_COMMITTER_DATE with @@ rather than @. > > start of the epoch and fails to parse it. The fix for the @@ is > quite simple, the date we read from the author script already has > an @ so we don't need to add another. Yes, that sounds like a minimum and straightforward fix. In any case, the tip of 'master' (hence the one that would become the final) is simpler to remedy by just reverting the merge, but there are a handful of in-flight topics that may have been queued by forking 'master' after the problematic merge was made (iow, anything after the fifth batch for this cycle), which I'd have to be a bit careful when I merge them down, lest they attempt to pull in the bad topic again. But that will be something we need to worry about after the release, not before the final. Thanks. [Footnote] *1* The list of still-in-flight topics that may be contaminated with the merge of ra/rebase-i-more-options into 'master' are: am/test-pathspec-f-f-error-cases am/update-pathspec-f-f-tests bc/hash-independent-tests-part-7 dl/merge-autostash ds/graph-horizontal-edges en/rebase-backend es/bugreport es/pathspec-f-f-grep hi/gpg-mintrustlevel hw/advice-add-nothing jn/promote-proto2-to-default jn/test-lint-one-shot-export-to-shell-function kw/fsmonitor-watchman-racefix sg/completion-worktree yz/p4-py3 I probably may requeue them by rebasing on top of 2.25 once the release is done.
Junio C Hamano <gitster@pobox.com> writes: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> 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. > > Let's do that. > ... >>> J > > In any case, the tip of 'master' (hence the one that would become > the final) is simpler to remedy by just reverting the merge, but > there are a handful of in-flight topics that may have been queued by > forking 'master' after the problematic merge was made (iow, anything > after the fifth batch for this cycle), which I'd have to be a bit > careful when I merge them down, lest they attempt to pull in the bad > topic again. But that will be something we need to worry about > after the release, not before the final. I will push out what I wish to be able to tag as the final [*1*] shortly but without actually tagging, so that it can get a bit wider exposure than just the usual "Gitster tested locally and then did let Travis try them" testing. Thanks. [Reference] *1* The tip of 'master' as of this writing is v2.25.0-rc2-24-gb4615e40a8
Junio C Hamano <gitster@pobox.com> writes: > I will push out what I wish to be able to tag as the final [*1*] > shortly but without actually tagging, so that it can get a bit wider > exposure than just the usual "Gitster tested locally and then did > let Travis try them" testing. I haven't heard from any failure report so (taking no news as good news) I'll cut the final today based on what is already on the public repositories everywhere.
Junio C Hamano <gitster@pobox.com> writes: > Junio C Hamano <gitster@pobox.com> writes: > >> I will push out what I wish to be able to tag as the final [*1*] >> shortly but without actually tagging, so that it can get a bit wider >> exposure than just the usual "Gitster tested locally and then did >> let Travis try them" testing. > > I haven't heard from any failure report so (taking no news as good > news) I'll cut the final today based on what is already on the > public repositories everywhere. By the way, as one of the methods to double check that my result of reverting the merge made sense, I ran "git rebase -ri v2.24.0 pu" and excised the merge and the problematic topic out of the todo list. With the rerere database populated beforehand, it was more or less a painless exercise (except for one topic, en/rebase-backend, which is one of the topics that was queued forking 'master' after the topic got merged *and* actually depended on what the topic did) and after about 1700+ steps (which did not take more than 20 minutes, including the time spent for the manual rebasing of en/rebase-backend topic) I got the same tree for 'pu' I pushed out last night. One thing I noticed that "rebase -ri" could be taught to handle better was that the side branches that were merged to the final result did not get relabeled. Those merges that appear on the first parent chain leading to 'pu' call themselves as "Merge branch 'blah'" and many of them (i.e. the ones that forked before the merge of the topic getting excised from the mainline) did just merge the tip of the named branch without touching the commits on the side branch, but some branches did have to be rebased, but their tips did not get updated (only the tentative rewritten/<topic> labels were pointing at the updated tip during the rebase, which are of course discarded after we are done). But other than that, it was quite nice. It is less transparent (at least to me) and probably less efficient than the current workflow to rebuild 'pu' for a few times every day ("less efficient" is primarily because the established workflow is quite optimized to the way I work), so it is not likely for me to switch to "rebase -ri" any time soon. But it makes me feel safe to know that there is another tool I can use to double-check the result of everyday workflow. Thanks.
Hi Junio, On Mon, 13 Jan 2020, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > > > Junio C Hamano <gitster@pobox.com> writes: > > > >> I will push out what I wish to be able to tag as the final [*1*] > >> shortly but without actually tagging, so that it can get a bit wider > >> exposure than just the usual "Gitster tested locally and then did let > >> Travis try them" testing. > > > > I haven't heard from any failure report so (taking no news as good > > news) I'll cut the final today based on what is already on the public > > repositories everywhere. > > By the way, as one of the methods to double check that my result of > reverting the merge made sense, I ran "git rebase -ri v2.24.0 pu" > and excised the merge and the problematic topic out of the todo > list. With the rerere database populated beforehand, it was more or > less a painless exercise (except for one topic, en/rebase-backend, > which is one of the topics that was queued forking 'master' after > the topic got merged *and* actually depended on what the topic did) > and after about 1700+ steps (which did not take more than 20 > minutes, including the time spent for the manual rebasing of > en/rebase-backend topic) I got the same tree for 'pu' I pushed out > last night. Nice! > One thing I noticed that "rebase -ri" could be taught to handle > better was that the side branches that were merged to the final > result did not get relabeled. Those merges that appear on the first > parent chain leading to 'pu' call themselves as "Merge branch 'blah'" > and many of them (i.e. the ones that forked before the merge of the > topic getting excised from the mainline) did just merge the tip of > the named branch without touching the commits on the side branch, > but some branches did have to be rebased, but their tips did not get > updated (only the tentative rewritten/<topic> labels were pointing > at the updated tip during the rebase, which are of course discarded > after we are done). This has been discussed on the list before this past September, but I think the discussion has stalled after v2 was sent, most likely due to my suggestions asking for more, I hate to admit: https://lore.kernel.org/git/20190907234413.1591-1-wh109@yahoo.com/ > But other than that, it was quite nice. > > It is less transparent (at least to me) and probably less efficient > than the current workflow to rebuild 'pu' for a few times every day > ("less efficient" is primarily because the established workflow is > quite optimized to the way I work), so it is not likely for me to > switch to "rebase -ri" any time soon. But it makes me feel safe to > know that there is another tool I can use to double-check the result > of everyday workflow. I understand. There is definitely a non-negligible cost involved whenever switching from one flow that works to another that might not yet work as well. I had the same hiccups when switching the Git garden shears over to `--rebase-merges` (it was worth it because the result is so much faster on Windows, of course). Having said that, if you ever find yourself wanting Just One Feature in `--rebase-merges` that would make it worthwhile for you to think about switching your patch-based workflow to a `rebase -ir`-based one, please let me know, and I will try my best to accommodate. Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> ... after about 1700+ steps (which did not take more than 20 >> minutes, including the time spent for the manual rebasing of >> en/rebase-backend topic) I got the same tree for 'pu' I pushed out >> last night. > > Nice! Nice indeed---I forgot to say more-or-less there, though ;-) It is quite an achievement to make it practical to rebuild the maint..pu chain, which would involve 1000+ commits, while allowing to edit only a fraction of them. [ellided] observation that tips of the branches that were rewritten in order to rebase the named tip that contains them were left stale > This has been discussed on the list before this past September, but I > think the discussion has stalled after v2 was sent,... That's OK. One step at a time ;-) > Having said that, if you ever find yourself wanting Just One Feature in > `--rebase-merges` that would make it worthwhile for you to think about > switching your patch-based workflow to a `rebase -ir`-based one, please > let me know, and I will try my best to accommodate. Another thing I noticed was that we may want to attempt to recreate an evil merge and then stop to ask confirmation. The "rebase -ri" I did to sanity-check my revert for example failed to bring in the change made in the existing evil merge when trying to recreate the merge of the dl/merge-autostash topic into master..pu chain and silently created a fails-to-build-from-the-source tree instead.
On 15/01/2020 19:14, Junio C Hamano wrote: > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > > Having said that, if you ever find yourself wanting Just One Feature > > in `--rebase-merges` that would make it worthwhile for you to think > > about switching your patch-based workflow to a `rebase -ir`-based > > one, please let me know, and I will try my best to accommodate. > > Another thing I noticed was that we may want to attempt to recreate > an evil merge and then stop to ask confirmation. The "rebase -ri" I > did to sanity-check my revert for example failed to bring in the > change made in the existing evil merge when trying to recreate the > merge of the dl/merge-autostash topic into master..pu chain and > silently created a fails-to-build-from-the-source tree instead. FYI (and anyone interested), it`s something we actually brought up some two years ago, at the time of introducing `--rebase-merges` (known as `--recreate-merges` back at the time), see[1]. It ended being a lengthy and heated discussion (inside a few different topics as well, like original RFC[2] and it`s v2 update[3]), myself being guilty for dropping out eventually and not following it through, though, life taking me in another direction at the moment... but I still find this functionality to be very useful, not to say essential, even, for reliable complex merge _rebasing_ (meaning keeping "evil merge" changes, too), and not just merge _recreating_ (loosing "evil merge" changes, and worse - doing it silently, as you experienced yourself now as well). p.s. Bringing that one up again, it can`t go without saying a huge thanks to Dscho for taking it this far in the meantime anyway <3 Regards, Buga [1]: https://lore.kernel.org/git/bc9f82fb-fd18-ee45-36a4-921a1381b32e@gmail.com/ [2]: https://lore.kernel.org/git/87y3jtqdyg.fsf@javad.com/ [3]: https://lore.kernel.org/git/87r2oxe3o1.fsf@javad.com/
Junio C Hamano <gitster@pobox.com> writes: >> Having said that, if you ever find yourself wanting Just One Feature in >> `--rebase-merges` that would make it worthwhile for you to think about >> switching your patch-based workflow to a `rebase -ir`-based one, please >> let me know, and I will try my best to accommodate. I missed a mention of 'patch' here when I prepared my earlier reply. I do not expect "rebase -ri" to play any role in the part of the workflow that accepts patches from the mailing list. The involvement of "rebase -ri" (vs Meta/Reintegrate) is purely what happens after a topic is queued and starts to get tested with other topics on integration branches, and no "patch based workflow" plays any role there---it does not make any sense to base that part on anything but merge (and possibly cherry-picking an evil merge from an earlier round).
Igor Djordjevic <igor.d.djordjevic@gmail.com> writes: > On 15/01/2020 19:14, Junio C Hamano wrote: >> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> > >> > Having said that, if you ever find yourself wanting Just One Feature >> > in `--rebase-merges` that would make it worthwhile for you to think >> > about switching your patch-based workflow to a `rebase -ir`-based >> > one, please let me know, and I will try my best to accommodate. >> >> Another thing I noticed was that we may want to attempt to recreate >> an evil merge and then stop to ask confirmation. The "rebase -ri" I >> did to sanity-check my revert for example failed to bring in the >> change made in the existing evil merge when trying to recreate the >> merge of the dl/merge-autostash topic into master..pu chain and >> silently created a fails-to-build-from-the-source tree instead. > > FYI (and anyone interested), it`s something we actually brought up > some two years ago, at the time of introducing `--rebase-merges` > (known as `--recreate-merges` back at the time), see[1]. > > It ended being a lengthy and heated discussion (inside a few > different topics as well, like original RFC[2] and it`s v2 update[3]), > myself being guilty for dropping out eventually and not following it > through, though, life taking me in another direction at the moment... For reference, there is a nice summary in "Git Rev News Edition 38": https://git.github.io/rev_news/2018/04/18/edition-38 > but I still find this functionality to be very useful, not to say > essential, even, for reliable complex merge _rebasing_ (meaning > keeping "evil merge" changes, too), and not just merge _recreating_ > (loosing "evil merge" changes, and worse - doing it silently, as you > experienced yourself now as well). Yeah, dropping user content silently and by default is still the most weird thing for git to do, be it a merge or not a merge. As an additional note, I came to conclusion that there is actually no such thing as "evil merge" that is somehow different from "evil commit" in general (a commit containing unrelated changes). Then, as "evil commit" belongs to user domain, we need to finally realize that "evil merge" belongs entirely to user domain as well, and thus, as it's out of git domain, we should stop using the term "evil merge" to excuse any kinds of weird git behaviors. Regards, Sergey
Hi Dscho On 12/01/2020 18:41, Johannes Schindelin wrote: > Hi Phillip, > > 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. > [...] >> --- >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. Best Wishes Phillip > Thanks again, > Dscho > >> res = setenv("GIT_COMMITTER_DATE", >> opts->ignore_date ? "" : datebuf.buf, 1); >> >>> The >>> test coverage of this series has always been pretty poor and I >>> think it needs improving for us to have confidence in it. I'm >>> also concerned that at least one of the >>> tests ('--committer-date-is-author-date works with rebase -r') >>> does not detect failures properly in the code below >>> >>> while read HASH >>> do >>> git show $HASH --pretty="format:%ai" >authortime >>> git show $HASH --pretty="format:%ci" >committertime >>> test_cmp authortime committertime >>> done <rev_list >>> >>> >>> Best Wishes >>> >>> Phillip >>> >>> [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/ >>> >>> --- >8 --- >>> diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh >>> index 5166f158dd..c81e1d7167 100755 >>> --- a/t/t3433-rebase-options-compatibility.sh >>> +++ b/t/t3433-rebase-options-compatibility.sh >>> @@ -6,6 +6,7 @@ >>> test_description='tests to ensure compatibility between am and interactive backends' >>> >>> . ./test-lib.sh >>> +. "$TEST_DIRECTORY"/lib-rebase.sh >>> >>> GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30" >>> export GIT_AUTHOR_DATE >>> @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' ' >>> done <rev_list >>> ' >>> >>> +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' ' >>> + git checkout commit2 && >>> + ( >>> + set_fake_editor && >>> + FAKE_LINES=2 && >>> + export FAKE_LINES && >>> + test_must_fail git rebase -i HEAD^^ >>> + ) && >>> + echo resolved > foo && >>> + git add foo && >>> + git rebase --continue && >>> + git log -1 --format=%at commit2 >expect && >>> + git log -1 --format=%ct HEAD >actual && >>> + test_cmp expect actual >>> +' >>> + >>> # Checking for +0000 in author time is enough since default >>> # timezone is UTC, but the timezone used while committing >>> # sets to +0530. >>> >>
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
--- a/t/t3433-rebase-options-compatibility.sh +++ b/t/t3433-rebase-options-compatibility.sh @@ -6,6 +6,7 @@ test_description='tests to ensure compatibility between am and interactive backends' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-rebase.sh GIT_AUTHOR_DATE="1999-04-02T08:03:20+05:30" export GIT_AUTHOR_DATE @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' ' done <rev_list ' +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' ' + git checkout commit2 && + ( + set_fake_editor && + FAKE_LINES=2 && + export FAKE_LINES && + test_must_fail git rebase -i HEAD^^ + ) && + echo resolved > foo && + git add foo && + git rebase --continue && + git log -1 --format=%at commit2 >expect && + git log -1 --format=%ct HEAD >actual && + test_cmp expect actual +' + # Checking for +0000 in author time is enough since default # timezone is UTC, but the timezone used while committing # sets to +0530.
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. The test coverage of this series has always been pretty poor and I think it needs improving for us to have confidence in it. I'm also concerned that at least one of the tests ('--committer-date-is-author-date works with rebase -r') does not detect failures properly in the code below while read HASH do git show $HASH --pretty="format:%ai" >authortime git show $HASH --pretty="format:%ci" >committertime test_cmp authortime committertime done <rev_list Best Wishes Phillip [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/ --- >8 --- diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh index 5166f158dd..c81e1d7167 100755