Re: [PATCH v2 2/2] check-ignore.c, dir.c: fix segfault with '.' argument from repo root

From: Adam Spiers <git@adamspiers.org>
Date: Feb 20, 2013, 01:57 UTC
Message-ID: <20130220015723.GB7860@pacific.linksys.moosehall>
In-Reply-To: <7vfw0s3qsq.fsf@alter.siamese.dyndns.org>

On Tue, Feb 19, 2013 at 02:03:01PM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> I started to suspect that may be the right approach.  Why not do this?
> 
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Tue, 19 Feb 2013 11:56:44 -0800
> Subject: [PATCH] name-hash: allow hashing an empty string
> 
> Usually we do not pass an empty string to the function hash_name()
> because we almost always ask for hash values for a path that is a
> candidate to be added to the index. However, check-ignore (and most
> likely check-attr, but I didn't check) apparently has a callchain
> to ask the hash value for an empty path when it was given a "." from
> the top-level directory to ask "Is the path . excluded by default?"

According to a single gdb run, 'git check-attr -a .' does not hit hash_name() for me. However that naive experiment doesn't rule out the possibility of it happening if the right attributes are set, but I don't know enough about that code to comment.

Show 7 quoted lines

> Make sure that hash_name() does not overrun the end of the given
> pathname even when it is empty.
> 
> Remove a sweep-the-issue-under-the-rug conditional in check-ignore
> that avoided to pass an empty string to the callchain while at it.
> It is a valid question to ask for check-ignore if the top-level is
> set to be ignored by default, even though the answer is most likely

Hmm, I see very little difference between the use of "most likely" and the use of the words "much" and "typically" which you previously considered "a sure sign that the design of the fix is iffy".

> no, if only because there is currently no way to specify such an
> entry in the .gitignore file. But it is an unusual thing to ask and
> it is not worth optimizing for it by special casing at the top level
> of the call chain.

Although I agree with your proposed patch's sentiment of avoiding sweeping this corner case under the rug, 'check-ignore .' still wouldn't match anything if for example './' was a supported mechanism for ignoring the top level. So, modulo the obvious advantages that defensive coding in hash_name() bring, I'm struggling to see a significant difference between any of the three patches proposed so far. All of them fix the segfault, and all would require the same amount of extra work in order to support matching against './'.

But I don't have any strong objections either, so you have my approval for whichever patch you prefer to take. Thanks.