Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rc tools patch/git: commands to apply selected lines #5004

Merged
merged 2 commits into from Nov 5, 2023

Conversation

krobelus
Copy link
Contributor

@krobelus krobelus commented Oct 22, 2023

One of the features I miss most from Magit/Fugitive/Tig is to
apply/revert/stage/unstage individual hunks or even exactly the
selected line(s). This provides a much more convenient way of
splitting changes than "git add/restore -p".

Implement a "patch" command that applies the selected lines within
a diff by piping them to the "patch" program.
It can also feed other programs like "git apply" (see the next commit).

Original discussion: https://discuss.kakoune.com/t/atomic-commits-in-kakoune/1446

Interestingly, :patch is defined outside the "patch" module. This is
to make it readily available for interactive use.
Putting it into the module does not save any work.
I tentatively added a patch module anyway so we can explicitly declare
this dependency.. although there is the argument that this is not
really needed?


The second commit adds :git apply, a somewhat discoverable frontend for common uses of the
patch command.

Here are some frequently used commands

# apply selected changes
git apply 
# revert selected changes
git apply -R
# stage selected changes
git apply --cached
# unstage selected changes
git apply --cached -R
# apply selected changes and stage them
git apply --index

For everyday use that's a lot of typing so I recommend adding mappings.

@krobelus krobelus changed the title rc filetype diff/git: commands to apply selected changed lines rc filetype diff/git: commands to apply selected lines Oct 22, 2023
@krobelus krobelus changed the title rc filetype diff/git: commands to apply selected lines rc tools patch/git: commands to apply selected lines Oct 28, 2023
@krobelus krobelus marked this pull request as ready for review October 28, 2023 17:37
@krobelus
Copy link
Contributor Author

krobelus commented Oct 28, 2023

changed the git wrappers to only git apply. Less quick to type but also less opinionated


Given some selections within a unified diff, apply the changed lines in
each selection by piping them to "patch <arguments> 1>&2"
(or "<arguments> 1>&2" if <arguments> starts with a non-option argument).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What may be confusing is that patch some-file.diff does not work.
We could try to make it work -- for example by using selections only if either no non-option argument is present or there is a "-" argument.
(Also that would mean we couldn't use the proposed <arguments> trick that enables "patch git apply".
That wouldn't be a big deal, we could pass "git apply" via an option or register..)

However there is not much point to :patch some-file.diff when we can do <a-!>patch some-file.diff so I don't think we care

Copy link
Owner

@mawww mawww left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats a great feature, my only concern is that it feels like something that should be done by an external, general tool rather than a Kakoune plugin... I think it is still worthwhile to merge.

execute-keys <a-_>
set-register a %arg{@}
set-register f nop
set-register | %{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command concerns me a litte bit. I wonder if we should rely on something like interdiff from patchutils instead (not sure if this is a good idea)... Or at least extract it into its own file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually had the perl script in a separate file originally but yanked that mostly because by convention we don't do that.
I'm not strongly against moving it to a separate script (located next to %val{source}) but right now its input is very specific to the invocation (all selected lines are prefixed with "X"). But if it helps hackability, sure. I don't expect too many changes unless we want to support more diff formats (like diff -u on a single file which doesn't add the ^diff line).

I didn't try interdiff yet but it looks like it requires two valid diff files as inputs, so we'd still need to parse and emit diff (to compute the "selected" diff) which is the bulk of the work.

I wonder how other people (Magit/Fugitive etc) solve this problem. It would be great if there was a reusable solution, because there is really no creativity required. My guess is that they have their own implementations as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patchutils is a weird mix of commands but it turns out it has what we want: filterdiff --lines=1-10,40-42 should do the same thing.
Should we still keep a fallback implementation in case filterdiff is not available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filterdiff is missing two options.

  1. filterdiff --lines filters by line numbers in the original file, not the raw diff which is more convenient for us.
    We can add a--diff-lines option.
  2. filterdiff only takes hunks as a whole. We want to stage individual lines (which already works fine).
    We can teach it a --dissect-hunks option.

With that, the shell section of the patch command will look like

set-register | %{
	set -e
	tmpdir=$(mktemp -d "${TMPDIR:-/tmp}"/kak-patch.XXXXXXXX)

        # The selected range to apply.
        IFS=' .,' read min_line _ max_line _ <<-EOF
        $kak_reg_s
        EOF
	lines=$((min_line - kak_cursor_line + 1))-$((max_line - kak_cursor_line + 1))

	# The diffs surrounding the range to apply.
	cat > "$tmpdir/whole.diff"

	filterdiff "$tmpdir/whole.diff" --dissect-hunks --diff-lines=$lines >"$tmpdir/applied.diff"
	if expr "$1" : '^$|^-'; then
	    set -- patch "$@"
	fi
	if ! "$@"; then
	    echo >&2 "patch: failed to run $@"
	    cat "$tmpdir/whole.diff"
	    exit 1
	fi
	# Output the input diffs minus the applied diffs.
	if printf %s\\n "$@" | grep -Ex '--reverse|-R'; then
	    interdiff -q "$tmpdir/applied.diff" /dev/null > "$tmpdir/reversed.diff"
	    # reversed.diff is the inverse of a subset of whole.diff. We need to compute their sum to cancel
	    # TODO generalize -p1
	    combinediff -q -p1 "$tmpdir/reversed.diff" "$tmpdir/whole.diff"
	else
	    # Since applied.diff is a subset of whole, we can use interdiff.
	    interdiff -q "$tmpdir/applied.diff" "$tmpdir/whole.diff"
	fi
	rm -rf "$tmpdir"
}

Untested (especially the interdiff/combinediff parts).


There are other issues left, like the fact that combinediff drops hunks that cancel each other out:

$ interdiff 1.diff /dev/null > 2.diff
$ combinediff -p1 1.diff 2.diff

whereas my implementation leaves them around as context lines, which is less jarring in the editor, and allows using diff-jump to inspect the file.


So one approach is to

  1. get the requisite changes into patchutils' filterdiff and maybe combinediff
  2. but also keep fallback here (at least filterdiff.pl), in case filterdiff` does not support the new options
  3. optionally, when we don't have fallbacks for combinediff/interdiff, fail at the end prompting the user to install patchutils
  4. after a few years, remove the fallbacks

It's not ideal to make users install patchutils for such a small feature.
But I do like the idea of having this functionality available in a command line tool.. and patchutils seems like the canconical place, even though the interfaces are a bit obsure.

right now its input is very specific to the invocation (all selected lines are prefixed with "X").

Found a way to get rid of this. I think for now I'll move it to a standalone script that can be run and tested independently.

In future we can use the patchutils approach. Bonus points if we use patchutils for diff-jump as well.

One alternative is to use something like Python's unidiff. But I don't think that supports diff addition/subtraction so it would only solve parsing.

One of the features I miss most from Magit/Fugitive/Tig is to
apply/revert/stage/unstage individual hunks or even exactly the
selected line(s).  This provides a much more convenient way of
splitting changes than "git add/restore -p".

Implement a "patch" command that applies the selected lines within
a diff by piping them to the "patch" program.
It can also feed other programs like "git apply" (see the next commit).

Original discussion: https://discuss.kakoune.com/t/atomic-commits-in-kakoune/1446

Interestingly, :patch is defined outside the "patch" module. This is
to make it readily available for interactive use.
Putting it into the module does not save any work.
I tentatively added a patch module anyway so we can explicitly declare
this dependency.. although there is the argument that this is not
really needed?
This adds a somewhat discoverable frontend for common uses of the
patch command.

Here are some frequently used commands

	# apply selected changes
	git apply 
	# revert selected changes
	git apply -R
	# stage selected changes
	git apply --cached
	# unstage selected changes
	git apply --cached -R
	# apply selected changes and stage them
	git apply --index

For everyday use that's a lot of typing so I recommend adding mappings.
@mawww mawww merged commit c0788f3 into mawww:master Nov 5, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants