Re: [RFC PATCH] git add -p: new "quit" command at the prompt.

Matthieu Moy [off-list ref] writes:

Junio C Hamano [off-list ref] writes:

quoted

Matthieu Moy [off-list ref] writes:

quoted

There's already 'd' to stop staging hunks in a file, but no command to
stop the interactive staging (for the current files and the remaining
ones). This patch implements this functionality, and binds it to 'q'.
---

I'm not familiar at all with the code in git-add--interactive.perl, so
my code is mostly cut-and-pasted+adapted from the 'd' command.

You can say 'd' and then ^C, I think.

Yes, you /can/, and that's what I'm doing right now in this situation.
But that's undocumented, not so intuitive (I found out I could do that
after trying ^C alone, which doesn't work, staged content is recorded
on disk at the end of the file only, not after each prompt), ...

I thought the situation was common enough to deserve an explicit
command. The 'd' command is natural for "git add -i" + patch
subcommand, but for "git add -p", I found 'd' mostly useless, and I
really want a "quit" command.

Sure, I can live without it, but if other people would like to have
it, please speak now ;-).

I think everybody agree that the intention of the patch is good (well,
several pro, and no real counter-argument). I'll send an updated
version with documentation soon, but I'd appreciate review and
comments on the code.

I'm not really happy with the fact that I mainly cut-and-pasted code
from the "d" command, but OTOH, that's already how the code is today
(huge if/elsif/... with similar elsif blocks for each command).

-- 
Matthieu

Matthieu Moy [off-list ref] writes:

I think everybody agree that the intention of the patch is good (well,
several pro, and no real counter-argument).

Oh, I think we have already passed that state long time ago.  It's queued
as c9cc8d9 ([NEEDS SIGN OFF, DOC, AND REVIEW] git add -p: new "quit"
command at the prompt., 2009-04-10) in 'pu'.

I agree that there is a "Huh?" factor in the repeated and similar hunks to
patch_update_file, but the first hunk is dealing with the mode change and
the second one is about the patch text.

Currently parse_diff_header() returns the mode line into $mode and the
main loop treats $mode differently from @hunk, which is an array of hunks
parsed by parse_diff().  Treating $mode as a "fake hunk" by unshifting it
at the beginning of @hunk array and teaching the main loop minor details
such as $mode "fake hunk" cannot be edited nor split, I suspect we _could_
unify the two.

ca72468 (add--interactive: allow user to choose mode update, 2008-03-27)
introduced this duplication.  Jeff, what do you think?  I am not sure if
it is worth it.

On Wed, Apr 15, 2009 at 04:25:55PM -0700, Junio C Hamano wrote:

Currently parse_diff_header() returns the mode line into $mode and the
main loop treats $mode differently from @hunk, which is an array of hunks
parsed by parse_diff().  Treating $mode as a "fake hunk" by unshifting it
at the beginning of @hunk array and teaching the main loop minor details
such as $mode "fake hunk" cannot be edited nor split, I suspect we _could_
unify the two.

ca72468 (add--interactive: allow user to choose mode update, 2008-03-27)
introduced this duplication.  Jeff, what do you think?  I am not sure if
it is worth it.

Yeah, at least at one point I thought that was possible:

  http://article.gmane.org/gmane.comp.version-control.git/78340

Let me see how painful it would be.

-Peff

On Thu, Apr 16, 2009 at 02:00:27AM -0400, Jeff King wrote:

quoted

Currently parse_diff_header() returns the mode line into $mode and the
main loop treats $mode differently from @hunk, which is an array of hunks
parsed by parse_diff().  Treating $mode as a "fake hunk" by unshifting it
at the beginning of @hunk array and teaching the main loop minor details
such as $mode "fake hunk" cannot be edited nor split, I suspect we _could_
unify the two.

Let me see how painful it would be.

Not too painful at all. We can _almost_ get away with a really minimal
"just unshift it" patch, but calling splittable_hunk() generates some
perl warnings (because the hunk lacks @@ lines), and doing a hunk edit
seems to confuse the code (it duplicates the "old mode" line for some
reason). It is not worth trying to fix, though, as editing the mode hunk
is not useful in practice (see the commit message below).

It would also be possible to refactor the header parsing to just pull
the mode lines out as we are parsing the hunks. However, it is probably
not worth the added complexity: you have header lines, then some mode
lines, then some more header lines, then some hunks. So you have to keep
more state around to do a linear parse.

Anyway, I think this is a nice improvement on its own, and it should
make Matthieu's patch a little cleaner.

-- >8 --
Subject: [PATCH] add-interactive: refactor mode hunk handling

The original implementation considered the mode separately
from the rest of the hunks, asking about it outside the main
hunk-selection loop. This patch instead places a mode change
as the first hunk in the loop. This has two advantages:

  1. less duplicated code (since we use the main selection
     loop). This also cleans up an inconsistency, which is
     that the main selection loop separates options with a
     comma, whereas the mode prompt used slashes.

  2. users can now skip the mode change and come back to it,
     search for it (via "/mode"), etc, as they can with other
     hunks.

To facilitate this, each hunk is now marked with a "type".
Mode hunks are not considered for splitting (which would
make no sense, and also confuses the split_hunk function),
nor are they editable. In theory, one could edit the mode
lines and change to an entirely new mode. In practice, there
are only two modes that git cares about (0644 and 0755), so
either you want to move from one to the other or not (and
you can do that by staging or not staging).

Signed-off-by: Jeff King <redacted>
---
 git-add--interactive.perl |   59 ++++++++++++++++-----------------------------
 1 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index def062a..b895e3b 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl

@@ -620,11 +620,12 @@ sub parse_diff {
 	if ($diff_use_color) {
 		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
 	}
-	my (@hunk) = { TEXT => [], DISPLAY => [] };
+	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
 	for (my $i = 0; $i < @diff; $i++) {
 		if ($diff[$i] =~ /^@@ /) {
-			push @hunk, { TEXT => [], DISPLAY => [] };
+			push @hunk, { TEXT => [], DISPLAY => [],
+				TYPE => 'hunk' };
 		}
 		push @{$hunk[-1]{TEXT}}, $diff[$i];
 		push @{$hunk[-1]{DISPLAY}},

@@ -636,8 +637,8 @@ sub parse_diff {
 sub parse_diff_header {
 	my $src = shift;
 
-	my $head = { TEXT => [], DISPLAY => [] };
-	my $mode = { TEXT => [], DISPLAY => [] };
+	my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
+	my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
 
 	for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
 		my $dest = $src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ?

@@ -684,6 +685,7 @@ sub split_hunk {
 		my $this = +{
 			TEXT => [],
 			DISPLAY => [],
+			TYPE => 'hunk',
 			OLD => $o_ofs,
 			NEW => $n_ofs,
 			OCNT => 0,

@@ -873,7 +875,11 @@ sub edit_hunk_loop {
 		if (!defined $text) {
 			return undef;
 		}
-		my $newhunk = { TEXT => $text, USE => 1 };
+		my $newhunk = {
+			TEXT => $text,
+			TYPE => $hunk->[$ix]->{TYPE},
+			USE => 1
+		};
 		if (diff_applies($head,
 				 @{$hunk}[0..$ix-1],
 				 $newhunk,

@@ -985,32 +991,7 @@ sub patch_update_file {
 	}
 
 	if (@{$mode->{TEXT}}) {
-		while (1) {
-			print @{$mode->{DISPLAY}};
-			print colored $prompt_color,
-				"Stage mode change [y/n/a/d/?]? ";
-			my $line = prompt_single_character;
-			if ($line =~ /^y/i) {
-				$mode->{USE} = 1;
-				last;
-			}
-			elsif ($line =~ /^n/i) {
-				$mode->{USE} = 0;
-				last;
-			}
-			elsif ($line =~ /^a/i) {
-				$_->{USE} = 1 foreach ($mode, @hunk);
-				last;
-			}
-			elsif ($line =~ /^d/i) {
-				$_->{USE} = 0 foreach ($mode, @hunk);
-				last;
-			}
-			else {
-				help_patch_cmd('');
-				next;
-			}
-		}
+		unshift @hunk, $mode;
 	}
 
 	$num = scalar @hunk;

@@ -1054,14 +1035,19 @@ sub patch_update_file {
 		}
 		last if (!$undecided);
 
-		if (hunk_splittable($hunk[$ix]{TEXT})) {
+		if ($hunk[$ix]{TYPE} eq 'hunk' &&
+		    hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= ',s';
 		}
-		$other .= ',e';
+		if ($hunk[$ix]{TYPE} eq 'hunk') {
+			$other .= ',e';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? ";
+		print colored $prompt_color, 'Stage ',
+		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		  " [y,n,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {

@@ -1193,7 +1179,7 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
-			elsif ($line =~ /^e/) {
+			elsif ($other =~ /e/ && $line =~ /^e/) {
 				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
 				if (defined $newhunk) {
 					splice @hunk, $ix, 1, $newhunk;

@@ -1214,9 +1200,6 @@ sub patch_update_file {
 
 	my $n_lofs = 0;
 	my @result = ();
-	if ($mode->{USE}) {
-		push @result, @{$mode->{TEXT}};
-	}
 	for (@hunk) {
 		if ($_->{USE}) {
 			push @result, @{$_->{TEXT}};

-- 
1.6.3.rc0.157.g02b29.dirty

On Thu, Apr 16, 2009 at 02:52:23AM -0400, Jeff King wrote:

Anyway, I think this is a nice improvement on its own, and it should
make Matthieu's patch a little cleaner.

Hmm, it looks like you just applied Matthieu's patch to next already.
Here is the rebased version of mine (the conflict resolution was pretty
trivial, though: just delete the newly added 'q' option from the mode
loop, which no longer exists).

-- >8 --
Subject: [PATCH] add-interactive: refactor mode hunk handling

The original implementation considered the mode separately
from the rest of the hunks, asking about it outside the main
hunk-selection loop. This patch instead places a mode change
as the first hunk in the loop. This has two advantages:

  1. less duplicated code (since we use the main selection
     loop). This also cleans up an inconsistency, which is
     that the main selection loop separates options with a
     comma, whereas the mode prompt used slashes.

  2. users can now skip the mode change and come back to it,
     search for it (via "/mode"), etc, as they can with other
     hunks.

To facilitate this, each hunk is now marked with a "type".
Mode hunks are not considered for splitting (which would
make no sense, and also confuses the split_hunk function),
nor are they editable. In theory, one could edit the mode
lines and change to a new mode. In practice, there are only
two modes that git cares about (0644 and 0755), so either
you want to move from one to the other or not (and you can
do that by staging or not staging).

Signed-off-by: Jeff King <redacted>
---
 git-add--interactive.perl |   64 ++++++++++++++------------------------------
 1 files changed, 21 insertions(+), 43 deletions(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 210d230..60dd1b5 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl

@@ -620,11 +620,12 @@ sub parse_diff {
 	if ($diff_use_color) {
 		@colored = run_cmd_pipe(qw(git diff-files -p --color --), $path);
 	}
-	my (@hunk) = { TEXT => [], DISPLAY => [] };
+	my (@hunk) = { TEXT => [], DISPLAY => [], TYPE => 'header' };
 
 	for (my $i = 0; $i < @diff; $i++) {
 		if ($diff[$i] =~ /^@@ /) {
-			push @hunk, { TEXT => [], DISPLAY => [] };
+			push @hunk, { TEXT => [], DISPLAY => [],
+				TYPE => 'hunk' };
 		}
 		push @{$hunk[-1]{TEXT}}, $diff[$i];
 		push @{$hunk[-1]{DISPLAY}},

@@ -636,8 +637,8 @@ sub parse_diff {
 sub parse_diff_header {
 	my $src = shift;
 
-	my $head = { TEXT => [], DISPLAY => [] };
-	my $mode = { TEXT => [], DISPLAY => [] };
+	my $head = { TEXT => [], DISPLAY => [], TYPE => 'header' };
+	my $mode = { TEXT => [], DISPLAY => [], TYPE => 'mode' };
 
 	for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
 		my $dest = $src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ?

@@ -684,6 +685,7 @@ sub split_hunk {
 		my $this = +{
 			TEXT => [],
 			DISPLAY => [],
+			TYPE => 'hunk',
 			OLD => $o_ofs,
 			NEW => $n_ofs,
 			OCNT => 0,

@@ -873,7 +875,11 @@ sub edit_hunk_loop {
 		if (!defined $text) {
 			return undef;
 		}
-		my $newhunk = { TEXT => $text, USE => 1 };
+		my $newhunk = {
+			TEXT => $text,
+			TYPE => $hunk->[$ix]->{TYPE},
+			USE => 1
+		};
 		if (diff_applies($head,
 				 @{$hunk}[0..$ix-1],
 				 $newhunk,

@@ -987,37 +993,7 @@ sub patch_update_file {
 	}
 
 	if (@{$mode->{TEXT}}) {
-		while (1) {
-			print @{$mode->{DISPLAY}};
-			print colored $prompt_color,
-				"Stage mode change [y/n/a/d/?]? ";
-			my $line = prompt_single_character;
-			if ($line =~ /^y/i) {
-				$mode->{USE} = 1;
-				last;
-			}
-			elsif ($line =~ /^n/i) {
-				$mode->{USE} = 0;
-				last;
-			}
-			elsif ($line =~ /^a/i) {
-				$_->{USE} = 1 foreach ($mode, @hunk);
-				last;
-			}
-			elsif ($line =~ /^d/i) {
-				$_->{USE} = 0 foreach ($mode, @hunk);
-				last;
-			}
-			elsif ($line =~ /^q/i) {
-				$_->{USE} = 0 foreach ($mode, @hunk);
-				$quit = 1;
-				last;
-			}
-			else {
-				help_patch_cmd('');
-				next;
-			}
-		}
+		unshift @hunk, $mode;
 	}
 
 	$num = scalar @hunk;

@@ -1061,14 +1037,19 @@ sub patch_update_file {
 		}
 		last if (!$undecided);
 
-		if (hunk_splittable($hunk[$ix]{TEXT})) {
+		if ($hunk[$ix]{TYPE} eq 'hunk' &&
+		    hunk_splittable($hunk[$ix]{TEXT})) {
 			$other .= ',s';
 		}
-		$other .= ',e';
+		if ($hunk[$ix]{TYPE} eq 'hunk') {
+			$other .= ',e';
+		}
 		for (@{$hunk[$ix]{DISPLAY}}) {
 			print;
 		}
-		print colored $prompt_color, "Stage this hunk [y,n,a,d,/$other,?]? ";
+		print colored $prompt_color, 'Stage ',
+		  ($hunk[$ix]{TYPE} eq 'mode' ? 'mode change' : 'this hunk'),
+		  " [y,n,a,d,/$other,?]? ";
 		my $line = prompt_single_character;
 		if ($line) {
 			if ($line =~ /^y/i) {

@@ -1210,7 +1191,7 @@ sub patch_update_file {
 				$num = scalar @hunk;
 				next;
 			}
-			elsif ($line =~ /^e/) {
+			elsif ($other =~ /e/ && $line =~ /^e/) {
 				my $newhunk = edit_hunk_loop($head, \@hunk, $ix);
 				if (defined $newhunk) {
 					splice @hunk, $ix, 1, $newhunk;

@@ -1231,9 +1212,6 @@ sub patch_update_file {
 
 	my $n_lofs = 0;
 	my @result = ();
-	if ($mode->{USE}) {
-		push @result, @{$mode->{TEXT}};
-	}
 	for (@hunk) {
 		if ($_->{USE}) {
 			push @result, @{$_->{TEXT}};

-- 
1.6.3.rc0.204.g6bb2