git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
To: phillip.wood@dunelm.org.uk
Cc: Junio C Hamano <gitster@pobox.com>,
	Git Mailing List <git@vger.kernel.org>,
	Gustavo Leite <gustavoleite.ti@gmail.com>,
	Igor Djordjevic <igor.d.djordjevic@gmail.com>,
	Fernando Vezzosi <fv@repnz.net>, Jeff King <peff@peff.net>
Subject: Re: [PATCH v3 0/3] add -p: select individual hunk lines
Date: Sat, 31 Mar 2018 21:20:09 +0200
Message-ID: <878ta8vyqe.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <a8dd262b-8b0c-8632-bf28-e03f9405317f@talktalk.net>


On Fri, Mar 30 2018, Phillip Wood wrote:

> On 29/03/18 19:32, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Since v2 I've updated the patches to use '-' instead of '^' to invert
>>> the selection to match the rest of add -i and clean -i.
>>>
>>> These patches build on top of the recount fixes in [1]. The commit
>>> message for the first patch describes the motivation:
>>>
>>> "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 haven't seen any review comments on this round, and as I am not a
>> heavy user of "add -i" interface (even though I admit that I
>> originally wrote it), I haven't had a chance to exercise the code
>> myself in the two weeks since the patches have been queued in my
>> tree.
>>
>> I am inclihned to merge them to 'next' soonish, but please stop me
>> if anybody (including the original author) has further comments.
>>
>> Thanks.
>>
> Hi Junio, if no one else has any comments, then I think it's ready for
> next. I've not used this latest incarnation much but I've used the
> previous versions quite a bit.

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.

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-19 11:36 [PATCH v1 " 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 [this message]
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
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=878ta8vyqe.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=fv@repnz.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=gustavoleite.ti@gmail.com \
    --cc=igor.d.djordjevic@gmail.com \
    --cc=peff@peff.net \
    --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