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

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

[Phillip Wood]

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

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

[Phillip Wood]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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.

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.

"rebase -ri" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)

[Junio C Hamano]

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.

Re: "rebase -ri" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)

[Johannes Schindelin]

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

Re: "rebase -ri" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)

[Junio C Hamano]

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.

Rebasing evil merges with --rebase-merges

[Igor Djordjevic]

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/

Re: "rebase -ri" (was Re: Problems with ra/rebase-i-more-options - should we revert it?)

[Junio C Hamano]

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

Re: Rebasing evil merges with --rebase-merges

[Sergey Organov]

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

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

[Phillip Wood]

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

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

[Johannes Schindelin]

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