{"thread":{"id":"52620","subject":"Problems with ra/rebase-i-more-options - should we revert it?","startedAt":"2020-01-12T16:12:41Z","lastAt":"2020-01-20T11:15:18Z","messageCount":14,"participants":["Phillip Wood","Johannes Schindelin","Junio C Hamano","Igor Djordjevic","Sergey Organov"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"389632","messageId":"f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com","threadId":"52620","inReplyTo":null,"subject":"Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-01-12T16:12:35Z","receivedAt":"2020-01-12T16:12:41Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"I'm concerned that there are some bugs in this series and think\nit may be best to revert it before releasing 2.25.0. Jonathan\nNieder posted a bug report on Friday [1] which I think is caused\nby this series. While trying to reproduce Jonathan's bug I came\nup with the test below which fails, but not in the same way. The\ntest coverage of this series has always been pretty poor and I\nthink it needs improving for us to have confidence in it. I'm\nalso concerned that at least one of the\ntests ('--committer-date-is-author-date works with rebase -r')\ndoes not detect failures properly in the code below\n\n\twhile read HASH\n\tdo\n\t\tgit show $HASH --pretty=\"format:%ai\" >authortime\n\t\tgit show $HASH --pretty=\"format:%ci\" >committertime\n\t\ttest_cmp authortime committertime\n\tdone <rev_list\n\n\nBest Wishes\n\nPhillip\n\n[1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/\n\n--- >8 ---\ndiff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh\nindex 5166f158dd..c81e1d7167 100755\n--- a/t/t3433-rebase-options-compatibility.sh\n+++ b/t/t3433-rebase-options-compatibility.sh\n@@ -6,6 +6,7 @@\n test_description='tests to ensure compatibility between am and interactive backends'\n \n . ./test-lib.sh\n+. \"$TEST_DIRECTORY\"/lib-rebase.sh\n \n GIT_AUTHOR_DATE=\"1999-04-02T08:03:20+05:30\"\n export GIT_AUTHOR_DATE\n@@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '\n        done <rev_list\n '\n \n+test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '\n+       git checkout commit2 &&\n+       (\n+               set_fake_editor &&\n+               FAKE_LINES=2 &&\n+               export FAKE_LINES &&\n+               test_must_fail git rebase -i HEAD^^\n+       ) &&\n+       echo resolved > foo &&\n+       git add foo &&\n+       git rebase --continue &&\n+       git log -1 --format=%at commit2 >expect &&\n+       git log -1 --format=%ct HEAD >actual &&\n+       test_cmp expect actual\n+'\n+\n # Checking for +0000 in author time is enough since default\n # timezone is UTC, but the timezone used while committing\n # sets to +0530.\n"},{"id":"389633","messageId":"089637d7-b4b6-f6ba-cce1-29e22ce47521@gmail.com","threadId":"52620","inReplyTo":"f2fe7437-8a48-3315-4d3f-8d51fe4bb8f1@gmail.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-01-12T17:31:13Z","receivedAt":"2020-01-12T17:31:19Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/01/2020 16:12, Phillip Wood wrote:\n> I'm concerned that there are some bugs in this series and think\n> it may be best to revert it before releasing 2.25.0. Jonathan\n> Nieder posted a bug report on Friday [1] which I think is caused\n> by this series. While trying to reproduce Jonathan's bug I came\n> up with the test below which fails, but not in the same way.\n\nDoh I forgot to add --committer-date-is-author-date to the rebase\ncommand line in that test. It passes with that added - how\nembarrassing. However it does appear that it prefixes the date in\nGIT_COMMITTER_DATE with @@ rather than @. I think (though am not\ncompletely certain yet) the reason the test still passes is that\nthe date has more than 8 digits so although\nmatch_object_header_date() fails because of the '@@'\nmatch_digit() succeeds once the loop in parse_date_basic() strips\nthat prefix. Jonathan's test date only has 7 digits so\nmatch_digit() does not treat it as a number of seconds since the\nstart of the epoch and fails to parse it. The fix for the @@ is\nquite simple, the date we read from the author script already has\nan @ so we don't need to add another. The diff below shows a \nbasic fix but we should get rid of datebuf altogether as we don't \nneed it. I need a break now I'll try and put a patch together \nlater in the week if no one else has by then.\n\nBest Wishes\n\nPhillip\n\n--- >8 ---\ndiff --git a/sequencer.c b/sequencer.c\nindex 763ccbbc45..22a38de47b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,\n                if (!date)\n                        return -1;\n \n-               strbuf_addf(&datebuf, \"@%s\", date);\n+               strbuf_addf(&datebuf, \"%s\", date);\n                res = setenv(\"GIT_COMMITTER_DATE\",\n                             opts->ignore_date ? \"\" : datebuf.buf, 1);\n \n> The\n> test coverage of this series has always been pretty poor and I\n> think it needs improving for us to have confidence in it. I'm\n> also concerned that at least one of the\n> tests ('--committer-date-is-author-date works with rebase -r')\n> does not detect failures properly in the code below\n> \n> \twhile read HASH\n> \tdo\n> \t\tgit show $HASH --pretty=\"format:%ai\" >authortime\n> \t\tgit show $HASH --pretty=\"format:%ci\" >committertime\n> \t\ttest_cmp authortime committertime\n> \tdone <rev_list\n> \n> \n> Best Wishes\n> \n> Phillip\n> \n> [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/\n> \n> --- >8 ---\n> diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh\n> index 5166f158dd..c81e1d7167 100755\n> --- a/t/t3433-rebase-options-compatibility.sh\n> +++ b/t/t3433-rebase-options-compatibility.sh\n> @@ -6,6 +6,7 @@\n>   test_description='tests to ensure compatibility between am and interactive backends'\n>   \n>   . ./test-lib.sh\n> +. \"$TEST_DIRECTORY\"/lib-rebase.sh\n>   \n>   GIT_AUTHOR_DATE=\"1999-04-02T08:03:20+05:30\"\n>   export GIT_AUTHOR_DATE\n> @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '\n>          done <rev_list\n>   '\n>   \n> +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '\n> +       git checkout commit2 &&\n> +       (\n> +               set_fake_editor &&\n> +               FAKE_LINES=2 &&\n> +               export FAKE_LINES &&\n> +               test_must_fail git rebase -i HEAD^^\n> +       ) &&\n> +       echo resolved > foo &&\n> +       git add foo &&\n> +       git rebase --continue &&\n> +       git log -1 --format=%at commit2 >expect &&\n> +       git log -1 --format=%ct HEAD >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>   # Checking for +0000 in author time is enough since default\n>   # timezone is UTC, but the timezone used while committing\n>   # sets to +0530.\n> \n"},{"id":"389635","messageId":"nycvar.QRO.7.76.6.2001121936290.46@tvgsbejvaqbjf.bet","threadId":"52620","inReplyTo":"089637d7-b4b6-f6ba-cce1-29e22ce47521@gmail.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-12T18:41:33Z","receivedAt":"2020-01-12T18:41:46Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Sun, 12 Jan 2020, Phillip Wood wrote:\n\n> On 12/01/2020 16:12, Phillip Wood wrote:\n> > I'm concerned that there are some bugs in this series and think\n> > it may be best to revert it before releasing 2.25.0. Jonathan\n> > Nieder posted a bug report on Friday [1] which I think is caused\n> > by this series. While trying to reproduce Jonathan's bug I came\n> > up with the test below which fails, but not in the same way.\n\nThank you so much for your thoughts and your work on this. For what it's\nworth, I totally agree with your assessment and your suggestion to revert\nthose patches _before_ releasing v2.25.0. (I seem to remember vaguely that\nthere were repeated requests for better test coverage and that those\nrequests went unaddressed, so I would not be surprised if there were more\nunfortunate surprises waiting for us.)\n\n> Doh I forgot to add --committer-date-is-author-date to the rebase\n> command line in that test. It passes with that added - how\n> embarrassing. However it does appear that it prefixes the date in\n> GIT_COMMITTER_DATE with @@ rather than @. I think (though am not\n> completely certain yet) the reason the test still passes is that\n> the date has more than 8 digits so although\n> match_object_header_date() fails because of the '@@'\n> match_digit() succeeds once the loop in parse_date_basic() strips\n> that prefix. Jonathan's test date only has 7 digits so\n> match_digit() does not treat it as a number of seconds since the\n> start of the epoch and fails to parse it. The fix for the @@ is\n> quite simple, the date we read from the author script already has\n> an @ so we don't need to add another. The diff below shows a\n> basic fix but we should get rid of datebuf altogether as we don't\n> need it. I need a break now I'll try and put a patch together\n> later in the week if no one else has by then.\n\nThank you so much!\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> --- >8 ---\n> diff --git a/sequencer.c b/sequencer.c\n> index 763ccbbc45..22a38de47b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,\n>                 if (!date)\n>                         return -1;\n>\n> -               strbuf_addf(&datebuf, \"@%s\", date);\n> +               strbuf_addf(&datebuf, \"%s\", date);\n\nI have to admit that I have not analyzed the code before this hunk (it\nwould be much easier to increase the context in a non-static reviewing\nenvironment, e.g. on GitHub, but the mailing list does not allow for\nthat), so I do not know just _how_ likely our `date` here is going to\nchange or remain prefixed by a `@`. Therefore, this suggestion might be\ntotally stupid: `\"@%s\", date + (*date == '@')`\n\nThanks again,\nDscho\n\n>                 res = setenv(\"GIT_COMMITTER_DATE\",\n>                              opts->ignore_date ? \"\" : datebuf.buf, 1);\n>\n> > The\n> > test coverage of this series has always been pretty poor and I\n> > think it needs improving for us to have confidence in it. I'm\n> > also concerned that at least one of the\n> > tests ('--committer-date-is-author-date works with rebase -r')\n> > does not detect failures properly in the code below\n> >\n> > \twhile read HASH\n> > \tdo\n> > \t\tgit show $HASH --pretty=\"format:%ai\" >authortime\n> > \t\tgit show $HASH --pretty=\"format:%ci\" >committertime\n> > \t\ttest_cmp authortime committertime\n> > \tdone <rev_list\n> >\n> >\n> > Best Wishes\n> >\n> > Phillip\n> >\n> > [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/\n> >\n> > --- >8 ---\n> > diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh\n> > index 5166f158dd..c81e1d7167 100755\n> > --- a/t/t3433-rebase-options-compatibility.sh\n> > +++ b/t/t3433-rebase-options-compatibility.sh\n> > @@ -6,6 +6,7 @@\n> >   test_description='tests to ensure compatibility between am and interactive backends'\n> >\n> >   . ./test-lib.sh\n> > +. \"$TEST_DIRECTORY\"/lib-rebase.sh\n> >\n> >   GIT_AUTHOR_DATE=\"1999-04-02T08:03:20+05:30\"\n> >   export GIT_AUTHOR_DATE\n> > @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '\n> >          done <rev_list\n> >   '\n> >\n> > +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '\n> > +       git checkout commit2 &&\n> > +       (\n> > +               set_fake_editor &&\n> > +               FAKE_LINES=2 &&\n> > +               export FAKE_LINES &&\n> > +               test_must_fail git rebase -i HEAD^^\n> > +       ) &&\n> > +       echo resolved > foo &&\n> > +       git add foo &&\n> > +       git rebase --continue &&\n> > +       git log -1 --format=%at commit2 >expect &&\n> > +       git log -1 --format=%ct HEAD >actual &&\n> > +       test_cmp expect actual\n> > +'\n> > +\n> >   # Checking for +0000 in author time is enough since default\n> >   # timezone is UTC, but the timezone used while committing\n> >   # sets to +0530.\n> >\n>\n"},{"id":"389641","messageId":"xmqqeew4l6qf.fsf@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"089637d7-b4b6-f6ba-cce1-29e22ce47521@gmail.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-12T21:12:08Z","receivedAt":"2020-01-12T21:12:17Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 12/01/2020 16:12, Phillip Wood wrote:\n>> I'm concerned that there are some bugs in this series and think\n>> it may be best to revert it before releasing 2.25.0.\n\nLet's do that.\n\n>> Jonathan\n>> Nieder posted a bug report on Friday [1] which I think is caused\n>> by this series. While trying to reproduce Jonathan's bug I came\n>> up with the test below which fails, but not in the same way.\n>\n> Doh I forgot to add --committer-date-is-author-date to the rebase\n> command line in that test. It passes with that added - how\n> embarrassing. However it does appear that it prefixes the date in\n> GIT_COMMITTER_DATE with @@ rather than @.\n>\n> start of the epoch and fails to parse it. The fix for the @@ is\n> quite simple, the date we read from the author script already has\n> an @ so we don't need to add another.\n\nYes, that sounds like a minimum and straightforward fix.\n\nIn any case, the tip of 'master' (hence the one that would become\nthe final) is simpler to remedy by just reverting the merge, but\nthere are a handful of in-flight topics that may have been queued by\nforking 'master' after the problematic merge was made (iow, anything\nafter the fifth batch for this cycle), which I'd have to be a bit\ncareful when I merge them down, lest they attempt to pull in the bad\ntopic again.  But that will be something we need to worry about\nafter the release, not before the final.\n\nThanks.\n\n\n[Footnote]\n\n*1* The list of still-in-flight topics that may be contaminated with\n    the merge of ra/rebase-i-more-options into 'master' are:\n\n    am/test-pathspec-f-f-error-cases\n    am/update-pathspec-f-f-tests\n    bc/hash-independent-tests-part-7\n    dl/merge-autostash\n    ds/graph-horizontal-edges\n    en/rebase-backend\n    es/bugreport\n    es/pathspec-f-f-grep\n    hi/gpg-mintrustlevel\n    hw/advice-add-nothing\n    jn/promote-proto2-to-default\n    jn/test-lint-one-shot-export-to-shell-function\n    kw/fsmonitor-watchman-racefix\n    sg/completion-worktree\n    yz/p4-py3\n\nI probably may requeue them by rebasing on top of 2.25 once the\nrelease is done.\n"},{"id":"389646","messageId":"xmqq5zhgkwxx.fsf@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"xmqqeew4l6qf.fsf@gitster-ct.c.googlers.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-13T00:43:38Z","receivedAt":"2020-01-13T00:43:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n>\n>> On 12/01/2020 16:12, Phillip Wood wrote:\n>>> I'm concerned that there are some bugs in this series and think\n>>> it may be best to revert it before releasing 2.25.0.\n>\n> Let's do that.\n> ...\n>>> J\n>\n> In any case, the tip of 'master' (hence the one that would become\n> the final) is simpler to remedy by just reverting the merge, but\n> there are a handful of in-flight topics that may have been queued by\n> forking 'master' after the problematic merge was made (iow, anything\n> after the fifth batch for this cycle), which I'd have to be a bit\n> careful when I merge them down, lest they attempt to pull in the bad\n> topic again.  But that will be something we need to worry about\n> after the release, not before the final.\n\nI will push out what I wish to be able to tag as the final [*1*]\nshortly but without actually tagging, so that it can get a bit wider\nexposure than just the usual \"Gitster tested locally and then did\nlet Travis try them\" testing.\n\nThanks.\n\n\n[Reference]\n\n*1* The tip of 'master' as of this writing is v2.25.0-rc2-24-gb4615e40a8\n\n"},{"id":"389720","messageId":"xmqqy2ubjkeo.fsf@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"xmqq5zhgkwxx.fsf@gitster-ct.c.googlers.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-13T18:11:59Z","receivedAt":"2020-01-13T18:12:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> I will push out what I wish to be able to tag as the final [*1*]\n> shortly but without actually tagging, so that it can get a bit wider\n> exposure than just the usual \"Gitster tested locally and then did\n> let Travis try them\" testing.\n\nI haven't heard from any failure report so (taking no news as good\nnews) I'll cut the final today based on what is already on the\npublic repositories everywhere.\n\n"},{"id":"389728","messageId":"xmqqpnfnj9p3.fsf_-_@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"xmqqy2ubjkeo.fsf@gitster-ct.c.googlers.com","subject":"\"rebase -ri\" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-13T22:03:20Z","receivedAt":"2020-01-13T22:03:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> I will push out what I wish to be able to tag as the final [*1*]\n>> shortly but without actually tagging, so that it can get a bit wider\n>> exposure than just the usual \"Gitster tested locally and then did\n>> let Travis try them\" testing.\n>\n> I haven't heard from any failure report so (taking no news as good\n> news) I'll cut the final today based on what is already on the\n> public repositories everywhere.\n\nBy the way, as one of the methods to double check that my result of\nreverting the merge made sense, I ran \"git rebase -ri v2.24.0 pu\"\nand excised the merge and the problematic topic out of the todo\nlist.  With the rerere database populated beforehand, it was more or\nless a painless exercise (except for one topic, en/rebase-backend,\nwhich is one of the topics that was queued forking 'master' after\nthe topic got merged *and* actually depended on what the topic did)\nand after about 1700+ steps (which did not take more than 20\nminutes, including the time spent for the manual rebasing of\nen/rebase-backend topic) I got the same tree for 'pu' I pushed out\nlast night.\n\nOne thing I noticed that \"rebase -ri\" could be taught to handle\nbetter was that the side branches that were merged to the final\nresult did not get relabeled.  Those merges that appear on the first\nparent chain leading to 'pu' call themselves as \"Merge branch 'blah'\"\nand many of them (i.e. the ones that forked before the merge of the\ntopic getting excised from the mainline) did just merge the tip of\nthe named branch without touching the commits on the side branch,\nbut some branches did have to be rebased, but their tips did not get\nupdated (only the tentative rewritten/<topic> labels were pointing\nat the updated tip during the rebase, which are of course discarded\nafter we are done).\n\nBut other than that, it was quite nice.\n\nIt is less transparent (at least to me) and probably less efficient\nthan the current workflow to rebuild 'pu' for a few times every day\n(\"less efficient\" is primarily because the established workflow is\nquite optimized to the way I work), so it is not likely for me to\nswitch to \"rebase -ri\" any time soon.  But it makes me feel safe to\nknow that there is another tool I can use to double-check the result\nof everyday workflow.\n\nThanks.\n"},{"id":"389801","messageId":"nycvar.QRO.7.76.6.2001151458100.46@tvgsbejvaqbjf.bet","threadId":"52620","inReplyTo":"xmqqpnfnj9p3.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: \"rebase -ri\" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-15T14:03:01Z","receivedAt":"2020-01-15T14:03:09Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 13 Jan 2020, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n> > Junio C Hamano <gitster@pobox.com> writes:\n> >\n> >> I will push out what I wish to be able to tag as the final [*1*]\n> >> shortly but without actually tagging, so that it can get a bit wider\n> >> exposure than just the usual \"Gitster tested locally and then did let\n> >> Travis try them\" testing.\n> >\n> > I haven't heard from any failure report so (taking no news as good\n> > news) I'll cut the final today based on what is already on the public\n> > repositories everywhere.\n>\n> By the way, as one of the methods to double check that my result of\n> reverting the merge made sense, I ran \"git rebase -ri v2.24.0 pu\"\n> and excised the merge and the problematic topic out of the todo\n> list.  With the rerere database populated beforehand, it was more or\n> less a painless exercise (except for one topic, en/rebase-backend,\n> which is one of the topics that was queued forking 'master' after\n> the topic got merged *and* actually depended on what the topic did)\n> and after about 1700+ steps (which did not take more than 20\n> minutes, including the time spent for the manual rebasing of\n> en/rebase-backend topic) I got the same tree for 'pu' I pushed out\n> last night.\n\nNice!\n\n> One thing I noticed that \"rebase -ri\" could be taught to handle\n> better was that the side branches that were merged to the final\n> result did not get relabeled.  Those merges that appear on the first\n> parent chain leading to 'pu' call themselves as \"Merge branch 'blah'\"\n> and many of them (i.e. the ones that forked before the merge of the\n> topic getting excised from the mainline) did just merge the tip of\n> the named branch without touching the commits on the side branch,\n> but some branches did have to be rebased, but their tips did not get\n> updated (only the tentative rewritten/<topic> labels were pointing\n> at the updated tip during the rebase, which are of course discarded\n> after we are done).\n\nThis has been discussed on the list before this past September, but I\nthink the discussion has stalled after v2 was sent, most likely due to my\nsuggestions asking for more, I hate to admit:\n\n\thttps://lore.kernel.org/git/20190907234413.1591-1-wh109@yahoo.com/\n\n> But other than that, it was quite nice.\n>\n> It is less transparent (at least to me) and probably less efficient\n> than the current workflow to rebuild 'pu' for a few times every day\n> (\"less efficient\" is primarily because the established workflow is\n> quite optimized to the way I work), so it is not likely for me to\n> switch to \"rebase -ri\" any time soon.  But it makes me feel safe to\n> know that there is another tool I can use to double-check the result\n> of everyday workflow.\n\nI understand. There is definitely a non-negligible cost involved whenever\nswitching from one flow that works to another that might not yet work as\nwell. I had the same hiccups when switching the Git garden shears over to\n`--rebase-merges` (it was worth it because the result is so much faster on\nWindows, of course).\n\nHaving said that, if you ever find yourself wanting Just One Feature in\n`--rebase-merges` that would make it worthwhile for you to think about\nswitching your patch-based workflow to a `rebase -ir`-based one, please\nlet me know, and I will try my best to accommodate.\n\nCiao,\nDscho\n"},{"id":"389809","messageId":"xmqqftggk2oi.fsf@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"nycvar.QRO.7.76.6.2001151458100.46@tvgsbejvaqbjf.bet","subject":"Re: \"rebase -ri\" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-15T18:14:05Z","receivedAt":"2020-01-15T18:14:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> ... after about 1700+ steps (which did not take more than 20\n>> minutes, including the time spent for the manual rebasing of\n>> en/rebase-backend topic) I got the same tree for 'pu' I pushed out\n>> last night.\n>\n> Nice!\n\nNice indeed---I forgot to say more-or-less there, though ;-)\n\nIt is quite an achievement to make it practical to rebuild the\nmaint..pu chain, which would involve 1000+ commits, while allowing\nto edit only a fraction of them.\n\n\t[ellided] observation that tips of the branches that were\n\trewritten in order to rebase the named tip that contains\n\tthem were left stale\n\n> This has been discussed on the list before this past September, but I\n> think the discussion has stalled after v2 was sent,...\n\nThat's OK.  One step at a time ;-)\n\n> Having said that, if you ever find yourself wanting Just One Feature in\n> `--rebase-merges` that would make it worthwhile for you to think about\n> switching your patch-based workflow to a `rebase -ir`-based one, please\n> let me know, and I will try my best to accommodate.\n\nAnother thing I noticed was that we may want to attempt to recreate\nan evil merge and then stop to ask confirmation.  The \"rebase -ri\" I\ndid to sanity-check my revert for example failed to bring in the\nchange made in the existing evil merge when trying to recreate the\nmerge of the dl/merge-autostash topic into master..pu chain and\nsilently created a fails-to-build-from-the-source tree instead.\n\n"},{"id":"389823","messageId":"9355c545-08a5-ef63-f7bf-65201d50acc8@gmail.com","threadId":"52620","inReplyTo":"xmqqftggk2oi.fsf@gitster-ct.c.googlers.com","subject":"Rebasing evil merges with --rebase-merges","fromName":"Igor Djordjevic","fromEmail":"igor.d.djordjevic@gmail.com","sentAt":"2020-01-15T21:23:34Z","receivedAt":"2020-01-15T21:23:39Z","isPatch":false,"sender":{"key":"igor.d.djordjevic@gmail.com","avatar":null},"body":"On 15/01/2020 19:14, Junio C Hamano wrote:\n> \n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> >\n> > Having said that, if you ever find yourself wanting Just One Feature \n> > in `--rebase-merges` that would make it worthwhile for you to think \n> > about switching your patch-based workflow to a `rebase -ir`-based \n> > one, please let me know, and I will try my best to accommodate.\n> \n> Another thing I noticed was that we may want to attempt to recreate\n> an evil merge and then stop to ask confirmation.  The \"rebase -ri\" I\n> did to sanity-check my revert for example failed to bring in the\n> change made in the existing evil merge when trying to recreate the\n> merge of the dl/merge-autostash topic into master..pu chain and\n> silently created a fails-to-build-from-the-source tree instead.\n\nFYI (and anyone interested), it`s something we actually brought up \nsome two years ago, at the time of introducing `--rebase-merges` \n(known as `--recreate-merges` back at the time), see[1].\n\nIt ended being a lengthy and heated discussion (inside a few \ndifferent topics as well, like original RFC[2] and it`s v2 update[3]), \nmyself being guilty for dropping out eventually and not following it \nthrough, though, life taking me in another direction at the moment... \nbut I still find this functionality to be very useful, not to say \nessential, even, for reliable complex merge _rebasing_ (meaning \nkeeping \"evil merge\" changes, too), and not just merge _recreating_ \n(loosing \"evil merge\" changes, and worse - doing it silently, as you \nexperienced yourself now as well).\n\np.s. Bringing that one up again, it can`t go without saying a huge \nthanks to Dscho for taking it this far in the meantime anyway <3\n\nRegards, Buga\n\n[1]: https://lore.kernel.org/git/bc9f82fb-fd18-ee45-36a4-921a1381b32e@gmail.com/\n[2]: https://lore.kernel.org/git/87y3jtqdyg.fsf@javad.com/\n[3]: https://lore.kernel.org/git/87r2oxe3o1.fsf@javad.com/\n"},{"id":"389829","messageId":"xmqqr200ib73.fsf@gitster-ct.c.googlers.com","threadId":"52620","inReplyTo":"xmqqftggk2oi.fsf@gitster-ct.c.googlers.com","subject":"Re: \"rebase -ri\" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-15T22:53:04Z","receivedAt":"2020-01-15T22:53:09Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Having said that, if you ever find yourself wanting Just One Feature in\n>> `--rebase-merges` that would make it worthwhile for you to think about\n>> switching your patch-based workflow to a `rebase -ir`-based one, please\n>> let me know, and I will try my best to accommodate.\n\nI missed a mention of 'patch' here when I prepared my earlier reply.\nI do not expect \"rebase -ri\" to play any role in the part of the\nworkflow that accepts patches from the mailing list.\n\nThe involvement of \"rebase -ri\" (vs Meta/Reintegrate) is purely what\nhappens after a topic is queued and starts to get tested with other\ntopics on integration branches, and no \"patch based workflow\" plays\nany role there---it does not make any sense to base that part on\nanything but merge (and possibly cherry-picking an evil merge from\nan earlier round).\n"},{"id":"389879","messageId":"87h80vg849.fsf@osv.gnss.ru","threadId":"52620","inReplyTo":"9355c545-08a5-ef63-f7bf-65201d50acc8@gmail.com","subject":"Re: Rebasing evil merges with --rebase-merges","fromName":"Sergey Organov","fromEmail":"sorganov@gmail.com","sentAt":"2020-01-16T07:42:30Z","receivedAt":"2020-01-16T07:42:34Z","isPatch":false,"sender":{"key":"sorganov@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8501568?v=4"},"body":"Igor Djordjevic <igor.d.djordjevic@gmail.com> writes:\n\n> On 15/01/2020 19:14, Junio C Hamano wrote:\n>> \n>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n>> >\n>> > Having said that, if you ever find yourself wanting Just One Feature \n>> > in `--rebase-merges` that would make it worthwhile for you to think \n>> > about switching your patch-based workflow to a `rebase -ir`-based \n>> > one, please let me know, and I will try my best to accommodate.\n>> \n>> Another thing I noticed was that we may want to attempt to recreate\n>> an evil merge and then stop to ask confirmation.  The \"rebase -ri\" I\n>> did to sanity-check my revert for example failed to bring in the\n>> change made in the existing evil merge when trying to recreate the\n>> merge of the dl/merge-autostash topic into master..pu chain and\n>> silently created a fails-to-build-from-the-source tree instead.\n>\n> FYI (and anyone interested), it`s something we actually brought up \n> some two years ago, at the time of introducing `--rebase-merges` \n> (known as `--recreate-merges` back at the time), see[1].\n>\n> It ended being a lengthy and heated discussion (inside a few \n> different topics as well, like original RFC[2] and it`s v2 update[3]), \n> myself being guilty for dropping out eventually and not following it \n> through, though, life taking me in another direction at the moment...\n\nFor reference, there is a nice summary in \"Git Rev News Edition 38\":\n\nhttps://git.github.io/rev_news/2018/04/18/edition-38\n\n> but I still find this functionality to be very useful, not to say \n> essential, even, for reliable complex merge _rebasing_ (meaning \n> keeping \"evil merge\" changes, too), and not just merge _recreating_ \n> (loosing \"evil merge\" changes, and worse - doing it silently, as you \n> experienced yourself now as well).\n\nYeah, dropping user content silently and by default is still the most\nweird thing for git to do, be it a merge or not a merge.\n\nAs an additional note, I came to conclusion that there is actually no\nsuch thing as \"evil merge\" that is somehow different from \"evil commit\"\nin general (a commit containing unrelated changes).\n\nThen, as \"evil commit\" belongs to user domain, we need to finally\nrealize that \"evil merge\" belongs entirely to user domain as well, and\nthus, as it's out of git domain, we should stop using the term \"evil\nmerge\" to excuse any kinds of weird git behaviors.\n\nRegards,\nSergey\n"},{"id":"389984","messageId":"cdada301-b521-78b4-badc-192af2fa3d08@gmail.com","threadId":"52620","inReplyTo":"nycvar.QRO.7.76.6.2001121936290.46@tvgsbejvaqbjf.bet","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-01-17T14:11:21Z","receivedAt":"2020-01-17T14:11:28Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 12/01/2020 18:41, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Sun, 12 Jan 2020, Phillip Wood wrote:\n> \n>> On 12/01/2020 16:12, Phillip Wood wrote:\n>>> I'm concerned that there are some bugs in this series and think\n>>> it may be best to revert it before releasing 2.25.0. Jonathan\n>>> Nieder posted a bug report on Friday [1] which I think is caused\n>>> by this series. While trying to reproduce Jonathan's bug I came\n>>> up with the test below which fails, but not in the same way.\n> \n> Thank you so much for your thoughts and your work on this. For what it's\n> worth, I totally agree with your assessment and your suggestion to revert\n> those patches _before_ releasing v2.25.0. (I seem to remember vaguely that\n> there were repeated requests for better test coverage and that those\n> requests went unaddressed, so I would not be surprised if there were more\n> unfortunate surprises waiting for us.)\n\nYes there were more surprises - when we fork `git merge` \n--committer-date-is-author-date is broken. That was tested but with a \ncommit where the author date was the current time so it did not detect \nthe failure.\n\n> [...]\n>> --- >8 ---\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 763ccbbc45..22a38de47b 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,\n>>                  if (!date)\n>>                          return -1;\n>>\n>> -               strbuf_addf(&datebuf, \"@%s\", date);\n>> +               strbuf_addf(&datebuf, \"%s\", date);\n> \n> I have to admit that I have not analyzed the code before this hunk (it\n> would be much easier to increase the context in a non-static reviewing\n> environment, e.g. on GitHub, but the mailing list does not allow for\n> that), so I do not know just _how_ likely our `date` here is going to\n> change or remain prefixed by a `@`. Therefore, this suggestion might be\n> totally stupid: `\"@%s\", date + (*date == '@')`\n\nThe date was read from the author-script so I think we should leave it \nas is in case the user has edited it and is using a different date \nformat. Having said that I'm keen to make a bigger change to Rohit's \nimplementation and just get the author date out of the argv_array \nholding the child's environment as this avoids re-reading the \nauthor-script file. It has taken a bit longer than I planned so it'll be \nnext week before I post the fixes.\n\nBest Wishes\n\nPhillip\n\n> Thanks again,\n> Dscho\n> \n>>                  res = setenv(\"GIT_COMMITTER_DATE\",\n>>                               opts->ignore_date ? \"\" : datebuf.buf, 1);\n>>\n>>> The\n>>> test coverage of this series has always been pretty poor and I\n>>> think it needs improving for us to have confidence in it. I'm\n>>> also concerned that at least one of the\n>>> tests ('--committer-date-is-author-date works with rebase -r')\n>>> does not detect failures properly in the code below\n>>>\n>>> \twhile read HASH\n>>> \tdo\n>>> \t\tgit show $HASH --pretty=\"format:%ai\" >authortime\n>>> \t\tgit show $HASH --pretty=\"format:%ci\" >committertime\n>>> \t\ttest_cmp authortime committertime\n>>> \tdone <rev_list\n>>>\n>>>\n>>> Best Wishes\n>>>\n>>> Phillip\n>>>\n>>> [1] https://lore.kernel.org/git/20200110231436.GA24315@google.com/\n>>>\n>>> --- >8 ---\n>>> diff --git a/t/t3433-rebase-options-compatibility.sh b/t/t3433-rebase-options-compatibility.sh\n>>> index 5166f158dd..c81e1d7167 100755\n>>> --- a/t/t3433-rebase-options-compatibility.sh\n>>> +++ b/t/t3433-rebase-options-compatibility.sh\n>>> @@ -6,6 +6,7 @@\n>>>    test_description='tests to ensure compatibility between am and interactive backends'\n>>>\n>>>    . ./test-lib.sh\n>>> +. \"$TEST_DIRECTORY\"/lib-rebase.sh\n>>>\n>>>    GIT_AUTHOR_DATE=\"1999-04-02T08:03:20+05:30\"\n>>>    export GIT_AUTHOR_DATE\n>>> @@ -99,6 +100,22 @@ test_expect_success '--committer-date-is-author-date works with rebase -r' '\n>>>           done <rev_list\n>>>    '\n>>>\n>>> +test_expect_success '--committer-date-is-author-date works when committing conflict resolution' '\n>>> +       git checkout commit2 &&\n>>> +       (\n>>> +               set_fake_editor &&\n>>> +               FAKE_LINES=2 &&\n>>> +               export FAKE_LINES &&\n>>> +               test_must_fail git rebase -i HEAD^^\n>>> +       ) &&\n>>> +       echo resolved > foo &&\n>>> +       git add foo &&\n>>> +       git rebase --continue &&\n>>> +       git log -1 --format=%at commit2 >expect &&\n>>> +       git log -1 --format=%ct HEAD >actual &&\n>>> +       test_cmp expect actual\n>>> +'\n>>> +\n>>>    # Checking for +0000 in author time is enough since default\n>>>    # timezone is UTC, but the timezone used while committing\n>>>    # sets to +0530.\n>>>\n>>\n"},{"id":"390070","messageId":"nycvar.QRO.7.76.6.2001201214260.46@tvgsbejvaqbjf.bet","threadId":"52620","inReplyTo":"cdada301-b521-78b4-badc-192af2fa3d08@gmail.com","subject":"Re: Problems with ra/rebase-i-more-options - should we revert it?","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2020-01-20T11:15:00Z","receivedAt":"2020-01-20T11:15:18Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 17 Jan 2020, Phillip Wood wrote:\n\n> On 12/01/2020 18:41, Johannes Schindelin wrote:\n> >\n> > On Sun, 12 Jan 2020, Phillip Wood wrote:\n> >\n> > > On 12/01/2020 16:12, Phillip Wood wrote:\n> > > > I'm concerned that there are some bugs in this series and think it\n> > > > may be best to revert it before releasing 2.25.0. Jonathan Nieder\n> > > > posted a bug report on Friday [1] which I think is caused by this\n> > > > series. While trying to reproduce Jonathan's bug I came up with\n> > > > the test below which fails, but not in the same way.\n> >\n> > Thank you so much for your thoughts and your work on this. For what\n> > it's worth, I totally agree with your assessment and your suggestion\n> > to revert those patches _before_ releasing v2.25.0. (I seem to\n> > remember vaguely that there were repeated requests for better test\n> > coverage and that those requests went unaddressed, so I would not be\n> > surprised if there were more unfortunate surprises waiting for us.)\n>\n> Yes there were more surprises - when we fork `git merge`\n> --committer-date-is-author-date is broken. That was tested but with a\n> commit where the author date was the current time so it did not detect\n> the failure.\n\nThanks for confirming.\n\n> > [...]\n> > > --- >8 ---\n> > > diff --git a/sequencer.c b/sequencer.c\n> > > index 763ccbbc45..22a38de47b 100644\n> > > --- a/sequencer.c\n> > > +++ b/sequencer.c\n> > > @@ -988,7 +988,7 @@ static int run_git_commit(struct repository *r,\n> > >                  if (!date)\n> > >                          return -1;\n> > >\n> > > -               strbuf_addf(&datebuf, \"@%s\", date);\n> > > +               strbuf_addf(&datebuf, \"%s\", date);\n> >\n> > I have to admit that I have not analyzed the code before this hunk (it\n> > would be much easier to increase the context in a non-static reviewing\n> > environment, e.g. on GitHub, but the mailing list does not allow for\n> > that), so I do not know just _how_ likely our `date` here is going to\n> > change or remain prefixed by a `@`. Therefore, this suggestion might be\n> > totally stupid: `\"@%s\", date + (*date == '@')`\n>\n> The date was read from the author-script so I think we should leave it as is\n> in case the user has edited it and is using a different date format. Having\n> said that I'm keen to make a bigger change to Rohit's implementation and just\n> get the author date out of the argv_array holding the child's environment as\n> this avoids re-reading the author-script file. It has taken a bit longer than\n> I planned so it'll be next week before I post the fixes.\n\nI look forward to it!\nDscho\n"}]}