* [PATCH v1 0/3] add -p: select individual hunk lines
@ 2018-02-19 11:36 Phillip Wood
2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
` (7 more replies)
0 siblings, 8 replies; 53+ messages in thread
From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw)
To: Git Mailing List; +Cc: Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
I need to update the add -i documentation but otherwise I think these
patches are OK so I thought I'd try and get some feedback. They 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/20180219112910.24471-1-phillip.wood@talktalk.net/T/#u
Phillip Wood (3):
add -p: select individual hunk lines
add -p: allow line selection to be inverted
add -p: optimize line selection for short hunks
git-add--interactive.perl | 179 +++++++++++++++++++++++++++++++++++++++++++++
t/t3701-add-interactive.sh | 63 ++++++++++++++++
2 files changed, 242 insertions(+)
--
2.16.1
^ permalink raw reply [flat|nested] 53+ messages in thread* [PATCH v1 1/3] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood @ 2018-02-19 11:36 ` Phillip Wood 2018-02-19 11:36 ` [PATCH v1 2/3] add -p: allow line selection to be inverted Phillip Wood ` (6 subsequent siblings) 7 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw) To: Git Mailing List; +Cc: Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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'. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 136 +++++++++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 63 +++++++++++++++++++++ 2 files changed, 199 insertions(+) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index a64c0db57d62ab02ef718b8c8f821105132d9920..8a33796e1f6a564d0a27ba06c216dbb9848827b9 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1003,6 +1003,126 @@ sub color_diff { } @_; } +sub label_hunk_lines { + local $_; + my $hunk = shift; + my $i = 0; + my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ]; + $i > 1 and @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); + return $i > 1; +} + +sub select_hunk_lines { + my ($hunk, $selected) = @_; + my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)}; + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); + my ($push_eol, @newtext); + # Lines with this mode will become context lines if they are + # not selected + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; + for $i (1..$#{$text}) { + my $mode = substr($text->[$i], 0, 1); + if ($mode eq '\\') { + push @newtext, $text->[$i] if ($push_eol); + undef $push_eol; + } elsif ($labels->[$i] and $selected->[$labels->[$i]]) { + push @newtext, $text->[$i]; + if ($mode eq '+') { + $n_cnt++; + } else { + $o_cnt++; + } + $push_eol = 1; + } elsif ($mode eq ' ' or $mode eq $context_mode) { + push @newtext, ' ' . substr($text->[$i], 1); + $o_cnt++; $n_cnt++; + $push_eol = 1; + } else { + undef $push_eol; + } + } + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = + parse_hunk_header($text->[0]); + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); + my $newhunk = { + TEXT => \@newtext, + DISPLAY => [ color_diff(@newtext) ], + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, + TYPE => $hunk->{TYPE}, + USE => 1, + }; + # If this hunk has previously been edited add the offset delta + # of the old hunk to get the real delta from the original + # hunk. + $hunk->{OFS_DELTA} and $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + return $newhunk; +} + +sub check_hunk_label { + my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]); + if ($label < 1 or $label > $max_label) { + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); + return 0; + } + return 1; +} + +sub parse_hunk_selection { + local $_; + my ($hunk, $line) = @_; + my $max_label = $hunk->{MAX_LABEL}; + my @selected = (0) x ($max_label + 1); + my @fields = split(/[,\s]+/, $line); + for (@fields) { + if (/^([0-9]*)-([0-9]*)$/) { + if ($1 eq '' and $2 eq '') { + error_msg __("range '-' missing upper or lower bound\n"); + return undef; + } + 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; + $hi < $lo and ($lo, $hi) = ($hi, $lo); + @selected[$lo..$hi] = (1) x (1 + $hi - $lo); + } elsif (/^([0-9]+)$/) { + check_hunk_label($hunk, $1) or return undef; + $selected[$1] = 1; + } else { + error_msg sprintf(__("invalid hunk line '%s'\n"), $_); + return undef; + } + } + return \@selected; +} + +sub display_hunk_lines { + my ($display, $labels, $max_label) = + @{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)}; + my $width = int(log($max_label) / log(10)) + 1; + my $padding = ' ' x ($width + 1); + for my $i (0..$#{$display}) { + if ($labels->[$i]) { + printf '%*d %s', $width, $labels->[$i], $display->[$i]; + } else { + print $padding . $display->[$i]; + } + } +} + +sub select_lines_loop { + my $hunk = shift; + display_hunk_lines($hunk); + my $selection = undef; + until (defined $selection) { + print colored $prompt_color, __("select lines? "); + my $text = <STDIN>; + defined $text and $text =~ /\S/ or return undef; + $selection = parse_hunk_selection($hunk, $text); + } + return select_hunk_lines($hunk, $selection); +} + my %edit_hunk_manually_modes = ( stage => N__( "If the patch applies cleanly, the edited hunk will immediately be @@ -1245,6 +1365,7 @@ j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk +l - select hunk lines to use s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help @@ -1461,6 +1582,9 @@ sub patch_update_file { if ($hunk[$ix]{TYPE} eq 'hunk') { $other .= ',e'; } + if (label_hunk_lines($hunk[$ix])) { + $other .= ',l'; + } for (@{$hunk[$ix]{DISPLAY}}) { print; } @@ -1600,6 +1724,18 @@ sub patch_update_file { next; } } + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); if (1 < @split) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 0fb9c0e0f140e21ef7ad467c40b9211d29f53db6..caa80327c461785949eb2b9c919c253f4bef72cc 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -342,6 +342,61 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + --- a/test + +++ b/test + @@ -1,6 +1,9 @@ + +5 + 10 + 20 + +21 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage individual lines of patch' ' + git reset && + printf 61 >>test && + printf "%s\n" l "-2 4" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD | sed /^index/d >actual && + test_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + +5 + 10 + 20 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can reset individual lines of patch' ' + printf "%s\n" l 2 | + EDITOR=: git reset -p 2>error && + test_must_be_empty error && + git diff --cached HEAD | sed /^index/d >actual && + test_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && @@ -558,4 +613,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'add -p selecting lines works with pathological context lines' ' + git reset && + printf "%s\n" l 2 y | + GIT_EDITOR=./editor git add -p && + git cat-file blob :a > actual && + test_cmp expected-2 actual +' + test_done -- 2.16.1 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v1 2/3] add -p: allow line selection to be inverted 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood 2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood @ 2018-02-19 11:36 ` Phillip Wood 2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood ` (5 subsequent siblings) 7 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw) To: Git Mailing List; +Cc: Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If the list of lines to be selected begins with '^' select all the lines except the ones listed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 15 ++++++++++++++- t/t3701-add-interactive.sh | 2 +- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 8a33796e1f6a564d0a27ba06c216dbb9848827b9..0e3960b1ecf004bff51d28d540f685a5dc91fad1 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1070,9 +1070,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 '') { @@ -1093,6 +1105,7 @@ sub parse_hunk_selection { return undef; } } + $invert and @selected = map { !$_ } @selected; return \@selected; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index caa80327c461785949eb2b9c919c253f4bef72cc..4ae706fd121f157e9cbd93ec293f45ce2a3a53b5 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -390,7 +390,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 | sed /^index/d >actual && -- 2.16.1 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v1 3/3] add -p: optimize line selection for short hunks 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines 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 ` Phillip Wood 2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite ` (4 subsequent siblings) 7 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw) To: Git Mailing List; +Cc: Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If there are fewer than ten changes in a hunk then make spaces optional when selecting individual lines. This means that for short hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 30 ++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 0e3960b1ecf004bff51d28d540f685a5dc91fad1..3d9720af03eb113c7f3d2b73f17b4b51a7685bf3 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1067,6 +1067,33 @@ sub check_hunk_label { return 1; } +sub split_hunk_selection { + local $_; + my @fields = @_; + my @ret; + for (@fields) { + if (/^(-[0-9])(.*)/) { + push @ret, $1; + $_ = $2; + } + while ($_ ne '') { + if (/^[0-9]-$/) { + push @ret, $_; + last; + } elsif (/^([0-9](?:-[0-9])?)(.*)/) { + push @ret, $1; + $_ = $2; + } else { + error_msg sprintf + __("invalid hunk line '%s'\n"), + substr($_, 0, 1); + return (); + } + } + } + return @ret; +} + sub parse_hunk_selection { local $_; my ($hunk, $line) = @_; @@ -1085,6 +1112,9 @@ sub parse_hunk_selection { } } } + if ($max_label < 10) { + @fields = split_hunk_selection(@fields) or return undef; + } for (@fields) { if (/^([0-9]*)-([0-9]*)$/) { if ($1 eq '' and $2 eq '') { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 4ae706fd121f157e9cbd93ec293f45ce2a3a53b5..c6d847dc495c92782e37ef7b0e2800d7936aabd7 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -390,7 +390,7 @@ test_expect_success 'setup expected diff' ' ' test_expect_success 'can reset individual lines of patch' ' - printf "%s\n" l "^1 3" | + printf "%s\n" l ^13 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD | sed /^index/d >actual && -- 2.16.1 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v1 0/3] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood ` (2 preceding siblings ...) 2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood @ 2018-02-19 12:20 ` Gustavo Leite 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood ` (3 subsequent siblings) 7 siblings, 0 replies; 53+ messages in thread From: Gustavo Leite @ 2018-02-19 12:20 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List 2018-02-19 8:36 GMT-03:00 Phillip Wood <phillip.wood@talktalk.net>: > > "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'." This is an interesting (and needed feature). Would be nice to see it merged. -- Gustavo Leite ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 0/3] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood ` (3 preceding siblings ...) 2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite @ 2018-03-06 10:17 ` Phillip Wood 2018-03-06 10:17 ` [PATCH v2 1/3] " Phillip Wood ` (3 more replies) 2018-03-16 10:13 ` [PATCH v3 " Phillip Wood ` (2 subsequent siblings) 7 siblings, 4 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> I've added some documentation to git-add.txt for the new selection mode and cleaned up some style issues, otherwise these are unchanged since v1. 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.fsf@gitster-ct.c.googlers.com/T/#m01d0f1af90f32b698e583b56f8e53b986bcec7c6 Interdiff from v1: Documentation/git-add.txt | 9 +++++++++ git-add--interactive.perl | 19 ++++++++++++++----- t/t3701-add-interactive.sh | 12 +++++++----- 3 files changed, 30 insertions(+), 10 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index d50fa339dc..d52acfc722 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -332,10 +332,19 @@ patch:: J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk + l - select hunk lines to use s - split the current hunk into smaller hunks 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. ++ 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 aa474c5149..9a6bcd5085 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1018,8 +1018,11 @@ sub label_hunk_lines { my $hunk = shift; my $i = 0; my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ]; - $i > 1 and @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); - return $i > 1; + if ($i > 1) { + @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); + return 1; + } + return 0; } sub select_hunk_lines { @@ -1064,7 +1067,9 @@ sub select_hunk_lines { # If this hunk has previously been edited add the offset delta # of the old hunk to get the real delta from the original # hunk. - $hunk->{OFS_DELTA} and $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + if ($hunk->{OFS_DELTA}) { + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + } return $newhunk; } @@ -1135,7 +1140,9 @@ sub parse_hunk_selection { my $hi = $2 eq '' ? $max_label : $2; check_hunk_label($hunk, $lo) or return undef; check_hunk_label($hunk, $hi) or return undef; - $hi < $lo and ($lo, $hi) = ($hi, $lo); + if ($hi < $lo) { + ($lo, $hi) = ($hi, $lo); + } @selected[$lo..$hi] = (1) x (1 + $hi - $lo); } elsif (/^([0-9]+)$/) { check_hunk_label($hunk, $1) or return undef; @@ -1145,7 +1152,9 @@ sub parse_hunk_selection { return undef; } } - $invert and @selected = map { !$_ } @selected; + if ($invert) { + @selected = map { !$_ } @selected; + } return \@selected; } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 7bea4a2341..d3bce154da 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -363,6 +363,7 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' test_expect_success 'setup expected diff' ' cat >expected <<-\EOF diff --git a/test b/test + index 0889435..341cc6b 100644 --- a/test +++ b/test @@ -1,6 +1,9 @@ @@ -385,13 +386,14 @@ test_expect_success 'can stage individual lines of patch' ' printf "%s\n" l "-2 4" | EDITOR=: git add -p 2>error && test_must_be_empty error && - git diff --cached HEAD | sed /^index/d >actual && - test_cmp expected actual + git diff --cached HEAD >actual && + diff_cmp expected actual ' test_expect_success 'setup expected diff' ' cat >expected <<-\EOF diff --git a/test b/test + index 0889435..cc6163b 100644 --- a/test +++ b/test @@ -1,6 +1,8 @@ @@ -411,8 +413,8 @@ test_expect_success 'can reset individual lines of patch' ' printf "%s\n" l ^13 | EDITOR=: git reset -p 2>error && test_must_be_empty error && - git diff --cached HEAD | sed /^index/d >actual && - test_cmp expected actual + git diff --cached HEAD >actual && + diff_cmp expected actual ' test_expect_success 'patch mode ignores unmerged entries' ' @@ -635,7 +637,7 @@ test_expect_success 'add -p selecting lines works with pathological context line git reset && printf "%s\n" l 2 y | GIT_EDITOR=./editor git add -p && - git cat-file blob :a > actual && + git cat-file blob :a >actual && test_cmp expected-2 actual ' Phillip Wood (3): add -p: select individual hunk lines add -p: allow line selection to be inverted add -p: optimize line selection for short hunks Documentation/git-add.txt | 9 +++ git-add--interactive.perl | 188 +++++++++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 65 ++++++++++++++++ 3 files changed, 262 insertions(+) -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v2 1/3] add -p: select individual hunk lines 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood @ 2018-03-06 10:17 ` Phillip Wood 2018-03-06 20:29 ` Igor Djordjevic 2018-03-06 10:17 ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood ` (2 subsequent siblings) 3 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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'. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 7 +++ git-add--interactive.perl | 143 +++++++++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 65 +++++++++++++++++++++ 3 files changed, 215 insertions(+) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index d50fa339dc..ad33fda9a2 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -332,10 +332,17 @@ patch:: J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk + l - select hunk lines to use s - split the current hunk into smaller hunks 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, to specify a range of lines use a dash +between them. ++ 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 f83e7450ad..a273b41e95 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1013,6 +1013,133 @@ sub color_diff { } @_; } +sub label_hunk_lines { + local $_; + my $hunk = shift; + my $i = 0; + my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ]; + if ($i > 1) { + @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); + return 1; + } + return 0; +} + +sub select_hunk_lines { + my ($hunk, $selected) = @_; + my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)}; + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); + my ($push_eol, @newtext); + # Lines with this mode will become context lines if they are + # not selected + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; + for $i (1..$#{$text}) { + my $mode = substr($text->[$i], 0, 1); + if ($mode eq '\\') { + push @newtext, $text->[$i] if ($push_eol); + undef $push_eol; + } elsif ($labels->[$i] and $selected->[$labels->[$i]]) { + push @newtext, $text->[$i]; + if ($mode eq '+') { + $n_cnt++; + } else { + $o_cnt++; + } + $push_eol = 1; + } elsif ($mode eq ' ' or $mode eq $context_mode) { + push @newtext, ' ' . substr($text->[$i], 1); + $o_cnt++; $n_cnt++; + $push_eol = 1; + } else { + undef $push_eol; + } + } + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = + parse_hunk_header($text->[0]); + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); + my $newhunk = { + TEXT => \@newtext, + DISPLAY => [ color_diff(@newtext) ], + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, + TYPE => $hunk->{TYPE}, + USE => 1, + }; + # If this hunk has previously been edited add the offset delta + # of the old hunk to get the real delta from the original + # hunk. + if ($hunk->{OFS_DELTA}) { + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + } + return $newhunk; +} + +sub check_hunk_label { + my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]); + if ($label < 1 or $label > $max_label) { + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); + return 0; + } + return 1; +} + +sub parse_hunk_selection { + local $_; + my ($hunk, $line) = @_; + my $max_label = $hunk->{MAX_LABEL}; + my @selected = (0) x ($max_label + 1); + my @fields = split(/[,\s]+/, $line); + for (@fields) { + if (/^([0-9]*)-([0-9]*)$/) { + if ($1 eq '' and $2 eq '') { + error_msg __("range '-' missing upper or lower bound\n"); + return undef; + } + 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) { + ($lo, $hi) = ($hi, $lo); + } + @selected[$lo..$hi] = (1) x (1 + $hi - $lo); + } elsif (/^([0-9]+)$/) { + check_hunk_label($hunk, $1) or return undef; + $selected[$1] = 1; + } else { + error_msg sprintf(__("invalid hunk line '%s'\n"), $_); + return undef; + } + } + return \@selected; +} + +sub display_hunk_lines { + my ($display, $labels, $max_label) = + @{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)}; + my $width = int(log($max_label) / log(10)) + 1; + my $padding = ' ' x ($width + 1); + for my $i (0..$#{$display}) { + if ($labels->[$i]) { + printf '%*d %s', $width, $labels->[$i], $display->[$i]; + } else { + print $padding . $display->[$i]; + } + } +} + +sub select_lines_loop { + my $hunk = shift; + display_hunk_lines($hunk); + my $selection = undef; + until (defined $selection) { + print colored $prompt_color, __("select lines? "); + my $text = <STDIN>; + defined $text and $text =~ /\S/ or return undef; + $selection = parse_hunk_selection($hunk, $text); + } + return select_hunk_lines($hunk, $selection); +} + my %edit_hunk_manually_modes = ( stage => N__( "If the patch applies cleanly, the edited hunk will immediately be @@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk +l - select hunk lines to use s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help @@ -1471,6 +1599,9 @@ sub patch_update_file { if ($hunk[$ix]{TYPE} eq 'hunk') { $other .= ',e'; } + if (label_hunk_lines($hunk[$ix])) { + $other .= ',l'; + } for (@{$hunk[$ix]{DISPLAY}}) { print; } @@ -1610,6 +1741,18 @@ sub patch_update_file { next; } } + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); if (1 < @split) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index a9a9478a29..65c8c3354b 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..341cc6b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,9 @@ + +5 + 10 + 20 + +21 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage individual lines of patch' ' + git reset && + printf 61 >>test && + printf "%s\n" l "-2 4" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + +5 + 10 + 20 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can reset individual lines of patch' ' + printf "%s\n" l 2 | + EDITOR=: git reset -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && @@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'add -p selecting lines works with pathological context lines' ' + git reset && + printf "%s\n" l 2 y | + GIT_EDITOR=./editor git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 1/3] add -p: select individual hunk lines 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 0 siblings, 1 reply; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 20:29 UTC (permalink / raw) To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite On 06/03/2018 11:17, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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'. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > Documentation/git-add.txt | 7 +++ > git-add--interactive.perl | 143 +++++++++++++++++++++++++++++++++++++++++++++ > t/t3701-add-interactive.sh | 65 +++++++++++++++++++++ > 3 files changed, 215 insertions(+) > > diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt > index d50fa339dc..ad33fda9a2 100644 > --- a/Documentation/git-add.txt > +++ b/Documentation/git-add.txt > @@ -332,10 +332,17 @@ patch:: > J - leave this hunk undecided, see next hunk > k - leave this hunk undecided, see previous undecided hunk > K - leave this hunk undecided, see previous hunk > + l - select hunk lines to use Might be more surrounding context aligned to say "stage hunk lines" here (phrase "stage", instead of "select to use"). > s - split the current hunk into smaller hunks > 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 Likewise, s/you wish to select/you wish to stage/. > +separated by a space or comma, to specify a range of lines use a dash > +between them. > ++ > 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 f83e7450ad..a273b41e95 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -1013,6 +1013,133 @@ sub color_diff { > } @_; > } > > +sub label_hunk_lines { > + local $_; > + my $hunk = shift; > + my $i = 0; > + my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ]; > + if ($i > 1) { > + @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); > + return 1; > + } > + return 0; > +} > + > +sub select_hunk_lines { This is just something I`ve spotted, but I have no actual idea if renaming this to "stage_hunk_lines" might be better, too, or not (depending on the surrounding code context), so please take this with a big grain of salt. > + my ($hunk, $selected) = @_; > + my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)}; > + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); > + my ($push_eol, @newtext); > + # Lines with this mode will become context lines if they are > + # not selected > + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; > + for $i (1..$#{$text}) { > + my $mode = substr($text->[$i], 0, 1); > + if ($mode eq '\\') { > + push @newtext, $text->[$i] if ($push_eol); > + undef $push_eol; > + } elsif ($labels->[$i] and $selected->[$labels->[$i]]) { > + push @newtext, $text->[$i]; > + if ($mode eq '+') { > + $n_cnt++; > + } else { > + $o_cnt++; > + } > + $push_eol = 1; > + } elsif ($mode eq ' ' or $mode eq $context_mode) { > + push @newtext, ' ' . substr($text->[$i], 1); > + $o_cnt++; $n_cnt++; > + $push_eol = 1; > + } else { > + undef $push_eol; > + } > + } > + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = > + parse_hunk_header($text->[0]); > + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); > + my $newhunk = { > + TEXT => \@newtext, > + DISPLAY => [ color_diff(@newtext) ], > + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, > + TYPE => $hunk->{TYPE}, > + USE => 1, > + }; > + # If this hunk has previously been edited add the offset delta > + # of the old hunk to get the real delta from the original > + # hunk. > + if ($hunk->{OFS_DELTA}) { > + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; > + } > + return $newhunk; > +} > + > +sub check_hunk_label { > + my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]); > + if ($label < 1 or $label > $max_label) { > + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); > + return 0; > + } > + return 1; > +} > + > +sub parse_hunk_selection { > + local $_; > + my ($hunk, $line) = @_; > + my $max_label = $hunk->{MAX_LABEL}; > + my @selected = (0) x ($max_label + 1); > + my @fields = split(/[,\s]+/, $line); > + for (@fields) { > + if (/^([0-9]*)-([0-9]*)$/) { > + if ($1 eq '' and $2 eq '') { > + error_msg __("range '-' missing upper or lower bound\n"); > + return undef; > + } > + 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) { > + ($lo, $hi) = ($hi, $lo); > + } > + @selected[$lo..$hi] = (1) x (1 + $hi - $lo); > + } elsif (/^([0-9]+)$/) { > + check_hunk_label($hunk, $1) or return undef; > + $selected[$1] = 1; > + } else { > + error_msg sprintf(__("invalid hunk line '%s'\n"), $_); > + return undef; > + } > + } > + return \@selected; > +} > + > +sub display_hunk_lines { > + my ($display, $labels, $max_label) = > + @{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)}; > + my $width = int(log($max_label) / log(10)) + 1; > + my $padding = ' ' x ($width + 1); > + for my $i (0..$#{$display}) { > + if ($labels->[$i]) { > + printf '%*d %s', $width, $labels->[$i], $display->[$i]; > + } else { > + print $padding . $display->[$i]; > + } > + } > +} > + > +sub select_lines_loop { > + my $hunk = shift; > + display_hunk_lines($hunk); > + my $selection = undef; > + until (defined $selection) { > + print colored $prompt_color, __("select lines? "); > + my $text = <STDIN>; > + defined $text and $text =~ /\S/ or return undef; > + $selection = parse_hunk_selection($hunk, $text); > + } > + return select_hunk_lines($hunk, $selection); > +} > + > my %edit_hunk_manually_modes = ( > stage => N__( > "If the patch applies cleanly, the edited hunk will immediately be > @@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk > J - leave this hunk undecided, see next hunk > k - leave this hunk undecided, see previous undecided hunk > K - leave this hunk undecided, see previous hunk > +l - select hunk lines to use s/select hunk lines to use/stage hunk lines/ > s - split the current hunk into smaller hunks > e - manually edit the current hunk > ? - print help > @@ -1471,6 +1599,9 @@ sub patch_update_file { > if ($hunk[$ix]{TYPE} eq 'hunk') { > $other .= ',e'; > } > + if (label_hunk_lines($hunk[$ix])) { > + $other .= ',l'; > + } > for (@{$hunk[$ix]{DISPLAY}}) { > print; > } > @@ -1610,6 +1741,18 @@ sub patch_update_file { > next; > } > } > + elsif ($line =~ /^l/) { > + unless ($other =~ /l/) { > + error_msg __("Cannot select line by line\n"); > + next; > + } > + my $newhunk = select_lines_loop($hunk[$ix]); > + if ($newhunk) { > + splice @hunk, $ix, 1, $newhunk; > + } else { > + next; > + } > + } > elsif ($other =~ /s/ && $line =~ /^s/) { > my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); > if (1 < @split) { > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index a9a9478a29..65c8c3354b 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' > ! grep "^+31" actual > ' > > +test_expect_success 'setup expected diff' ' > + cat >expected <<-\EOF > + diff --git a/test b/test > + index 0889435..341cc6b 100644 > + --- a/test > + +++ b/test > + @@ -1,6 +1,9 @@ > + +5 > + 10 > + 20 > + +21 > + 30 > + 40 > + 50 > + 60 > + +61 > + \ No newline at end of file > + EOF > +' > + > +test_expect_success 'can stage individual lines of patch' ' Here, you`re actually using "stage lines" yourself, good ;) > + git reset && > + printf 61 >>test && > + printf "%s\n" l "-2 4" | > + EDITOR=: git add -p 2>error && > + test_must_be_empty error && > + git diff --cached HEAD >actual && > + diff_cmp expected actual > +' > + > +test_expect_success 'setup expected diff' ' > + cat >expected <<-\EOF > + diff --git a/test b/test > + index 0889435..cc6163b 100644 > + --- a/test > + +++ b/test > + @@ -1,6 +1,8 @@ > + +5 > + 10 > + 20 > + 30 > + 40 > + 50 > + 60 > + +61 > + \ No newline at end of file > + EOF > +' > + > +test_expect_success 'can reset individual lines of patch' ' > + printf "%s\n" l 2 | > + EDITOR=: git reset -p 2>error && > + test_must_be_empty error && > + git diff --cached HEAD >actual && > + diff_cmp expected actual > +' > + > test_expect_success 'patch mode ignores unmerged entries' ' > git reset --hard && > test_commit conflict && > @@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' > test_cmp expected-2 actual > ' > > +test_expect_success 'add -p selecting lines works with pathological context lines' ' Maybe s/selecting lines/staging lines/ ? > + git reset && > + printf "%s\n" l 2 y | > + GIT_EDITOR=./editor git add -p && > + git cat-file blob :a >actual && > + test_cmp expected-2 actual > +' > + > test_done > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 1/3] add -p: select individual hunk lines 2018-03-06 20:29 ` Igor Djordjevic @ 2018-03-06 21:33 ` Igor Djordjevic 0 siblings, 0 replies; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 21:33 UTC (permalink / raw) To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite On 06/03/2018 21:29, Igor Djordjevic wrote: > > > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > > index f83e7450ad..a273b41e95 100755 > > --- a/git-add--interactive.perl > > +++ b/git-add--interactive.perl > > > > [...] > > > > @@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk > > J - leave this hunk undecided, see next hunk > > k - leave this hunk undecided, see previous undecided hunk > > K - leave this hunk undecided, see previous hunk > > +l - select hunk lines to use > > s/select hunk lines to use/stage hunk lines/ I was wrong here - in the context of Junio`s remark, I now think this might even belong to context-aware "help_patch_modes" instead, phrased accordingly in there (stage/stash/unstage... etc.). ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood 2018-03-06 10:17 ` [PATCH v2 1/3] " Phillip Wood @ 2018-03-06 10:17 ` Phillip Wood 2018-03-06 19:57 ` Junio C Hamano 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:19 ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic 3 siblings, 2 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If the list of lines to be selected begins with '^' select all the lines except the ones listed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- 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 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 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-06 20:41 ` Igor Djordjevic 1 sibling, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-03-06 19:57 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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? ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-06 19:57 ` Junio C Hamano @ 2018-03-08 11:05 ` Phillip Wood 2018-03-08 17:53 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-03-08 11:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood On 06/03/18 19:57, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-08 11:05 ` Phillip Wood @ 2018-03-08 17:53 ` Junio C Hamano 2018-03-13 12:06 ` Phillip Wood 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-03-08 17:53 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-08 17:53 ` Junio C Hamano @ 2018-03-13 12:06 ` Phillip Wood 2018-03-13 16:32 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-03-13 12:06 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood On 08/03/18 17:53, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-13 12:06 ` Phillip Wood @ 2018-03-13 16:32 ` Junio C Hamano 2018-03-14 11:02 ` Phillip Wood 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-03-13 16:32 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > On 08/03/18 17:53, Junio C Hamano wrote: >> Phillip Wood <phillip.wood@talktalk.net> 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 2018-03-13 16:32 ` Junio C Hamano @ 2018-03-14 11:02 ` Phillip Wood 0 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-14 11:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood On 13/03/18 16:32, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> On 08/03/18 17:53, Junio C Hamano wrote: >>> Phillip Wood <phillip.wood@talktalk.net> 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 ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted 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-06 20:41 ` Igor Djordjevic 1 sibling, 0 replies; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 20:41 UTC (permalink / raw) To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite On 06/03/2018 11:17, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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 <phillip.wood@dunelm.org.uk> > --- > 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 && > ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v2 3/3] add -p: optimize line selection for short hunks 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood 2018-03-06 10:17 ` [PATCH v2 1/3] " Phillip Wood 2018-03-06 10:17 ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood @ 2018-03-06 10:17 ` 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 3 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw) To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If there are fewer than ten changes in a hunk then make spaces optional when selecting individual lines. This means that for short hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 3 ++- git-add--interactive.perl | 30 ++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 0e2c11e97b..d52acfc722 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -340,7 +340,8 @@ patch:: 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 +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. + diff --git a/git-add--interactive.perl b/git-add--interactive.perl index 6fa3d0a87c..9a6bcd5085 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1082,6 +1082,33 @@ sub check_hunk_label { return 1; } +sub split_hunk_selection { + local $_; + my @fields = @_; + my @ret; + for (@fields) { + if (/^(-[0-9])(.*)/) { + push @ret, $1; + $_ = $2; + } + while ($_ ne '') { + if (/^[0-9]-$/) { + push @ret, $_; + last; + } elsif (/^([0-9](?:-[0-9])?)(.*)/) { + push @ret, $1; + $_ = $2; + } else { + error_msg sprintf + __("invalid hunk line '%s'\n"), + substr($_, 0, 1); + return (); + } + } + } + return @ret; +} + sub parse_hunk_selection { local $_; my ($hunk, $line) = @_; @@ -1100,6 +1127,9 @@ sub parse_hunk_selection { } } } + if ($max_label < 10) { + @fields = split_hunk_selection(@fields) or return undef; + } for (@fields) { if (/^([0-9]*)-([0-9]*)$/) { if ($1 eq '' and $2 eq '') { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 89c0e73f2b..d3bce154da 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 "^1 3" | + printf "%s\n" l ^13 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v2 3/3] add -p: optimize line selection for short hunks 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 0 siblings, 0 replies; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 20:33 UTC (permalink / raw) To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite On 06/03/2018 11:17, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > If there are fewer than ten changes in a hunk then make spaces > optional when selecting individual lines. This means that for short Not sure if using s/selecting individual lines/staging individual lines/ would make sense here, too, but not that important (as you later do say "to stage lines"). > hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7. > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > Documentation/git-add.txt | 3 ++- > git-add--interactive.perl | 30 ++++++++++++++++++++++++++++++ > t/t3701-add-interactive.sh | 2 +- > 3 files changed, 33 insertions(+), 2 deletions(-) > > diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt > index 0e2c11e97b..d52acfc722 100644 > --- a/Documentation/git-add.txt > +++ b/Documentation/git-add.txt > @@ -340,7 +340,8 @@ patch:: > 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 > +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. > + > diff --git a/git-add--interactive.perl b/git-add--interactive.perl > index 6fa3d0a87c..9a6bcd5085 100755 > --- a/git-add--interactive.perl > +++ b/git-add--interactive.perl > @@ -1082,6 +1082,33 @@ sub check_hunk_label { > return 1; > } > > +sub split_hunk_selection { > + local $_; > + my @fields = @_; > + my @ret; > + for (@fields) { > + if (/^(-[0-9])(.*)/) { > + push @ret, $1; > + $_ = $2; > + } > + while ($_ ne '') { > + if (/^[0-9]-$/) { > + push @ret, $_; > + last; > + } elsif (/^([0-9](?:-[0-9])?)(.*)/) { > + push @ret, $1; > + $_ = $2; > + } else { > + error_msg sprintf > + __("invalid hunk line '%s'\n"), > + substr($_, 0, 1); > + return (); > + } > + } > + } > + return @ret; > +} > + > sub parse_hunk_selection { > local $_; > my ($hunk, $line) = @_; > @@ -1100,6 +1127,9 @@ sub parse_hunk_selection { > } > } > } > + if ($max_label < 10) { > + @fields = split_hunk_selection(@fields) or return undef; > + } > for (@fields) { > if (/^([0-9]*)-([0-9]*)$/) { > if ($1 eq '' and $2 eq '') { > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 89c0e73f2b..d3bce154da 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 "^1 3" | > + printf "%s\n" l ^13 | > EDITOR=: git reset -p 2>error && > test_must_be_empty error && > git diff --cached HEAD >actual && > ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 0/3] add -p: select individual hunk lines 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood ` (2 preceding siblings ...) 2018-03-06 10:17 ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood @ 2018-03-06 20:19 ` Igor Djordjevic 2018-03-06 21:03 ` Junio C Hamano 3 siblings, 1 reply; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 20:19 UTC (permalink / raw) To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite Hi Phillip, On 06/03/2018 11:17, Phillip Wood wrote: > > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > I've added some documentation to git-add.txt for the new selection > mode and cleaned up some style issues, otherwise these are unchanged > since v1. 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.fsf@gitster-ct.c.googlers.com/T/#m01d0f1af90f32b698e583b56f8e53b986bcec7c6 Nice, thank you :) A small nitpick - I see you use phrasing like "select lines", where the other commands usually talk about "staging", instead, so "stage lines" might be more aligned with the existing text. I`ll quickly go through the patches regarding this (not being of much help for the code itself at the moment, sorry!). Regards, Buga ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 0/3] add -p: select individual hunk lines 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 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-03-06 21:03 UTC (permalink / raw) To: Igor Djordjevic; +Cc: Phillip Wood, Git Mailing List, Gustavo Leite Igor Djordjevic <igor.d.djordjevic@gmail.com> writes: > A small nitpick - I see you use phrasing like "select lines", where > the other commands usually talk about "staging", instead, so "stage > lines" might be more aligned with the existing text. Isn't this machinery shared across "add -p" and "reset -p"? What is done to the selected lines when you are using this UI while running "reset -p"? I hope it is not "staging". If the interface only "selects lines" and what is done to the selected lines depends on what operation is using this backend, then the current phrasing is perfectly fine and saying "staging" makes it actively worse. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v2 0/3] add -p: select individual hunk lines 2018-03-06 21:03 ` Junio C Hamano @ 2018-03-06 21:20 ` Igor Djordjevic 0 siblings, 0 replies; 53+ messages in thread From: Igor Djordjevic @ 2018-03-06 21:20 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phillip Wood, Git Mailing List, Gustavo Leite Hi Junio, On 06/03/2018 22:03, Junio C Hamano wrote: > > > A small nitpick - I see you use phrasing like "select lines", where > > the other commands usually talk about "staging", instead, so "stage > > lines" might be more aligned with the existing text. > > Isn't this machinery shared across "add -p" and "reset -p"? What is > done to the selected lines when you are using this UI while running > "reset -p"? I hope it is not "staging". If the interface only > "selects lines" and what is done to the selected lines depends on > what operation is using this backend, then the current phrasing is > perfectly fine and saying "staging" makes it actively worse. Hmm, if that is the case, I agree, but I was merely trying to review the files being changed - for example, inside "Documentation/git-add.txt": y - stage this hunk n - do not stage this hunk q - quit; do not stage this hunk or any of the remaining ones a - stage this hunk and all later hunks in the file d - do not stage this hunk or any of the later hunks in the file g - select a hunk to go to / - search for a hunk matching the given regex j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help In there, adding "l" should follow "stage" phrasing, I would think. But you are right for "git-add--interactive.perl", for example - in there, I didn`t notice the line (seems to be?) added inside the shared "help_patch_cmd". But if so, I guess it should then be moved to more context-related "help_patch_modes", being phrased accordingly in there. Thanks for pointing this out, let me recheck my comments. Regards, Buga ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v3 0/3] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood ` (4 preceding siblings ...) 2018-03-06 10:17 ` [PATCH v2 " Phillip Wood @ 2018-03-16 10:13 ` Phillip Wood 2018-03-16 10:13 ` [PATCH v3 1/3] " Phillip Wood ` (3 more replies) 2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood 7 siblings, 4 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood 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'." [1] https://public-inbox.org/git/xmqqbmg29x1n.fsf@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 +1111,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 -13 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && Phillip Wood (3): add -p: select individual hunk lines add -p: allow line selection to be inverted add -p: optimize line selection for short hunks Documentation/git-add.txt | 10 +++ git-add--interactive.perl | 181 +++++++++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 65 ++++++++++++++++ 3 files changed, 256 insertions(+) -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 1/3] add -p: select individual hunk lines 2018-03-16 10:13 ` [PATCH v3 " Phillip Wood @ 2018-03-16 10:13 ` Phillip Wood 2018-03-16 10:13 ` [PATCH v3 2/3] add -p: allow line selection to be inverted Phillip Wood ` (2 subsequent siblings) 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 'b' may be omitted to mean all lines from 'a' to the end of the hunk. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2: - remove support for omitting the lower bound of a range of lines as this clashes with using '-' for inverting the selection. - document that the upper bound can be omitted. Documentation/git-add.txt | 8 +++ git-add--interactive.perl | 140 +++++++++++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 65 +++++++++++++++++++++ 3 files changed, 213 insertions(+) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index d50fa339dc..965e192a09 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -332,10 +332,18 @@ patch:: J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk + l - select hunk lines to use s - split the current hunk into smaller hunks 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, 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. ++ 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 f83e7450ad..712226b34c 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1013,6 +1013,130 @@ sub color_diff { } @_; } +sub label_hunk_lines { + local $_; + my $hunk = shift; + my $i = 0; + my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ]; + if ($i > 1) { + @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i); + return 1; + } + return 0; +} + +sub select_hunk_lines { + my ($hunk, $selected) = @_; + my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)}; + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); + my ($push_eol, @newtext); + # Lines with this mode will become context lines if they are + # not selected + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; + for $i (1..$#{$text}) { + my $mode = substr($text->[$i], 0, 1); + if ($mode eq '\\') { + push @newtext, $text->[$i] if ($push_eol); + undef $push_eol; + } elsif ($labels->[$i] and $selected->[$labels->[$i]]) { + push @newtext, $text->[$i]; + if ($mode eq '+') { + $n_cnt++; + } else { + $o_cnt++; + } + $push_eol = 1; + } elsif ($mode eq ' ' or $mode eq $context_mode) { + push @newtext, ' ' . substr($text->[$i], 1); + $o_cnt++; $n_cnt++; + $push_eol = 1; + } else { + undef $push_eol; + } + } + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = + parse_hunk_header($text->[0]); + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); + my $newhunk = { + TEXT => \@newtext, + DISPLAY => [ color_diff(@newtext) ], + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, + TYPE => $hunk->{TYPE}, + USE => 1, + }; + # If this hunk has previously been edited add the offset delta + # of the old hunk to get the real delta from the original + # hunk. + if ($hunk->{OFS_DELTA}) { + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + } + return $newhunk; +} + +sub check_hunk_label { + my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]); + if ($label < 1 or $label > $max_label) { + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); + return 0; + } + return 1; +} + +sub parse_hunk_selection { + local $_; + my ($hunk, $line) = @_; + my $max_label = $hunk->{MAX_LABEL}; + my @selected = (0) x ($max_label + 1); + my @fields = split(/[,\s]+/, $line); + for (@fields) { + if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) { + if ($hi eq '') { + $hi = $max_label; + } + check_hunk_label($hunk, $lo) or return undef; + check_hunk_label($hunk, $hi) or return undef; + if ($hi < $lo) { + ($lo, $hi) = ($hi, $lo); + } + @selected[$lo..$hi] = (1) x (1 + $hi - $lo); + } elsif (/^([0-9]+)$/) { + check_hunk_label($hunk, $1) or return undef; + $selected[$1] = 1; + } else { + error_msg sprintf(__("invalid hunk line '%s'\n"), $_); + return undef; + } + } + return \@selected; +} + +sub display_hunk_lines { + my ($display, $labels, $max_label) = + @{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)}; + my $width = int(log($max_label) / log(10)) + 1; + my $padding = ' ' x ($width + 1); + for my $i (0..$#{$display}) { + if ($labels->[$i]) { + printf '%*d %s', $width, $labels->[$i], $display->[$i]; + } else { + print $padding . $display->[$i]; + } + } +} + +sub select_lines_loop { + my $hunk = shift; + display_hunk_lines($hunk); + my $selection = undef; + until (defined $selection) { + print colored $prompt_color, __("select lines? "); + my $text = <STDIN>; + defined $text and $text =~ /\S/ or return undef; + $selection = parse_hunk_selection($hunk, $text); + } + return select_hunk_lines($hunk, $selection); +} + my %edit_hunk_manually_modes = ( stage => N__( "If the patch applies cleanly, the edited hunk will immediately be @@ -1255,6 +1379,7 @@ j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk +l - select hunk lines to use s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help @@ -1471,6 +1596,9 @@ sub patch_update_file { if ($hunk[$ix]{TYPE} eq 'hunk') { $other .= ',e'; } + if (label_hunk_lines($hunk[$ix])) { + $other .= ',l'; + } for (@{$hunk[$ix]{DISPLAY}}) { print; } @@ -1610,6 +1738,18 @@ sub patch_update_file { next; } } + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); if (1 < @split) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index a9a9478a29..5f3fcb1758 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..341cc6b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,9 @@ + +5 + 10 + 20 + +21 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage individual lines of patch' ' + git reset && + printf 61 >>test && + printf "%s\n" l "1,2 4-" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + +5 + 10 + 20 + 30 + 40 + 50 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can reset individual lines of patch' ' + printf "%s\n" l 2 | + EDITOR=: git reset -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && @@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'add -p selecting lines works with pathological context lines' ' + git reset && + printf "%s\n" l 2 y | + GIT_EDITOR=./editor git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 2/3] add -p: allow line selection to be inverted 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 ` 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 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If the list of lines to be selected begins with '-' select all the lines except the ones listed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2: - use '-' to invert the selection instead of '^' to be consistent with the rest of add -i and clean -i. 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 965e192a09..01ff4d7d24 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -342,7 +342,8 @@ 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. If the upper bound of a range of lines is omitted it defaults to -the last line. +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 712226b34c..54fbe114f3 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 (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) { if ($hi eq '') { @@ -1107,6 +1119,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 5f3fcb1758..46814babf3 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 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v3 3/3] add -p: optimize line selection for short hunks 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 ` Phillip Wood 2018-03-29 18:32 ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw) To: Git Mailing List Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If there are fewer than ten changes in a hunk then make spaces optional when selecting individual lines. This means that for short hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Notes: changes since v2: - removed code that handled the default lower bound now that '-...' means invert the selection. Documentation/git-add.txt | 9 +++++---- git-add--interactive.perl | 26 ++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 01ff4d7d24..f3c81dfb11 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -340,10 +340,11 @@ patch:: 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. 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. +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 54fbe114f3..d65ad7c26d 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1082,6 +1082,29 @@ sub check_hunk_label { return 1; } +sub split_hunk_selection { + local $_; + my @fields = @_; + my @ret; + for (@fields) { + while ($_ ne '') { + if (/^[0-9]-$/) { + push @ret, $_; + last; + } elsif (/^([0-9](?:-[0-9])?)(.*)/) { + push @ret, $1; + $_ = $2; + } else { + error_msg sprintf + __("invalid hunk line '%s'\n"), + substr($_, 0, 1); + return (); + } + } + } + return @ret; +} + sub parse_hunk_selection { local $_; my ($hunk, $line) = @_; @@ -1100,6 +1123,9 @@ sub parse_hunk_selection { } } } + if ($max_label < 10) { + @fields = split_hunk_selection(@fields) or return undef; + } for (@fields) { if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) { if ($hi eq '') { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 46814babf3..467c6eff0e 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 "-1 3" | + printf "%s\n" l -13 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.16.2 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/3] add -p: select individual hunk lines 2018-03-16 10:13 ` [PATCH v3 " Phillip Wood ` (2 preceding siblings ...) 2018-03-16 10:13 ` [PATCH v3 3/3] add -p: optimize line selection for short hunks Phillip Wood @ 2018-03-29 18:32 ` Junio C Hamano 2018-03-30 11:09 ` Phillip Wood 3 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-03-29 18:32 UTC (permalink / raw) To: Git Mailing List Cc: Phillip Wood, Gustavo Leite, Igor Djordjevic, Phillip Wood 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/3] add -p: select individual hunk lines 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 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-03-30 11:09 UTC (permalink / raw) To: Junio C Hamano, Git Mailing List Cc: Gustavo Leite, Igor Djordjevic, Phillip Wood 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. Best Wishes Phillip ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/3] add -p: select individual hunk lines 2018-03-30 11:09 ` Phillip Wood @ 2018-03-31 19:20 ` Ævar Arnfjörð Bjarmason 2018-04-02 10:55 ` Phillip Wood 0 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-03-31 19:20 UTC (permalink / raw) To: phillip.wood Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic, Fernando Vezzosi, Jeff King 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/3] add -p: select individual hunk lines 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 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-04-02 10:55 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, phillip.wood Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic, Fernando Vezzosi, Jeff King 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 <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. 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-<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. I agree it's useless here I've got some suggestions on how to fix it though I've not spent much time thinking them through - any comments would be most welcome. I think we want something that pairs up groups of selected deletions and insertions rather than lines, so that it can handle cases where the number of inserted lines differs from the number of deletions but there are the same number of groups. For example given 1 -* first 2 -* second 3 -* third 4 +* a longer description of the 5 + first item 6 +* the second item 7 +* the third item 1,3,4-5,7 would give -* first +* a longer description of the + first item * second -* third +* the third item If the number of deletion groups differs from the number of insertion groups then there is no way to automagically pair them up so we'd need some syntax to do that. We could make the order of the numbers given by the user significant so that 1,6,7,2,4,9 would pair line 1 with 6 & 7 and line 4 with 9 and delete line 2. It would then be an error if the an earlier deletion was pair with a later insertion, so in the example above 1,6,7,4,9,2 or 2,4,9,1,6,7 would be valid but 1,9,2,4,8 wouldn't). We could also use brackets to group things e.g. [1,6,7],2,[4,9] which makes the groups clearer visually. Thinking further there is a problem with 1 -original 2 +a new paragraph before 3 +original 4 + 5 +modified original if the user wants to commit the new paragraph before the modified original then they need a way to specify that the insertion should come before the deletion. At the moment giving 2-4 will put the new paragraph after the unmodified original. To solve that there needs to be a way to indicate that a group of insertions needs to come before the context lines created by not staging the deletions - perhaps ^[2-4] or [^2-4]? (we could distinguish between commas and spaces so that ^2,3,5 is different to ^2 3,5 but that is then different to how lists work in the rest of git) > 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. > Thanks for your feedback it's been really useful, let me know what you think about grouping things. Best Wishes Phillip ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v3 0/3] add -p: select individual hunk lines 2018-04-02 10:55 ` Phillip Wood @ 2018-04-02 11:39 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-04-02 11:39 UTC (permalink / raw) To: phillip.wood Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic, Fernando Vezzosi, Jeff King 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 <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. > > 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-<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. > > I agree it's useless here I've got some suggestions on how to fix it > though I've not spent much time thinking them through - any comments > would be most welcome. > > I think we want something that pairs up groups of selected deletions and > insertions rather than lines, so that it can handle cases where the > number of inserted lines differs from the number of deletions but there > are the same number of groups. For example given > > 1 -* first > 2 -* second > 3 -* third > 4 +* a longer description of the > 5 + first item > 6 +* the second item > 7 +* the third item > > 1,3,4-5,7 would give > > -* first > +* a longer description of the > + first item > * second > -* third > +* the third item > > If the number of deletion groups differs from the number of insertion > groups then there is no way to automagically pair them up so we'd need > some syntax to do that. We could make the order of the numbers given by > the user significant so that 1,6,7,2,4,9 would pair line 1 with 6 & 7 > and line 4 with 9 and delete line 2. It would then be an error if the an > earlier deletion was pair with a later insertion, so in the example > above 1,6,7,4,9,2 or 2,4,9,1,6,7 would be valid but 1,9,2,4,8 wouldn't). > We could also use brackets to group things e.g. [1,6,7],2,[4,9] which > makes the groups clearer visually. > > Thinking further there is a problem with > > 1 -original > 2 +a new paragraph before > 3 +original > 4 + > 5 +modified original > > if the user wants to commit the new paragraph before the modified > original then they need a way to specify that the insertion should come > before the deletion. At the moment giving 2-4 will put the new paragraph > after the unmodified original. To solve that there needs to be a way to > indicate that a group of insertions needs to come before the context > lines created by not staging the deletions - perhaps ^[2-4] or [^2-4]? > (we could distinguish between commas and spaces so that ^2,3,5 is > different to ^2 3,5 but that is then different to how lists work in the > rest of git) > > >> 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. >> > > Thanks for your feedback it's been really useful, let me know what you > think about grouping things. All of that makes sense to me. Yes as you noted earlier there's only so far we can get in this case by automatically trying to pair up removed/added lines, and actually your example wasn't even very pathological, consider: 1 -* a longer description of the 2 - first item 3 -* second 4 -* third 5 +* first 6 +* a longer description of the 7 + second item 8 +* the third item There the naïve heuristic I initially noted for selecting 4,8 would work, but not we couldn't in the general case rely on the removed/added lines being equal in number as some fallback for the simpler transpose behavior. It sounds like you're interested in hacking more on this, great. I'll be happy to review & add tests once you have something. I'll just say that I wonder if we should simply leave some of these more complex cases on the table, and only handle some subsets of simple cases in liue of coming up with a syntax that handles everything. there's always "edit" as the fallback for the complex cases. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC PATCH v4 0/4] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood ` (5 preceding siblings ...) 2018-03-16 10:13 ` [PATCH v3 " Phillip Wood @ 2018-07-26 10:22 ` Phillip Wood 2018-07-26 10:22 ` [PATCH v4 1/4] " Phillip Wood ` (3 more replies) 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood 7 siblings, 4 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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'." Phillip Wood (4): add -p: select individual hunk lines add -p: select modified lines correctly add -p: allow line selection to be inverted add -p: optimize line selection for short hunks Documentation/git-add.txt | 10 ++ git-add--interactive.perl | 350 +++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 209 ++++++++++++++++++++++ 3 files changed, 569 insertions(+) -- 2.18.0 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v4 1/4] add -p: select individual hunk lines 2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood @ 2018-07-26 10:22 ` Phillip Wood 2018-07-26 10:22 ` [RFC PATCH v4 2/4] add -p: select modified lines correctly Phillip Wood ` (2 subsequent siblings) 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 'b' may be omitted to mean all lines from 'a' to the end of the hunk. Modified lines are not handled correctly, that will be fixed in the next commit. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 8 ++ git-add--interactive.perl | 181 +++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 103 +++++++++++++++++++++ 3 files changed, 292 insertions(+) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index d50fa339dc..965e192a09 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -332,10 +332,18 @@ patch:: J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk + l - select hunk lines to use s - split the current hunk into smaller hunks 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, 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. ++ 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 8361ef45e7..cbc9e5698a 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1013,6 +1013,171 @@ sub color_diff { } @_; } +use constant { + NO_NEWLINE => 1, +}; + +sub label_hunk_lines { + my $hunk = shift; + my $text = $hunk->{TEXT}; + my (@line_flags, @lines); + my ($block, $label, $last_mode) = (0, 0, ''); + for my $line (1..$#{$text}) { + $line_flags[$line] = 0; + my $mode = substr($text->[$line], 0, 1); + if ($mode eq '\\') { + $line_flags[$line - 1] |= NO_NEWLINE; + } + if ($mode eq '-' or $mode eq '+') { + $lines[++$label] = $line; + } + } + if ($label > 1) { + $hunk->{LABELS} = { + LINES => \@lines, + }; + $hunk->{LINE_FLAGS} = \@line_flags; + return 1; + } + return 0; +} + +sub select_hunk_lines { + my ($hunk, $selected) = @_; + my ($line_flags, $text) = @{$hunk}{qw(LINE_FLAGS TEXT)}; + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); + my @newtext; + + my $select_lines = sub { + for my $i (@_) { + my $line = $text->[$i]; + my $mode = substr($line, 0, 1); + push @newtext, $line; + if ($mode eq '+') { + $n_cnt++; + } elsif ($mode eq '-') { + $o_cnt++; + } + } + }; + + my ($lo, $hi) = splice(@$selected, 0, 2); + # Lines with this mode will become context lines if they are + # not selected + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; + for $i (1..$#{$text}) { + if ($lo <= $i and $i <= $hi) { + $select_lines->($i); + } else { + my $line = $text->[$i]; + my $mode = substr($line, 0, 1); + if ($mode eq ' ' or $mode eq $context_mode) { + push @newtext, ' ' . substr($line, 1); + $o_cnt++; $n_cnt++; + if ($line_flags->[$i] & NO_NEWLINE) { + push @newtext, $text->[$i + 1]; + } + } + } + if ($i == $hi) { + if (@$selected) { + ($lo, $hi) = splice(@$selected, 0, 2); + } + } + } + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = + parse_hunk_header($text->[0]); + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); + my $newhunk = { + TEXT => \@newtext, + DISPLAY => [ color_diff(@newtext) ], + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, + TYPE => $hunk->{TYPE}, + USE => 1, + }; + # If this hunk has previously been edited add the offset delta + # of the old hunk to get the real delta from the original + # hunk. + if ($hunk->{OFS_DELTA}) { + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + } + return $newhunk; +} + +sub check_hunk_label { + my ($max_label, $label) = @_; + if ($label < 1 or $label > $max_label) { + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); + return 0; + } + return 1; +} + +sub parse_hunk_selection { + my ($hunk, $line) = @_; + my $lines = $hunk->{LABELS}->{LINES}; + my $max_label = $#{$lines}; + my %selected; + my @fields = split(/[,\s]+/, $line); + for my $f (@fields) { + if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { + if ($hi eq '') { + $hi = $max_label; + } + check_hunk_label($max_label, $lo) or return undef; + check_hunk_label($max_label, $hi) or return undef; + if ($hi < $lo) { + ($lo, $hi) = ($hi, $lo); + } + undef @selected{$lo..$hi}; + } elsif (my ($label) = ($f =~ /^([0-9]+)$/)) { + check_hunk_label($max_label, $label) or return undef; + undef $selected{$label}; + } else { + error_msg sprintf(__("invalid hunk line '%s'\n"), $f); + return undef; + } + } + [ map { + my $line = $lines->[$_]; + if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) { + ($line, $line + 1); + } else { + ($line, $line); + } + } sort { $a <=> $b } keys(%selected) ]; +} + +sub display_hunk_lines { + my $hunk = shift; + my ($display, $lines) = ($hunk->{DISPLAY}, $hunk->{LABELS}->{LINES}); + my $max_label = $#{$lines}; + my $width = int(log($max_label) / log(10)) + 1; + my $padding = ' ' x ($width + 1); + my $label = 1; + for my $line (0..$#{$display}) { + if ($lines->[$label] == $line) { + printf '%*d %s', $width, $label, $display->[$line]; + $label++ if ($label < $max_label); + } else { + print $padding . $display->[$line]; + } + } +} + +sub select_lines_loop { + my $hunk = shift; + display_hunk_lines($hunk); + my $selection = undef; + until (defined $selection) { + print colored $prompt_color, __("select lines? "); + my $text = <STDIN>; + defined $text and $text =~ /\S/ or return undef; + $selection = parse_hunk_selection($hunk, $text); + } + return select_hunk_lines($hunk, $selection); +} + my %edit_hunk_manually_modes = ( stage => N__( "If the patch applies cleanly, the edited hunk will immediately be @@ -1255,6 +1420,7 @@ j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk +l - select hunk lines to use s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help @@ -1471,6 +1637,9 @@ sub patch_update_file { if ($hunk[$ix]{TYPE} eq 'hunk') { $other .= ',e'; } + if (label_hunk_lines($hunk[$ix])) { + $other .= ',l'; + } for (@{$hunk[$ix]{DISPLAY}}) { print; } @@ -1610,6 +1779,18 @@ sub patch_update_file { next; } } + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); if (1 < @split) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index f1bb879ea4..5b535a22d5 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -403,6 +403,101 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'setup test file' ' + test_write_lines 10 31 32 33 60 >test && + printf 61 >>test +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..341cc6b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,6 @@ + 10 + 20 + -30 + -40 + -50 + +31 + +33 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage individual lines of patch' ' + git reset && + printf "%s\n" l "5-2,7-" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,7 @@ + 10 + 20 + 30 + -40 + 50 + +33 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can reset individual lines of patch' ' + printf "%s\n" l 4,1,3 | + EDITOR=: git reset -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup file' ' + test_write_lines ten twenty thirty forty fifty >test && + printf sixty >>test +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + -30 + +sixty + 40 + 50 + 60 + EOF +' + +test_expect_failure 'can stage modified lines of patch (1)' ' + git reset && + printf "%s\n" l "1,3 7-9 12" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && @@ -571,4 +666,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'add -p selecting lines works with pathological context lines' ' + git reset && + printf "%s\n" l 2 y | + GIT_EDITOR=./editor git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [RFC PATCH v4 2/4] add -p: select modified lines correctly 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 ` 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 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a set of lines is modified the hunk contains deletions followed by insertions. To correctly stage a subset of the modified lines we need to match up the selected deletions with the selected insertions otherwise we end up with deletions and context lines followed by insertions which is not what we want. For example given the hunk 1 -* a longer description of the 2 - first item 3 -* second 4 -* third 5 +* first 6 + second item 7 +* the third item If the user selects 1,2,4–5,7 then we should generate -* a longer description of the - first item +* first * second -* third +* the third item not -* a longer description of the - first item * second -* third +* first +* the third item Currently the code can only cope with selections that contain the same number of groups of insertions and deletions, though each group need not contain the same number of insertions and deletions. If the user wants to stage an unpaired deletion or insertion in a hunk where they also want to stage modified lines they have to do it with two invocations of 'git add -p'. It would be possible to add some syntax to allow lines to be excluded from groups to allow the user to stage such changes in a single go. It may also be useful to allow users to explicitly group lines. If in the example above the second item is deleted we have 1 -* a longer description of the 2 - first item 3 -* second 4 -* third 5 +* first 6 +* the third item Selecting 1,2,4–6 will give an error. If lines could be grouped explicitly then it would be possible to type something like 1,2,4,[5],6 to indicate that there are two groups of insertions giving -* a longer description of the - first item +* first * second -* third +* the third item We may want to be able to stage an insertion before an unselected deletion to allow the user to stage a new paragraph before the unmodified original in 1 -original 2 +a new paragraph before 3 +original 4 + 5 +modified original by specifying something like ^2-4 to give +a new paragraph before +original + original I'm not sure how common these cases are in real life and how much effort it's worth putting into handling them at the moment when the user can edit the hunk if need be. Perhaps it would be better to leave it for future extensions when it becomes clearer what would be most useful. Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 148 ++++++++++++++++++++++++++++++++++--- t/t3701-add-interactive.sh | 108 ++++++++++++++++++++++++++- 2 files changed, 243 insertions(+), 13 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index cbc9e5698a..b7e3da0210 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1015,26 +1015,47 @@ sub color_diff { use constant { NO_NEWLINE => 1, + LAST_ADD_DEL => 2, + FIRST_ADD => 4 }; sub label_hunk_lines { my $hunk = shift; my $text = $hunk->{TEXT}; - my (@line_flags, @lines); + # A block contains the insertions and deletions occurring context + # lines. + my (@blocks, @line_flags, @lines, @modes); my ($block, $label, $last_mode) = (0, 0, ''); for my $line (1..$#{$text}) { $line_flags[$line] = 0; my $mode = substr($text->[$line], 0, 1); if ($mode eq '\\') { $line_flags[$line - 1] |= NO_NEWLINE; } + if ($mode ne '-' and $last_mode eq '-' or + $mode ne '+' and $last_mode eq '+') { + $line_flags[$line - 1] |= LAST_ADD_DEL; + } + if ($mode eq '+' and $last_mode ne '+') { + $line_flags[$line] |= FIRST_ADD; + } if ($mode eq '-' or $mode eq '+') { - $lines[++$label] = $line; + $blocks[++$label] = $block; + $lines[$label] = $line; + $modes[$label] = $mode; + } elsif ($mode eq ' ' and $last_mode ne ' ') { + $block++; } + $last_mode = $mode; + } + if ($last_mode eq '-' or $last_mode eq '+') { + $line_flags[-1] |= LAST_ADD_DEL; } if ($label > 1) { $hunk->{LABELS} = { + BLOCKS => \@blocks, LINES => \@lines, + MODES => \@modes }; $hunk->{LINE_FLAGS} = \@line_flags; return 1; @@ -1061,11 +1082,14 @@ sub select_hunk_lines { } }; - my ($lo, $hi) = splice(@$selected, 0, 2); + my ($lo, $hi, $pre_insert, $post_insert) = splice(@$selected, 0, 4); # Lines with this mode will become context lines if they are # not selected my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; for $i (1..$#{$text}) { + if ($i == $lo and defined($pre_insert)) { + $select_lines->(@$pre_insert); + } if ($lo <= $i and $i <= $hi) { $select_lines->($i); } else { @@ -1080,8 +1104,12 @@ sub select_hunk_lines { } } if ($i == $hi) { + if (defined($post_insert)) { + $select_lines->(@$post_insert); + } if (@$selected) { - ($lo, $hi) = splice(@$selected, 0, 2); + ($lo, $hi, $pre_insert, $post_insert) = + splice(@$selected, 0, 4); } } } @@ -1104,6 +1132,109 @@ sub select_hunk_lines { return $newhunk; } +sub pair_hunk_selection { + my $line_flags = shift; + my @ret; + my $reverse = $patch_mode_flavour{IS_REVERSE}; + for my $sel (@_) { + my $i = 0; + my ($del, $add) = @$sel; + my $ndel = @$del / 2; + my $nadd = @$add / 2; + # If there the same number of insertion and deletion + # groups then pair up in insertions and deletions. + if ($nadd == $ndel) { + while ($i < @{$del}) { + my ($ra, $rb) = $reverse ? ($add, $del) : + ($del, $add); + my ($starta, $enda) = @{$ra}[$i, $i + 1]; + my ($startb, $endb) = @{$rb}[$i, $i + 1]; + if ($line_flags->[$enda] & LAST_ADD_DEL and + $line_flags->[$endb] & NO_NEWLINE) { + $endb++; + } + if ($line_flags->[$enda] & NO_NEWLINE) { + $enda++; + } + push @ret, $reverse ? + ($starta, $enda, + [ $startb..$endb ], undef) : + ($starta, $enda, + undef, [ $startb..$endb ]); + $i += 2; + } + } + # If there are only deletions or only insertions or + # a) we're adding to the index and there is a single group + # of insertions starting at the first inserted line + # or + # b) we're resetting the index and there is a single group + # of deletions finishing with the last deleted line + # then it is okay just to have deletions followed by + # insertions. + } elsif (!$nadd or !$ndel or + (!$reverse and $ndel == 1 and + $line_flags->[$del->[1]] & LAST_ADD_DEL) or + ($reverse and $nadd == 1 and + $line_flags->[$add->[0]] & FIRST_ADD)) { + while ($i < @$del) { + my ($start, $end) = @{$del}[$i, $i + 1]; + if ($line_flags->[$end] & NO_NEWLINE) { + $end++; + } + push @ret, $start, $end, undef, undef; + $i += 2; + } + $i = 0; + while ($i < @$add) { + my ($start, $end) = @{$add}[$i, $i + 1]; + if ($line_flags->[$end] & NO_NEWLINE) { + $end++; + } + push @ret, $start, $end, undef, undef; + $i += 2; + } + } else { + printf STDERR __("unable to pair up insertions and deletions\n"); + return undef; + } + } + return \@ret; +} + +sub process_hunk_selection { + my ($labels, $line_flags) = @{shift()}{qw(LABELS LINE_FLAGS)}; + my ($blocks, $lines, $modes) = + @{$labels}{qw(BLOCKS LINES MODES)}; + my @selection = sort { $a <=> $b } @_; + unless (@selection) { + return []; + } + my $last_label = shift @selection; + my ($last_block, $last_line, $last_mode) = + ($blocks->[$last_label], $lines->[$last_label], $modes->[$last_label]); + my ($del, $add) = ([], []); + push @{$last_mode eq '-' ? $del : $add}, $last_line; + my @sel; + for my $label (@selection) { + my ($block, $line, $mode) = + ($blocks->[$label], $lines->[$label], $modes->[$label]); + if ($block != $last_block) { + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @sel, [ $del, $add ]; + ($del, $add) = ([], []); + push @{$mode eq '-' ? $del : $add}, $line; + } elsif ($line != $last_line + 1 or $mode ne $last_mode) { + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @{$mode eq '-' ? $del : $add}, $line; + } + ($last_block, $last_line, $last_mode) = ($block, $line, $mode); + } + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @sel, [ $del, $add ]; + pair_hunk_selection($line_flags, @sel); +} + sub check_hunk_label { my ($max_label, $label) = @_; if ($label < 1 or $label > $max_label) { @@ -1138,14 +1269,7 @@ sub parse_hunk_selection { return undef; } } - [ map { - my $line = $lines->[$_]; - if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) { - ($line, $line + 1); - } else { - ($line, $line); - } - } sort { $a <=> $b } keys(%selected) ]; + return process_hunk_selection($hunk, keys(%selected)); } sub display_hunk_lines { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5b535a22d5..1d917ad018 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -489,7 +489,7 @@ test_expect_success 'setup expected diff' ' EOF ' -test_expect_failure 'can stage modified lines of patch (1)' ' +test_expect_success 'can stage modified lines of patch (1)' ' git reset && printf "%s\n" l "1,3 7-9 12" | EDITOR=: git add -p 2>error && @@ -498,6 +498,112 @@ test_expect_failure 'can stage modified lines of patch (1)' ' diff_cmp expected actual ' +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,7 @@ + 10 + -20 + +twenty + +thirty + 30 + 40 + 50 + -60 + +sixty + \ No newline at end of file + EOF +' + +test_expect_success 'can stage modified lines of patch (2)' ' + git reset && + printf "%s\n" l "2,6,8,9,12" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup HEAD' ' + git reset && + git apply --cached <<-\EOF && + diff --git a/test b/test + --- a/test + +++ b/test + @@ -3,4 +3,4 @@ + 30 + 40 + 50 + -60 + +60 + \ No newline at end of file + EOF + git commit +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + -30 + +fifty + 40 + 50 + 60 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage modified lines of patch (3)' ' + git reset && + printf "%s\n" l "11,1,3 7-9" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + 30 + 40 + 50 + -60 + \ No newline at end of file + +fifty + EOF +' + +test_expect_success 'can stage modified lines of patch (4)' ' + git reset && + printf "%s\n" l "9-6 11,1" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 3/4] add -p: allow line selection to be inverted 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 ` Phillip Wood 2018-07-26 10:22 ` [PATCH v4 4/4] add -p: optimize line selection for short hunks Phillip Wood 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If the list of lines to be selected begins with '-' select all the lines except the ones listed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 3 ++- git-add--interactive.perl | 19 +++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 965e192a09..01ff4d7d24 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -342,7 +342,8 @@ 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. If the upper bound of a range of lines is omitted it defaults to -the last line. +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 b7e3da0210..0b490cf1e9 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1248,8 +1248,21 @@ sub parse_hunk_selection { my ($hunk, $line) = @_; my $lines = $hunk->{LABELS}->{LINES}; my $max_label = $#{$lines}; + my $invert = undef; my %selected; my @fields = split(/[,\s]+/, $line); + if (my ($rest) = ($fields[0] =~ /^-(.*)/)) { + $invert = 1; + if ($rest ne '') { + $fields[0] = $rest; + } else { + shift @fields; + unless (@fields) { + error_msg __("no lines to invert\n"); + return undef; + } + } + } for my $f (@fields) { if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { if ($hi eq '') { @@ -1269,6 +1282,12 @@ sub parse_hunk_selection { return undef; } } + if ($invert) { + my %inverted; + undef @inverted{1..$max_label}; + delete @inverted{keys(%selected)}; + %selected = %inverted; + } return process_hunk_selection($hunk, keys(%selected)); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 1d917ad018..2fd456017f 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' ' ' test_expect_success 'can reset individual lines of patch' ' - printf "%s\n" l 4,1,3 | + printf "%s\n" l -6,2,5 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v4 4/4] add -p: optimize line selection for short hunks 2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood ` (2 preceding siblings ...) 2018-07-26 10:22 ` [PATCH v4 3/4] add -p: allow line selection to be inverted Phillip Wood @ 2018-07-26 10:22 ` Phillip Wood 3 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If there are fewer than ten changes in a hunk then make spaces optional when selecting individual lines. This means that for short hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 9 +++++---- git-add--interactive.perl | 26 ++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 01ff4d7d24..f3c81dfb11 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -340,10 +340,11 @@ patch:: 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. 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. +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 0b490cf1e9..56bc7ab496 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1244,6 +1244,29 @@ sub check_hunk_label { return 1; } +sub split_hunk_selection { + my @fields = @_; + my @ret; + for my $field (@fields) { + while ($field ne '') { + if ($field =~ /^[0-9]-$/) { + push @ret, $field; + last; + } elsif (my ($sel, $rest) = + ($field =~ /^([0-9](?:-[0-9])?)(.*)/)) { + push @ret, $sel; + $field = $rest; + } else { + error_msg sprintf + __("invalid hunk line '%s'\n"), + substr($field, 0, 1); + return (); + } + } + } + return @ret; +} + sub parse_hunk_selection { my ($hunk, $line) = @_; my $lines = $hunk->{LABELS}->{LINES}; @@ -1263,6 +1286,9 @@ sub parse_hunk_selection { } } } + if ($max_label < 10) { + @fields = split_hunk_selection(@fields) or return undef; + } for my $f (@fields) { if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { if ($hi eq '') { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 2fd456017f..b2b808275c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' ' ' test_expect_success 'can reset individual lines of patch' ' - printf "%s\n" l -6,2,5 | + printf "%s\n" l -625 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [RFC PATCH v5 0/4] add -p: select individual hunk lines 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood ` (6 preceding siblings ...) 2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood @ 2018-07-26 15:58 ` Phillip Wood 2018-07-26 15:58 ` [PATCH v5 1/4] " Phillip Wood ` (5 more replies) 7 siblings, 6 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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'." Phillip Wood (4): add -p: select individual hunk lines add -p: select modified lines correctly add -p: allow line selection to be inverted add -p: optimize line selection for short hunks Documentation/git-add.txt | 10 ++ git-add--interactive.perl | 349 +++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 209 ++++++++++++++++++++++ 3 files changed, 568 insertions(+) -- 2.18.0 ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v5 1/4] add -p: select individual hunk lines 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood @ 2018-07-26 15:58 ` Phillip Wood 2018-07-26 19:36 ` Junio C Hamano 2018-07-26 15:58 ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood ` (4 subsequent siblings) 5 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> 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 'b' may be omitted to mean all lines from 'a' to the end of the hunk. Modified lines are not handled correctly, that will be fixed in the next commit. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 8 ++ git-add--interactive.perl | 181 +++++++++++++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 103 +++++++++++++++++++++ 3 files changed, 292 insertions(+) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index d50fa339dc..965e192a09 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -332,10 +332,18 @@ patch:: J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk + l - select hunk lines to use s - split the current hunk into smaller hunks 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, 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. ++ 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 8361ef45e7..cbc9e5698a 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1013,6 +1013,171 @@ sub color_diff { } @_; } +use constant { + NO_NEWLINE => 1, +}; + +sub label_hunk_lines { + my $hunk = shift; + my $text = $hunk->{TEXT}; + my (@line_flags, @lines); + my ($block, $label, $last_mode) = (0, 0, ''); + for my $line (1..$#{$text}) { + $line_flags[$line] = 0; + my $mode = substr($text->[$line], 0, 1); + if ($mode eq '\\') { + $line_flags[$line - 1] |= NO_NEWLINE; + } + if ($mode eq '-' or $mode eq '+') { + $lines[++$label] = $line; + } + } + if ($label > 1) { + $hunk->{LABELS} = { + LINES => \@lines, + }; + $hunk->{LINE_FLAGS} = \@line_flags; + return 1; + } + return 0; +} + +sub select_hunk_lines { + my ($hunk, $selected) = @_; + my ($line_flags, $text) = @{$hunk}{qw(LINE_FLAGS TEXT)}; + my ($i, $o_cnt, $n_cnt) = (0, 0, 0); + my @newtext; + + my $select_lines = sub { + for my $i (@_) { + my $line = $text->[$i]; + my $mode = substr($line, 0, 1); + push @newtext, $line; + if ($mode eq '+') { + $n_cnt++; + } elsif ($mode eq '-') { + $o_cnt++; + } + } + }; + + my ($lo, $hi) = splice(@$selected, 0, 2); + # Lines with this mode will become context lines if they are + # not selected + my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; + for $i (1..$#{$text}) { + if ($lo <= $i and $i <= $hi) { + $select_lines->($i); + } else { + my $line = $text->[$i]; + my $mode = substr($line, 0, 1); + if ($mode eq ' ' or $mode eq $context_mode) { + push @newtext, ' ' . substr($line, 1); + $o_cnt++; $n_cnt++; + if ($line_flags->[$i] & NO_NEWLINE) { + push @newtext, $text->[$i + 1]; + } + } + } + if ($i == $hi) { + if (@$selected) { + ($lo, $hi) = splice(@$selected, 0, 2); + } + } + } + my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) = + parse_hunk_header($text->[0]); + unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt); + my $newhunk = { + TEXT => \@newtext, + DISPLAY => [ color_diff(@newtext) ], + OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt, + TYPE => $hunk->{TYPE}, + USE => 1, + }; + # If this hunk has previously been edited add the offset delta + # of the old hunk to get the real delta from the original + # hunk. + if ($hunk->{OFS_DELTA}) { + $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA}; + } + return $newhunk; +} + +sub check_hunk_label { + my ($max_label, $label) = @_; + if ($label < 1 or $label > $max_label) { + error_msg sprintf(__("invalid hunk line '%d'\n"), $label); + return 0; + } + return 1; +} + +sub parse_hunk_selection { + my ($hunk, $line) = @_; + my $lines = $hunk->{LABELS}->{LINES}; + my $max_label = $#{$lines}; + my %selected; + my @fields = split(/[,\s]+/, $line); + for my $f (@fields) { + if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { + if ($hi eq '') { + $hi = $max_label; + } + check_hunk_label($max_label, $lo) or return undef; + check_hunk_label($max_label, $hi) or return undef; + if ($hi < $lo) { + ($lo, $hi) = ($hi, $lo); + } + undef @selected{$lo..$hi}; + } elsif (my ($label) = ($f =~ /^([0-9]+)$/)) { + check_hunk_label($max_label, $label) or return undef; + undef $selected{$label}; + } else { + error_msg sprintf(__("invalid hunk line '%s'\n"), $f); + return undef; + } + } + [ map { + my $line = $lines->[$_]; + if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) { + ($line, $line + 1); + } else { + ($line, $line); + } + } sort { $a <=> $b } keys(%selected) ]; +} + +sub display_hunk_lines { + my $hunk = shift; + my ($display, $lines) = ($hunk->{DISPLAY}, $hunk->{LABELS}->{LINES}); + my $max_label = $#{$lines}; + my $width = int(log($max_label) / log(10)) + 1; + my $padding = ' ' x ($width + 1); + my $label = 1; + for my $line (0..$#{$display}) { + if ($lines->[$label] == $line) { + printf '%*d %s', $width, $label, $display->[$line]; + $label++ if ($label < $max_label); + } else { + print $padding . $display->[$line]; + } + } +} + +sub select_lines_loop { + my $hunk = shift; + display_hunk_lines($hunk); + my $selection = undef; + until (defined $selection) { + print colored $prompt_color, __("select lines? "); + my $text = <STDIN>; + defined $text and $text =~ /\S/ or return undef; + $selection = parse_hunk_selection($hunk, $text); + } + return select_hunk_lines($hunk, $selection); +} + my %edit_hunk_manually_modes = ( stage => N__( "If the patch applies cleanly, the edited hunk will immediately be @@ -1255,6 +1420,7 @@ j - leave this hunk undecided, see next undecided hunk J - leave this hunk undecided, see next hunk k - leave this hunk undecided, see previous undecided hunk K - leave this hunk undecided, see previous hunk +l - select hunk lines to use s - split the current hunk into smaller hunks e - manually edit the current hunk ? - print help @@ -1471,6 +1637,9 @@ sub patch_update_file { if ($hunk[$ix]{TYPE} eq 'hunk') { $other .= ',e'; } + if (label_hunk_lines($hunk[$ix])) { + $other .= ',l'; + } for (@{$hunk[$ix]{DISPLAY}}) { print; } @@ -1610,6 +1779,18 @@ sub patch_update_file { next; } } + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY}); if (1 < @split) { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index f1bb879ea4..5b535a22d5 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -403,6 +403,101 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' ' ! grep "^+31" actual ' +test_expect_success 'setup test file' ' + test_write_lines 10 31 32 33 60 >test && + printf 61 >>test +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..341cc6b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,6 @@ + 10 + 20 + -30 + -40 + -50 + +31 + +33 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage individual lines of patch' ' + git reset && + printf "%s\n" l "5-2,7-" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,7 @@ + 10 + 20 + 30 + -40 + 50 + +33 + 60 + +61 + \ No newline at end of file + EOF +' + +test_expect_success 'can reset individual lines of patch' ' + printf "%s\n" l 4,1,3 | + EDITOR=: git reset -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup file' ' + test_write_lines ten twenty thirty forty fifty >test && + printf sixty >>test +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + -30 + +sixty + 40 + 50 + 60 + EOF +' + +test_expect_failure 'can stage modified lines of patch (1)' ' + git reset && + printf "%s\n" l "1,3 7-9 12" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && @@ -571,4 +666,12 @@ test_expect_success 'add -p patch editing works with pathological context lines' test_cmp expected-2 actual ' +test_expect_success 'add -p selecting lines works with pathological context lines' ' + git reset && + printf "%s\n" l 2 y | + GIT_EDITOR=./editor git add -p && + git cat-file blob :a >actual && + test_cmp expected-2 actual +' + test_done -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [PATCH v5 1/4] add -p: select individual hunk lines 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 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-07-26 19:36 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > +sub label_hunk_lines { > + my $hunk = shift; > + my $text = $hunk->{TEXT}; > + my (@line_flags, @lines); > + my ($block, $label, $last_mode) = (0, 0, ''); > + for my $line (1..$#{$text}) { $text is a ref to an array so @$text is the whole thing, $#{$text} is the index of the last item in that array, and $text->[0] is the first element of that array. This for loop runs with $line == 1 thru $line == $#{$text}, so we are somehow excluding the very first element? > + $line_flags[$line] = 0; > + my $mode = substr($text->[$line], 0, 1); > + if ($mode eq '\\') { > + $line_flags[$line - 1] |= NO_NEWLINE; > + } > + if ($mode eq '-' or $mode eq '+') { > + $lines[++$label] = $line; > + } > + } > + if ($label > 1) { ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 1/4] add -p: select individual hunk lines 2018-07-26 19:36 ` Junio C Hamano @ 2018-07-27 10:05 ` Phillip Wood 2018-07-27 16:09 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-07-27 10:05 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood On 26/07/18 20:36, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > >> +sub label_hunk_lines { >> + my $hunk = shift; >> + my $text = $hunk->{TEXT}; >> + my (@line_flags, @lines); >> + my ($block, $label, $last_mode) = (0, 0, ''); >> + for my $line (1..$#{$text}) { > > $text is a ref to an array so @$text is the whole thing, $#{$text} > is the index of the last item in that array, and $text->[0] is the > first element of that array. This for loop runs with $line == 1 > thru $line == $#{$text}, so we are somehow excluding the very first > element? Yes that's right, $text->[0] contains the hunk header >> + $line_flags[$line] = 0; >> + my $mode = substr($text->[$line], 0, 1); >> + if ($mode eq '\\') { >> + $line_flags[$line - 1] |= NO_NEWLINE; >> + } >> + if ($mode eq '-' or $mode eq '+') { >> + $lines[++$label] = $line; >> + } >> + } >> + if ($label > 1) { ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [PATCH v5 1/4] add -p: select individual hunk lines 2018-07-27 10:05 ` Phillip Wood @ 2018-07-27 16:09 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-07-27 16:09 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > On 26/07/18 20:36, Junio C Hamano wrote: >> Phillip Wood <phillip.wood@talktalk.net> writes: >> >>> +sub label_hunk_lines { >>> + my $hunk = shift; >>> + my $text = $hunk->{TEXT}; >>> + my (@line_flags, @lines); >>> + my ($block, $label, $last_mode) = (0, 0, ''); >>> + for my $line (1..$#{$text}) { >> >> $text is a ref to an array so @$text is the whole thing, $#{$text} >> is the index of the last item in that array, and $text->[0] is the >> first element of that array. This for loop runs with $line == 1 >> thru $line == $#{$text}, so we are somehow excluding the very first >> element? > > Yes that's right, $text->[0] contains the hunk header OK, thanks. > >>> + $line_flags[$line] = 0; >>> + my $mode = substr($text->[$line], 0, 1); >>> + if ($mode eq '\\') { >>> + $line_flags[$line - 1] |= NO_NEWLINE; >>> + } >>> + if ($mode eq '-' or $mode eq '+') { >>> + $lines[++$label] = $line; >>> + } >>> + } >>> + if ($label > 1) { ^ permalink raw reply [flat|nested] 53+ messages in thread
* [RFC PATCH v5 2/4] add -p: select modified lines correctly 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 15:58 ` Phillip Wood 2018-07-26 19:30 ` Junio C Hamano 2018-07-26 15:58 ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood ` (3 subsequent siblings) 5 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> When a set of lines is modified the hunk contains deletions followed by insertions. To correctly stage a subset of the modified lines we need to match up the selected deletions with the selected insertions otherwise we end up with deletions and context lines followed by insertions which is not what we want. For example given the hunk 1 -* a longer description of the 2 - first item 3 -* second 4 -* third 5 +* first 6 + second item 7 +* the third item If the user selects 1,2,4–5,7 then we should generate -* a longer description of the - first item +* first * second -* third +* the third item not -* a longer description of the - first item * second -* third +* first +* the third item Currently the code can only cope with selections that contain the same number of groups of insertions and deletions, though each group need not contain the same number of insertions and deletions. If the user wants to stage an unpaired deletion or insertion in a hunk where they also want to stage modified lines they have to do it with two invocations of 'git add -p'. It would be possible to add some syntax to allow lines to be excluded from groups to allow the user to stage such changes in a single go. It may also be useful to allow users to explicitly group lines. If in the example above the second item is deleted we have 1 -* a longer description of the 2 - first item 3 -* second 4 -* third 5 +* first 6 +* the third item Selecting 1,2,4–6 will give an error. If lines could be grouped explicitly then it would be possible to type something like 1,2,4,[5],6 to indicate that there are two groups of insertions giving -* a longer description of the - first item +* first * second -* third +* the third item We may want to be able to stage an insertion before an unselected deletion to allow the user to stage a new paragraph before the unmodified original in 1 -original 2 +a new paragraph before 3 +original 4 + 5 +modified original by specifying something like ^2-4 to give +a new paragraph before +original + original I'm not sure how common these cases are in real life and how much effort it's worth putting into handling them at the moment when the user can edit the hunk if need be. Perhaps it would be better to leave it for future extensions when it becomes clearer what would be most useful. Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- git-add--interactive.perl | 147 ++++++++++++++++++++++++++++++++++--- t/t3701-add-interactive.sh | 108 ++++++++++++++++++++++++++- 2 files changed, 242 insertions(+), 13 deletions(-) diff --git a/git-add--interactive.perl b/git-add--interactive.perl index cbc9e5698a..7e4daee2fc 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1015,26 +1015,47 @@ sub color_diff { use constant { NO_NEWLINE => 1, + LAST_ADD_DEL => 2, + FIRST_ADD => 4 }; sub label_hunk_lines { my $hunk = shift; my $text = $hunk->{TEXT}; - my (@line_flags, @lines); + # A block contains the insertions and deletions occurring context + # lines. + my (@blocks, @line_flags, @lines, @modes); my ($block, $label, $last_mode) = (0, 0, ''); for my $line (1..$#{$text}) { $line_flags[$line] = 0; my $mode = substr($text->[$line], 0, 1); if ($mode eq '\\') { $line_flags[$line - 1] |= NO_NEWLINE; } + if ($mode ne '-' and $last_mode eq '-' or + $mode ne '+' and $last_mode eq '+') { + $line_flags[$line - 1] |= LAST_ADD_DEL; + } + if ($mode eq '+' and $last_mode ne '+') { + $line_flags[$line] |= FIRST_ADD; + } if ($mode eq '-' or $mode eq '+') { - $lines[++$label] = $line; + $blocks[++$label] = $block; + $lines[$label] = $line; + $modes[$label] = $mode; + } elsif ($mode eq ' ' and $last_mode ne ' ') { + $block++; } + $last_mode = $mode; + } + if ($last_mode eq '-' or $last_mode eq '+') { + $line_flags[-1] |= LAST_ADD_DEL; } if ($label > 1) { $hunk->{LABELS} = { + BLOCKS => \@blocks, LINES => \@lines, + MODES => \@modes }; $hunk->{LINE_FLAGS} = \@line_flags; return 1; @@ -1061,11 +1082,14 @@ sub select_hunk_lines { } }; - my ($lo, $hi) = splice(@$selected, 0, 2); + my ($lo, $hi, $pre_insert, $post_insert) = splice(@$selected, 0, 4); # Lines with this mode will become context lines if they are # not selected my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-'; for $i (1..$#{$text}) { + if ($i == $lo and defined($pre_insert)) { + $select_lines->(@$pre_insert); + } if ($lo <= $i and $i <= $hi) { $select_lines->($i); } else { @@ -1080,8 +1104,12 @@ sub select_hunk_lines { } } if ($i == $hi) { + if (defined($post_insert)) { + $select_lines->(@$post_insert); + } if (@$selected) { - ($lo, $hi) = splice(@$selected, 0, 2); + ($lo, $hi, $pre_insert, $post_insert) = + splice(@$selected, 0, 4); } } } @@ -1104,6 +1132,108 @@ sub select_hunk_lines { return $newhunk; } +sub pair_hunk_selection { + my $line_flags = shift; + my @ret; + my $reverse = $patch_mode_flavour{IS_REVERSE}; + for my $sel (@_) { + my $i = 0; + my ($del, $add) = @$sel; + my $ndel = @$del / 2; + my $nadd = @$add / 2; + # If there the same number of insertion and deletion + # groups then pair up in insertions and deletions. + if ($nadd == $ndel) { + while ($i < @{$del}) { + my ($ra, $rb) = $reverse ? ($add, $del) : + ($del, $add); + my ($starta, $enda) = @{$ra}[$i, $i + 1]; + my ($startb, $endb) = @{$rb}[$i, $i + 1]; + if ($line_flags->[$enda] & LAST_ADD_DEL and + $line_flags->[$endb] & NO_NEWLINE) { + $endb++; + } + if ($line_flags->[$enda] & NO_NEWLINE) { + $enda++; + } + push @ret, $reverse ? + ($starta, $enda, + [ $startb..$endb ], undef) : + ($starta, $enda, + undef, [ $startb..$endb ]); + $i += 2; + } + # If there are only deletions or only insertions or + # a) we're adding to the index and there is a single group + # of insertions starting at the first inserted line + # or + # b) we're resetting the index and there is a single group + # of deletions finishing with the last deleted line + # then it is okay just to have deletions followed by + # insertions. + } elsif (!$nadd or !$ndel or + (!$reverse and $ndel == 1 and + $line_flags->[$del->[1]] & LAST_ADD_DEL) or + ($reverse and $nadd == 1 and + $line_flags->[$add->[0]] & FIRST_ADD)) { + while ($i < @$del) { + my ($start, $end) = @{$del}[$i, $i + 1]; + if ($line_flags->[$end] & NO_NEWLINE) { + $end++; + } + push @ret, $start, $end, undef, undef; + $i += 2; + } + $i = 0; + while ($i < @$add) { + my ($start, $end) = @{$add}[$i, $i + 1]; + if ($line_flags->[$end] & NO_NEWLINE) { + $end++; + } + push @ret, $start, $end, undef, undef; + $i += 2; + } + } else { + printf STDERR __("unable to pair up insertions and deletions\n"); + return undef; + } + } + return \@ret; +} + +sub process_hunk_selection { + my ($labels, $line_flags) = @{shift()}{qw(LABELS LINE_FLAGS)}; + my ($blocks, $lines, $modes) = + @{$labels}{qw(BLOCKS LINES MODES)}; + my @selection = sort { $a <=> $b } @_; + unless (@selection) { + return []; + } + my $last_label = shift @selection; + my ($last_block, $last_line, $last_mode) = + ($blocks->[$last_label], $lines->[$last_label], $modes->[$last_label]); + my ($del, $add) = ([], []); + push @{$last_mode eq '-' ? $del : $add}, $last_line; + my @sel; + for my $label (@selection) { + my ($block, $line, $mode) = + ($blocks->[$label], $lines->[$label], $modes->[$label]); + if ($block != $last_block) { + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @sel, [ $del, $add ]; + ($del, $add) = ([], []); + push @{$mode eq '-' ? $del : $add}, $line; + } elsif ($line != $last_line + 1 or $mode ne $last_mode) { + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @{$mode eq '-' ? $del : $add}, $line; + } + ($last_block, $last_line, $last_mode) = ($block, $line, $mode); + } + push @{$last_mode eq '-' ? $del : $add}, $last_line; + push @sel, [ $del, $add ]; + pair_hunk_selection($line_flags, @sel); +} + sub check_hunk_label { my ($max_label, $label) = @_; if ($label < 1 or $label > $max_label) { @@ -1138,14 +1268,7 @@ sub parse_hunk_selection { return undef; } } - [ map { - my $line = $lines->[$_]; - if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) { - ($line, $line + 1); - } else { - ($line, $line); - } - } sort { $a <=> $b } keys(%selected) ]; + return process_hunk_selection($hunk, keys(%selected)); } sub display_hunk_lines { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 5b535a22d5..1d917ad018 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -489,7 +489,7 @@ test_expect_success 'setup expected diff' ' EOF ' -test_expect_failure 'can stage modified lines of patch (1)' ' +test_expect_success 'can stage modified lines of patch (1)' ' git reset && printf "%s\n" l "1,3 7-9 12" | EDITOR=: git add -p 2>error && @@ -498,6 +498,112 @@ test_expect_failure 'can stage modified lines of patch (1)' ' diff_cmp expected actual ' +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,7 @@ + 10 + -20 + +twenty + +thirty + 30 + 40 + 50 + -60 + +sixty + \ No newline at end of file + EOF +' + +test_expect_success 'can stage modified lines of patch (2)' ' + git reset && + printf "%s\n" l "2,6,8,9,12" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup HEAD' ' + git reset && + git apply --cached <<-\EOF && + diff --git a/test b/test + --- a/test + +++ b/test + @@ -3,4 +3,4 @@ + 30 + 40 + 50 + -60 + +60 + \ No newline at end of file + EOF + git commit +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + -30 + +fifty + 40 + 50 + 60 + \ No newline at end of file + EOF +' + +test_expect_success 'can stage modified lines of patch (3)' ' + git reset && + printf "%s\n" l "11,1,3 7-9" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + +test_expect_success 'setup expected diff' ' + cat >expected <<-\EOF + diff --git a/test b/test + index 0889435..cc6163b 100644 + --- a/test + +++ b/test + @@ -1,6 +1,8 @@ + -10 + +ten + +twenty + +thirty + 20 + 30 + 40 + 50 + -60 + \ No newline at end of file + +fifty + EOF +' + +test_expect_success 'can stage modified lines of patch (4)' ' + git reset && + printf "%s\n" l "9-6 11,1" | + EDITOR=: git add -p 2>error && + test_must_be_empty error && + git diff --cached HEAD >actual && + diff_cmp expected actual +' + test_expect_success 'patch mode ignores unmerged entries' ' git reset --hard && test_commit conflict && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly 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 0 siblings, 1 reply; 53+ messages in thread From: Junio C Hamano @ 2018-07-26 19:30 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: An interesting problem you are solving ;-) > For example given the hunk > 1 -* a longer description of the > 2 - first item > 3 -* second > 4 -* third > 5 +* first > 6 + second item > 7 +* the third item > > If the user selects 1,2,4–5,7 then we should generate > -* a longer description of the > - first item > +* first > * second > -* third > +* the third item I understood this example as "3 that is removal and 6 that is addition are excluded---we consider that these two lines (one in the pre-image and the other in the post-image) are _matching". As we are excluding a deletion, it becomes the common context line, and any removal or addition that appear before that must stay to happen before the common context line (i.e. removal of 1 and 2, and addition of 5, both precede common context line "second") and any removal or addition that appear after that must stay after the common context (i.e. removal of "third" and addition of "the third item" come after "second"). But then it is not clear to me what you mean by "group" below. What groups does the above example have? Ones before the retained "second" (i.e. removal 1, 2, 4 and addition 5) form one group and ones after it (i.e. removal 4 and addition 7) form another group? > Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Is this fixing any bug? I usually see "Reported-by" only for a bugfix patch but this seems to be adding a new feature (and lack of feature is usually not a bug). ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly 2018-07-26 19:30 ` Junio C Hamano @ 2018-07-27 10:19 ` Phillip Wood 2018-07-27 16:14 ` Junio C Hamano 0 siblings, 1 reply; 53+ messages in thread From: Phillip Wood @ 2018-07-27 10:19 UTC (permalink / raw) To: Junio C Hamano Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood Hi Junio, thanks for the comments On 26/07/18 20:30, Junio C Hamano wrote: > Phillip Wood <phillip.wood@talktalk.net> writes: > > An interesting problem you are solving ;-) > >> For example given the hunk >> 1 -* a longer description of the >> 2 - first item >> 3 -* second >> 4 -* third >> 5 +* first >> 6 + second item >> 7 +* the third item >> >> If the user selects 1,2,4–5,7 then we should generate >> -* a longer description of the >> - first item >> +* first >> * second >> -* third >> +* the third item > > I understood this example as "3 that is removal and 6 that is > addition are excluded---we consider that these two lines (one in the > pre-image and the other in the post-image) are _matching" As we> are excluding a deletion, it becomes the common context line, and > any removal or addition that appear before that must stay to happen > before the common context line (i.e. removal of 1 and 2, and > addition of 5, both precede common context line "second") and any > removal or addition that appear after that must stay after the > common context (i.e. removal of "third" and addition of "the third > item" come after "second"). > > But then it is not clear to me what you mean by "group" below. What > groups does the above example have? Ones before the retained > "second" (i.e. removal 1, 2, 4 and addition 5) form one group and > ones after it (i.e. removal 4 and addition 7) form another group? The code actually looks at the lines that are selected rather than omitted. So in the example above it groups them as [1,2] (because they are contiguous), [4],[5] (these are split because one is an insertion and one a deletion) and [7]. It then sees that there are two groups of deletions ([1,2],[4]) and two groups of insertions ([5],[7]) and so pairs up the deletions in [12] with the insertion in [5] and likewise with [4] and [7]. Lines 3 and 6 are never explicitly paired, although they basically behave as if they were. One the insertions are all paired up it walks over the list and creates a new hunk where the paired insertions come immediately after their corresponding deletions, unselected deletions are converted to context lines and unselected additions are dropped. > >> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > > Is this fixing any bug? I usually see "Reported-by" only for a > bugfix patch but this seems to be adding a new feature (and lack of > feature is usually not a bug). I guess I meant that the previous series was effectively buggy as it would give the wrong result for modified lines. I wanted to acknowledge that Ævar spent some time testing it and pointed that out. Best Wishes Phillip ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly 2018-07-27 10:19 ` Phillip Wood @ 2018-07-27 16:14 ` Junio C Hamano 0 siblings, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-07-27 16:14 UTC (permalink / raw) To: Phillip Wood Cc: Git Mailing List, Ævar Arnfjörð Bjarmason, Igor Djordjevic, Phillip Wood Phillip Wood <phillip.wood@talktalk.net> writes: > The code actually looks at the lines that are selected rather than > omitted. So in the example above it groups them as [1,2] (because they > are contiguous), [4],[5] (these are split because one is an insertion > and one a deletion) and [7]. It then sees that there are two groups of > deletions ([1,2],[4]) and two groups of insertions ([5],[7]) and so > pairs up the deletions in [12] with the insertion in [5] and likewise > with [4] and [7]. Lines 3 and 6 are never explicitly paired, although > they basically behave as if they were. One the insertions are all paired > up it walks over the list and creates a new hunk where the paired > insertions come immediately after their corresponding deletions, > unselected deletions are converted to context lines and unselected > additions are dropped. Now, without that much explanation in help text, can an average end user use the feature, specifically, understand the reason why the tool says it cannot handle a particular set of selected lines, and follow the workaround suggested by the tool to do it in two (or more) batches? That was the real question I was getting at. I haven't played with the feature long enough to answer that question. >>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> >> Is this fixing any bug? I usually see "Reported-by" only for a >> bugfix patch but this seems to be adding a new feature (and lack of >> feature is usually not a bug). > > I guess I meant that the previous series was effectively buggy as it > would give the wrong result for modified lines. I wanted to acknowledge > that Ævar spent some time testing it and pointed that out. Ah, I see. We generally throw these into "Helped-by", I'd think. Thanks. ^ permalink raw reply [flat|nested] 53+ messages in thread
* [PATCH v5 3/4] add -p: allow line selection to be inverted 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 15:58 ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood @ 2018-07-26 15:58 ` Phillip Wood 2018-07-26 15:58 ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood ` (2 subsequent siblings) 5 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If the list of lines to be selected begins with '-' select all the lines except the ones listed. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 3 ++- git-add--interactive.perl | 19 +++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 965e192a09..01ff4d7d24 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -342,7 +342,8 @@ 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. If the upper bound of a range of lines is omitted it defaults to -the last line. +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 7e4daee2fc..63541d0f90 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1247,8 +1247,21 @@ sub parse_hunk_selection { my ($hunk, $line) = @_; my $lines = $hunk->{LABELS}->{LINES}; my $max_label = $#{$lines}; + my $invert = undef; my %selected; my @fields = split(/[,\s]+/, $line); + if (my ($rest) = ($fields[0] =~ /^-(.*)/)) { + $invert = 1; + if ($rest ne '') { + $fields[0] = $rest; + } else { + shift @fields; + unless (@fields) { + error_msg __("no lines to invert\n"); + return undef; + } + } + } for my $f (@fields) { if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { if ($hi eq '') { @@ -1268,6 +1281,12 @@ sub parse_hunk_selection { return undef; } } + if ($invert) { + my %inverted; + undef @inverted{1..$max_label}; + delete @inverted{keys(%selected)}; + %selected = %inverted; + } return process_hunk_selection($hunk, keys(%selected)); } diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 1d917ad018..2fd456017f 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' ' ' test_expect_success 'can reset individual lines of patch' ' - printf "%s\n" l 4,1,3 | + printf "%s\n" l -6,2,5 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* [PATCH v5 4/4] add -p: optimize line selection for short hunks 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood ` (2 preceding siblings ...) 2018-07-26 15:58 ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood @ 2018-07-26 15:58 ` 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 12:40 ` Ævar Arnfjörð Bjarmason 5 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw) To: Git Mailing List, Ævar Arnfjörð Bjarmason Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood From: Phillip Wood <phillip.wood@dunelm.org.uk> If there are fewer than ten changes in a hunk then make spaces optional when selecting individual lines. This means that for short hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- Documentation/git-add.txt | 9 +++++---- git-add--interactive.perl | 26 ++++++++++++++++++++++++++ t/t3701-add-interactive.sh | 2 +- 3 files changed, 32 insertions(+), 5 deletions(-) diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt index 01ff4d7d24..f3c81dfb11 100644 --- a/Documentation/git-add.txt +++ b/Documentation/git-add.txt @@ -340,10 +340,11 @@ patch:: 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. 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. +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 63541d0f90..054c1168a7 100755 --- a/git-add--interactive.perl +++ b/git-add--interactive.perl @@ -1243,6 +1243,29 @@ sub check_hunk_label { return 1; } +sub split_hunk_selection { + my @fields = @_; + my @ret; + for my $field (@fields) { + while ($field ne '') { + if ($field =~ /^[0-9]-$/) { + push @ret, $field; + last; + } elsif (my ($sel, $rest) = + ($field =~ /^([0-9](?:-[0-9])?)(.*)/)) { + push @ret, $sel; + $field = $rest; + } else { + error_msg sprintf + __("invalid hunk line '%s'\n"), + substr($field, 0, 1); + return (); + } + } + } + return @ret; +} + sub parse_hunk_selection { my ($hunk, $line) = @_; my $lines = $hunk->{LABELS}->{LINES}; @@ -1262,6 +1285,9 @@ sub parse_hunk_selection { } } } + if ($max_label < 10) { + @fields = split_hunk_selection(@fields) or return undef; + } for my $f (@fields) { if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) { if ($hi eq '') { diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 2fd456017f..b2b808275c 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' ' ' test_expect_success 'can reset individual lines of patch' ' - printf "%s\n" l -6,2,5 | + printf "%s\n" l -625 | EDITOR=: git reset -p 2>error && test_must_be_empty error && git diff --cached HEAD >actual && -- 2.18.0 ^ permalink raw reply related [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood ` (3 preceding siblings ...) 2018-07-26 15:58 ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood @ 2018-07-27 18:27 ` Ævar Arnfjörð Bjarmason 2018-07-28 10:08 ` Phillip Wood 2018-07-28 12:40 ` Ævar Arnfjörð Bjarmason 5 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-07-27 18:27 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano On Thu, Jul 26 2018, Phillip Wood wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > 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. I was going to review this, but can't find what it's based on, I can't apply 1/4 to master, next or pu. It seems to be based on some older version of master, e.g. 1/4 has this hunk: + elsif ($line =~ /^l/) { + unless ($other =~ /l/) { + error_msg __("Cannot select line by line\n"); + next; + } + my $newhunk = select_lines_loop($hunk[$ix]); + if ($newhunk) { + splice @hunk, $ix, 1, $newhunk; + } else { + next; + } + } elsif ($other =~ /s/ && $line =~ /^s/) { Which seems to conflict with your 4bdd6e7ce3 ("add -p: improve error messages", 2018-02-13). I could have tried to manually apply this, but figured I'd bounce this back to you... Having just skimmed through the patches themselves I agree with this approach of handling the simple case (as discussed before) and leaving the rest for some future change, but let's see about the details once I have this running. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 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 0 siblings, 0 replies; 53+ messages in thread From: Phillip Wood @ 2018-07-28 10:08 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Phillip Wood Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano On 27/07/18 19:27, Ævar Arnfjörð Bjarmason wrote: > > On Thu, Jul 26 2018, Phillip Wood wrote: > >> From: Phillip Wood <phillip.wood@dunelm.org.uk> >> >> 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. > > I was going to review this, but can't find what it's based on, I can't > apply 1/4 to master, next or pu. It seems to be based on some older > version of master, e.g. 1/4 has this hunk: > > + elsif ($line =~ /^l/) { > + unless ($other =~ /l/) { > + error_msg __("Cannot select line by line\n"); > + next; > + } > + my $newhunk = select_lines_loop($hunk[$ix]); > + if ($newhunk) { > + splice @hunk, $ix, 1, $newhunk; > + } else { > + next; > + } > + } > elsif ($other =~ /s/ && $line =~ /^s/) { > > Which seems to conflict with your 4bdd6e7ce3 ("add -p: improve error > messages", 2018-02-13). I could have tried to manually apply this, but > figured I'd bounce this back to you... Yes, I wasn't sure whether to rebase or not and in the end I didn't. The line below where you cut the cover letter message says it is based on f4d35a6b49 "add -p: fix counting empty context lines in edited patches". So you could just apply it there and test it. You can fetch it with git fetch https://github.com/phillipwood/git add-i-select-lines-v5 > Having just skimmed through the patches themselves I agree with this > approach of handling the simple case (as discussed before) and leaving > the rest for some future change, but let's see about the details once I > have this running. Thanks Phillip ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood ` (4 preceding siblings ...) 2018-07-27 18:27 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason @ 2018-07-28 12:40 ` Ævar Arnfjörð Bjarmason 2018-08-03 10:01 ` Phillip Wood 5 siblings, 1 reply; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-07-28 12:40 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano 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. ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 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 0 siblings, 2 replies; 53+ messages in thread From: Phillip Wood @ 2018-08-03 10:01 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason, Phillip Wood Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano Hi Ævar Thanks for looking at this. On 28/07/18 13:40, Ævar Arnfjörð Bjarmason wrote: > > 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... Thanks for point this out I meant to add some help and then forgot. As for exiting adding "q" could be a good idea, at the moment it follows the behavior of the other sub-prompts of 'add -i' which isn't that user friendly. > > 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... That's something I thought about but in the end I didn't do it, maybe I should look at it again as it makes it easier to get around the limitations of the selection grouping. On the other hand if I've just selected the lines I want it is convenient to move directly to the next hunk without having to somehow quit the current one. There are some questions about how the remaining lines of the hunk should be presented - should they be broken up if you end up with a big block of context in the middle after the first edit for instance. There's also a practical problem of coloring the hunk if the user has set interactive.diffFilter as it appears reading and writing to a process is tricky on Windows. Part of the commit message of 01143847db ("add--interactive: allow custom diff highlighting programs", 2016-2-27) reads 2. add--interactive will re-colorize a diff which has been hand-edited, but it won't have run through the filter. Fixing this is conceptually easy (just pipe the diff through the filter), but practically hard to do without using tempfiles (it would need to feed data to and read the result from the filter without deadlocking; this raises portability questions with respect to Windows). > > 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. I did wonder about forcing the user to explicitly pair lines but shied away from it as I assumed that is what would be wanted most of the time and it is awkward for the user to have to pair them up and it involved more error checking on the input to make sure they don't say 1:4 2:3. > > 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. Yes that needs explaining. To implement staging modified lines the code needs to pair up each deleted line with its replacement. It does this by grouping consecutive selected lines together, so it has a list of groups of deleted lines and another list of inserted lines, it then pairs the deletions and insertions by their index in the list. So in the last example lines 1 and 2 are grouped together so there is a single group of deletions and two groups of insertions. There is no way for it to tell if lines 1 and 2 should be replaced by line 4 and line 5 should inserted after 'c' or if 1 and 2 should be replaced by 4 and 5 with 'c' coming after. It seems clear for your comment and Junio's that I need to improve the documentation, I'm not sure if that will be enough though or do we need to change the behavior? [I'm beginning to see why all the other programs I tried while writing this (tig, gitg, gitk and mercurial's version of add -i) don't make any attempt to stage modified lines correctly, though I think git should have some way of doing it.] Best Wishes Phillip ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 2018-08-03 10:01 ` Phillip Wood @ 2018-08-03 16:51 ` Junio C Hamano 2018-08-03 17:59 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 53+ messages in thread From: Junio C Hamano @ 2018-08-03 16:51 UTC (permalink / raw) To: Phillip Wood Cc: Ævar Arnfjörð Bjarmason, Phillip Wood, Git Mailing List, Igor Djordjevic Phillip Wood <phillip.wood@talktalk.net> writes: > ... [I'm beginning to see why all the other programs I tried while > writing this (tig, gitg, gitk and mercurial's version of add -i) > don't make any attempt to stage modified lines correctly, though I > think git should have some way of doing it.] Yes, this is a kind of feature that one can propose and implement something that works well for some of the limited cases one uses, but not in other cases, and it becomes very hard to explain how to work around the implementation limitation---that is why I stopped at "split this hunk?" and did not go beyond it when I designed the original "incremental add" feature. I think the real reason why it is hard is that there is no good definition of "modified" in "stage modified lines", and worse, there is no good way to mechanically figure it out, because a patch only gives you "these were deleted" and "these are added", without giving you "this line in the deleted block corresponds to these two lines in the added block" (i.e. "this original one line was modified into this thing in the result"). ^ permalink raw reply [flat|nested] 53+ messages in thread
* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines 2018-08-03 10:01 ` Phillip Wood 2018-08-03 16:51 ` Junio C Hamano @ 2018-08-03 17:59 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 53+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2018-08-03 17:59 UTC (permalink / raw) To: phillip.wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano On Fri, Aug 03 2018, Phillip Wood wrote: [snip (all made sense)] > It seems clear for your comment and Junio's that I need to improve the > documentation, I'm not sure if that will be enough though or do we need > to change the behavior? [I'm beginning to see why all the other programs > I tried while writing this (tig, gitg, gitk and mercurial's version of > add -i) don't make any attempt to stage modified lines correctly, though > I think git should have some way of doing it.] I think this is ready to move forward conceptually, i.e. in these two review rounds I've been focusing on the UI this exposes, and whether this is even a viable thing to have conceptually. I'm convinced that it is, even though there's osme confusing edge cases (as noted in my mail) I don't see how those could be entire avoided, a feature like this will inherently have such edge cases. I've avoided going deep into how the actual code itself works / is implemented in these review rounds, figuring that given the topic it was better to move past RFC for that type of review once we had docs. Thanks for working on this. ^ permalink raw reply [flat|nested] 53+ messages in thread
end of thread, other threads:[~2018-08-03 18:00 UTC | newest] Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines 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 2018-08-03 10:01 ` Phillip Wood 2018-08-03 16:51 ` Junio C Hamano 2018-08-03 17:59 ` Ævar Arnfjörð Bjarmason
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).