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

[Eric Sunshine]

{cc:+junio}

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

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

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

[Wink Saville]

On Tue, Mar 20, 2018 at 2:47 PM, Eric Sunshine  wrote:
> On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville  wrote:
>> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  
>> 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?

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

[Eric Sunshine]

On Tue, Mar 20, 2018 at 5:23 PM, Wink Saville  wrote:
> On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  
> 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.

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

[Wink Saville]

On Tue, Mar 20, 2018 at 2:11 PM, Eric Sunshine  wrote:
> On Tue, Mar 20, 2018 at 4:45 PM, Wink Saville  wrote:
>> 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.

Must all intermediate commits continue build and pass tests?

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

[Eric Sunshine]

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

[RFC PATCH 0/3] rebase-interactive

[Wink Saville]

I've not worked on the git sources before and while looking into
fixing test_expect_failure 'exchange two commits with -p' in
t3404-rebase-interactive.sh, I found it difficult to understand
the git testing infracture and git-rebase--interactive.sh.

So as part of learning my way around I thought I'd refactor
git-rebase--interactive to make it easier for me to understand.
At this point I do have some understanding and will be working
on fixing the bug. In the mean time I'm requesting comments on
this refactoring patch sequence.

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.

Wink Saville (3):
  rebase-interactive: create git-rebase--interactive--lib.sh
  rebase-interactive: create git-rebase--interactive--preserve-merges
  rebase-interactive: refactor git-rebase--interactive to use library

 .gitignore  |2 +
 Makefile|2 +
 git-rebase--interactive--lib.sh |  944 +
 git-rebase--interactive--preserve-merges.sh |  134 
 git-rebase--interactive.sh  | 1019 +--
 git-rebase.sh   |7 +-
 6 files changed, 1107 insertions(+), 1001 deletions(-)
 create mode 100644 git-rebase--interactive--lib.sh
 create mode 100644 git-rebase--interactive--preserve-merges.sh

-- 
2.16.2