{"thread":{"id":"21676","subject":"[RFC PATCH] {checkout,reset} -p: make patch direction configurable","startedAt":"2009-11-19T22:03:57Z","lastAt":"2009-11-27T06:41:07Z","messageCount":3,"participants":["Thomas Rast","Štěpán Němec","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"127933","messageId":"527e9296b638eb4c9993b3fb0d1c6f51b64f4c2c.1258667920.git.trast@student.ethz.ch","threadId":"21676","inReplyTo":null,"subject":"[RFC PATCH] {checkout,reset} -p: make patch direction configurable","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2009-11-19T22:03:57Z","receivedAt":"2009-11-19T22:03:57Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"When we implemented the -p mode for checkout, reset and stash, some\ndiscussion revolved around the involved patch direction.\n\nMake this configurable for reset and checkout with the following\nchoices:\n\n             index/HEAD       other\n  forward    undo addition    undo addition\n  mixed      undo addition    apply removal\n  reverse    apply removal    apply removal\n\nWhere for added lines, 'undo addition' means that you will see a '+'\npatch and can reverse-apply it; 'apply removal' means you will see a\n'-' patch and can forward-apply it.\n\nThe default is 'mixed', to keep the behaviour consistent with that\nfrom before the patch.\n\nSigned-off-by: Thomas Rast <trast@student.ethz.ch>\n---\n\nThis patch is much easier to read with --color-words.\n\nISTR that Peff wanted this, and maybe some others.  I'm not too\ninterested because I'm still convinced 'mixed' is the Right Option,\nbut it was somewhere deep on my todo stack and maybe you like it ;-)\n\nWe could do the same for stash, but I don't really see the use there;\nit does not have any of the direction-switching issues because it does\nnot support a rev argument.\n\n Documentation/config.txt  |   12 +++++\n git-add--interactive.perl |  100 +++++++++++++++++++++++++++++++-------------\n 2 files changed, 82 insertions(+), 30 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex cb73d75..f5f9b80 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1146,6 +1146,18 @@ interactive.singlekey::\n \tlinkgit:git-add[1].  Note that this setting is silently\n \tignored if portable keystroke input is not available.\n \n+interactive.reset.direction::\n+interactive.checkout.direction::\n+\tDirection of diffs used in `git reset -p` and `git checkout\n+\t-p`.  Must be one of 'forward', 'mixed' (default), or\n+\t'reverse'.\n++\n+With 'forward', diffs are taken forward and applied in reverse, i.e.,\n+if you added a block of code you see an addition patch and are asked\n+if you want to remove it.  'reverse' instead shows a removal patch\n+and asks if you to apply it.  'mixed' is the same as 'forward' when\n+comparing to HEAD or the index, and 'reverse' otherwise.\n+\n log.date::\n \tSet default date-time mode for the log command. Setting log.date\n \tvalue is similar to using 'git-log'\\'s --date option. The value is one of the\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 8ce1ec9..051e47e 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -74,6 +74,25 @@\n my $patch_mode;\n my $patch_mode_revision;\n \n+sub get_patch_direction_config {\n+\tmy ($key, $default, $forward, $reverse) = @_;\n+\tmy $value = $repo->config($key, $default) || $default;\n+\tif ($value =~ /^forward$/i) {\n+\t\treturn (0, 0);\n+\t} elsif ($value =~ /^mixed$/i) {\n+\t\treturn (0, 1);\n+\t} elsif ($value =~ /^reverse?/i) {\n+\t\treturn (1, 1);\n+\t} else {\n+\t\tdie \"$key must be one of 'forward', 'mixed' or 'reverse'\";\n+\t}\n+}\n+\n+my ($reset_reverse_head, $reset_reverse_nothead)\n+\t= get_patch_direction_config(\"interactive.reset.direction\", \"mixed\");\n+my ($checkout_reverse_head, $checkout_reverse_nothead)\n+\t= get_patch_direction_config(\"interactive.checkout.direction\", \"mixed\");\n+\n sub apply_patch;\n sub apply_patch_for_checkout_commit;\n sub apply_patch_for_stash;\n@@ -98,48 +117,69 @@\n \t\tFILTER => undef,\n \t},\n \t'reset_head' => {\n-\t\tDIFF => 'diff-index -p --cached',\n-\t\tAPPLY => sub { apply_patch 'apply -R --cached', @_; },\n-\t\tAPPLY_CHECK => 'apply -R --cached',\n-\t\tVERB => 'Unstage',\n-\t\tTARGET => '',\n-\t\tPARTICIPLE => 'unstaging',\n+\t\tDIFF => 'diff-index -p --cached '\n+\t\t\t. ($reset_reverse_head ? '-R' : ''),\n+\t\tAPPLY => sub {\n+\t\t\tapply_patch 'apply --cached '\n+\t\t\t\t. ($reset_reverse_head ? '' : '-R'), @_;\n+\t\t},\n+\t\tAPPLY_CHECK => 'apply --cached '\n+\t\t\t. ($reset_reverse_head ? '' : '-R'),\n+\t\tVERB => $reset_reverse_head ? 'Apply' : 'Unstage',\n+\t\tTARGET => $reset_reverse_head ? ' to index' : '',\n+\t\tPARTICIPLE => $reset_reverse_head ? 'applying' : 'unstaging',\n \t\tFILTER => 'index-only',\n \t},\n \t'reset_nothead' => {\n-\t\tDIFF => 'diff-index -R -p --cached',\n-\t\tAPPLY => sub { apply_patch 'apply --cached', @_; },\n-\t\tAPPLY_CHECK => 'apply --cached',\n-\t\tVERB => 'Apply',\n-\t\tTARGET => ' to index',\n-\t\tPARTICIPLE => 'applying',\n+\t\tDIFF => 'diff-index -p --cached '\n+\t\t\t. ($reset_reverse_nothead ? '-R' : ''),\n+\t\tAPPLY => sub {\n+\t\t\tapply_patch 'apply --cached '\n+\t\t\t\t. ($reset_reverse_nothead ? '' : '-R'), @_;\n+\t\t},\n+\t\tAPPLY_CHECK => 'apply --cached '\n+\t\t\t. ($reset_reverse_nothead ? '' : '-R'),\n+\t\tVERB => $reset_reverse_nothead ? 'Apply' : 'Unstage',\n+\t\tTARGET => $reset_reverse_nothead ? ' to index' : '',\n+\t\tPARTICIPLE => $reset_reverse_nothead ? 'applying' : 'unstaging',\n \t\tFILTER => 'index-only',\n \t},\n \t'checkout_index' => {\n-\t\tDIFF => 'diff-files -p',\n-\t\tAPPLY => sub { apply_patch 'apply -R', @_; },\n-\t\tAPPLY_CHECK => 'apply -R',\n-\t\tVERB => 'Discard',\n-\t\tTARGET => ' from worktree',\n-\t\tPARTICIPLE => 'discarding',\n+\t\tDIFF => 'diff-files -p ' . ($checkout_reverse_head ? '-R' : ''),\n+\t\tAPPLY => sub {\n+\t\t\tapply_patch 'apply '\n+\t\t\t\t. ($checkout_reverse_head ? '' : '-R'), @_;\n+\t\t},\n+\t\tAPPLY_CHECK => 'apply ' . ($checkout_reverse_head ? '' : '-R'),\n+\t\tVERB => $checkout_reverse_head ? 'Apply' : 'Discard',\n+\t\tTARGET => $checkout_reverse_head ? ' to worktree' : ' from worktree',\n+\t\tPARTICIPLE => $checkout_reverse_head ? 'applying' : 'discarding',\n \t\tFILTER => 'file-only',\n \t},\n \t'checkout_head' => {\n-\t\tDIFF => 'diff-index -p',\n-\t\tAPPLY => sub { apply_patch_for_checkout_commit '-R', @_ },\n-\t\tAPPLY_CHECK => 'apply -R',\n-\t\tVERB => 'Discard',\n-\t\tTARGET => ' from index and worktree',\n-\t\tPARTICIPLE => 'discarding',\n+\t\tDIFF => 'diff-index -p ' . ($checkout_reverse_head ? '-R' : ''),\n+\t\tAPPLY => sub {\n+\t\t\tapply_patch_for_checkout_commit\n+\t\t\t\t$checkout_reverse_head ? '' : '-R', @_;\n+\t\t},\n+\t\tAPPLY_CHECK => 'apply ' . ($checkout_reverse_head ? '' : '-R'),\n+\t\tVERB => $checkout_reverse_head ? 'Apply' : 'Discard',\n+\t\tTARGET => ($checkout_reverse_head ? ' to index and worktree'\n+\t\t\t   : ' from index and worktree'),\n+\t\tPARTICIPLE => $checkout_reverse_head ? 'applying' : 'discarding',\n \t\tFILTER => undef,\n \t},\n \t'checkout_nothead' => {\n-\t\tDIFF => 'diff-index -R -p',\n-\t\tAPPLY => sub { apply_patch_for_checkout_commit '', @_ },\n-\t\tAPPLY_CHECK => 'apply',\n-\t\tVERB => 'Apply',\n-\t\tTARGET => ' to index and worktree',\n-\t\tPARTICIPLE => 'applying',\n+\t\tDIFF => 'diff-index -p ' . ($checkout_reverse_nothead ? '-R' : ''),\n+\t\tAPPLY => sub {\n+\t\t\tapply_patch_for_checkout_commit\n+\t\t\t\t$checkout_reverse_nothead ? '' : '-R', @_;\n+\t\t},\n+\t\tAPPLY_CHECK => 'apply ' . ($checkout_reverse_nothead ? '' : '-R'),\n+\t\tVERB => $checkout_reverse_nothead ? 'Apply' : 'Discard',\n+\t\tTARGET => ($checkout_reverse_nothead ? ' to index and worktree'\n+\t\t\t   : ' from index and worktree'),\n+\t\tPARTICIPLE => $checkout_reverse_nothead ? 'applying' : 'discarding',\n \t\tFILTER => undef,\n \t},\n );\n-- \n1.6.5.3.439.g7a64a6.dirty\n"},{"id":"127992","messageId":"20091120100800.GA26162@headley","threadId":"21676","inReplyTo":"527e9296b638eb4c9993b3fb0d1c6f51b64f4c2c.1258667920.git.trast@student.ethz.ch","subject":"Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable","fromName":"Štěpán Němec","fromEmail":"stepnem@gmail.com","sentAt":"2009-11-20T10:08:00Z","receivedAt":"2009-11-20T10:08:00Z","isPatch":true,"sender":{"key":"stepnem@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106838?v=4"},"body":"Hello,\nI bet it would get caught in the review process anyway, but just in\ncase -- a small typo:\n> +With 'forward', diffs are taken forward and applied in reverse, i.e.,\n> +if you added a block of code you see an addition patch and are asked\n> +if you want to remove it.  'reverse' instead shows a removal patch\n> +and asks if you to apply it.  'mixed' is the same as 'forward' when\n\nShould be 'asks if you /want/ to apply it.'\n\nŠtěpán\n"},{"id":"128529","messageId":"20091127064107.GC20844@coredump.intra.peff.net","threadId":"21676","inReplyTo":"527e9296b638eb4c9993b3fb0d1c6f51b64f4c2c.1258667920.git.trast@student.ethz.ch","subject":"Re: [RFC PATCH] {checkout,reset} -p: make patch direction configurable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-11-27T06:41:07Z","receivedAt":"2009-11-27T06:41:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 19, 2009 at 11:03:57PM +0100, Thomas Rast wrote:\n\n> When we implemented the -p mode for checkout, reset and stash, some\n> discussion revolved around the involved patch direction.\n> \n> Make this configurable for reset and checkout with the following\n> choices:\n> \n>              index/HEAD       other\n>   forward    undo addition    undo addition\n>   mixed      undo addition    apply removal\n>   reverse    apply removal    apply removal\n> [...]\n> ISTR that Peff wanted this, and maybe some others.  I'm not too\n> interested because I'm still convinced 'mixed' is the Right Option,\n> but it was somewhere deep on my todo stack and maybe you like it ;-)\n\nActually, I am pretty happy with the current \"discard this hunk\" most of\nthe time. It is easy enough to see \"you made this change, did you want\nto get rid of it?\". The one exception is during patch editing. Try\nsomething simple like:\n\ncat >file <<EOF\nthis\nis\na\nfile\nwith\nsome\ncontent\nin\nit\nEOF\n\ngit add file\ngit commit -m base\n\ncat >file <<EOF\nthis\nis\na\nfile\nwith\nsome\nother\ncontent\nEOF\n\ngit checkout -p\n\nNow try to 'e'dit the patch to throw away the addition of \"other\", but\nkeep the deletion of the other two lines. Do you find it easy or hard?\nNow try it with interactive.checkout.direction set to forward. I find\nediting the forward direction _much_ simpler.\n\nAssuming you agree, I'm not sure what that tells us, though. I probably\nwouldn't personally set interactive.*.direction for the yes/no part. But\nI would find it more convenient to do patch editing always in the\nforward direction. I'm worried that it would be too jarring to the user,\nthough, to see the patch presented in one direction but edit it in the\nopposite direction. Maybe it would be nice to have yet another\ninteractive option to swap between the two for this particular hunk, and\nthen you could edit the direction you prefer.\n\n\nJunio raised the question of consistency in another thread. I don't see\na consistency problem here. This is by definition an interactive\nprocedure. I don't see a reason not to allow the user their preferred\nstyle. But I think if I could swap when editing, I would personally not\ncare all that much about the other direction.\n\n-Peff\n"}]}