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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 21, 2018, 08:54 UTC
Message-ID: <CAPig+cSm3kHF_hTRKutCCNsY82sE3xZfXJkQsKi+zedq9rdnPw@mail.gmail.com>
In-Reply-To: <CAKk8ispayA9ceLLRTQJd8_h_S7VaQqSoTBKGz+GjB3EbB7WO+g@mail.gmail.com>

{cc:+junio}

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

Show 5 quoted lines

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

"blame -C" detects code movement across files, so you needn't feel constrained in that sense. (In fact, "blame -C -C" should detect the copied -- not moved -- code in your patch 1/2, so my objection is not entirely valid, although "-C -C" is potentially much slower.)

Nevertheless, leaving all the code in git-rebase--interactive.sh sounds sensible if there is not a compelling reason to split it out, especially since each refactoring (especially a big one) has the potential to collide with in-flight or planned topics.

Show 6 quoted lines

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

Don't worry about indentation changes during refactoring and code movement; it's frequently necessary, and "blame" still works (with "-w", as you point out).

> Thoughts or other ideas?

A few...

Although you may have the entire refactoring in your head -- you know what you moved where and why and what changes were needed to make the move possible -- reviewers don't have that luxury. A nearly 1000-line patch, such as 1/3, which copies code (possibly verbatim or possibly with edits -- reviewers don't know which) and which contains newly-written code is a significant burden on reviewers. Crafting a patch series such that it holds the hands of reviewers by laying out each change in easily digested chunks helps significantly, both with attracting more and better reviews, and with potential buy-in that the changes are worthwhile.

However, I'm just one (potential) reviewer, and I don't want you wasting your time embarking on large-scale changes based upon my comments before hearing from Dscho (Johannes) and Junio if such refactoring is even welcome.