threads / patch / 12889

patch, 2 partsadd--interactive: ignore mode change in 'p'atch command

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

## tl;dr

7 messages between Mar 27, 2008 and Mar 27, 2008. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Jeff King· Mar 27, 2008, 07:30 UTC · lore

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(-)
Show changes to 2 files +25 −0

git-add--interactive.perl, t/t3701-add-interactive.sh

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
Junio C Hamano· Mar 27, 2008, 16:24 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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?

Jeff King· Mar 27, 2008, 17:10 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Mar 27, 2008, 18:29 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.
Jeff King· Mar 27, 2008, 19:31 UTC · re: Junio C Hamano · lore

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

On Thu, Mar 27, 2008 at 11:29:58AM -0700, Junio C Hamano wrote:
Show 6 quoted lines
> 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. ;)

Show 9 quoted lines
> 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
Jeff King· Mar 27, 2008, 07:32 UTC · lore

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

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(-)
Show changes to 2 files +44 −1

git-add--interactive.perl, t/t3701-add-interactive.sh

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
Jeff King· Mar 27, 2008, 22:16 UTC · lore

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

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

← back to recent threads