Add shortcut to discard changes

[RianFuro]

Add a new command x that will discard changes under the cursor,
after user confirmation. Works on hunks, unstaged and staged files.
Does not currently work on untracked files due to a bug in hunk
detection firing on those.
Does not currently work on whole sections or selections.

[TimUntersberger]

Does not currently work on untracked files due to a bug in hunk
detection firing on those.
Does not currently work on whole sections or selections.

Do you want to fix these before merging?

[RianFuro]

Do you want to fix these before merging?

Unsure. The problem with hunk detection is a bit bigger of an issue I'd like to tackle separately.
I haven't thought too much about discarding from selections yet. I'd basically need to create a patch on the fly, then reverse apply it. If you want to wait for it, I can try my hands at it. Otherwise I'd just go ahead with what's there.

[TimUntersberger]

I'd basically need to create a patch on the fly, then reverse apply it. If you want to wait for it, I can try my hands at it

Can't you just copy most of the logic from unstage/stage?

[TimUntersberger]

The problem with hunk detection is a bit bigger of an issue I'd like to tackle separately.

I sometimes can't toggle the diff of some changes. Could this be related to this issue? Also could you give me a TLDR about this issue?

[RianFuro]

Can't you just copy most of the logic from unstage/stage?

Probably. You don't write an actual patch though, right? You're just writing a modified object for the object database there. As far as I could tell, I can't do the equivalent for resetting though, so I'm using git apply --reverse and feeding it a patch to reverse. For whole hunks that's fine (since I just need to apply diff headers), for individual selections I'll need to construct the patch myself.

Could this be related to this issue? Also could you give me a TLDR about this issue?

I don't think so. The problem is really simple:
https://github.com/TimUntersberger/neogit/blob/a0db00143861e370681e0e30cb1ae7402e774b4b/lua/neogit/status.lua#L541-L543
This check is not accurate. Since files in the untracked section do not have any prefix, this check returns true erroneously (actually on all lines that are not files with a prefix).
To fix this I need to distinguish hunks lines from non-hunk lines though, which isn't actually that easy. I would refactor to detect hunks by line index instead - and maybe centralize the logic for getting the cursor context while i'm at it.

[TimUntersberger]

git apply seems way more cleaner. We could also use this for stage and unstage right?

[RianFuro]

git apply seems way more cleaner. We could also use this for stage and unstage right?

I wanted to suggest that yes, seems like the do-it-all solution. I appreciate your approach with manually inserting objects though, I learned something from that :)

Also, maybe I shouldn't code past 23:00... I added discarding untracked files. Apparently it's not too hard when you order things correctly 😅

[RianFuro]

Ok I'm having real trouble creating correct patches when selecting only part of a replacement operation and stuff like that. I'll study how magit accomplishes it's magic on the weekend and see what I can apply.
So, from my side, I'm good to merge this for the time being. (Do I have to do anything regarding the change request? I marked it as resolved already, but I guess that's only for me) Nope. nope nope nope. Just noticed I have the exact same problem when staging subsequent hunks, if previous hunks would offset the line tally; have to account for that as well.

[TimUntersberger]

Ok I'm having real trouble creating correct patches when selecting only part of a replacement operation and stuff like that. I'll study how magit accomplishes it's magic on the weekend and see what I can apply.

My solution for stage/unstage correctly generates a "hunk" from the selection and then inserts it in the file. Maybe you can extract the hunk generation and use that for git apply ?

[TimUntersberger]

I added a function that generates a valid patch from a selection, but now I am not sure how to correctly apply any patches?

Even using the most basic git diff > test.patch and then git apply test.patch is not working for me? Are you encountering the same thing?

[RianFuro]

Even using the most basic git diff > test.patch and then git apply test.patch is not working for me? Are you encountering the same thing?

You're missing a step inbetween. Since your working copy has the patch already applied (since you created the diff from that) you first need to reset to HEAD to apply the patch again. Alternatively, you can run git apply --reverse test.patch to remove the changes outlined in the patch file, after which you can then of course apply the patch normally again with git apply test.patch

[RianFuro]

My solution for stage/unstage correctly generates a "hunk" from the selection and then inserts it in the file.

I'm afraid we're falling into the same trap. Hunks are a subsequent chain of changes, that need to be read in order. This part of the code is not always correct, since the hunk indices can shift when you apply them out of order:
https://github.com/TimUntersberger/neogit/blob/fe316c8a8daed9e1f29eeba70a2f9b7586e7f091/lua/neogit/status.lua#L453

I feel the magit docs actually explain that really well:

(defun magit-apply--adjust-hunk-new-starts (hunks)
  "Adjust new line numbers in headers of HUNKS for partial application.
HUNKS should be a list of ordered, contiguous hunks to be applied
from a file.  For example, if there is a sequence of hunks with
the headers

  @@ -2,6 +2,7 @@
  @@ -10,6 +11,7 @@
  @@ -18,6 +20,7 @@

and only the second and third are to be applied, they would be
adjusted as \"@@ -10,6 +10,7 @@\" and \"@@ -18,6 +19,7 @@\"."
)

(in this example, the first hunk appends a line to the new version that is not present if you only take the second hunk, thus it's index would need to shift to accomodate)

[TimUntersberger]

Shouldn't this only matter if we support staging multiple hunks at once? Right now this is not supported.

[RianFuro]

No, this is already an issue with only one hunk, and double the issue when only applying part of a hunk. With your approach, we might actually get away without any additional complexity for partial selection. I tried to keep the patch as small as possible and adjusted the start of the hunk to align with the selection... guess I was trying to be too smart for my own good 😅

In their example they say they'd apply the 2nd and 3rd hunk, but you actually have the same problem just applying the 2nd hunk. The problem is that the first hunk is skipped, since the insert from the first hunk is the reason the indices are different (-10, +11) in the second hunk.

[RianFuro]

@TimUntersberger
Ok, after thinking it over with various scenarios, it's actually not that involved:
For a general use case, since the offset between index and disk only accumulates down a patch file, we can just use the current hunk (to be staged) as a starting point and use it's offset to correct itself and all subsequent hunks by simply subtracting it's offset from each disk offset.

That means, for the specific case of staging a single hunk, we just need to set disk offset = index offset, so in your implementation:

-local diff_header = string.format("@@ -%d,%d +%d,%d @@", hunk.index_from, hunk.index_len, hunk.disk_from, applied_len)
+local diff_header = string.format("@@ -%d,%d +%d,%d @@", hunk.index_from, hunk.index_len, hunk.index_from, applied_len)

should solve the issue.

[RianFuro]

Maybe you can extract the hunk generation and use that for git apply ?

I had to adapt it a little bit to work in certain edge cases, but discard via selection now works :) Discarding a single hunk should now also work in all cases, so I'm good to merge now!

[TimUntersberger]

@RianFuro it should also be possible to use the generate_patch_from_selection function for staging/unstaging right?

[RianFuro]

I haven't tested it, but I took care of still allowing for the normal staging case, yes.
Unstaging is the same flow as discarding but with --cached instead of --index so that definitely works.