Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 28, 2018, 12:40 UTC
Message-ID: <874lgjv8h0.fsf@evledraar.gmail.com>
In-Reply-To: <20180726155854.20832-1-phillip.wood@talktalk.net>

On Thu, Jul 26 2018, Phillip Wood wrote:

Show 29 quoted lines

> Unfortuantely v4 had test failures due to a suprious brace from a last
> minute edit to a comment that I forgot to test. This version fixes
> that, my applogies for the patch churn.
>
> I've updated this series based on Ævar's feedback on v3 (to paraphrase
> stop using '$_' so much and fix staging modified lines.). The first
> patch is functionally equivalent to the previous version but with a
> reworked implementation. Patch 2 is new, it implements correctly
> staging modified lines with a couple of limitations - see the commit
> message for more details, I'm keen to get some feedback on it. Patches
> 3 and 4 are essentially rebased and tweaked versions of patches 2 and
> 3 from the previous version.
>
> This series is based on pw/add-p-recount (f4d35a6b49 "add -p: fix
> counting empty context lines in edited patches")
>
> The motivation for this series is summed up in the first commit
> message:
>
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."

I tested this with an eye towards what I pointed out in https://public-inbox.org/git/878ta8vyqe.fsf@evledraar.gmail.com/

Using the same workflow (search for "So what I was expecting" in that E-Mail) this now does the right thing in that example:

    select lines? 4,10
    [...]
    $ git diff --staged -U1
    diff --git a/README.md b/README.md
    index ff990622a3..6d16f7e52b 100644
    --- a/README.md
    +++ b/README.md
    @@ -20,3 +20,3 @@ See [Documentation/gittutorial.txt][] to get started, then see
     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
    u git ((49703a4754...) $) $

Some other comments on this:

1) It needs to be more obvious how to exit this sub-mode, i.e. consider
this confused user:

    Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,l,?]? l
    select lines? ?
    invalid hunk line '?'
    select lines? q
    invalid hunk line 'q'
    select lines? exit
    invalid hunk line 'exit'
    select lines? quit
    invalid hunk line 'quit'
    select lines? :wq
    invalid hunk line ':wq'
    select lines? help
    invalid hunk line 'help'

Just doing Ctrl+D or RET exits it. Instead "?" should print some help related to this sub-mode showing what the syntax is, and how to exit the sub-mode. I think it would make sense for "q" to by synonymous with "RET", i.e. you'd need "q<RET>q<RET>" to fully exit, but I don't know...

2) I think it's confusing UI that selecting some of the lines won't
re-present the hunk to you again in line mode, but I see this is
consistent with how e.g. "e" works, it won't re-present the hunk to you
if there's still something to do, you need to exit and run "git add -p"
again.

I think it makes sense to change that and you'd either "e" or "l" and then "n" to proceed, or continue, but that's per-se unrelated to this feature. Just something I ran into...

3) I don't see any way around this, but we need to carefully explain
that selecting a list of things in one session is *not* the same thing
as selecting them incrementally in multiple sessions. I.e. consider this
diff:

    @@ -1,3 +1,3 @@
    -a
    -b
    -c
    +1
    +2
    +3

If I select 1,4 I get, as expected:

    @@ -1,3 +1,3 @@
    -a
    +1
     b
     c

And then in the next session:

      @@ -1,3 +1,3 @@
       1
    1 -b
    2 -c
    3 +2
    4 +3
    select lines? 1,3

Yields, as expected:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +2
     c

But this is not the same as redoing the whole thing as:

    select lines? 1,4
    select lines? 1
    select lines? 3

Which instead yields:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
     c
    +3

Now, rummaging through my wetware and that E-Mail from back in March I don't see how it could work differently, and you *also* want to be able to select one line at a time like that.

Just something that's not very intuative / hard to explain, and maybe there should be a different syntax (e.g. 1:4) for this "swap 1 for 4" operation, as opposed to selecting lines 1 and 4 as they appear in the diff.

4) With that abc 123 diff noted above, why am I in two sessions allowed
to do:

    @@ -1,3 +1,3 @@
    1 -a
    2 -b
    3 -c
    4 +1
    5 +2
    6 +3
    select lines? 1,4
    select lines? 1,4

To end up with this staged:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +3
     c

But not allowed to do the same thing in one operation via:

      @@ -1,3 +1,3 @@
    1 -a
    2 -b
    3 -c
    4 +1
    5 +2
    6 +3
    select lines? 1,4,2,6
    unable to pair up insertions and deletions

But I am allowed to do e.g.:

    select lines? 1,4,2,5

To end up with:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +2
     c

I can do this in two steps.