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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 09/27/2012 11:27 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> @@ -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/