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

Doesn't work correctly when calling 'git --no-pager diff' #279

Closed
mortenfyhn opened this issue Jan 15, 2018 · 10 comments
Closed

Doesn't work correctly when calling 'git --no-pager diff' #279

mortenfyhn opened this issue Jan 15, 2018 · 10 comments

Comments

@mortenfyhn
Copy link

mortenfyhn commented Jan 15, 2018

First of all, thanks for this tool, I like it a lot.

So, maybe this is the expected behaviour. I'm using zsh, which by default activates the pager when running git diff. I don't like that, so my git diff aliases use the --no-pager option. From what I see, diff-so-fancy disables the pager again. I think it would then be better if running git diff and git --no-pager diff produced the same output.

You can see here what I'm getting:

screenshot from 2018-01-15 11-09-24

EDIT: I see now that diff-so-fancy uses the pager only when the diff doesn't fit in the terminal window, but I guess the issue still is valid.

@scottchiefbaker
Copy link
Contributor

I'm a little confused here... Are you implying that d-s-f is setting --no-pager for you? d-s-f prefers that the git pager be disabled, so it certainly doesn't enable it. What you're screenshot shows is a bunch of shell aliases, which d-s-f does not set. I suspect your shell is getting in the way and somehow setting those aliases that enable the pager.

You can grep the d-s-f code for pager and see that the only interaction we have with any type of pager is in our documentation listing how to disable it.

@mortenfyhn
Copy link
Author

My screenshot is a bit confusing. The aliases are mine, and the repo and file shown are arbitrary. I wanted to show that using --no-pager makes the output different. For instance, the word-delimited highlighting disappears, and the modified: <filename> with lines around too.

It can be avoided by removing without --no-pager, but it took me a while to realize, so I reckoned maybe some other are using aliases with --no-pager and getting the same issue.

@scottchiefbaker
Copy link
Contributor

I'm still not following 100% but it's starting to make more sense.

The top screenshot is a vanilla git diff, there is no d-s-f output on that. The bottom screenshot is git diff piped to d-s-f.

Ultimately this is not a d-s-f problem. It appears to be more of "git not calling d-s-f" problem. I'd investigate git, or your shell? I'm not sure what would cause --no-pager to not call d-s-f.

@scottchiefbaker
Copy link
Contributor

I stand corrected!

I just tested git --no-pager diff on my local repo and I get the same output that you do. d-s-f is never called when --no-pager is used. I suspect this has to do with the alias that we use to instantiate d-s-f.

git config --global core.pager "diff-so-fancy | less --tabs=4 -RFX"

Perhaps we need another alias for no-pager? I'm not super familiar with git aliases though.

@scottchiefbaker
Copy link
Contributor

For what it's worth, git --no-pager diff | diff-so-fancy works as you would expect.

The issue is that the internal git alias for --no-pager doesn't call d-s-f. I'm not a git expert, so I'm open to suggestions on a fix.

@mortenfyhn
Copy link
Author

I think even just mentioning it in the readme would be useful, I reckon that's where people first look if something isn't working.

@bew
Copy link

bew commented Mar 12, 2018

To have the correct behavior, I think you can use the following alias:

fancy-diff = "!f() { git diff $@ | diff-so-fancy; }; f"

Using it like: git fancy-diff a_file.txt or git --no-pager fancy-diff a_file.txt

@tucq88
Copy link

tucq88 commented Jan 2, 2019

My special case

image

Left is classic git diff
Right is git diff | diff-so-fancy

It's only be incorrect with last files, tried many times :(

@scottchiefbaker
Copy link
Contributor

@mortenfyhn if you write the doc update to explain this (you understand it better than I do) I'll gladly merge it.

@scottchiefbaker
Copy link
Contributor

Looks like this is fixed... perhaps a documentation update is needed. If someone writes up the doc updates I'll land them.

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

No branches or pull requests

4 participants