All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/3] add -p: select individual hunk lines
@ 2018-02-19 11:36 Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
                   ` (7 more replies)
  0 siblings, 8 replies; 53+ messages in thread
From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I need to update the add -i documentation but otherwise I think these
patches are OK so I thought I'd try and get some feedback. They build
on top of the recount fixes in [1]. The commit message for the first
patch describes the motivation:

"When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'."

[1] https://public-inbox.org/git/20180219112910.24471-1-phillip.wood@talktalk.net/T/#u

Phillip Wood (3):
  add -p: select individual hunk lines
  add -p: allow line selection to be inverted
  add -p: optimize line selection for short hunks

 git-add--interactive.perl  | 179 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  63 ++++++++++++++++
 2 files changed, 242 insertions(+)

-- 
2.16.1


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v1 1/3] add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
@ 2018-02-19 11:36 ` Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 2/3] add -p: allow line selection to be inverted Phillip Wood
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 136 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  63 +++++++++++++++++++++
 2 files changed, 199 insertions(+)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a64c0db57d62ab02ef718b8c8f821105132d9920..8a33796e1f6a564d0a27ba06c216dbb9848827b9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1003,6 +1003,126 @@ sub color_diff {
 	} @_;
 }
 
+sub label_hunk_lines {
+	local $_;
+	my $hunk = shift;
+	my $i = 0;
+	my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ];
+	$i > 1 and @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
+	return $i > 1;
+}
+
+sub select_hunk_lines {
+	my ($hunk, $selected) = @_;
+	my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)};
+	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
+	my ($push_eol, @newtext);
+	# Lines with this mode will become context lines if they are
+	# not selected
+	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
+	for $i (1..$#{$text}) {
+		my $mode = substr($text->[$i], 0, 1);
+		if ($mode eq '\\') {
+			push @newtext, $text->[$i] if ($push_eol);
+			undef $push_eol;
+		} elsif ($labels->[$i] and $selected->[$labels->[$i]]) {
+			push @newtext, $text->[$i];
+			if ($mode eq '+') {
+				$n_cnt++;
+			} else {
+				$o_cnt++;
+			}
+			$push_eol = 1;
+		} elsif ($mode eq ' ' or $mode eq $context_mode) {
+			push @newtext, ' ' . substr($text->[$i], 1);
+			$o_cnt++; $n_cnt++;
+			$push_eol = 1;
+		} else {
+			undef $push_eol;
+		}
+	}
+	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
+					parse_hunk_header($text->[0]);
+	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my $newhunk = {
+		TEXT => \@newtext,
+		DISPLAY => [ color_diff(@newtext) ],
+		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
+		TYPE => $hunk->{TYPE},
+		USE => 1,
+	};
+	# If this hunk has previously been edited add the offset delta
+	# of the old hunk to get the real delta from the original
+	# hunk.
+	$hunk->{OFS_DELTA} and $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	return $newhunk;
+}
+
+sub check_hunk_label {
+	my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]);
+	if ($label < 1 or $label > $max_label) {
+		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
+		return 0;
+	}
+	return 1;
+}
+
+sub parse_hunk_selection {
+	local $_;
+	my ($hunk, $line) = @_;
+	my $max_label = $hunk->{MAX_LABEL};
+	my @selected = (0) x ($max_label + 1);
+	my @fields = split(/[,\s]+/, $line);
+	for (@fields) {
+		if (/^([0-9]*)-([0-9]*)$/) {
+			if ($1 eq '' and $2 eq '') {
+				error_msg __("range '-' missing upper or lower bound\n");
+				return undef;
+			}
+			my $lo = $1 eq '' ? 1 : $1;
+			my $hi = $2 eq '' ? $max_label : $2;
+			check_hunk_label($hunk, $lo) or return undef;
+			check_hunk_label($hunk, $hi) or return undef;
+			$hi < $lo and ($lo, $hi) = ($hi, $lo);
+			@selected[$lo..$hi] = (1) x (1 + $hi - $lo);
+		} elsif (/^([0-9]+)$/) {
+			check_hunk_label($hunk, $1) or return undef;
+			$selected[$1] = 1;
+		} else {
+			error_msg sprintf(__("invalid hunk line '%s'\n"), $_);
+			return undef;
+		}
+	}
+	return \@selected;
+}
+
+sub display_hunk_lines {
+	my ($display, $labels, $max_label) =
+				@{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)};
+	my $width = int(log($max_label) / log(10)) + 1;
+	my $padding = ' ' x ($width + 1);
+	for my $i (0..$#{$display}) {
+		if ($labels->[$i]) {
+			printf '%*d %s', $width, $labels->[$i], $display->[$i];
+		} else {
+			print $padding . $display->[$i];
+		}
+	}
+}
+
+sub select_lines_loop {
+	my $hunk = shift;
+	display_hunk_lines($hunk);
+	my $selection = undef;
+	until (defined $selection) {
+		print colored $prompt_color, __("select lines? ");
+		my $text = <STDIN>;
+		defined $text and $text =~ /\S/ or return undef;
+		$selection = parse_hunk_selection($hunk, $text);
+	}
+	return select_hunk_lines($hunk, $selection);
+}
+
 my %edit_hunk_manually_modes = (
 	stage => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
@@ -1245,6 +1365,7 @@ j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
+l - select hunk lines to use
 s - split the current hunk into smaller hunks
 e - manually edit the current hunk
 ? - print help
@@ -1461,6 +1582,9 @@ sub patch_update_file {
 		if ($hunk[$ix]{TYPE} eq 'hunk') {
 			$other .= ',e';
 		}
+		if (label_hunk_lines($hunk[$ix])) {
+			$other .= ',l';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -1600,6 +1724,18 @@ sub patch_update_file {
 					next;
 				}
 			}
+			elsif ($line =~ /^l/) {
+				unless ($other =~ /l/) {
+					error_msg __("Cannot select line by line\n");
+					next;
+				}
+				my $newhunk = select_lines_loop($hunk[$ix]);
+				if ($newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				} else {
+					next;
+				}
+			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 0fb9c0e0f140e21ef7ad467c40b9211d29f53db6..caa80327c461785949eb2b9c919c253f4bef72cc 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -342,6 +342,61 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 	! grep "^+31" actual
 '
 
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,9 @@
+	+5
+	 10
+	 20
+	+21
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage individual lines of patch' '
+	git reset &&
+	printf 61 >>test &&
+	printf "%s\n" l "-2 4" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD | sed /^index/d >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	+5
+	 10
+	 20
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can reset individual lines of patch' '
+	printf "%s\n" l 2 |
+	EDITOR=: git reset -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD | sed /^index/d >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
@@ -558,4 +613,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
 	test_cmp expected-2 actual
 '
 
+test_expect_success 'add -p selecting lines works with pathological context lines' '
+	git reset &&
+	printf "%s\n" l 2 y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a > actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.16.1


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v1 2/3] add -p: allow line selection to be inverted
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
@ 2018-02-19 11:36 ` Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 15 ++++++++++++++-
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8a33796e1f6a564d0a27ba06c216dbb9848827b9..0e3960b1ecf004bff51d28d540f685a5dc91fad1 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1070,9 +1070,21 @@ sub check_hunk_label {
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
-	my $max_label = $hunk->{MAX_LABEL};
+	my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
 	my @selected = (0) x ($max_label + 1);
 	my @fields = split(/[,\s]+/, $line);
+	if ($fields[0] =~ /^\^(.*)/) {
+		$invert = 1;
+		if ($1 ne '') {
+			$fields[0] = $1;
+		} else {
+			shift @fields;
+			unless (@fields) {
+				error_msg __("no lines to invert\n");
+				return undef;
+			}
+		}
+	}
 	for (@fields) {
 		if (/^([0-9]*)-([0-9]*)$/) {
 			if ($1 eq '' and $2 eq '') {
@@ -1093,6 +1105,7 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
+	$invert and @selected = map { !$_ } @selected;
 	return \@selected;
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index caa80327c461785949eb2b9c919c253f4bef72cc..4ae706fd121f157e9cbd93ec293f45ce2a3a53b5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -390,7 +390,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l 2 |
+	printf "%s\n" l "^1 3" |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD | sed /^index/d >actual &&
-- 
2.16.1


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v1 3/3] add -p: optimize line selection for short hunks
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
  2018-02-19 11:36 ` [PATCH v1 2/3] add -p: allow line selection to be inverted Phillip Wood
@ 2018-02-19 11:36 ` Phillip Wood
  2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-02-19 11:36 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are fewer than ten changes in a hunk then make spaces
optional when selecting individual lines. This means that for short
hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 30 ++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 0e3960b1ecf004bff51d28d540f685a5dc91fad1..3d9720af03eb113c7f3d2b73f17b4b51a7685bf3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1067,6 +1067,33 @@ sub check_hunk_label {
 	return 1;
 }
 
+sub split_hunk_selection {
+	local $_;
+	my @fields = @_;
+	my @ret;
+	for (@fields) {
+		if (/^(-[0-9])(.*)/) {
+			push @ret, $1;
+			$_ = $2;
+		}
+		while ($_ ne '') {
+			if (/^[0-9]-$/) {
+				push @ret, $_;
+				last;
+			} elsif (/^([0-9](?:-[0-9])?)(.*)/) {
+				push @ret, $1;
+				$_ = $2;
+			} else {
+				error_msg sprintf
+				    __("invalid hunk line '%s'\n"),
+				    substr($_, 0, 1);
+				return ();
+			}
+		}
+	}
+	return @ret;
+}
+
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
@@ -1085,6 +1112,9 @@ sub parse_hunk_selection {
 			}
 		}
 	}
+	if ($max_label < 10) {
+		@fields = split_hunk_selection(@fields) or return undef;
+	}
 	for (@fields) {
 		if (/^([0-9]*)-([0-9]*)$/) {
 			if ($1 eq '' and $2 eq '') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 4ae706fd121f157e9cbd93ec293f45ce2a3a53b5..c6d847dc495c92782e37ef7b0e2800d7936aabd7 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -390,7 +390,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l "^1 3" |
+	printf "%s\n" l ^13 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD | sed /^index/d >actual &&
-- 
2.16.1


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v1 0/3] add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
                   ` (2 preceding siblings ...)
  2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood
@ 2018-02-19 12:20 ` Gustavo Leite
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 53+ messages in thread
From: Gustavo Leite @ 2018-02-19 12:20 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List

2018-02-19 8:36 GMT-03:00 Phillip Wood <phillip.wood@talktalk.net>:
>
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."

This is an interesting (and needed feature). Would be nice to see it merged.

--
Gustavo Leite

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v2 0/3] add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
                   ` (3 preceding siblings ...)
  2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite
@ 2018-03-06 10:17 ` Phillip Wood
  2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
                     ` (3 more replies)
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
                   ` (2 subsequent siblings)
  7 siblings, 4 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I've added some documentation to git-add.txt for the new selection
mode and cleaned up some style issues, otherwise these are unchanged
since v1.  These patches build on top of the recount fixes in [1]. The
commit message for the first patch describes the motivation:

"When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'."

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

Interdiff from v1:

 Documentation/git-add.txt  |  9 +++++++++
 git-add--interactive.perl  | 19 ++++++++++++++-----
 t/t3701-add-interactive.sh | 12 +++++++-----
 3 files changed, 30 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d50fa339dc..d52acfc722 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -332,10 +332,19 @@ patch::
        J - leave this hunk undecided, see next hunk
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
+       l - select hunk lines to use
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        ? - print help
 +
+If you press "l" then the hunk will be reprinted with each insertion
+or deletion labelled with a number and you will be prompted to enter
+which lines you wish to select. Individual line numbers should be
+separated by a space or comma (these can be omitted if there are fewer
+than ten labelled lines), to specify a range of lines use a dash
+between them. To invert the selection prefix it with "\^" so "^3-5,8"
+will select everything except lines 3, 4, 5 and 8.
++
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index aa474c5149..9a6bcd5085 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1018,8 +1018,11 @@ sub label_hunk_lines {
 	my $hunk = shift;
 	my $i = 0;
 	my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ];
-	$i > 1 and @{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
-	return $i > 1;
+	if ($i > 1) {
+		@{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
+		return 1;
+	}
+	return 0;
 }
 
 sub select_hunk_lines {
@@ -1064,7 +1067,9 @@ sub select_hunk_lines {
 	# If this hunk has previously been edited add the offset delta
 	# of the old hunk to get the real delta from the original
 	# hunk.
-	$hunk->{OFS_DELTA} and $newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	if ($hunk->{OFS_DELTA}) {
+		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	}
 	return $newhunk;
 }
 
@@ -1135,7 +1140,9 @@ sub parse_hunk_selection {
 			my $hi = $2 eq '' ? $max_label : $2;
 			check_hunk_label($hunk, $lo) or return undef;
 			check_hunk_label($hunk, $hi) or return undef;
-			$hi < $lo and ($lo, $hi) = ($hi, $lo);
+			if ($hi < $lo) {
+				($lo, $hi) = ($hi, $lo);
+			}
 			@selected[$lo..$hi] = (1) x (1 + $hi - $lo);
 		} elsif (/^([0-9]+)$/) {
 			check_hunk_label($hunk, $1) or return undef;
@@ -1145,7 +1152,9 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
-	$invert and @selected = map { !$_ } @selected;
+	if ($invert) {
+		@selected = map { !$_ } @selected;
+	}
 	return \@selected;
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 7bea4a2341..d3bce154da 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -363,6 +363,7 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 test_expect_success 'setup expected diff' '
 	cat >expected <<-\EOF
 	diff --git a/test b/test
+	index 0889435..341cc6b 100644
 	--- a/test
 	+++ b/test
 	@@ -1,6 +1,9 @@
@@ -385,13 +386,14 @@ test_expect_success 'can stage individual lines of patch' '
 	printf "%s\n" l "-2 4" |
 	EDITOR=: git add -p 2>error &&
 	test_must_be_empty error &&
-	git diff --cached HEAD | sed /^index/d >actual &&
-	test_cmp expected actual
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
 '
 
 test_expect_success 'setup expected diff' '
 	cat >expected <<-\EOF
 	diff --git a/test b/test
+	index 0889435..cc6163b 100644
 	--- a/test
 	+++ b/test
 	@@ -1,6 +1,8 @@
@@ -411,8 +413,8 @@ test_expect_success 'can reset individual lines of patch' '
 	printf "%s\n" l ^13 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
-	git diff --cached HEAD | sed /^index/d >actual &&
-	test_cmp expected actual
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
 '
 
 test_expect_success 'patch mode ignores unmerged entries' '
@@ -635,7 +637,7 @@ test_expect_success 'add -p selecting lines works with pathological context line
 	git reset &&
 	printf "%s\n" l 2 y |
 	GIT_EDITOR=./editor git add -p &&
-	git cat-file blob :a > actual &&
+	git cat-file blob :a >actual &&
 	test_cmp expected-2 actual
 '
 

Phillip Wood (3):
  add -p: select individual hunk lines
  add -p: allow line selection to be inverted
  add -p: optimize line selection for short hunks

 Documentation/git-add.txt  |   9 +++
 git-add--interactive.perl  | 188 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  65 ++++++++++++++++
 3 files changed, 262 insertions(+)

-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v2 1/3] add -p: select individual hunk lines
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
@ 2018-03-06 10:17   ` Phillip Wood
  2018-03-06 20:29     ` Igor Djordjevic
  2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |   7 +++
 git-add--interactive.perl  | 143 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  65 +++++++++++++++++++++
 3 files changed, 215 insertions(+)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d50fa339dc..ad33fda9a2 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -332,10 +332,17 @@ patch::
        J - leave this hunk undecided, see next hunk
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
+       l - select hunk lines to use
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        ? - print help
 +
+If you press "l" then the hunk will be reprinted with each insertion
+or deletion labelled with a number and you will be prompted to enter
+which lines you wish to select. Individual line numbers should be
+separated by a space or comma, to specify a range of lines use a dash
+between them.
++
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f83e7450ad..a273b41e95 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1013,6 +1013,133 @@ sub color_diff {
 	} @_;
 }
 
+sub label_hunk_lines {
+	local $_;
+	my $hunk = shift;
+	my $i = 0;
+	my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ];
+	if ($i > 1) {
+		@{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
+		return 1;
+	}
+	return 0;
+}
+
+sub select_hunk_lines {
+	my ($hunk, $selected) = @_;
+	my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)};
+	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
+	my ($push_eol, @newtext);
+	# Lines with this mode will become context lines if they are
+	# not selected
+	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
+	for $i (1..$#{$text}) {
+		my $mode = substr($text->[$i], 0, 1);
+		if ($mode eq '\\') {
+			push @newtext, $text->[$i] if ($push_eol);
+			undef $push_eol;
+		} elsif ($labels->[$i] and $selected->[$labels->[$i]]) {
+			push @newtext, $text->[$i];
+			if ($mode eq '+') {
+				$n_cnt++;
+			} else {
+				$o_cnt++;
+			}
+			$push_eol = 1;
+		} elsif ($mode eq ' ' or $mode eq $context_mode) {
+			push @newtext, ' ' . substr($text->[$i], 1);
+			$o_cnt++; $n_cnt++;
+			$push_eol = 1;
+		} else {
+			undef $push_eol;
+		}
+	}
+	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
+					parse_hunk_header($text->[0]);
+	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my $newhunk = {
+		TEXT => \@newtext,
+		DISPLAY => [ color_diff(@newtext) ],
+		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
+		TYPE => $hunk->{TYPE},
+		USE => 1,
+	};
+	# If this hunk has previously been edited add the offset delta
+	# of the old hunk to get the real delta from the original
+	# hunk.
+	if ($hunk->{OFS_DELTA}) {
+		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	}
+	return $newhunk;
+}
+
+sub check_hunk_label {
+	my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]);
+	if ($label < 1 or $label > $max_label) {
+		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
+		return 0;
+	}
+	return 1;
+}
+
+sub parse_hunk_selection {
+	local $_;
+	my ($hunk, $line) = @_;
+	my $max_label = $hunk->{MAX_LABEL};
+	my @selected = (0) x ($max_label + 1);
+	my @fields = split(/[,\s]+/, $line);
+	for (@fields) {
+		if (/^([0-9]*)-([0-9]*)$/) {
+			if ($1 eq '' and $2 eq '') {
+				error_msg __("range '-' missing upper or lower bound\n");
+				return undef;
+			}
+			my $lo = $1 eq '' ? 1 : $1;
+			my $hi = $2 eq '' ? $max_label : $2;
+			check_hunk_label($hunk, $lo) or return undef;
+			check_hunk_label($hunk, $hi) or return undef;
+			if ($hi < $lo) {
+				($lo, $hi) = ($hi, $lo);
+			}
+			@selected[$lo..$hi] = (1) x (1 + $hi - $lo);
+		} elsif (/^([0-9]+)$/) {
+			check_hunk_label($hunk, $1) or return undef;
+			$selected[$1] = 1;
+		} else {
+			error_msg sprintf(__("invalid hunk line '%s'\n"), $_);
+			return undef;
+		}
+	}
+	return \@selected;
+}
+
+sub display_hunk_lines {
+	my ($display, $labels, $max_label) =
+				@{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)};
+	my $width = int(log($max_label) / log(10)) + 1;
+	my $padding = ' ' x ($width + 1);
+	for my $i (0..$#{$display}) {
+		if ($labels->[$i]) {
+			printf '%*d %s', $width, $labels->[$i], $display->[$i];
+		} else {
+			print $padding . $display->[$i];
+		}
+	}
+}
+
+sub select_lines_loop {
+	my $hunk = shift;
+	display_hunk_lines($hunk);
+	my $selection = undef;
+	until (defined $selection) {
+		print colored $prompt_color, __("select lines? ");
+		my $text = <STDIN>;
+		defined $text and $text =~ /\S/ or return undef;
+		$selection = parse_hunk_selection($hunk, $text);
+	}
+	return select_hunk_lines($hunk, $selection);
+}
+
 my %edit_hunk_manually_modes = (
 	stage => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
@@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
+l - select hunk lines to use
 s - split the current hunk into smaller hunks
 e - manually edit the current hunk
 ? - print help
@@ -1471,6 +1599,9 @@ sub patch_update_file {
 		if ($hunk[$ix]{TYPE} eq 'hunk') {
 			$other .= ',e';
 		}
+		if (label_hunk_lines($hunk[$ix])) {
+			$other .= ',l';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -1610,6 +1741,18 @@ sub patch_update_file {
 					next;
 				}
 			}
+			elsif ($line =~ /^l/) {
+				unless ($other =~ /l/) {
+					error_msg __("Cannot select line by line\n");
+					next;
+				}
+				my $newhunk = select_lines_loop($hunk[$ix]);
+				if ($newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				} else {
+					next;
+				}
+			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index a9a9478a29..65c8c3354b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 	! grep "^+31" actual
 '
 
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..341cc6b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,9 @@
+	+5
+	 10
+	 20
+	+21
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage individual lines of patch' '
+	git reset &&
+	printf 61 >>test &&
+	printf "%s\n" l "-2 4" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	+5
+	 10
+	 20
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can reset individual lines of patch' '
+	printf "%s\n" l 2 |
+	EDITOR=: git reset -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
@@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
 	test_cmp expected-2 actual
 '
 
+test_expect_success 'add -p selecting lines works with pathological context lines' '
+	git reset &&
+	printf "%s\n" l 2 y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a >actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
  2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
@ 2018-03-06 10:17   ` Phillip Wood
  2018-03-06 19:57     ` Junio C Hamano
  2018-03-06 20:41     ` Igor Djordjevic
  2018-03-06 10:17   ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood
  2018-03-06 20:19   ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic
  3 siblings, 2 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  3 ++-
 git-add--interactive.perl  | 17 ++++++++++++++++-
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index ad33fda9a2..0e2c11e97b 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -341,7 +341,8 @@ If you press "l" then the hunk will be reprinted with each insertion
 or deletion labelled with a number and you will be prompted to enter
 which lines you wish to select. Individual line numbers should be
 separated by a space or comma, to specify a range of lines use a dash
-between them.
+between them. To invert the selection prefix it with "\^" so "^3-5,8"
+will select everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a273b41e95..6fa3d0a87c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1085,9 +1085,21 @@ sub check_hunk_label {
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
-	my $max_label = $hunk->{MAX_LABEL};
+	my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
 	my @selected = (0) x ($max_label + 1);
 	my @fields = split(/[,\s]+/, $line);
+	if ($fields[0] =~ /^\^(.*)/) {
+		$invert = 1;
+		if ($1 ne '') {
+			$fields[0] = $1;
+		} else {
+			shift @fields;
+			unless (@fields) {
+				error_msg __("no lines to invert\n");
+				return undef;
+			}
+		}
+	}
 	for (@fields) {
 		if (/^([0-9]*)-([0-9]*)$/) {
 			if ($1 eq '' and $2 eq '') {
@@ -1110,6 +1122,9 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
+	if ($invert) {
+		@selected = map { !$_ } @selected;
+	}
 	return \@selected;
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 65c8c3354b..89c0e73f2b 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l 2 |
+	printf "%s\n" l "^1 3" |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v2 3/3] add -p: optimize line selection for short hunks
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
  2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
  2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
@ 2018-03-06 10:17   ` Phillip Wood
  2018-03-06 20:33     ` Igor Djordjevic
  2018-03-06 20:19   ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic
  3 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-03-06 10:17 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are fewer than ten changes in a hunk then make spaces
optional when selecting individual lines. This means that for short
hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  3 ++-
 git-add--interactive.perl  | 30 ++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 0e2c11e97b..d52acfc722 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -340,7 +340,8 @@ patch::
 If you press "l" then the hunk will be reprinted with each insertion
 or deletion labelled with a number and you will be prompted to enter
 which lines you wish to select. Individual line numbers should be
-separated by a space or comma, to specify a range of lines use a dash
+separated by a space or comma (these can be omitted if there are fewer
+than ten labelled lines), to specify a range of lines use a dash
 between them. To invert the selection prefix it with "\^" so "^3-5,8"
 will select everything except lines 3, 4, 5 and 8.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 6fa3d0a87c..9a6bcd5085 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1082,6 +1082,33 @@ sub check_hunk_label {
 	return 1;
 }
 
+sub split_hunk_selection {
+	local $_;
+	my @fields = @_;
+	my @ret;
+	for (@fields) {
+		if (/^(-[0-9])(.*)/) {
+			push @ret, $1;
+			$_ = $2;
+		}
+		while ($_ ne '') {
+			if (/^[0-9]-$/) {
+				push @ret, $_;
+				last;
+			} elsif (/^([0-9](?:-[0-9])?)(.*)/) {
+				push @ret, $1;
+				$_ = $2;
+			} else {
+				error_msg sprintf
+				    __("invalid hunk line '%s'\n"),
+				    substr($_, 0, 1);
+				return ();
+			}
+		}
+	}
+	return @ret;
+}
+
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
@@ -1100,6 +1127,9 @@ sub parse_hunk_selection {
 			}
 		}
 	}
+	if ($max_label < 10) {
+		@fields = split_hunk_selection(@fields) or return undef;
+	}
 	for (@fields) {
 		if (/^([0-9]*)-([0-9]*)$/) {
 			if ($1 eq '' and $2 eq '') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 89c0e73f2b..d3bce154da 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l "^1 3" |
+	printf "%s\n" l ^13 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
@ 2018-03-06 19:57     ` Junio C Hamano
  2018-03-08 11:05       ` Phillip Wood
  2018-03-06 20:41     ` Igor Djordjevic
  1 sibling, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-03-06 19:57 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> If the list of lines to be selected begins with '^' select all the
> lines except the ones listed.

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

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 0/3] add -p: select individual hunk lines
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
                     ` (2 preceding siblings ...)
  2018-03-06 10:17   ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood
@ 2018-03-06 20:19   ` Igor Djordjevic
  2018-03-06 21:03     ` Junio C Hamano
  3 siblings, 1 reply; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 20:19 UTC (permalink / raw)
  To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite

Hi Phillip,

On 06/03/2018 11:17, Phillip Wood wrote:
> 
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> I've added some documentation to git-add.txt for the new selection
> mode and cleaned up some style issues, otherwise these are unchanged
> since v1.  These patches build on top of the recount fixes in [1]. The
> commit message for the first patch describes the motivation:
> 
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."
> 
> [1] https://public-inbox.org/git/xmqqbmg29x1n.fsf@gitster-ct.c.googlers.com/T/#m01d0f1af90f32b698e583b56f8e53b986bcec7c6

Nice, thank you :)

A small nitpick - I see you use phrasing like "select lines", where 
the other commands usually talk about "staging", instead, so "stage 
lines" might be more aligned with the existing text.

I`ll quickly go through the patches regarding this (not being of much 
help for the code itself at the moment, sorry!).

Regards, Buga

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 1/3] add -p: select individual hunk lines
  2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
@ 2018-03-06 20:29     ` Igor Djordjevic
  2018-03-06 21:33       ` Igor Djordjevic
  0 siblings, 1 reply; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 20:29 UTC (permalink / raw)
  To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite

On 06/03/2018 11:17, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-add.txt  |   7 +++
>  git-add--interactive.perl  | 143 +++++++++++++++++++++++++++++++++++++++++++++
>  t/t3701-add-interactive.sh |  65 +++++++++++++++++++++
>  3 files changed, 215 insertions(+)
> 
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index d50fa339dc..ad33fda9a2 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -332,10 +332,17 @@ patch::
>         J - leave this hunk undecided, see next hunk
>         k - leave this hunk undecided, see previous undecided hunk
>         K - leave this hunk undecided, see previous hunk
> +       l - select hunk lines to use

Might be more surrounding context aligned to say "stage hunk lines" 
here (phrase "stage", instead of "select to use").

>         s - split the current hunk into smaller hunks
>         e - manually edit the current hunk
>         ? - print help
>  +
> +If you press "l" then the hunk will be reprinted with each insertion
> +or deletion labelled with a number and you will be prompted to enter
> +which lines you wish to select. Individual line numbers should be

Likewise, s/you wish to select/you wish to stage/.

> +separated by a space or comma, to specify a range of lines use a dash
> +between them.
> ++
>  After deciding the fate for all hunks, if there is any hunk
>  that was chosen, the index is updated with the selected hunks.
>  +
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index f83e7450ad..a273b41e95 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1013,6 +1013,133 @@ sub color_diff {
>  	} @_;
>  }
>  
> +sub label_hunk_lines {
> +	local $_;
> +	my $hunk = shift;
> +	my $i = 0;
> +	my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ];
> +	if ($i > 1) {
> +		@{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +sub select_hunk_lines {

This is just something I`ve spotted, but I have no actual idea if 
renaming this to "stage_hunk_lines" might be better, too, or not 
(depending on the surrounding code context), so please take this with 
a big grain of salt.

> +	my ($hunk, $selected) = @_;
> +	my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)};
> +	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
> +	my ($push_eol, @newtext);
> +	# Lines with this mode will become context lines if they are
> +	# not selected
> +	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
> +	for $i (1..$#{$text}) {
> +		my $mode = substr($text->[$i], 0, 1);
> +		if ($mode eq '\\') {
> +			push @newtext, $text->[$i] if ($push_eol);
> +			undef $push_eol;
> +		} elsif ($labels->[$i] and $selected->[$labels->[$i]]) {
> +			push @newtext, $text->[$i];
> +			if ($mode eq '+') {
> +				$n_cnt++;
> +			} else {
> +				$o_cnt++;
> +			}
> +			$push_eol = 1;
> +		} elsif ($mode eq ' ' or $mode eq $context_mode) {
> +			push @newtext, ' ' . substr($text->[$i], 1);
> +			$o_cnt++; $n_cnt++;
> +			$push_eol = 1;
> +		} else {
> +			undef $push_eol;
> +		}
> +	}
> +	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
> +					parse_hunk_header($text->[0]);
> +	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
> +	my $newhunk = {
> +		TEXT => \@newtext,
> +		DISPLAY => [ color_diff(@newtext) ],
> +		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
> +		TYPE => $hunk->{TYPE},
> +		USE => 1,
> +	};
> +	# If this hunk has previously been edited add the offset delta
> +	# of the old hunk to get the real delta from the original
> +	# hunk.
> +	if ($hunk->{OFS_DELTA}) {
> +		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
> +	}
> +	return $newhunk;
> +}
> +
> +sub check_hunk_label {
> +	my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]);
> +	if ($label < 1 or $label > $max_label) {
> +		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
> +		return 0;
> +	}
> +	return 1;
> +}
> +
> +sub parse_hunk_selection {
> +	local $_;
> +	my ($hunk, $line) = @_;
> +	my $max_label = $hunk->{MAX_LABEL};
> +	my @selected = (0) x ($max_label + 1);
> +	my @fields = split(/[,\s]+/, $line);
> +	for (@fields) {
> +		if (/^([0-9]*)-([0-9]*)$/) {
> +			if ($1 eq '' and $2 eq '') {
> +				error_msg __("range '-' missing upper or lower bound\n");
> +				return undef;
> +			}
> +			my $lo = $1 eq '' ? 1 : $1;
> +			my $hi = $2 eq '' ? $max_label : $2;
> +			check_hunk_label($hunk, $lo) or return undef;
> +			check_hunk_label($hunk, $hi) or return undef;
> +			if ($hi < $lo) {
> +				($lo, $hi) = ($hi, $lo);
> +			}
> +			@selected[$lo..$hi] = (1) x (1 + $hi - $lo);
> +		} elsif (/^([0-9]+)$/) {
> +			check_hunk_label($hunk, $1) or return undef;
> +			$selected[$1] = 1;
> +		} else {
> +			error_msg sprintf(__("invalid hunk line '%s'\n"), $_);
> +			return undef;
> +		}
> +	}
> +	return \@selected;
> +}
> +
> +sub display_hunk_lines {
> +	my ($display, $labels, $max_label) =
> +				@{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)};
> +	my $width = int(log($max_label) / log(10)) + 1;
> +	my $padding = ' ' x ($width + 1);
> +	for my $i (0..$#{$display}) {
> +		if ($labels->[$i]) {
> +			printf '%*d %s', $width, $labels->[$i], $display->[$i];
> +		} else {
> +			print $padding . $display->[$i];
> +		}
> +	}
> +}
> +
> +sub select_lines_loop {
> +	my $hunk = shift;
> +	display_hunk_lines($hunk);
> +	my $selection = undef;
> +	until (defined $selection) {
> +		print colored $prompt_color, __("select lines? ");
> +		my $text = <STDIN>;
> +		defined $text and $text =~ /\S/ or return undef;
> +		$selection = parse_hunk_selection($hunk, $text);
> +	}
> +	return select_hunk_lines($hunk, $selection);
> +}
> +
>  my %edit_hunk_manually_modes = (
>  	stage => N__(
>  "If the patch applies cleanly, the edited hunk will immediately be
> @@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk
>  J - leave this hunk undecided, see next hunk
>  k - leave this hunk undecided, see previous undecided hunk
>  K - leave this hunk undecided, see previous hunk
> +l - select hunk lines to use

s/select hunk lines to use/stage hunk lines/

>  s - split the current hunk into smaller hunks
>  e - manually edit the current hunk
>  ? - print help
> @@ -1471,6 +1599,9 @@ sub patch_update_file {
>  		if ($hunk[$ix]{TYPE} eq 'hunk') {
>  			$other .= ',e';
>  		}
> +		if (label_hunk_lines($hunk[$ix])) {
> +			$other .= ',l';
> +		}
>  		for (@{$hunk[$ix]{DISPLAY}}) {
>  			print;
>  		}
> @@ -1610,6 +1741,18 @@ sub patch_update_file {
>  					next;
>  				}
>  			}
> +			elsif ($line =~ /^l/) {
> +				unless ($other =~ /l/) {
> +					error_msg __("Cannot select line by line\n");
> +					next;
> +				}
> +				my $newhunk = select_lines_loop($hunk[$ix]);
> +				if ($newhunk) {
> +					splice @hunk, $ix, 1, $newhunk;
> +				} else {
> +					next;
> +				}
> +			}
>  			elsif ($other =~ /s/ && $line =~ /^s/) {
>  				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
>  				if (1 < @split) {
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index a9a9478a29..65c8c3354b 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
>  	! grep "^+31" actual
>  '
>  
> +test_expect_success 'setup expected diff' '
> +	cat >expected <<-\EOF
> +	diff --git a/test b/test
> +	index 0889435..341cc6b 100644
> +	--- a/test
> +	+++ b/test
> +	@@ -1,6 +1,9 @@
> +	+5
> +	 10
> +	 20
> +	+21
> +	 30
> +	 40
> +	 50
> +	 60
> +	+61
> +	\ No newline at end of file
> +	EOF
> +'
> +
> +test_expect_success 'can stage individual lines of patch' '

Here, you`re actually using "stage lines" yourself, good ;)

> +	git reset &&
> +	printf 61 >>test &&
> +	printf "%s\n" l "-2 4" |
> +	EDITOR=: git add -p 2>error &&
> +	test_must_be_empty error &&
> +	git diff --cached HEAD >actual &&
> +	diff_cmp expected actual
> +'
> +
> +test_expect_success 'setup expected diff' '
> +	cat >expected <<-\EOF
> +	diff --git a/test b/test
> +	index 0889435..cc6163b 100644
> +	--- a/test
> +	+++ b/test
> +	@@ -1,6 +1,8 @@
> +	+5
> +	 10
> +	 20
> +	 30
> +	 40
> +	 50
> +	 60
> +	+61
> +	\ No newline at end of file
> +	EOF
> +'
> +
> +test_expect_success 'can reset individual lines of patch' '
> +	printf "%s\n" l 2 |
> +	EDITOR=: git reset -p 2>error &&
> +	test_must_be_empty error &&
> +	git diff --cached HEAD >actual &&
> +	diff_cmp expected actual
> +'
> +
>  test_expect_success 'patch mode ignores unmerged entries' '
>  	git reset --hard &&
>  	test_commit conflict &&
> @@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
>  	test_cmp expected-2 actual
>  '
>  
> +test_expect_success 'add -p selecting lines works with pathological context lines' '

Maybe s/selecting lines/staging lines/ ?

> +	git reset &&
> +	printf "%s\n" l 2 y |
> +	GIT_EDITOR=./editor git add -p &&
> +	git cat-file blob :a >actual &&
> +	test_cmp expected-2 actual
> +'
> +
>  test_done
> 

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 3/3] add -p: optimize line selection for short hunks
  2018-03-06 10:17   ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood
@ 2018-03-06 20:33     ` Igor Djordjevic
  0 siblings, 0 replies; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 20:33 UTC (permalink / raw)
  To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite

On 06/03/2018 11:17, Phillip Wood wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> If there are fewer than ten changes in a hunk then make spaces
> optional when selecting individual lines. This means that for short

Not sure if using s/selecting individual lines/staging individual lines/ 
would make sense here, too, but not that important (as you later do 
say "to stage lines").

> hunks one can just type -357 to stage lines 1, 2, 3, 5 & 7.
> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-add.txt  |  3 ++-
>  git-add--interactive.perl  | 30 ++++++++++++++++++++++++++++++
>  t/t3701-add-interactive.sh |  2 +-
>  3 files changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 0e2c11e97b..d52acfc722 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -340,7 +340,8 @@ patch::
>  If you press "l" then the hunk will be reprinted with each insertion
>  or deletion labelled with a number and you will be prompted to enter
>  which lines you wish to select. Individual line numbers should be
> -separated by a space or comma, to specify a range of lines use a dash
> +separated by a space or comma (these can be omitted if there are fewer
> +than ten labelled lines), to specify a range of lines use a dash
>  between them. To invert the selection prefix it with "\^" so "^3-5,8"
>  will select everything except lines 3, 4, 5 and 8.
>  +
> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> index 6fa3d0a87c..9a6bcd5085 100755
> --- a/git-add--interactive.perl
> +++ b/git-add--interactive.perl
> @@ -1082,6 +1082,33 @@ sub check_hunk_label {
>  	return 1;
>  }
>  
> +sub split_hunk_selection {
> +	local $_;
> +	my @fields = @_;
> +	my @ret;
> +	for (@fields) {
> +		if (/^(-[0-9])(.*)/) {
> +			push @ret, $1;
> +			$_ = $2;
> +		}
> +		while ($_ ne '') {
> +			if (/^[0-9]-$/) {
> +				push @ret, $_;
> +				last;
> +			} elsif (/^([0-9](?:-[0-9])?)(.*)/) {
> +				push @ret, $1;
> +				$_ = $2;
> +			} else {
> +				error_msg sprintf
> +				    __("invalid hunk line '%s'\n"),
> +				    substr($_, 0, 1);
> +				return ();
> +			}
> +		}
> +	}
> +	return @ret;
> +}
> +
>  sub parse_hunk_selection {
>  	local $_;
>  	my ($hunk, $line) = @_;
> @@ -1100,6 +1127,9 @@ sub parse_hunk_selection {
>  			}
>  		}
>  	}
> +	if ($max_label < 10) {
> +		@fields = split_hunk_selection(@fields) or return undef;
> +	}
>  	for (@fields) {
>  		if (/^([0-9]*)-([0-9]*)$/) {
>  			if ($1 eq '' and $2 eq '') {
> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index 89c0e73f2b..d3bce154da 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
>  '
>  
>  test_expect_success 'can reset individual lines of patch' '
> -	printf "%s\n" l "^1 3" |
> +	printf "%s\n" l ^13 |
>  	EDITOR=: git reset -p 2>error &&
>  	test_must_be_empty error &&
>  	git diff --cached HEAD >actual &&
> 

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
  2018-03-06 19:57     ` Junio C Hamano
@ 2018-03-06 20:41     ` Igor Djordjevic
  1 sibling, 0 replies; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 20:41 UTC (permalink / raw)
  To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite

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

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

> 
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-add.txt  |  3 ++-
>  git-add--interactive.perl  | 17 ++++++++++++++++-
>  t/t3701-add-interactive.sh |  2 +-
>  3 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index ad33fda9a2..0e2c11e97b 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -341,7 +341,8 @@ If you press "l" then the hunk will be reprinted with each insertion
>  or deletion labelled with a number and you will be prompted to enter
>  which lines you wish to select. Individual line numbers should be
>  separated by a space or comma, to specify a range of lines use a dash
> -between them.
> +between them. To invert the selection prefix it with "\^" so "^3-5,8"
> +will select everything except lines 3, 4, 5 and 8.

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

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

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

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 0/3] add -p: select individual hunk lines
  2018-03-06 20:19   ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic
@ 2018-03-06 21:03     ` Junio C Hamano
  2018-03-06 21:20       ` Igor Djordjevic
  0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-03-06 21:03 UTC (permalink / raw)
  To: Igor Djordjevic; +Cc: Phillip Wood, Git Mailing List, Gustavo Leite

Igor Djordjevic <igor.d.djordjevic@gmail.com> writes:

> A small nitpick - I see you use phrasing like "select lines", where 
> the other commands usually talk about "staging", instead, so "stage 
> lines" might be more aligned with the existing text.

Isn't this machinery shared across "add -p" and "reset -p"?  What is
done to the selected lines when you are using this UI while running
"reset -p"?  I hope it is not "staging".  If the interface only
"selects lines" and what is done to the selected lines depends on
what operation is using this backend, then the current phrasing is
perfectly fine and saying "staging" makes it actively worse.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 0/3] add -p: select individual hunk lines
  2018-03-06 21:03     ` Junio C Hamano
@ 2018-03-06 21:20       ` Igor Djordjevic
  0 siblings, 0 replies; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 21:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Phillip Wood, Git Mailing List, Gustavo Leite

Hi Junio,

On 06/03/2018 22:03, Junio C Hamano wrote:
> 
> > A small nitpick - I see you use phrasing like "select lines", where 
> > the other commands usually talk about "staging", instead, so "stage 
> > lines" might be more aligned with the existing text.
> 
> Isn't this machinery shared across "add -p" and "reset -p"?  What is
> done to the selected lines when you are using this UI while running
> "reset -p"?  I hope it is not "staging".  If the interface only
> "selects lines" and what is done to the selected lines depends on
> what operation is using this backend, then the current phrasing is
> perfectly fine and saying "staging" makes it actively worse.

Hmm, if that is the case, I agree, but I was merely trying to review 
the files being changed - for example, inside "Documentation/git-add.txt":

       y - stage this hunk
       n - do not stage this hunk
       q - quit; do not stage this hunk or any of the remaining ones
       a - stage this hunk and all later hunks in the file
       d - do not stage this hunk or any of the later hunks in the file
       g - select a hunk to go to
       / - search for a hunk matching the given regex
       j - leave this hunk undecided, see next undecided hunk
       J - leave this hunk undecided, see next hunk
       k - leave this hunk undecided, see previous undecided hunk
       K - leave this hunk undecided, see previous hunk
       s - split the current hunk into smaller hunks
       e - manually edit the current hunk
       ? - print help


In there, adding "l" should follow "stage" phrasing, I would think.

But you are right for "git-add--interactive.perl", for example - in 
there, I didn`t notice the line (seems to be?) added inside the shared 
"help_patch_cmd".

But if so, I guess it should then be moved to more context-related 
"help_patch_modes", being phrased accordingly in there.

Thanks for pointing this out, let me recheck my comments.

Regards, Buga

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 1/3] add -p: select individual hunk lines
  2018-03-06 20:29     ` Igor Djordjevic
@ 2018-03-06 21:33       ` Igor Djordjevic
  0 siblings, 0 replies; 53+ messages in thread
From: Igor Djordjevic @ 2018-03-06 21:33 UTC (permalink / raw)
  To: Phillip Wood, Git Mailing List; +Cc: Junio C Hamano, Gustavo Leite

On 06/03/2018 21:29, Igor Djordjevic wrote:
> 
> > diff --git a/git-add--interactive.perl b/git-add--interactive.perl
> > index f83e7450ad..a273b41e95 100755
> > --- a/git-add--interactive.perl
> > +++ b/git-add--interactive.perl
> > 
> > [...]
> > 
> > @@ -1255,6 +1382,7 @@ j - leave this hunk undecided, see next undecided hunk
> >  J - leave this hunk undecided, see next hunk
> >  k - leave this hunk undecided, see previous undecided hunk
> >  K - leave this hunk undecided, see previous hunk
> > +l - select hunk lines to use
> 
> s/select hunk lines to use/stage hunk lines/

I was wrong here - in the context of Junio`s remark, I now think this 
might even belong to context-aware "help_patch_modes" instead, 
phrased accordingly in there (stage/stash/unstage... etc.).

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-06 19:57     ` Junio C Hamano
@ 2018-03-08 11:05       ` Phillip Wood
  2018-03-08 17:53         ` Junio C Hamano
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-03-08 11:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

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

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-08 11:05       ` Phillip Wood
@ 2018-03-08 17:53         ` Junio C Hamano
  2018-03-13 12:06           ` Phillip Wood
  0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-03-08 17:53 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

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

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


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-08 17:53         ` Junio C Hamano
@ 2018-03-13 12:06           ` Phillip Wood
  2018-03-13 16:32             ` Junio C Hamano
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-03-13 12:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

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

Thanks

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-13 12:06           ` Phillip Wood
@ 2018-03-13 16:32             ` Junio C Hamano
  2018-03-14 11:02               ` Phillip Wood
  0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-03-13 16:32 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

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

My observation of the situation behind my reasoning is:

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

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

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

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

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



^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v2 2/3] add -p: allow line selection to be inverted
  2018-03-13 16:32             ` Junio C Hamano
@ 2018-03-14 11:02               ` Phillip Wood
  0 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-14 11:02 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git Mailing List, Gustavo Leite, Phillip Wood

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

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v3 0/3] add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
                   ` (4 preceding siblings ...)
  2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
@ 2018-03-16 10:13 ` Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 1/3] " Phillip Wood
                     ` (3 more replies)
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
  7 siblings, 4 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Since v2 I've updated the patches to use '-' instead of '^' to invert
the selection to match the rest of add -i and clean -i.

These patches build on top of the recount fixes in [1]. The commit
message for the first patch describes the motivation:

"When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'."

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

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


Phillip Wood (3):
  add -p: select individual hunk lines
  add -p: allow line selection to be inverted
  add -p: optimize line selection for short hunks

 Documentation/git-add.txt  |  10 +++
 git-add--interactive.perl  | 181 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  65 ++++++++++++++++
 3 files changed, 256 insertions(+)

-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 1/3] add -p: select individual hunk lines
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
@ 2018-03-16 10:13   ` Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 2/3] add -p: allow line selection to be inverted Phillip Wood
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where 'b' may be omitted to mean
all lines from 'a' to the end of the hunk.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    changes since v2:
     - remove support for omitting the lower bound of a range of lines as
       this clashes with using '-' for inverting the selection.
     - document that the upper bound can be omitted.

 Documentation/git-add.txt  |   8 +++
 git-add--interactive.perl  | 140 +++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  65 +++++++++++++++++++++
 3 files changed, 213 insertions(+)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d50fa339dc..965e192a09 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -332,10 +332,18 @@ patch::
        J - leave this hunk undecided, see next hunk
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
+       l - select hunk lines to use
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        ? - print help
 +
+If you press "l" then the hunk will be reprinted with each insertion or
+deletion labelled with a number and you will be prompted to enter which
+lines you wish to select. Individual line numbers should be separated by
+a space or comma, to specify a range of lines use a dash between
+them. If the upper bound of a range of lines is omitted it defaults to
+the last line.
++
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index f83e7450ad..712226b34c 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1013,6 +1013,130 @@ sub color_diff {
 	} @_;
 }
 
+sub label_hunk_lines {
+	local $_;
+	my $hunk = shift;
+	my $i = 0;
+	my $labels = [ map { /^[-+]/ ? ++$i : 0 } @{$hunk->{TEXT}} ];
+	if ($i > 1) {
+		@{$hunk}{qw(LABELS MAX_LABEL)} = ($labels, $i);
+		return 1;
+	}
+	return 0;
+}
+
+sub select_hunk_lines {
+	my ($hunk, $selected) = @_;
+	my ($text, $labels) = @{$hunk}{qw(TEXT LABELS)};
+	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
+	my ($push_eol, @newtext);
+	# Lines with this mode will become context lines if they are
+	# not selected
+	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
+	for $i (1..$#{$text}) {
+		my $mode = substr($text->[$i], 0, 1);
+		if ($mode eq '\\') {
+			push @newtext, $text->[$i] if ($push_eol);
+			undef $push_eol;
+		} elsif ($labels->[$i] and $selected->[$labels->[$i]]) {
+			push @newtext, $text->[$i];
+			if ($mode eq '+') {
+				$n_cnt++;
+			} else {
+				$o_cnt++;
+			}
+			$push_eol = 1;
+		} elsif ($mode eq ' ' or $mode eq $context_mode) {
+			push @newtext, ' ' . substr($text->[$i], 1);
+			$o_cnt++; $n_cnt++;
+			$push_eol = 1;
+		} else {
+			undef $push_eol;
+		}
+	}
+	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
+					parse_hunk_header($text->[0]);
+	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my $newhunk = {
+		TEXT => \@newtext,
+		DISPLAY => [ color_diff(@newtext) ],
+		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
+		TYPE => $hunk->{TYPE},
+		USE => 1,
+	};
+	# If this hunk has previously been edited add the offset delta
+	# of the old hunk to get the real delta from the original
+	# hunk.
+	if ($hunk->{OFS_DELTA}) {
+		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	}
+	return $newhunk;
+}
+
+sub check_hunk_label {
+	my ($max_label, $label) = ($_[0]->{MAX_LABEL}, $_[1]);
+	if ($label < 1 or $label > $max_label) {
+		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
+		return 0;
+	}
+	return 1;
+}
+
+sub parse_hunk_selection {
+	local $_;
+	my ($hunk, $line) = @_;
+	my $max_label = $hunk->{MAX_LABEL};
+	my @selected = (0) x ($max_label + 1);
+	my @fields = split(/[,\s]+/, $line);
+	for (@fields) {
+		if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) {
+			if ($hi eq '') {
+				$hi = $max_label;
+			}
+			check_hunk_label($hunk, $lo) or return undef;
+			check_hunk_label($hunk, $hi) or return undef;
+			if ($hi < $lo) {
+				($lo, $hi) = ($hi, $lo);
+			}
+			@selected[$lo..$hi] = (1) x (1 + $hi - $lo);
+		} elsif (/^([0-9]+)$/) {
+			check_hunk_label($hunk, $1) or return undef;
+			$selected[$1] = 1;
+		} else {
+			error_msg sprintf(__("invalid hunk line '%s'\n"), $_);
+			return undef;
+		}
+	}
+	return \@selected;
+}
+
+sub display_hunk_lines {
+	my ($display, $labels, $max_label) =
+				@{$_[0]}{qw(DISPLAY LABELS MAX_LABEL)};
+	my $width = int(log($max_label) / log(10)) + 1;
+	my $padding = ' ' x ($width + 1);
+	for my $i (0..$#{$display}) {
+		if ($labels->[$i]) {
+			printf '%*d %s', $width, $labels->[$i], $display->[$i];
+		} else {
+			print $padding . $display->[$i];
+		}
+	}
+}
+
+sub select_lines_loop {
+	my $hunk = shift;
+	display_hunk_lines($hunk);
+	my $selection = undef;
+	until (defined $selection) {
+		print colored $prompt_color, __("select lines? ");
+		my $text = <STDIN>;
+		defined $text and $text =~ /\S/ or return undef;
+		$selection = parse_hunk_selection($hunk, $text);
+	}
+	return select_hunk_lines($hunk, $selection);
+}
+
 my %edit_hunk_manually_modes = (
 	stage => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
@@ -1255,6 +1379,7 @@ j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
+l - select hunk lines to use
 s - split the current hunk into smaller hunks
 e - manually edit the current hunk
 ? - print help
@@ -1471,6 +1596,9 @@ sub patch_update_file {
 		if ($hunk[$ix]{TYPE} eq 'hunk') {
 			$other .= ',e';
 		}
+		if (label_hunk_lines($hunk[$ix])) {
+			$other .= ',l';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -1610,6 +1738,18 @@ sub patch_update_file {
 					next;
 				}
 			}
+			elsif ($line =~ /^l/) {
+				unless ($other =~ /l/) {
+					error_msg __("Cannot select line by line\n");
+					next;
+				}
+				my $newhunk = select_lines_loop($hunk[$ix]);
+				if ($newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				} else {
+					next;
+				}
+			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index a9a9478a29..5f3fcb1758 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -360,6 +360,63 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 	! grep "^+31" actual
 '
 
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..341cc6b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,9 @@
+	+5
+	 10
+	 20
+	+21
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage individual lines of patch' '
+	git reset &&
+	printf 61 >>test &&
+	printf "%s\n" l "1,2 4-" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	+5
+	 10
+	 20
+	 30
+	 40
+	 50
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can reset individual lines of patch' '
+	printf "%s\n" l 2 |
+	EDITOR=: git reset -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
@@ -576,4 +633,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
 	test_cmp expected-2 actual
 '
 
+test_expect_success 'add -p selecting lines works with pathological context lines' '
+	git reset &&
+	printf "%s\n" l 2 y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a >actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 2/3] add -p: allow line selection to be inverted
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 1/3] " Phillip Wood
@ 2018-03-16 10:13   ` Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 3/3] add -p: optimize line selection for short hunks Phillip Wood
  2018-03-29 18:32   ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    changes since v2:
     - use '-' to invert the selection instead of '^' to be consistent
       with the rest of add -i and clean -i.

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

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 965e192a09..01ff4d7d24 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -342,7 +342,8 @@ deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
 a space or comma, to specify a range of lines use a dash between
 them. If the upper bound of a range of lines is omitted it defaults to
-the last line.
+the last line. To invert the selection prefix it with "-" so "-3-5,8"
+will select everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 712226b34c..54fbe114f3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1085,9 +1085,21 @@ sub check_hunk_label {
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
-	my $max_label = $hunk->{MAX_LABEL};
+	my ($max_label, $invert) = ($hunk->{MAX_LABEL}, undef);
 	my @selected = (0) x ($max_label + 1);
 	my @fields = split(/[,\s]+/, $line);
+	if ($fields[0] =~ /^-(.*)/) {
+		$invert = 1;
+		if ($1 ne '') {
+			$fields[0] = $1;
+		} else {
+			shift @fields;
+			unless (@fields) {
+				error_msg __("no lines to invert\n");
+				return undef;
+			}
+		}
+	}
 	for (@fields) {
 		if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) {
 			if ($hi eq '') {
@@ -1107,6 +1119,9 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
+	if ($invert) {
+		@selected = map { !$_ } @selected;
+	}
 	return \@selected;
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5f3fcb1758..46814babf3 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l 2 |
+	printf "%s\n" l "-1 3" |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v3 3/3] add -p: optimize line selection for short hunks
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 1/3] " Phillip Wood
  2018-03-16 10:13   ` [PATCH v3 2/3] add -p: allow line selection to be inverted Phillip Wood
@ 2018-03-16 10:13   ` Phillip Wood
  2018-03-29 18:32   ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-03-16 10:13 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Junio C Hamano, Gustavo Leite, Igor Djordjevic, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are fewer than ten changes in a hunk then make spaces
optional when selecting individual lines. This means that for short
hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---

Notes:
    changes since v2:
     - removed code that handled the default lower bound now that '-...'
       means invert the selection.

 Documentation/git-add.txt  |  9 +++++----
 git-add--interactive.perl  | 26 ++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 01ff4d7d24..f3c81dfb11 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -340,10 +340,11 @@ patch::
 If you press "l" then the hunk will be reprinted with each insertion or
 deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
-a space or comma, to specify a range of lines use a dash between
-them. If the upper bound of a range of lines is omitted it defaults to
-the last line. To invert the selection prefix it with "-" so "-3-5,8"
-will select everything except lines 3, 4, 5 and 8.
+a space or comma (these can be omitted if there are fewer than ten
+labelled lines), to specify a range of lines use a dash between them. If
+the upper bound of a range of lines is omitted it defaults to the last
+line. To invert the selection prefix it with "-" so "-3-5,8" will select
+everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 54fbe114f3..d65ad7c26d 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1082,6 +1082,29 @@ sub check_hunk_label {
 	return 1;
 }
 
+sub split_hunk_selection {
+	local $_;
+	my @fields = @_;
+	my @ret;
+	for (@fields) {
+		while ($_ ne '') {
+			if (/^[0-9]-$/) {
+				push @ret, $_;
+				last;
+			} elsif (/^([0-9](?:-[0-9])?)(.*)/) {
+				push @ret, $1;
+				$_ = $2;
+			} else {
+				error_msg sprintf
+				    __("invalid hunk line '%s'\n"),
+				    substr($_, 0, 1);
+				return ();
+			}
+		}
+	}
+	return @ret;
+}
+
 sub parse_hunk_selection {
 	local $_;
 	my ($hunk, $line) = @_;
@@ -1100,6 +1123,9 @@ sub parse_hunk_selection {
 			}
 		}
 	}
+	if ($max_label < 10) {
+		@fields = split_hunk_selection(@fields) or return undef;
+	}
 	for (@fields) {
 		if (my ($lo, $hi) = /^([0-9]+)-([0-9]*)$/) {
 			if ($hi eq '') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 46814babf3..467c6eff0e 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -410,7 +410,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l "-1 3" |
+	printf "%s\n" l -13 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.16.2


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/3] add -p: select individual hunk lines
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
                     ` (2 preceding siblings ...)
  2018-03-16 10:13   ` [PATCH v3 3/3] add -p: optimize line selection for short hunks Phillip Wood
@ 2018-03-29 18:32   ` Junio C Hamano
  2018-03-30 11:09     ` Phillip Wood
  3 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-03-29 18:32 UTC (permalink / raw)
  To: Git Mailing List
  Cc: Phillip Wood, Gustavo Leite, Igor Djordjevic, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Since v2 I've updated the patches to use '-' instead of '^' to invert
> the selection to match the rest of add -i and clean -i.
>
> These patches build on top of the recount fixes in [1]. The commit
> message for the first patch describes the motivation:
>
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."

I haven't seen any review comments on this round, and as I am not a
heavy user of "add -i" interface (even though I admit that I
originally wrote it), I haven't had a chance to exercise the code
myself in the two weeks since the patches have been queued in my
tree.

I am inclihned to merge them to 'next' soonish, but please stop me
if anybody (including the original author) has further comments.

Thanks.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/3] add -p: select individual hunk lines
  2018-03-29 18:32   ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano
@ 2018-03-30 11:09     ` Phillip Wood
  2018-03-31 19:20       ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-03-30 11:09 UTC (permalink / raw)
  To: Junio C Hamano, Git Mailing List
  Cc: Gustavo Leite, Igor Djordjevic, Phillip Wood

On 29/03/18 19:32, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Since v2 I've updated the patches to use '-' instead of '^' to invert
>> the selection to match the rest of add -i and clean -i.
>>
>> These patches build on top of the recount fixes in [1]. The commit
>> message for the first patch describes the motivation:
>>
>> "When I end up editing hunks it is almost always because I want to
>> stage a subset of the lines in the hunk. Doing this by editing the
>> hunk is inconvenient and error prone (especially so if the patch is
>> going to be reversed before being applied). Instead offer an option
>> for add -p to stage individual lines. When the user presses 'l' the
>> hunk is redrawn with labels by the insertions and deletions and they
>> are prompted to enter a list of the lines they wish to stage. Ranges
>> of lines may be specified using 'a-b' where either 'a' or 'b' may be
>> omitted to mean all lines from 'a' to the end of the hunk or all lines
>> from 1 upto and including 'b'."
> 
> I haven't seen any review comments on this round, and as I am not a
> heavy user of "add -i" interface (even though I admit that I
> originally wrote it), I haven't had a chance to exercise the code
> myself in the two weeks since the patches have been queued in my
> tree.
> 
> I am inclihned to merge them to 'next' soonish, but please stop me
> if anybody (including the original author) has further comments.
> 
> Thanks.
> 
Hi Junio, if no one else has any comments, then I think it's ready for
next. I've not used this latest incarnation much but I've used the
previous versions quite a bit.

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/3] add -p: select individual hunk lines
  2018-03-30 11:09     ` Phillip Wood
@ 2018-03-31 19:20       ` Ævar Arnfjörð Bjarmason
  2018-04-02 10:55         ` Phillip Wood
  0 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-03-31 19:20 UTC (permalink / raw)
  To: phillip.wood
  Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic,
	Fernando Vezzosi, Jeff King


On Fri, Mar 30 2018, Phillip Wood wrote:

> On 29/03/18 19:32, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>
>>> Since v2 I've updated the patches to use '-' instead of '^' to invert
>>> the selection to match the rest of add -i and clean -i.
>>>
>>> These patches build on top of the recount fixes in [1]. The commit
>>> message for the first patch describes the motivation:
>>>
>>> "When I end up editing hunks it is almost always because I want to
>>> stage a subset of the lines in the hunk. Doing this by editing the
>>> hunk is inconvenient and error prone (especially so if the patch is
>>> going to be reversed before being applied). Instead offer an option
>>> for add -p to stage individual lines. When the user presses 'l' the
>>> hunk is redrawn with labels by the insertions and deletions and they
>>> are prompted to enter a list of the lines they wish to stage. Ranges
>>> of lines may be specified using 'a-b' where either 'a' or 'b' may be
>>> omitted to mean all lines from 'a' to the end of the hunk or all lines
>>> from 1 upto and including 'b'."
>>
>> I haven't seen any review comments on this round, and as I am not a
>> heavy user of "add -i" interface (even though I admit that I
>> originally wrote it), I haven't had a chance to exercise the code
>> myself in the two weeks since the patches have been queued in my
>> tree.
>>
>> I am inclihned to merge them to 'next' soonish, but please stop me
>> if anybody (including the original author) has further comments.
>>
>> Thanks.
>>
> Hi Junio, if no one else has any comments, then I think it's ready for
> next. I've not used this latest incarnation much but I've used the
> previous versions quite a bit.

First of all thinks for working on this. Something like this is a
feature I've long wanted to have and have just been manually using edit.

As for the code, one comment: For reasons of avoiding something like the
2.17.0-rc* bug I just sent a patch for, I think you should change your
use of the implicit $_ to something where you explicitly create lexical
variables instead.

It's bad style in Perl to use $_ for anything except a one-liner, and
similar to the $1 bug with your other patch, you'll get buggy code
(regardless of your use of local $_) if one of the functions you're
calling in these >10 line for-loops starts doing something to set $_
itself, as demonstrated by:

    $ perl -wE 'sub foo { local $_; for (1..3) { bar(); say } } sub bar { $_ = $_ ** 2; } foo()'
    1
    4
    9

Let's just name these variables, even if it wasn't for that caveat it
would still be a good idea, since for any non-trivial use of $_ you've
got to mentally keep track of what set $_ where, so it's hard to read.

As for the implementation, I *want* to love this, but it seems the way
it works is just fatally flawed, consider. *The* use-case I've had for
something like this (maybe yours differs?) is something where I do e.g.:

    $ perl -pi -e 's/git/Git/g' README.md

Which gives me (among other things):

    -See [Documentation/gittutorial.txt][] to get started, then see
    -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
    -Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
    -read with `man gittutorial` or `git help tutorial`, and the
    -documentation of each command with `man git-<commandname>` or `git help
    +See [Documentation/Gittutorial.txt][] to get started, then see
    +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
    +Documentation/Git-<commandname>.txt for documentation of each command.
    +If Git has been correctly installed, then the tutorial can also be
    +read with `man Gittutorial` or `Git help tutorial`, and the
    +documentation of each command with `man Git-<commandname>` or `Git help

Which to me, is a perfect use-case for this feature. Here I
hypothetically want to change "git" to "Git" in prose, so I only want to
change that "If git has been" line, the rest are all references to
filenames or command names.

So I would manually edit the hunk via "e" to:

     See [Documentation/gittutorial.txt][] to get started, then see
     [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
    +If Git has been correctly installed, then the tutorial can also be
     read with `man gittutorial` or `git help tutorial`, and the
     documentation of each command with `man git-<commandname>` or `git help
     <commandname>`.

Yay, but very tedious. Now let's use your feature to do this:

     1 -See [Documentation/gittutorial.txt][] to get started, then see
     2 -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     3 -Documentation/git-<commandname>.txt for documentation of each command.
     4 -If git has been correctly installed, then the tutorial can also be
     5 -read with `man gittutorial` or `git help tutorial`, and the
     6 -documentation of each command with `man git-<commandname>` or `git help
     7 +See [Documentation/Gittutorial.txt][] to get started, then see
     8 +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
     9 +Documentation/Git-<commandname>.txt for documentation of each command.
    10 +If Git has been correctly installed, then the tutorial can also be
    11 +read with `man Gittutorial` or `Git help tutorial`, and the
    12 +documentation of each command with `man Git-<commandname>` or `Git help
        <commandname>`.

    select lines? 4,10

So what I was expecting this to do was some automagic where it would
pair up the 4 line, and based on the removed/added count figure out
which line I'm also adding corresponds to that. I.e. both selected lines
are the 4th line removed/added, so it should transpose the 10th to the
4th, but instead I get a patch that looks like this:

    diff --git a/README.md b/README.md
    index f17af66a97..7234756e64 100644
    --- a/README.md
    +++ b/README.md
    @@ -18,9 +18,9 @@ including full documentation and Git related tools.
     See [Documentation/gittutorial.txt][] to get started, then see
     [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
     Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
     read with `man gittutorial` or `git help tutorial`, and the
     documentation of each command with `man git-<commandname>` or `git help
    +If Git has been correctly installed, then the tutorial can also be
     <commandname>`.

I.e. it just grepped out the removed line from the removed chunk, and
the same for the added bit, which of course means that now the added
line doesn't get injected into the correct place, but added to the end.

I can see *why* that happens, but I can't imagine a case where this
behavior isn't useless.

What this seems useful for now is for chunks that only consist of lines
that are added or removed, maybe there's similar edge cases with those,
but I can't think of any, there I think we should do the obvious and
intuitive thing.

But I think that as this stands we really should at least disable this
where we present the user with a hunk that consists of both removed &
added lines, since I think the desired behavior I've described above
should be the default, and once we pick one we're going to have to
support it forever, so it's important to get it right to begin with.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/3] add -p: select individual hunk lines
  2018-03-31 19:20       ` Ævar Arnfjörð Bjarmason
@ 2018-04-02 10:55         ` Phillip Wood
  2018-04-02 11:39           ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-04-02 10:55 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, phillip.wood
  Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic,
	Fernando Vezzosi, Jeff King

On 31/03/18 20:20, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 30 2018, Phillip Wood wrote:
> 
>> On 29/03/18 19:32, Junio C Hamano wrote:
>>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>>
>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>
>>>> Since v2 I've updated the patches to use '-' instead of '^' to invert
>>>> the selection to match the rest of add -i and clean -i.
>>>>
>>>> These patches build on top of the recount fixes in [1]. The commit
>>>> message for the first patch describes the motivation:
>>>>
>>>> "When I end up editing hunks it is almost always because I want to
>>>> stage a subset of the lines in the hunk. Doing this by editing the
>>>> hunk is inconvenient and error prone (especially so if the patch is
>>>> going to be reversed before being applied). Instead offer an option
>>>> for add -p to stage individual lines. When the user presses 'l' the
>>>> hunk is redrawn with labels by the insertions and deletions and they
>>>> are prompted to enter a list of the lines they wish to stage. Ranges
>>>> of lines may be specified using 'a-b' where either 'a' or 'b' may be
>>>> omitted to mean all lines from 'a' to the end of the hunk or all lines
>>>> from 1 upto and including 'b'."
>>>
>>> I haven't seen any review comments on this round, and as I am not a
>>> heavy user of "add -i" interface (even though I admit that I
>>> originally wrote it), I haven't had a chance to exercise the code
>>> myself in the two weeks since the patches have been queued in my
>>> tree.
>>>
>>> I am inclihned to merge them to 'next' soonish, but please stop me
>>> if anybody (including the original author) has further comments.
>>>
>>> Thanks.
>>>
>> Hi Junio, if no one else has any comments, then I think it's ready for
>> next. I've not used this latest incarnation much but I've used the
>> previous versions quite a bit.

Ah it seems I spoke too soon.

Thanks for taking a look at this Ævar

> First of all thinks for working on this. Something like this is a
> feature I've long wanted to have and have just been manually using edit.
> 
> As for the code, one comment: For reasons of avoiding something like the
> 2.17.0-rc* bug I just sent a patch for, I think you should change your
> use of the implicit $_ to something where you explicitly create lexical
> variables instead.
> 
> It's bad style in Perl to use $_ for anything except a one-liner, and
> similar to the $1 bug with your other patch, you'll get buggy code
> (regardless of your use of local $_) if one of the functions you're
> calling in these >10 line for-loops starts doing something to set $_
> itself, as demonstrated by:
> 
>     $ perl -wE 'sub foo { local $_; for (1..3) { bar(); say } } sub bar { $_ = $_ ** 2; } foo()'
>     1
>     4
>     9
> 
> Let's just name these variables, even if it wasn't for that caveat it
> would still be a good idea, since for any non-trivial use of $_ you've
> got to mentally keep track of what set $_ where, so it's hard to read.

Right, I'll use lexical variables.

> 
> As for the implementation, I *want* to love this, but it seems the way
> it works is just fatally flawed, consider. *The* use-case I've had for
> something like this (maybe yours differs?) is something where I do e.g.:

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

	-original
	+modified
	+new stuff

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

>     $ perl -pi -e 's/git/Git/g' README.md
> 
> Which gives me (among other things):
> 
>     -See [Documentation/gittutorial.txt][] to get started, then see
>     -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>     -Documentation/git-<commandname>.txt for documentation of each command.
>     -If git has been correctly installed, then the tutorial can also be
>     -read with `man gittutorial` or `git help tutorial`, and the
>     -documentation of each command with `man git-<commandname>` or `git help
>     +See [Documentation/Gittutorial.txt][] to get started, then see
>     +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
>     +Documentation/Git-<commandname>.txt for documentation of each command.
>     +If Git has been correctly installed, then the tutorial can also be
>     +read with `man Gittutorial` or `Git help tutorial`, and the
>     +documentation of each command with `man Git-<commandname>` or `Git help
> 
> Which to me, is a perfect use-case for this feature. Here I
> hypothetically want to change "git" to "Git" in prose, so I only want to
> change that "If git has been" line, the rest are all references to
> filenames or command names.
> 
> So I would manually edit the hunk via "e" to:
> 
>      See [Documentation/gittutorial.txt][] to get started, then see
>      [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>      Documentation/git-<commandname>.txt for documentation of each command.
>     -If git has been correctly installed, then the tutorial can also be
>     +If Git has been correctly installed, then the tutorial can also be
>      read with `man gittutorial` or `git help tutorial`, and the
>      documentation of each command with `man git-<commandname>` or `git help
>      <commandname>`.
> 
> Yay, but very tedious. Now let's use your feature to do this:
> 
>      1 -See [Documentation/gittutorial.txt][] to get started, then see
>      2 -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>      3 -Documentation/git-<commandname>.txt for documentation of each command.
>      4 -If git has been correctly installed, then the tutorial can also be
>      5 -read with `man gittutorial` or `git help tutorial`, and the
>      6 -documentation of each command with `man git-<commandname>` or `git help
>      7 +See [Documentation/Gittutorial.txt][] to get started, then see
>      8 +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
>      9 +Documentation/Git-<commandname>.txt for documentation of each command.
>     10 +If Git has been correctly installed, then the tutorial can also be
>     11 +read with `man Gittutorial` or `Git help tutorial`, and the
>     12 +documentation of each command with `man Git-<commandname>` or `Git help
>         <commandname>`.
> 
>     select lines? 4,10
> 
> So what I was expecting this to do was some automagic where it would
> pair up the 4 line, and based on the removed/added count figure out
> which line I'm also adding corresponds to that. I.e. both selected lines
> are the 4th line removed/added, so it should transpose the 10th to the
> 4th, but instead I get a patch that looks like this:
> 
>     diff --git a/README.md b/README.md
>     index f17af66a97..7234756e64 100644
>     --- a/README.md
>     +++ b/README.md
>     @@ -18,9 +18,9 @@ including full documentation and Git related tools.
>      See [Documentation/gittutorial.txt][] to get started, then see
>      [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>      Documentation/git-<commandname>.txt for documentation of each command.
>     -If git has been correctly installed, then the tutorial can also be
>      read with `man gittutorial` or `git help tutorial`, and the
>      documentation of each command with `man git-<commandname>` or `git help
>     +If Git has been correctly installed, then the tutorial can also be
>      <commandname>`.
> 
> I.e. it just grepped out the removed line from the removed chunk, and
> the same for the added bit, which of course means that now the added
> line doesn't get injected into the correct place, but added to the end.
> 
> I can see *why* that happens, but I can't imagine a case where this
> behavior isn't useless.

I agree it's useless here I've got some suggestions on how to fix it
though I've not spent much time thinking them through - any comments
would be most welcome.

I think we want something that pairs up groups of selected deletions and
insertions rather than lines, so that it can handle cases where the
number of inserted lines differs from the number of deletions but there
are the same number of groups. For example given

	1 -* first
	2 -* second
	3 -* third
	4 +* a longer description of the
	5 +  first item
	6 +* the second item
	7 +* the third item

1,3,4-5,7 would give

	-* first
	+* a longer description of the
	+  first item
	 * second
	-* third
	+* the third item

If the number of deletion groups differs from the number of insertion
groups then there is no way to automagically pair them up so we'd need
some syntax to do that. We could make the order of the numbers given by
the user significant so that 1,6,7,2,4,9 would pair line 1 with 6 & 7
and line 4 with 9 and delete line 2. It would then be an error if the an
earlier deletion was pair with a later insertion, so in the example
above 1,6,7,4,9,2 or 2,4,9,1,6,7 would be valid but 1,9,2,4,8 wouldn't).
We could also use brackets to group things e.g. [1,6,7],2,[4,9] which
makes the groups clearer visually.

Thinking further there is a problem with

	1 -original
	2 +a new paragraph before
	3 +original
	4 +
	5 +modified original

if the user wants to commit the new paragraph before the modified
original then they need a way to specify that the insertion should come
before the deletion. At the moment giving 2-4 will put the new paragraph
after the unmodified original. To solve that there needs to be a way to
indicate that a group of insertions needs to come before the context
lines created by not staging the deletions - perhaps ^[2-4] or [^2-4]?
(we could distinguish between commas and spaces so that ^2,3,5 is
different to ^2 3,5 but that is then different to how lists work in the
rest of git)


> What this seems useful for now is for chunks that only consist of lines
> that are added or removed, maybe there's similar edge cases with those,
> but I can't think of any, there I think we should do the obvious and
> intuitive thing.
> 
> But I think that as this stands we really should at least disable this
> where we present the user with a hunk that consists of both removed &
> added lines, since I think the desired behavior I've described above
> should be the default, and once we pick one we're going to have to
> support it forever, so it's important to get it right to begin with.
> 

Thanks for your feedback it's been really useful, let me know what you
think about grouping things.

Best Wishes

Phillip


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v3 0/3] add -p: select individual hunk lines
  2018-04-02 10:55         ` Phillip Wood
@ 2018-04-02 11:39           ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-04-02 11:39 UTC (permalink / raw)
  To: phillip.wood
  Cc: Junio C Hamano, Git Mailing List, Gustavo Leite, Igor Djordjevic,
	Fernando Vezzosi, Jeff King


On Mon, Apr 02 2018, Phillip Wood wrote:

> On 31/03/18 20:20, Ævar Arnfjörð Bjarmason wrote:
>>
>> On Fri, Mar 30 2018, Phillip Wood wrote:
>>
>>> On 29/03/18 19:32, Junio C Hamano wrote:
>>>> Phillip Wood <phillip.wood@talktalk.net> writes:
>>>>
>>>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>>>>
>>>>> Since v2 I've updated the patches to use '-' instead of '^' to invert
>>>>> the selection to match the rest of add -i and clean -i.
>>>>>
>>>>> These patches build on top of the recount fixes in [1]. The commit
>>>>> message for the first patch describes the motivation:
>>>>>
>>>>> "When I end up editing hunks it is almost always because I want to
>>>>> stage a subset of the lines in the hunk. Doing this by editing the
>>>>> hunk is inconvenient and error prone (especially so if the patch is
>>>>> going to be reversed before being applied). Instead offer an option
>>>>> for add -p to stage individual lines. When the user presses 'l' the
>>>>> hunk is redrawn with labels by the insertions and deletions and they
>>>>> are prompted to enter a list of the lines they wish to stage. Ranges
>>>>> of lines may be specified using 'a-b' where either 'a' or 'b' may be
>>>>> omitted to mean all lines from 'a' to the end of the hunk or all lines
>>>>> from 1 upto and including 'b'."
>>>>
>>>> I haven't seen any review comments on this round, and as I am not a
>>>> heavy user of "add -i" interface (even though I admit that I
>>>> originally wrote it), I haven't had a chance to exercise the code
>>>> myself in the two weeks since the patches have been queued in my
>>>> tree.
>>>>
>>>> I am inclihned to merge them to 'next' soonish, but please stop me
>>>> if anybody (including the original author) has further comments.
>>>>
>>>> Thanks.
>>>>
>>> Hi Junio, if no one else has any comments, then I think it's ready for
>>> next. I've not used this latest incarnation much but I've used the
>>> previous versions quite a bit.
>
> Ah it seems I spoke too soon.
>
> Thanks for taking a look at this Ævar
>
>> First of all thinks for working on this. Something like this is a
>> feature I've long wanted to have and have just been manually using edit.
>>
>> As for the code, one comment: For reasons of avoiding something like the
>> 2.17.0-rc* bug I just sent a patch for, I think you should change your
>> use of the implicit $_ to something where you explicitly create lexical
>> variables instead.
>>
>> It's bad style in Perl to use $_ for anything except a one-liner, and
>> similar to the $1 bug with your other patch, you'll get buggy code
>> (regardless of your use of local $_) if one of the functions you're
>> calling in these >10 line for-loops starts doing something to set $_
>> itself, as demonstrated by:
>>
>>     $ perl -wE 'sub foo { local $_; for (1..3) { bar(); say } } sub bar { $_ = $_ ** 2; } foo()'
>>     1
>>     4
>>     9
>>
>> Let's just name these variables, even if it wasn't for that caveat it
>> would still be a good idea, since for any non-trivial use of $_ you've
>> got to mentally keep track of what set $_ where, so it's hard to read.
>
> Right, I'll use lexical variables.
>
>>
>> As for the implementation, I *want* to love this, but it seems the way
>> it works is just fatally flawed, consider. *The* use-case I've had for
>> something like this (maybe yours differs?) is something where I do e.g.:
>
> I've used it for selecting a subset of additions or deletions when my
> work has run ahead of a logical commit boundary. I've also used it in
> cases such as
>
> 	-original
> 	+modified
> 	+new stuff
>
> To separate the modification from the addition of new stuff, but I've
> not used it on a list of modifications as in your example.

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

>>     $ perl -pi -e 's/git/Git/g' README.md
>>
>> Which gives me (among other things):
>>
>>     -See [Documentation/gittutorial.txt][] to get started, then see
>>     -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>>     -Documentation/git-<commandname>.txt for documentation of each command.
>>     -If git has been correctly installed, then the tutorial can also be
>>     -read with `man gittutorial` or `git help tutorial`, and the
>>     -documentation of each command with `man git-<commandname>` or `git help
>>     +See [Documentation/Gittutorial.txt][] to get started, then see
>>     +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
>>     +Documentation/Git-<commandname>.txt for documentation of each command.
>>     +If Git has been correctly installed, then the tutorial can also be
>>     +read with `man Gittutorial` or `Git help tutorial`, and the
>>     +documentation of each command with `man Git-<commandname>` or `Git help
>>
>> Which to me, is a perfect use-case for this feature. Here I
>> hypothetically want to change "git" to "Git" in prose, so I only want to
>> change that "If git has been" line, the rest are all references to
>> filenames or command names.
>>
>> So I would manually edit the hunk via "e" to:
>>
>>      See [Documentation/gittutorial.txt][] to get started, then see
>>      [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>>      Documentation/git-<commandname>.txt for documentation of each command.
>>     -If git has been correctly installed, then the tutorial can also be
>>     +If Git has been correctly installed, then the tutorial can also be
>>      read with `man gittutorial` or `git help tutorial`, and the
>>      documentation of each command with `man git-<commandname>` or `git help
>>      <commandname>`.
>>
>> Yay, but very tedious. Now let's use your feature to do this:
>>
>>      1 -See [Documentation/gittutorial.txt][] to get started, then see
>>      2 -[Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>>      3 -Documentation/git-<commandname>.txt for documentation of each command.
>>      4 -If git has been correctly installed, then the tutorial can also be
>>      5 -read with `man gittutorial` or `git help tutorial`, and the
>>      6 -documentation of each command with `man git-<commandname>` or `git help
>>      7 +See [Documentation/Gittutorial.txt][] to get started, then see
>>      8 +[Documentation/Giteveryday.txt][] for a useful minimum set of commands, and
>>      9 +Documentation/Git-<commandname>.txt for documentation of each command.
>>     10 +If Git has been correctly installed, then the tutorial can also be
>>     11 +read with `man Gittutorial` or `Git help tutorial`, and the
>>     12 +documentation of each command with `man Git-<commandname>` or `Git help
>>         <commandname>`.
>>
>>     select lines? 4,10
>>
>> So what I was expecting this to do was some automagic where it would
>> pair up the 4 line, and based on the removed/added count figure out
>> which line I'm also adding corresponds to that. I.e. both selected lines
>> are the 4th line removed/added, so it should transpose the 10th to the
>> 4th, but instead I get a patch that looks like this:
>>
>>     diff --git a/README.md b/README.md
>>     index f17af66a97..7234756e64 100644
>>     --- a/README.md
>>     +++ b/README.md
>>     @@ -18,9 +18,9 @@ including full documentation and Git related tools.
>>      See [Documentation/gittutorial.txt][] to get started, then see
>>      [Documentation/giteveryday.txt][] for a useful minimum set of commands, and
>>      Documentation/git-<commandname>.txt for documentation of each command.
>>     -If git has been correctly installed, then the tutorial can also be
>>      read with `man gittutorial` or `git help tutorial`, and the
>>      documentation of each command with `man git-<commandname>` or `git help
>>     +If Git has been correctly installed, then the tutorial can also be
>>      <commandname>`.
>>
>> I.e. it just grepped out the removed line from the removed chunk, and
>> the same for the added bit, which of course means that now the added
>> line doesn't get injected into the correct place, but added to the end.
>>
>> I can see *why* that happens, but I can't imagine a case where this
>> behavior isn't useless.
>
> I agree it's useless here I've got some suggestions on how to fix it
> though I've not spent much time thinking them through - any comments
> would be most welcome.
>
> I think we want something that pairs up groups of selected deletions and
> insertions rather than lines, so that it can handle cases where the
> number of inserted lines differs from the number of deletions but there
> are the same number of groups. For example given
>
> 	1 -* first
> 	2 -* second
> 	3 -* third
> 	4 +* a longer description of the
> 	5 +  first item
> 	6 +* the second item
> 	7 +* the third item
>
> 1,3,4-5,7 would give
>
> 	-* first
> 	+* a longer description of the
> 	+  first item
> 	 * second
> 	-* third
> 	+* the third item
>
> If the number of deletion groups differs from the number of insertion
> groups then there is no way to automagically pair them up so we'd need
> some syntax to do that. We could make the order of the numbers given by
> the user significant so that 1,6,7,2,4,9 would pair line 1 with 6 & 7
> and line 4 with 9 and delete line 2. It would then be an error if the an
> earlier deletion was pair with a later insertion, so in the example
> above 1,6,7,4,9,2 or 2,4,9,1,6,7 would be valid but 1,9,2,4,8 wouldn't).
> We could also use brackets to group things e.g. [1,6,7],2,[4,9] which
> makes the groups clearer visually.
>
> Thinking further there is a problem with
>
> 	1 -original
> 	2 +a new paragraph before
> 	3 +original
> 	4 +
> 	5 +modified original
>
> if the user wants to commit the new paragraph before the modified
> original then they need a way to specify that the insertion should come
> before the deletion. At the moment giving 2-4 will put the new paragraph
> after the unmodified original. To solve that there needs to be a way to
> indicate that a group of insertions needs to come before the context
> lines created by not staging the deletions - perhaps ^[2-4] or [^2-4]?
> (we could distinguish between commas and spaces so that ^2,3,5 is
> different to ^2 3,5 but that is then different to how lists work in the
> rest of git)
>
>
>> What this seems useful for now is for chunks that only consist of lines
>> that are added or removed, maybe there's similar edge cases with those,
>> but I can't think of any, there I think we should do the obvious and
>> intuitive thing.
>>
>> But I think that as this stands we really should at least disable this
>> where we present the user with a hunk that consists of both removed &
>> added lines, since I think the desired behavior I've described above
>> should be the default, and once we pick one we're going to have to
>> support it forever, so it's important to get it right to begin with.
>>
>
> Thanks for your feedback it's been really useful, let me know what you
> think about grouping things.

All of that makes sense to me. Yes as you noted earlier there's only so
far we can get in this case by automatically trying to pair up
removed/added lines, and actually your example wasn't even very
pathological, consider:

    1 -* a longer description of the
    2 -  first item
    3 -* second
    4 -* third
    5 +* first
    6 +* a longer description of the
    7 +  second item
    8 +* the third item

There the naïve heuristic I initially noted for selecting 4,8 would
work, but not we couldn't in the general case rely on the removed/added
lines being equal in number as some fallback for the simpler transpose
behavior.

It sounds like you're interested in hacking more on this, great. I'll be
happy to review & add tests once you have something.

I'll just say that I wonder if we should simply leave some of these more
complex cases on the table, and only handle some subsets of simple cases
in liue of coming up with a syntax that handles everything. there's
always "edit" as the fallback for the complex cases.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* [RFC PATCH v4 0/4]  add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
                   ` (5 preceding siblings ...)
  2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
@ 2018-07-26 10:22 ` Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 1/4] " Phillip Wood
                     ` (3 more replies)
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
  7 siblings, 4 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

I've updated this series based on Ævar's feedback on v3 (to paraphrase
stop using '$_' so much and fix staging modified lines.). The first
patch is functionally equivalent to the previous version but with a
reworked implementation. Patch 2 is new, it implements correctly
staging modified lines with a couple of limitations - see the commit
message for more details, I'm keen to get some feedback on it. Patches
3 and 4 are essentially rebased and tweaked versions of patches 2 and
3 from the previous version.

This series is based on pw/add-p-recount (f4d35a6b49 "add -p: fix
counting empty context lines in edited patches")

The motivation for this series is summed up in the first commit
message:

"When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'."

Phillip Wood (4):
  add -p: select individual hunk lines
  add -p: select modified lines correctly
  add -p: allow line selection to be inverted
  add -p: optimize line selection for short hunks

 Documentation/git-add.txt  |  10 ++
 git-add--interactive.perl  | 350 +++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh | 209 ++++++++++++++++++++++
 3 files changed, 569 insertions(+)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v4 1/4] add -p: select individual hunk lines
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
@ 2018-07-26 10:22   ` Phillip Wood
  2018-07-26 10:22   ` [RFC PATCH v4 2/4] add -p: select modified lines correctly Phillip Wood
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where 'b' may be omitted to mean
all lines from 'a' to the end of the hunk. Modified lines are not
handled correctly, that will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |   8 ++
 git-add--interactive.perl  | 181 +++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh | 103 +++++++++++++++++++++
 3 files changed, 292 insertions(+)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d50fa339dc..965e192a09 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -332,10 +332,18 @@ patch::
        J - leave this hunk undecided, see next hunk
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
+       l - select hunk lines to use
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        ? - print help
 +
+If you press "l" then the hunk will be reprinted with each insertion or
+deletion labelled with a number and you will be prompted to enter which
+lines you wish to select. Individual line numbers should be separated by
+a space or comma, to specify a range of lines use a dash between
+them. If the upper bound of a range of lines is omitted it defaults to
+the last line.
++
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8361ef45e7..cbc9e5698a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1013,6 +1013,171 @@ sub color_diff {
 	} @_;
 }
 
+use constant {
+	NO_NEWLINE => 1,
+};
+
+sub label_hunk_lines {
+	my $hunk = shift;
+	my $text = $hunk->{TEXT};
+	my (@line_flags, @lines);
+	my ($block, $label, $last_mode) = (0, 0, '');
+	for my $line (1..$#{$text}) {
+		$line_flags[$line] = 0;
+		my $mode = substr($text->[$line], 0, 1);
+		if ($mode eq '\\') {
+			$line_flags[$line - 1] |= NO_NEWLINE;
+		}
+		if ($mode eq '-' or $mode eq '+') {
+			$lines[++$label] = $line;
+		}
+	}
+	if ($label > 1) {
+		$hunk->{LABELS} = {
+			LINES => \@lines,
+		};
+		$hunk->{LINE_FLAGS} = \@line_flags;
+		return 1;
+	}
+	return 0;
+}
+
+sub select_hunk_lines {
+	my ($hunk, $selected) = @_;
+	my ($line_flags, $text) = @{$hunk}{qw(LINE_FLAGS TEXT)};
+	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
+	my @newtext;
+
+	my $select_lines = sub {
+		for my $i (@_) {
+			my $line = $text->[$i];
+			my $mode = substr($line, 0, 1);
+			push @newtext, $line;
+			if ($mode eq '+') {
+				$n_cnt++;
+			} elsif ($mode eq '-') {
+				$o_cnt++;
+			}
+		}
+	};
+
+	my ($lo, $hi) = splice(@$selected, 0, 2);
+	# Lines with this mode will become context lines if they are
+	# not selected
+	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
+	for $i (1..$#{$text}) {
+		if ($lo <= $i and $i <= $hi) {
+			$select_lines->($i);
+		} else {
+			my $line = $text->[$i];
+			my $mode = substr($line, 0, 1);
+			if ($mode eq ' ' or $mode eq $context_mode) {
+				push @newtext, ' ' . substr($line, 1);
+				$o_cnt++; $n_cnt++;
+				if ($line_flags->[$i] & NO_NEWLINE) {
+					push @newtext, $text->[$i + 1];
+				}
+			}
+		}
+		if ($i == $hi) {
+			if (@$selected) {
+				($lo, $hi) = splice(@$selected, 0, 2);
+			}
+		}
+	}
+	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
+					parse_hunk_header($text->[0]);
+	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my $newhunk = {
+		TEXT => \@newtext,
+		DISPLAY => [ color_diff(@newtext) ],
+		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
+		TYPE => $hunk->{TYPE},
+		USE => 1,
+	};
+	# If this hunk has previously been edited add the offset delta
+	# of the old hunk to get the real delta from the original
+	# hunk.
+	if ($hunk->{OFS_DELTA}) {
+		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	}
+	return $newhunk;
+}
+
+sub check_hunk_label {
+	my ($max_label, $label) = @_;
+	if ($label < 1 or $label > $max_label) {
+		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
+		return 0;
+	}
+	return 1;
+}
+
+sub parse_hunk_selection {
+	my ($hunk, $line) = @_;
+	my $lines = $hunk->{LABELS}->{LINES};
+	my $max_label = $#{$lines};
+	my %selected;
+	my @fields = split(/[,\s]+/, $line);
+	for my $f (@fields) {
+		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
+			if ($hi eq '') {
+				$hi = $max_label;
+			}
+			check_hunk_label($max_label, $lo) or return undef;
+			check_hunk_label($max_label, $hi) or return undef;
+			if ($hi < $lo) {
+				($lo, $hi) = ($hi, $lo);
+			}
+			undef @selected{$lo..$hi};
+		} elsif (my ($label) = ($f =~ /^([0-9]+)$/)) {
+			check_hunk_label($max_label, $label) or return undef;
+			undef $selected{$label};
+		} else {
+			error_msg sprintf(__("invalid hunk line '%s'\n"), $f);
+			return undef;
+		}
+	}
+	[ map {
+		my $line = $lines->[$_];
+		if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) {
+			($line, $line + 1);
+		} else {
+			($line, $line);
+		}
+	} sort { $a <=> $b } keys(%selected) ];
+}
+
+sub display_hunk_lines {
+	my $hunk = shift;
+	my ($display, $lines) = ($hunk->{DISPLAY}, $hunk->{LABELS}->{LINES});
+	my $max_label = $#{$lines};
+	my $width = int(log($max_label) / log(10)) + 1;
+	my $padding = ' ' x ($width + 1);
+	my $label = 1;
+	for my $line (0..$#{$display}) {
+		if ($lines->[$label] == $line) {
+			printf '%*d %s', $width, $label, $display->[$line];
+			$label++ if ($label < $max_label);
+		} else {
+			print $padding . $display->[$line];
+		}
+	}
+}
+
+sub select_lines_loop {
+	my $hunk = shift;
+	display_hunk_lines($hunk);
+	my $selection = undef;
+	until (defined $selection) {
+		print colored $prompt_color, __("select lines? ");
+		my $text = <STDIN>;
+		defined $text and $text =~ /\S/ or return undef;
+		$selection = parse_hunk_selection($hunk, $text);
+	}
+	return select_hunk_lines($hunk, $selection);
+}
+
 my %edit_hunk_manually_modes = (
 	stage => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
@@ -1255,6 +1420,7 @@ j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
+l - select hunk lines to use
 s - split the current hunk into smaller hunks
 e - manually edit the current hunk
 ? - print help
@@ -1471,6 +1637,9 @@ sub patch_update_file {
 		if ($hunk[$ix]{TYPE} eq 'hunk') {
 			$other .= ',e';
 		}
+		if (label_hunk_lines($hunk[$ix])) {
+			$other .= ',l';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -1610,6 +1779,18 @@ sub patch_update_file {
 					next;
 				}
 			}
+			elsif ($line =~ /^l/) {
+				unless ($other =~ /l/) {
+					error_msg __("Cannot select line by line\n");
+					next;
+				}
+				my $newhunk = select_lines_loop($hunk[$ix]);
+				if ($newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				} else {
+					next;
+				}
+			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index f1bb879ea4..5b535a22d5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -403,6 +403,101 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 	! grep "^+31" actual
 '
 
+test_expect_success 'setup test file' '
+	test_write_lines 10 31 32 33 60 >test &&
+	printf 61 >>test
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..341cc6b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,6 @@
+	 10
+	 20
+	-30
+	-40
+	-50
+	+31
+	+33
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage individual lines of patch' '
+	git reset &&
+	printf "%s\n" l "5-2,7-" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,7 @@
+	 10
+	 20
+	 30
+	-40
+	 50
+	+33
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can reset individual lines of patch' '
+	printf "%s\n" l 4,1,3 |
+	EDITOR=: git reset -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup file' '
+	test_write_lines ten twenty thirty forty fifty >test &&
+	printf sixty >>test
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	-30
+	+sixty
+	 40
+	 50
+	 60
+	EOF
+'
+
+test_expect_failure 'can stage modified lines of patch (1)' '
+	git reset &&
+	printf "%s\n" l "1,3 7-9 12" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
@@ -571,4 +666,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
 	test_cmp expected-2 actual
 '
 
+test_expect_success 'add -p selecting lines works with pathological context lines' '
+	git reset &&
+	printf "%s\n" l 2 y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a >actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH v4 2/4] add -p: select modified lines correctly
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 1/4] " Phillip Wood
@ 2018-07-26 10:22   ` Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 3/4] add -p: allow line selection to be inverted Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 4/4] add -p: optimize line selection for short hunks Phillip Wood
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a set of lines is modified the hunk contains deletions followed
by insertions. To correctly stage a subset of the modified lines we
need to match up the selected deletions with the selected insertions
otherwise we end up with deletions and context lines followed by
insertions which is not what we want.

For example given the hunk
      1 -* a longer description of the
      2 -  first item
      3 -* second
      4 -* third
      5 +* first
      6 +  second item
      7 +* the third item

If the user selects 1,2,4–5,7 then we should generate
	-* a longer description of the
	-  first item
	+* first
	 * second
	-* third
	+* the third item

not
	-* a longer description of the
	-  first item
	 * second
	-* third
	+* first
	+* the third item

Currently the code can only cope with selections that contain the same
number of groups of insertions and deletions, though each group need
not contain the same number of insertions and deletions. If the user
wants to stage an unpaired deletion or insertion in a hunk where they
also want to stage modified lines they have to do it with two
invocations of 'git add -p'.

It would be possible to add some syntax to allow lines to be excluded
from groups to allow the user to stage such changes in a single
go. It may also be useful to allow users to explicitly group lines. If
in the example above the second item is deleted we have

      1 -* a longer description of the
      2 - first item
      3 -* second
      4 -* third
      5 +* first
      6 +* the third item

Selecting 1,2,4–6 will give an error. If lines could be grouped
explicitly then it would be possible to type something like
1,2,4,[5],6 to indicate that there are two groups of insertions giving

	-* a longer description of the
	- first item
	+* first
	 * second
	-* third
	+* the third item

We may want to be able to stage an insertion before an unselected
deletion to allow the user to stage a new paragraph before the
unmodified original in

      1 -original
      2 +a new paragraph before
      3 +original
      4 +
      5 +modified original

by specifying something like ^2-4 to give

	+a new paragraph before
	+original
	+
	 original

I'm not sure how common these cases are in real life and how much
effort it's worth putting into handling them at the moment when the
user can edit the hunk if need be. Perhaps it would be better to leave
it for future extensions when it becomes clearer what would be most
useful.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 148 ++++++++++++++++++++++++++++++++++---
 t/t3701-add-interactive.sh | 108 ++++++++++++++++++++++++++-
 2 files changed, 243 insertions(+), 13 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cbc9e5698a..b7e3da0210 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1015,26 +1015,47 @@ sub color_diff {
 
 use constant {
 	NO_NEWLINE => 1,
+	LAST_ADD_DEL => 2,
+	FIRST_ADD => 4
 };
 
 sub label_hunk_lines {
 	my $hunk = shift;
 	my $text = $hunk->{TEXT};
-	my (@line_flags, @lines);
+	# A block contains the insertions and deletions occurring context
+	# lines.
+	my (@blocks, @line_flags, @lines, @modes);
 	my ($block, $label, $last_mode) = (0, 0, '');
 	for my $line (1..$#{$text}) {
 		$line_flags[$line] = 0;
 		my $mode = substr($text->[$line], 0, 1);
 		if ($mode eq '\\') {
 			$line_flags[$line - 1] |= NO_NEWLINE;
 		}
+		if ($mode ne '-' and $last_mode eq '-' or
+		    $mode ne '+' and $last_mode eq '+') {
+			$line_flags[$line - 1] |= LAST_ADD_DEL;
+		}
+		if ($mode eq '+' and $last_mode ne '+') {
+			$line_flags[$line] |= FIRST_ADD;
+		}
 		if ($mode eq '-' or $mode eq '+') {
-			$lines[++$label] = $line;
+			$blocks[++$label] = $block;
+			$lines[$label] = $line;
+			$modes[$label] = $mode;
+		} elsif ($mode eq ' ' and $last_mode ne ' ') {
+			$block++;
 		}
+		$last_mode = $mode;
+	}
+	if ($last_mode eq '-' or $last_mode eq '+') {
+		$line_flags[-1] |= LAST_ADD_DEL;
 	}
 	if ($label > 1) {
 		$hunk->{LABELS} = {
+			BLOCKS => \@blocks,
 			LINES => \@lines,
+			MODES => \@modes
 		};
 		$hunk->{LINE_FLAGS} = \@line_flags;
 		return 1;
@@ -1061,11 +1082,14 @@ sub select_hunk_lines {
 		}
 	};
 
-	my ($lo, $hi) = splice(@$selected, 0, 2);
+	my ($lo, $hi, $pre_insert, $post_insert) = splice(@$selected, 0, 4);
 	# Lines with this mode will become context lines if they are
 	# not selected
 	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
 	for $i (1..$#{$text}) {
+		if ($i == $lo and defined($pre_insert)) {
+			$select_lines->(@$pre_insert);
+		}
 		if ($lo <= $i and $i <= $hi) {
 			$select_lines->($i);
 		} else {
@@ -1080,8 +1104,12 @@ sub select_hunk_lines {
 			}
 		}
 		if ($i == $hi) {
+			if (defined($post_insert)) {
+				$select_lines->(@$post_insert);
+			}
 			if (@$selected) {
-				($lo, $hi) = splice(@$selected, 0, 2);
+				($lo, $hi, $pre_insert, $post_insert) =
+						splice(@$selected, 0, 4);
 			}
 		}
 	}
@@ -1104,6 +1132,109 @@ sub select_hunk_lines {
 	return $newhunk;
 }
 
+sub pair_hunk_selection {
+	my $line_flags = shift;
+	my @ret;
+	my $reverse = $patch_mode_flavour{IS_REVERSE};
+	for my $sel (@_) {
+		my $i = 0;
+		my ($del, $add) = @$sel;
+		my $ndel = @$del / 2;
+		my $nadd = @$add / 2;
+		# If there the same number of insertion and deletion
+		# groups then pair up in insertions and deletions.
+		if ($nadd == $ndel) {
+			while ($i < @{$del}) {
+				my ($ra, $rb) = $reverse ? ($add, $del) :
+							   ($del, $add);
+				my ($starta, $enda) = @{$ra}[$i, $i + 1];
+				my ($startb, $endb) = @{$rb}[$i, $i + 1];
+				if ($line_flags->[$enda] & LAST_ADD_DEL and
+				    $line_flags->[$endb] & NO_NEWLINE) {
+					$endb++;
+				}
+				if ($line_flags->[$enda] & NO_NEWLINE) {
+					$enda++;
+				}
+				push @ret, $reverse ?
+					($starta, $enda,
+					 [ $startb..$endb ], undef) :
+					($starta, $enda,
+					 undef, [ $startb..$endb ]);
+				$i += 2;
+			}
+		}
+		# If there are only deletions or only insertions or
+		# a) we're adding to the index and there is a single group
+		#    of insertions starting at the first inserted line
+		# or
+		# b) we're resetting the index and there is a single group
+		#    of deletions finishing with the last deleted line
+		# then it is okay just to have deletions followed by
+		# insertions.
+		} elsif (!$nadd or !$ndel or
+			 (!$reverse and $ndel == 1 and
+			  $line_flags->[$del->[1]] & LAST_ADD_DEL) or
+			 ($reverse and $nadd == 1 and
+			  $line_flags->[$add->[0]] & FIRST_ADD)) {
+			while ($i < @$del) {
+				my ($start, $end) = @{$del}[$i, $i + 1];
+				if ($line_flags->[$end] & NO_NEWLINE) {
+					$end++;
+				}
+				push @ret, $start, $end, undef, undef;
+				$i += 2;
+			}
+			$i = 0;
+			while ($i < @$add) {
+				my ($start, $end) = @{$add}[$i, $i + 1];
+				if ($line_flags->[$end] & NO_NEWLINE) {
+					$end++;
+				}
+				push @ret, $start, $end, undef, undef;
+				$i += 2;
+			}
+		} else {
+			printf STDERR __("unable to pair up insertions and deletions\n");
+			return undef;
+		}
+	}
+	return \@ret;
+}
+
+sub process_hunk_selection {
+	my ($labels, $line_flags) = @{shift()}{qw(LABELS LINE_FLAGS)};
+	my ($blocks, $lines, $modes) =
+				@{$labels}{qw(BLOCKS LINES MODES)};
+	my @selection = sort { $a <=> $b } @_;
+	unless (@selection) {
+		return [];
+	}
+	my $last_label = shift @selection;
+	my ($last_block, $last_line, $last_mode) =
+	    ($blocks->[$last_label], $lines->[$last_label], $modes->[$last_label]);
+	my ($del, $add) = ([], []);
+	push @{$last_mode eq '-' ? $del : $add}, $last_line;
+	my @sel;
+	for my $label (@selection) {
+		my ($block, $line, $mode) =
+			($blocks->[$label], $lines->[$label], $modes->[$label]);
+		if ($block != $last_block) {
+			push @{$last_mode eq '-' ? $del : $add}, $last_line;
+			push @sel, [ $del, $add ];
+			($del, $add) = ([], []);
+			push @{$mode eq '-' ? $del : $add}, $line;
+		} elsif ($line != $last_line + 1 or $mode ne $last_mode) {
+			push @{$last_mode eq '-' ? $del : $add}, $last_line;
+			push @{$mode eq '-' ? $del : $add}, $line;
+		}
+		($last_block, $last_line, $last_mode) = ($block, $line, $mode);
+	}
+	push @{$last_mode eq '-' ? $del : $add}, $last_line;
+	push @sel, [ $del, $add ];
+	pair_hunk_selection($line_flags, @sel);
+}
+
 sub check_hunk_label {
 	my ($max_label, $label) = @_;
 	if ($label < 1 or $label > $max_label) {
@@ -1138,14 +1269,7 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
-	[ map {
-		my $line = $lines->[$_];
-		if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) {
-			($line, $line + 1);
-		} else {
-			($line, $line);
-		}
-	} sort { $a <=> $b } keys(%selected) ];
+	return process_hunk_selection($hunk, keys(%selected));
 }
 
 sub display_hunk_lines {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5b535a22d5..1d917ad018 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -489,7 +489,7 @@ test_expect_success 'setup expected diff' '
 	EOF
 '
 
-test_expect_failure 'can stage modified lines of patch (1)' '
+test_expect_success 'can stage modified lines of patch (1)' '
 	git reset &&
 	printf "%s\n" l "1,3 7-9 12" |
 	EDITOR=: git add -p 2>error &&
@@ -498,6 +498,112 @@ test_expect_failure 'can stage modified lines of patch (1)' '
 	diff_cmp expected actual
 '
 
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,7 @@
+	 10
+	-20
+	+twenty
+	+thirty
+	 30
+	 40
+	 50
+	-60
+	+sixty
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (2)' '
+	git reset &&
+	printf "%s\n" l "2,6,8,9,12" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup HEAD' '
+	git reset &&
+	git apply --cached <<-\EOF &&
+	diff --git a/test b/test
+	--- a/test
+	+++ b/test
+	@@ -3,4 +3,4 @@
+	 30
+	 40
+	 50
+	-60
+	+60
+	\ No newline at end of file
+	EOF
+	git commit
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	-30
+	+fifty
+	 40
+	 50
+	 60
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (3)' '
+	git reset &&
+	printf "%s\n" l "11,1,3 7-9" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	 30
+	 40
+	 50
+	-60
+	\ No newline at end of file
+	+fifty
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (4)' '
+	git reset &&
+	printf "%s\n" l "9-6 11,1" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v4 3/4] add -p: allow line selection to be inverted
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 1/4] " Phillip Wood
  2018-07-26 10:22   ` [RFC PATCH v4 2/4] add -p: select modified lines correctly Phillip Wood
@ 2018-07-26 10:22   ` Phillip Wood
  2018-07-26 10:22   ` [PATCH v4 4/4] add -p: optimize line selection for short hunks Phillip Wood
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  3 ++-
 git-add--interactive.perl  | 19 +++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 965e192a09..01ff4d7d24 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -342,7 +342,8 @@ deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
 a space or comma, to specify a range of lines use a dash between
 them. If the upper bound of a range of lines is omitted it defaults to
-the last line.
+the last line. To invert the selection prefix it with "-" so "-3-5,8"
+will select everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index b7e3da0210..0b490cf1e9 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1248,8 +1248,21 @@ sub parse_hunk_selection {
 	my ($hunk, $line) = @_;
 	my $lines = $hunk->{LABELS}->{LINES};
 	my $max_label = $#{$lines};
+	my $invert = undef;
 	my %selected;
 	my @fields = split(/[,\s]+/, $line);
+	if (my ($rest) = ($fields[0] =~ /^-(.*)/)) {
+		$invert = 1;
+		if ($rest ne '') {
+			$fields[0] = $rest;
+		} else {
+			shift @fields;
+			unless (@fields) {
+				error_msg __("no lines to invert\n");
+				return undef;
+			}
+		}
+	}
 	for my $f (@fields) {
 		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
 			if ($hi eq '') {
@@ -1269,6 +1282,12 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
+	if ($invert) {
+		my %inverted;
+		undef @inverted{1..$max_label};
+		delete @inverted{keys(%selected)};
+		%selected = %inverted;
+	}
 	return process_hunk_selection($hunk, keys(%selected));
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 1d917ad018..2fd456017f 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l 4,1,3 |
+	printf "%s\n" l -6,2,5 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v4 4/4] add -p: optimize line selection for short hunks
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
                     ` (2 preceding siblings ...)
  2018-07-26 10:22   ` [PATCH v4 3/4] add -p: allow line selection to be inverted Phillip Wood
@ 2018-07-26 10:22   ` Phillip Wood
  3 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 10:22 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are fewer than ten changes in a hunk then make spaces
optional when selecting individual lines. This means that for short
hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  9 +++++----
 git-add--interactive.perl  | 26 ++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 01ff4d7d24..f3c81dfb11 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -340,10 +340,11 @@ patch::
 If you press "l" then the hunk will be reprinted with each insertion or
 deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
-a space or comma, to specify a range of lines use a dash between
-them. If the upper bound of a range of lines is omitted it defaults to
-the last line. To invert the selection prefix it with "-" so "-3-5,8"
-will select everything except lines 3, 4, 5 and 8.
+a space or comma (these can be omitted if there are fewer than ten
+labelled lines), to specify a range of lines use a dash between them. If
+the upper bound of a range of lines is omitted it defaults to the last
+line. To invert the selection prefix it with "-" so "-3-5,8" will select
+everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 0b490cf1e9..56bc7ab496 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1244,6 +1244,29 @@ sub check_hunk_label {
 	return 1;
 }
 
+sub split_hunk_selection {
+	my @fields = @_;
+	my @ret;
+	for my $field (@fields) {
+		while ($field ne '') {
+			if ($field =~ /^[0-9]-$/) {
+				push @ret, $field;
+				last;
+			} elsif (my ($sel, $rest) =
+					($field =~ /^([0-9](?:-[0-9])?)(.*)/)) {
+				push @ret, $sel;
+				$field = $rest;
+			} else {
+				error_msg sprintf
+				    __("invalid hunk line '%s'\n"),
+				    substr($field, 0, 1);
+				return ();
+			}
+		}
+	}
+	return @ret;
+}
+
 sub parse_hunk_selection {
 	my ($hunk, $line) = @_;
 	my $lines = $hunk->{LABELS}->{LINES};
@@ -1263,6 +1286,9 @@ sub parse_hunk_selection {
 			}
 		}
 	}
+	if ($max_label < 10) {
+		@fields = split_hunk_selection(@fields) or return undef;
+	}
 	for my $f (@fields) {
 		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
 			if ($hi eq '') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2fd456017f..b2b808275c 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l -6,2,5 |
+	printf "%s\n" l -625 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
                   ` (6 preceding siblings ...)
  2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
@ 2018-07-26 15:58 ` Phillip Wood
  2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
                     ` (5 more replies)
  7 siblings, 6 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

Unfortuantely v4 had test failures due to a suprious brace from a last
minute edit to a comment that I forgot to test. This version fixes
that, my applogies for the patch churn.

I've updated this series based on Ævar's feedback on v3 (to paraphrase
stop using '$_' so much and fix staging modified lines.). The first
patch is functionally equivalent to the previous version but with a
reworked implementation. Patch 2 is new, it implements correctly
staging modified lines with a couple of limitations - see the commit
message for more details, I'm keen to get some feedback on it. Patches
3 and 4 are essentially rebased and tweaked versions of patches 2 and
3 from the previous version.

This series is based on pw/add-p-recount (f4d35a6b49 "add -p: fix
counting empty context lines in edited patches")

The motivation for this series is summed up in the first commit
message:

"When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where either 'a' or 'b' may be
omitted to mean all lines from 'a' to the end of the hunk or all lines
from 1 upto and including 'b'."

Phillip Wood (4):
  add -p: select individual hunk lines
  add -p: select modified lines correctly
  add -p: allow line selection to be inverted
  add -p: optimize line selection for short hunks

 Documentation/git-add.txt  |  10 ++
 git-add--interactive.perl  | 349 +++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh | 209 ++++++++++++++++++++++
 3 files changed, 568 insertions(+)

-- 
2.18.0


^ permalink raw reply	[flat|nested] 53+ messages in thread

* [PATCH v5 1/4] add -p: select individual hunk lines
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
@ 2018-07-26 15:58   ` Phillip Wood
  2018-07-26 19:36     ` Junio C Hamano
  2018-07-26 15:58   ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When I end up editing hunks it is almost always because I want to
stage a subset of the lines in the hunk. Doing this by editing the
hunk is inconvenient and error prone (especially so if the patch is
going to be reversed before being applied). Instead offer an option
for add -p to stage individual lines. When the user presses 'l' the
hunk is redrawn with labels by the insertions and deletions and they
are prompted to enter a list of the lines they wish to stage. Ranges
of lines may be specified using 'a-b' where 'b' may be omitted to mean
all lines from 'a' to the end of the hunk. Modified lines are not
handled correctly, that will be fixed in the next commit.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |   8 ++
 git-add--interactive.perl  | 181 +++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh | 103 +++++++++++++++++++++
 3 files changed, 292 insertions(+)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index d50fa339dc..965e192a09 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -332,10 +332,18 @@ patch::
        J - leave this hunk undecided, see next hunk
        k - leave this hunk undecided, see previous undecided hunk
        K - leave this hunk undecided, see previous hunk
+       l - select hunk lines to use
        s - split the current hunk into smaller hunks
        e - manually edit the current hunk
        ? - print help
 +
+If you press "l" then the hunk will be reprinted with each insertion or
+deletion labelled with a number and you will be prompted to enter which
+lines you wish to select. Individual line numbers should be separated by
+a space or comma, to specify a range of lines use a dash between
+them. If the upper bound of a range of lines is omitted it defaults to
+the last line.
++
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
 +
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 8361ef45e7..cbc9e5698a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1013,6 +1013,171 @@ sub color_diff {
 	} @_;
 }
 
+use constant {
+	NO_NEWLINE => 1,
+};
+
+sub label_hunk_lines {
+	my $hunk = shift;
+	my $text = $hunk->{TEXT};
+	my (@line_flags, @lines);
+	my ($block, $label, $last_mode) = (0, 0, '');
+	for my $line (1..$#{$text}) {
+		$line_flags[$line] = 0;
+		my $mode = substr($text->[$line], 0, 1);
+		if ($mode eq '\\') {
+			$line_flags[$line - 1] |= NO_NEWLINE;
+		}
+		if ($mode eq '-' or $mode eq '+') {
+			$lines[++$label] = $line;
+		}
+	}
+	if ($label > 1) {
+		$hunk->{LABELS} = {
+			LINES => \@lines,
+		};
+		$hunk->{LINE_FLAGS} = \@line_flags;
+		return 1;
+	}
+	return 0;
+}
+
+sub select_hunk_lines {
+	my ($hunk, $selected) = @_;
+	my ($line_flags, $text) = @{$hunk}{qw(LINE_FLAGS TEXT)};
+	my ($i, $o_cnt, $n_cnt) = (0, 0, 0);
+	my @newtext;
+
+	my $select_lines = sub {
+		for my $i (@_) {
+			my $line = $text->[$i];
+			my $mode = substr($line, 0, 1);
+			push @newtext, $line;
+			if ($mode eq '+') {
+				$n_cnt++;
+			} elsif ($mode eq '-') {
+				$o_cnt++;
+			}
+		}
+	};
+
+	my ($lo, $hi) = splice(@$selected, 0, 2);
+	# Lines with this mode will become context lines if they are
+	# not selected
+	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
+	for $i (1..$#{$text}) {
+		if ($lo <= $i and $i <= $hi) {
+			$select_lines->($i);
+		} else {
+			my $line = $text->[$i];
+			my $mode = substr($line, 0, 1);
+			if ($mode eq ' ' or $mode eq $context_mode) {
+				push @newtext, ' ' . substr($line, 1);
+				$o_cnt++; $n_cnt++;
+				if ($line_flags->[$i] & NO_NEWLINE) {
+					push @newtext, $text->[$i + 1];
+				}
+			}
+		}
+		if ($i == $hi) {
+			if (@$selected) {
+				($lo, $hi) = splice(@$selected, 0, 2);
+			}
+		}
+	}
+	my ($o_ofs, $orig_o_cnt, $n_ofs, $orig_n_cnt) =
+					parse_hunk_header($text->[0]);
+	unshift @newtext, format_hunk_header($o_ofs, $o_cnt, $n_ofs, $n_cnt);
+	my $newhunk = {
+		TEXT => \@newtext,
+		DISPLAY => [ color_diff(@newtext) ],
+		OFS_DELTA => $orig_o_cnt - $orig_n_cnt - $o_cnt + $n_cnt,
+		TYPE => $hunk->{TYPE},
+		USE => 1,
+	};
+	# If this hunk has previously been edited add the offset delta
+	# of the old hunk to get the real delta from the original
+	# hunk.
+	if ($hunk->{OFS_DELTA}) {
+		$newhunk->{OFS_DELTA} += $hunk->{OFS_DELTA};
+	}
+	return $newhunk;
+}
+
+sub check_hunk_label {
+	my ($max_label, $label) = @_;
+	if ($label < 1 or $label > $max_label) {
+		error_msg sprintf(__("invalid hunk line '%d'\n"), $label);
+		return 0;
+	}
+	return 1;
+}
+
+sub parse_hunk_selection {
+	my ($hunk, $line) = @_;
+	my $lines = $hunk->{LABELS}->{LINES};
+	my $max_label = $#{$lines};
+	my %selected;
+	my @fields = split(/[,\s]+/, $line);
+	for my $f (@fields) {
+		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
+			if ($hi eq '') {
+				$hi = $max_label;
+			}
+			check_hunk_label($max_label, $lo) or return undef;
+			check_hunk_label($max_label, $hi) or return undef;
+			if ($hi < $lo) {
+				($lo, $hi) = ($hi, $lo);
+			}
+			undef @selected{$lo..$hi};
+		} elsif (my ($label) = ($f =~ /^([0-9]+)$/)) {
+			check_hunk_label($max_label, $label) or return undef;
+			undef $selected{$label};
+		} else {
+			error_msg sprintf(__("invalid hunk line '%s'\n"), $f);
+			return undef;
+		}
+	}
+	[ map {
+		my $line = $lines->[$_];
+		if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) {
+			($line, $line + 1);
+		} else {
+			($line, $line);
+		}
+	} sort { $a <=> $b } keys(%selected) ];
+}
+
+sub display_hunk_lines {
+	my $hunk = shift;
+	my ($display, $lines) = ($hunk->{DISPLAY}, $hunk->{LABELS}->{LINES});
+	my $max_label = $#{$lines};
+	my $width = int(log($max_label) / log(10)) + 1;
+	my $padding = ' ' x ($width + 1);
+	my $label = 1;
+	for my $line (0..$#{$display}) {
+		if ($lines->[$label] == $line) {
+			printf '%*d %s', $width, $label, $display->[$line];
+			$label++ if ($label < $max_label);
+		} else {
+			print $padding . $display->[$line];
+		}
+	}
+}
+
+sub select_lines_loop {
+	my $hunk = shift;
+	display_hunk_lines($hunk);
+	my $selection = undef;
+	until (defined $selection) {
+		print colored $prompt_color, __("select lines? ");
+		my $text = <STDIN>;
+		defined $text and $text =~ /\S/ or return undef;
+		$selection = parse_hunk_selection($hunk, $text);
+	}
+	return select_hunk_lines($hunk, $selection);
+}
+
 my %edit_hunk_manually_modes = (
 	stage => N__(
 "If the patch applies cleanly, the edited hunk will immediately be
@@ -1255,6 +1420,7 @@ j - leave this hunk undecided, see next undecided hunk
 J - leave this hunk undecided, see next hunk
 k - leave this hunk undecided, see previous undecided hunk
 K - leave this hunk undecided, see previous hunk
+l - select hunk lines to use
 s - split the current hunk into smaller hunks
 e - manually edit the current hunk
 ? - print help
@@ -1471,6 +1637,9 @@ sub patch_update_file {
 		if ($hunk[$ix]{TYPE} eq 'hunk') {
 			$other .= ',e';
 		}
+		if (label_hunk_lines($hunk[$ix])) {
+			$other .= ',l';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -1610,6 +1779,18 @@ sub patch_update_file {
 					next;
 				}
 			}
+			elsif ($line =~ /^l/) {
+				unless ($other =~ /l/) {
+					error_msg __("Cannot select line by line\n");
+					next;
+				}
+				my $newhunk = select_lines_loop($hunk[$ix]);
+				if ($newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				} else {
+					next;
+				}
+			}
 			elsif ($other =~ /s/ && $line =~ /^s/) {
 				my @split = split_hunk($hunk[$ix]{TEXT}, $hunk[$ix]{DISPLAY});
 				if (1 < @split) {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index f1bb879ea4..5b535a22d5 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -403,6 +403,101 @@ test_expect_failure 'split hunk "add -p (no, yes, edit)"' '
 	! grep "^+31" actual
 '
 
+test_expect_success 'setup test file' '
+	test_write_lines 10 31 32 33 60 >test &&
+	printf 61 >>test
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..341cc6b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,6 @@
+	 10
+	 20
+	-30
+	-40
+	-50
+	+31
+	+33
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage individual lines of patch' '
+	git reset &&
+	printf "%s\n" l "5-2,7-" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,7 @@
+	 10
+	 20
+	 30
+	-40
+	 50
+	+33
+	 60
+	+61
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can reset individual lines of patch' '
+	printf "%s\n" l 4,1,3 |
+	EDITOR=: git reset -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup file' '
+	test_write_lines ten twenty thirty forty fifty >test &&
+	printf sixty >>test
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	-30
+	+sixty
+	 40
+	 50
+	 60
+	EOF
+'
+
+test_expect_failure 'can stage modified lines of patch (1)' '
+	git reset &&
+	printf "%s\n" l "1,3 7-9 12" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
@@ -571,4 +666,12 @@ test_expect_success 'add -p patch editing works with pathological context lines'
 	test_cmp expected-2 actual
 '
 
+test_expect_success 'add -p selecting lines works with pathological context lines' '
+	git reset &&
+	printf "%s\n" l 2 y |
+	GIT_EDITOR=./editor git add -p &&
+	git cat-file blob :a >actual &&
+	test_cmp expected-2 actual
+'
+
 test_done
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [RFC PATCH v5 2/4] add -p: select modified lines correctly
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
  2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
@ 2018-07-26 15:58   ` Phillip Wood
  2018-07-26 19:30     ` Junio C Hamano
  2018-07-26 15:58   ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood
                     ` (3 subsequent siblings)
  5 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

When a set of lines is modified the hunk contains deletions followed
by insertions. To correctly stage a subset of the modified lines we
need to match up the selected deletions with the selected insertions
otherwise we end up with deletions and context lines followed by
insertions which is not what we want.

For example given the hunk
      1 -* a longer description of the
      2 -  first item
      3 -* second
      4 -* third
      5 +* first
      6 +  second item
      7 +* the third item

If the user selects 1,2,4–5,7 then we should generate
	-* a longer description of the
	-  first item
	+* first
	 * second
	-* third
	+* the third item

not
	-* a longer description of the
	-  first item
	 * second
	-* third
	+* first
	+* the third item

Currently the code can only cope with selections that contain the same
number of groups of insertions and deletions, though each group need
not contain the same number of insertions and deletions. If the user
wants to stage an unpaired deletion or insertion in a hunk where they
also want to stage modified lines they have to do it with two
invocations of 'git add -p'.

It would be possible to add some syntax to allow lines to be excluded
from groups to allow the user to stage such changes in a single
go. It may also be useful to allow users to explicitly group lines. If
in the example above the second item is deleted we have

      1 -* a longer description of the
      2 - first item
      3 -* second
      4 -* third
      5 +* first
      6 +* the third item

Selecting 1,2,4–6 will give an error. If lines could be grouped
explicitly then it would be possible to type something like
1,2,4,[5],6 to indicate that there are two groups of insertions giving

	-* a longer description of the
	- first item
	+* first
	 * second
	-* third
	+* the third item

We may want to be able to stage an insertion before an unselected
deletion to allow the user to stage a new paragraph before the
unmodified original in

      1 -original
      2 +a new paragraph before
      3 +original
      4 +
      5 +modified original

by specifying something like ^2-4 to give

	+a new paragraph before
	+original
	+
	 original

I'm not sure how common these cases are in real life and how much
effort it's worth putting into handling them at the moment when the
user can edit the hunk if need be. Perhaps it would be better to leave
it for future extensions when it becomes clearer what would be most
useful.

Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 git-add--interactive.perl  | 147 ++++++++++++++++++++++++++++++++++---
 t/t3701-add-interactive.sh | 108 ++++++++++++++++++++++++++-
 2 files changed, 242 insertions(+), 13 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index cbc9e5698a..7e4daee2fc 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1015,26 +1015,47 @@ sub color_diff {
 
 use constant {
 	NO_NEWLINE => 1,
+	LAST_ADD_DEL => 2,
+	FIRST_ADD => 4
 };
 
 sub label_hunk_lines {
 	my $hunk = shift;
 	my $text = $hunk->{TEXT};
-	my (@line_flags, @lines);
+	# A block contains the insertions and deletions occurring context
+	# lines.
+	my (@blocks, @line_flags, @lines, @modes);
 	my ($block, $label, $last_mode) = (0, 0, '');
 	for my $line (1..$#{$text}) {
 		$line_flags[$line] = 0;
 		my $mode = substr($text->[$line], 0, 1);
 		if ($mode eq '\\') {
 			$line_flags[$line - 1] |= NO_NEWLINE;
 		}
+		if ($mode ne '-' and $last_mode eq '-' or
+		    $mode ne '+' and $last_mode eq '+') {
+			$line_flags[$line - 1] |= LAST_ADD_DEL;
+		}
+		if ($mode eq '+' and $last_mode ne '+') {
+			$line_flags[$line] |= FIRST_ADD;
+		}
 		if ($mode eq '-' or $mode eq '+') {
-			$lines[++$label] = $line;
+			$blocks[++$label] = $block;
+			$lines[$label] = $line;
+			$modes[$label] = $mode;
+		} elsif ($mode eq ' ' and $last_mode ne ' ') {
+			$block++;
 		}
+		$last_mode = $mode;
+	}
+	if ($last_mode eq '-' or $last_mode eq '+') {
+		$line_flags[-1] |= LAST_ADD_DEL;
 	}
 	if ($label > 1) {
 		$hunk->{LABELS} = {
+			BLOCKS => \@blocks,
 			LINES => \@lines,
+			MODES => \@modes
 		};
 		$hunk->{LINE_FLAGS} = \@line_flags;
 		return 1;
@@ -1061,11 +1082,14 @@ sub select_hunk_lines {
 		}
 	};
 
-	my ($lo, $hi) = splice(@$selected, 0, 2);
+	my ($lo, $hi, $pre_insert, $post_insert) = splice(@$selected, 0, 4);
 	# Lines with this mode will become context lines if they are
 	# not selected
 	my $context_mode = $patch_mode_flavour{IS_REVERSE} ? '+' : '-';
 	for $i (1..$#{$text}) {
+		if ($i == $lo and defined($pre_insert)) {
+			$select_lines->(@$pre_insert);
+		}
 		if ($lo <= $i and $i <= $hi) {
 			$select_lines->($i);
 		} else {
@@ -1080,8 +1104,12 @@ sub select_hunk_lines {
 			}
 		}
 		if ($i == $hi) {
+			if (defined($post_insert)) {
+				$select_lines->(@$post_insert);
+			}
 			if (@$selected) {
-				($lo, $hi) = splice(@$selected, 0, 2);
+				($lo, $hi, $pre_insert, $post_insert) =
+						splice(@$selected, 0, 4);
 			}
 		}
 	}
@@ -1104,6 +1132,108 @@ sub select_hunk_lines {
 	return $newhunk;
 }
 
+sub pair_hunk_selection {
+	my $line_flags = shift;
+	my @ret;
+	my $reverse = $patch_mode_flavour{IS_REVERSE};
+	for my $sel (@_) {
+		my $i = 0;
+		my ($del, $add) = @$sel;
+		my $ndel = @$del / 2;
+		my $nadd = @$add / 2;
+		# If there the same number of insertion and deletion
+		# groups then pair up in insertions and deletions.
+		if ($nadd == $ndel) {
+			while ($i < @{$del}) {
+				my ($ra, $rb) = $reverse ? ($add, $del) :
+							   ($del, $add);
+				my ($starta, $enda) = @{$ra}[$i, $i + 1];
+				my ($startb, $endb) = @{$rb}[$i, $i + 1];
+				if ($line_flags->[$enda] & LAST_ADD_DEL and
+				    $line_flags->[$endb] & NO_NEWLINE) {
+					$endb++;
+				}
+				if ($line_flags->[$enda] & NO_NEWLINE) {
+					$enda++;
+				}
+				push @ret, $reverse ?
+					($starta, $enda,
+					 [ $startb..$endb ], undef) :
+					($starta, $enda,
+					 undef, [ $startb..$endb ]);
+				$i += 2;
+			}
+		# If there are only deletions or only insertions or
+		# a) we're adding to the index and there is a single group
+		#    of insertions starting at the first inserted line
+		# or
+		# b) we're resetting the index and there is a single group
+		#    of deletions finishing with the last deleted line
+		# then it is okay just to have deletions followed by
+		# insertions.
+		} elsif (!$nadd or !$ndel or
+			 (!$reverse and $ndel == 1 and
+			  $line_flags->[$del->[1]] & LAST_ADD_DEL) or
+			 ($reverse and $nadd == 1 and
+			  $line_flags->[$add->[0]] & FIRST_ADD)) {
+			while ($i < @$del) {
+				my ($start, $end) = @{$del}[$i, $i + 1];
+				if ($line_flags->[$end] & NO_NEWLINE) {
+					$end++;
+				}
+				push @ret, $start, $end, undef, undef;
+				$i += 2;
+			}
+			$i = 0;
+			while ($i < @$add) {
+				my ($start, $end) = @{$add}[$i, $i + 1];
+				if ($line_flags->[$end] & NO_NEWLINE) {
+					$end++;
+				}
+				push @ret, $start, $end, undef, undef;
+				$i += 2;
+			}
+		} else {
+			printf STDERR __("unable to pair up insertions and deletions\n");
+			return undef;
+		}
+	}
+	return \@ret;
+}
+
+sub process_hunk_selection {
+	my ($labels, $line_flags) = @{shift()}{qw(LABELS LINE_FLAGS)};
+	my ($blocks, $lines, $modes) =
+				@{$labels}{qw(BLOCKS LINES MODES)};
+	my @selection = sort { $a <=> $b } @_;
+	unless (@selection) {
+		return [];
+	}
+	my $last_label = shift @selection;
+	my ($last_block, $last_line, $last_mode) =
+	    ($blocks->[$last_label], $lines->[$last_label], $modes->[$last_label]);
+	my ($del, $add) = ([], []);
+	push @{$last_mode eq '-' ? $del : $add}, $last_line;
+	my @sel;
+	for my $label (@selection) {
+		my ($block, $line, $mode) =
+			($blocks->[$label], $lines->[$label], $modes->[$label]);
+		if ($block != $last_block) {
+			push @{$last_mode eq '-' ? $del : $add}, $last_line;
+			push @sel, [ $del, $add ];
+			($del, $add) = ([], []);
+			push @{$mode eq '-' ? $del : $add}, $line;
+		} elsif ($line != $last_line + 1 or $mode ne $last_mode) {
+			push @{$last_mode eq '-' ? $del : $add}, $last_line;
+			push @{$mode eq '-' ? $del : $add}, $line;
+		}
+		($last_block, $last_line, $last_mode) = ($block, $line, $mode);
+	}
+	push @{$last_mode eq '-' ? $del : $add}, $last_line;
+	push @sel, [ $del, $add ];
+	pair_hunk_selection($line_flags, @sel);
+}
+
 sub check_hunk_label {
 	my ($max_label, $label) = @_;
 	if ($label < 1 or $label > $max_label) {
@@ -1138,14 +1268,7 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
-	[ map {
-		my $line = $lines->[$_];
-		if ($hunk->{LINE_FLAGS}->[$line] & NO_NEWLINE) {
-			($line, $line + 1);
-		} else {
-			($line, $line);
-		}
-	} sort { $a <=> $b } keys(%selected) ];
+	return process_hunk_selection($hunk, keys(%selected));
 }
 
 sub display_hunk_lines {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 5b535a22d5..1d917ad018 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -489,7 +489,7 @@ test_expect_success 'setup expected diff' '
 	EOF
 '
 
-test_expect_failure 'can stage modified lines of patch (1)' '
+test_expect_success 'can stage modified lines of patch (1)' '
 	git reset &&
 	printf "%s\n" l "1,3 7-9 12" |
 	EDITOR=: git add -p 2>error &&
@@ -498,6 +498,112 @@ test_expect_failure 'can stage modified lines of patch (1)' '
 	diff_cmp expected actual
 '
 
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,7 @@
+	 10
+	-20
+	+twenty
+	+thirty
+	 30
+	 40
+	 50
+	-60
+	+sixty
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (2)' '
+	git reset &&
+	printf "%s\n" l "2,6,8,9,12" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup HEAD' '
+	git reset &&
+	git apply --cached <<-\EOF &&
+	diff --git a/test b/test
+	--- a/test
+	+++ b/test
+	@@ -3,4 +3,4 @@
+	 30
+	 40
+	 50
+	-60
+	+60
+	\ No newline at end of file
+	EOF
+	git commit
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	-30
+	+fifty
+	 40
+	 50
+	 60
+	\ No newline at end of file
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (3)' '
+	git reset &&
+	printf "%s\n" l "11,1,3 7-9" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
+test_expect_success 'setup expected diff' '
+	cat >expected <<-\EOF
+	diff --git a/test b/test
+	index 0889435..cc6163b 100644
+	--- a/test
+	+++ b/test
+	@@ -1,6 +1,8 @@
+	-10
+	+ten
+	+twenty
+	+thirty
+	 20
+	 30
+	 40
+	 50
+	-60
+	\ No newline at end of file
+	+fifty
+	EOF
+'
+
+test_expect_success 'can stage modified lines of patch (4)' '
+	git reset &&
+	printf "%s\n" l "9-6 11,1" |
+	EDITOR=: git add -p 2>error &&
+	test_must_be_empty error &&
+	git diff --cached HEAD >actual &&
+	diff_cmp expected actual
+'
+
 test_expect_success 'patch mode ignores unmerged entries' '
 	git reset --hard &&
 	test_commit conflict &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v5 3/4] add -p: allow line selection to be inverted
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
  2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
  2018-07-26 15:58   ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood
@ 2018-07-26 15:58   ` Phillip Wood
  2018-07-26 15:58   ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

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

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  3 ++-
 git-add--interactive.perl  | 19 +++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 965e192a09..01ff4d7d24 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -342,7 +342,8 @@ deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
 a space or comma, to specify a range of lines use a dash between
 them. If the upper bound of a range of lines is omitted it defaults to
-the last line.
+the last line. To invert the selection prefix it with "-" so "-3-5,8"
+will select everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 7e4daee2fc..63541d0f90 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1247,8 +1247,21 @@ sub parse_hunk_selection {
 	my ($hunk, $line) = @_;
 	my $lines = $hunk->{LABELS}->{LINES};
 	my $max_label = $#{$lines};
+	my $invert = undef;
 	my %selected;
 	my @fields = split(/[,\s]+/, $line);
+	if (my ($rest) = ($fields[0] =~ /^-(.*)/)) {
+		$invert = 1;
+		if ($rest ne '') {
+			$fields[0] = $rest;
+		} else {
+			shift @fields;
+			unless (@fields) {
+				error_msg __("no lines to invert\n");
+				return undef;
+			}
+		}
+	}
 	for my $f (@fields) {
 		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
 			if ($hi eq '') {
@@ -1268,6 +1281,12 @@ sub parse_hunk_selection {
 			return undef;
 		}
 	}
+	if ($invert) {
+		my %inverted;
+		undef @inverted{1..$max_label};
+		delete @inverted{keys(%selected)};
+		%selected = %inverted;
+	}
 	return process_hunk_selection($hunk, keys(%selected));
 }
 
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 1d917ad018..2fd456017f 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l 4,1,3 |
+	printf "%s\n" l -6,2,5 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* [PATCH v5 4/4] add -p: optimize line selection for short hunks
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
                     ` (2 preceding siblings ...)
  2018-07-26 15:58   ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood
@ 2018-07-26 15:58   ` Phillip Wood
  2018-07-27 18:27   ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason
  2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason
  5 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-26 15:58 UTC (permalink / raw)
  To: Git Mailing List, Ævar Arnfjörð Bjarmason
  Cc: Igor Djordjevic, Junio C Hamano, Phillip Wood

From: Phillip Wood <phillip.wood@dunelm.org.uk>

If there are fewer than ten changes in a hunk then make spaces
optional when selecting individual lines. This means that for short
hunks one can just type 1-357 to stage lines 1, 2, 3, 5 & 7.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-add.txt  |  9 +++++----
 git-add--interactive.perl  | 26 ++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |  2 +-
 3 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 01ff4d7d24..f3c81dfb11 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -340,10 +340,11 @@ patch::
 If you press "l" then the hunk will be reprinted with each insertion or
 deletion labelled with a number and you will be prompted to enter which
 lines you wish to select. Individual line numbers should be separated by
-a space or comma, to specify a range of lines use a dash between
-them. If the upper bound of a range of lines is omitted it defaults to
-the last line. To invert the selection prefix it with "-" so "-3-5,8"
-will select everything except lines 3, 4, 5 and 8.
+a space or comma (these can be omitted if there are fewer than ten
+labelled lines), to specify a range of lines use a dash between them. If
+the upper bound of a range of lines is omitted it defaults to the last
+line. To invert the selection prefix it with "-" so "-3-5,8" will select
+everything except lines 3, 4, 5 and 8.
 +
 After deciding the fate for all hunks, if there is any hunk
 that was chosen, the index is updated with the selected hunks.
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 63541d0f90..054c1168a7 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1243,6 +1243,29 @@ sub check_hunk_label {
 	return 1;
 }
 
+sub split_hunk_selection {
+	my @fields = @_;
+	my @ret;
+	for my $field (@fields) {
+		while ($field ne '') {
+			if ($field =~ /^[0-9]-$/) {
+				push @ret, $field;
+				last;
+			} elsif (my ($sel, $rest) =
+					($field =~ /^([0-9](?:-[0-9])?)(.*)/)) {
+				push @ret, $sel;
+				$field = $rest;
+			} else {
+				error_msg sprintf
+				    __("invalid hunk line '%s'\n"),
+				    substr($field, 0, 1);
+				return ();
+			}
+		}
+	}
+	return @ret;
+}
+
 sub parse_hunk_selection {
 	my ($hunk, $line) = @_;
 	my $lines = $hunk->{LABELS}->{LINES};
@@ -1262,6 +1285,9 @@ sub parse_hunk_selection {
 			}
 		}
 	}
+	if ($max_label < 10) {
+		@fields = split_hunk_selection(@fields) or return undef;
+	}
 	for my $f (@fields) {
 		if (my ($lo, $hi) = ($f =~ /^([0-9]+)-([0-9]*)$/)) {
 			if ($hi eq '') {
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 2fd456017f..b2b808275c 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -457,7 +457,7 @@ test_expect_success 'setup expected diff' '
 '
 
 test_expect_success 'can reset individual lines of patch' '
-	printf "%s\n" l -6,2,5 |
+	printf "%s\n" l -625 |
 	EDITOR=: git reset -p 2>error &&
 	test_must_be_empty error &&
 	git diff --cached HEAD >actual &&
-- 
2.18.0


^ permalink raw reply related	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly
  2018-07-26 15:58   ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood
@ 2018-07-26 19:30     ` Junio C Hamano
  2018-07-27 10:19       ` Phillip Wood
  0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-07-26 19:30 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

An interesting problem you are solving ;-)

> For example given the hunk
>       1 -* a longer description of the
>       2 -  first item
>       3 -* second
>       4 -* third
>       5 +* first
>       6 +  second item
>       7 +* the third item
>
> If the user selects 1,2,4–5,7 then we should generate
> 	-* a longer description of the
> 	-  first item
> 	+* first
> 	 * second
> 	-* third
> 	+* the third item

I understood this example as "3 that is removal and 6 that is
addition are excluded---we consider that these two lines (one in the
pre-image and the other in the post-image) are _matching".  As we
are excluding a deletion, it becomes the common context line, and
any removal or addition that appear before that must stay to happen
before the common context line (i.e. removal of 1 and 2, and
addition of 5, both precede common context line "second") and any
removal or addition that appear after that must stay after the
common context (i.e. removal of "third" and addition of "the third
item" come after "second").

But then it is not clear to me what you mean by "group" below.  What
groups does the above example have?  Ones before the retained
"second" (i.e. removal 1, 2, 4 and addition 5) form one group and
ones after it (i.e. removal 4 and addition 7) form another group?

> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

Is this fixing any bug?  I usually see "Reported-by" only for a
bugfix patch but this seems to be adding a new feature (and lack of
feature is usually not a bug).

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v5 1/4] add -p: select individual hunk lines
  2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
@ 2018-07-26 19:36     ` Junio C Hamano
  2018-07-27 10:05       ` Phillip Wood
  0 siblings, 1 reply; 53+ messages in thread
From: Junio C Hamano @ 2018-07-26 19:36 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> +sub label_hunk_lines {
> +	my $hunk = shift;
> +	my $text = $hunk->{TEXT};
> +	my (@line_flags, @lines);
> +	my ($block, $label, $last_mode) = (0, 0, '');
> +	for my $line (1..$#{$text}) {

$text is a ref to an array so @$text is the whole thing, $#{$text}
is the index of the last item in that array, and $text->[0] is the
first element of that array.  This for loop runs with $line == 1
thru $line == $#{$text}, so we are somehow excluding the very first
element?

> +		$line_flags[$line] = 0;
> +		my $mode = substr($text->[$line], 0, 1);
> +		if ($mode eq '\\') {
> +			$line_flags[$line - 1] |= NO_NEWLINE;
> +		}
> +		if ($mode eq '-' or $mode eq '+') {
> +			$lines[++$label] = $line;
> +		}
> +	}
> +	if ($label > 1) {

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v5 1/4] add -p: select individual hunk lines
  2018-07-26 19:36     ` Junio C Hamano
@ 2018-07-27 10:05       ` Phillip Wood
  2018-07-27 16:09         ` Junio C Hamano
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-07-27 10:05 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

On 26/07/18 20:36, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
>> +sub label_hunk_lines {
>> +	my $hunk = shift;
>> +	my $text = $hunk->{TEXT};
>> +	my (@line_flags, @lines);
>> +	my ($block, $label, $last_mode) = (0, 0, '');
>> +	for my $line (1..$#{$text}) {
> 
> $text is a ref to an array so @$text is the whole thing, $#{$text}
> is the index of the last item in that array, and $text->[0] is the
> first element of that array.  This for loop runs with $line == 1
> thru $line == $#{$text}, so we are somehow excluding the very first
> element?

Yes that's right, $text->[0] contains the hunk header

>> +		$line_flags[$line] = 0;
>> +		my $mode = substr($text->[$line], 0, 1);
>> +		if ($mode eq '\\') {
>> +			$line_flags[$line - 1] |= NO_NEWLINE;
>> +		}
>> +		if ($mode eq '-' or $mode eq '+') {
>> +			$lines[++$label] = $line;
>> +		}
>> +	}
>> +	if ($label > 1) {


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly
  2018-07-26 19:30     ` Junio C Hamano
@ 2018-07-27 10:19       ` Phillip Wood
  2018-07-27 16:14         ` Junio C Hamano
  0 siblings, 1 reply; 53+ messages in thread
From: Phillip Wood @ 2018-07-27 10:19 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

Hi Junio, thanks for the comments

On 26/07/18 20:30, Junio C Hamano wrote:
> Phillip Wood <phillip.wood@talktalk.net> writes:
> 
> An interesting problem you are solving ;-)
> 
>> For example given the hunk
>>       1 -* a longer description of the
>>       2 -  first item
>>       3 -* second
>>       4 -* third
>>       5 +* first
>>       6 +  second item
>>       7 +* the third item
>>
>> If the user selects 1,2,4–5,7 then we should generate
>> 	-* a longer description of the
>> 	-  first item
>> 	+* first
>> 	 * second
>> 	-* third
>> 	+* the third item
> 
> I understood this example as "3 that is removal and 6 that is
> addition are excluded---we consider that these two lines (one in the
> pre-image and the other in the post-image) are _matching" As we> are excluding a deletion, it becomes the common context line, and
> any removal or addition that appear before that must stay to happen
> before the common context line (i.e. removal of 1 and 2, and
> addition of 5, both precede common context line "second") and any
> removal or addition that appear after that must stay after the
> common context (i.e. removal of "third" and addition of "the third
> item" come after "second").
> 
> But then it is not clear to me what you mean by "group" below.  What
> groups does the above example have?  Ones before the retained
> "second" (i.e. removal 1, 2, 4 and addition 5) form one group and
> ones after it (i.e. removal 4 and addition 7) form another group?

The code actually looks at the lines that are selected rather than
omitted. So in the example above it groups them as [1,2] (because they
are contiguous), [4],[5] (these are split because one is an insertion
and one a deletion) and [7]. It then sees that there are two groups of
deletions ([1,2],[4]) and two groups of insertions ([5],[7]) and so
pairs up the deletions in [12] with the insertion in [5] and likewise
with [4] and [7]. Lines 3 and 6 are never explicitly paired, although
they basically behave as if they were. One the insertions are all paired
up it walks over the list and creates a new hunk where the paired
insertions come immediately after their corresponding deletions,
unselected deletions are converted to context lines and unselected
additions are dropped.

> 
>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> 
> Is this fixing any bug?  I usually see "Reported-by" only for a
> bugfix patch but this seems to be adding a new feature (and lack of
> feature is usually not a bug).

I guess I meant that the previous series was effectively buggy as it
would give the wrong result for modified lines. I wanted to acknowledge
that Ævar spent some time testing it and pointed that out.

Best Wishes

Phillip


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [PATCH v5 1/4] add -p: select individual hunk lines
  2018-07-27 10:05       ` Phillip Wood
@ 2018-07-27 16:09         ` Junio C Hamano
  0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2018-07-27 16:09 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> On 26/07/18 20:36, Junio C Hamano wrote:
>> Phillip Wood <phillip.wood@talktalk.net> writes:
>> 
>>> +sub label_hunk_lines {
>>> +	my $hunk = shift;
>>> +	my $text = $hunk->{TEXT};
>>> +	my (@line_flags, @lines);
>>> +	my ($block, $label, $last_mode) = (0, 0, '');
>>> +	for my $line (1..$#{$text}) {
>> 
>> $text is a ref to an array so @$text is the whole thing, $#{$text}
>> is the index of the last item in that array, and $text->[0] is the
>> first element of that array.  This for loop runs with $line == 1
>> thru $line == $#{$text}, so we are somehow excluding the very first
>> element?
>
> Yes that's right, $text->[0] contains the hunk header

OK, thanks.

>
>>> +		$line_flags[$line] = 0;
>>> +		my $mode = substr($text->[$line], 0, 1);
>>> +		if ($mode eq '\\') {
>>> +			$line_flags[$line - 1] |= NO_NEWLINE;
>>> +		}
>>> +		if ($mode eq '-' or $mode eq '+') {
>>> +			$lines[++$label] = $line;
>>> +		}
>>> +	}
>>> +	if ($label > 1) {

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 2/4] add -p: select modified lines correctly
  2018-07-27 10:19       ` Phillip Wood
@ 2018-07-27 16:14         ` Junio C Hamano
  0 siblings, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2018-07-27 16:14 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Git Mailing List, Ævar Arnfjörð Bjarmason,
	Igor Djordjevic, Phillip Wood

Phillip Wood <phillip.wood@talktalk.net> writes:

> The code actually looks at the lines that are selected rather than
> omitted. So in the example above it groups them as [1,2] (because they
> are contiguous), [4],[5] (these are split because one is an insertion
> and one a deletion) and [7]. It then sees that there are two groups of
> deletions ([1,2],[4]) and two groups of insertions ([5],[7]) and so
> pairs up the deletions in [12] with the insertion in [5] and likewise
> with [4] and [7]. Lines 3 and 6 are never explicitly paired, although
> they basically behave as if they were. One the insertions are all paired
> up it walks over the list and creates a new hunk where the paired
> insertions come immediately after their corresponding deletions,
> unselected deletions are converted to context lines and unselected
> additions are dropped.

Now, without that much explanation in help text, can an average end
user use the feature, specifically, understand the reason why the
tool says it cannot handle a particular set of selected lines, and
follow the workaround suggested by the tool to do it in two (or
more) batches?  That was the real question I was getting at.  I
haven't played with the feature long enough to answer that question.

>>> Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> 
>> Is this fixing any bug?  I usually see "Reported-by" only for a
>> bugfix patch but this seems to be adding a new feature (and lack of
>> feature is usually not a bug).
>
> I guess I meant that the previous series was effectively buggy as it
> would give the wrong result for modified lines. I wanted to acknowledge
> that Ævar spent some time testing it and pointed that out.

Ah, I see.  We generally throw these into "Helped-by", I'd think.

Thanks.


^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
                     ` (3 preceding siblings ...)
  2018-07-26 15:58   ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood
@ 2018-07-27 18:27   ` Ævar Arnfjörð Bjarmason
  2018-07-28 10:08     ` Phillip Wood
  2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason
  5 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-27 18:27 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano


On Thu, Jul 26 2018, Phillip Wood wrote:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Unfortuantely v4 had test failures due to a suprious brace from a last
> minute edit to a comment that I forgot to test. This version fixes
> that, my applogies for the patch churn.
>
> I've updated this series based on Ævar's feedback on v3 (to paraphrase
> stop using '$_' so much and fix staging modified lines.). The first
> patch is functionally equivalent to the previous version but with a
> reworked implementation. Patch 2 is new, it implements correctly
> staging modified lines with a couple of limitations - see the commit
> message for more details, I'm keen to get some feedback on it. Patches
> 3 and 4 are essentially rebased and tweaked versions of patches 2 and
> 3 from the previous version.

I was going to review this, but can't find what it's based on, I can't
apply 1/4 to master, next or pu. It seems to be based on some older
version of master, e.g. 1/4 has this hunk:

    +			elsif ($line =~ /^l/) {
    +				unless ($other =~ /l/) {
    +					error_msg __("Cannot select line by line\n");
    +					next;
    +				}
    +				my $newhunk = select_lines_loop($hunk[$ix]);
    +				if ($newhunk) {
    +					splice @hunk, $ix, 1, $newhunk;
    +				} else {
    +					next;
    +				}
    +			}
     			elsif ($other =~ /s/ && $line =~ /^s/) {

Which seems to conflict with your 4bdd6e7ce3 ("add -p: improve error
messages", 2018-02-13). I could have tried to manually apply this, but
figured I'd bounce this back to you...

Having just skimmed through the patches themselves I agree with this
approach of handling the simple case (as discussed before) and leaving
the rest for some future change, but let's see about the details once I
have this running.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-07-27 18:27   ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason
@ 2018-07-28 10:08     ` Phillip Wood
  0 siblings, 0 replies; 53+ messages in thread
From: Phillip Wood @ 2018-07-28 10:08 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Phillip Wood
  Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano

On 27/07/18 19:27, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jul 26 2018, Phillip Wood wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Unfortuantely v4 had test failures due to a suprious brace from a last
>> minute edit to a comment that I forgot to test. This version fixes
>> that, my applogies for the patch churn.
>>
>> I've updated this series based on Ævar's feedback on v3 (to paraphrase
>> stop using '$_' so much and fix staging modified lines.). The first
>> patch is functionally equivalent to the previous version but with a
>> reworked implementation. Patch 2 is new, it implements correctly
>> staging modified lines with a couple of limitations - see the commit
>> message for more details, I'm keen to get some feedback on it. Patches
>> 3 and 4 are essentially rebased and tweaked versions of patches 2 and
>> 3 from the previous version.
> 
> I was going to review this, but can't find what it's based on, I can't
> apply 1/4 to master, next or pu. It seems to be based on some older
> version of master, e.g. 1/4 has this hunk:
> 
>     +			elsif ($line =~ /^l/) {
>     +				unless ($other =~ /l/) {
>     +					error_msg __("Cannot select line by line\n");
>     +					next;
>     +				}
>     +				my $newhunk = select_lines_loop($hunk[$ix]);
>     +				if ($newhunk) {
>     +					splice @hunk, $ix, 1, $newhunk;
>     +				} else {
>     +					next;
>     +				}
>     +			}
>      			elsif ($other =~ /s/ && $line =~ /^s/) {
> 
> Which seems to conflict with your 4bdd6e7ce3 ("add -p: improve error
> messages", 2018-02-13). I could have tried to manually apply this, but
> figured I'd bounce this back to you...

Yes, I wasn't sure whether to rebase or not and in the end I didn't. The
line below where you cut the cover letter message says it is based on
f4d35a6b49 "add -p: fix counting empty context lines in edited patches".
So you could just apply it there and test it. You can fetch it with

git fetch https://github.com/phillipwood/git add-i-select-lines-v5

> Having just skimmed through the patches themselves I agree with this
> approach of handling the simple case (as discussed before) and leaving
> the rest for some future change, but let's see about the details once I
> have this running.

Thanks

Phillip



^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
                     ` (4 preceding siblings ...)
  2018-07-27 18:27   ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason
@ 2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason
  2018-08-03 10:01     ` Phillip Wood
  5 siblings, 1 reply; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-07-28 12:40 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano


On Thu, Jul 26 2018, Phillip Wood wrote:

> Unfortuantely v4 had test failures due to a suprious brace from a last
> minute edit to a comment that I forgot to test. This version fixes
> that, my applogies for the patch churn.
>
> I've updated this series based on Ævar's feedback on v3 (to paraphrase
> stop using '$_' so much and fix staging modified lines.). The first
> patch is functionally equivalent to the previous version but with a
> reworked implementation. Patch 2 is new, it implements correctly
> staging modified lines with a couple of limitations - see the commit
> message for more details, I'm keen to get some feedback on it. Patches
> 3 and 4 are essentially rebased and tweaked versions of patches 2 and
> 3 from the previous version.
>
> This series is based on pw/add-p-recount (f4d35a6b49 "add -p: fix
> counting empty context lines in edited patches")
>
> The motivation for this series is summed up in the first commit
> message:
>
> "When I end up editing hunks it is almost always because I want to
> stage a subset of the lines in the hunk. Doing this by editing the
> hunk is inconvenient and error prone (especially so if the patch is
> going to be reversed before being applied). Instead offer an option
> for add -p to stage individual lines. When the user presses 'l' the
> hunk is redrawn with labels by the insertions and deletions and they
> are prompted to enter a list of the lines they wish to stage. Ranges
> of lines may be specified using 'a-b' where either 'a' or 'b' may be
> omitted to mean all lines from 'a' to the end of the hunk or all lines
> from 1 upto and including 'b'."

I tested this with an eye towards what I pointed out in
https://public-inbox.org/git/878ta8vyqe.fsf@evledraar.gmail.com/

Using the same workflow (search for "So what I was expecting" in that
E-Mail) this now does the right thing in that example:

    select lines? 4,10
    [...]
    $ git diff --staged -U1
    diff --git a/README.md b/README.md
    index ff990622a3..6d16f7e52b 100644
    --- a/README.md
    +++ b/README.md
    @@ -20,3 +20,3 @@ See [Documentation/gittutorial.txt][] to get started, then see
     Documentation/git-<commandname>.txt for documentation of each command.
    -If git has been correctly installed, then the tutorial can also be
    +If Git has been correctly installed, then the tutorial can also be
     read with `man gittutorial` or `git help tutorial`, and the
    u git ((49703a4754...) $) $

Some other comments on this:

1) It needs to be more obvious how to exit this sub-mode, i.e. consider
this confused user:

    Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,l,?]? l
    select lines? ?
    invalid hunk line '?'
    select lines? q
    invalid hunk line 'q'
    select lines? exit
    invalid hunk line 'exit'
    select lines? quit
    invalid hunk line 'quit'
    select lines? :wq
    invalid hunk line ':wq'
    select lines? help
    invalid hunk line 'help'

Just doing Ctrl+D or RET exits it. Instead "?" should print some help
related to this sub-mode showing what the syntax is, and how to exit the
sub-mode. I think it would make sense for "q" to by synonymous with
"RET", i.e. you'd need "q<RET>q<RET>" to fully exit, but I don't know...

2) I think it's confusing UI that selecting some of the lines won't
re-present the hunk to you again in line mode, but I see this is
consistent with how e.g. "e" works, it won't re-present the hunk to you
if there's still something to do, you need to exit and run "git add -p"
again.

I think it makes sense to change that and you'd either "e" or "l" and
then "n" to proceed, or continue, but that's per-se unrelated to this
feature. Just something I ran into...

3) I don't see any way around this, but we need to carefully explain
that selecting a list of things in one session is *not* the same thing
as selecting them incrementally in multiple sessions. I.e. consider this
diff:

    @@ -1,3 +1,3 @@
    -a
    -b
    -c
    +1
    +2
    +3

If I select 1,4 I get, as expected:

    @@ -1,3 +1,3 @@
    -a
    +1
     b
     c

And then in the next session:

      @@ -1,3 +1,3 @@
       1
    1 -b
    2 -c
    3 +2
    4 +3
    select lines? 1,3

Yields, as expected:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +2
     c

But this is not the same as redoing the whole thing as:

    select lines? 1,4
    select lines? 1
    select lines? 3

Which instead yields:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
     c
    +3

Now, rummaging through my wetware and that E-Mail from back in March I
don't see how it could work differently, and you *also* want to be able
to select one line at a time like that.

Just something that's not very intuative / hard to explain, and maybe
there should be a different syntax (e.g. 1:4) for this "swap 1 for 4"
operation, as opposed to selecting lines 1 and 4 as they appear in the
diff.

4) With that abc 123 diff noted above, why am I in two sessions allowed
to do:

    @@ -1,3 +1,3 @@
    1 -a
    2 -b
    3 -c
    4 +1
    5 +2
    6 +3
    select lines? 1,4
    select lines? 1,4

To end up with this staged:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +3
     c

But not allowed to do the same thing in one operation via:

      @@ -1,3 +1,3 @@
    1 -a
    2 -b
    3 -c
    4 +1
    5 +2
    6 +3
    select lines? 1,4,2,6
    unable to pair up insertions and deletions

But I am allowed to do e.g.:

    select lines? 1,4,2,5

To end up with:

    @@ -1,3 +1,3 @@
    -a
    -b
    +1
    +2
     c

I can do this in two steps.

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason
@ 2018-08-03 10:01     ` Phillip Wood
  2018-08-03 16:51       ` Junio C Hamano
  2018-08-03 17:59       ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 53+ messages in thread
From: Phillip Wood @ 2018-08-03 10:01 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Phillip Wood
  Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano

Hi Ævar

Thanks for looking at this.

On 28/07/18 13:40, Ævar Arnfjörð Bjarmason wrote:
> 
> On Thu, Jul 26 2018, Phillip Wood wrote:
> 
>> Unfortuantely v4 had test failures due to a suprious brace from a last
>> minute edit to a comment that I forgot to test. This version fixes
>> that, my applogies for the patch churn.
>>
>> I've updated this series based on Ævar's feedback on v3 (to paraphrase
>> stop using '$_' so much and fix staging modified lines.). The first
>> patch is functionally equivalent to the previous version but with a
>> reworked implementation. Patch 2 is new, it implements correctly
>> staging modified lines with a couple of limitations - see the commit
>> message for more details, I'm keen to get some feedback on it. Patches
>> 3 and 4 are essentially rebased and tweaked versions of patches 2 and
>> 3 from the previous version.
>>
>> This series is based on pw/add-p-recount (f4d35a6b49 "add -p: fix
>> counting empty context lines in edited patches")
>>
>> The motivation for this series is summed up in the first commit
>> message:
>>
>> "When I end up editing hunks it is almost always because I want to
>> stage a subset of the lines in the hunk. Doing this by editing the
>> hunk is inconvenient and error prone (especially so if the patch is
>> going to be reversed before being applied). Instead offer an option
>> for add -p to stage individual lines. When the user presses 'l' the
>> hunk is redrawn with labels by the insertions and deletions and they
>> are prompted to enter a list of the lines they wish to stage. Ranges
>> of lines may be specified using 'a-b' where either 'a' or 'b' may be
>> omitted to mean all lines from 'a' to the end of the hunk or all lines
>> from 1 upto and including 'b'."
> 
> I tested this with an eye towards what I pointed out in
> https://public-inbox.org/git/878ta8vyqe.fsf@evledraar.gmail.com/
> 
> Using the same workflow (search for "So what I was expecting" in that
> E-Mail) this now does the right thing in that example:
> 
>     select lines? 4,10
>     [...]
>     $ git diff --staged -U1
>     diff --git a/README.md b/README.md
>     index ff990622a3..6d16f7e52b 100644
>     --- a/README.md
>     +++ b/README.md
>     @@ -20,3 +20,3 @@ See [Documentation/gittutorial.txt][] to get started, then see
>      Documentation/git-<commandname>.txt for documentation of each command.
>     -If git has been correctly installed, then the tutorial can also be
>     +If Git has been correctly installed, then the tutorial can also be
>      read with `man gittutorial` or `git help tutorial`, and the
>     u git ((49703a4754...) $) $
> 
> Some other comments on this:
> 
> 1) It needs to be more obvious how to exit this sub-mode, i.e. consider
> this confused user:
> 
>     Stage this hunk [y,n,q,a,d,/,j,J,g,s,e,l,?]? l
>     select lines? ?
>     invalid hunk line '?'
>     select lines? q
>     invalid hunk line 'q'
>     select lines? exit
>     invalid hunk line 'exit'
>     select lines? quit
>     invalid hunk line 'quit'
>     select lines? :wq
>     invalid hunk line ':wq'
>     select lines? help
>     invalid hunk line 'help'
> 
> Just doing Ctrl+D or RET exits it. Instead "?" should print some help
> related to this sub-mode showing what the syntax is, and how to exit the
> sub-mode. I think it would make sense for "q" to by synonymous with
> "RET", i.e. you'd need "q<RET>q<RET>" to fully exit, but I don't know...

Thanks for point this out I meant to add some help and then forgot. As
for exiting adding "q" could be a good idea, at the moment it follows
the behavior of the other sub-prompts of 'add -i' which isn't that user
friendly.

> 
> 2) I think it's confusing UI that selecting some of the lines won't
> re-present the hunk to you again in line mode, but I see this is
> consistent with how e.g. "e" works, it won't re-present the hunk to you
> if there's still something to do, you need to exit and run "git add -p"
> again.
> 
> I think it makes sense to change that and you'd either "e" or "l" and
> then "n" to proceed, or continue, but that's per-se unrelated to this
> feature. Just something I ran into...

That's something I thought about but in the end I didn't do it, maybe I
should look at it again as it makes it easier to get around the
limitations of the selection grouping. On the other hand if I've just
selected the lines I want it is convenient to move directly to the next
hunk without having to somehow quit the current one.

There are some questions about how the remaining lines of the hunk
should be presented - should they be broken up if you end up with a big
block of context in the middle after the first edit for instance.
There's also a practical problem of coloring the hunk if the user has
set interactive.diffFilter as it appears reading and writing to a
process is tricky on Windows. Part of the commit message of 01143847db
("add--interactive: allow custom diff highlighting programs", 2016-2-27)
reads

      2. add--interactive will re-colorize a diff which has been
         hand-edited, but it won't have run through the filter.
         Fixing this is conceptually easy (just pipe the diff
         through the filter), but practically hard to do without
         using tempfiles (it would need to feed data to and read
         the result from the filter without deadlocking; this
         raises portability questions with respect to Windows).

> 
> 3) I don't see any way around this, but we need to carefully explain
> that selecting a list of things in one session is *not* the same thing
> as selecting them incrementally in multiple sessions. I.e. consider this
> diff:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     -b
>     -c
>     +1
>     +2
>     +3
> 
> If I select 1,4 I get, as expected:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     +1
>      b
>      c
> 
> And then in the next session:
> 
>       @@ -1,3 +1,3 @@
>        1
>     1 -b
>     2 -c
>     3 +2
>     4 +3
>     select lines? 1,3
> 
> Yields, as expected:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     -b
>     +1
>     +2
>      c
> 
> But this is not the same as redoing the whole thing as:
> 
>     select lines? 1,4
>     select lines? 1
>     select lines? 3
> 
> Which instead yields:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     -b
>     +1
>      c
>     +3
> 
> Now, rummaging through my wetware and that E-Mail from back in March I
> don't see how it could work differently, and you *also* want to be able
> to select one line at a time like that.
> 
> Just something that's not very intuative / hard to explain, and maybe
> there should be a different syntax (e.g. 1:4) for this "swap 1 for 4"
> operation, as opposed to selecting lines 1 and 4 as they appear in the
> diff.

I did wonder about forcing the user to explicitly pair lines but shied
away from it as I assumed that is what would be wanted most of the time
and it is awkward for the user to have to pair them up and it involved
more error checking on the input to make sure they don't say 1:4 2:3.

> 
> 4) With that abc 123 diff noted above, why am I in two sessions allowed
> to do:
> 
>     @@ -1,3 +1,3 @@
>     1 -a
>     2 -b
>     3 -c
>     4 +1
>     5 +2
>     6 +3
>     select lines? 1,4
>     select lines? 1,4
> 
> To end up with this staged:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     -b
>     +1
>     +3
>      c
> 
> But not allowed to do the same thing in one operation via:
> 
>       @@ -1,3 +1,3 @@
>     1 -a
>     2 -b
>     3 -c
>     4 +1
>     5 +2
>     6 +3
>     select lines? 1,4,2,6
>     unable to pair up insertions and deletions
> 
> But I am allowed to do e.g.:
> 
>     select lines? 1,4,2,5
> 
> To end up with:
> 
>     @@ -1,3 +1,3 @@
>     -a
>     -b
>     +1
>     +2
>      c
> 
> I can do this in two steps.

Yes that needs explaining. To implement staging modified lines the code
needs to pair up each deleted line with its replacement. It does this by
grouping consecutive selected lines together, so it has a list of groups
of deleted lines and another list of inserted lines, it then pairs the
deletions and insertions by their index in the list. So in the last
example lines 1 and 2 are grouped together so there is a single group of
deletions and two groups of insertions. There is no way for it to tell
if lines 1 and 2 should be replaced by line 4 and line 5 should inserted
after 'c' or if 1 and 2 should be replaced by 4 and 5 with 'c' coming after.

It seems clear for your comment and Junio's that I need to improve the
documentation, I'm not sure if that will be enough though or do we need
to change the behavior? [I'm beginning to see why all the other programs
I tried while writing this (tig, gitg, gitk and mercurial's version of
add -i) don't make any attempt to stage modified lines correctly, though
I think git should have some way of doing it.]

Best Wishes

Phillip

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-08-03 10:01     ` Phillip Wood
@ 2018-08-03 16:51       ` Junio C Hamano
  2018-08-03 17:59       ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 53+ messages in thread
From: Junio C Hamano @ 2018-08-03 16:51 UTC (permalink / raw)
  To: Phillip Wood
  Cc: Ævar Arnfjörð Bjarmason, Phillip Wood,
	Git Mailing List, Igor Djordjevic

Phillip Wood <phillip.wood@talktalk.net> writes:

> ... [I'm beginning to see why all the other programs I tried while
> writing this (tig, gitg, gitk and mercurial's version of add -i)
> don't make any attempt to stage modified lines correctly, though I
> think git should have some way of doing it.]

Yes, this is a kind of feature that one can propose and implement
something that works well for some of the limited cases one uses,
but not in other cases, and it becomes very hard to explain how to
work around the implementation limitation---that is why I stopped
at "split this hunk?" and did not go beyond it when I designed the
original "incremental add" feature.

I think the real reason why it is hard is that there is no good
definition of "modified" in "stage modified lines", and worse, there
is no good way to mechanically figure it out, because a patch only
gives you "these were deleted" and "these are added", without giving
you "this line in the deleted block corresponds to these two lines
in the added block" (i.e. "this original one line was modified into
this thing in the result").

^ permalink raw reply	[flat|nested] 53+ messages in thread

* Re: [RFC PATCH v5 0/4] add -p: select individual hunk lines
  2018-08-03 10:01     ` Phillip Wood
  2018-08-03 16:51       ` Junio C Hamano
@ 2018-08-03 17:59       ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 53+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-08-03 17:59 UTC (permalink / raw)
  To: phillip.wood; +Cc: Git Mailing List, Igor Djordjevic, Junio C Hamano


On Fri, Aug 03 2018, Phillip Wood wrote:

[snip (all made sense)]

> It seems clear for your comment and Junio's that I need to improve the
> documentation, I'm not sure if that will be enough though or do we need
> to change the behavior? [I'm beginning to see why all the other programs
> I tried while writing this (tig, gitg, gitk and mercurial's version of
> add -i) don't make any attempt to stage modified lines correctly, though
> I think git should have some way of doing it.]

I think this is ready to move forward conceptually, i.e. in these two
review rounds I've been focusing on the UI this exposes, and whether
this is even a viable thing to have conceptually.

I'm convinced that it is, even though there's osme confusing edge cases
(as noted in my mail) I don't see how those could be entire avoided, a
feature like this will inherently have such edge cases.

I've avoided going deep into how the actual code itself works / is
implemented in these review rounds, figuring that given the topic it was
better to move past RFC for that type of review once we had docs.

Thanks for working on this.

^ permalink raw reply	[flat|nested] 53+ messages in thread

end of thread, other threads:[~2018-08-03 18:00 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-19 11:36 [PATCH v1 0/3] add -p: select individual hunk lines Phillip Wood
2018-02-19 11:36 ` [PATCH v1 1/3] " Phillip Wood
2018-02-19 11:36 ` [PATCH v1 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-02-19 11:36 ` [PATCH v1 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-02-19 12:20 ` [PATCH v1 0/3] add -p: select individual hunk lines Gustavo Leite
2018-03-06 10:17 ` [PATCH v2 " Phillip Wood
2018-03-06 10:17   ` [PATCH v2 1/3] " Phillip Wood
2018-03-06 20:29     ` Igor Djordjevic
2018-03-06 21:33       ` Igor Djordjevic
2018-03-06 10:17   ` [PATCH v2 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-03-06 19:57     ` Junio C Hamano
2018-03-08 11:05       ` Phillip Wood
2018-03-08 17:53         ` Junio C Hamano
2018-03-13 12:06           ` Phillip Wood
2018-03-13 16:32             ` Junio C Hamano
2018-03-14 11:02               ` Phillip Wood
2018-03-06 20:41     ` Igor Djordjevic
2018-03-06 10:17   ` [PATCH v2 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-03-06 20:33     ` Igor Djordjevic
2018-03-06 20:19   ` [PATCH v2 0/3] add -p: select individual hunk lines Igor Djordjevic
2018-03-06 21:03     ` Junio C Hamano
2018-03-06 21:20       ` Igor Djordjevic
2018-03-16 10:13 ` [PATCH v3 " Phillip Wood
2018-03-16 10:13   ` [PATCH v3 1/3] " Phillip Wood
2018-03-16 10:13   ` [PATCH v3 2/3] add -p: allow line selection to be inverted Phillip Wood
2018-03-16 10:13   ` [PATCH v3 3/3] add -p: optimize line selection for short hunks Phillip Wood
2018-03-29 18:32   ` [PATCH v3 0/3] add -p: select individual hunk lines Junio C Hamano
2018-03-30 11:09     ` Phillip Wood
2018-03-31 19:20       ` Ævar Arnfjörð Bjarmason
2018-04-02 10:55         ` Phillip Wood
2018-04-02 11:39           ` Ævar Arnfjörð Bjarmason
2018-07-26 10:22 ` [RFC PATCH v4 0/4] " Phillip Wood
2018-07-26 10:22   ` [PATCH v4 1/4] " Phillip Wood
2018-07-26 10:22   ` [RFC PATCH v4 2/4] add -p: select modified lines correctly Phillip Wood
2018-07-26 10:22   ` [PATCH v4 3/4] add -p: allow line selection to be inverted Phillip Wood
2018-07-26 10:22   ` [PATCH v4 4/4] add -p: optimize line selection for short hunks Phillip Wood
2018-07-26 15:58 ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Phillip Wood
2018-07-26 15:58   ` [PATCH v5 1/4] " Phillip Wood
2018-07-26 19:36     ` Junio C Hamano
2018-07-27 10:05       ` Phillip Wood
2018-07-27 16:09         ` Junio C Hamano
2018-07-26 15:58   ` [RFC PATCH v5 2/4] add -p: select modified lines correctly Phillip Wood
2018-07-26 19:30     ` Junio C Hamano
2018-07-27 10:19       ` Phillip Wood
2018-07-27 16:14         ` Junio C Hamano
2018-07-26 15:58   ` [PATCH v5 3/4] add -p: allow line selection to be inverted Phillip Wood
2018-07-26 15:58   ` [PATCH v5 4/4] add -p: optimize line selection for short hunks Phillip Wood
2018-07-27 18:27   ` [RFC PATCH v5 0/4] add -p: select individual hunk lines Ævar Arnfjörð Bjarmason
2018-07-28 10:08     ` Phillip Wood
2018-07-28 12:40   ` Ævar Arnfjörð Bjarmason
2018-08-03 10:01     ` Phillip Wood
2018-08-03 16:51       ` Junio C Hamano
2018-08-03 17:59       ` Ævar Arnfjörð Bjarmason

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.