add--interactive: ignore mode change in 'p'atch command

When a path is examined in the patch subcommand, any mode changes in the file are given to use in the diff header by git-diff. If no hunks are staged, then we throw out that header. But if _any_ hunks are staged, we use the header, and the mode is changed.

Since the 'p'atch command should just be dealing with hunks that are shown to the user, it makes sense to just ignore mode changes entirely. We do squirrel away the mode, though, since a future patch will allow users to select the mode update.

---
Like I said, I don't necessarily like this behavior, but it is a stepping stone to 1/2.

 git-add--interactive.perl  |   16 ++++++++++++++++
 t/t3701-add-interactive.sh |    9 +++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

index a0a81f1..5cdda29 100755
@@ -550,6 +550,21 @@ sub parse_diff {
 	return @hunk;
 }
 
+sub parse_diff_header {
+	my $src = shift;
+
+	my $head = { TEXT => [], DISPLAY => [] };
+	my $mode = { TEXT => [], DISPLAY => [] };
+
+	for (my $i = 0; $i < @{$src->{TEXT}}; $i++) {
+		my $dest = $src->{TEXT}->[$i] =~ /^(old|new) mode (\d+)$/ ?
+			$mode : $head;
+		push @{$dest->{TEXT}}, $src->{TEXT}->[$i];
+		push @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];
+	}
+	return ($head, $mode);
+}
+
 sub hunk_splittable {
 	my ($text) = @_;
 

@@ -795,6 +810,7 @@ sub patch_update_file {
 	my ($ix, $num);
 	my $path = shift;
 	my ($head, @hunk) = parse_diff($path);
+	($head, my $mode) = parse_diff_header($head);
 	for (@{$head->{DISPLAY}}) {
 		print;
 	}

index 77c90f6..d920d06 100755
@@ -66,4 +66,13 @@ test_expect_success 'revert works (commit)' '
 	grep "unchanged *+3/-0 file" output
 '
 
+test_expect_success 'patch does not affect mode' '
+	git reset --hard &&
+	echo content >>file &&
+	chmod +x file &&
+	printf "y\\n" | git add -p &&
+	git show :file | grep content &&
+	git diff file | grep "new mode"
+'
+
 test_done

1.5.5.rc1.141.g50ecd.dirty

I agree that treating each logical block of the metainformation as if it is a hunk on its own makes sense. Possibly, it would be useful to make the interactive patch session look like this:

    diff --git a/gostak b/doshes
    old mode 100644
    new mode 100755
    Stage the mode change [y/n/a/d/j/J/?]?
    similarity index 90%
    rename from gostak
    rename to doshes
    Stage the name change [y/n/a/d/j/J/?]?
    @@ -1,5 +1,5 @@
     The
    -gostak
    +Gostak
     distims
     doshes
    Stage this hunk [y/n/a/d/j/J/?]?

Handlilng the rename needs a bit more thought and enhancements not just in your parse_diff_header() code but the way we extract the diff text (we would need to first find renames by whole-tree diff and then feed two (src,dst) paths in parse_diff() to obtain the text), but it should be doable.

By the way, why was it done as a new sub called from parse_diff() and not as a part of parse_diff() itself?

I hadn't thought about renames. But I wonder if it really makes sense in the context of a single path. If I have content in "doshes", what does it matter at this point that it came from "gostak"? IOW, what does saying 'y' here really do? What is the workflow around it?

Code clarity.

The parsing code seemed less convoluted to me that way. parse_diff is about linearly splitting the input into hunks. The first hunk just happens to be "everything before the first patch hunk".

But splitting head versus mode requires non-linear parsing. Doing it in parse_diff would require adding some state to the loop. It seemed more readable to me to compose it from two simple loops rather than one more complex loop.

I can do it the other way if you prefer.

-Peff

Yeah, but the user is really into microcommits, like "separate mode change" thing really matters, maybe the user would want to make three commits (1) chmod +x, (2) pure rename, and (3) content changes.

I personally think that is not worth it, so I am agreeing with you on the "rename" one.

Even though your two patches make perfect sense at the philosophical level and I very much like it, I doubt "separating mode change" is so useful from the practical point of view for that matter.

I would even imagine that, if we did not have "add -p" before, and the very initial implementation of it had started from the behaviour your two patches bring in, and then a patch came to make it the current "not asking about mode change separately and if the user chooses to add anything from the patch hunks, stage the mode change along with it" behaviour, people might even think that such a patch is an improvement in usability by asking one less question. I dunno.

That I'd agree with very much. Thanks for clarificaiton.

My feeling on the rename microcommit is that it is reasonable (though I am not such a microcommitter), but that "git add -p" is probably not the right tool for doing it. My view is that mode change and hunks are _actual_ changes to the file, and you can pick and choose the changes you have made. "rename" is not a change you made, but rather something we infer from the changes that are available.

But I can also see how one has the opposite view. I dunno. It's hard to speculate since I don't actually want to _use_ rename. ;)

I don't think I would probably use mode change very often, but I found the current behavior quite non-intuitive, and I think I would prefer if it were explicit (and in 99% of cases, it won't come up at all, since you haven't changed the mode!).

But this is all clearly post-1.5.5, so hopefully we can let it stew and get some more comments from the list on what makes sense to people.

-Peff

FWIW, the diff "header" that "git add -p" gives you now does mention the mode change. But it's very easy to miss, since you are presumably looking for the hunk text. Asking is much nicer, IMHO.

-Peff

When using the 'p'atch command, instead of just throwing out any mode change, let's present it to the user in the same way that we show hunks.

---
This is the "minimal intrusion" implementation. It always asks about the mode just once at the beginning of the file. I think you could probably refactor the hunk handling so that the mode change went into the main hunk loop, and you could skip it, go back to it, etc, like a regular hunk.

 git-add--interactive.perl  |   33 +++++++++++++++++++++++++++++++++
 t/t3701-add-interactive.sh |   12 +++++++++++-
 2 files changed, 44 insertions(+), 1 deletions(-)

index 5cdda29..903953e 100755
@@ -814,6 +814,36 @@ sub patch_update_file {
 	for (@{$head->{DISPLAY}}) {
 		print;
 	}
+
+	if (@{$mode->{TEXT}}) {
+		while (1) {
+			print @{$mode->{DISPLAY}};
+			print colored $prompt_color,
+				"Stage mode change [y/n/a/d/?]? ";
+			my $line = <STDIN>;
+			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;
+			}
+		}
+	}
+
 	$num = scalar @hunk;
 	$ix = 0;
 

@@ -936,6 +966,9 @@ sub patch_update_file {
 
 	my $n_lofs = 0;
 	my @result = ();
+	if ($mode->{USE}) {
+		push @result, @{$mode->{TEXT}};
+	}
 	for (@hunk) {
 		my $text = $_->{TEXT};
 		my ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =

index d920d06..f15be93 100755
@@ -70,9 +70,19 @@ test_expect_success 'patch does not affect mode' '
 	git reset --hard &&
 	echo content >>file &&
 	chmod +x file &&
-	printf "y\\n" | git add -p &&
+	printf "n\\ny\\n" | git add -p &&
 	git show :file | grep content &&
 	git diff file | grep "new mode"
 '
 
+test_expect_success 'stage mode but not hunk' '
+	git reset --hard &&
+	echo content >>file &&
+	chmod +x file &&
+	printf "y\\nn\\n" | git add -p &&
+	git diff --cached file | grep "new mode" &&
+	git diff          file | grep "+content"
+'
+
+
 test_done

1.5.5.rc1.141.g50ecd.dirty