{"thread":{"id":"54955","subject":"Feature request - git add --patch should have an option to discard or drop a hunk Inbox","startedAt":"2021-01-07T16:45:05Z","lastAt":"2021-01-08T16:16:04Z","messageCount":4,"participants":["Jon Sagotsky","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"413714","messageId":"CAJamm=+Z=88G6cH3DRkttGQkrf9tng310+p=4cKiy-GvdCG3zg@mail.gmail.com","threadId":"54955","inReplyTo":null,"subject":"Feature request - git add --patch should have an option to discard or drop a hunk Inbox","fromName":"Jon Sagotsky","fromEmail":"sagotsky@gmail.com","sentAt":"2021-01-07T16:43:45Z","receivedAt":"2021-01-07T16:45:05Z","isPatch":false,"sender":{"key":"sagotsky@gmail.com","avatar":null},"body":"I frequently make changes to a number of files.  I use `git add -p` to\nselect a number of changes and wrap them up in a commit.  Then I run\nit again to build another commit.  Wash, rinse, repeat.\n\nSometimes I make a change I don't intend to keep.  Maybe it's a typo,\nor some debug code, or my linter does something naughty.  As\ndescribed, I have to skip that hunk once per commit I'm building.\nUsually this is merely inconvenient, but each pass through `git add\n-p` runs the risk of me mistakenly adding unintended changes to my\ncommit.\n\nI could use `e` to edit away that hunk, but for some odd reason that\nfeels too tedious.  What I'd really like as a small quality of life\nenhancement is the ability to discard a hunk so that it's deleted\nimmediately and I don't have to skip it again.  `d` is taken, so I\nsuggest `x` as a shortcut.\n\nThanks!\nJon\n"},{"id":"413796","messageId":"X/ghgg+IdZYri1KV@coredump.intra.peff.net","threadId":"54955","inReplyTo":"CAJamm=+Z=88G6cH3DRkttGQkrf9tng310+p=4cKiy-GvdCG3zg@mail.gmail.com","subject":"Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-08T09:10:26Z","receivedAt":"2021-01-08T09:11:25Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 07, 2021 at 11:43:45AM -0500, Jon Sagotsky wrote:\n\n> I frequently make changes to a number of files.  I use `git add -p` to\n> select a number of changes and wrap them up in a commit.  Then I run\n> it again to build another commit.  Wash, rinse, repeat.\n> \n> Sometimes I make a change I don't intend to keep.  Maybe it's a typo,\n> or some debug code, or my linter does something naughty.  As\n> described, I have to skip that hunk once per commit I'm building.\n> Usually this is merely inconvenient, but each pass through `git add\n> -p` runs the risk of me mistakenly adding unintended changes to my\n> commit.\n\nI do something similar and have run into the same problem. There's an\nold discussion here going into some ideas:\n\n  https://lore.kernel.org/git/EE89F0A1-1C07-4597-B654-035F657AD09F@me.com/\n\nThe interesting bits are I think:\n\n  - some tools let you do this already (magit was mentioned there, and\n    I'd expect vim plugins like fugitive can probably do the same; tig\n    can also do so from its \"stage\" interface)\n\n  - it probably wouldn't be _too_ hard to implement, because we already\n    drive those actions from the same code that is invoked by \"add -p\"\n    and \"git checkout -p\"; the difference is just which program we feed\n    the hunks to.\n\n  - in the most general form of the tool, it would let you take a pass\n    through the hunks and annotating them. The simple common form is two\n    annotations: stage these ones, discard those ones. There may be room\n    for a more generalized tool, or it may just be over-complicating\n    things.\n\n    The generalized form probably shouldn't be \"add -p\". The simplified\n    one (just adding \"discard this hunk from the worktree\") could\n    perhaps be, though it does feel a little weird for \"git add\" to\n    modify working tree files.\n\n-Peff\n"},{"id":"413825","messageId":"nycvar.QRO.7.76.6.2101081614440.2213@tvgsbejvaqbjf.bet","threadId":"54955","inReplyTo":"X/ghgg+IdZYri1KV@coredump.intra.peff.net","subject":"Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-01-08T15:21:05Z","receivedAt":"2021-01-08T15:22:44Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Jan 2021, Jeff King wrote:\n\n> On Thu, Jan 07, 2021 at 11:43:45AM -0500, Jon Sagotsky wrote:\n>\n> > I frequently make changes to a number of files.  I use `git add -p` to\n> > select a number of changes and wrap them up in a commit.  Then I run\n> > it again to build another commit.  Wash, rinse, repeat.\n> >\n> > Sometimes I make a change I don't intend to keep.  Maybe it's a typo,\n> > or some debug code, or my linter does something naughty.  As\n> > described, I have to skip that hunk once per commit I'm building.\n> > Usually this is merely inconvenient, but each pass through `git add\n> > -p` runs the risk of me mistakenly adding unintended changes to my\n> > commit.\n>\n> I do something similar and have run into the same problem. There's an\n> old discussion here going into some ideas:\n>\n>   https://lore.kernel.org/git/EE89F0A1-1C07-4597-B654-035F657AD09F@me.com/\n>\n> The interesting bits are I think:\n>\n>   - some tools let you do this already (magit was mentioned there, and\n>     I'd expect vim plugins like fugitive can probably do the same; tig\n>     can also do so from its \"stage\" interface)\n>\n>   - it probably wouldn't be _too_ hard to implement, because we already\n>     drive those actions from the same code that is invoked by \"add -p\"\n>     and \"git checkout -p\"; the difference is just which program we feed\n>     the hunks to.\n>\n>   - in the most general form of the tool, it would let you take a pass\n>     through the hunks and annotating them. The simple common form is two\n>     annotations: stage these ones, discard those ones. There may be room\n>     for a more generalized tool, or it may just be over-complicating\n>     things.\n>\n>     The generalized form probably shouldn't be \"add -p\". The simplified\n>     one (just adding \"discard this hunk from the worktree\") could\n>     perhaps be, though it does feel a little weird for \"git add\" to\n>     modify working tree files.\n\nWe do have `git stash -p`, which I used sometimes to discard such unwanted\npieces. However, `git stash -p` suffers from a couple implementation\ndetails (see e.g.\nhttps://lore.kernel.org/git/20200731165140.29197-1-alban.gruin@gmail.com/)\n\nMore often than not, I am actually using `git add -p` to commit those\nchanges instead, using the commit message `TO-DROP` and later using\n`rebase -i` to actually drop them.\n\nI could imagine that it should not be _too_ hard to implement an action in\n`git add -p` to allow stashing a given hunk. The biggest challenge there\nwould be to make sure that potentially edited, to-be-stashed changes won't\ninterfere with staging what was intended (because `git add -p` batches the\nchanging per-file, and if the index changes while picking what to stage,\nthings might conflict).\n\nMaybe an Outreachy project?\n\nCiao,\nDscho\n"},{"id":"413827","messageId":"X/iFCo0bXLR+LZXs@coredump.intra.peff.net","threadId":"54955","inReplyTo":"nycvar.QRO.7.76.6.2101081614440.2213@tvgsbejvaqbjf.bet","subject":"Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-01-08T16:15:06Z","receivedAt":"2021-01-08T16:16:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 08, 2021 at 04:21:05PM +0100, Johannes Schindelin wrote:\n\n> >   - in the most general form of the tool, it would let you take a pass\n> >     through the hunks and annotating them. The simple common form is two\n> >     annotations: stage these ones, discard those ones. There may be room\n> >     for a more generalized tool, or it may just be over-complicating\n> >     things.\n> >\n> >     The generalized form probably shouldn't be \"add -p\". The simplified\n> >     one (just adding \"discard this hunk from the worktree\") could\n> >     perhaps be, though it does feel a little weird for \"git add\" to\n> >     modify working tree files.\n> \n> We do have `git stash -p`, which I used sometimes to discard such unwanted\n> pieces. However, `git stash -p` suffers from a couple implementation\n> details (see e.g.\n> https://lore.kernel.org/git/20200731165140.29197-1-alban.gruin@gmail.com/)\n\nYeah, and I often will take a first pass with \"stash -p\" to get rid of\nunwanted hunks, and _then_ start dividing up the hunks into commits that\nI want. But that requires multiple passes. I think the point is that you\noften see the undesirable hunk (that you'd like to stash or discard)\nwhile you're making another pass, and would like to deal with it then\nand there, instead of separately.\n\nTo be clear, I'm not objecting to a \"-p\" command modifying the working\ntree or anything like that. Just that mixing the modification of the\nworking tree into \"add -p\" feels weird, because of the \"add\" half, not\nthe \"-p\" half. :) Conceptually it's a separate action: take a single\npass over the hunks and decide what to do with them. But I do agree that\nin practice that's the operation people are trying to do with \"add -p\",\nso it may be a practical place to put it.\n\n> More often than not, I am actually using `git add -p` to commit those\n> changes instead, using the commit message `TO-DROP` and later using\n> `rebase -i` to actually drop them.\n\nThat works OK, too, but suffers from the same multi-pass problem. You\nhave to do a separate pass to put them in a TO-DROP commit, and likely\nyou've already marked a bunch of other hunks to be staged for a real\ncommit.\n\n> I could imagine that it should not be _too_ hard to implement an action in\n> `git add -p` to allow stashing a given hunk. The biggest challenge there\n> would be to make sure that potentially edited, to-be-stashed changes won't\n> interfere with staging what was intended (because `git add -p` batches the\n> changing per-file, and if the index changes while picking what to stage,\n> things might conflict).\n> \n> Maybe an Outreachy project?\n\nYeah, it's probably about the right size. The actual mechanics are\npretty easy. Here's a really ugly proof-of-concept in the perl script:\n\ndiff --git a/git-add--interactive.perl b/git-add--interactive.perl\nindex 96426a53c6..528688e553 100755\n--- a/git-add--interactive.perl\n+++ b/git-add--interactive.perl\n@@ -106,6 +106,7 @@ sub colored {\n \t\tAPPLY_CHECK => 'apply --cached',\n \t\tFILTER => 'file-only',\n \t\tIS_REVERSE => 0,\n+\t\tDISCARD => sub { apply_patch 'apply -R', @_ },\n \t},\n \t'stash' => {\n \t\tDIFF => 'diff-index -p HEAD',\n@@ -1568,6 +1569,9 @@ sub patch_update_file {\n \t\t\t\tprint;\n \t\t\t}\n \t\t}\n+\t\tif (exists $patch_mode_flavour{DISCARD}) {\n+\t\t\t$other .= ',x';\n+\t\t}\n \t\tmy $type = $num ? $hunk[$ix]{TYPE} : $head->{TYPE};\n \t\tprint colored $prompt_color, \"(\", ($ix+1), \"/\", ($num ? $num : 1), \") \",\n \t\t\tsprintf(__($patch_update_prompt_modes{$patch_mode}{$type}), $other);\n@@ -1761,6 +1765,18 @@ sub patch_update_file {\n \t\t\t\t\tsplice @hunk, $ix, 1, $newhunk;\n \t\t\t\t}\n \t\t\t}\n+\t\t\telsif ($line =~ /^x/) {\n+\t\t\t\tunless ($other =~ /x/) {\n+\t\t\t\t\terror_msg(\"don't know how to discard in this mode\");\n+\t\t\t\t}\n+\t\t\t\t# should probably save up discards and apply at\n+\t\t\t\t# the end?\n+\t\t\t\tmy @patch = reassemble_patch($head->{TEXT},\n+\t\t\t\t\t                     @{$hunk[$ix]->{TEXT}});\n+\t\t\t\t$patch_mode_flavour{DISCARD}->(@patch);\n+\t\t\t\t# should mark hunk as used so we don't\n+\t\t\t\t# redisplay it\n+\t\t\t}\n \t\t\telse {\n \t\t\t\thelp_patch_cmd($other);\n \t\t\t\tnext;\n\nI left this intentionally ugly because I do not even want to consider\nimplementing this for real in the perl version, not when we're so close\nto getting rid of it (but I'm familiar enough with it and it's fast\nenough to hack on that I did not want to even try the ugly thing in the\nC code). It's just meant to show that we basically have all the\nlow-level parts available already.\n\nThere are some obvious unfinished bits in the comments, but I agree the\nreally tricky parts will be corner cases: what happens to edited hunks\nor ones which overlap. It may all Just Work because we're applying to\ntwo different spots, so overlap isn't a problem. But I haven't thought\ndeeply about it.\n\n-Peff\n"}]}