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

[Ævar Arnfjörð Bjarmason]

On Mon, Apr 02 2018, Phillip Wood wrote:

> On 31/03/18 20:20, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Mar 30 2018, Phillip Wood wrote:
>>
>>> On 29/03/18 19:32, Junio C Hamano wrote:
 Phillip Wood  writes:

> From: Phillip Wood 
>
> 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.
>
> Ah it seems I spoke too soon.
>
> Thanks for taking a look at this Ævar
>
>> 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.
>
> Right, I'll use lexical variables.
>
>>
>> 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.:
>
> I've used it for selecting a subset of additions or deletions when my
> work has run ahead of a logical commit boundary. I've also used it in
> cases such as
>
>   -original
>   +modified
>   +new stuff
>
> To separate the modification from the addition of new stuff, but I've
> not used it on a list of modifications as in your example.

Right. I was wrong in saying that it wouldn't work as expected for hunks
with removed/added lines, but only for a subset of those cases.

>> $ 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-.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-` 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-.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-` 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" 

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

[Phillip Wood]

On 31/03/18 20:20, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 30 2018, Phillip Wood wrote:
> 
>> On 29/03/18 19:32, Junio C Hamano wrote:
>>> Phillip Wood  writes:
>>>
 From: Phillip Wood 

 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.

Ah it seems I spoke too soon.

Thanks for taking a look at this Ævar

> 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.

Right, I'll use lexical variables.

> 
> 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.:

I've used it for selecting a subset of additions or deletions when my
work has run ahead of a logical commit boundary. I've also used it in
cases such as

-original
+modified
+new stuff

To separate the modification from the addition of new stuff, but I've
not used it on a list of modifications as in your example.

> $ 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-.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-` 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-.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-` 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
>  

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

[Ævar Arnfjörð Bjarmason]

On Fri, Mar 30 2018, Phillip Wood wrote:

> On 29/03/18 19:32, Junio C Hamano wrote:
>> Phillip Wood  writes:
>>
>>> From: Phillip Wood 
>>>
>>> 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-.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-` 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-.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-` 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-.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-` or `git help
 `.

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-.txt for documentation of each command.
 4 -If git has been correctly 

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

[Phillip Wood]

On 29/03/18 19:32, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> From: Phillip Wood 
>>
>> 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.

Best Wishes

Phillip

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

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> 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.

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

[Phillip Wood]

From: Phillip Wood 

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'."

[1] 
https://public-inbox.org/git/xmqqbmg29x1n@gitster-ct.c.googlers.com/T/#m01d0f1af90f32b698e583b56f8e53b986bcec7c6

Interdiff to v2:
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d52acfc722..f3c81dfb11 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -337,13 +337,14 @@ patch::
e - manually edit the current hunk
? - print help
 +
-If you press "l" then the hunk will be reprinted with each insertion
-or deletion labelled with a number and you will be prompted to enter
-which lines you wish to select. Individual line numbers should be
-separated by a space or comma (these can be omitted if there are fewer
-than ten labelled lines), to specify a range of lines use a dash
-between them. To invert the selection prefix it with "\^" so "^3-5,8"
-will select everything except lines 3, 4, 5 and 8.
+If you press "l" then the hunk will be reprinted with each insertion or
+deletion labelled with a number and you will be prompted to enter which
+lines you wish to select. Individual line numbers should be separated by
+a space or comma (these can be omitted if there are fewer than ten
+labelled lines), to specify a range of lines use a dash between them. If
+the upper bound of a range of lines is omitted it defaults to the last
+line. To invert the selection prefix it with "-" so "-3-5,8" will select
+everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 9a6bcd5085..d65ad7c26d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1087,10 +1087,6 @@ sub split_hunk_selection {
my @fields = @_;
my @ret;
for (@fields) {
-   if (/^(-[0-9])(.*)/) {
-   push @ret, $1;
-   $_ = $2;
-   }
while ($_ ne '') {
if (/^[0-9]-$/) {
push @ret, $_;
@@ -1115,7 +,7 @@ sub parse_hunk_selection {
my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
my @selected = (0) x ($max_label + 1);
my @fields = split(/[,\s]+/, $line);
-   if ($fields[0] =~ /^\^(.*)/) {
+   if ($fields[0] =~ /^-(.*)/) {
$invert = 1;
if ($1 ne '') {
$fields[0] = $1;
@@ -1131,13 +1127,10 @@ sub parse_hunk_selection {
@fields = split_hunk_selection(@fields) or return undef;
}
for (@fields) {
-   if (/^([0-9]*)-([0-9]*)$/) {
-   if ($1 eq '' and $2 eq '') {
-   error_msg __("range '-' missing upper or lower 
bound\n");
-   return undef;
+   if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) {
+   if ($hi eq '') {
+   $hi = $max_label;
}
-   my $lo = $1 eq '' ? 1 : $1;
-   my $hi = $2 eq '' ? $max_label : $2;
check_hunk_label($hunk, $lo) or return undef;
check_hunk_label($hunk, $hi) or return undef;
if ($hi < $lo) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d3bce154da..467c6eff0e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -383,7 +383,7 @@ test_expect_success 'setup expected diff' '
 test_expect_success 'can stage individual lines of patch' '
git reset &&
printf 61 >>test &&
-   printf "%s\n" l "-2 4" |
+   printf "%s\n" l "1,2 4-" |
EDITOR=: git add -p 2>error &&
test_must_be_empty error &&
git diff --cached HEAD >actual &&
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-   printf "%s\n" l ^13 |
+   printf "%s\n" l