{"thread":{"id":"45764","subject":"[PATCH 0/6] removing more calls to git_path()","startedAt":"2017-04-20T21:08:01Z","lastAt":"2017-04-21T04:06:11Z","messageCount":8,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"317396","messageId":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","threadId":"45764","inReplyTo":null,"subject":"[PATCH 0/6] removing more calls to git_path()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:07:54Z","receivedAt":"2017-04-20T21:08:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After the discussion about the git_path()-related bug in:\n\n  http://public-inbox.org/git/20170329080820.8084-1-chriscool@tuxfamily.org/\n\nI looked over some of the calls. When I introduced git_pathdup() a few\nyears ago I converted most of the dangerous git_path() calls. But there\nwere still some left, and of course new ones have been added. This patch\ncleans up some of the low-hanging fruit.\n\nThere's probably more that could be done, but this is just what I\nhappened to produce when looking through some of the cases the other\nday. I don't think any of these is fixing a triggerable bug; they can\nall be considered cleanup.\n\n  [1/6]: bisect: add git_path_bisect_terms helper\n  [2/6]: branch: add edit_description() helper\n  [3/6]: use git_path_* helper functions\n  [4/6]: replace xstrdup(git_path(...)) with git_pathdup(...)\n  [5/6]: replace strbuf_addstr(git_path()) with git_path_buf()\n  [6/6]: am: drop \"dir\" parameter from am_state_init\n\n bisect.c           |  3 ++-\n builtin/am.c       | 10 ++++------\n builtin/branch.c   |  6 +++---\n builtin/commit.c   |  6 +++---\n builtin/config.c   |  5 +++--\n builtin/pull.c     |  4 ++--\n builtin/worktree.c |  6 ++----\n fast-import.c      |  2 +-\n notes-merge.c      |  4 ++--\n sequencer.c        | 12 ++++++------\n 10 files changed, 28 insertions(+), 30 deletions(-)\n\n"},{"id":"317397","messageId":"20170420210825.lr55sl7se4uloncf@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 1/6] bisect: add git_path_bisect_terms helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:08:25Z","receivedAt":"2017-04-20T21:08:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids using the dangerous git_path(). Right now\nthere's only one call site (because the writing half is\nstill part of the shell script), but it may come in handy in\nthe future as more of bisect is written in C. It also\nmatches how we access the other BISECT_* files.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n bisect.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 03af06c66..08c9fb726 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -432,6 +432,7 @@ static int read_bisect_refs(void)\n \n static GIT_PATH_FUNC(git_path_bisect_names, \"BISECT_NAMES\")\n static GIT_PATH_FUNC(git_path_bisect_expected_rev, \"BISECT_EXPECTED_REV\")\n+static GIT_PATH_FUNC(git_path_bisect_terms, \"BISECT_TERMS\")\n \n static void read_bisect_paths(struct argv_array *array)\n {\n@@ -906,7 +907,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)\n void read_bisect_terms(const char **read_bad, const char **read_good)\n {\n \tstruct strbuf str = STRBUF_INIT;\n-\tconst char *filename = git_path(\"BISECT_TERMS\");\n+\tconst char *filename = git_path_bisect_terms();\n \tFILE *fp = fopen(filename, \"r\");\n \n \tif (!fp) {\n-- \n2.13.0.rc0.363.g8726c260e\n\n"},{"id":"317398","messageId":"20170420210840.hufdbroebp2dpcsz@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 2/6] branch: add edit_description() helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:08:41Z","receivedAt":"2017-04-20T21:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Rather than have a variable with a short name that is fed to\ngit_path(), let's add a helper function that returns the\nfull path. This avoids the dangerous git_path() function.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0552c42ad..48a513a84 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -504,7 +504,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \tstrbuf_release(&newsection);\n }\n \n-static const char edit_description[] = \"BRANCH_DESCRIPTION\";\n+static GIT_PATH_FUNC(edit_description, \"EDIT_DESCRIPTION\")\n \n static int edit_branch_description(const char *branch_name)\n {\n@@ -519,9 +519,9 @@ static int edit_branch_description(const char *branch_name)\n \t\t      \"  %s\\n\"\n \t\t      \"Lines starting with '%c' will be stripped.\\n\"),\n \t\t    branch_name, comment_line_char);\n-\twrite_file_buf(git_path(edit_description), buf.buf, buf.len);\n+\twrite_file_buf(edit_description(), buf.buf, buf.len);\n \tstrbuf_reset(&buf);\n-\tif (launch_editor(git_path(edit_description), &buf, NULL)) {\n+\tif (launch_editor(edit_description(), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\n \t\treturn -1;\n \t}\n-- \n2.13.0.rc0.363.g8726c260e\n\n"},{"id":"317399","messageId":"20170420210854.npnbs6rfbtc4ohgx@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 3/6] use git_path_* helper functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:08:54Z","receivedAt":"2017-04-20T21:09:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Long ago we added functions like git_path_merge_msg() to\nreplace the more dangerous git_path(\"MERGE_MSG\"). Over time\nsome new calls to the latter have crept it. Let's convert\nthem to use the safer form.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/commit.c |  6 +++---\n builtin/pull.c   |  4 ++--\n sequencer.c      | 12 ++++++------\n 3 files changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 4e288bc51..98927d962 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -821,9 +821,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t\t\"If this is not correct, please remove the file\\n\"\n \t\t\t\t\t\"\t%s\\n\"\n \t\t\t\t\t\"and try again.\\n\"),\n-\t\t\t\tgit_path(whence == FROM_MERGE\n-\t\t\t\t\t ? \"MERGE_HEAD\"\n-\t\t\t\t\t : \"CHERRY_PICK_HEAD\"));\n+\t\t\t\twhence == FROM_MERGE ?\n+\t\t\t\t\tgit_path_merge_head() :\n+\t\t\t\t\tgit_path_cherry_pick_head());\n \t\t}\n \n \t\tfprintf(s->fp, \"\\n\");\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex d8aa26d8a..dd1a4a94e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -332,7 +332,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)\n  */\n static void get_merge_heads(struct oid_array *merge_heads)\n {\n-\tconst char *filename = git_path(\"FETCH_HEAD\");\n+\tconst char *filename = git_path_fetch_head();\n \tFILE *fp;\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct object_id oid;\n@@ -791,7 +791,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"pull\");\n \n-\tif (file_exists(git_path(\"MERGE_HEAD\")))\n+\tif (file_exists(git_path_merge_head()))\n \t\tdie_conclude_merge();\n \n \tif (get_oid(\"HEAD\", &orig_head))\ndiff --git a/sequencer.c b/sequencer.c\nindex 77afecaeb..130cc868e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1065,12 +1065,12 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t\t\tflags |= CLEANUP_MSG;\n \t\t\tmsg_file = rebase_path_fixup_msg();\n \t\t} else {\n-\t\t\tconst char *dest = git_path(\"SQUASH_MSG\");\n+\t\t\tconst char *dest = git_path_squash_msg();\n \t\t\tunlink(dest);\n \t\t\tif (copy_file(dest, rebase_path_squash_msg(), 0666))\n \t\t\t\treturn error(_(\"could not rename '%s' to '%s'\"),\n \t\t\t\t\t     rebase_path_squash_msg(), dest);\n-\t\t\tunlink(git_path(\"MERGE_MSG\"));\n+\t\t\tunlink(git_path_merge_msg());\n \t\t\tmsg_file = dest;\n \t\t\tflags |= EDIT_MSG;\n \t\t}\n@@ -1820,10 +1820,10 @@ static int error_failed_squash(struct commit *commit,\n \t\treturn error(_(\"could not rename '%s' to '%s'\"),\n \t\t\trebase_path_squash_msg(), rebase_path_message());\n \tunlink(rebase_path_fixup_msg());\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tif (copy_file(git_path(\"MERGE_MSG\"), rebase_path_message(), 0666))\n+\tunlink(git_path_merge_msg());\n+\tif (copy_file(git_path_merge_msg(), rebase_path_message(), 0666))\n \t\treturn error(_(\"could not copy '%s' to '%s'\"),\n-\t\t\t     rebase_path_message(), git_path(\"MERGE_MSG\"));\n+\t\t\t     rebase_path_message(), git_path_merge_msg());\n \treturn error_with_patch(commit, subject, subject_len, opts, 1, 0);\n }\n \n@@ -2167,7 +2167,7 @@ static int commit_staged_changes(struct replay_opts *opts)\n \tif (has_unstaged_changes(1))\n \t\treturn error(_(\"cannot rebase: You have unstaged changes.\"));\n \tif (!has_uncommitted_changes(0)) {\n-\t\tconst char *cherry_pick_head = git_path(\"CHERRY_PICK_HEAD\");\n+\t\tconst char *cherry_pick_head = git_path_cherry_pick_head();\n \n \t\tif (file_exists(cherry_pick_head) && unlink(cherry_pick_head))\n \t\t\treturn error(_(\"could not remove CHERRY_PICK_HEAD\"));\n-- \n2.13.0.rc0.363.g8726c260e\n\n"},{"id":"317400","messageId":"20170420210909.ehy4kim6losllux7@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 4/6] replace xstrdup(git_path(...)) with git_pathdup(...)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:09:09Z","receivedAt":"2017-04-20T21:09:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's more efficient to use git_pathdup(), as it skips an\nextra copy of the path. And by removing some calls to\ngit_path(), it makes it easier to audit for dangerous uses.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/config.c | 5 +++--\n fast-import.c    | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 4f49a0edb..3407a8b87 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -597,8 +597,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\tif (given_config_source.blob)\n \t\t\tdie(\"editing blobs is not supported\");\n \t\tgit_config(git_default_config, NULL);\n-\t\tconfig_file = xstrdup(given_config_source.file ?\n-\t\t\t\t      given_config_source.file : git_path(\"config\"));\n+\t\tconfig_file = given_config_source.file ?\n+\t\t\t\txstrdup(given_config_source.file) :\n+\t\t\t\tgit_pathdup(\"config\");\n \t\tif (use_global_config) {\n \t\t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \t\t\tif (fd >= 0) {\ndiff --git a/fast-import.c b/fast-import.c\nindex 1ea3a0860..cf58f875b 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3202,7 +3202,7 @@ static char* make_fast_import_path(const char *path)\n {\n \tif (!relative_marks_paths || is_absolute_path(path))\n \t\treturn xstrdup(path);\n-\treturn xstrdup(git_path(\"info/fast-import/%s\", path));\n+\treturn git_pathdup(\"info/fast-import/%s\", path);\n }\n \n static void option_import_marks(const char *marks,\n-- \n2.13.0.rc0.363.g8726c260e\n\n"},{"id":"317401","messageId":"20170420210930.uh4kirjqs4x2zwhg@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 5/6] replace strbuf_addstr(git_path()) with git_path_buf()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:09:30Z","receivedAt":"2017-04-20T21:09:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Writing directly into the strbuf avoids a useless copy of\nthe data, and dropping calls to git_path() makes it easier\nto audit for dangerous calls.\n\nNote that git_path() does an implicit strbuf_reset(), but in\neach of these cases we were either already doing that reset,\nor writing into a fresh strbuf anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/worktree.c | 6 ++----\n notes-merge.c      | 4 ++--\n 2 files changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex 9993ded41..57caa0855 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -106,8 +106,7 @@ static void prune_worktrees(void)\n \t\t\tprintf(\"%s\\n\", reason.buf);\n \t\tif (show_only)\n \t\t\tcontinue;\n-\t\tstrbuf_reset(&path);\n-\t\tstrbuf_addstr(&path, git_path(\"worktrees/%s\", d->d_name));\n+\t\tgit_path_buf(&path, \"worktrees/%s\", d->d_name);\n \t\tret = remove_dir_recursively(&path, 0);\n \t\tif (ret < 0 && errno == ENOTDIR)\n \t\t\tret = unlink(path.buf);\n@@ -215,8 +214,7 @@ static int add_worktree(const char *path, const char *refname,\n \t}\n \n \tname = worktree_basename(path, &len);\n-\tstrbuf_addstr(&sb_repo,\n-\t\t      git_path(\"worktrees/%.*s\", (int)(path + len - name), name));\n+\tgit_path_buf(&sb_repo, \"worktrees/%.*s\", (int)(path + len - name), name);\n \tlen = sb_repo.len;\n \tif (safe_create_leading_directories_const(sb_repo.buf))\n \t\tdie_errno(_(\"could not create leading directories of '%s'\"),\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 5998605ac..32caaaff7 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -676,7 +676,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tconst char *msg = strstr(buffer, \"\\n\\n\");\n \tint baselen;\n \n-\tstrbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));\n+\tgit_path_buf(&path, NOTES_MERGE_WORKTREE);\n \tif (o->verbosity >= 3)\n \t\tprintf(\"Committing notes in notes merge worktree at %s\\n\",\n \t\t\tpath.buf);\n@@ -741,7 +741,7 @@ int notes_merge_abort(struct notes_merge_options *o)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \n-\tstrbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));\n+\tgit_path_buf(&buf, NOTES_MERGE_WORKTREE);\n \tif (o->verbosity >= 3)\n \t\tprintf(\"Removing notes merge worktree at %s/*\\n\", buf.buf);\n \tret = remove_dir_recursively(&buf, REMOVE_DIR_KEEP_TOPLEVEL);\n-- \n2.13.0.rc0.363.g8726c260e\n\n"},{"id":"317402","messageId":"20170420210935.dx2old5l6yuakqhq@sigill.intra.peff.net","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"[PATCH 6/6] am: drop \"dir\" parameter from am_state_init","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-20T21:09:35Z","receivedAt":"2017-04-20T21:09:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The only caller of this function passes in a static buffer\nreturned from git_path(). This looks dangerous at first\nglance, but turns out to be OK because the first thing we do\nis xstrdup() the result.\n\nLet's turn this into a git_pathdup(). That's slightly more\nefficient (no extra copy), and makes it easier to audit for\ndangerous git_path() invocations.\n\nSince there's only a single caller, let's just set this\ndefault path inside the init function. That makes the memory\nownership clear.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c | 10 ++++------\n 1 file changed, 4 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f7a7a971f..97849d4dc 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -134,17 +134,15 @@ struct am_state {\n };\n \n /**\n- * Initializes am_state with the default values. The state directory is set to\n- * dir.\n+ * Initializes am_state with the default values.\n  */\n-static void am_state_init(struct am_state *state, const char *dir)\n+static void am_state_init(struct am_state *state)\n {\n \tint gpgsign;\n \n \tmemset(state, 0, sizeof(*state));\n \n-\tassert(dir);\n-\tstate->dir = xstrdup(dir);\n+\tstate->dir = git_pathdup(\"rebase-apply\");\n \n \tstate->prec = 4;\n \n@@ -2322,7 +2320,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \tgit_config(git_am_config, NULL);\n \n-\tam_state_init(&state, git_path(\"rebase-apply\"));\n+\tam_state_init(&state);\n \n \tin_progress = am_in_progress(&state);\n \tif (in_progress)\n-- \n2.13.0.rc0.363.g8726c260e\n"},{"id":"317454","messageId":"xmqqbmrq5rt9.fsf@gitster.mtv.corp.google.com","threadId":"45764","inReplyTo":"20170420210754.bzrnc74dpjs7fgpf@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] removing more calls to git_path()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-21T04:05:06Z","receivedAt":"2017-04-21T04:06:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All patches in the series looked sensible. Thanks.\n"}]}