Re: [PATCH v3 0/3] add -p: select individual hunk lines

On Fri, Mar 30 2018, Phillip Wood wrote:

First of all thinks for working on this. Something like this is a feature I've long wanted to have and have just been manually using edit.

As for the code, one comment: For reasons of avoiding something like the 2.17.0-rc* bug I just sent a patch for, I think you should change your use of the implicit $_ to something where you explicitly create lexical variables instead.

It's bad style in Perl to use $_ for anything except a one-liner, and similar to the $1 bug with your other patch, you'll get buggy code (regardless of your use of local $_) if one of the functions you're calling in these >10 line for-loops starts doing something to set $_ itself, as demonstrated by:

    $ perl -wE 'sub foo { local $_; for (1..3) { bar(); say } } sub bar { $_ = $_ ** 2; } foo()'
    1
    4
    9

Let's just name these variables, even if it wasn't for that caveat it would still be a good idea, since for any non-trivial use of $_ you've got to mentally keep track of what set $_ where, so it's hard to read.

As for the implementation, I *want* to love this, but it seems the way it works is just fatally flawed, consider. *The* use-case I've had for something like this (maybe yours differs?) is something where I do e.g.:

    $ perl -pi -e 's/git/Git/g' README.md

Which gives me (among other things):

    -See [Documentation/gittutorial.txt][] to get started, then see
    -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
    -Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
    -read with `man gittutorial` or `git help tutorial`, and the
    -documentation of each command with `man git-<commandname>` or `git help
    +See [Documentation/Gittutorial.txt][] to get started, then see
    +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
    +Documentation/Git-<commandname>.txt for documentation of each command.
    +If Git has been correctly installed, then the tutorial can also be
    +read with `man Gittutorial` or `Git help tutorial`, and the
    +documentation of each command with `man Git-<commandname>` or `Git help

Which to me, is a perfect use-case for this feature. Here I hypothetically want to change "git" to "Git" in prose, so I only want to change that "If git has been" line, the rest are all references to filenames or command names.

So I would manually edit the hunk via "e" to:

     See [Documentation/gittutorial.txt][] to get started, then see
     [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
    +If Git has been correctly installed, then the tutorial can also be
     read with `man gittutorial` or `git help tutorial`, and the
     documentation of each command with `man git-<commandname>` or `git help
     <commandname>`.

Yay, but very tedious. Now let's use your feature to do this:

     1 -See [Documentation/gittutorial.txt][] to get started, then see
     2 -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     3 -Documentation/git-<commandname>.txt for documentation of each command.
     4 -If git has been correctly installed, then the tutorial can also be
     5 -read with `man gittutorial` or `git help tutorial`, and the
     6 -documentation of each command with `man git-<commandname>` or `git help
     7 +See [Documentation/Gittutorial.txt][] to get started, then see
     8 +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
     9 +Documentation/Git-<commandname>.txt for documentation of each command.
    10 +If Git has been correctly installed, then the tutorial can also be
    11 +read with `man Gittutorial` or `Git help tutorial`, and the
    12 +documentation of each command with `man Git-<commandname>` or `Git help
        <commandname>`.

    select lines? 4,10

So what I was expecting this to do was some automagic where it would pair up the 4 line, and based on the removed/added count figure out which line I'm also adding corresponds to that. I.e. both selected lines are the 4th line removed/added, so it should transpose the 10th to the 4th, but instead I get a patch that looks like this:

    diff --git a/README.md b/README.md
    index f17af66a97..7234756e64 100644
    --- a/README.md
    +++ b/README.md
    @@ -18,9 +18,9 @@ including full documentation and Git related tools.
     See [Documentation/gittutorial.txt][] to get started, then see
     [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
     read with `man gittutorial` or `git help tutorial`, and the
     documentation of each command with `man git-<commandname>` or `git help
    +If Git has been correctly installed, then the tutorial can also be
     <commandname>`.

I.e. it just grepped out the removed line from the removed chunk, and the same for the added bit, which of course means that now the added line doesn't get injected into the correct place, but added to the end.

I can see *why* that happens, but I can't imagine a case where this behavior isn't useless.

What this seems useful for now is for chunks that only consist of lines that are added or removed, maybe there's similar edge cases with those, but I can't think of any, there I think we should do the obvious and intuitive thing.

But I think that as this stands we really should at least disable this where we present the user with a hunk that consists of both removed & added lines, since I think the desired behavior I've described above should be the default, and once we pick one we're going to have to support it forever, so it's important to get it right to begin with.