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

> 
> On Thu, Jul 26 2018, Phillip Wood wrote:
> 
>> 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.