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

From: Adam Spiers <git@adamspiers.org>
Date: Feb 19, 2013, 19:07 UTC
Message-ID: <CAOkDyE9VVuFn6B=Fe4XHxGCEW0MFgndx1X0+9hO36Soxb37YQw@mail.gmail.com>
In-Reply-To: <7v1ucc6vgd.fsf@alter.siamese.dyndns.org>

On Tue, Feb 19, 2013 at 5:54 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Adam Spiers <git@adamspiers.org> writes:
>
>> Fix a corner case where check-ignore would segfault when run with the
>> '.' argument from the top level of a repository, due to prefix_path()
>> converting '.' into the empty string.  It doesn't make much sense to
>> call check-ignore from the top level with '.' as a parameter, since
>> the top-level directory would never typically be ignored, but of
>> course it should not segfault in this case.
>>
>> Signed-off-by: Adam Spiers <git@adamspiers.org>
>> ---
>
> Please step back a bit and explain why the original had check for
> path[0] in the first place?

I can't remember to be honest.

> If the answer is "the code wanted to special case the question 'is
> the top-level excluded?',

Yes, I think that's the most likely explanation. Maybe it got missed in a variable renaming refactoring.

Show 16 quoted lines

> but used a wrong variable to implement the
> check, and this patch is a fix to that", then the proposed commit
> log message looks incomplete.  The cause of the segv is not that
> prefix_path() returns an empty string, but because the function
> called inside the "if" block was written without expecting to be fed
> the path that refers to the top-level of the working tree, no?
>
> While this change certainly will prevent the "check the top-level"
> request to last-exclude-matching-path, I have to wonder if it is a
> good idea to force the caller of the l-e-m-p function to even care.
>
> In other words, would it be a cleaner approach to fix the l-e-m-p
> function so that the caller can ask "check the top-level" and give a
> sensible answer (perhaps the answer may be "nothing matches"), and
> remove the "&& path[0]" (or "&& full_path[0]") special case from
> this call site?

Yes, that did cross my mind. I also wondered whether hash_name() should do stricter input validation, but I guess that could have an impact on performance.

> The last sentence "It doesn't make much sense..." in the proposed
> log message would become a good justification for such a special
> case at the beginning of l-e-m-p function, I would think.

Fair enough.  I'll reply to this with a new version.[0]

[0] I wish there was a clean way to include the new version inline,
    but as I've noted before, there doesn't seem to be:

    http://article.gmane.org/gmane.comp.version-control.git/146110