Re: Bug: .gitconfig folder

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2015, 17:06 UTC
Message-ID: <xmqqbnh4iqcc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20150528075142.GB3688@peff.net>

Jeff King <peff@peff.net> writes:

Show 17 quoted lines

> On Wed, May 27, 2015 at 03:38:12PM -0700, Junio C Hamano wrote:
>
>> The patch was meant to be a tongue-in-cheek tangent that is a vast
>> improvement for cases where we absolutely need to use mmap but does
>> not help the OP at all ;-)  I do not think there is any need for the
>> config reader to read the existing file via mmap interface; just
>> open it, strbuf_read() the whole thing (and complain when it cannot)
>> and we should be ok.
>> 
>> Or do we write back through the mmaped region or something?
>
> No, I think we must never do that in our code because our compat mmap
> implementation uses pread(). So all maps must be MAP_PRIVATE (and our
> compat mmap barfs if it is not).
>
> I started to go the strbuf_read() route, but it just felt so dirty to
> change the way the code works only to try to get a better error message.

Hmm. I actually thought that we long time ago updated the system to read small loose object files via read(2) instead of mmap(2) purely as an optimization, as mmap(2) is a bad match if you are going to read the whole thing from the beginning to the end anyway, and the "why not strbuf_read() the whole configuration file" was a suggestion along that line.

But apparently we do not have such an optimization in read_object() codepath, perhaps I was hallucinating X-<.

> ... but the config-writing code is such a tangled
> mess that I don't want to spend the time or risk the regressions.

That part I agree with. I was kinda hoping that the previous GSoC would clean it up, but that did not happen.