Re: [PATCH v2 1/3] add -p: select individual hunk lines

[Igor Djordjevic <igor.d.djordjevic@gmail.com>]

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
>