Re: [PATCH v2] read_index_from(): Skip verification of the cache entry order to speed index loading

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2017, 01:49 UTC
Message-ID: <xmqq8tfs3x3m.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <11666ccf-6406-d585-f519-7a1934c2973a@gmail.com>

Ben Peart <peartben@gmail.com> writes:

Show 8 quoted lines

> Any updates or thoughts on this one?  While the patch has become quite
> trivial, it does results in a savings of 5%-15% in index load time.
>
> I thought the compromise of having this test only run when DEBUG is
> defined should limit it to developer builds (hopefully everyone
> developing on git is running DEBUG builds :)).  Since the test is
> trying to detect buggy code when writing the index, I thought that was
> the right time to test/catch any issues.

This check is more about catching a possible breakage (and a malicious repository) early before we go too far into the operation. I do not think this check is about debugging the implementation of Git. How would it be useful to turn it on in DEBUG build?

While I do think pursuing any runtime improvements better than a couple of percents is worth it, I do not think the approach taken by this iteration makes much sense; the previous one that at least allowed fsck to catch breakage may have been already too leaky to catch real issues (i.e. when you are asked to visit and look at an unknown repository, you wouldn't start your session with "git fsck" to protect yourself), and this round makes it much worse.

Besides, I see no -DDEBUG from "grep -e '-D[A-Z]*DEBUG' Makefile". If this check were about allowing us easier time debugging the binary (which I do not think it is), this probably should be '#ifndef NDEBUG' instead.