Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Phillip Wood]

On 13/03/18 16:32, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> On 08/03/18 17:53, Junio C Hamano wrote:
>>> Phillip Wood  writes:
>>>
 and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
 to mean everything from 'n' to the last line though.
>>>
>>> Thanks for double checking.  It would be a better endgame to follow
>>> up with an update to existing "range selection" code to also support
>>> "n-", if you go that route.
>>>
>> I'm afraid I'm not sure exactly what you're suggesting. At the moment
>> the range selection code is in the first patch and supports incomplete
>> ranges. Are you suggesting that support for incomplete ranges should be
>> in a separate patch or have I misunderstood?
> 
> My observation of the situation behind my reasoning is:
> 
>  - There is an existing UI that uses "-X" to mean "exclude what
>matches X" and that was the reason why you decided to follow suit
>instead of using "^X" for inversion of X.
> 
>  - Such an existing UI would not have used "-X" to mean "the first
>possible choice thru X".  You will lose that from your new thing
>and you accepted that.
> 
>  - It is likely (I did not check, though) that the existing UI would
>not have used "Y-" to mean "starting from Y all the possible
>choices thru to the end", but that is merely for symmetry with
>the lack (inability to use) of "-X".  There is no fundamental
>reason why "Y-" cannot mean that, and you are tempted to allow do
>so in your new thing for the same reason.
> 
> So if we are going to have "N-" to mean "everything from N to the
> last line", then the same "Starting at N to the end of the all the
> possible choices" should be allowed in the existing UI (i.e. the one
> that forced you to give up "^X" for the sake of consistency) for the
> same consistency reasons, no?
> 
> For that, if you want to keep the "n-" you did in your first patch,
> the most logical thing is to have a preparatory enhancement to teach
> "N-" to list_and_choose(), and then build your series on top.  Or
> you can do without such a change to list_and_choose() in your series,
> in which case, you drop "n-" support and then at the very end after
> the series settles, add "n-" support to the new code in this series
> and to list_and_choose() at the same time in a follow-up patch.
> 
> 
Thanks for taking the time to clarify that, I hadn't twigged you were
talking about changing list_and_choose() before. I think it would make
sense for that and 'git clean' to match the line selection with "n-". I
just opened it up my editor to do that and was pleasantly surprised to
discover that "n-" is already implemented for both list_and_choose() and
'git clean' so there's nothing that needs doing on that front. I'll
reroll this series with the other changes and send it later this week or
next week

Best Wishes

Phillip

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Junio C Hamano]

Phillip Wood  writes:

> On 08/03/18 17:53, Junio C Hamano wrote:
>> Phillip Wood  writes:
>> 
>>> and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
>>> to mean everything from 'n' to the last line though.
>> 
>> Thanks for double checking.  It would be a better endgame to follow
>> up with an update to existing "range selection" code to also support
>> "n-", if you go that route.
>> 
> I'm afraid I'm not sure exactly what you're suggesting. At the moment
> the range selection code is in the first patch and supports incomplete
> ranges. Are you suggesting that support for incomplete ranges should be
> in a separate patch or have I misunderstood?

My observation of the situation behind my reasoning is:

 - There is an existing UI that uses "-X" to mean "exclude what
   matches X" and that was the reason why you decided to follow suit
   instead of using "^X" for inversion of X.

 - Such an existing UI would not have used "-X" to mean "the first
   possible choice thru X".  You will lose that from your new thing
   and you accepted that.

 - It is likely (I did not check, though) that the existing UI would
   not have used "Y-" to mean "starting from Y all the possible
   choices thru to the end", but that is merely for symmetry with
   the lack (inability to use) of "-X".  There is no fundamental
   reason why "Y-" cannot mean that, and you are tempted to allow do
   so in your new thing for the same reason.

So if we are going to have "N-" to mean "everything from N to the
last line", then the same "Starting at N to the end of the all the
possible choices" should be allowed in the existing UI (i.e. the one
that forced you to give up "^X" for the sake of consistency) for the
same consistency reasons, no?

For that, if you want to keep the "n-" you did in your first patch,
the most logical thing is to have a preparatory enhancement to teach
"N-" to list_and_choose(), and then build your series on top.  Or
you can do without such a change to list_and_choose() in your series,
in which case, you drop "n-" support and then at the very end after
the series settles, add "n-" support to the new code in this series
and to list_and_choose() at the same time in a follow-up patch.

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Phillip Wood]

On 08/03/18 17:53, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
>> to mean everything from 'n' to the last line though.
> 
> Thanks for double checking.  It would be a better endgame to follow
> up with an update to existing "range selection" code to also support
> "n-", if you go that route.
> 
I'm afraid I'm not sure exactly what you're suggesting. At the moment
the range selection code is in the first patch and supports incomplete
ranges. Are you suggesting that support for incomplete ranges should be
in a separate patch or have I misunderstood?

Thanks

Phillip

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Junio C Hamano]

Phillip Wood  writes:

> and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
> to mean everything from 'n' to the last line though.

Thanks for double checking.  It would be a better endgame to follow
up with an update to existing "range selection" code to also support
"n-", if you go that route.

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Phillip Wood]

On 06/03/18 19:57, Junio C Hamano wrote:
> Phillip Wood  writes:
> 
>> From: Phillip Wood 
>>
>> If the list of lines to be selected begins with '^' select all the
>> lines except the ones listed.
> 
> There is "# Input that begins with '-'; unchoose" in list_and_choose
> helper.  Does it make things inconsistent to use '^' for negation
> like this patch does with it?
> 
Hmm yes, I think it probably does (I've just checked and git clean also
uses '-' for de-selection). I think I'll remove support for open-ended
ranges on the left side (it's not so hard to type '1-n' instead of '-n')
and use a leading '-' for inversion. I'm tempted to keep supporting 'n-'
to mean everything from 'n' to the last line though.

Best Wishes

Phillip

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Igor Djordjevic]

On 06/03/2018 11:17, Phillip Wood wrote:
> From: Phillip Wood 
> 
> If the list of lines to be selected begins with '^' select all the
> lines except the ones listed.

s/to be selected begins with '^' select all/to be staged begins with '^' stage 
all/

> 
> Signed-off-by: Phillip Wood 
> ---
>  Documentation/git-add.txt  |  3 ++-
>  git-add--interactive.perl  | 17 -
>  t/t3701-add-interactive.sh |  2 +-
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index ad33fda9a2..0e2c11e97b 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -341,7 +341,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, to specify a range of lines use a dash
> -between them.
> +between them. To invert the selection prefix it with "\^" so "^3-5,8"
> +will select everything except lines 3, 4, 5 and 8.

Hmm, here, first "selection" seems to make sense as it is (I guess),
but might still be better to later say 
s/will select everything/will stage everything/ ...?

That said, might be "to invert the selection" could rather be "to unstage," 
instead? Not sure, though.

>  +
>  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 a273b41e95..6fa3d0a87c 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1085,9 +1085,21 @@ sub check_hunk_label {
>  sub parse_hunk_selection {
>   local $_;
>   my ($hunk, $line) = @_;
> - my $max_label = $hunk->{MAX_LABEL};
> + my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
>   my @selected = (0) x ($max_label + 1);
>   my @fields = split(/[,\s]+/, $line);
> + if ($fields[0] =~ /^\^(.*)/) {
> + $invert = 1;
> + if ($1 ne '') {
> + $fields[0] = $1;
> + } else {
> + shift @fields;
> + unless (@fields) {
> + error_msg __("no lines to invert\n");
> + return undef;
> + }
> + }
> + }
>   for (@fields) {
>   if (/^([0-9]*)-([0-9]*)$/) {
>   if ($1 eq '' and $2 eq '') {
> @@ -1110,6 +1122,9 @@ sub parse_hunk_selection {
>   return undef;
>   }
>   }
> + if ($invert) {
> + @selected = map { !$_ } @selected;
> + }
>   return \@selected;
>  }
>  
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 65c8c3354b..89c0e73f2b 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
>  '
>  
>  test_expect_success 'can reset individual lines of patch' '
> - printf "%s\n" l 2 |
> + printf "%s\n" l "^1 3" |
>   EDITOR=: git reset -p 2>error &&
>   test_must_be_empty error &&
>   git diff --cached HEAD >actual &&
> 

Re: [PATCH v2 2/3] add -p: allow line selection to be inverted

[Junio C Hamano]

Phillip Wood  writes:

> From: Phillip Wood 
>
> If the list of lines to be selected begins with '^' select all the
> lines except the ones listed.

There is "# Input that begins with '-'; unchoose" in list_and_choose
helper.  Does it make things inconsistent to use '^' for negation
like this patch does with it?

[PATCH v2 2/3] add -p: allow line selection to be inverted

[Phillip Wood]

From: Phillip Wood 

If the list of lines to be selected begins with '^' select all the
lines except the ones listed.

Signed-off-by: Phillip Wood 
---
 Documentation/git-add.txt  |  3 ++-
 git-add--interactive.perl  | 17 -
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index ad33fda9a2..0e2c11e97b 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -341,7 +341,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, to specify a range of lines use a dash
-between them.
+between them. 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 a273b41e95..6fa3d0a87c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1085,9 +1085,21 @@ sub check_hunk_label {
 sub parse_hunk_selection {
local $_;
my ($hunk, $line) = @_;
-   my $max_label = $hunk->{MAX_LABEL};
+   my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
my @selected = (0) x ($max_label + 1);
my @fields = split(/[,\s]+/, $line);
+   if ($fields[0] =~ /^\^(.*)/) {
+   $invert = 1;
+   if ($1 ne '') {
+   $fields[0] = $1;
+   } else {
+   shift @fields;
+   unless (@fields) {
+   error_msg __("no lines to invert\n");
+   return undef;
+   }
+   }
+   }
for (@fields) {
if (/^([0-9]*)-([0-9]*)$/) {
if ($1 eq '' and $2 eq '') {
@@ -1110,6 +1122,9 @@ sub parse_hunk_selection {
return undef;
}
}
+   if ($invert) {
+   @selected = map { !$_ } @selected;
+   }
return \@selected;
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 65c8c3354b..89c0e73f2b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-   printf "%s\n" l 2 |
+   printf "%s\n" l "^1 3" |
EDITOR=: git reset -p 2>error &&
test_must_be_empty error &&
git diff --cached HEAD >actual &&
-- 
2.16.2