Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()

From: Duy Nguyen <pclouds@gmail.com>
Date: Jul 18, 2014, 10:49 UTC
Message-ID: <CACsJy8D5X5svApB9edHK+1EaGi+q2ZRSOvyDYaDieVV2psgZPg@mail.gmail.com>
In-Reply-To: <53C8562C.4000304@gmail.com>

On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees <karsten.blees@gmail.com> wrote:

Show 11 quoted lines

> Am 17.07.2014 19:05, schrieb René Scharfe:
>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:
> [...]
>> "These routines have traditionally been used by programs to save the
>> name of a working directory for the purpose of returning to it. A much
>> faster and less error-prone method of accomplishing this is to open the
>> current directory (.) and use the fchdir(2) function to return."
>>
>
> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not
> use realpath() directly (which would also be thread-safe)?

But realpath still needs a given buffer (of PATH_MAX at least again). Unless you pass a NULL pointer as "resolved_name", then Linux can allocate the buffer but that's implementation specific [1]. I guess we can make a wrapper "git_getcwd" that returns a new buffer. The default implementation returns one of PATH_MAX chars, certain platforms like Linux can use realpath (or something else) to return a longer path?

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html

Show 19 quoted lines

> For non-XSI-compliant platforms, we could keep the current implementation.
> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from
> lockfile.c to all path components.
>
>
> If I may bother you with the Windows point of view:
>
> There is no fchdir(), and I'm pretty sure open(".") won't work either.
>
> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).
> realpath() is pretty much what GetFinalPathNameByHandle() does. However,
> both of these APIs require Windows Vista.
>
> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,
> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be
> emulated in our mingw_open() wrapper, though).
>
> ...lots of work for little benefit, I would think.
>

We could wrap this "get cwd, cd back" pattern as well. So "save_cwd" returns an opaque pointer, "go_back" takes the pointer, (f)chdir back and free the pointer if needed. On platforms that support fchdir, it can be used, else we fall back to chdir. I think there are only four places that follow this pattern, here, setup.c (.git discovery), git.c (restore_env) and unix-socket.c. Enough call sites to make it worth the effort?

-- 
Duy