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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 29, 2012, 05:40 UTC
Message-ID: <7vy5jtidyd.fsf@alter.siamese.dyndns.org>
In-Reply-To: <50667F7E.7040903@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 23 quoted lines

>> 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.

Thanks for an example, and it is perfectly OK if we really have to have that "only fires once and only if needed" logic in the loop.

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

I think that is an improvement; or we could rename it not cwd[] but some other name that includes the word "original" in it.

Thanks.

Show 23 quoted lines

> 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;