Re: The git spring cleanup challenge

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jun 3, 2021, 16:44 UTC
Message-ID: <87tume6g0g.fsf@evledraar.gmail.com>
In-Reply-To: <dbbb5a73-12b5-27a1-63d4-eece1e60f57b@gmail.com>

On Thu, Jun 03 2021, Phillip Wood wrote:

Show 19 quoted lines

> 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

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.

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.

Show 9 quoted lines

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