Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
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
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
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
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
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
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
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
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