[PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index a0a81f1..5cdda29 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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;
 	}
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 77c90f6..d920d06 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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

Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

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?

Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Jeff King]

On Thu, Mar 27, 2008 at 09:24:10AM -0700, Junio C Hamano wrote:

>     similarity index 90%
>     rename from gostak
>     rename to doshes
>     Stage the name change [y/n/a/d/j/J/?]?

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?

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

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

Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 27, 2008 at 09:24:10AM -0700, Junio C Hamano wrote:
>
>>     similarity index 90%
>>     rename from gostak
>>     rename to doshes
>>     Stage the name change [y/n/a/d/j/J/?]?
>
> I hadn't thought about renames. But I wonder if it really makes sense in
> the context of a single path.

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.

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

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

Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Jeff King]

On Thu, Mar 27, 2008 at 11:29:58AM -0700, Junio C Hamano wrote:

> 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.

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. ;)

> 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.
> [...]
> ...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.

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

Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command

[Jeff King]

On Thu, Mar 27, 2008 at 12:51:04PM -0700, Jakub Narebski wrote:

> Well, one hypothetical situation where having mode-change as 0th
> pseudo-hunk in git-add--interactive would be useful is the situation
> where you have changed file mode by accident, and do not want it
> staged (included in the commit), at all.

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

[PATCH 2/2] add--interactive: allow user to choose mode update

[Jeff King]

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.

Signed-off-by: Jeff King <peff@peff.net>
---
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(-)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 5cdda29..903953e 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -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) =
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index d920d06..f15be93 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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