git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
To: Phillip Wood <phillip.wood@dunelm.org.uk>
Cc: Git Mailing List <git@vger.kernel.org>,
	Igor Djordjevic <igor.d.djordjevic@gmail.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
Date: Sat, 28 Jul 2018 14:40:59 +0200
Message-ID: <874lgjv8h0.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20180726155854.20832-1-phillip.wood@talktalk.net>


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.

  parent reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-19 11:36 [PATCH v1 0/3] " Phillip Wood
2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
2018-02-19 11:36 ` [PATCH v1 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite
2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
2018-03-06 20:29     ` Igor Djordjevic
2018-03-06 21:33       ` Igor Djordjevic
2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-03-06 19:57     ` Junio C Hamano
2018-03-08 11:05       ` Phillip Wood
2018-03-08 17:53         ` Junio C Hamano
2018-03-13 12:06           ` Phillip Wood
2018-03-13 16:32             ` Junio C Hamano
2018-03-14 11:02               ` Phillip Wood
2018-03-06 20:41     ` Igor Djordjevic
2018-03-06 10:17   ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-03-06 20:33     ` Igor Djordjevic
2018-03-06 20:19   ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic
2018-03-06 21:03     ` Junio C Hamano
2018-03-06 21:20       ` Igor Djordjevic
2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
2018-03-16 10:13   ` [PATCH v3 1/3] " Phillip Wood
2018-03-16 10:13   ` [PATCH v3 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-03-16 10:13   ` [PATCH v3 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-03-29 18:32   ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano
2018-03-30 11:09     ` Phillip Wood
2018-03-31 19:20       ` Ævar Arnfjörð Bjarmason
2018-04-02 10:55         ` Phillip Wood
2018-04-02 11:39           ` Ævar Arnfjörð Bjarmason
2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
2018-07-26 10:22   ` [PATCH v4 1/4] " Phillip Wood
2018-07-26 10:22   ` [RFC PATCH v4 2/4] add -p: select modified lines correctly Phillip Wood
2018-07-26 10:22   ` [PATCH v4 3/4] add -p: allow line selection to be inverted Phillip Wood
2018-07-26 10:22   ` [PATCH v4 4/4] add -p: optimize line selection for short hunks Phillip Wood
2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
2018-07-26 19:36     ` Junio C Hamano
2018-07-27 10:05       ` Phillip Wood
2018-07-27 16:09         ` Junio C Hamano
2018-07-26 15:58   ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood
2018-07-26 19:30     ` Junio C Hamano
2018-07-27 10:19       ` Phillip Wood
2018-07-27 16:14         ` Junio C Hamano
2018-07-26 15:58   ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood
2018-07-26 15:58   ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood
2018-07-27 18:27   ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason
2018-07-28 10:08     ` Phillip Wood
2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason [this message]
2018-08-03 10:01     ` Phillip Wood
2018-08-03 16:51       ` Junio C Hamano
2018-08-03 17:59       ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publically 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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=874lgjv8h0.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=igor.d.djordjevic@gmail.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox