Feature request - git add --patch should have an option to discard or drop a hunk Inbox

Feature request - git add --patch should have an option to discard or drop a hunk Inbox

[Jon Sagotsky]

I frequently make changes to a number of files.  I use `git add -p` to
select a number of changes and wrap them up in a commit.  Then I run
it again to build another commit.  Wash, rinse, repeat.

Sometimes I make a change I don't intend to keep.  Maybe it's a typo,
or some debug code, or my linter does something naughty.  As
described, I have to skip that hunk once per commit I'm building.
Usually this is merely inconvenient, but each pass through `git add
-p` runs the risk of me mistakenly adding unintended changes to my
commit.

I could use `e` to edit away that hunk, but for some odd reason that
feels too tedious.  What I'd really like as a small quality of life
enhancement is the ability to discard a hunk so that it's deleted
immediately and I don't have to skip it again.  `d` is taken, so I
suggest `x` as a shortcut.

Thanks!
Jon

Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox

[Jeff King]

On Thu, Jan 07, 2021 at 11:43:45AM -0500, Jon Sagotsky wrote:

> I frequently make changes to a number of files.  I use `git add -p` to
> select a number of changes and wrap them up in a commit.  Then I run
> it again to build another commit.  Wash, rinse, repeat.
> 
> Sometimes I make a change I don't intend to keep.  Maybe it's a typo,
> or some debug code, or my linter does something naughty.  As
> described, I have to skip that hunk once per commit I'm building.
> Usually this is merely inconvenient, but each pass through `git add
> -p` runs the risk of me mistakenly adding unintended changes to my
> commit.

I do something similar and have run into the same problem. There's an
old discussion here going into some ideas:

  https://lore.kernel.org/git/EE89F0A1-1C07-4597-B654-035F657AD09F@me.com/

The interesting bits are I think:

  - some tools let you do this already (magit was mentioned there, and
    I'd expect vim plugins like fugitive can probably do the same; tig
    can also do so from its "stage" interface)

  - it probably wouldn't be _too_ hard to implement, because we already
    drive those actions from the same code that is invoked by "add -p"
    and "git checkout -p"; the difference is just which program we feed
    the hunks to.

  - in the most general form of the tool, it would let you take a pass
    through the hunks and annotating them. The simple common form is two
    annotations: stage these ones, discard those ones. There may be room
    for a more generalized tool, or it may just be over-complicating
    things.

    The generalized form probably shouldn't be "add -p". The simplified
    one (just adding "discard this hunk from the worktree") could
    perhaps be, though it does feel a little weird for "git add" to
    modify working tree files.

-Peff

Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox

[Johannes Schindelin]

Hi Peff,

On Fri, 8 Jan 2021, Jeff King wrote:

> On Thu, Jan 07, 2021 at 11:43:45AM -0500, Jon Sagotsky wrote:
>
> > I frequently make changes to a number of files.  I use `git add -p` to
> > select a number of changes and wrap them up in a commit.  Then I run
> > it again to build another commit.  Wash, rinse, repeat.
> >
> > Sometimes I make a change I don't intend to keep.  Maybe it's a typo,
> > or some debug code, or my linter does something naughty.  As
> > described, I have to skip that hunk once per commit I'm building.
> > Usually this is merely inconvenient, but each pass through `git add
> > -p` runs the risk of me mistakenly adding unintended changes to my
> > commit.
>
> I do something similar and have run into the same problem. There's an
> old discussion here going into some ideas:
>
>   https://lore.kernel.org/git/EE89F0A1-1C07-4597-B654-035F657AD09F@me.com/
>
> The interesting bits are I think:
>
>   - some tools let you do this already (magit was mentioned there, and
>     I'd expect vim plugins like fugitive can probably do the same; tig
>     can also do so from its "stage" interface)
>
>   - it probably wouldn't be _too_ hard to implement, because we already
>     drive those actions from the same code that is invoked by "add -p"
>     and "git checkout -p"; the difference is just which program we feed
>     the hunks to.
>
>   - in the most general form of the tool, it would let you take a pass
>     through the hunks and annotating them. The simple common form is two
>     annotations: stage these ones, discard those ones. There may be room
>     for a more generalized tool, or it may just be over-complicating
>     things.
>
>     The generalized form probably shouldn't be "add -p". The simplified
>     one (just adding "discard this hunk from the worktree") could
>     perhaps be, though it does feel a little weird for "git add" to
>     modify working tree files.

We do have `git stash -p`, which I used sometimes to discard such unwanted
pieces. However, `git stash -p` suffers from a couple implementation
details (see e.g.
https://lore.kernel.org/git/20200731165140.29197-1-alban.gruin@gmail.com/)

More often than not, I am actually using `git add -p` to commit those
changes instead, using the commit message `TO-DROP` and later using
`rebase -i` to actually drop them.

I could imagine that it should not be _too_ hard to implement an action in
`git add -p` to allow stashing a given hunk. The biggest challenge there
would be to make sure that potentially edited, to-be-stashed changes won't
interfere with staging what was intended (because `git add -p` batches the
changing per-file, and if the index changes while picking what to stage,
things might conflict).

Maybe an Outreachy project?

Ciao,
Dscho

Re: Feature request - git add --patch should have an option to discard or drop a hunk Inbox

[Jeff King]

On Fri, Jan 08, 2021 at 04:21:05PM +0100, Johannes Schindelin wrote:

> >   - in the most general form of the tool, it would let you take a pass
> >     through the hunks and annotating them. The simple common form is two
> >     annotations: stage these ones, discard those ones. There may be room
> >     for a more generalized tool, or it may just be over-complicating
> >     things.
> >
> >     The generalized form probably shouldn't be "add -p". The simplified
> >     one (just adding "discard this hunk from the worktree") could
> >     perhaps be, though it does feel a little weird for "git add" to
> >     modify working tree files.
> 
> We do have `git stash -p`, which I used sometimes to discard such unwanted
> pieces. However, `git stash -p` suffers from a couple implementation
> details (see e.g.
> https://lore.kernel.org/git/20200731165140.29197-1-alban.gruin@gmail.com/)

Yeah, and I often will take a first pass with "stash -p" to get rid of
unwanted hunks, and _then_ start dividing up the hunks into commits that
I want. But that requires multiple passes. I think the point is that you
often see the undesirable hunk (that you'd like to stash or discard)
while you're making another pass, and would like to deal with it then
and there, instead of separately.

To be clear, I'm not objecting to a "-p" command modifying the working
tree or anything like that. Just that mixing the modification of the
working tree into "add -p" feels weird, because of the "add" half, not
the "-p" half. :) Conceptually it's a separate action: take a single
pass over the hunks and decide what to do with them. But I do agree that
in practice that's the operation people are trying to do with "add -p",
so it may be a practical place to put it.

> More often than not, I am actually using `git add -p` to commit those
> changes instead, using the commit message `TO-DROP` and later using
> `rebase -i` to actually drop them.

That works OK, too, but suffers from the same multi-pass problem. You
have to do a separate pass to put them in a TO-DROP commit, and likely
you've already marked a bunch of other hunks to be staged for a real
commit.

> I could imagine that it should not be _too_ hard to implement an action in
> `git add -p` to allow stashing a given hunk. The biggest challenge there
> would be to make sure that potentially edited, to-be-stashed changes won't
> interfere with staging what was intended (because `git add -p` batches the
> changing per-file, and if the index changes while picking what to stage,
> things might conflict).
> 
> Maybe an Outreachy project?

Yeah, it's probably about the right size. The actual mechanics are
pretty easy. Here's a really ugly proof-of-concept in the perl script:

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 96426a53c6..528688e553 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -106,6 +106,7 @@ sub colored {
 		APPLY_CHECK => 'apply --cached',
 		FILTER => 'file-only',
 		IS_REVERSE => 0,
+		DISCARD => sub { apply_patch 'apply -R', @_ },
 	},
 	'stash' => {
 		DIFF => 'diff-index -p HEAD',
@@ -1568,6 +1569,9 @@ sub patch_update_file {
 				print;
 			}
 		}
+		if (exists $patch_mode_flavour{DISCARD}) {
+			$other .= ',x';
+		}
 		my $type = $num ? $hunk[$ix]{TYPE} : $head->{TYPE};
 		print colored $prompt_color, "(", ($ix+1), "/", ($num ? $num : 1), ") ",
 			sprintf(__($patch_update_prompt_modes{$patch_mode}{$type}), $other);
@@ -1761,6 +1765,18 @@ sub patch_update_file {
 					splice @hunk, $ix, 1, $newhunk;
 				}
 			}
+			elsif ($line =~ /^x/) {
+				unless ($other =~ /x/) {
+					error_msg("don't know how to discard in this mode");
+				}
+				# should probably save up discards and apply at
+				# the end?
+				my @patch = reassemble_patch($head->{TEXT},
+					                     @{$hunk[$ix]->{TEXT}});
+				$patch_mode_flavour{DISCARD}->(@patch);
+				# should mark hunk as used so we don't
+				# redisplay it
+			}
 			else {
 				help_patch_cmd($other);
 				next;

I left this intentionally ugly because I do not even want to consider
implementing this for real in the perl version, not when we're so close
to getting rid of it (but I'm familiar enough with it and it's fast
enough to hack on that I did not want to even try the ugly thing in the
C code). It's just meant to show that we basically have all the
low-level parts available already.

There are some obvious unfinished bits in the comments, but I agree the
really tricky parts will be corner cases: what happens to edited hunks
or ones which overlap. It may all Just Work because we're applying to
two different spots, so overlap isn't a problem. But I haven't thought
deeply about it.

-Peff