Re: [PATCH v2 3/8] absolute_path(): reject the empty string
On 09/07/2012 01:09 AM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- I think I asked why this matters (iow, why it is the right thing to do to reject an empty string, instead of treating it as the current directory) in the previous round. I would have expected to find the answer be above the S-o-b line here. The reasons that the change is desirable: 1. The empty string is not a legitimate path according to POSIX; e.g., see Linux's path_resolution(7): Empty pathname In the original UNIX, the empty pathname referred to the current directory. Nowadays POSIX decrees that an empty pathname must not be resolved successfully. Linux returns ENOENT in this case. Accordingly, comparable standard functions like realpath(3) reject the empty string. 2. The functions did not handle the empty path consistently with the way they handled other paths (namely, the return value contained a trailing slash). 3. This unusual behavior was undocumented. The above points let me to the conclusion that the anomalous handling of the empty string was a bug in the implementation rather than an intended behavior. Moreover, a quick check of callers didn't turn up any that seemed to rely on the strange behavior. Do you want a re-roll with this verbiage added to the commit messages of the two relevant commits? Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 v2 3/8] absolute_path(): reject the empty string
Michael Haggerty mhag...@alum.mit.edu writes: On 09/07/2012 01:09 AM, Junio C Hamano wrote: Michael Haggerty mhag...@alum.mit.edu writes: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- I think I asked why this matters (iow, why it is the right thing to do to reject an empty string, instead of treating it as the current directory) in the previous round. I would have expected to find the answer be above the S-o-b line here. The reasons that the change is desirable: 1. The empty string is not a legitimate path according to POSIX; e.g., see Linux's path_resolution(7): Empty pathname In the original UNIX, the empty pathname referred to the current directory. Nowadays POSIX decrees that an empty pathname must not be resolved successfully. Linux returns ENOENT in this case. Accordingly, comparable standard functions like realpath(3) reject the empty string. 2. The functions did not handle the empty path consistently with the way they handled other paths (namely, the return value contained a trailing slash). 3. This unusual behavior was undocumented. The above points let me to the conclusion that the anomalous handling of the empty string was a bug in the implementation rather than an intended behavior. Moreover, a quick check of callers didn't turn up any that seemed to rely on the strange behavior. Do you want a re-roll with this verbiage added to the commit messages of the two relevant commits? What the function used to do does not really matter when we are trying to see if the behaviour of the new implementation makes sense, even though it is a good supporting argument to help us judge that this change will not cause regressions to existing callers. A two sentence paragraph, with realpath(3) from POSIX.1 rejects an empty string as input as the primary justification, with no existing callers passes an empty string and expects to get the current directory as a supporting argument, should suffice, I would think. -- 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 v2 3/8] absolute_path(): reject the empty string
Michael Haggerty mhag...@alum.mit.edu writes:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
I think I asked why this matters (iow, why it is the right thing to
do to reject an empty string, instead of treating it as the current
directory) in the previous round. I would have expected to find
the answer be above the S-o-b line here.
The same comment applies to the other patch for real_path().
abspath.c | 4 +++-
t/t0060-path-utils.sh | 2 +-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/abspath.c b/abspath.c
index f04ac18..5d62430 100644
--- a/abspath.c
+++ b/abspath.c
@@ -123,7 +123,9 @@ const char *absolute_path(const char *path)
{
static char buf[PATH_MAX + 1];
- if (is_absolute_path(path)) {
+ if (!*path) {
+ die(The empty string is not a valid path);
+ } else if (is_absolute_path(path)) {
if (strlcpy(buf, path, PATH_MAX) = PATH_MAX)
die(Too long path: %.*s, 60, path);
} else {
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index d91e516..924aa60 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -140,7 +140,7 @@ test_expect_success 'strip_path_suffix' '
c:/msysgit/libexec//git-core libexec/git-core)
'
-test_expect_failure 'absolute path rejects the empty string' '
+test_expect_success 'absolute path rejects the empty string' '
test_must_fail test-path-utils absolute_path
'
--
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
