{"thread":{"id":"37149","subject":"[PATCH] abspath.c: use PATH_MAX in real_path_internal()","startedAt":"2014-07-17T12:45:43Z","lastAt":"2014-07-21T02:25:01Z","messageCount":17,"participants":["Nguyễn Thái Ngọc Duy","René Scharfe","Junio C Hamano","Karsten Blees","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246226","messageId":"1405601143-31354-1-git-send-email-pclouds@gmail.com","threadId":"37149","inReplyTo":null,"subject":"[PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-17T12:45:43Z","receivedAt":"2014-07-17T12:45:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This array 'cwd' is used to store the result from getcwd() and chdir()\nback. PATH_MAX is the right constant for the job. On systems with\nlonger PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,\ne.g. \"git init\". Make it static too to reduce stack usage.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n abspath.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex ca33558..c0c868f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t * here so that we can chdir() back to it at the end of the\n \t * function:\n \t */\n-\tchar cwd[1024] = \"\";\n+\tstatic char cwd[PATH_MAX];\n \n \tint buf_index = 1;\n \n@@ -49,6 +49,8 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \tchar *last_elem = NULL;\n \tstruct stat st;\n \n+\t*cwd = '\\0';\n+\n \t/* We've already done it */\n \tif (path == buf || path == next_buf)\n \t\treturn path;\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246251","messageId":"53C80265.5030903@web.de","threadId":"37149","inReplyTo":"1405601143-31354-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-17T17:05:41Z","receivedAt":"2014-07-17T17:05:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n> This array 'cwd' is used to store the result from getcwd() and chdir()\n> back. PATH_MAX is the right constant for the job.\n\nPATH_MAX may be better than 1024, but there can't really be a correct\nconstant.  The actual limit depends on the file system.\n\n> On systems with\n> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,\n> e.g. \"git init\". \n\nOut of curiosity, I just created a path with over 130000 characters on\nLinux.  It's not actually usable but it shows that 4096 is not a real\nlimit on Linux.  Here's how I created that insane path:\n\n\ta=1234567890\n\tx=$a$a$a$a$a\n\ty=$x$x$x$x$x\n\tcd /tmp\n\twhile true\n\tdo\n\t\tmkdir $y || break\n\t\tcd $y || break\n\tdone\n\tpwd >/tmp/y\n\tcd /tmp\n\twc -c <y\n\nAnother experiment with the program listed below shows that getcwd() on\nLinux works fine with such paths if the provided buffer is large\nenough, even though PATH_MAX is 4096:\n\n\t#include <limits.h>\n\t#include <stdio.h>\n\t#include <string.h>\n\t#include <unistd.h>\n\tint main(int argc, char **argv)\n\t{\n\t\tchar cwd[200000];\n\t\tprintf(\"PATH_MAX = %d\\n\", PATH_MAX);\n\t\tif (getcwd(cwd, sizeof(cwd)))\n\t\t\tprintf(\"strlen(getcwd()) = %zu\\n\", strlen(cwd));\n\t\treturn 0;\n\t}\n\n> Make it static too to reduce stack usage.\n\nWhy is that needed?  Is recursion involved?  (Didn't find any in the\nfunction itself after a very brief look.)\n\n\nThere is get_current_dir_name(), a GNU extension that allocates the\nnecessary memory.  Perhaps we can use it instead and provide a\ncompatibility implementation based on getcwd() for platforms that don't\nhave it?\n\nBut then there's also this advice from the getcwd(3) manpage on OpenBSD:\n\n\"These routines have traditionally been used by programs to save the\nname of a working directory for the purpose of returning to it. A much\nfaster and less error-prone method of accomplishing this is to open the\ncurrent directory (.) and use the fchdir(2) function to return.\"\n\nSo, how about something like this?\n\n---\n abspath.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex ca33558..7fff13a 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \n \t/*\n \t * If we have to temporarily chdir(), store the original CWD\n-\t * here so that we can chdir() back to it at the end of the\n+\t * here so that we can fchdir() back to it at the end of the\n \t * function:\n \t */\n-\tchar cwd[1024] = \"\";\n+\tint cwd_fd = -1;\n \n \tint buf_index = 1;\n \n@@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t\t}\n \n \t\tif (*buf) {\n-\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n+\t\t\tif (cwd_fd < 0)\n+\t\t\t\tcwd_fd = open(\".\", O_RDONLY);\n+\t\t\tif (cwd_fd < 0) {\n \t\t\t\tif (die_on_error)\n \t\t\t\t\tdie_errno(\"Could not get current working directory\");\n \t\t\t\telse\n@@ -142,8 +144,11 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \tretval = buf;\n error_out:\n \tfree(last_elem);\n-\tif (*cwd && chdir(cwd))\n-\t\tdie_errno(\"Could not change back to '%s'\", cwd);\n+\tif (cwd_fd >= 0) {\n+\t\tif (fchdir(cwd_fd))\n+\t\t\tdie_errno(\"Could not change back to the original working directory\");\n+\t\tclose(cwd_fd);\n+\t}\n \n \treturn retval;\n }\n-- \n2.0.2\n"},{"id":"246255","messageId":"xmqqoawnq2vv.fsf@gitster.dls.corp.google.com","threadId":"37149","inReplyTo":"1405601143-31354-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-17T18:03:16Z","receivedAt":"2014-07-17T18:03:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> This array 'cwd' is used to store the result from getcwd() and chdir()\n> back. PATH_MAX is the right constant for the job. On systems with\n> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,\n> e.g. \"git init\". Make it static too to reduce stack usage.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> ---\n\nThanks.  It seems that this 1024 has been with us since the\nbeginning of this piece of code.  I briefly wondered if there are\nstrange platform that will have PATH_MAX shorter than 1024 that will\nbe hurt by this change, but the result in cwd[] is used to grow the\nfinal result bufs[] that is sized based on PATH_MAX anyway, so it\nwill not be an issue (besides, the absurdly short one seems to be\na different macro, MAX_PATH, on Windows).\n\nWill queue.\n\n>  abspath.c | 4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/abspath.c b/abspath.c\n> index ca33558..c0c868f 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)\n>  \t * here so that we can chdir() back to it at the end of the\n>  \t * function:\n>  \t */\n> -\tchar cwd[1024] = \"\";\n> +\tstatic char cwd[PATH_MAX];\n>  \n>  \tint buf_index = 1;\n>  \n> @@ -49,6 +49,8 @@ static const char *real_path_internal(const char *path, int die_on_error)\n>  \tchar *last_elem = NULL;\n>  \tstruct stat st;\n>  \n> +\t*cwd = '\\0';\n> +\n>  \t/* We've already done it */\n>  \tif (path == buf || path == next_buf)\n>  \t\treturn path;\n"},{"id":"246257","messageId":"xmqqk37bq2f3.fsf@gitster.dls.corp.google.com","threadId":"37149","inReplyTo":"53C80265.5030903@web.de","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-17T18:13:20Z","receivedAt":"2014-07-17T18:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> \"These routines have traditionally been used by programs to save the\n> name of a working directory for the purpose of returning to it. A much\n> faster and less error-prone method of accomplishing this is to open the\n> current directory (.) and use the fchdir(2) function to return.\"\n>\n> So, how about something like this?\n\nYeah, I've always wanted to see us use fchdir() for coming back\n(another thing I wanted to see is to use openat() and friends).\n\nI do not offhand recall if the run of chdir() in gitdir discovery\ncode has a similar \"now we are done, let's go back to the original\"\nuse of chdir() there, but if we do, we should fix it, too.\n\nLooks sensible from a cursory read.\n\n>\n> ---\n>  abspath.c | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/abspath.c b/abspath.c\n> index ca33558..7fff13a 100644\n> --- a/abspath.c\n> +++ b/abspath.c\n> @@ -38,10 +38,10 @@ static const char *real_path_internal(const char *path, int die_on_error)\n>  \n>  \t/*\n>  \t * If we have to temporarily chdir(), store the original CWD\n> -\t * here so that we can chdir() back to it at the end of the\n> +\t * here so that we can fchdir() back to it at the end of the\n>  \t * function:\n>  \t */\n> -\tchar cwd[1024] = \"\";\n> +\tint cwd_fd = -1;\n>  \n>  \tint buf_index = 1;\n>  \n> @@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int die_on_error)\n>  \t\t}\n>  \n>  \t\tif (*buf) {\n> -\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n> +\t\t\tif (cwd_fd < 0)\n> +\t\t\t\tcwd_fd = open(\".\", O_RDONLY);\n> +\t\t\tif (cwd_fd < 0) {\n>  \t\t\t\tif (die_on_error)\n>  \t\t\t\t\tdie_errno(\"Could not get current working directory\");\n>  \t\t\t\telse\n> @@ -142,8 +144,11 @@ static const char *real_path_internal(const char *path, int die_on_error)\n>  \tretval = buf;\n>  error_out:\n>  \tfree(last_elem);\n> -\tif (*cwd && chdir(cwd))\n> -\t\tdie_errno(\"Could not change back to '%s'\", cwd);\n> +\tif (cwd_fd >= 0) {\n> +\t\tif (fchdir(cwd_fd))\n> +\t\t\tdie_errno(\"Could not change back to the original working directory\");\n> +\t\tclose(cwd_fd);\n> +\t}\n>  \n>  \treturn retval;\n>  }\n"},{"id":"246276","messageId":"53C8561F.5090600@gmail.com","threadId":"37149","inReplyTo":"xmqqoawnq2vv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-07-17T23:02:55Z","receivedAt":"2014-07-17T23:02:55Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 17.07.2014 20:03, schrieb Junio C Hamano:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n> \n>> This array 'cwd' is used to store the result from getcwd() and chdir()\n>> back. PATH_MAX is the right constant for the job. On systems with\n>> longer PATH_MAX (eg. 4096 on Linux), hard coding 1024 fails stuff,\n>> e.g. \"git init\". Make it static too to reduce stack usage.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n>> ---\n> \n> Thanks.  It seems that this 1024 has been with us since the\n> beginning of this piece of code.  I briefly wondered if there are\n> strange platform that will have PATH_MAX shorter than 1024 that will\n> be hurt by this change, but the result in cwd[] is used to grow the\n> final result bufs[] that is sized based on PATH_MAX anyway, so it\n> will not be an issue (besides, the absurdly short one seems to be\n> a different macro, MAX_PATH, on Windows).\n> \n\nIndeed, there's a strange platform where PATH_MAX is only 259. With\nUnicode support, the current directory can be that many wide characters,\nwhich means up to 3 * PATH_MAX UTF-8 bytes (if all of them are CJK).\n\nI don't think this will be a problem, though, as the return buffer is\nPATH_MAX-bounded as well.\n"},{"id":"246277","messageId":"53C85625.6020808@gmail.com","threadId":"37149","inReplyTo":"1405601143-31354-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-07-17T23:03:01Z","receivedAt":"2014-07-17T23:03:01Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n> e.g. \"git init\". Make it static too to reduce stack usage.\n\nBut wouldn't this increase overall memory usage? Stack memory\nwill be reused by subsequent code, while static memory cannot\nbe reused (but still increases the process working set).\n"},{"id":"246278","messageId":"53C8562C.4000304@gmail.com","threadId":"37149","inReplyTo":"53C80265.5030903@web.de","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-07-17T23:03:08Z","receivedAt":"2014-07-17T23:03:08Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 17.07.2014 19:05, schrieb René Scharfe:\n> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n[...]\n> \"These routines have traditionally been used by programs to save the\n> name of a working directory for the purpose of returning to it. A much\n> faster and less error-prone method of accomplishing this is to open the\n> current directory (.) and use the fchdir(2) function to return.\"\n> \n\nfchdir() is part of the POSIX-XSI extension, as is realpath(). So why not\nuse realpath() directly (which would also be thread-safe)?\n\nFor non-XSI-compliant platforms, we could keep the current implementation.\nOr re-implement a thread-safe version, e.g. applying resolve_symlink() from\nlockfile.c to all path components.\n\n\nIf I may bother you with the Windows point of view: \n\nThere is no fchdir(), and I'm pretty sure open(\".\") won't work either.\n\nfchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).\nrealpath() is pretty much what GetFinalPathNameByHandle() does. However,\nboth of these APIs require Windows Vista.\n\nOpening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,\nwhich AFAICT MSVCRT.dll's open() implementation does _not_ do (could be\nemulated in our mingw_open() wrapper, though).\n\n...lots of work for little benefit, I would think.\n"},{"id":"246301","messageId":"CACsJy8D5X5svApB9edHK+1EaGi+q2ZRSOvyDYaDieVV2psgZPg@mail.gmail.com","threadId":"37149","inReplyTo":"53C8562C.4000304@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-18T10:49:04Z","receivedAt":"2014-07-18T10:49:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees <karsten.blees@gmail.com> wrote:\n> Am 17.07.2014 19:05, schrieb René Scharfe:\n>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n> [...]\n>> \"These routines have traditionally been used by programs to save the\n>> name of a working directory for the purpose of returning to it. A much\n>> faster and less error-prone method of accomplishing this is to open the\n>> current directory (.) and use the fchdir(2) function to return.\"\n>>\n>\n> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not\n> use realpath() directly (which would also be thread-safe)?\n\nBut realpath still needs a given buffer (of PATH_MAX at least again).\nUnless you pass a NULL pointer as \"resolved_name\", then Linux can\nallocate the buffer but that's implementation specific [1]. I guess we\ncan make a wrapper \"git_getcwd\" that returns a new buffer. The default\nimplementation returns one of PATH_MAX chars, certain platforms like\nLinux can use realpath (or something else) to return a longer path?\n\n[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html\n\n> For non-XSI-compliant platforms, we could keep the current implementation.\n> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from\n> lockfile.c to all path components.\n>\n>\n> If I may bother you with the Windows point of view:\n>\n> There is no fchdir(), and I'm pretty sure open(\".\") won't work either.\n>\n> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).\n> realpath() is pretty much what GetFinalPathNameByHandle() does. However,\n> both of these APIs require Windows Vista.\n>\n> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,\n> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be\n> emulated in our mingw_open() wrapper, though).\n>\n> ...lots of work for little benefit, I would think.\n>\n\nWe could wrap this \"get cwd, cd back\" pattern as well. So \"save_cwd\"\nreturns an opaque pointer, \"go_back\" takes the pointer, (f)chdir back\nand free the pointer if needed. On platforms that support fchdir, it\ncan be used, else we fall back to chdir. I think there are only four\nplaces that follow this pattern, here, setup.c (.git discovery), git.c\n(restore_env) and unix-socket.c. Enough call sites to make it worth\nthe effort?\n-- \nDuy\n"},{"id":"246303","messageId":"53C905EB.3010908@web.de","threadId":"37149","inReplyTo":"53C8562C.4000304@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T11:32:59Z","receivedAt":"2014-07-18T11:32:59Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.2014 01:03, schrieb Karsten Blees:\n> Am 17.07.2014 19:05, schrieb René Scharfe:\n>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n> [...]\n>> \"These routines have traditionally been used by programs to save the\n>> name of a working directory for the purpose of returning to it. A much\n>> faster and less error-prone method of accomplishing this is to open the\n>> current directory (.) and use the fchdir(2) function to return.\"\n>>\n>\n> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not\n> use realpath() directly (which would also be thread-safe)?\n\nThat's a good question; thanks for stepping back and looking at the \nbigger picture.  If there is widespread OS support for a functionality \nthen we should use it and just provide a compatibility implementation \nfor those platforms lacking it.  The downside is that compat code gets \nless testing.\n\nSeeing that readlink() is left as a stub in compat/mingw.h that only \nerrors out, would the equivalent function on Windows be PathCanonicalize \n(http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?\n\n> For non-XSI-compliant platforms, we could keep the current implementation.\n\nOK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping \nPathCanonicalize() for Windows and the current code for the rest?\n\n> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from\n> lockfile.c to all path components.\n\nThread safety sounds good.  We'd also need something like \nnormalize_path_copy() but without the conversion of backslashes to \nslashes, in order to get rid of \".\" and \"..\" path components and \nsomething like absolute_path() that doesn't die on error, no?\n\n> If I may bother you with the Windows point of view:\n>\n> There is no fchdir(), and I'm pretty sure open(\".\") won't work either.\n\nOn Windows, there *is* an absolute path length limit of 260 in the \nnormal case and a bit more than 32000 for some functions using the \\\\?\\ \nnamespace.  So one could get away with using a constant-sized buffer for \na \"remember the place and return later\" function here.\n\nAlso, _getcwd can be asked to allocate an appropriately-sized buffer for \nuse, like GNU's get_current_dir_name, by specifying NULL as its first \nparameter (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).\n\nNot having to move around at all as mentioned above is still better, of \ncourse.\n\nRené\n"},{"id":"246312","messageId":"53C9387D.4090504@web.de","threadId":"37149","inReplyTo":"CACsJy8D5X5svApB9edHK+1EaGi+q2ZRSOvyDYaDieVV2psgZPg@mail.gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-18T15:08:45Z","receivedAt":"2014-07-18T15:08:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.07.2014 12:49, schrieb Duy Nguyen:\n> On Fri, Jul 18, 2014 at 6:03 AM, Karsten Blees <karsten.blees@gmail.com> wrote:\n>> Am 17.07.2014 19:05, schrieb René Scharfe:\n>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n>> [...]\n>>> \"These routines have traditionally been used by programs to save the\n>>> name of a working directory for the purpose of returning to it. A much\n>>> faster and less error-prone method of accomplishing this is to open the\n>>> current directory (.) and use the fchdir(2) function to return.\"\n>>>\n>>\n>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not\n>> use realpath() directly (which would also be thread-safe)?\n> \n> But realpath still needs a given buffer (of PATH_MAX at least again).\n> Unless you pass a NULL pointer as \"resolved_name\", then Linux can\n> allocate the buffer but that's implementation specific [1]. I guess we\n> can make a wrapper \"git_getcwd\" that returns a new buffer. The default\n> implementation returns one of PATH_MAX chars, certain platforms like\n> Linux can use realpath (or something else) to return a longer path?\n> \n> [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html\n\nrealpath() can be used to implement the whole of real_path_internal(),\nIIUC, so there would be no need to change the current directory anymore.\n\nThe realpath(3) manpage for Linux mentions that behaviour of realpath()\nwith resolved_path being NULL is not standardized by POSIX.1-2001 but\nby POSIX.1-2008.  At least Linux, OpenBSD and FreeBSD seem to support\nthat, based on their manpages.  Here's the standard doc:\n\nhttp://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html\n\n>> For non-XSI-compliant platforms, we could keep the current implementation.\n>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from\n>> lockfile.c to all path components.\n>>\n>>\n>> If I may bother you with the Windows point of view:\n>>\n>> There is no fchdir(), and I'm pretty sure open(\".\") won't work either.\n>>\n>> fchdir() could be emulated using GetFileInformationByHandleEx(FileNameInfo).\n>> realpath() is pretty much what GetFinalPathNameByHandle() does. However,\n>> both of these APIs require Windows Vista.\n>>\n>> Opening a handle to a directory can be done using FILE_FLAG_BACKUP_SEMANTICS,\n>> which AFAICT MSVCRT.dll's open() implementation does _not_ do (could be\n>> emulated in our mingw_open() wrapper, though).\n>>\n>> ...lots of work for little benefit, I would think.\n>>\n> \n> We could wrap this \"get cwd, cd back\" pattern as well. So \"save_cwd\"\n> returns an opaque pointer, \"go_back\" takes the pointer, (f)chdir back\n> and free the pointer if needed. On platforms that support fchdir, it\n> can be used, else we fall back to chdir. I think there are only four\n> places that follow this pattern, here, setup.c (.git discovery), git.c\n> (restore_env) and unix-socket.c. Enough call sites to make it worth\n> the effort?\n\nOn a cursory look I didn't find any more potential users.  Something\nlike this?  Applying it to setup.c looks difficult to impossible,\nthough.\n\n---\n Makefile          |  8 ++++++++\n abspath.c         | 10 ++++++----\n cache.h           |  1 +\n git.c             |  9 +++++----\n save-cwd-fchdir.c | 25 +++++++++++++++++++++++++\n save-cwd.c        | 22 ++++++++++++++++++++++\n save-cwd.h        |  3 +++\n unix-socket.c     | 11 ++++++-----\n 8 files changed, 76 insertions(+), 13 deletions(-)\n create mode 100644 save-cwd-fchdir.c\n create mode 100644 save-cwd.c\n create mode 100644 save-cwd.h\n\ndiff --git a/Makefile b/Makefile\nindex 840057c..ecf3129 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -32,6 +32,8 @@ all::\n #\n # Define HAVE_ALLOCA_H if you have working alloca(3) defined in that header.\n #\n+# Define HAVE_FCHDIR if you have fchdir(2).\n+#\n # Define NO_CURL if you do not have libcurl installed.  git-http-fetch and\n # git-http-push are not built, and you cannot use http:// and https://\n # transports (neither smart nor dumb).\n@@ -1118,6 +1120,12 @@ ifdef HAVE_ALLOCA_H\n \tBASIC_CFLAGS += -DHAVE_ALLOCA_H\n endif\n \n+ifdef HAVE_FCHDIR\n+\tCOMPAT_OBJS += save-cwd-fchdir.o\n+else\n+\tCOMPAT_OBJS += save-cwd.o\n+endif\n+\n ifdef NO_CURL\n \tBASIC_CFLAGS += -DNO_CURL\n \tREMOTE_CURL_PRIMARY =\ndiff --git a/abspath.c b/abspath.c\nindex ca33558..f49bacc 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -41,7 +41,7 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t * here so that we can chdir() back to it at the end of the\n \t * function:\n \t */\n-\tchar cwd[1024] = \"\";\n+\tstruct saved_cwd *cwd = NULL;\n \n \tint buf_index = 1;\n \n@@ -80,7 +80,9 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t\t}\n \n \t\tif (*buf) {\n-\t\t\tif (!*cwd && !getcwd(cwd, sizeof(cwd))) {\n+\t\t\tif (!cwd)\n+\t\t\t\tcwd = save_cwd();\n+\t\t\tif (!cwd) {\n \t\t\t\tif (die_on_error)\n \t\t\t\t\tdie_errno(\"Could not get current working directory\");\n \t\t\t\telse\n@@ -142,8 +144,8 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \tretval = buf;\n error_out:\n \tfree(last_elem);\n-\tif (*cwd && chdir(cwd))\n-\t\tdie_errno(\"Could not change back to '%s'\", cwd);\n+\tif (cwd && restore_cwd(cwd))\n+\t\tdie_errno(\"Could not change back to the original working directory\");\n \n \treturn retval;\n }\ndiff --git a/cache.h b/cache.h\nindex 92fc9f1..5b4e9cd 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -7,6 +7,7 @@\n #include \"advice.h\"\n #include \"gettext.h\"\n #include \"convert.h\"\n+#include \"save-cwd.h\"\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\ndiff --git a/git.c b/git.c\nindex 5b6c761..946cc82 100644\n--- a/git.c\n+++ b/git.c\n@@ -20,7 +20,7 @@ const char git_more_info_string[] =\n \n static struct startup_info git_startup_info;\n static int use_pager = -1;\n-static char orig_cwd[PATH_MAX];\n+static struct saved_cwd *orig_cwd;\n static const char *env_names[] = {\n \tGIT_DIR_ENVIRONMENT,\n \tGIT_WORK_TREE_ENVIRONMENT,\n@@ -36,7 +36,8 @@ static void save_env(void)\n \tif (saved_environment)\n \t\treturn;\n \tsaved_environment = 1;\n-\tif (!getcwd(orig_cwd, sizeof(orig_cwd)))\n+\torig_cwd = save_cwd();\n+\tif (!orig_cwd)\n \t\tdie_errno(\"cannot getcwd\");\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\torig_env[i] = getenv(env_names[i]);\n@@ -48,8 +49,8 @@ static void save_env(void)\n static void restore_env(void)\n {\n \tint i;\n-\tif (*orig_cwd && chdir(orig_cwd))\n-\t\tdie_errno(\"could not move to %s\", orig_cwd);\n+\tif (orig_cwd && restore_cwd(orig_cwd))\n+\t\tdie_errno(\"could not move to the original working directory\");\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\tif (orig_env[i])\n \t\t\tsetenv(env_names[i], orig_env[i], 1);\ndiff --git a/save-cwd-fchdir.c b/save-cwd-fchdir.c\nnew file mode 100644\nindex 0000000..5327932\n--- /dev/null\n+++ b/save-cwd-fchdir.c\n@@ -0,0 +1,25 @@\n+#include \"cache.h\"\n+\n+struct saved_cwd {\n+\tint cwd_fd;\n+};\n+\n+struct saved_cwd *save_cwd(void)\n+{\n+\tstruct saved_cwd *cwd = xmalloc(sizeof(*cwd));\n+\tcwd->cwd_fd = open(\".\", O_RDONLY);\n+\tif (cwd->cwd_fd < 0) {\n+\t\tfree(cwd);\n+\t\treturn NULL;\n+\t}\n+\treturn cwd;\n+}\n+\n+int restore_cwd(struct saved_cwd *cwd)\n+{\n+\tint rc = fchdir(cwd->cwd_fd);\n+\tif (!rc)\n+\t\trc = close(cwd->cwd_fd);\n+\tfree(cwd);\n+\treturn rc;\n+}\ndiff --git a/save-cwd.c b/save-cwd.c\nnew file mode 100644\nindex 0000000..1864e9f\n--- /dev/null\n+++ b/save-cwd.c\n@@ -0,0 +1,22 @@\n+#include \"cache.h\"\n+\n+struct saved_cwd {\n+\tchar cwd[PATH_MAX];\n+};\n+\n+struct saved_cwd *save_cwd(void)\n+{\n+\tstruct saved_cwd *cwd = xmalloc(sizeof(*cwd));\n+\tif (!getcwd(cwd->cwd, sizeof(cwd->cwd))) {\n+\t\tfree(cwd);\n+\t\treturn NULL;\n+\t}\n+\treturn cwd;\n+}\n+\n+int restore_cwd(struct saved_cwd *cwd)\n+{\n+\tint rc = chdir(cwd->cwd);\n+\tfree(cwd);\n+\treturn rc;\n+}\ndiff --git a/save-cwd.h b/save-cwd.h\nnew file mode 100644\nindex 0000000..1087ed3\n--- /dev/null\n+++ b/save-cwd.h\n@@ -0,0 +1,3 @@\n+struct saved_cwd;\n+struct saved_cwd *save_cwd(void);\n+int restore_cwd(struct saved_cwd *);\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 01f119f..65d92f2 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -18,19 +18,19 @@ static int chdir_len(const char *orig, int len)\n }\n \n struct unix_sockaddr_context {\n-\tchar orig_dir[PATH_MAX];\n+\tstruct saved_cwd *orig_dir;\n };\n \n static void unix_sockaddr_cleanup(struct unix_sockaddr_context *ctx)\n {\n-\tif (!ctx->orig_dir[0])\n+\tif (!ctx->orig_dir)\n \t\treturn;\n \t/*\n \t * If we fail, we can't just return an error, since we have\n \t * moved the cwd of the whole process, which could confuse calling\n \t * code.  We are better off to just die.\n \t */\n-\tif (chdir(ctx->orig_dir) < 0)\n+\tif (restore_cwd(ctx->orig_dir))\n \t\tdie(\"unable to restore original working directory\");\n }\n \n@@ -39,7 +39,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,\n {\n \tint size = strlen(path) + 1;\n \n-\tctx->orig_dir[0] = '\\0';\n+\tctx->orig_dir = NULL;\n \tif (size > sizeof(sa->sun_path)) {\n \t\tconst char *slash = find_last_dir_sep(path);\n \t\tconst char *dir;\n@@ -57,7 +57,8 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,\n \t\t\treturn -1;\n \t\t}\n \n-\t\tif (!getcwd(ctx->orig_dir, sizeof(ctx->orig_dir))) {\n+\t\tctx->orig_dir = save_cwd();\n+\t\tif (!ctx->orig_dir) {\n \t\t\terrno = ENAMETOOLONG;\n \t\t\treturn -1;\n \t\t}\n-- \n2.0.2\n"},{"id":"246316","messageId":"xmqqbnsmobsq.fsf@gitster.dls.corp.google.com","threadId":"37149","inReplyTo":"53C85625.6020808@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T16:45:57Z","receivedAt":"2014-07-18T16:45:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karsten Blees <karsten.blees@gmail.com> writes:\n\n> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n>> e.g. \"git init\". Make it static too to reduce stack usage.\n>\n> But wouldn't this increase overall memory usage? Stack memory\n> will be reused by subsequent code, while static memory cannot\n> be reused (but still increases the process working set).\n\nCorrect.  And this is a function that is not involved in deep\nrecursion, so avoiding stack allocation seems quite misguided.\n"},{"id":"246367","messageId":"CACsJy8Dq9Em8qZtkep+iVE2M9GPM5RN0oyvyt-nWwK50kSmsQQ@mail.gmail.com","threadId":"37149","inReplyTo":"53C9387D.4090504@web.de","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-19T12:51:05Z","receivedAt":"2014-07-19T12:51:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 18, 2014 at 10:08 PM, René Scharfe <l.s.r@web.de> wrote:\n>> We could wrap this \"get cwd, cd back\" pattern as well. So \"save_cwd\"\n>> returns an opaque pointer, \"go_back\" takes the pointer, (f)chdir back\n>> and free the pointer if needed. On platforms that support fchdir, it\n>> can be used, else we fall back to chdir. I think there are only four\n>> places that follow this pattern, here, setup.c (.git discovery), git.c\n>> (restore_env) and unix-socket.c. Enough call sites to make it worth\n>> the effort?\n>\n> On a cursory look I didn't find any more potential users.  Something\n> like this?\n\nI think it looks nice.\n\n> Applying it to setup.c looks difficult to impossible, though.\n\nYeah, setup.c needs cwd as a string because it does something else to\ncwd besides going back. Too bad.\n-- \nDuy\n"},{"id":"246397","messageId":"53CB0575.5020707@gmail.com","threadId":"37149","inReplyTo":"53C905EB.3010908@web.de","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-07-19T23:55:33Z","receivedAt":"2014-07-19T23:55:33Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 18.07.2014 13:32, schrieb René Scharfe:\n> Am 18.07.2014 01:03, schrieb Karsten Blees:\n>> Am 17.07.2014 19:05, schrieb René Scharfe:\n>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n>> [...]\n>>> \"These routines have traditionally been used by programs to save the\n>>> name of a working directory for the purpose of returning to it. A much\n>>> faster and less error-prone method of accomplishing this is to open the\n>>> current directory (.) and use the fchdir(2) function to return.\"\n>>>\n>>\n>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So why not\n>> use realpath() directly (which would also be thread-safe)?\n> \n> That's a good question; thanks for stepping back and looking at the bigger picture.  If there is widespread OS support for a functionality then we should use it and just provide a compatibility implementation for those platforms lacking it.  The downside is that compat code gets less testing.\n> \n\nI just noticed that in contrast to the POSIX realpath(), our real_path() doesn't require the last path component to exist. I don't know if this property is required by the calling code, though.\n\n> Seeing that readlink()\n\nYou mean realpath()? We don't have a stub for that yet.\n\n> is left as a stub in compat/mingw.h that only errors out, would the equivalent function on Windows be PathCanonicalize (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?\n> \n\nPathCanonicalize() doesn't return an absolute path, the realpath() equivalent would be GetFullPathName() (doesn't resolve symlinks) or GetFinalPathNameByHandle() (requires Vista, resolves symlinks, requires the path to exist).\n\n>> For non-XSI-compliant platforms, we could keep the current implementation.\n> \n> OK, so realpath() for Linux and the BSDs, mingw_realpath() wrapping PathCanonicalize() for Windows and the current code for the rest?\n> \n>> Or re-implement a thread-safe version, e.g. applying resolve_symlink() from\n>> lockfile.c to all path components.\n> \n> Thread safety sounds good.  We'd also need something like normalize_path_copy() but without the conversion of backslashes to slashes, in order to get rid of \".\" and \"..\" path components and something like absolute_path() that doesn't die on error, no?\n> \n\nWindows can handle forward slashes, so normalize_path_copy works just fine.\n\n>> If I may bother you with the Windows point of view:\n>>\n>> There is no fchdir(), and I'm pretty sure open(\".\") won't work either.\n> \n> On Windows, there *is* an absolute path length limit of 260 in the normal case and a bit more than 32000 for some functions using the \\\\?\\ namespace. So one could get away with using a constant-sized buffer for a \"remember the place and return later\" function here.\n> \n\nThe current directory is pretty much the only exception to the \\\\?\\ trick [1]. So a fixed buffer for getcwd() would actually be fine on Windows (although it would have to be 3 * PATH_MAX, as PATH_MAX wide chars will convert to at most 3 * PATH_MAX UTF-8 chars).\n\nHowever, a POSIX conformant getcwd must fail with ERANGE if the buffer is too small. So a better alternative would be to add a strbuf_getcwd() that works similar to strbuf_readlink() (i.e. resize the buffer until its large enough).\n\nSide note: the 'hard' 260 limit for the current directory also means that as long as we *simulate* realpath() via chdir()/getcwd(), long paths [1] don't work here.\n\n> Also, _getcwd can be asked to allocate an appropriately-sized buffer for use, like GNU's get_current_dir_name, by specifying NULL as its first parameter (http://msdn.microsoft.com/en-us/library/sf98bd4y.aspx).\n> \n\nWe use nedmalloc in the Windows builds, so unfortuately we cannot free memory allocated by MSVCRT.dll.\n\n\n[1] http://msdn.microsoft.com/en-us/library/windows/desktop/aa365530%28v=vs.85%29.aspx\n[2] https://github.com/msysgit/git/commit/84393750\n"},{"id":"246398","messageId":"53CB0D81.1070404@gmail.com","threadId":"37149","inReplyTo":"CACsJy8D5X5svApB9edHK+1EaGi+q2ZRSOvyDYaDieVV2psgZPg@mail.gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Karsten Blees","fromEmail":"karsten.blees@gmail.com","sentAt":"2014-07-20T00:29:53Z","receivedAt":"2014-07-20T00:29:53Z","isPatch":true,"sender":{"key":"karsten.blees@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1111200?v=4"},"body":"Am 18.07.2014 12:49, schrieb Duy Nguyen:\n> can be used, else we fall back to chdir. I think there are only four\n> places that follow this pattern, here, setup.c (.git discovery), git.c\n> (restore_env) and unix-socket.c. Enough call sites to make it worth\n> the effort?\n> \n\nreal_path(): here we actually don't want to cd / cd back, we just do\nthis because getcwd reolves symlinks.\n\nsetup.c: AFAICT this sets up the work dir and stays there (no cd back).\n\ngit.c: Only does a 'preliminary' repo setup so that the alias mechanism\nreads the correct config files. Then it reverts the setup if the\nresulting git command doesn't need it. Perhaps it would be better (and\nfaster) to teach the alias code to read the right config file in the\nfirst place?\n\nunix-socket.c: This looks pretty broken. The cd / cd back logic is only\never used if the socket path is too long. In this case, after cd'ing to\nthe parent directory of the socket, unix_stream_listen tries to unlink\nthe *original* socket path, instead of the remaining socket basename in\nsockaddr_un.sun_path. I.e. the subsequent bind() will fail on second\ninvocation of the credential daemon.\n"},{"id":"246402","messageId":"53CB7729.40101@web.de","threadId":"37149","inReplyTo":"53CB0D81.1070404@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-20T08:00:41Z","receivedAt":"2014-07-20T08:00:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.07.2014 02:29, schrieb Karsten Blees:\n> unix-socket.c: This looks pretty broken. The cd / cd back logic is only\n> ever used if the socket path is too long. In this case, after cd'ing to\n> the parent directory of the socket, unix_stream_listen tries to unlink\n> the *original* socket path, instead of the remaining socket basename in\n> sockaddr_un.sun_path. I.e. the subsequent bind() will fail on second\n> invocation of the credential daemon.\n\n-- >8 --\nSubject: [PATCH] unix-socket: remove stale socket before calling chdir()\n\nunix_stream_listen() is given a path.  It calls unix_sockaddr_init(),\nwhich in turn can call chdir().  After that a relative path doesn't\nmean the same as before.  Any use of the original path should thus\nhappen before that call.  For that reason, unlink the given path\n(to get rid of a possibly existing stale socket) right at the\nbeginning of the function.\n\nNoticed-by: Karsten Blees <karsten.blees@gmail.com>\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n unix-socket.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 01f119f..91bd6b8 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -99,11 +99,12 @@ int unix_stream_listen(const char *path)\n \tstruct sockaddr_un sa;\n \tstruct unix_sockaddr_context ctx;\n \n+\tunlink(path);\n+\n \tif (unix_sockaddr_init(&sa, path, &ctx) < 0)\n \t\treturn -1;\n \tfd = unix_stream_socket();\n \n-\tunlink(path);\n \tif (bind(fd, (struct sockaddr *)&sa, sizeof(sa)) < 0)\n \t\tgoto fail;\n \n-- \n2.0.2\n"},{"id":"246403","messageId":"53CBA55B.5010606@web.de","threadId":"37149","inReplyTo":"53CB0575.5020707@gmail.com","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-07-20T11:17:47Z","receivedAt":"2014-07-20T11:17:47Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 20.07.2014 01:55, schrieb Karsten Blees:\n> Am 18.07.2014 13:32, schrieb René Scharfe:\n>> Am 18.07.2014 01:03, schrieb Karsten Blees:\n>>> Am 17.07.2014 19:05, schrieb René Scharfe:\n>>>> Am 17.07.2014 14:45, schrieb Nguyễn Thái Ngọc Duy:\n>>> [...]\n>>>> \"These routines have traditionally been used by programs to\n>>>> save the name of a working directory for the purpose of\n>>>> returning to it. A much faster and less error-prone method of\n>>>> accomplishing this is to open the current directory (.) and use\n>>>> the fchdir(2) function to return.\"\n>>>>\n>>>\n>>> fchdir() is part of the POSIX-XSI extension, as is realpath(). So\n>>> why not use realpath() directly (which would also be\n>>> thread-safe)?\n>>\n>> That's a good question; thanks for stepping back and looking at the\n>> bigger picture.  If there is widespread OS support for a\n>> functionality then we should use it and just provide a\n>> compatibility implementation for those platforms lacking it.  The\n>> downside is that compat code gets less testing.\n>>\n>\n> I just noticed that in contrast to the POSIX realpath(), our\n> real_path() doesn't require the last path component to exist. I don't\n> know if this property is required by the calling code, though.\n\nIf it's important then removing the last component on ENOENT, calling \nrealpath() again and appending it should handle that, right?\n\n>> Seeing that readlink()\n>\n> You mean realpath()? We don't have a stub for that yet.\n>\n>> is left as a stub in compat/mingw.h that only errors out, would the\n>> equivalent function on Windows be PathCanonicalize\n>> (http://msdn.microsoft.com/en-us/library/windows/desktop/bb773569%28v=vs.85%29.aspx)?\n>>\n> PathCanonicalize() doesn't return an absolute path, the realpath()\n> equivalent would be GetFullPathName() (doesn't resolve symlinks) or\n> GetFinalPathNameByHandle() (requires Vista, resolves symlinks,\n> requires the path to exist).\n\nI meant readlink() and assumed its ENOSYS stub in compat/mingw.h means \nthat git doesn't handle symlinks on Windows at all.\n\n> However, a POSIX conformant getcwd must fail with ERANGE if the\n> buffer is too small. So a better alternative would be to add a\n> strbuf_getcwd() that works similar to strbuf_readlink() (i.e. resize\n> the buffer until its large enough).\n\nThat's a good idea anyway, will send patches.\n\n> Side note: the 'hard' 260 limit for the current directory also means\n> that as long as we *simulate* realpath() via chdir()/getcwd(), long\n> paths [1] don't work here.\n\nGreat, so that motivation for getting a chdir()-free implementation \nremains. :)\n"},{"id":"246428","messageId":"20140721022501.GB22750@peff.net","threadId":"37149","inReplyTo":"53CB7729.40101@web.de","subject":"Re: [PATCH] abspath.c: use PATH_MAX in real_path_internal()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-07-21T02:25:01Z","receivedAt":"2014-07-21T02:25:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jul 20, 2014 at 10:00:41AM +0200, René Scharfe wrote:\n\n> -- >8 --\n> Subject: [PATCH] unix-socket: remove stale socket before calling chdir()\n> \n> unix_stream_listen() is given a path.  It calls unix_sockaddr_init(),\n> which in turn can call chdir().  After that a relative path doesn't\n> mean the same as before.  Any use of the original path should thus\n> happen before that call.  For that reason, unlink the given path\n> (to get rid of a possibly existing stale socket) right at the\n> beginning of the function.\n\nThanks, I think this ordering problem was just missed in 1eb10f4\n(unix-socket: handle long socket pathnames, 2012-01-09).\n\nYour solution looks OK, though I think also just using:\n\n  unlink(sa.sun_path);\n\nwould work, too (that is the path we are feeding to bind(), whether we\nhave chdir'd or not, so perhaps it is a little more obviously correct?).\nI'm OK with either.\n\n> diff --git a/unix-socket.c b/unix-socket.c\n> index 01f119f..91bd6b8 100644\n> --- a/unix-socket.c\n> +++ b/unix-socket.c\n> @@ -99,11 +99,12 @@ int unix_stream_listen(const char *path)\n>  \tstruct sockaddr_un sa;\n>  \tstruct unix_sockaddr_context ctx;\n>  \n> +\tunlink(path);\n> +\n>  \tif (unix_sockaddr_init(&sa, path, &ctx) < 0)\n>  \t\treturn -1;\n>  \tfd = unix_stream_socket();\n>  \n> -\tunlink(path);\n\nI briefly wondered if this should be unlinking only when we get EEXIST,\nbut I don't think it is worth caring about. The only caller is\ncredential-cache, and it always wants to unconditionally replace\nwhatever is there (it will already have tried to contact any existing\nsocket).\n\n-Peff\n"}]}