On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > 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. 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?
