{"thread":{"id":"12889","subject":"[PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","startedAt":"2008-03-27T07:30:43Z","lastAt":"2008-03-27T22:16:17Z","messageCount":7,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"73192","messageId":"20080327073043.GB22444@coredump.intra.peff.net","threadId":"12889","inReplyTo":"cover.1206602393.git.peff@peff.net","subject":"[PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-27T07:30:43Z","receivedAt":"2008-03-27T07:30:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a path is examined in the patch subcommand, any mode\nchanges in the file are given to use in the diff header by\ngit-diff. If no hunks are staged, then we throw out that\nheader. But if _any_ hunks are staged, we use the header,\nand the mode is changed.\n\nSince the 'p'atch command should just be dealing with hunks\nthat are shown to the user, it makes sense to just ignore\nmode changes entirely. We do squirrel away the mode, though,\nsince a future patch will allow users to select the mode\nupdate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nLike I said, I don't necessarily like this behavior, but it is a\nstepping stone to 1/2.\n\n git-add--interactive.perl  |   16 ++++++++++++++++\n t/t3701-add-interactive.sh |    9 +++++++++\n 2 files changed, 25 insertions(+), 0 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex a0a81f1..5cdda29 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -550,6 +550,21 @@ sub parse_diff {\n \treturn @hunk;\n }\n \n+sub parse_diff_header {\n+\tmy $src = shift;\n+\n+\tmy $head = { TEXT => [], DISPLAY => [] };\n+\tmy $mode = { TEXT => [], DISPLAY => [] };\n+\n+\tfor (my $i = 0; $i < @{$src->{TEXT}}; $i++) {\n+\t\tmy $dest = $src->{TEXT}->[$i] =~ /^(old|new) mode (\\d+)$/ ?\n+\t\t\t$mode : $head;\n+\t\tpush @{$dest->{TEXT}}, $src->{TEXT}->[$i];\n+\t\tpush @{$dest->{DISPLAY}}, $src->{DISPLAY}->[$i];\n+\t}\n+\treturn ($head, $mode);\n+}\n+\n sub hunk_splittable {\n \tmy ($text) = @_;\n \n@@ -795,6 +810,7 @@ sub patch_update_file {\n \tmy ($ix, $num);\n \tmy $path = shift;\n \tmy ($head, @hunk) = parse_diff($path);\n+\t($head, my $mode) = parse_diff_header($head);\n \tfor (@{$head->{DISPLAY}}) {\n \t\tprint;\n \t}\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 77c90f6..d920d06 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -66,4 +66,13 @@ test_expect_success 'revert works (commit)' '\n \tgrep \"unchanged *+3/-0 file\" output\n '\n \n+test_expect_success 'patch does not affect mode' '\n+\tgit reset --hard &&\n+\techo content >>file &&\n+\tchmod +x file &&\n+\tprintf \"y\\\\n\" | git add -p &&\n+\tgit show :file | grep content &&\n+\tgit diff file | grep \"new mode\"\n+'\n+\n test_done\n-- \n1.5.5.rc1.141.g50ecd.dirty\n"},{"id":"73193","messageId":"20080327073225.GC22444@coredump.intra.peff.net","threadId":"12889","inReplyTo":"cover.1206602393.git.peff@peff.net","subject":"[PATCH 2/2] add--interactive: allow user to choose mode update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-27T07:32:25Z","receivedAt":"2008-03-27T07:32:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When using the 'p'atch command, instead of just throwing out\nany mode change, let's present it to the user in the same\nway that we show hunks.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is the \"minimal intrusion\" implementation. It always asks about the\nmode just once at the beginning of the file. I think you could probably\nrefactor the hunk handling so that the mode change went into the main\nhunk loop, and you could skip it, go back to it, etc, like a regular\nhunk.\n\n git-add--interactive.perl  |   33 +++++++++++++++++++++++++++++++++\n t/t3701-add-interactive.sh |   12 +++++++++++-\n 2 files changed, 44 insertions(+), 1 deletions(-)\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 5cdda29..903953e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -814,6 +814,36 @@ sub patch_update_file {\n \tfor (@{$head->{DISPLAY}}) {\n \t\tprint;\n \t}\n+\n+\tif (@{$mode->{TEXT}}) {\n+\t\twhile (1) {\n+\t\t\tprint @{$mode->{DISPLAY}};\n+\t\t\tprint colored $prompt_color,\n+\t\t\t\t\"Stage mode change [y/n/a/d/?]? \";\n+\t\t\tmy $line = <STDIN>;\n+\t\t\tif ($line =~ /^y/i) {\n+\t\t\t\t$mode->{USE} = 1;\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t\telsif ($line =~ /^n/i) {\n+\t\t\t\t$mode->{USE} = 0;\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t\telsif ($line =~ /^a/i) {\n+\t\t\t\t$_->{USE} = 1 foreach ($mode, @hunk);\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t\telsif ($line =~ /^d/i) {\n+\t\t\t\t$_->{USE} = 0 foreach ($mode, @hunk);\n+\t\t\t\tlast;\n+\t\t\t}\n+\t\t\telse {\n+\t\t\t\thelp_patch_cmd('');\n+\t\t\t\tnext;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n \t$num = scalar @hunk;\n \t$ix = 0;\n \n@@ -936,6 +966,9 @@ sub patch_update_file {\n \n \tmy $n_lofs = 0;\n \tmy @result = ();\n+\tif ($mode->{USE}) {\n+\t\tpush @result, @{$mode->{TEXT}};\n+\t}\n \tfor (@hunk) {\n \t\tmy $text = $_->{TEXT};\n \t\tmy ($o_ofs, $o_cnt, $n_ofs, $n_cnt) =\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex d920d06..f15be93 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -70,9 +70,19 @@ test_expect_success 'patch does not affect mode' '\n \tgit reset --hard &&\n \techo content >>file &&\n \tchmod +x file &&\n-\tprintf \"y\\\\n\" | git add -p &&\n+\tprintf \"n\\\\ny\\\\n\" | git add -p &&\n \tgit show :file | grep content &&\n \tgit diff file | grep \"new mode\"\n '\n \n+test_expect_success 'stage mode but not hunk' '\n+\tgit reset --hard &&\n+\techo content >>file &&\n+\tchmod +x file &&\n+\tprintf \"y\\\\nn\\\\n\" | git add -p &&\n+\tgit diff --cached file | grep \"new mode\" &&\n+\tgit diff          file | grep \"+content\"\n+'\n+\n+\n test_done\n-- \n1.5.5.rc1.141.g50ecd.dirty\n"},{"id":"73205","messageId":"7v7ifob0et.fsf@gitster.siamese.dyndns.org","threadId":"12889","inReplyTo":"20080327073043.GB22444@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-27T16:24:10Z","receivedAt":"2008-03-27T16:24:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> When a path is examined in the patch subcommand, any mode\n> changes in the file are given to use in the diff header by\n> git-diff. If no hunks are staged, then we throw out that\n> header. But if _any_ hunks are staged, we use the header,\n> and the mode is changed.\n>\n> Since the 'p'atch command should just be dealing with hunks\n> that are shown to the user, it makes sense to just ignore\n> mode changes entirely. We do squirrel away the mode, though,\n> since a future patch will allow users to select the mode\n> update.\n\nI agree that treating each logical block of the metainformation as if it\nis a hunk on its own makes sense.  Possibly, it would be useful to make\nthe interactive patch session look like this:\n\n    diff --git a/gostak b/doshes\n    old mode 100644\n    new mode 100755\n    Stage the mode change [y/n/a/d/j/J/?]?\n    similarity index 90%\n    rename from gostak\n    rename to doshes\n    Stage the name change [y/n/a/d/j/J/?]?\n    @@ -1,5 +1,5 @@\n     The\n    -gostak\n    +Gostak\n     distims\n     doshes\n    Stage this hunk [y/n/a/d/j/J/?]?\n\nHandlilng the rename needs a bit more thought and enhancements not just in\nyour parse_diff_header() code but the way we extract the diff text (we\nwould need to first find renames by whole-tree diff and then feed two\n(src,dst) paths in parse_diff() to obtain the text), but it should be\ndoable.\n\nBy the way, why was it done as a new sub called from parse_diff() and not\nas a part of parse_diff() itself?\n"},{"id":"73209","messageId":"20080327171022.GA27189@coredump.intra.peff.net","threadId":"12889","inReplyTo":"7v7ifob0et.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-27T17:10:23Z","receivedAt":"2008-03-27T17:10:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2008 at 09:24:10AM -0700, Junio C Hamano wrote:\n\n>     similarity index 90%\n>     rename from gostak\n>     rename to doshes\n>     Stage the name change [y/n/a/d/j/J/?]?\n\nI hadn't thought about renames. But I wonder if it really makes sense in\nthe context of a single path. If I have content in \"doshes\", what does\nit matter at this point that it came from \"gostak\"? IOW, what does\nsaying 'y' here really do? What is the workflow around it?\n\n> By the way, why was it done as a new sub called from parse_diff() and not\n> as a part of parse_diff() itself?\n\nCode clarity.\n\nThe parsing code seemed less convoluted to me that way. parse_diff is\nabout linearly splitting the input into hunks. The first hunk just\nhappens to be \"everything before the first patch hunk\".\n\nBut splitting head versus mode requires non-linear parsing. Doing it in\nparse_diff would require adding some state to the loop. It seemed\nmore readable to me to compose it from two simple loops rather than one\nmore complex loop.\n\nI can do it the other way if you prefer.\n\n-Peff\n"},{"id":"73211","messageId":"7vprtg9g0p.fsf@gitster.siamese.dyndns.org","threadId":"12889","inReplyTo":"20080327171022.GA27189@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-27T18:29:58Z","receivedAt":"2008-03-27T18:29:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Mar 27, 2008 at 09:24:10AM -0700, Junio C Hamano wrote:\n>\n>>     similarity index 90%\n>>     rename from gostak\n>>     rename to doshes\n>>     Stage the name change [y/n/a/d/j/J/?]?\n>\n> I hadn't thought about renames. But I wonder if it really makes sense in\n> the context of a single path.\n\nYeah, but the user is really into microcommits, like \"separate mode\nchange\" thing really matters, maybe the user would want to make three\ncommits (1) chmod +x, (2) pure rename, and (3) content changes.\n\nI personally think that is not worth it, so I am agreeing with you on the\n\"rename\" one.\n\nEven though your two patches make perfect sense at the philosophical level\nand I very much like it, I doubt \"separating mode change\" is so useful\nfrom the practical point of view for that matter.\n\nI would even imagine that, if we did not have \"add -p\" before, and the\nvery initial implementation of it had started from the behaviour your two\npatches bring in, and then a patch came to make it the current \"not asking\nabout mode change separately and if the user chooses to add anything from\nthe patch hunks, stage the mode change along with it\" behaviour, people\nmight even think that such a patch is an improvement in usability by\nasking one less question.  I dunno.\n\n>> By the way, why was it done as a new sub called from parse_diff() and not\n>> as a part of parse_diff() itself?\n>\n> Code clarity.\n\nThat I'd agree with very much.  Thanks for clarificaiton.\n"},{"id":"73215","messageId":"20080327193147.GA10034@sigill.intra.peff.net","threadId":"12889","inReplyTo":"7vprtg9g0p.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-27T19:31:47Z","receivedAt":"2008-03-27T19:31:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2008 at 11:29:58AM -0700, Junio C Hamano wrote:\n\n> Yeah, but the user is really into microcommits, like \"separate mode\n> change\" thing really matters, maybe the user would want to make three\n> commits (1) chmod +x, (2) pure rename, and (3) content changes.\n> \n> I personally think that is not worth it, so I am agreeing with you on the\n> \"rename\" one.\n\nMy feeling on the rename microcommit is that it is reasonable (though I\nam not such a microcommitter), but that \"git add -p\" is probably not the\nright tool for doing it. My view is that mode change and hunks are\n_actual_ changes to the file, and you can pick and choose the changes\nyou have made. \"rename\" is not a change you made, but rather something\nwe infer from the changes that are available.\n\nBut I can also see how one has the opposite view. I dunno. It's hard to\nspeculate since I don't actually want to _use_ rename. ;)\n\n> Even though your two patches make perfect sense at the philosophical level\n> and I very much like it, I doubt \"separating mode change\" is so useful\n> from the practical point of view for that matter.\n> [...]\n> ...then a patch came to make it the current \"not asking\n> about mode change separately and if the user chooses to add anything from\n> the patch hunks, stage the mode change along with it\" behaviour, people\n> might even think that such a patch is an improvement in usability by\n> asking one less question.  I dunno.\n\nI don't think I would probably use mode change very often, but I found\nthe current behavior quite non-intuitive, and I think I would prefer if\nit were explicit (and in 99% of cases, it won't come up at all, since\nyou haven't changed the mode!).\n\nBut this is all clearly post-1.5.5, so hopefully we can let it stew and\nget some more comments from the list on what makes sense to people.\n\n-Peff\n"},{"id":"73221","messageId":"20080327221616.GA28179@coredump.intra.peff.net","threadId":"12889","inReplyTo":"m3fxucuesq.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] add--interactive: ignore mode change in 'p'atch command","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2008-03-27T22:16:17Z","receivedAt":"2008-03-27T22:16:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2008 at 12:51:04PM -0700, Jakub Narebski wrote:\n\n> Well, one hypothetical situation where having mode-change as 0th\n> pseudo-hunk in git-add--interactive would be useful is the situation\n> where you have changed file mode by accident, and do not want it\n> staged (included in the commit), at all.\n\nFWIW, the diff \"header\" that \"git add -p\" gives you now does mention the\nmode change. But it's very easy to miss, since you are presumably\nlooking for the hunk text. Asking is much nicer, IMHO.\n\n-Peff\n"}]}