Re: The git spring cleanup challenge

[Phillip Wood <phillip.wood123@gmail.com>]

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.
>