Git add --patch does not use diff-so-fancy

[mrageh]

Hi

Not sure if this is the correct place for this issue.

Just started using this npm package and it's great, it makes reading diffs in my terminal great.

However when I use git add --patch it uses the old style diff to display each chunk that's about to be staged.

Is git add --patch not supposed to use diff under the hood, which is now diff-so-fancy?

Is there anything we can do to use diff-so-fancy with git add --patch?

Thanks

[paulirish]

Someone else pinged me about this a month ago. I'm not sure.

Based on https://github.com/git/git/blob/master/git-add--interactive.perl#L747-L768 it doesn't look good.

But i'd love to be proven wrong. There's a lot of mention of color in here, so there may be SOME configurability, but not sure if we can intercept with a pager or something.

[paulirish]

There's some other people who were poking around this:

http://www.nickcoding.com/2014/10/30/making-git-prose-friendly/
http://git.661346.n2.nabble.com/Different-diff-strategies-in-add-interactive-td7588815.html

.. which my unveil some solutions.

[mrageh]

Thanks for the swift response @paulirish, would it be worth leaving this issue open or do you think we should close it?

[paulirish]

Leave it open. It still requires some research to see if it's possible or not.

[tonekk]

👍 was just going to add the same issue

[danielcompton]

git commit --verbose doesn't use it either.

[hultberg]

As @danielcompton pointed out, git commit -v does not use diff-so-fancy.

[paulirish]

The solution here appears to be to fork $brewdir/Cellar/git/2.7.0/libexec/git-core/git-add--interactive and make some edits.

I don't see a cleaner way to configure this.

[hultberg]

We could open a feature request to git itself and see if we could get a config for this?

[paulirish]

We could open a feature request to git itself and see if we could get a config for this?

Yup, we'll need to get this supported upstream in git core first.

---

@peff I think you're the right person to help us out. First of all, thank you. This project has been a big success mostly due to your work on diff-highlight.

You can read the above issue for context, but it appears we are lacking support in git-core for a key area. We'd like to propose, I think, that add--interactive can support a configurable pager. Mostly, we want a hook to run diff-highlight and our tool while add--interactive is printing hunks. Does this make sense to you, and does it sound possible?

[OJFord]

Please forgive me for asking the obvious - I'm sure others have a firmer grasp of it than I do - but perhaps it's best to get a note of it anyway.

Does a configurable pager make sense for add--interactive in any context other than diff-so-fancy or similar tool?

For example, most have pager set to less (plus some options) or maybe vim. How does this make sense for multiple fragments? I'm not sure it would be desirable to jump in and out. I think what's needed (that is, desired) is an option separate to PAGER, which does some formatting prior to the pager or interactive prompt and pipes on through.

Ultimately this would mean that whether interactive or not, diff-so-fancy could do it's thing before the pager or the interactive prompt, and would not require modifying the PAGER from default setup at all; perhaps this would be something like FORMATTER=diff-so-fancy. Or have I misunderstood?

[paulirish]

@OJFord Yeah you're right. It's not a pager, but just a hook to pipe the diff output somewhere. It does seem like the value is mostly for diff formatting tools. yup.

[peff]

@paulirish Yes, I think what you're asking for makes sense. I don't use diff-so-fancy, but I have been annoyed at not getting diff-highlight support via git add -p before. :)

The infrastructure is already there to have "diffs to apply" versus "diffs to show the user" (that's how we do the color stuff). It assumes there's a 1:1 correlation in lines between the two versions of the diff (which is certainly true for diff-highlight; I don't know enough about diff-so-fancy to say more there). Without that correlation, all bets are off (because git can do stuff like hunk-splitting, and needs to split the user-visible one, too).

I think you'd basically just need to pipe the colorized git-diff invocation through the custom program. It shouldn't be more than a few-line change (...he says without having looked closely yet).

[peff]

I just sent http://article.gmane.org/gmane.comp.version-control.git/287659 to the list. Not well tested, but it works for me. You can also fetch it from the jk/interactive-diff-filter branch of git://github.com/peff/git.git.

[paulirish]

An update here:

@peff put up a patch on the git mailing list (thank you, btw! so perfect). and it's now been merged into git's next branch.

commit: git/git@0114384

When it's available in git's master, folks can try things with brew install --HEAD git and configure the interactive.diffFilter.

If anyone wants to try it now, they can build git from the next branch.

[peff]

@paulirish You're welcome. I'll be interested to hear whether diff-so-fancy users run into problems with things like hunk-splitting. The colorization code in add--interactive relies on a one-to-one correspondence between the stock and fancy diff lines. diff-so-fancy doesn't quite do that, but I think the line counts add up, so it should work OK in practice. Caveat emptor. :)

[megalithic]

@peff thanks for implementing this for interactive adds; from what you've seen in the code base, does it seem possible to relatively easily implement for interactive commits, e.g. --verbose?

thanks again!

[peff]

@megalithic I'm not quite sure what you mean. If you mean commit --interactive, that should work already with my patch (along with checkout -p, reset -p, etc, as they are all fed by the add--interactive script).

The commit --verbose code path is quite different, and is all done internally, with no separate processes. I suspect it would not be too hard to add an option to tell git to run some arbitrary process rather than its internal diff, when given --verbose. But then, I think you can pretty much already do that with a prepare-commit-msg hook (you can either add commented-out lines, or you can have a matching commit-msg hook to strip them out after the user has edited).

[paulirish]

@peff i built git from the next branch but so far I'm unable to get the patch working..

I'm doing ~/code/temp/git/git add -p and I have this in my ~/.gitconfig:

[interactive]
    diffFilter = "tail -n 3"

But I'm seeing full headers, lines of context for each prompt. Any suggestions?

[peff]

@paulirish I assume ~/code/temp/git/git is your build directory. Running it directly from there means that git will still find the older version of any external programs (like the git-add--interactive helper) in your $PATH.

Either install it (with make install PREFIX=/wherever...) and run it from the installed directory, or run it at ~/code/temp/git/bin-wrappers/git add -p, which will add the build directory to the front of your $PATH while git is running.

[paulirish]

run it at ~/code/temp/git/bin-wrappers/git add -p, which will add the build directory to the front of your $PATH while git is running.

ah thanks. That sounds like the ticket. will try.

[paulirish]

It works!

---

... however it looks like we're getting an error..

Use of uninitialized value $_ in print at /Users/paulirish/code/temp/git/git-add--interactive line 1364, line 1.

it could very likely be however we're handling the output. cc @scottchiefbaker .. you can see the hunk headers are a bit confused:

(on the left i'm using cat as my diffFilter)

[peff]

The problem is that git doesn't actually re-parse the filtered output (nor would you want it to, since it doesn't understand your fancy output). It just assumes that line 1 of the diff corresponds to line 1 of the filtered output, and so on.

The diff header has 4 lines (diff --git..., the index line, and the ---/+++ lines), but your fancy output only has 3 (the two border lines, and the filename). So everything it shows is off-by-one, and at the end perl complains that we look past the final element of the filtered array, as it's one line shorter than the diff.

I think you'll need to output an extra line somewhere in the fancy header to match git. Note that git's diffs can actually vary a little, too, depending on whether there is other data like renames, and possibly with mode changes, too. So you may want to actually count the number of lines to the first hunk, output your header, and then insert the appropriate amount of filler.

[stevemao]

Is there a way to make an offset somehow? Or even fake it?
If not, we have to add empty lines to make it work (Paul might be sad about it).
Thanks @peff for the help!

[scottchiefbaker]

Considering the number of respondents on this issue I figured we'd have some more testers. I haven't heard anything from anyone about this issue. Has anyone tested this fix with --patch? Is it working for you?

[dorian-marchal]

I'm testing it since your last message (8 days ago), works fine for me. 👍

[nthapaliya]

Same as @dorian-marchal. Have been testing over the last week, and it works as advertised. No problems yet.

[wvega-godaddy]

I've been testing the --patch mode for a couple of days with good results in almost all cases. The only problem I've found is when you do the following:

1. Create a new file with content
2. Use git add --intent-to-add /path/to/file
3. Now git add -p fails with the error below

fatal: mismatched output from interactive.diffFilter
hint: Your filter must maintain a one-to-one correspondence
hint: between its input and output lines.

I'm using Git 2.29.2 on macOS with interactive.diffFilter set to diff-so-fancy --patch.

Thank @wren @scottchiefbaker and everyone that made diff-so-fancy --patch possible. I would continue to use the new version even if the edge case above cannot be resolved. The new mode is a great improvement!

[wren]

@wvega-godaddy I wasn't aware of the --intent-to-add flag, so I haven't tested with it. I think I might have some free time this weekend, though, so I can look at it deeper then.

If anyone else has some time to test this before then, please go ahead, and let me know whatever change the PR needs.

[wren]

@wvega-godaddy Actually, nevermind. After sending my last comment, I realized that I already knew the problem, so I went ahead an pushed a new commit. Give it a shot and see if that fixes your problem.

The problem was that--because I didn't know about --intent-to-add--I thought that new files couldn't ever show up in patch mode, so I didn't include the fix for that section. Turns out I was wrong, so here we are.

[wvega-godaddy]

@wren thank you so much for the quick reply! I checked out your branch and the error is gone using that version 🎉

I accidentally got the error again when I attempted to add an empty file, but there is not point in using --intent-to-add and git add -p with an empty file. For those you can just do git add.

I can now do git add -N /path/to/file and then git add -p and edit the hunk to stage part of the new file only, just like with changes on existing files. Great work!

[scottchiefbaker]

I'm going to close this issue because I just released v.1.4.0 which should address this.

If issues come up related to --patch mode let's start a new issue.

[nthapaliya]

Please update the fat-packed versions as well!

[BuonOmo]

FYI macos users: the Homebrew version is here :)

[karlhorky]

Really nice, thanks so much to everyone that put in hard work here!

Should the Git configuration / setup of the --patch mode be documented in the readme?

[scottchiefbaker]

@nthapaliya I don't maintain the fatpack version anymore because so few people use it. It's pretty easy to clone the whole repo and use it directly so the fatpacked version wasn't the best use of my time.

That said I did a one-off and updated the release to include a fatpacked binary just for you. It will probably be the last time though.

The code to build a fatpacked version is in third_party/build_fatpack so you can build your own.

[yanndinendal]

Is a npm release planed?

[OJFord]

Is a npm release [of v1.4.0] planned?

I believe @paulirish handles those?

[khellang]

Hey @paulirish and/or @stevemao! 👋🏻

What would it take to get a 1.4.2 NPM package out the door? Anything any of us can do to help? 👼🏻

[JamesMcMahon]

I am seeing the mismatched output from interactive.diffFilter issue with version v1.4.3 installed via Homebrew.

Is this a possible regression?