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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 20, 2018, 21:11 UTC
Message-ID: <CAPig+cRw4MSfcKJcgT-srCE7sDYi3qA0TrNApXBDBsgodVb3Pg@mail.gmail.com>
In-Reply-To: <20180320204507.12623-1-wink@saville.com>

On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville <wink@saville.com> wrote:

Show 23 quoted lines

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