Re: [PATCH] git-add--interactive: manual hunk editing mode

7 messages, 3 authors, 2016-06-15 · open the first message on its own page

Re: [PATCH] git-add--interactive: manual hunk editing mode

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:44:52

Thomas Rast [off-list ref] writes:
quoted hunk
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..6bb117a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -2,6 +2,7 @@
 
 use strict;
 use Git;
+use File::Temp;
People with minimum Perl installation should still be able to use "add -i"
as long as they do not use 'e' subcommand, shouldn't they?  Shouldn't we
do something like:

	my $can_use_temp = eval {
        	require File::Temp;
                1;
	};

and disable 'e' subcommand unless $can_use_temp?
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $t = File::Temp->new(
+		TEMPLATE => $repo->repo_path . "/git-hunk-edit.XXXXXX",
+		SUFFIX => '.diff'
+	);
+	print $t "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $t @$oldtext;
+	print $t <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
Don't you want to say "Do not touch lines that begin with ' '"?
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
Hmm, perhaps not even giving the "@@ ... @@" lines to the editor would be
a more robust solution?
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --recount --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
Have to wonder where the potential error message would go, and if it would
confuse the end users...
quoted hunk
@@ -1002,7 +1123,8 @@ sub patch_update_file {
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached';
+		open $fh, '| git apply --cached'
+			. ($need_recount ? ' --recount' : '');
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
I recall that the original "add--interactive" carefully counted numbers in
hunks it reassembles (as it can let you split and then you can choose to
use both parts, which requires it to merge overlapping hunks back), but if
you are going to use --recount anyway, perhaps we can discard that logic?
It may make the patch application less robust, though.  I dunno.

An alternative, and probably more robust, approach would be to recount
what we have in @{$mode->{TEXT}}, after letting the user edit some of
them, so that "add--interactive" still knows what it is doing after
applying your patch without having to rely on "apply --recount".

Re: [PATCH] git-add--interactive: manual hunk editing mode

From: Thomas Rast <hidden>
Date: 2016-06-15 22:44:52

Junio C Hamano wrote:
People with minimum Perl installation should still be able to use "add -i"
as long as they do not use 'e' subcommand, shouldn't they?  Shouldn't we
do something like:
[...]
and disable 'e' subcommand unless $can_use_temp?
I can just call the temporary editing file something constant, like
the commit message already is.  I don't care much about that point,
and honestly assumed File::Temp came with every Perl (it's in
perl-base here).
quoted
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
Don't you want to say "Do not touch lines that begin with ' '"?
Why?  You can make them '-' instead if you really want to :-)
quoted
+	# Reinsert the first hunk header if the user accidentally deleted it
Hmm, perhaps not even giving the "@@ ... @@" lines to the editor would be
a more robust solution?
I originally left it there because Emacs automatically recounts the
header, and it also reminds the user of the function the hunk applies
to.
quoted
+	open $fh, '| git apply --recount --cached --check';
Have to wonder where the potential error message would go, and if it would
confuse the end users...
To the terminal.  If we eat that error message, the user gets
absolutely no indication as to _what_ might be wrong with the edited
patch, so I kind of deliberately left it there.

Then again the message from git-apply is not exactly transparent
either.
I recall that the original "add--interactive" carefully counted numbers in
hunks it reassembles (as it can let you split and then you can choose to
use both parts, which requires it to merge overlapping hunks back), but if
you are going to use --recount anyway, perhaps we can discard that logic?
We briefly talked about that[1], but Jeff thought it should be a
separate patch, and I agree.  Can't sneakily rip out two dozen lines
of otherwise unrelated code in my feature patch, can I?
It may make the patch application less robust, though.  I dunno.
I've become convinced it can't, apart from making it less likely to
trip over bugs in the script itself of course.
An alternative, and probably more robust, approach would be to recount
what we have in @{$mode->{TEXT}}, after letting the user edit some of
them, so that "add--interactive" still knows what it is doing after
applying your patch without having to rely on "apply --recount".
You lost me here.  Why recount the mode part?

If you mean the _hunk_ headers, that's what the first three versions
of this patch did, at the expense of a lot of code complication (and
now that git-apply implements --recount, actually _duplication_).

- Thomas


[1] http://article.gmane.org/gmane.comp.version-control.git/84698

-- 
Thomas Rast
trast@student.ethz.ch

Re: [PATCH] git-add--interactive: manual hunk editing mode

From: Jeff King <hidden>
Date: 2016-06-15 22:44:52

On Tue, Jul 01, 2008 at 10:39:58PM -0700, Junio C Hamano wrote:
quoted
+use File::Temp;
People with minimum Perl installation should still be able to use "add -i"
as long as they do not use 'e' subcommand, shouldn't they?  Shouldn't we
do something like:

	my $can_use_temp = eval {
        	require File::Temp;
                1;
	};

and disable 'e' subcommand unless $can_use_temp?
According to Module::CoreList, File::Temp has shipped as part of core
perl since 5.006001. "add -i" doesn't work with perl < 5.6 already due to
things like 3-argument open.

So if the problem is "old perl", I don't think it is an issue. Are there
modern perl installations in the wild that don't have File::Temp?

-Peff

[PATCH 0/3] git-add--interactive: use --recount, editing

From: Thomas Rast <hidden>
Date: 2016-06-15 22:44:52

Junio C Hamano wrote:
I recall that the original "add--interactive" carefully counted numbers in
hunks it reassembles (as it can let you split and then you can choose to
use both parts, which requires it to merge overlapping hunks back), but if
you are going to use --recount anyway, perhaps we can discard that logic?
It may make the patch application less robust, though.  I dunno.
This series takes it a bit further.  I played around with 'apply', and
it seems there is no reason to even merge the hunks.  (It would be
great if someone who knows builtin-apply.c could confirm this.)  So we
can get rid of all recounting except for the correct splitting
boundaries.  These are the first two patches.

Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
I wonder why bother trying to import things when you do not need them to 
begin with!  I mean, it is _obvious_ that in this case, we want .git/ to 
be writable _anyway_, so why not stick with a fixed name in that?
Good suggestion -- I love that simplicity.  Thomas?
Well, changed that back.

Apart from that, no real changes to 3/3, but the $needs_recount code
has become unnecessary because 1/3 already forces --recount.

- Thomas



 Documentation/git-add.txt  |    1 +
 git-add--interactive.perl  |  203 ++++++++++++++++++++++---------------------
 t/t3701-add-interactive.sh |   67 +++++++++++++++
 3 files changed, 172 insertions(+), 99 deletions(-)

[PATCH 1/3] git-add--interactive: replace hunk recounting with apply --recount

From: Thomas Rast <hidden>
Date: 2016-06-15 22:44:52

We recounted the postimage offsets to compensate for hunks that were
not selected.  Now apply --recount can do the job for us.

Signed-off-by: Thomas Rast <redacted>
---
 git-add--interactive.perl |   30 +++---------------------------
 1 files changed, 3 insertions(+), 27 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 903953e..e1964a5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -970,39 +970,15 @@ sub patch_update_file {
 		push @result, @{$mode->{TEXT}};
 	}
 	for (@hunk) {
-		my $text = $_->{TEXT};
-		my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =
-		    parse_hunk_header($text->[0]);
-
-		if (!$_->{USE}) {
-			# We would have added ($n_cnt - $o_cnt) lines
-			# to the postimage if we were to use this hunk,
-			# but we didn't.  So the line number that the next
-			# hunk starts at would be shifted by that much.
-			$n_lofs -= ($n_cnt - $o_cnt);
-			next;
-		}
-		else {
-			if ($n_lofs) {
-				$n_ofs += $n_lofs;
-				$text->[0] = ("@@ -$o_ofs" .
-					      (($o_cnt != 1)
-					       ? ",$o_cnt" : '') .
-					      " +$n_ofs" .
-					      (($n_cnt != 1)
-					       ? ",$n_cnt" : '') .
-					      " @@\n");
-			}
-			for (@$text) {
-				push @result, $_;
-			}
+		if ($_->{USE}) {
+			push @result, @{$_->{TEXT}};
 		}
 	}
 
 	if (@result) {
 		my $fh;
 
-		open $fh, '| git apply --cached';
+		open $fh, '| git apply --cached --recount';
 		for (@{$head->{TEXT}}, @result) {
 			print $fh $_;
 		}
-- 
1.5.6.1.276.gde9a

[PATCH 2/3] git-add--interactive: remove hunk coalescing

From: Thomas Rast <hidden>
Date: 2016-06-15 22:44:52

Current git-apply has no trouble at all applying chunks that have
overlapping context, as produced by the splitting feature. So we can
drop the manual coalescing.

Signed-off-by: Thomas Rast <redacted>
---
 git-add--interactive.perl |   89 ---------------------------------------------
 1 files changed, 0 insertions(+), 89 deletions(-)
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index e1964a5..a4234d3 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -682,93 +682,6 @@ sub split_hunk {
 	return @split;
 }
 
-sub find_last_o_ctx {
-	my ($it) = @_;
-	my $text = $it->{TEXT};
-	my ($o_ofs, $o_cnt) = parse_hunk_header($text->[0]);
-	my $i = @{$text};
-	my $last_o_ctx = $o_ofs + $o_cnt;
-	while (0 < --$i) {
-		my $line = $text->[$i];
-		if ($line =~ /^ /) {
-			$last_o_ctx--;
-			next;
-		}
-		last;
-	}
-	return $last_o_ctx;
-}
-
-sub merge_hunk {
-	my ($prev, $this) = @_;
-	my ($o0_ofs, $o0_cnt, $n0_ofs, $n0_cnt) =
-	    parse_hunk_header($prev->{TEXT}[0]);
-	my ($o1_ofs, $o1_cnt, $n1_ofs, $n1_cnt) =
-	    parse_hunk_header($this->{TEXT}[0]);
-
-	my (@line, $i, $ofs, $o_cnt, $n_cnt);
-	$ofs = $o0_ofs;
-	$o_cnt = $n_cnt = 0;
-	for ($i = 1; $i < @{$prev->{TEXT}}; $i++) {
-		my $line = $prev->{TEXT}[$i];
-		if ($line =~ /^\+/) {
-			$n_cnt++;
-			push @line, $line;
-			next;
-		}
-
-		last if ($o1_ofs <= $ofs);
-
-		$o_cnt++;
-		$ofs++;
-		if ($line =~ /^ /) {
-			$n_cnt++;
-		}
-		push @line, $line;
-	}
-
-	for ($i = 1; $i < @{$this->{TEXT}}; $i++) {
-		my $line = $this->{TEXT}[$i];
-		if ($line =~ /^\+/) {
-			$n_cnt++;
-			push @line, $line;
-			next;
-		}
-		$ofs++;
-		$o_cnt++;
-		if ($line =~ /^ /) {
-			$n_cnt++;
-		}
-		push @line, $line;
-	}
-	my $head = ("@@ -$o0_ofs" .
-		    (($o_cnt != 1) ? ",$o_cnt" : '') .
-		    " +$n0_ofs" .
-		    (($n_cnt != 1) ? ",$n_cnt" : '') .
-		    " @@\n");
-	@{$prev->{TEXT}} = ($head, @line);
-}
-
-sub coalesce_overlapping_hunks {
-	my (@in) = @_;
-	my @out = ();
-
-	my ($last_o_ctx);
-
-	for (grep { $_->{USE} } @in) {
-		my $text = $_->{TEXT};
-		my ($o_ofs) = parse_hunk_header($text->[0]);
-		if (defined $last_o_ctx &&
-		    $o_ofs <= $last_o_ctx) {
-			merge_hunk($out[-1], $_);
-		}
-		else {
-			push @out, $_;
-		}
-		$last_o_ctx = find_last_o_ctx($out[-1]);
-	}
-	return @out;
-}
 
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
@@ -962,8 +875,6 @@ sub patch_update_file {
 		}
 	}
 
-	@hunk = coalesce_overlapping_hunks(@hunk);
-
 	my $n_lofs = 0;
 	my @result = ();
 	if ($mode->{USE}) {
-- 
1.5.6.1.276.gde9a

[PATCH 3/3] git-add--interactive: manual hunk editing mode

From: Thomas Rast <hidden>
Date: 2016-06-15 22:44:52

Adds a new option 'e' to the 'add -p' command loop that lets you edit
the current hunk in your favourite editor.

If the resulting patch applies cleanly, the edited hunk will
immediately be marked for staging. If it does not apply cleanly, you
will be given an opportunity to edit again. If all lines of the hunk
are removed, then the edit is aborted and the hunk is left unchanged.

Applying the changed hunk(s) relies on Johannes Schindelin's new
--recount option for git-apply.

Note that the "real patch" test intentionally uses
  (echo e; echo n; echo d) | git add -p
even though the 'n' and 'd' are superfluous at first sight.  They
serve to get out of the interaction loop if git add -p wrongly
concludes the patch does not apply.

Many thanks to Jeff King [off-list ref] for lots of help and
suggestions.

Signed-off-by: Thomas Rast <redacted>
---
 Documentation/git-add.txt  |    1 +
 git-add--interactive.perl  |  119 ++++++++++++++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |   67 +++++++++++++++++++++++++
 3 files changed, 187 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 011a743..46dd56c 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -236,6 +236,7 @@ patch::
        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
 +
 After deciding the fate for all hunks, if there is any hunk
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a4234d3..801d7c0 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -18,6 +18,18 @@ my ($fraginfo_color) =
 	$diff_use_color ? (
 		$repo->get_color('color.diff.frag', 'cyan'),
 	) : ();
+my ($diff_plain_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.plain', ''),
+	) : ();
+my ($diff_old_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.old', 'red'),
+	) : ();
+my ($diff_new_color) =
+	$diff_use_color ? (
+		$repo->get_color('color.diff.new', 'green'),
+	) : ();
 
 my $normal_color = $repo->get_color("", "reset");
 
@@ -683,6 +695,105 @@ sub split_hunk {
 }
 
 
+sub color_diff {
+	return map {
+		colored((/^@/  ? $fraginfo_color :
+			 /^\+/ ? $diff_new_color :
+			 /^-/  ? $diff_old_color :
+			 $diff_plain_color),
+			$_);
+	} @_;
+}
+
+sub edit_hunk_manually {
+	my ($oldtext) = @_;
+
+	my $hunkfile = $repo->repo_path . "/addp-hunk-edit.diff";
+	my $fh;
+	open $fh, '>', $hunkfile
+		or die "failed to open hunk edit file for writing: " . $!;
+	print $fh "# Manual hunk edit mode -- see bottom for a quick guide\n";
+	print $fh @$oldtext;
+	print $fh <<EOF;
+# ---
+# To remove '-' lines, make them ' ' lines (context).
+# To remove '+' lines, delete them.
+# Lines starting with # will be removed.
+#
+# If the patch applies cleanly, the edited hunk will immediately be
+# marked for staging. If it does not apply cleanly, you will be given
+# an opportunity to edit again. If all lines of the hunk are removed,
+# then the edit is aborted and the hunk is left unchanged.
+EOF
+	close $fh;
+
+	my $editor = $ENV{GIT_EDITOR} || $repo->config("core.editor")
+		|| $ENV{VISUAL} || $ENV{EDITOR} || "vi";
+	system('sh', '-c', $editor.' "$@"', $editor, $hunkfile);
+
+	open $fh, '<', $hunkfile
+		or die "failed to open hunk edit file for reading: " . $!;
+	my @newtext = grep { !/^#/ } <$fh>;
+	close $fh;
+	unlink $hunkfile;
+
+	# Abort if nothing remains
+	if (!grep { /\S/ } @newtext) {
+		return undef;
+	}
+
+	# Reinsert the first hunk header if the user accidentally deleted it
+	if ($newtext[0] !~ /^@/) {
+		unshift @newtext, $oldtext->[0];
+	}
+	return \@newtext;
+}
+
+sub diff_applies {
+	my $fh;
+	open $fh, '| git apply --recount --cached --check';
+	for my $h (@_) {
+		print $fh @{$h->{TEXT}};
+	}
+	return close $fh;
+}
+
+sub prompt_yesno {
+	my ($prompt) = @_;
+	while (1) {
+		print colored $prompt_color, $prompt;
+		my $line = <STDIN>;
+		return 0 if $line =~ /^n/i;
+		return 1 if $line =~ /^y/i;
+	}
+}
+
+sub edit_hunk_loop {
+	my ($head, $hunk, $ix) = @_;
+	my $text = $hunk->[$ix]->{TEXT};
+
+	while (1) {
+		$text = edit_hunk_manually($text);
+		if (!defined $text) {
+			return undef;
+		}
+		my $newhunk = { TEXT => $text, USE => 1 };
+		if (diff_applies($head,
+				 @{$hunk}[0..$ix-1],
+				 $newhunk,
+				 @{$hunk}[$ix+1..$#{$hunk}])) {
+			$newhunk->{DISPLAY} = [color_diff(@{$text})];
+			return $newhunk;
+		}
+		else {
+			prompt_yesno(
+				'Your edited hunk does not apply. Edit again '
+				. '(saying "no" discards!) [y/n]? '
+				) or return undef;
+		}
+	}
+}
+
 sub help_patch_cmd {
 	print colored $help_color, <<\EOF ;
 y - stage this hunk
@@ -694,6 +805,7 @@ 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
 EOF
 }
@@ -798,6 +910,7 @@ sub patch_update_file {
 		if (hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= '/s';
 		}
+		$other .= '/e';
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
@@ -862,6 +975,12 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
+			elsif ($line =~ /^e/) {
+				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
+				if (defined $newhunk) {
+					splice @hunk, $ix, 1, $newhunk;
+				}
+			}
 			else {
 				help_patch_cmd($other);
 				next;
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index fae64ea..e95663d 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -66,6 +66,73 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+cat >expected <<EOF
+EOF
+cat >fake_editor.sh <<EOF
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'dummy edit works' '
+	(echo e; echo a) | git add -p &&
+	git diff > diff &&
+	test_cmp expected diff
+'
+
+cat >patch <<EOF
+@@ -1,1 +1,4 @@
+ this
++patch
+-doesn't
+ apply
+EOF
+echo "#!$SHELL_PATH" >fake_editor.sh
+cat >>fake_editor.sh <<\EOF
+mv -f "$1" oldpatch &&
+mv -f patch "$1"
+EOF
+chmod a+x fake_editor.sh
+test_set_editor "$(pwd)/fake_editor.sh"
+test_expect_success 'bad edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+this patch
+is garbage
+EOF
+test_expect_success 'garbage edit rejected' '
+	git reset &&
+	(echo e; echo n; echo d) | git add -p >output &&
+	grep "hunk does not apply" output
+'
+
+cat >patch <<EOF
+@@ -1,0 +1,0 @@
+ baseline
++content
++newcontent
++lines
+EOF
+cat >expected <<EOF
+diff --git a/file b/file
+index b5dd6c9..f910ae9 100644
+--- a/file
++++ b/file
+@@ -1,4 +1,4 @@
+ baseline
+ content
+-newcontent
++more
+ lines
+EOF
+test_expect_success 'real edit works' '
+	(echo e; echo n; echo d) | git add -p &&
+	git diff >output &&
+	test_cmp expected output
+'
+
 if test "$(git config --bool core.filemode)" = false
 then
     say 'skipping filemode tests (filesystem does not properly support modes)'
-- 
1.5.6.1.276.gde9a
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help