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

[Wink Saville <wink@saville.com>]

On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville <wink@saville.com> wrote:
>> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine <sunshine@sunshineco.com> 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.

That's what I figured.

Anyway, I've played around and my current thoughts are to not create
any new files and
keep git_rebase__interactive and the new
git_rebase__interactive__preserve_merges
functions in git-rebase--interactive.sh.

Doing that will preserve the blame for the existing functions. But if
I do indentation
reformating as I extract functions that will be shared between
git_rebase__interactive
and git_rebase__interactive__preserve_merges then we still lose the blame
information unless the "-w" parameter is passed to blame. I could choose to
not do the indentation, but that doesn't seem like a good idea.

An alternative is that we don't accept the refactoring. What I'd
probably do is use
the refactored code to figure out a fix for the bug and then back port
the fix to the
existing code.

My opinion is that to not accept "improved" code because we lose blame
information
is not a good trade off. Of course what I might think is "improved"
others may rightfully
say the refactoring is gratuitous. If that is the case than not doing
the refactoring is the
right solution. I don't see a right or wong here, just a typical
engineering trade off.

Thoughts or other ideas?