mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <>
To: Wink Saville <>
Cc: Git List <>,
	Johannes Schindelin <>,
	Junio C Hamano <>
Subject: Re: [RFC PATCH 0/3] rebase-interactive
Date: Wed, 21 Mar 2018 04:54:43 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>


On Tue, Mar 20, 2018 at 11:31 PM, Wink Saville <> wrote:
> 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
> 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
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.

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

  reply	other threads:[~2018-03-21  8:54 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-20 20:45 [RFC PATCH 0/3] rebase-interactive Wink Saville
2018-03-20 20:45 ` [RFC PATCH 1/3] rebase-interactive: create Wink Saville
2018-03-20 20:45 ` [RFC PATCH 2/3] rebase-interactive: create git-rebase--interactive--preserve-merges Wink Saville
2018-03-20 20:45 ` [RFC PATCH 3/3] rebase-interactive: refactor git-rebase--interactive to use library Wink Saville
2018-03-20 21:11 ` [RFC PATCH 0/3] rebase-interactive Eric Sunshine
2018-03-20 21:23   ` Wink Saville
2018-03-20 21:47     ` Eric Sunshine
2018-03-21  3:31       ` Wink Saville
2018-03-21  8:54         ` Eric Sunshine [this message]
2018-03-21 17:44           ` [RFC PATCH v2 0/1] rebase-interactive: Add git_rebase__interactive__preserve_merges Wink Saville
2018-03-21 17:44           ` [RFC PATCH v2 1/1] " Wink Saville
2018-03-21 19:42             ` Junio C Hamano
2018-03-21 21:49               ` Wink Saville
2018-03-21 22:43                 ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).