Re: [RFC PATCH 0/3] rebase-interactive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville <wink@xxxxxxxxxxx> wrote:
> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>> A problem with this approach is that it loses "blame" information. A
>> git-blame of git-rebase--interactive--lib.sh shows all code in that
>> file as having arisen spontaneously from thin air; it is unable to
>> trace its real history. It would be much better to actually _move_
>> code to the new file (and update callers if necessary), which would
>> preserve provenance.
>>
>> Ideally, patches which move code around should do so verbatim (or at
>> least as close to verbatim as possible) to ease review burden.
>> Sometimes changes to code are needed to make it relocatable before
>> movement, in which case those changes should be made as separate
>> preparatory patches, again to ease review.
>>
>> As it is, without detailed spelunking, it is not immediately clear to
>> a reviewer which functions in git-rebase--interactive--lib.sh are
>> newly written, and which are merely moved (or moved and edited) from
>> git-rebase--interactive.sh. This shortcoming suggests that the patch
>> series could be re-worked to do the refactoring in a more piecemeal
>> fashion which more clearly holds the hands of those trying to
>> understand the changes. (For instance, one or more preparatory patches
>> may be needed to make the code relocatable, followed by verbatim code
>> relocation, possibly iterating these steps if some changes depend upon
>> earlier changes, etc.)
>
> Must all intermediate commits continue build and pass tests?

Yes, not just because it is good hygiene, but also because it
preserves git-bisect'ability.



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux