Re: [PATCH 1/8] Introduce new static function real_path_internal()

On 09/27/2012 11:27 PM, Junio C Hamano wrote:

Michael Haggerty [off-list ref] writes:

quoted

@@ -54,20 +73,36 @@ const char *real_path(const char *path)
 		}
 
 		if (*buf) {
-			if (!*cwd && !getcwd(cwd, sizeof(cwd)))
-				die_errno ("Could not get current working directory");
+			if (!*cwd && !getcwd(cwd, sizeof(cwd))) {
+				if (die_on_error)
+					die_errno("Could not get current working directory");
+				else
+					goto error_out;
+			}
 
-			if (chdir(buf))
-				die_errno ("Could not switch to '%s'", buf);
+			if (chdir(buf)) {
+				if (die_on_error)
+					die_errno("Could not switch to '%s'", buf);
+				else
+					goto error_out;
+			}
+		}

The patch makes sense, but while you are touching this code, I have
to wonder if there is an easy way to tell, before entering the loop,
if we have to chdir() around in the loop.  That would allow us to
hoist the getcwd() that is done only so that we can come back to
where we started outside the loop, making it clear why the call is
there, instead of cryptic "if (!*cwd &&" to ensure we do getcwd()
once and before doing any chdir().

I don't see an easy way to predict, before entering the loop, whether
chdir() will be needed.  For example, compare

    touch filename
    ln -s filename foo
    ./test-path-utils real_path foo

with

    touch filename
    ln -s $(pwd)/filename foo
    ./test-path-utils real_path foo

In the first case no chdir() is needed, whereas in the second case a
chdir() is needed but only on the second loop iteration.

All I can offer you is a palliative like the one below.

Michael

diff --git a/abspath.c b/abspath.c
index 5748b91..40cdc46 100644
--- a/abspath.c
+++ b/abspath.c

@@ -35,7 +35,14 @@ static const char *real_path_internal(const char

*path, int die_on_error)
 {
        static char bufs[2][PATH_MAX + 1], *buf = bufs[0], *next_buf =
bufs[1];
        char *retval = NULL;
+
+       /*
+        * If we have to temporarily chdir(), store the original CWD
+        * here so that we can chdir() back to it at the end of the
+        * function:
+        */
        char cwd[1024] = "";
+
        int buf_index = 1;

        int depth = MAXDEPTH;

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/