diff-so-fancy 1.1.0 struggles with large/slow inputs

[alex]

Specifically, there appears to be a bug with buffering. Running env HGPLAIN=1 hg log | diff-so-fancy will hang forever as hg computes the log of everything, and diff-so-fancy doesn't display it (until the entire log is written to stdout).

I'm not sure if there's any other information I can provide which would help reproduce this.

[alex]

(This was not a problem before 1.1.0)

[scottchiefbaker]

We don't officially support hg yet. See #193

How big is the hg output?

[alex]

This repo has 350,000 commits, so pretty big :-) (It's the firefox codebase)

I know hg isn't officially supported, but before 1.1.0 it worked "well enough" :-)

[scottchiefbaker]

Actually I know why this is an issue now. We're parse the entirety of the input before we display anything (we didn't used to do this). This was part of moving to d-h internally instead of an external script. This is something we may change in the future.

[alex]

This seems to cause an issue on Sufficiently Large Git Repos (tm). e.g. git log -p now blocks for quite a while in a checkout for https://github.com/servo/servo

[scottchiefbaker]

Yes that's the same issue. I'll have to ponder a good solution.

In version 1.0 we shelled out to diff-highlight.pl for intraline highlighting. In version 1.1.0 we inlined d-h so we wouldn't have to shell out, and this is the side effect.

[scottchiefbaker]

This issue has less to do with hg and more with large input. Can you either:

1. Change the title to reflect that large input is the issue
2. Close this issue and open a new one with the appropriate title

This will help get eyes on the issue.

[alex]

@scottchiefbaker updated the title, let me know if you want me to update the original text as well

[scottchiefbaker]

Can you provide some benchmarks on before and after? If I git show on the Vim source code comparing 1.0.0 and 1.1.0 I get:

1.0.0 = ~54 seconds
1.1.0 = ~58 seconds

On really small input the difference is barely noticeable. Maybe I'm doing my tests wrong...?

# Test several thousand commits
time git show 281daf62aac8b40d1c6cccfb57535c5a1ac1f5db..HEAD | wc

[alex]

a) Are you sure diff-so-fancy is still run when piped to wc? I thought git might disable the pager when stdout isn't a tty

b) I'm not sure what you're measuring makes sense, time git show ... | wc measures the time to display everything, whereas the important thing is "time to first paint"

[scottchiefbaker]

Ahhhh that makes more sense... the delay is in DISPLAYING the data, not total output. Duh... Thus my testing was bad. I can see a big diff between 1.0.0 and 1.1.0 when you first see data on the screen of large diffs. Thanks for setting me straight.

[OJFord]

git doesn't disable the pager, but we did some work a while ago to get dsf to disable its affect, the motivation there being redirection to a patch file. Disabling when piping was actually a bad consequence of that, since it means the user can't modify their actual pager - the pipe to less and its options had to be brought inside dsf. I think it wasn't merged for that reason, so unless @scottchiefbaker found a different way of doing that, I think it must be running as usual when piping to wc.

-- Oliver Ford

On Tue, 18 Jul 2017, at 21:59, Alex Gaynor wrote: a) Are you sure diff-so-fancy is still run when piped to wc? I thought git might disable the pager when stdout isn't a tty> b) I'm not sure what you're measuring makes sense, time git show ... | wc measures the time to display everything, whereas the important thing is "time to first pain"> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub[1], or mute the thread[2]. >

Links: 1. #251 (comment) 2. https://github.com/notifications/unsubscribe-auth/ADw2Hr7Tj_ta6XokW_6lNTuLJwKytfknks5sPRywgaJpZM4OaNOd

[mpolden]

This makes diff-so-fancy unusable for me and I had to disable it. Even in a moderately sized repository with 15K commits, git log -p takes way too long.

I also noticed that diff-so-fancy now writes to my ~/.gitconfig. I (and I assume many others) have my dotfiles under version control and having programs silently manipulate my dotfiles is highly annoying.

[wwwdata]

I can also confirm the problem in very large git repos. For me also a normal log with statistic output is hanging forever git log --stat

[jasonventresca]

Yep, I'm having this issue too, with a large git repo.

Running git log -p was virtually instant before, and is now taking roughly a minute.

Keep up the great work, I love d-s-f 💯

[externl]

Same issue here (repo with 25,000 commits). Until this is fixed I've added this as a temporary workaround:

[core]
	pager = diff-so-fancy | less --tabs=4 -RFX
[pager]
	log  = diff-highlight | less
        show =  diff-so-fancy | less --tabs=4 -RFX
	diff =  diff-so-fancy | less --tabs=4 -RFX

This way git diff still uses diff-so-fancy and git log -p doesn't hang forever.

EDIT: clarification

[scottchiefbaker]

I believe I've fixed this issue in an unpublished alpha build. If this issue affects you please try that build and let me know if it solves this issue.

[jasonmp85]

Have you all considered pulling the release until this is fixed? This is a show-stopper, IMO.

[scottchiefbaker]

No...

d-s-f was never designed to work on HUGE inputs. It's main focus is making single diffs easily consumable by humans. It happens to work on large input like git show or git log but it's a side effect.

This will be fixed in the next version. Feel free to try the alpha build, or downgrade if this is a huge issue for you.

[externl]

@scottchiefbaker

If that is indeed not that case, maybe the instructions should be updated to not recommend using it for the pager, only for diff?

EDIT: Or is it now officially supporting "HUGE" inputs.

EDIT2: After thinking more about this, I often do "HUGE" diffs, so I think it's really all or nothing.

[jasonnoble]

@scottchiefbaker Regarding #251 (comment), the alpha build fixed the issue for me.

[jasonnoble]

@externl #251 (comment) didn't work for me, got diff-highlight: command not found. Could be related to #164.

[externl]

@jasonnoble, on macOS I do this:

[pager]
	log  = /usr/local/opt/git/share/git-core/contrib/diff-highlight/diff-highlight | less --tabs=4 -RFX

[rtlechow]

fwiw, ran into this as well (slow git-log), and the alpha build fixed it. 👍

[scottchiefbaker]

Time permitting I'll do an official release on this pretty soon.

[watsoncj]

I've verified the alpha-build resolves the slowness with large inputs. (git log -p)

[scottchiefbaker]

My biggest hold up right now is the changelog. If someone wants to go through the git history and pull out appropriate changelog stuff that'll help me get a new release out the door.

[boris-petrov]

@scottchiefbaker - perhaps you could use something like https://github.com/skywinder/github-changelog-generator?

[stevemao]

@scottchiefbaker Or you could try https://github.com/conventional-changelog/standard-version 😄

[richardjharris]

I think the readme should clearly indicate that diff-so-fancy is not suitable for git log -p users.

The current release will try to render the entire log, which is extremely slow for mature repos. The alpha-build dies part-way so my git log -p output only has about 10 commits in it. Even if there are errors rendering the diff, there should be a fallback so the log output is not truncated/corrupted.

[justo]

I would really love to see this fixed. git log -p is my main way of viewing logs, and it's just too slow to use now. I don't think I can go back to normal diffs haha.

[scottchiefbaker]

I'm closing this because a new release is pending ASAP.

[boris-petrov]

@scottchiefbaker - this is not fixed for me in 1.2.0. :( Am I missing something? 1.0.0 is the last version that works for me. :(

[scottchiefbaker]

@boris-petrov It must be just you?

I just tested git log -p on the Vim git repo which has 5072204 output lines and I was able to see d-s-f output right away. What command are you running?

[boris-petrov]

@scottchiefbaker - Actually, I've been installing 1.1.1. Why isn't 1.2.0 on NPM?

[scottchiefbaker]

@boris-petrov oh good call. I don't do NPM, I think @stevemao does that. Maybe he can update it.