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

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville <wink@saville.com> wrote:
> Patch 0001 creates a library of functions which can be
> used by git-rebase--interactive and
> git-rebase--interactive--preserve-merges. The functions are
> those that exist in git-rebase--interactive.sh plus new
> functions created from the body of git_rebase_interactive
> that will be used git_rebase_interactive in the third patch
> and git_rebase_interactive_preserve_merges in the second
> patch. None of the functions are invoked so there is no
> logic changes and the system builds and passes all tests
> on travis-ci.org.
>
> Patch 0002 creates git-rebase--interactive--preserve-merges.sh
> with the function git_rebase_interactive_preserve_merges. The contents
> of the function are refactored from git_rebase_interactive and
> uses existing and new functions in the library. A small modification
> of git-rebase is also done to invoke the new function when the -p
> switch is used with git-rebase. When this is applied on top of
> 0001 the system builds and passes all tests on travis-ci.org.
>
> The final patch, 0003, removes all unused code from
> git_rebase_interactive and uses the functions from the library
> where appropriate. And, of course, when applied on top of 0002
> the system builds and passes all tests on travis-ci.org.

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

Thanks.