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.
