Hi Ævar
On 03/06/2021 17:44, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jun 03 2021, Phillip Wood wrote:
Hi Ævar
On 03/06/2021 13:31, Ævar Arnfjörð Bjarmason wrote:
On Thu, Jun 03 2021, Felipe Contreras wrote:
Ævar Arnfjörð Bjarmason wrote:
So I skipped the "disable most config", but for what it's worth I think
I'd miss these the most, I couldn't pick just N favorites, sorry:
* diff.colorMoved=true: super useful, but I'd be vary of turning it on
by default in its current form. E.g. on gcc.git's changelog files it
has really pathological performance characteristics.
Would you be able say a bit more about this so I can try and reproduce
it please. I'm working on some patches [1] to improve the performance
of `diff --color-moved` and `diff --color-moved-ws` and it would be
good to test them on a problematic repo. At the moment I diffing two
releases of git to test performance on a large diff. I just cloned the
last 18 months of gcc.git and Changelog seems to just be appended to.
I misremembered the gcc.git ChangeLog issue, sorry. That's about
something else entirely.
The issue with the color moved code can just be reproduced on most large
diffs, e.g. on git.git:
$ time git diff --color-moved=true v2.25.0..v2.30.0 >/dev/null
real 0m10.112s
$ time git diff --color-moved=false v2.25.0..v2.30.0 >/dev/null
real 0m0.939s
"Big diffs are slow with --color-moved" is the problem I've been
focusing on. With my patches I see the time for --color-moved=true go
down from 16s to 4.3s for that example. --color-moved=false takes 0.8s
so the --color-moved=true is still quite a bit slower but it's not as
bad. --color-moved-ws=allow-indentation-change goes from 8 minutes(!) to
6 seconds. I'm seeing a slight (few percent) slowdown for `git log
--patch --no-merges -n1000` though which I'd like to avoid.
So 10x slower, and e.g. diffing from v2.22.0 makes it 25s and 1.1s,
respectively.
In some sense that slowness is expected, it simly takes time to compute
this. I think for turning it on by default we should have something like
the diff.renameLimit, and change the default to some "auto" that would
punt out if it was taking too long to compute.
I run with it by default so this doesn't bother me, but I think it's
probably a semi-common use-case of some people to e.g. diff the last N
releases of Linux, and then use their pager to search around in the
diff.
Yeah I can see people doing that.
We don't want commands like that to take 25s instead of 1s, but I think
it would be fine (and we should) warn that we aborted on the color move
if it's otherwise the default.
Otherwise it'll take 1s, and if you normally depend on it you'll
conclude that some code you're looking at wasn't moved, which would also
suck, better to punt on it and warn, just like the diff.renameLimit.
That's a nice idea, one other thought I had was to fall back to just
looking for moved lines within the same file - that would be much faster
on a large diff with lots of files but maybe it is confusing to
highlight only some of the moved lines when there are interfile as well
as intrafile moves.
Best Wishes
Phillip
Very nice! I didn't know about it. I'll pick it for my third day.
It makes patch review a lot easier, and also integrates nicely with
-w.
I find --color-moved-ws=allow-indentation-change helpful as well (it
is quite a bit slower at the moment but I have some patches to bring
it up to the speed of the other --color-moved modes.
[1] https://github.com/phillipwood/git/tree/wip/diff-color-moved-tweaks
Nice, I didn't know about that option. Will try it.
