Re: [PATCH 0/7] Fix some bugs in abspath.c

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 4, 2012, 19:03 UTC
Message-ID: <7vehmha9uw.fsf@alter.siamese.dyndns.org>
In-Reply-To: <7va9x5bqz4.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> mhagger@alum.mit.edu writes:
>
>> Please note that both absolute_path("") and real_path("") used to
>> return the current directory followed by a slash.  I believe that this
>> was a bug, and that it is more appropriate for both functions to
>> reject the empty string.  The evidence is as follows:
>>
>> * If this were intended behavior, presumably the return value would
>>   *not* have a trailing slash.
>
> That is weak.  The only thing you can infer from that observation is
> that the presense or absense of trailing '/' would not make any
> difference to the caller who wanted a path to the cwd (and is more
> convenient if the call is made so that a path relative to the cwd is
> tucked after it).

I still wonder why you want to reject "" as an input, as it could be argued that it is a valid way to say the current directory.

What does realpath(3) do?

    ... goes and looks ...

The realpath(3) wants an existing path, and "" naturally gives NOENT, so we cannot really draw parallel from it.

Ok, let's do this again.

    $ ./test-path-utils absolute_path ""
    /srv/git/git.git/
    $ ./test-path-utils absolute_path "foo"
    /srv/git/git.git/foo

so a caller has to be prepared to see the returned path sometimes terminated with slash and sometimes without, to form an absolute path to the file "bar" in the directory it gives to these functions (i.e. "/srv/git/git.git/bar" vs "/srv/git/git.git/foo/bar").

That is a reasonably good sign that either nobody calls these functions with "" or existing callers are buggy and would not handle that case well and need to be fixed anyway.

So I am fine with die() in your patch.

Thanks.

Show 9 quoted lines

>> * I couldn't find any callers that appeared to depend on the old
>>   behavior.
>
> That is a very good argument (especially if the audit were
> thorough).
>
> I would be tempted to say that we should die() on "" for now, cook
> the result outside "master" for a few weeks while auditing the
> callchains, and see if any of them complains.