Re: [PATCH 1/6] Revert "Revert "Merge branch 'ra/rebase-i-more-options'""

From: Phillip Wood <phillip.wood123@gmail.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Junio C Hamano <gitster@pobox.com>
Cc: Elijah Newren <newren@gmail.com>,
	Phillip Wood <phillip.wood@dunelm.org.uk>,
	Git Mailing List <git@vger.kernel.org>,
	Rohit Ashiwal <rohit.ashiwal265@gmail.com>,
	Jonathan Nieder <jrnieder@gmail.com>
Subject: Re: [PATCH 1/6] Revert "Revert "Merge branch 'ra/rebase-i-more-options'""
Date: Mon, 13 Apr 2020 10:58:34 +0100	[thread overview]
Message-ID: <43d06bc0-b2ee-0ae6-f22c-9850e4033d45@gmail.com> (raw)
In-Reply-To: <nycvar.QRO.7.76.6.2004121944590.46@tvgsbejvaqbjf.bet>

Hi All

On 12/04/2020 18:47, Johannes Schindelin wrote:
> Hi Phillip, Elijah & Junio,
> 
> On Tue, 7 Apr 2020, Junio C Hamano wrote:
> 
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Elijah Newren <newren@gmail.com> writes:
>>>
>>>> On Tue, Apr 7, 2020 at 7:11 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>>>
>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>
>>>>> This reverts commit 4d924528d8bfe947abfc54ee9bd3892ab509c8cd.
>>>>>
>>>>> This is being reverted to enable some fixups for
>>>>> ra/rebase-i-more-options to be built on this commit.
>>>>
>>>> This makes sense to me, but it will be only the second 'Revert
>>>> "Revert..."' commit in all of git.git and I'm curious if Junio will be
>>>> unhappy with it.
>>>
>>> Nah, there isn't much to become unhappy about.
>>>
>>> I however suspect that the alternative would certainly be much nicer
>>> and easier to understand, which is to rebuild the 7-patch series
>>> c58ae96fc4..d82dfa7f5b but bugs already fixed, instead of doing this
>>> patch to take us back to a known buggy state and then fix the result
>>> with 5 more patches.  Is that what you meant?
>>
>> After looking at the conflict resolution while merging the result of
>> applying these patches on top of the older codebase, I would have to
>> say that the approach """I've opted to add some cleanup commits on
>> top of Rohit's work rather than reworking his patches.""" may not
>> have been particularly a brilliant idea, not because the conflicts
>> arising from an older codebase are unpleasant to resolve (they seem
>> to be reasonably contained), but because it resurrects other
>> unwanted cruft we have cleaned up since then, and worse yet, it does
>> so without triggering conflicts.  For example, we'll end up seeing
>> mentions of "'am' backend", which have all been updated to "'apply'
>> backend", in the documentation, and patches [2-6/6] do not fix them.
>>
>> [5/6] is an example of one more "unwanted" thing the reversion
>> resurrects that needed to be fixed, I guess?
>>
>> The result of applying all these patches and merging it to 'master'
>> and/or 'pu' may be more or less right, as far as the new features
>> added to the "rebase -i" by the series are concerned but there may
>> be many small "unwanted cruft" we may be resurrecting with [1/6],
>> so...
> 
> I agree that it would make for a much nicer read if the entire patch
> series was simply rebased on top of v2.26.0, with drops instead of
> reverts. I suspect that 4/6 will not even become a fixup, but that the
> resulting patch is really more of an `Initial-patch-by: Rohit` material
> with Phillip as the author on record.
> 
> As to the changes, I had a brief look over them, and I have nothing to add
> to Elijah's review except to stress how excited I am about the increased
> test coverage. From my perspective, this makes the patch series 10x
> better.

Thanks for all your feedback, I'll reroll as a new patch series based on
master

Best Wishes

Phillip

> Thanks,
> Dscho
>