Re: [PATCH 0/7] Fix some bugs in abspath.c
Junio C Hamano writes:
> mhag...@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.
>> * 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/7] Fix some bugs in abspath.c
mhag...@alum.mit.edu writes:
> From: Michael Haggerty
>
> I really just wanted to tidy up filter_refs(), but I've been sucked
> into a cascade of recursive yak shaving. This is my first attempt to
> pop the yak stack.
Thanks.
> 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 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] Fix some bugs in abspath.c
From: Michael Haggerty
I really just wanted to tidy up filter_refs(), but I've been sucked
into a cascade of recursive yak shaving. This is my first attempt to
pop the yak stack.
I want to use real_path() for making the handling of
GIT_CEILING_DIRECTORIES more robust, but I noticed that it is broken
for some cases that will be important in this context. This patch
series adds some new tests of that function and fixes the ones that
are broken, including a similar breakage in absolute_path(). It
applies to the current master.
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.
* I couldn't find any callers that appeared to depend on the old
behavior.
* The test suite runs without errors after the change.
But I didn't do a thorough code review to ensure that no callers ever
rely on the old behavior. The most likely scenario would be if one of
these functions were used to parse something like $PATH, where the
empty string denotes the current directory, but I didn't see any
callers that seemed to be doing anything like this.
Michael Haggerty (7):
t: verify that absolute_path() fails if passed the empty string
absolute_path(): reject the empty string
t: verify that real_path() fails if passed the empty string
real_path(): reject the empty string
t: verify that real_path() works correctly with absolute paths
real_path(): properly handle nonexistent top-level paths
t: verify that real_path() removes extra slashes
abspath.c| 12 ++--
t/t-basic.sh | 31 ++-
2 files changed, 40 insertions(+), 3 deletions(-)
--
1.7.11.3
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
