{"thread":{"id":"37160","subject":"[PATCH] Make locked paths absolute when current directory is changed","startedAt":"2014-07-18T13:08:57Z","lastAt":"2014-09-03T08:00:21Z","messageCount":27,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Johannes Sixt","Duy Nguyen","Philip Oakley","Ramsay Jones","Yue Lin Ho","Torsten Bögershausen","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"246308","messageId":"1405688937-22925-1-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":null,"subject":"[PATCH] Make locked paths absolute when current directory is changed","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-18T13:08:57Z","receivedAt":"2014-07-18T13:08:57Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Locked paths are saved in a linked list so that if something wrong\nhappens, *.lock are removed. This works fine if we keep cwd the same,\nwhich is true 99% of time except:\n\n - update-index and read-tree hold the lock on $GIT_DIR/index really\n early, then later on may call setup_work_tree() to move cwd.\n\n - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n which temporarily moves cwd away and back.\n\nDuring that time when cwd is moved (either permanently or temporarily)\nand we decide to die(), attempts to remove relative *.lock will fail,\nand the next operation will complain that some files are still locked.\n\nAvoid this case by turning relative paths to absolute when chdir() is\ncalled (or soon to be called, in setup_git_directory_gently case).\n\nReported-by: Yue Lin Ho \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n It occurred to me while writing this commit message that it would be\n better if we can unlink via a file descriptor so we don't have to\n convert paths. But I'm not sure about the availability of unlinkat(),\n especially on Windows.\n\n abspath.c | 2 +-\n cache.h | 6 ++++++\n git.c | 6 +++---\n lockfile.c | 16 ++++++++++++++++\n path.c | 4 ++--\n run-command.c | 2 +-\n setup.c | 3 ++-\n unix-socket.c | 2 +-\n 8 files changed, 32 insertions(+), 9 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex ca33558..78c963f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -87,7 +87,7 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t\t\t\t\tgoto error_out;\n \t\t\t}\n \n-\t\t\tif (chdir(buf)) {\n+\t\t\tif (chdir_safe(buf)) {\n \t\t\t\tif (die_on_error)\n \t\t\t\t\tdie_errno(\"Could not switch to '%s'\", buf);\n \t\t\t\telse\ndiff --git a/cache.h b/cache.h\nindex 44aa439..d3f2596 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -564,6 +564,12 @@ extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n+extern void make_locked_paths_absolute(void);\n+static inline int chdir_safe(const char *path)\n+{\n+\tmake_locked_paths_absolute();\n+\treturn chdir(path);\n+}\n \n extern int hold_locked_index(struct lock_file *, int);\n extern int commit_locked_index(struct lock_file *);\ndiff --git a/git.c b/git.c\nindex 5b6c761..27766c3 100644\n--- a/git.c\n+++ b/git.c\n@@ -48,7 +48,7 @@ static void save_env(void)\n static void restore_env(void)\n {\n \tint i;\n-\tif (*orig_cwd && chdir(orig_cwd))\n+\tif (*orig_cwd && chdir_safe(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\tif (orig_env[i])\n@@ -206,7 +206,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for -C.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tif (chdir((*argv)[1]))\n+\t\t\tif (chdir_safe((*argv)[1]))\n \t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n@@ -292,7 +292,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\tif (subdir && chdir(subdir))\n+\tif (subdir && chdir_safe(subdir))\n \t\tdie_errno(\"Cannot change to '%s'\", subdir);\n \n \terrno = saved_errno;\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..a70d107 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)\n \t}\n \tlk->filename[0] = 0;\n }\n+\n+void make_locked_paths_absolute(void)\n+{\n+\tstruct lock_file *lk;\n+\tconst char *abspath;\n+\tfor (lk = lock_file_list; lk != NULL; lk = lk->next) {\n+\t\tif (!lk->filename[0] || lk->filename[0] == '/')\n+\t\t\tcontinue;\n+\t\tabspath = absolute_path(lk->filename);\n+\t\tif (strlen(abspath) >= sizeof(lk->filename))\n+\t\t\twarning(\"locked path %s is relative when current directory \"\n+\t\t\t\t\"is changed\", lk->filename);\n+\t\telse\n+\t\t\tstrcpy(lk->filename, abspath);\n+\t}\n+}\ndiff --git a/path.c b/path.c\nindex bc804a3..9e8e101 100644\n--- a/path.c\n+++ b/path.c\n@@ -372,11 +372,11 @@ const char *enter_repo(const char *path, int strict)\n \t\tgitfile = read_gitfile(used_path) ;\n \t\tif (gitfile)\n \t\t\tstrcpy(used_path, gitfile);\n-\t\tif (chdir(used_path))\n+\t\tif (chdir_safe(used_path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n-\telse if (chdir(path))\n+\telse if (chdir_safe(path))\n \t\treturn NULL;\n \n \tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\ndiff --git a/run-command.c b/run-command.c\nindex be07d4a..55f360e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -399,7 +399,7 @@ fail_pipe:\n \t\t\tclose(cmd->out);\n \t\t}\n \n-\t\tif (cmd->dir && chdir(cmd->dir))\n+\t\tif (cmd->dir && chdir_safe(cmd->dir))\n \t\t\tdie_errno(\"exec '%s': cd to '%s' failed\", cmd->argv[0],\n \t\t\t cmd->dir);\n \t\tif (cmd->env) {\ndiff --git a/setup.c b/setup.c\nindex 0a22f8b..921045e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -290,7 +290,7 @@ void setup_work_tree(void)\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n \t\tgit_dir = real_path(get_git_dir());\n-\tif (!work_tree || chdir(work_tree))\n+\tif (!work_tree || chdir_safe(work_tree))\n \t\tdie(\"This operation must be run in a work tree\");\n \n \t/*\n@@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \t\tdie_errno(\"Unable to read current working directory\");\n \toffset = len = strlen(cwd);\n \n+\tmake_locked_paths_absolute();\n \t/*\n \t * If GIT_DIR is set explicitly, we're not going\n \t * to do any discovery, but we still do repository\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 01f119f..eeb8007 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -12,7 +12,7 @@ static int unix_stream_socket(void)\n static int chdir_len(const char *orig, int len)\n {\n \tchar *path = xmemdupz(orig, len);\n-\tint r = chdir(path);\n+\tint r = chdir_safe(path);\n \tfree(path);\n \treturn r;\n }\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246323","messageId":"xmqqmwc6mueh.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"1405688937-22925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Make locked paths absolute when current directory is changed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-18T17:47:02Z","receivedAt":"2014-07-18T17:47:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Locked paths are saved in a linked list so that if something wrong\n> happens, *.lock are removed. This works fine if we keep cwd the same,\n> which is true 99% of time except:\n>\n> - update-index and read-tree hold the lock on $GIT_DIR/index really\n> early, then later on may call setup_work_tree() to move cwd.\n>\n> - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n> down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n> which temporarily moves cwd away and back.\n>\n> During that time when cwd is moved (either permanently or temporarily)\n> and we decide to die(), attempts to remove relative *.lock will fail,\n> and the next operation will complain that some files are still locked.\n>\n> Avoid this case by turning relative paths to absolute when chdir() is\n> called (or soon to be called, in setup_git_directory_gently case).\n\nThe rationale makes sense.\n\n> +extern void make_locked_paths_absolute(void);\n> +static inline int chdir_safe(const char *path)\n> +{\n> +\tmake_locked_paths_absolute();\n> +\treturn chdir(path);\n> +}\n\nClever ;-). Instead of making paths absolute when you receive\nrequests to lock them, you lazily turn the ones relative to cwd()\nabsolute just before they are about to become invalid/problematic\nbecause the program wants to chdir.\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 8fbcb6a..a70d107 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)\n> \t}\n> \tlk->filename[0] = 0;\n> }\n> +\n> +void make_locked_paths_absolute(void)\n> +{\n> +\tstruct lock_file *lk;\n> +\tconst char *abspath;\n> +\tfor (lk = lock_file_list; lk != NULL; lk = lk->next) {\n> +\t\tif (!lk->filename[0] || lk->filename[0] == '/')\n> +\t\t\tcontinue;\n\nDo we have to worry about Windows?\n\n> +\t\tabspath = absolute_path(lk->filename);\n> +\t\tif (strlen(abspath) >= sizeof(lk->filename))\n> +\t\t\twarning(\"locked path %s is relative when current directory \"\n> +\t\t\t\t\"is changed\", lk->filename);\n\nShouldn't this be a die() or an error return (which will kill the\ncaller anyway)?\n\n> @@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n> \t\tdie_errno(\"Unable to read current working directory\");\n> \toffset = len = strlen(cwd);\n> \n> +\tmake_locked_paths_absolute();\n\nJust being curious, but this early in the start-up sequence, what\nfiles do we have locks on?\n"},{"id":"246336","messageId":"53C98717.3060600@kdbg.org","threadId":"37160","inReplyTo":"1405688937-22925-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Make locked paths absolute when current directory is changed","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-07-18T20:44:07Z","receivedAt":"2014-07-18T20:44:07Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 18.07.2014 15:08, schrieb Nguyễn Thái Ngọc Duy:\n> diff --git a/lockfile.c b/lockfile.c\n> index 8fbcb6a..a70d107 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -280,3 +280,19 @@ void rollback_lock_file(struct lock_file *lk)\n> \t}\n> \tlk->filename[0] = 0;\n> }\n> +\n> +void make_locked_paths_absolute(void)\n> +{\n> +\tstruct lock_file *lk;\n> +\tconst char *abspath;\n> +\tfor (lk = lock_file_list; lk != NULL; lk = lk->next) {\n> +\t\tif (!lk->filename[0] || lk->filename[0] == '/')\n\nPlease use is_absolute_path().\n\n> +\t\t\tcontinue;\n> +\t\tabspath = absolute_path(lk->filename);\n> +\t\tif (strlen(abspath) >= sizeof(lk->filename))\n> +\t\t\twarning(\"locked path %s is relative when current directory \"\n> +\t\t\t\t\"is changed\", lk->filename);\n> +\t\telse\n> +\t\t\tstrcpy(lk->filename, abspath);\n> +\t}\n> +}\n\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -399,7 +399,7 @@ fail_pipe:\n> \t\t\tclose(cmd->out);\n> \t\t}\n> \n> -\t\tif (cmd->dir && chdir(cmd->dir))\n> +\t\tif (cmd->dir && chdir_safe(cmd->dir))\n\nThis one shouldn't be necessary: It's in the child, and the child\nprocess does not release the locks; see the check for the owner in\nremove_lock_file.\n\n> \t\t\tdie_errno(\"exec '%s': cd to '%s' failed\", cmd->argv[0],\n> \t\t\t cmd->dir);\n> \t\tif (cmd->env) {\n\n-- Hannes\n"},{"id":"246366","messageId":"CACsJy8CpoOCw+3Q5AZk+cuXsUcoqW3hoHS9mX_=VuYtu8638+w@mail.gmail.com","threadId":"37160","inReplyTo":"xmqqmwc6mueh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Make locked paths absolute when current directory is changed","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-19T12:40:48Z","receivedAt":"2014-07-19T12:40:48Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jul 19, 2014 at 12:47 AM, Junio C Hamano wrote:\n>> + abspath = absolute_path(lk->filename);\n>> + if (strlen(abspath) >= sizeof(lk->filename))\n>> + warning(\"locked path %s is relative when current directory \"\n>> + \"is changed\", lk->filename);\n>\n> Shouldn't this be a die() or an error return (which will kill the\n> caller anyway)?\n\nWe don't know for sure there will be a die() or something to trigger\nthe roll back (or commit). If the chdir() is temporary, absolute path\nnot fitting in PATH_MAX chars is not fatal because cwd will be\nreverted before commit/rollback. A better solution is probably avoid\nPATH_MAX in lk->filename. But yeah, changing it to die() is safer\n(especially when cwd is moved permanently for some options in\nupdate-index and read-tree)\n\n>> @@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n>> die_errno(\"Unable to read current working directory\");\n>> offset = len = strlen(cwd);\n>>\n>> + make_locked_paths_absolute();\n>\n> Just being curious, but this early in the start-up sequence, what\n> files do we have locks on?\n\nWe don't know. For most builtin commands, the setup is done early and\nwe can be sure of no locks. Some commands (especially non-builtin) can\nstill delay calling setup_git_directory() until later and they might\ndo something in between, so better be safe than sorry.\n-- \nDuy\n"},{"id":"246408","messageId":"1405858399-23082-1-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1405688937-22925-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-20T12:13:18Z","receivedAt":"2014-07-20T12:13:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Something extra is, because struct lock_file is usually used as static\nvariables in many places. This patch reduces bss section by about 80k\nbytes (or 23%) on Linux.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n This helps remove the length check in v1 of the next patch.\n\n cache.h | 2 +-\n lockfile.c | 56 ++++++++++++++++++++++++++++++++------------------------\n 2 files changed, 33 insertions(+), 25 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 44aa439..9ecb636 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -554,7 +554,7 @@ struct lock_file {\n \tint fd;\n \tpid_t owner;\n \tchar on_list;\n-\tchar filename[PATH_MAX];\n+\tchar *filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..968b28f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -7,13 +7,19 @@\n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n \n+static void clear_filename(struct lock_file *lk)\n+{\n+\tfree(lk->filename);\n+\tlk->filename = NULL;\n+}\n+\n static void remove_lock_file(void)\n {\n \tpid_t me = getpid();\n \n \twhile (lock_file_list) {\n \t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->filename[0]) {\n+\t\t lock_file_list->filename) {\n \t\t\tif (lock_file_list->fd >= 0)\n \t\t\t\tclose(lock_file_list->fd);\n \t\t\tunlink_or_warn(lock_file_list->filename);\n@@ -77,10 +83,16 @@ static char *last_path_elm(char *p)\n * Always returns p.\n */\n \n-static char *resolve_symlink(char *p, size_t s)\n+static char *resolve_symlink(const char *in)\n {\n+\tstatic char p[PATH_MAX];\n+\tsize_t s = sizeof(p);\n \tint depth = MAXDEPTH;\n \n+\tif (strlen(in) >= sizeof(p))\n+\t\treturn NULL;\n+\tstrcpy(p, in);\n+\n \twhile (depth--) {\n \t\tchar link[PATH_MAX];\n \t\tint link_len = readlink(p, link, sizeof(link));\n@@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\t/*\n-\t * subtract 5 from size to make sure there's room for adding\n-\t * \".lock\" for the lock file name\n-\t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n-\n-\tif (strlen(path) >= max_path_len)\n+\tint len;\n+\tif (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))\n \t\treturn -1;\n+\tlen = strlen(path) + 5; /* .lock */\n+\tlk->filename = xmallocz(len);\n \tstrcpy(lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n@@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\t\t lk->filename);\n \t}\n \telse\n-\t\tlk->filename[0] = 0;\n+\t\tclear_filename(lk);\n \treturn lk->fd;\n }\n \n@@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar result_file[PATH_MAX];\n-\tsize_t i;\n-\tif (lk->fd >= 0 && close_lock_file(lk))\n+\tchar *result_file;\n+\tif ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n \t\treturn -1;\n-\tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - 5; /* .lock */\n-\tresult_file[i] = 0;\n-\tif (rename(lk->filename, result_file))\n+\tresult_file = xmemdupz(lk->filename,\n+\t\t\t strlen(lk->filename) - 5 /* .lock */);\n+\tif (rename(lk->filename, result_file)) {\n+\t\tfree(result_file);\n \t\treturn -1;\n-\tlk->filename[0] = 0;\n+\t}\n+\tfree(result_file);\n+\tclear_filename(lk);\n \treturn 0;\n }\n \n@@ -260,11 +268,11 @@ void set_alternate_index_output(const char *name)\n int commit_locked_index(struct lock_file *lk)\n {\n \tif (alternate_index_output) {\n-\t\tif (lk->fd >= 0 && close_lock_file(lk))\n+\t\tif ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n \t\t\treturn -1;\n \t\tif (rename(lk->filename, alternate_index_output))\n \t\t\treturn -1;\n-\t\tlk->filename[0] = 0;\n+\t\tclear_filename(lk);\n \t\treturn 0;\n \t}\n \telse\n@@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->filename[0]) {\n+\tif (lk->filename) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose(lk->fd);\n \t\tunlink_or_warn(lk->filename);\n \t}\n-\tlk->filename[0] = 0;\n+\tclear_filename(lk);\n }\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246409","messageId":"1405858399-23082-2-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1405858399-23082-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-20T12:13:19Z","receivedAt":"2014-07-20T12:13:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Locked paths are saved in a linked list so that if something wrong\nhappens, *.lock are removed. This works fine if we keep cwd the same,\nwhich is true 99% of time except:\n\n - update-index and read-tree hold the lock on $GIT_DIR/index really\n early, then later on may call setup_work_tree() to move cwd.\n\n - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n which temporarily moves cwd away and back.\n\nDuring that time when cwd is moved (either permanently or temporarily)\nand we decide to die(), attempts to remove relative *.lock will fail,\nand the next operation will complain that some files are still locked.\n\nAvoid this case by turning relative paths to absolute when chdir() is\ncalled (or soon to be called, in setup_git_directory_gently case).\n\nReported-by: Yue Lin Ho \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Compared to v1, make_locked_paths_absolute() now always succeeds (ok\n absolute_path could die inside, but that's a separate problem) and\n supports Windows.\n\n abspath.c | 2 +-\n cache.h | 6 ++++++\n git.c | 6 +++---\n lockfile.c | 12 ++++++++++++\n path.c | 4 ++--\n run-command.c | 2 +-\n setup.c | 3 ++-\n unix-socket.c | 2 +-\n 8 files changed, 28 insertions(+), 9 deletions(-)\n\ndiff --git a/abspath.c b/abspath.c\nindex ca33558..78c963f 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -87,7 +87,7 @@ static const char *real_path_internal(const char *path, int die_on_error)\n \t\t\t\t\tgoto error_out;\n \t\t\t}\n \n-\t\t\tif (chdir(buf)) {\n+\t\t\tif (chdir_safe(buf)) {\n \t\t\t\tif (die_on_error)\n \t\t\t\t\tdie_errno(\"Could not switch to '%s'\", buf);\n \t\t\t\telse\ndiff --git a/cache.h b/cache.h\nindex 9ecb636..48ffa21 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -564,6 +564,12 @@ extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n extern int commit_lock_file(struct lock_file *);\n extern void update_index_if_able(struct index_state *, struct lock_file *);\n+extern void make_locked_paths_absolute(void);\n+static inline int chdir_safe(const char *path)\n+{\n+\tmake_locked_paths_absolute();\n+\treturn chdir(path);\n+}\n \n extern int hold_locked_index(struct lock_file *, int);\n extern int commit_locked_index(struct lock_file *);\ndiff --git a/git.c b/git.c\nindex 5b6c761..27766c3 100644\n--- a/git.c\n+++ b/git.c\n@@ -48,7 +48,7 @@ static void save_env(void)\n static void restore_env(void)\n {\n \tint i;\n-\tif (*orig_cwd && chdir(orig_cwd))\n+\tif (*orig_cwd && chdir_safe(orig_cwd))\n \t\tdie_errno(\"could not move to %s\", orig_cwd);\n \tfor (i = 0; i < ARRAY_SIZE(env_names); i++) {\n \t\tif (orig_env[i])\n@@ -206,7 +206,7 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\t\tfprintf(stderr, \"No directory given for -C.\\n\" );\n \t\t\t\tusage(git_usage_string);\n \t\t\t}\n-\t\t\tif (chdir((*argv)[1]))\n+\t\t\tif (chdir_safe((*argv)[1]))\n \t\t\t\tdie_errno(\"Cannot change to '%s'\", (*argv)[1]);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n@@ -292,7 +292,7 @@ static int handle_alias(int *argcp, const char ***argv)\n \t\tret = 1;\n \t}\n \n-\tif (subdir && chdir(subdir))\n+\tif (subdir && chdir_safe(subdir))\n \t\tdie_errno(\"Cannot change to '%s'\", subdir);\n \n \terrno = saved_errno;\ndiff --git a/lockfile.c b/lockfile.c\nindex 968b28f..cf1e795 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -288,3 +288,15 @@ void rollback_lock_file(struct lock_file *lk)\n \t}\n \tclear_filename(lk);\n }\n+\n+void make_locked_paths_absolute(void)\n+{\n+\tstruct lock_file *lk;\n+\tfor (lk = lock_file_list; lk != NULL; lk = lk->next) {\n+\t\tif (lk->filename && !is_absolute_path(lk->filename)) {\n+\t\t\tchar *to_free = lk->filename;\n+\t\t\tlk->filename = xstrdup(absolute_path(lk->filename));\n+\t\t\tfree(to_free);\n+\t\t}\n+\t}\n+}\ndiff --git a/path.c b/path.c\nindex bc804a3..9e8e101 100644\n--- a/path.c\n+++ b/path.c\n@@ -372,11 +372,11 @@ const char *enter_repo(const char *path, int strict)\n \t\tgitfile = read_gitfile(used_path) ;\n \t\tif (gitfile)\n \t\t\tstrcpy(used_path, gitfile);\n-\t\tif (chdir(used_path))\n+\t\tif (chdir_safe(used_path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n-\telse if (chdir(path))\n+\telse if (chdir_safe(path))\n \t\treturn NULL;\n \n \tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\ndiff --git a/run-command.c b/run-command.c\nindex be07d4a..55f360e 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -399,7 +399,7 @@ fail_pipe:\n \t\t\tclose(cmd->out);\n \t\t}\n \n-\t\tif (cmd->dir && chdir(cmd->dir))\n+\t\tif (cmd->dir && chdir_safe(cmd->dir))\n \t\t\tdie_errno(\"exec '%s': cd to '%s' failed\", cmd->argv[0],\n \t\t\t cmd->dir);\n \t\tif (cmd->env) {\ndiff --git a/setup.c b/setup.c\nindex 0a22f8b..921045e 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -290,7 +290,7 @@ void setup_work_tree(void)\n \tgit_dir = get_git_dir();\n \tif (!is_absolute_path(git_dir))\n \t\tgit_dir = real_path(get_git_dir());\n-\tif (!work_tree || chdir(work_tree))\n+\tif (!work_tree || chdir_safe(work_tree))\n \t\tdie(\"This operation must be run in a work tree\");\n \n \t/*\n@@ -636,6 +636,7 @@ static const char *setup_git_directory_gently_1(int *nongit_ok)\n \t\tdie_errno(\"Unable to read current working directory\");\n \toffset = len = strlen(cwd);\n \n+\tmake_locked_paths_absolute();\n \t/*\n \t * If GIT_DIR is set explicitly, we're not going\n \t * to do any discovery, but we still do repository\ndiff --git a/unix-socket.c b/unix-socket.c\nindex 01f119f..eeb8007 100644\n--- a/unix-socket.c\n+++ b/unix-socket.c\n@@ -12,7 +12,7 @@ static int unix_stream_socket(void)\n static int chdir_len(const char *orig, int len)\n {\n \tchar *path = xmemdupz(orig, len);\n-\tint r = chdir(path);\n+\tint r = chdir_safe(path);\n \tfree(path);\n \treturn r;\n }\n-- \n1.9.1.346.ga2b5940\n"},{"id":"246413","messageId":"232C089CF449490C9F5B398996DD314C@PhilipOakley","threadId":"37160","inReplyTo":"1405858399-23082-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2014-07-20T12:47:03Z","receivedAt":"2014-07-20T12:47:03Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Nguyễn Thái Ngọc Duy\" \n> Something extra is, because struct lock_file is usually used as static\n> variables in many places. This patch reduces bss section by about 80k\n> bytes (or 23%) on Linux.\n\nThis didn't scan for me. Perhaps it's the punctuation. Maybe:\n\nAdditionally, because the struct lock_file variables are [were] in many\nplaces static, this patch reduces the bss section size by about 80k\nbytes (or 23%) on Linux.\n\nDoes my tweak reflect your intent?\nPhilip.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> This helps remove the length check in v1 of the next patch.\n>\n> cache.h | 2 +-\n> lockfile.c | 56 \n> ++++++++++++++++++++++++++++++++------------------------\n> 2 files changed, 33 insertions(+), 25 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index 44aa439..9ecb636 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -554,7 +554,7 @@ struct lock_file {\n> int fd;\n> pid_t owner;\n> char on_list;\n> - char filename[PATH_MAX];\n> + char *filename;\n> };\n> #define LOCK_DIE_ON_ERROR 1\n> #define LOCK_NODEREF 2\n> diff --git a/lockfile.c b/lockfile.c\n> index 8fbcb6a..968b28f 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -7,13 +7,19 @@\n> static struct lock_file *lock_file_list;\n> static const char *alternate_index_output;\n>\n> +static void clear_filename(struct lock_file *lk)\n> +{\n> + free(lk->filename);\n> + lk->filename = NULL;\n> +}\n> +\n> static void remove_lock_file(void)\n> {\n> pid_t me = getpid();\n>\n> while (lock_file_list) {\n> if (lock_file_list->owner == me &&\n> - lock_file_list->filename[0]) {\n> + lock_file_list->filename) {\n> if (lock_file_list->fd >= 0)\n> close(lock_file_list->fd);\n> unlink_or_warn(lock_file_list->filename);\n> @@ -77,10 +83,16 @@ static char *last_path_elm(char *p)\n> * Always returns p.\n> */\n>\n> -static char *resolve_symlink(char *p, size_t s)\n> +static char *resolve_symlink(const char *in)\n> {\n> + static char p[PATH_MAX];\n> + size_t s = sizeof(p);\n> int depth = MAXDEPTH;\n>\n> + if (strlen(in) >= sizeof(p))\n> + return NULL;\n> + strcpy(p, in);\n> +\n> while (depth--) {\n> char link[PATH_MAX];\n> int link_len = readlink(p, link, sizeof(link));\n> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)\n>\n> static int lock_file(struct lock_file *lk, const char *path, int \n> flags)\n> {\n> - /*\n> - * subtract 5 from size to make sure there's room for adding\n> - * \".lock\" for the lock file name\n> - */\n> - static const size_t max_path_len = sizeof(lk->filename) - 5;\n> -\n> - if (strlen(path) >= max_path_len)\n> + int len;\n> + if (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))\n> return -1;\n> + len = strlen(path) + 5; /* .lock */\n> + lk->filename = xmallocz(len);\n> strcpy(lk->filename, path);\n> - if (!(flags & LOCK_NODEREF))\n> - resolve_symlink(lk->filename, max_path_len);\n> strcat(lk->filename, \".lock\");\n> lk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n> if (0 <= lk->fd) {\n> @@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const \n> char *path, int flags)\n> lk->filename);\n> }\n> else\n> - lk->filename[0] = 0;\n> + clear_filename(lk);\n> return lk->fd;\n> }\n>\n> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)\n>\n> int commit_lock_file(struct lock_file *lk)\n> {\n> - char result_file[PATH_MAX];\n> - size_t i;\n> - if (lk->fd >= 0 && close_lock_file(lk))\n> + char *result_file;\n> + if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n> return -1;\n> - strcpy(result_file, lk->filename);\n> - i = strlen(result_file) - 5; /* .lock */\n> - result_file[i] = 0;\n> - if (rename(lk->filename, result_file))\n> + result_file = xmemdupz(lk->filename,\n> + strlen(lk->filename) - 5 /* .lock */);\n> + if (rename(lk->filename, result_file)) {\n> + free(result_file);\n> return -1;\n> - lk->filename[0] = 0;\n> + }\n> + free(result_file);\n> + clear_filename(lk);\n> return 0;\n> }\n>\n> @@ -260,11 +268,11 @@ void set_alternate_index_output(const char \n> *name)\n> int commit_locked_index(struct lock_file *lk)\n> {\n> if (alternate_index_output) {\n> - if (lk->fd >= 0 && close_lock_file(lk))\n> + if ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n> return -1;\n> if (rename(lk->filename, alternate_index_output))\n> return -1;\n> - lk->filename[0] = 0;\n> + clear_filename(lk);\n> return 0;\n> }\n> else\n> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)\n>\n> void rollback_lock_file(struct lock_file *lk)\n> {\n> - if (lk->filename[0]) {\n> + if (lk->filename) {\n> if (lk->fd >= 0)\n> close(lk->fd);\n> unlink_or_warn(lk->filename);\n> }\n> - lk->filename[0] = 0;\n> + clear_filename(lk);\n> }\n> -- \n> 1.9.1.346.ga2b5940\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n> \n"},{"id":"246414","messageId":"CACsJy8DtYrP4CndD+Ud2qpYT3J7NehukL9a_sgiq7vZ82KH-0g@mail.gmail.com","threadId":"37160","inReplyTo":"232C089CF449490C9F5B398996DD314C@PhilipOakley","subject":"Re: [PATCH v2 1/2] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-20T12:50:03Z","receivedAt":"2014-07-20T12:50:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Jul 20, 2014 at 7:47 PM, Philip Oakley wrote:\n> From: \"Nguyễn Thái Ngọc Duy\" \n>\n>> Something extra is, because struct lock_file is usually used as static\n>> variables in many places. This patch reduces bss section by about 80k\n>> bytes (or 23%) on Linux.\n>\n>\n> This didn't scan for me. Perhaps it's the punctuation. Maybe:\n>\n> Additionally, because the struct lock_file variables are [were] in many\n> places static, this patch reduces the bss section size by about 80k\n>\n> bytes (or 23%) on Linux.\n>\n> Does my tweak reflect your intent?\n\nYes. But I should probably put that below the \"---\" line. We're not\nbusybox. 80k is pratically nothing.\n-- \nDuy\n"},{"id":"246448","messageId":"53CD1529.9080102@ramsay1.demon.co.uk","threadId":"37160","inReplyTo":"1405858399-23082-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-21T13:27:05Z","receivedAt":"2014-07-21T13:27:05Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 20/07/14 13:13, Nguyễn Thái Ngọc Duy wrote:\n> Locked paths are saved in a linked list so that if something wrong\n> happens, *.lock are removed. This works fine if we keep cwd the same,\n> which is true 99% of time except:\n> \n> - update-index and read-tree hold the lock on $GIT_DIR/index really\n> early, then later on may call setup_work_tree() to move cwd.\n> \n> - Suppose a lock is being held (e.g. by \"git add\") then somewhere\n> down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n> which temporarily moves cwd away and back.\n> \n> During that time when cwd is moved (either permanently or temporarily)\n> and we decide to die(), attempts to remove relative *.lock will fail,\n> and the next operation will complain that some files are still locked.\n> \n> Avoid this case by turning relative paths to absolute when chdir() is\n> called (or soon to be called, in setup_git_directory_gently case).\n> \n> Reported-by: Yue Lin Ho \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\n[snip]\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 968b28f..cf1e795 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -288,3 +288,15 @@ void rollback_lock_file(struct lock_file *lk)\n> \t}\n> \tclear_filename(lk);\n> }\n> +\n> +void make_locked_paths_absolute(void)\n> +{\n> +\tstruct lock_file *lk;\n> +\tfor (lk = lock_file_list; lk != NULL; lk = lk->next) {\n> +\t\tif (lk->filename && !is_absolute_path(lk->filename)) {\n> +\t\t\tchar *to_free = lk->filename;\n> +\t\t\tlk->filename = xstrdup(absolute_path(lk->filename));\n> +\t\t\tfree(to_free);\n> +\t\t}\n> +\t}\n> +}\n\nI just have to ask, why are we putting relative pathnames in this\nlist to begin with? Why not use an absolute path when taking the\nlock in all cases? (calling absolute_path() and using the result\nto take the lock, storing it in the lock_file list, should not be\nin the critical path, right? Not that I have measured it, of course! :)\n\nATB,\nRamsay Jones\n"},{"id":"246452","messageId":"CACsJy8AXc4jvLPNpGyGdY9uzrnN-SbEeiksLDpS_=29gJ1KMnQ@mail.gmail.com","threadId":"37160","inReplyTo":"53CD1529.9080102@ramsay1.demon.co.uk","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-21T13:47:39Z","receivedAt":"2014-07-21T13:47:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones\n wrote:\n>> +void make_locked_paths_absolute(void)\n>> +{\n>> + struct lock_file *lk;\n>> + for (lk = lock_file_list; lk != NULL; lk = lk->next) {\n>> + if (lk->filename && !is_absolute_path(lk->filename)) {\n>> + char *to_free = lk->filename;\n>> + lk->filename = xstrdup(absolute_path(lk->filename));\n>> + free(to_free);\n>> + }\n>> + }\n>> +}\n>\n> I just have to ask, why are we putting relative pathnames in this\n> list to begin with? Why not use an absolute path when taking the\n> lock in all cases? (calling absolute_path() and using the result\n> to take the lock, storing it in the lock_file list, should not be\n> in the critical path, right? Not that I have measured it, of course! :)\n\nConservative :) I'm still scared from 044bbbc (Make git_dir a path\nrelative to work_tree in setup_work_tree() - 2008-06-19). But yeah\nlooking through \"grep hold_\" I think none of the locks is in critical\npath. absolute_path() can die() if cwd is longer than PATH_MAX (and\ndoing this reduces the chances of that happening). But René is adding\nstrbuf_getcwd() that can remove that PATH_MAX. So I guess we should be\nfine with putting absolute_path() in hold_lock_file_...*\n-- \nDuy\n"},{"id":"246457","messageId":"53CD227F.5070708@ramsay1.demon.co.uk","threadId":"37160","inReplyTo":"CACsJy8AXc4jvLPNpGyGdY9uzrnN-SbEeiksLDpS_=29gJ1KMnQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2014-07-21T14:23:59Z","receivedAt":"2014-07-21T14:23:59Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 21/07/14 14:47, Duy Nguyen wrote:\n> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones\n> wrote:\n>>> +void make_locked_paths_absolute(void)\n>>> +{\n>>> + struct lock_file *lk;\n>>> + for (lk = lock_file_list; lk != NULL; lk = lk->next) {\n>>> + if (lk->filename && !is_absolute_path(lk->filename)) {\n>>> + char *to_free = lk->filename;\n>>> + lk->filename = xstrdup(absolute_path(lk->filename));\n>>> + free(to_free);\n>>> + }\n>>> + }\n>>> +}\n>>\n>> I just have to ask, why are we putting relative pathnames in this\n>> list to begin with? Why not use an absolute path when taking the\n>> lock in all cases? (calling absolute_path() and using the result\n>> to take the lock, storing it in the lock_file list, should not be\n>> in the critical path, right? Not that I have measured it, of course! :)\n> \n> Conservative :) I'm still scared from 044bbbc (Make git_dir a path\n> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah\n> looking through \"grep hold_\" I think none of the locks is in critical\n> path. absolute_path() can die() if cwd is longer than PATH_MAX (and\n> doing this reduces the chances of that happening). But René is adding\n> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be\n> fine with putting absolute_path() in hold_lock_file_...*\n\nHmm, yes, thank you for reminding me about 044bbbc. So, yes it could\ncause a (small) performance hit and a change in behaviour (die) in\ndeeply nested working directories. Hmm, OK.\n\nATB,\nRamsay Jones\n"},{"id":"246463","messageId":"xmqqr41ek5hl.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"CACsJy8AXc4jvLPNpGyGdY9uzrnN-SbEeiksLDpS_=29gJ1KMnQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-07-21T17:04:54Z","receivedAt":"2014-07-21T17:04:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones\n> wrote:\n>>> +void make_locked_paths_absolute(void)\n>>> +{\n>>> + struct lock_file *lk;\n>>> + for (lk = lock_file_list; lk != NULL; lk = lk->next) {\n>>> + if (lk->filename && !is_absolute_path(lk->filename)) {\n>>> + char *to_free = lk->filename;\n>>> + lk->filename = xstrdup(absolute_path(lk->filename));\n>>> + free(to_free);\n>>> + }\n>>> + }\n>>> +}\n>>\n>> I just have to ask, why are we putting relative pathnames in this\n>> list to begin with? Why not use an absolute path when taking the\n>> lock in all cases? (calling absolute_path() and using the result\n>> to take the lock, storing it in the lock_file list, should not be\n>> in the critical path, right? Not that I have measured it, of course! :)\n>\n> Conservative :) I'm still scared from 044bbbc (Make git_dir a path\n> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah\n> looking through \"grep hold_\" I think none of the locks is in critical\n> path. absolute_path() can die() if cwd is longer than PATH_MAX (and\n> doing this reduces the chances of that happening). But René is adding\n> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be\n> fine with putting absolute_path() in hold_lock_file_...*\n\nOK, we should center these efforts around the strbuf_getcwd() topic,\nbasing the other topic on realpath() and this one on it then?\n"},{"id":"246553","messageId":"CACsJy8B6JpqOnbGZuKQPGrY1y8SyKzg+4aSP2iiM-Gb=3Jv5sw@mail.gmail.com","threadId":"37160","inReplyTo":"xmqqr41ek5hl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-23T11:55:31Z","receivedAt":"2014-07-23T11:55:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano wrote:\n> Duy Nguyen writes:\n>\n>> On Mon, Jul 21, 2014 at 8:27 PM, Ramsay Jones\n>> wrote:\n>>>> +void make_locked_paths_absolute(void)\n>>>> +{\n>>>> + struct lock_file *lk;\n>>>> + for (lk = lock_file_list; lk != NULL; lk = lk->next) {\n>>>> + if (lk->filename && !is_absolute_path(lk->filename)) {\n>>>> + char *to_free = lk->filename;\n>>>> + lk->filename = xstrdup(absolute_path(lk->filename));\n>>>> + free(to_free);\n>>>> + }\n>>>> + }\n>>>> +}\n>>>\n>>> I just have to ask, why are we putting relative pathnames in this\n>>> list to begin with? Why not use an absolute path when taking the\n>>> lock in all cases? (calling absolute_path() and using the result\n>>> to take the lock, storing it in the lock_file list, should not be\n>>> in the critical path, right? Not that I have measured it, of course! :)\n>>\n>> Conservative :) I'm still scared from 044bbbc (Make git_dir a path\n>> relative to work_tree in setup_work_tree() - 2008-06-19). But yeah\n>> looking through \"grep hold_\" I think none of the locks is in critical\n>> path. absolute_path() can die() if cwd is longer than PATH_MAX (and\n>> doing this reduces the chances of that happening). But René is adding\n>> strbuf_getcwd() that can remove that PATH_MAX. So I guess we should be\n>> fine with putting absolute_path() in hold_lock_file_...*\n>\n> OK, we should center these efforts around the strbuf_getcwd() topic,\n> basing the other topic on realpath() and this one on it then?\n\nOK.\n-- \nDuy\n"},{"id":"247012","messageId":"1406775673399-7616119.post@n2.nabble.com","threadId":"37160","inReplyTo":"CACsJy8B6JpqOnbGZuKQPGrY1y8SyKzg+4aSP2iiM-Gb=3Jv5sw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Yue Lin Ho","fromEmail":"yuelinho777@gmail.com","sentAt":"2014-07-31T03:01:13Z","receivedAt":"2014-07-31T03:01:13Z","isPatch":true,"sender":{"key":"yuelinho777@gmail.com","avatar":"https://gravatar.com/avatar/dbf9652003664c7518c86149f9e24df4d178c52526eaffab6f3c3d15b61671e2?d=mp&s=160"},"body":"Hi:\n\n> 2014-07-23 19:55 GMT+08:00 Duy Nguyen :\n> On Tue, Jul 22, 2014 at 12:04 AM, Junio C Hamano \n> wrote:\n> > Duy Nguyen writes:\n​[snip]​\n> > OK, we should center these efforts around the strbuf_getcwd() topic,\n> > basing the other topic on realpath() and this one on it then?\n> \n> OK.\n> --\n> Duy\n\n​Excuse me.\nHow do I trace these patches applied?\n\nI just fetch from https://github.com/gitster/git.git\nThen tried to find these patches if it is applied.\n(Seems not.)\n\nThen, I took a look at\nhttp://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Jul-2014-04-Tue-22-td7615627.html\nseems no related information there.\n\nSo, could you please tell me how to trace it?\n\nThank you. ^_^\n\nYue Lin Ho\n​\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7616119.html\nSent from the git mailing list archive at Nabble.com.\n"},{"id":"247024","messageId":"CACsJy8B9yJrQRSiCa2eQeeVZu_JPwjpykM1DXjVi3+JrKRF8Fg@mail.gmail.com","threadId":"37160","inReplyTo":"1406775673399-7616119.post@n2.nabble.com","subject":"Re: [PATCH v2 2/2] Make locked paths absolute when current directory is changed","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-31T09:58:43Z","receivedAt":"2014-07-31T09:58:43Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jul 31, 2014 at 10:01 AM, Yue Lin Ho wrote:\n> Hi:\n> How do I trace these patches applied?\n\nThey are not applied yet. I'll needto redo them on top of rs/strbuf-getcwd.\n-- \nDuy\n"},{"id":"247045","messageId":"1406814214-21725-1-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1405858399-23082-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 0/3] Keep .lock file paths absolute","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-31T13:43:31Z","receivedAt":"2014-07-31T13:43:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 requires rs/strbuf-getcwd, turns paths to absolute from the\nbeginning, and kills the last use of PATH_MAX in lockfile.c thanks to\nstrbuf_readlink().\n\nNguyễn Thái Ngọc Duy (3):\n lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)\n lockfile.c: remove PATH_MAX limit in resolve_symlink()\n lockfile.c: store absolute path\n\n cache.h | 2 +-\n lockfile.c | 95 +++++++++++++++++++------------------------\n t/t2107-update-index-basic.sh | 15 +++++++\n 3 files changed, 58 insertions(+), 54 deletions(-)\n\n-- \n2.1.0.rc0.78.gc0d8480\n"},{"id":"247046","messageId":"1406814214-21725-2-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1406814214-21725-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-31T13:43:32Z","receivedAt":"2014-07-31T13:43:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 2 +-\n lockfile.c | 56 ++++++++++++++++++++++++++++++++------------------------\n 2 files changed, 33 insertions(+), 25 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex cc46be4..0d8dce7 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -539,7 +539,7 @@ struct lock_file {\n \tint fd;\n \tpid_t owner;\n \tchar on_list;\n-\tchar filename[PATH_MAX];\n+\tchar *filename;\n };\n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NODEREF 2\ndiff --git a/lockfile.c b/lockfile.c\nindex 8fbcb6a..968b28f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -7,13 +7,19 @@\n static struct lock_file *lock_file_list;\n static const char *alternate_index_output;\n \n+static void clear_filename(struct lock_file *lk)\n+{\n+\tfree(lk->filename);\n+\tlk->filename = NULL;\n+}\n+\n static void remove_lock_file(void)\n {\n \tpid_t me = getpid();\n \n \twhile (lock_file_list) {\n \t\tif (lock_file_list->owner == me &&\n-\t\t lock_file_list->filename[0]) {\n+\t\t lock_file_list->filename) {\n \t\t\tif (lock_file_list->fd >= 0)\n \t\t\t\tclose(lock_file_list->fd);\n \t\t\tunlink_or_warn(lock_file_list->filename);\n@@ -77,10 +83,16 @@ static char *last_path_elm(char *p)\n * Always returns p.\n */\n \n-static char *resolve_symlink(char *p, size_t s)\n+static char *resolve_symlink(const char *in)\n {\n+\tstatic char p[PATH_MAX];\n+\tsize_t s = sizeof(p);\n \tint depth = MAXDEPTH;\n \n+\tif (strlen(in) >= sizeof(p))\n+\t\treturn NULL;\n+\tstrcpy(p, in);\n+\n \twhile (depth--) {\n \t\tchar link[PATH_MAX];\n \t\tint link_len = readlink(p, link, sizeof(link));\n@@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\t/*\n-\t * subtract 5 from size to make sure there's room for adding\n-\t * \".lock\" for the lock file name\n-\t */\n-\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n-\n-\tif (strlen(path) >= max_path_len)\n+\tint len;\n+\tif (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))\n \t\treturn -1;\n+\tlen = strlen(path) + 5; /* .lock */\n+\tlk->filename = xmallocz(len);\n \tstrcpy(lk->filename, path);\n-\tif (!(flags & LOCK_NODEREF))\n-\t\tresolve_symlink(lk->filename, max_path_len);\n \tstrcat(lk->filename, \".lock\");\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n@@ -153,7 +160,7 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \t\t\t\t lk->filename);\n \t}\n \telse\n-\t\tlk->filename[0] = 0;\n+\t\tclear_filename(lk);\n \treturn lk->fd;\n }\n \n@@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)\n \n int commit_lock_file(struct lock_file *lk)\n {\n-\tchar result_file[PATH_MAX];\n-\tsize_t i;\n-\tif (lk->fd >= 0 && close_lock_file(lk))\n+\tchar *result_file;\n+\tif ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n \t\treturn -1;\n-\tstrcpy(result_file, lk->filename);\n-\ti = strlen(result_file) - 5; /* .lock */\n-\tresult_file[i] = 0;\n-\tif (rename(lk->filename, result_file))\n+\tresult_file = xmemdupz(lk->filename,\n+\t\t\t strlen(lk->filename) - 5 /* .lock */);\n+\tif (rename(lk->filename, result_file)) {\n+\t\tfree(result_file);\n \t\treturn -1;\n-\tlk->filename[0] = 0;\n+\t}\n+\tfree(result_file);\n+\tclear_filename(lk);\n \treturn 0;\n }\n \n@@ -260,11 +268,11 @@ void set_alternate_index_output(const char *name)\n int commit_locked_index(struct lock_file *lk)\n {\n \tif (alternate_index_output) {\n-\t\tif (lk->fd >= 0 && close_lock_file(lk))\n+\t\tif ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n \t\t\treturn -1;\n \t\tif (rename(lk->filename, alternate_index_output))\n \t\t\treturn -1;\n-\t\tlk->filename[0] = 0;\n+\t\tclear_filename(lk);\n \t\treturn 0;\n \t}\n \telse\n@@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)\n \n void rollback_lock_file(struct lock_file *lk)\n {\n-\tif (lk->filename[0]) {\n+\tif (lk->filename) {\n \t\tif (lk->fd >= 0)\n \t\t\tclose(lk->fd);\n \t\tunlink_or_warn(lk->filename);\n \t}\n-\tlk->filename[0] = 0;\n+\tclear_filename(lk);\n }\n-- \n2.1.0.rc0.78.gc0d8480\n"},{"id":"247047","messageId":"1406814214-21725-3-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1406814214-21725-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 2/3] lockfile.c: remove PATH_MAX limit in resolve_symlink()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-31T13:43:33Z","receivedAt":"2014-07-31T13:43:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Signed-off-by: Nguyễn Thái Ngọc Duy \n---\n lockfile.c | 47 ++++++++++++++---------------------------------\n 1 file changed, 14 insertions(+), 33 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 968b28f..154915f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -85,52 +85,33 @@ static char *last_path_elm(char *p)\n \n static char *resolve_symlink(const char *in)\n {\n-\tstatic char p[PATH_MAX];\n-\tsize_t s = sizeof(p);\n+\tstatic struct strbuf p = STRBUF_INIT;\n+\tstruct strbuf link = STRBUF_INIT;\n \tint depth = MAXDEPTH;\n \n-\tif (strlen(in) >= sizeof(p))\n-\t\treturn NULL;\n-\tstrcpy(p, in);\n+\tstrbuf_reset(&p);\n+\tstrbuf_addstr(&p, in);\n \n \twhile (depth--) {\n-\t\tchar link[PATH_MAX];\n-\t\tint link_len = readlink(p, link, sizeof(link));\n-\t\tif (link_len < 0) {\n-\t\t\t/* not a symlink anymore */\n-\t\t\treturn p;\n-\t\t}\n-\t\telse if (link_len < sizeof(link))\n-\t\t\t/* readlink() never null-terminates */\n-\t\t\tlink[link_len] = '\\0';\n-\t\telse {\n-\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\treturn p;\n-\t\t}\n+\t\tif (strbuf_readlink(&link, p.buf, 0) < 0)\n+\t\t\tbreak;\t/* not a symlink anymore */\n \n-\t\tif (is_absolute_path(link)) {\n+\t\tif (is_absolute_path(link.buf)) {\n \t\t\t/* absolute path simply replaces p */\n-\t\t\tif (link_len < s)\n-\t\t\t\tstrcpy(p, link);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n-\t\t\t}\n+\t\t\tstrbuf_reset(&p);\n+\t\t\tstrbuf_addbuf(&p, &link);\n \t\t} else {\n \t\t\t/*\n \t\t\t * link is a relative path, so I must replace the\n \t\t\t * last element of p with it.\n \t\t\t */\n-\t\t\tchar *r = (char *)last_path_elm(p);\n-\t\t\tif (r - p + link_len < s)\n-\t\t\t\tstrcpy(r, link);\n-\t\t\telse {\n-\t\t\t\twarning(\"%s: symlink too long\", p);\n-\t\t\t\treturn p;\n-\t\t\t}\n+\t\t\tchar *r = (char *)last_path_elm(p.buf);\n+\t\t\tstrbuf_setlen(&p, r - p.buf);\n+\t\t\tstrbuf_addbuf(&p, &link);\n \t\t}\n \t}\n-\treturn p;\n+\tstrbuf_release(&link);\n+\treturn p.buf;\n }\n \n \n-- \n2.1.0.rc0.78.gc0d8480\n"},{"id":"247048","messageId":"1406814214-21725-4-git-send-email-pclouds@gmail.com","threadId":"37160","inReplyTo":"1406814214-21725-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 3/3] lockfile.c: store absolute path","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2014-07-31T13:43:34Z","receivedAt":"2014-07-31T13:43:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Locked paths can be saved in a linked list so that if something wrong\nhappens, *.lock are removed. For relative paths, this works fine if we\nkeep cwd the same, which is true 99% of time except:\n\n- update-index and read-tree hold the lock on $GIT_DIR/index really\n early, then later on may call setup_work_tree() to move cwd.\n\n- Suppose a lock is being held (e.g. by \"git add\") then somewhere\n down the line, somebody calls real_path (e.g. \"link_alt_odb_entry\"),\n which temporarily moves cwd away and back.\n\nDuring that time when cwd is moved (either permanently or temporarily)\nand we decide to die(), attempts to remove relative *.lock will fail,\nand the next operation will complain that some files are still locked.\n\nAvoid this case by turning relative paths to absolute before storing\nthe path in \"filename\" field.\n\nReported-by: Yue Lin Ho \nHelped-by: Ramsay Jones \nHelped-by: Johannes Sixt \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n lockfile.c | 10 +++++-----\n t/t2107-update-index-basic.sh | 15 +++++++++++++++\n 2 files changed, 20 insertions(+), 5 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 154915f..0aa70a5 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -117,13 +117,13 @@ static char *resolve_symlink(const char *in)\n \n static int lock_file(struct lock_file *lk, const char *path, int flags)\n {\n-\tint len;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n \tif (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))\n \t\treturn -1;\n-\tlen = strlen(path) + 5; /* .lock */\n-\tlk->filename = xmallocz(len);\n-\tstrcpy(lk->filename, path);\n-\tstrcat(lk->filename, \".lock\");\n+\tstrbuf_add_absolute_path(&sb, path);\n+\tstrbuf_addstr(&sb, \".lock\");\n+\tlk->filename = strbuf_detach(&sb, NULL);\n \tlk->fd = open(lk->filename, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (0 <= lk->fd) {\n \t\tif (!lock_file_list) {\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex 1bafb90..dfe02f4 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -65,4 +65,19 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '.lock files cleaned up' '\n+\tmkdir cleanup &&\n+\t(\n+\tcd cleanup &&\n+\tmkdir worktree &&\n+\tgit init repo &&\n+\tcd repo &&\n+\tgit config core.worktree ../../worktree &&\n+\t# --refresh triggers late setup_work_tree,\n+\t# active_cache_changed is zero, rollback_lock_file fails\n+\tgit update-index --refresh &&\n+\t! test -f .git/index.lock\n+\t)\n+'\n+\n test_done\n-- \n2.1.0.rc0.78.gc0d8480\n"},{"id":"247105","messageId":"xmqqfvhgw3q9.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"1406814214-21725-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-01T16:53:50Z","receivedAt":"2014-08-01T16:53:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n\nSomewhat underexplained, given that it seems to add some new\nsemantics.\n\n> +static void clear_filename(struct lock_file *lk)\n> +{\n> +\tfree(lk->filename);\n> +\tlk->filename = NULL;\n> +}\n\nIt is good to abstract out lk->filename[0] = '\\0', which used to be\nthe way we say that we are done with the lock. But I am somewhat\nsurprised to see that there aren't so many locations that used to\ncheck !!lk->filename[0] to see if we are done with the lock to require\na corresponding wrapper.\n\n> static void remove_lock_file(void)\n> {\n> \tpid_t me = getpid();\n> \n> \twhile (lock_file_list) {\n> \t\tif (lock_file_list->owner == me &&\n> -\t\t lock_file_list->filename[0]) {\n> +\t\t lock_file_list->filename) {\n\n... and this seems to be the only location?\n\n> @@ -124,17 +136,12 @@ static char *resolve_symlink(char *p, size_t s)\n> \n> static int lock_file(struct lock_file *lk, const char *path, int flags)\n> {\n> -\t/*\n> -\t * subtract 5 from size to make sure there's room for adding\n> -\t * \".lock\" for the lock file name\n> -\t */\n> -\tstatic const size_t max_path_len = sizeof(lk->filename) - 5;\n> -\n> -\tif (strlen(path) >= max_path_len)\n> +\tint len;\n> +\tif (!(flags & LOCK_NODEREF) && !(path = resolve_symlink(path)))\n> \t\treturn -1;\n\nSomehow I found it unnecessarily denser; had to read it twice before\ncaffeine kicked in ;-)\n\n> @@ -231,16 +238,17 @@ int close_lock_file(struct lock_file *lk)\n> \n> int commit_lock_file(struct lock_file *lk)\n> {\n> -\tchar result_file[PATH_MAX];\n> -\tsize_t i;\n> -\tif (lk->fd >= 0 && close_lock_file(lk))\n> +\tchar *result_file;\n> +\tif ((lk->fd >= 0 && close_lock_file(lk)) || !lk->filename)\n> \t\treturn -1;\n\nWe did not protect against somebody calling this with an already\nclosed lock, but we now return early without attempting renameing\netc., which is a good change but is not explained. Was there a\nspecific code path that you needed this change for?\n\nAlso the order of the check is not consistent with how the same\ncheck is done in rollback_lock_file(). The order you use in this\nnew code (and also in commit_locked_index()) may be better than the\nexisting order in the rollback code path; we want to see the fd\nclosed, if it is open, even if lk->filename has already been\ncleared. On the other hand, one could argue that anything such a\nbroken caller tells this function is suspicious, and we shouldn't\nclose random file descriptor that is likely not owned by the caller\nin the first place. I dunno.\n\n> @@ -273,10 +281,10 @@ int commit_locked_index(struct lock_file *lk)\n> \n> void rollback_lock_file(struct lock_file *lk)\n> {\n> -\tif (lk->filename[0]) {\n> +\tif (lk->filename) {\n> \t\tif (lk->fd >= 0)\n> \t\t\tclose(lk->fd);\n> \t\tunlink_or_warn(lk->filename);\n> \t}\n> -\tlk->filename[0] = 0;\n> +\tclear_filename(lk);\n> }\n"},{"id":"247116","messageId":"xmqqbns4w1us.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"1406814214-21725-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-01T17:34:19Z","receivedAt":"2014-08-01T17:34:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> diff --git a/lockfile.c b/lockfile.c\n> index 8fbcb6a..968b28f 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -7,13 +7,19 @@\n> static struct lock_file *lock_file_list;\n> static const char *alternate_index_output;\n> \n> +static void clear_filename(struct lock_file *lk)\n> +{\n> +\tfree(lk->filename);\n> +\tlk->filename = NULL;\n> +}\n> +\n\nGiven that you move commit_locked_index(), which you need to use\nthis function in, to read-cache.c in your own nd/split-index series,\nthis will need to be exposed to the wider world, and at that point,\nits name will turn out to be too generic.\n"},{"id":"247119","messageId":"xmqqtx5wuma8.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"xmqqfvhgw3q9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-01T17:55:59Z","receivedAt":"2014-08-01T17:55:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>\n> Somewhat underexplained, given that it seems to add some new\n> semantics.\n>\n>> +static void clear_filename(struct lock_file *lk)\n>> +{\n>> +\tfree(lk->filename);\n>> +\tlk->filename = NULL;\n>> +}\n>\n> It is good to abstract out lk->filename[0] = '\\0', which used to be\n> the way we say that we are done with the lock. But I am somewhat\n> surprised to see that there aren't so many locations that used to\n> check !!lk->filename[0] to see if we are done with the lock to require\n> a corresponding wrapper.\n>\n>> static void remove_lock_file(void)\n>> {\n>> \tpid_t me = getpid();\n>> \n>> \twhile (lock_file_list) {\n>> \t\tif (lock_file_list->owner == me &&\n>> -\t\t lock_file_list->filename[0]) {\n>> +\t\t lock_file_list->filename) {\n>\n> ... and this seems to be the only location?\n\nWhile looking at possible fallout of merging this topic to any\nbranch, I am starting to suspect that it is probably a bad idea for\nclear-filename to free lk->filename. I am wondering if it would be\nsafer to do:\n\n - in lock_file(), free lk->filename if it already exists before\n what you do in that function with your series;\n\n - update \"is this lock already held?\" check !!lk->filename[0] to\n check for (lk->filename && !!lk->filename[0]);\n\n - in clear_filename(), clear lk->filename[0] = '\\0', but do not\n free lk->filename itself.\n\nThen existing callers that never suspected that lk->filename can be\nNULL and thought that it does not need freeing can keep doing the\nsame thing as before without leaking nor breaking.\n\nIf we want to adopt the new world order at once, alternatively, you\ncan keep the code in this series but then lk->filename needs to be\nrenamed to something that the current code base has not heard of to\nforce breakage at the link time for us to notice.\n\nI grepped for 'lk->filename' and checked if the ones in read-cache.c\nand refs.c are OK (they seem to be), but that is not a very robust\ncheck.\n\nI dunno.\n"},{"id":"247185","messageId":"53DD2A54.1030403@web.de","threadId":"37160","inReplyTo":"xmqqtx5wuma8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-08-02T18:13:40Z","receivedAt":"2014-08-02T18:13:40Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 08/01/2014 07:55 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>> Somewhat underexplained, given that it seems to add some new\n>> semantics.\n>>\n>>> +static void clear_filename(struct lock_file *lk)\n>>> +{\n>>> +\tfree(lk->filename);\n>>> +\tlk->filename = NULL;\n>>> +}\n>> It is good to abstract out lk->filename[0] = '\\0', which used to be\n>> the way we say that we are done with the lock. But I am somewhat\n>> surprised to see that there aren't so many locations that used to\n>> check !!lk->filename[0] to see if we are done with the lock to require\n>> a corresponding wrapper.\n>>\n>>> static void remove_lock_file(void)\n>>> {\n>>> \tpid_t me = getpid();\n>>> \n>>> \twhile (lock_file_list) {\n>>> \t\tif (lock_file_list->owner == me &&\n>>> -\t\t lock_file_list->filename[0]) {\n>>> +\t\t lock_file_list->filename) {\n>> ... and this seems to be the only location?\n> While looking at possible fallout of merging this topic to any\n> branch, I am starting to suspect that it is probably a bad idea for\n> clear-filename to free lk->filename. I am wondering if it would be\n> safer to do:\n>\n> - in lock_file(), free lk->filename if it already exists before\n> what you do in that function with your series;\n>\n> - update \"is this lock already held?\" check !!lk->filename[0] to\n> check for (lk->filename && !!lk->filename[0]);\n>\n> - in clear_filename(), clear lk->filename[0] = '\\0', but do not\n> free lk->filename itself.\n>\n> Then existing callers that never suspected that lk->filename can be\n> NULL and thought that it does not need freeing can keep doing the\n> same thing as before without leaking nor breaking.\n>\n> If we want to adopt the new world order at once, alternatively, you\n> can keep the code in this series but then lk->filename needs to be\n> renamed to something that the current code base has not heard of to\n> force breakage at the link time for us to notice.\n>\n> I grepped for 'lk->filename' and checked if the ones in read-cache.c\n> and refs.c are OK (they seem to be), but that is not a very robust\n> check.\n>\n> I dunno.\n\nMy first impression reading this patch was to rename\nclear_filename() into free_and_clear_filename() or better free_filename(),\nbut I never pressed the send button ;-)\n\nReading the discussion above makes me wonder if lk->filename may be replaced by a strbuf\nsome day, and in this case clear_filename() will become reset_filenmae() ?\n"},{"id":"247205","messageId":"CACsJy8BAB3n5BRVaveTBrhdSDpiPBtm==TRjiv4ZR2P6iMne_w@mail.gmail.com","threadId":"37160","inReplyTo":"53DD2A54.1030403@web.de","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-08-04T10:13:41Z","receivedAt":"2014-08-04T10:13:41Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen wrote:\n> On 08/01/2014 07:55 PM, Junio C Hamano wrote:\n>>\n>> Junio C Hamano writes:\n>>\n>>> Nguyễn Thái Ngọc Duy writes:\n>>>\n>>>> Signed-off-by: Nguyễn Thái Ngọc Duy \n>>>\n>>> Somewhat underexplained, given that it seems to add some new\n>>> semantics.\n>>>\n>>>> +static void clear_filename(struct lock_file *lk)\n>>>> +{\n>>>> + free(lk->filename);\n>>>> + lk->filename = NULL;\n>>>> +}\n>>>\n>>> It is good to abstract out lk->filename[0] = '\\0', which used to be\n>>> the way we say that we are done with the lock. But I am somewhat\n>>> surprised to see that there aren't so many locations that used to\n>>> check !!lk->filename[0] to see if we are done with the lock to require\n>>> a corresponding wrapper.\n>>>\n>>>> static void remove_lock_file(void)\n>>>> {\n>>>> pid_t me = getpid();\n>>>> while (lock_file_list) {\n>>>> if (lock_file_list->owner == me &&\n>>>> - lock_file_list->filename[0]) {\n>>>> + lock_file_list->filename) {\n>>>\n>>> ... and this seems to be the only location?\n>>\n>> While looking at possible fallout of merging this topic to any\n>> branch, I am starting to suspect that it is probably a bad idea for\n>> clear-filename to free lk->filename. I am wondering if it would be\n>> safer to do:\n>>\n>> - in lock_file(), free lk->filename if it already exists before\n>> what you do in that function with your series;\n>>\n>> - update \"is this lock already held?\" check !!lk->filename[0] to\n>> check for (lk->filename && !!lk->filename[0]);\n>>\n>> - in clear_filename(), clear lk->filename[0] = '\\0', but do not\n>> free lk->filename itself.\n>>\n>> Then existing callers that never suspected that lk->filename can be\n>> NULL and thought that it does not need freeing can keep doing the\n>> same thing as before without leaking nor breaking.\n>>\n>> If we want to adopt the new world order at once, alternatively, you\n>> can keep the code in this series but then lk->filename needs to be\n>> renamed to something that the current code base has not heard of to\n>> force breakage at the link time for us to notice.\n>>\n>> I grepped for 'lk->filename' and checked if the ones in read-cache.c\n>> and refs.c are OK (they seem to be), but that is not a very robust\n>> check.\n>>\n>> I dunno.\n>\n>\n> My first impression reading this patch was to rename\n> clear_filename() into free_and_clear_filename() or better free_filename(),\n> but I never pressed the send button ;-)\n>\n> Reading the discussion above makes me wonder if lk->filename may be replaced\n> by a strbuf\n> some day, and in this case clear_filename() will become reset_filenmae() ?\n\nI didn't realize Mike is making a lot more changes in lockfile.c, part\nof that is converting lk->filename to use strbuf [1]. Perhaps I should\njust withdraw this series, wait until Mike's series is merged, then\nredo 3/3 on top. Or Mike could just take 3/3 in as part of his series.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232\n-- \nDuy\n"},{"id":"247217","messageId":"xmqqtx5srw1t.fsf@gitster.dls.corp.google.com","threadId":"37160","inReplyTo":"CACsJy8BAB3n5BRVaveTBrhdSDpiPBtm==TRjiv4ZR2P6iMne_w@mail.gmail.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-04T17:42:22Z","receivedAt":"2014-08-04T17:42:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I didn't realize Mike is making a lot more changes in lockfile.c, part\n> of that is converting lk->filename to use strbuf [1]. Perhaps I should\n> just withdraw this series, wait until Mike's series is merged, then\n> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.\n\nDuring the pre-release freeze I would like to see new topics be\ncalmer ;-) Serializing or not, inter-developer coordination is\nalways very much appreciated.\n\nThanks.\n"},{"id":"247276","messageId":"53E101F0.5090408@alum.mit.edu","threadId":"37160","inReplyTo":"CACsJy8BAB3n5BRVaveTBrhdSDpiPBtm==TRjiv4ZR2P6iMne_w@mail.gmail.com","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-08-05T16:10:24Z","receivedAt":"2014-08-05T16:10:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/04/2014 03:13 AM, Duy Nguyen wrote:\n> On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen wrote:\n> [...]\n>> My first impression reading this patch was to rename\n>> clear_filename() into free_and_clear_filename() or better free_filename(),\n>> but I never pressed the send button ;-)\n>>\n>> Reading the discussion above makes me wonder if lk->filename may be replaced\n>> by a strbuf\n>> some day, and in this case clear_filename() will become reset_filenmae() ?\n> \n> I didn't realize Mike is making a lot more changes in lockfile.c, part\n> of that is converting lk->filename to use strbuf [1]. Perhaps I should\n> just withdraw this series, wait until Mike's series is merged, then\n> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series.\n> \n> [1] http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232\n\nI've neglected my patch series for ages (sorry!) The last round of\nreview pointed out a couple of places where lock_file objects were still\nbeing left in undefined states, and since then it also bit-rotted.\n\nOver the past few days I re-rolled the patch series and fixed some more\ncode paths. I still want to check it over before submitting it to the\nlist, but if you are interested the current version is here [1].\n\nDuy, I'll try to look at your patches, but probably won't get to it\nuntil next week when I return from vacation.\n\nMichael\n\n[1] https://github.com/mhagger/git branch \"lock-correctness\"\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"248775","messageId":"1409731221311-7617967.post@n2.nabble.com","threadId":"37160","inReplyTo":"53E101F0.5090408@alum.mit.edu","subject":"Re: [PATCH v3 1/3] lockfile.c: remove PATH_MAX limitation (except in resolve_symlink)","fromName":"Yue Lin Ho","fromEmail":"yuelinho777@gmail.com","sentAt":"2014-09-03T08:00:21Z","receivedAt":"2014-09-03T08:00:21Z","isPatch":true,"sender":{"key":"yuelinho777@gmail.com","avatar":"https://gravatar.com/avatar/dbf9652003664c7518c86149f9e24df4d178c52526eaffab6f3c3d15b61671e2?d=mp&s=160"},"body":"Hi Michael:\n\n> On 08/04/2014 03:13 AM, Duy Nguyen wrote:\n>\n>> On Sun, Aug 3, 2014 at 1:13 AM, Torsten Bögershausen <[hidden email]>\n>> wrote: \n>> [...] \n>>> My first impression reading this patch was to rename \n>>> clear_filename() into free_and_clear_filename() or better\n>>> free_filename(), \n>>> but I never pressed the send button ;-) \n>>> \n>>> Reading the discussion above makes me wonder if lk->filename may be\n>>> replaced \n>>> by a strbuf \n>>> some day, and in this case clear_filename() will become reset_filenmae()\n>>> ? \n>> \n>> I didn't realize Mike is making a lot more changes in lockfile.c, part \n>> of that is converting lk->filename to use strbuf [1]. Perhaps I should \n>> just withdraw this series, wait until Mike's series is merged, then \n>> redo 3/3 on top. Or Mike could just take 3/3 in as part of his series. \n>> \n>> [1]\n>> http://thread.gmane.org/gmane.comp.version-control.git/246222/focus=246232\n> \n> I've neglected my patch series for ages (sorry!) The last round of \n> review pointed out a couple of places where lock_file objects were still \n> being left in undefined states, and since then it also bit-rotted. \n> \n> Over the past few days I re-rolled the patch series and fixed some more \n> code paths. I still want to check it over before submitting it to the \n> list, but if you are interested the current version is here [1]. \n> \n> Duy, I'll try to look at your patches, but probably won't get to it \n> until next week when I return from vacation. \n> \n> Michael \n> \n> [1] https://github.com/mhagger/git branch \"lock-correctness\" \n\n​I am tracing the lock path issue.\n(http://git.661346.n2.nabble.com/git-update-index-not-delete-lock-file-when-using-different-worktree-td7615300.html)\n\nand I see mh/lockfile part in \nhttp://git.661346.n2.nabble.com/What-s-cooking-in-git-git-Sep-2014-01-Tue-2-td7617955.html\nas following:\n\n* mh/lockfile (2014-04-15) 25 commits\n . trim_last_path_elm(): replace last_path_elm()\n . resolve_symlink(): take a strbuf parameter\n . resolve_symlink(): use a strbuf for internal scratch space\n . change lock_file::filename into a strbuf\n . commit_lock_file(): use a strbuf to manage temporary space\n . try_merge_strategy(): use a statically-allocated lock_file object\n . try_merge_strategy(): remove redundant lock_file allocation\n . struct lock_file: declare some fields volatile\n . lockfile: avoid transitory invalid states\n . commit_lock_file(): die() if called for unlocked lockfile object\n . commit_lock_file(): inline temporary variable\n . remove_lock_file(): call rollback_lock_file()\n . lock_file(): exit early if lockfile cannot be opened\n . write_packed_entry_fn(): convert cb_data into a (const int *)\n . prepare_index(): declare return value to be (const char *)\n . delete_ref_loose(): don't muck around in the lock_file's filename\n . cache.h: define constants LOCK_SUFFIX and LOCK_SUFFIX_LEN\n . lockfile.c: document the various states of lock_file objects\n . lock_file(): always add lock_file object to lock_file_list\n . hold_lock_file_for_append(): release lock on errors\n . lockfile: unlock file if lockfile permissions cannot be adjusted\n . rollback_lock_file(): set fd to -1\n . rollback_lock_file(): do not clear filename redundantly\n . api-lockfile: expand the documentation\n . unable_to_lock_die(): rename function from unable_to_lock_index_die()\n\n Expecting a reroll.\n\nSo, do you have any plan about mh/lockfile and the lock path issue?\n\nThank you.​ ^_^\n\nYue Lin Ho\n\n\n\n--\nView this message in context: http://git.661346.n2.nabble.com/PATCH-Make-locked-paths-absolute-when-current-directory-is-changed-tp7615398p7617967.html\nSent from the git mailing list archive at Nabble.com.\n"}]}