git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [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 ` Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 29+ 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	[flat|nested] 29+ 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 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
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 29+ 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	[flat|nested] 29+ 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
  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 ` Gustavo Leite
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
  4 siblings, 0 replies; 29+ 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] 29+ 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
                   ` (2 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
  4 siblings, 4 replies; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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
                   ` (3 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)
  4 siblings, 4 replies; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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	[flat|nested] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ 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; 29+ 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] 29+ messages in thread

end of thread, back to index

Thread overview: 29+ 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 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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox