{"thread":{"id":"40042","subject":"[PATCH 0/17] removing questionable uses of git_path","startedAt":"2015-08-10T09:27:32Z","lastAt":"2015-08-15T09:05:27Z","messageCount":34,"participants":["Jeff King","Michael Haggerty","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":17},"messages":[{"id":"267706","messageId":"20150810092731.GA9027@sigill.intra.peff.net","threadId":"40042","inReplyTo":null,"subject":"[PATCH 0/17] removing questionable uses of git_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:27:32Z","receivedAt":"2015-08-10T09:27:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Recently Michael and I were working on a patch series (not yet\npublished), which did something like:\n\n\n  const char *path = git_path(\"foo\");\n\n  ... do stuff with path ...\n\n  for_each_ref(some_callback, NULL);\n\n  ... do some other stuff ...\n\n  unlink(path);\n\nClever readers may have spotted the bug immediately, but we did not,\nuntil we found that random loose refs were being deleted from the\nrepository.\n\nThe problem is that git_path uses a static buffer that gets overwritten\nby subsequent calls. The ref code uses it to iterate over all of the\nloose refs in a directory, so our original path is trashed before\nfor_each_ref returns. Except to make it even more exciting, git_path\nactually has a ring of _four_ buffers, so any trivial test you write\nwill probably work just fine; it's only when you use a real repository\nthat it causes problems (and then, only if the code path is such that\nthe loose refs were not previously accessed and cached!).\n\nMichael likened git_path to \"a hand-grenade with the pin pulled out\",\nand I tend to agree. On the other hand, it's pretty darn useful to be\nable to get a quick path without having to deal with memory allocation\nand ownership.  This patch series tries to document the danger, and\nremove some of the more questionable uses. I don't know whether this is\nfixing any actual latent bugs; I traced a number of the code paths\nmanually, but never found a bug. There were some near misses, though,\nwhich make me believe that seemingly-unrelated refactoring could\nintroduce a bug.\n\nI stopped short of trying to eradicate git_path entirely, and settled\nfor:\n\n  git grep -E '[^_](git_|mk)path\\('\n\nproducing a fairly tame-looking set of function calls. It's OK to pass\nthe result of git_path() to a system call, or something that is a thin\nwrapper around one (e.g., strbuf_read_file).\n\nI think this takes us most of the way there. I left out a few cases\nwhere introducing allocations would have been awkward, and I verified\nthat there were no bugs (e.g., rerere_path). And I left out a few spots\nthat conflict with topics in \"next\" (and luckily, in all cases what is\nin next makes the problem go away, so we do not have to follow-up for\nthose sites).\n\nAlong the way, there are a few cleanups (e.g., I polished off the recent\nhold_lock_file_for_append topic which was on the list, as it had some\nproblematic calls).\n\n  [01/17]: cache.h: clarify documentation for git_path, et al\n  [02/17]: cache.h: complete set of git_path_submodule helpers\n  [03/17]: t5700: modernize style\n  [04/17]: add_to_alternates_file: don't add duplicate entries\n  [05/17]: remove hold_lock_file_for_append\n  [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases\n  [07/17]: prefer mkpathdup to mkpath in assignments\n  [08/17]: remote.c: drop extraneous local variable from migrate_file\n  [09/17]: refs.c: remove extra git_path calls from read_loose refs\n  [10/17]: path.c: drop git_path_submodule\n  [11/17]: refs.c: simplify strbufs in reflog setup and writing\n  [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log\n  [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic\n  [14/17]: refs.c: remove_empty_directories can take a strbuf\n  [15/17]: find_hook: keep our own static buffer\n  [16/17]: get_repo_path: refactor path-allocation\n  [17/17]: memoize common git-path \"constant\" files\n\n-Peff\n"},{"id":"267707","messageId":"20150810093207.GA30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 01/17] cache.h: clarify documentation for git_path, et al","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:32:07Z","receivedAt":"2015-08-10T09:32:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The comment above these functions actually describes\nsha1_file_name, and comes from the very first revision of\ngit. Commit 723c31f (Add \"git_path()\" and \"head_ref()\"\nhelper functions., 2005-07-05) added git_path, pushing the\ncomment away from the function it describes; later commits\nadded more functions in this block.\n\nLet's fix the comment to describe these related functions in\nmore detail. Let's also make sure to point out their safer\nalternatives (and move those alternatives below, which makes\nmore sense when reading the file).\n\nNote that we do not need to move the existing comment to\nsha1_file_name.  Commit d40d535 (sha1_file.c: document a\nbunch of functions defined in the file, 2014-02-21) already\nadded a much more descriptive comment to it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h | 17 ++++++++++++-----\n 1 file changed, 12 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6bb7119..8db884e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -708,6 +708,18 @@ extern int check_repository_format(void);\n #define DATA_CHANGED    0x0020\n #define TYPE_CHANGED    0x0040\n \n+/*\n+ * Return a statically allocated filename, either generically (mkpath), in\n+ * the repository directory (git_path), or in a submodule's repository\n+ * directory (git_path_submodule). In all cases, note that the result\n+ * may be overwritten by another call to _any_ of the functions. Consider\n+ * using the safer \"dup\" or \"strbuf\" formats below.\n+ */\n+extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n+extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n+extern const char *git_path_submodule(const char *path, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n+\n extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n@@ -717,11 +729,6 @@ extern char *git_pathdup(const char *fmt, ...)\n extern char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n \n-/* Return a statically allocated filename matching the sha1 signature */\n-extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern const char *git_path_submodule(const char *path, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n extern void report_linked_checkout_garbage(void);\n \n /*\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267708","messageId":"20150810093222.GB30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 02/17] cache.h: complete set of git_path_submodule helpers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:32:22Z","receivedAt":"2015-08-10T09:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git_path function has \"git_pathdup\" and\n\"strbuf_git_path\" variants, but git_submodule_path only\ncomes in the dangerous, static-buffer variant. That makes\nrefactoring callers to use the safer functions hard (since\nthey don't exist).\n\nSince we're already using a strbuf behind the scenes, it's\neasy to expose all three of these interfaces with thin\nwrappers.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |  5 +++++\n path.c  | 35 ++++++++++++++++++++++++++++++-----\n 2 files changed, 35 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 8db884e..6f74f33 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -724,10 +724,15 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n+extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,\n+\t\t\t\t      const char *fmt, ...)\n+\t__attribute__((format (printf, 3, 4)));\n extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n extern char *mkpathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n+extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n \n extern void report_linked_checkout_garbage(void);\n \ndiff --git a/path.c b/path.c\nindex 10f4cbf..9aad9a1 100644\n--- a/path.c\n+++ b/path.c\n@@ -224,11 +224,10 @@ const char *mkpath(const char *fmt, ...)\n \treturn cleanup_path(pathname->buf);\n }\n \n-const char *git_path_submodule(const char *path, const char *fmt, ...)\n+static void do_submodule_path(struct strbuf *buf, const char *path,\n+\t\t\t      const char *fmt, va_list args)\n {\n-\tstruct strbuf *buf = get_pathname();\n \tconst char *git_dir;\n-\tva_list args;\n \n \tstrbuf_addstr(buf, path);\n \tif (buf->len && buf->buf[buf->len - 1] != '/')\n@@ -242,13 +241,39 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)\n \t}\n \tstrbuf_addch(buf, '/');\n \n-\tva_start(args, fmt);\n \tstrbuf_vaddf(buf, fmt, args);\n-\tva_end(args);\n \tstrbuf_cleanup_path(buf);\n+}\n+\n+const char *git_path_submodule(const char *path, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tstruct strbuf *buf = get_pathname();\n+\tva_start(args, fmt);\n+\tdo_submodule_path(buf, path, fmt, args);\n+\tva_end(args);\n \treturn buf->buf;\n }\n \n+char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tva_start(args, fmt);\n+\tdo_submodule_path(&buf, path, fmt, args);\n+\tva_end(args);\n+\treturn strbuf_detach(&buf, NULL);\n+}\n+\n+void strbuf_git_path_submodule(struct strbuf *buf, const char *path,\n+\t\t\t       const char *fmt, ...)\n+{\n+\tva_list args;\n+\tva_start(args, fmt);\n+\tdo_submodule_path(buf, path, fmt, args);\n+\tva_end(args);\n+}\n+\n int validate_headref(const char *path)\n {\n \tstruct stat st;\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267709","messageId":"20150810093229.GC30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 03/17] t5700: modernize style","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:32:30Z","receivedAt":"2015-08-10T09:32:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The early part of this test is rather old, and does not\nfollow our usual style guidelines. In particular:\n\n  - the tests liberally chdir, and expect out-of-test \"cd\"\n    commands to return them to a sane state\n\n  - test commands aren't indented at all\n\n  - there are a lot of minor formatting nits, like the\n    opening quote of the test block on the wrong line,\n    spaces after \">\", etc\n\nThis patch fixes the style issues, and uses a few helper\nfunctions, along with subshells and \"git -C\", to avoid\nchanging the cwd of the main script.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5700-clone-reference.sh | 193 +++++++++++++++++++--------------------------\n 1 file changed, 81 insertions(+), 112 deletions(-)\n\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex 3e783fc..51d131a 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -10,49 +10,51 @@ base_dir=`pwd`\n \n U=$base_dir/UPLOAD_LOG\n \n-test_expect_success 'preparing first repository' \\\n-'test_create_repo A && cd A &&\n-echo first > file1 &&\n-git add file1 &&\n-git commit -m initial'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'preparing second repository' \\\n-'git clone A B && cd B &&\n-echo second > file2 &&\n-git add file2 &&\n-git commit -m addition &&\n-git repack -a -d &&\n-git prune'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'cloning with reference (-l -s)' \\\n-'git clone -l -s --reference B A C'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'existence of info/alternates' \\\n-'test_line_count = 2 C/.git/objects/info/alternates'\n-\n-cd \"$base_dir\"\n+# create a commit in repo $1 with name $2\n+commit_in () {\n+\t(\n+\t\tcd \"$1\" &&\n+\t\techo \"$2\" >\"$2\" &&\n+\t\tgit add \"$2\" &&\n+\t\tgit commit -m \"$2\"\n+\t)\n+}\n+\n+# check that there are $2 loose objects in repo $1\n+test_objcount () {\n+\techo \"$2\" >expect &&\n+\tgit -C \"$1\" count-objects >actual.raw &&\n+\tcut -d' ' -f1 <actual.raw >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success 'preparing first repository' '\n+\ttest_create_repo A &&\n+\tcommit_in A file1\n+'\n \n-test_expect_success 'pulling from reference' \\\n-'cd C &&\n-git pull ../B master'\n+test_expect_success 'preparing second repository' '\n+\tgit clone A B &&\n+\tcommit_in B file2 &&\n+\tgit -C B repack -ad &&\n+\tgit -C B prune\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'cloning with reference (-l -s)' '\n+\tgit clone -l -s --reference B A C\n+'\n \n-test_expect_success 'that reference gets used' \\\n-'cd C &&\n-echo \"0 objects, 0 kilobytes\" > expected &&\n-git count-objects > current &&\n-test_cmp expected current'\n+test_expect_success 'existence of info/alternates' '\n+\ttest_line_count = 2 C/.git/objects/info/alternates\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'pulling from reference' '\n+\tgit -C C pull ../B master\n+'\n \n-rm -f \"$U.D\"\n+test_expect_success 'that reference gets used' '\n+\ttest_objcount C 0\n+'\n \n test_expect_success 'cloning with reference (no -l -s)' '\n \tGIT_TRACE_PACKET=$U.D git clone --reference B \"file://$(pwd)/A\" D\n@@ -63,95 +65,64 @@ test_expect_success 'fetched no objects' '\n \t! grep \" want\" \"$U.D\"\n '\n \n-cd \"$base_dir\"\n-\n-test_expect_success 'existence of info/alternates' \\\n-'test_line_count = 1 D/.git/objects/info/alternates'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'pulling from reference' \\\n-'cd D && git pull ../B master'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'that reference gets used' \\\n-'cd D && echo \"0 objects, 0 kilobytes\" > expected &&\n-git count-objects > current &&\n-test_cmp expected current'\n-\n-cd \"$base_dir\"\n+test_expect_success 'existence of info/alternates' '\n+\ttest_line_count = 1 D/.git/objects/info/alternates\n+'\n \n-test_expect_success 'updating origin' \\\n-'cd A &&\n-echo third > file3 &&\n-git add file3 &&\n-git commit -m update &&\n-git repack -a -d &&\n-git prune'\n+test_expect_success 'pulling from reference' '\n+\tgit -C D pull ../B master\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'that reference gets used' '\n+\ttest_objcount D 0\n+'\n \n-test_expect_success 'pulling changes from origin' \\\n-'cd C &&\n-git pull origin'\n+test_expect_success 'updating origin' '\n+\tcommit_in A file3 &&\n+\tgit -C A repack -ad &&\n+\tgit -C A prune\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'pulling changes from origin' '\n+\tgit -C C pull origin\n+'\n \n # the 2 local objects are commit and tree from the merge\n-test_expect_success 'that alternate to origin gets used' \\\n-'cd C &&\n-echo \"2 objects\" > expected &&\n-git count-objects | cut -d, -f1 > current &&\n-test_cmp expected current'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'pulling changes from origin' \\\n-'cd D &&\n-git pull origin'\n+test_expect_success 'that alternate to origin gets used' '\n+\ttest_objcount C 2\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'pulling changes from origin' '\n+\tgit -C D pull origin\n+'\n \n # the 5 local objects are expected; file3 blob, commit in A to add it\n # and its tree, and 2 are our tree and the merge commit.\n-test_expect_success 'check objects expected to exist locally' \\\n-'cd D &&\n-echo \"5 objects\" > expected &&\n-git count-objects | cut -d, -f1 > current &&\n-test_cmp expected current'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'preparing alternate repository #1' \\\n-'test_create_repo F && cd F &&\n-echo first > file1 &&\n-git add file1 &&\n-git commit -m initial'\n-\n-cd \"$base_dir\"\n-\n-test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' \\\n-'git clone F G && cd F &&\n-echo second > file2 &&\n-git add file2 &&\n-git commit -m addition'\n-\n-cd \"$base_dir\"\n+test_expect_success 'check objects expected to exist locally' '\n+\ttest_objcount D 5\n+'\n \n-test_expect_success 'cloning alternate repo #1, using #2 as reference' \\\n-'git clone --reference G F H'\n+test_expect_success 'preparing alternate repository #1' '\n+\ttest_create_repo F &&\n+\tcommit_in F file1\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' '\n+\tgit clone F G &&\n+\tcommit_in F file2\n+'\n \n-test_expect_success 'cloning with reference being subset of source (-l -s)' \\\n-'git clone -l -s --reference A B E'\n+test_expect_success 'cloning alternate repo #1, using #2 as reference' '\n+\tgit clone --reference G F H\n+'\n \n-cd \"$base_dir\"\n+test_expect_success 'cloning with reference being subset of source (-l -s)' '\n+\tgit clone -l -s --reference A B E\n+'\n \n test_expect_success 'clone with reference from a tagged repository' '\n \t(\n-\t\tcd A && git tag -a -m 'tagged' HEAD\n+\t\tcd A && git tag -a -m tagged HEAD\n \t) &&\n \tgit clone --reference=A A I\n '\n@@ -168,8 +139,6 @@ test_expect_success 'prepare branched repository' '\n \t)\n '\n \n-rm -f \"$U.K\"\n-\n test_expect_success 'fetch with incomplete alternates' '\n \tgit init K &&\n \techo \"$base_dir/A/.git/objects\" >K/.git/objects/info/alternates &&\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267710","messageId":"20150810093446.GD30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 04/17] add_to_alternates_file: don't add duplicate entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:34:46Z","receivedAt":"2015-08-10T09:34:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The add_to_alternates_file function blindly uses\nhold_lock_file_for_append to copy the existing contents, and\nthen adds the new line to it. This has two minor problems:\n\n  1. We might add duplicate entries, which are ugly and\n     inefficient.\n\n  2. We do not check that the file ends with a newline, in\n     which case we would bogusly append to the final line.\n     This is quite unlikely in practice, though, as we call\n     this function only from git-clone, so presumably we are\n     the only writers of the file (and we always add a\n     newline).\n\nInstead of using hold_lock_file_for_append, let's copy the\nfile line by line, which ensures all records are properly\nterminated. If we see an extra line, we can simply abort the\nupdate (there is no point in even copying the rest, as we\nknow that it would be identical to the original).\n\nAs a bonus, we also get rid of some calls to the\nstatic-buffer mkpath and git_path functions.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis is a polishing of the thread at:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/270341\n\n sha1_file.c                | 47 +++++++++++++++++++++++++++++++++++++++-------\n t/t5700-clone-reference.sh |  5 +++++\n 2 files changed, 45 insertions(+), 7 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 1cee438..3400b8b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -404,13 +404,46 @@ void read_info_alternates(const char * relative_base, int depth)\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n-\tconst char *alt = mkpath(\"%s\\n\", reference);\n-\twrite_or_die(fd, alt, strlen(alt));\n-\tif (commit_lock_file(lock))\n-\t\tdie(\"could not close alternates file\");\n-\tif (alt_odb_tail)\n-\t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n+\tchar *alts = git_pathdup(\"objects/info/alternates\");\n+\tFILE *in, *out;\n+\n+\thold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);\n+\tout = fdopen_lock_file(lock, \"w\");\n+\tif (!out)\n+\t\tdie_errno(\"unable to fdopen alternates lockfile\");\n+\n+\tin = fopen(alts, \"r\");\n+\tif (in) {\n+\t\tstruct strbuf line = STRBUF_INIT;\n+\t\tint found = 0;\n+\n+\t\twhile (strbuf_getline(&line, in, '\\n') != EOF) {\n+\t\t\tif (!strcmp(reference, line.buf)) {\n+\t\t\t\tfound = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfprintf_or_die(out, \"%s\\n\", line.buf);\n+\t\t}\n+\n+\t\tstrbuf_release(&line);\n+\t\tfclose(in);\n+\n+\t\tif (found) {\n+\t\t\trollback_lock_file(lock);\n+\t\t\tlock = NULL;\n+\t\t}\n+\t}\n+\telse if (errno != ENOENT)\n+\t\tdie_errno(\"unable to read alternates file\");\n+\n+\tif (lock) {\n+\t\tfprintf_or_die(out, \"%s\\n\", reference);\n+\t\tif (commit_lock_file(lock))\n+\t\t\tdie_errno(\"unable to move new alternates file into place\");\n+\t\tif (alt_odb_tail)\n+\t\t\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+\t}\n+\tfree(alts);\n }\n \n int foreach_alt_odb(alt_odb_fn fn, void *cb)\ndiff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh\nindex 51d131a..ef1779f 100755\n--- a/t/t5700-clone-reference.sh\n+++ b/t/t5700-clone-reference.sh\n@@ -120,6 +120,11 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' '\n \tgit clone -l -s --reference A B E\n '\n \n+test_expect_success 'cloning with multiple references drops duplicates' '\n+\tgit clone -s --reference B --reference A --reference B A dups &&\n+\ttest_line_count = 2 dups/.git/objects/info/alternates\n+'\n+\n test_expect_success 'clone with reference from a tagged repository' '\n \t(\n \t\tcd A && git tag -a -m tagged HEAD\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267711","messageId":"20150810093514.GE30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 05/17] remove hold_lock_file_for_append","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:35:14Z","receivedAt":"2015-08-10T09:35:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jim Hill <gjthill@gmail.com>\n\nNo users of hold_lock_file_for_append remain, so remove it.\n\nhold_lock_file_for_append copies its target file internally.\nThis makes it too heavyweight for true append-only logging\nand too limited for anything else (which probably wants to\nprocess the contents). It shouldn't be used.\n\n[jk: tweaked commit message, and dropped declaration and\n     documentation, too]\n\nSigned-off-by: Jim Hill <gjthill@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAnd this is the obvious continuation of the last patch.\n\n Documentation/technical/api-lockfile.txt | 26 ++++++++--------------\n lockfile.c                               | 38 --------------------------------\n lockfile.h                               |  7 ++----\n 3 files changed, 11 insertions(+), 60 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 93b5f23..0f5c481 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -40,7 +40,7 @@ The caller:\n \n * Attempts to create a lockfile by passing that variable and the path\n   of the final destination (e.g. `$GIT_DIR/index`) to\n-  `hold_lock_file_for_update` or `hold_lock_file_for_append`.\n+  `hold_lock_file_for_update`.\n \n * Writes new content for the destination file by either:\n \n@@ -64,8 +64,7 @@ When finished writing, the caller can:\n \n Even after the lockfile is committed or rolled back, the `lock_file`\n object must not be freed or altered by the caller. However, it may be\n-reused; just pass it to another call of `hold_lock_file_for_update` or\n-`hold_lock_file_for_append`.\n+reused; just pass it to another call of `hold_lock_file_for_update`.\n \n If the program exits before you have called one of `commit_lock_file`,\n `commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an\n@@ -111,8 +110,7 @@ appropriately, do their best to roll back the lockfile, and return -1.\n Flags\n -----\n \n-The following flags can be passed to `hold_lock_file_for_update` or\n-`hold_lock_file_for_append`:\n+The following flags can be passed to `hold_lock_file_for_update`:\n \n LOCK_NO_DEREF::\n \n@@ -141,12 +139,6 @@ hold_lock_file_for_update::\n \tabove). Attempt to create a lockfile for the destination and\n \treturn the file descriptor for writing to the file.\n \n-hold_lock_file_for_append::\n-\n-\tLike `hold_lock_file_for_update`, but before returning copy\n-\tthe existing contents of the file (if any) to the lockfile and\n-\tposition its write pointer at the end of the file.\n-\n fdopen_lock_file::\n \n \tAssociate a stdio stream with the lockfile. Return NULL\n@@ -162,8 +154,8 @@ get_locked_file_path::\n commit_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\n-\tearlier call to `hold_lock_file_for_update` or\n-\t`hold_lock_file_for_append`, close the file descriptor, and\n+\tearlier call to `hold_lock_file_for_update`,\n+\tclose the file descriptor, and\n \trename the lockfile to its final destination. Return 0 upon\n \tsuccess. On failure, roll back the lock file and return -1,\n \twith `errno` set to the value from the failing call to\n@@ -180,8 +172,8 @@ commit_lock_file_to::\n rollback_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\n-\tearlier call to `hold_lock_file_for_update` or\n-\t`hold_lock_file_for_append`, close the file descriptor and\n+\tearlier call to `hold_lock_file_for_update`,\n+\tclose the file descriptor and\n \tremove the lockfile. It is a NOOP to call\n \t`rollback_lock_file()` for a `lock_file` object that has\n \talready been committed or rolled back.\n@@ -189,8 +181,8 @@ rollback_lock_file::\n close_lock_file::\n \n \tTake a pointer to the `struct lock_file` initialized with an\n-\tearlier call to `hold_lock_file_for_update` or\n-\t`hold_lock_file_for_append`. Close the file descriptor (and\n+\tearlier call to `hold_lock_file_for_update`.\n+\tClose the file descriptor (and\n \tthe file pointer if it has been opened using\n \t`fdopen_lock_file`). Return 0 upon success. On failure to\n \t`close(2)`, return a negative value and roll back the lock\ndiff --git a/lockfile.c b/lockfile.c\nindex 993bb82..b1ceec6 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -249,44 +249,6 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,\n \treturn fd;\n }\n \n-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n-{\n-\tint fd, orig_fd;\n-\n-\tfd = lock_file(lk, path, flags);\n-\tif (fd < 0) {\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_die(path, errno);\n-\t\treturn fd;\n-\t}\n-\n-\torig_fd = open(path, O_RDONLY);\n-\tif (orig_fd < 0) {\n-\t\tif (errno != ENOENT) {\n-\t\t\tint save_errno = errno;\n-\n-\t\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\t\tdie(\"cannot open '%s' for copying\", path);\n-\t\t\trollback_lock_file(lk);\n-\t\t\terror(\"cannot open '%s' for copying\", path);\n-\t\t\terrno = save_errno;\n-\t\t\treturn -1;\n-\t\t}\n-\t} else if (copy_fd(orig_fd, fd)) {\n-\t\tint save_errno = errno;\n-\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tdie(\"failed to prepare '%s' for appending\", path);\n-\t\tclose(orig_fd);\n-\t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n-\t\treturn -1;\n-\t} else {\n-\t\tclose(orig_fd);\n-\t}\n-\treturn fd;\n-}\n-\n FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n {\n \tif (!lk->active)\ndiff --git a/lockfile.h b/lockfile.h\nindex b4abc61..1373f5c 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -27,8 +27,8 @@\n  *   soon as the object is used in any way, it is irrevocably\n  *   registered in the lock_file_list, and on_list is set.\n  *\n- * - Locked, lockfile open (after hold_lock_file_for_update(),\n- *   hold_lock_file_for_append(), or reopen_lock_file()). In this\n+ * - Locked, lockfile open (after hold_lock_file_for_update()\n+ *   or reopen_lock_file()). In this\n  *   state:\n  *   - the lockfile exists\n  *   - active is set\n@@ -85,9 +85,6 @@ static inline int hold_lock_file_for_update(\n \treturn hold_lock_file_for_update_timeout(lk, path, flags, 0);\n }\n \n-extern int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n-\t\t\t\t     int flags);\n-\n extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267712","messageId":"20150810093531.GF30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 06/17] prefer git_pathdup to git_path in some possibly-dangerous cases","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:35:31Z","receivedAt":"2015-08-10T09:35:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because git_path uses a static buffer that is shared with\ncalls to git_path, mkpath, etc, it can be dangerous to\nassign the result to a variable or pass it to a non-trivial\nfunction. The value may change unexpectedly due to other\ncalls.\n\nNone of the cases changed here has a known bug, but they're\nworth converting away from git_path because:\n\n  1. It's easy to use git_pathdup in these cases.\n\n  2. They use constructs (like assignment) that make it\n     hard to tell whether they're safe or not.\n\nThe extra malloc overhead should be trivial, as an\nallocation should be an order of magnitude cheaper than a\nsystem call (which we are clearly about to make, since we\nare constructing a filename). The real cost is that we must\nremember to free the result.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/fsck.c |  4 +++-\n fast-import.c  |  4 +++-\n http-backend.c |  3 ++-\n notes-merge.c  |  3 ++-\n refs.c         | 14 ++++++++------\n unpack-trees.c |  4 +++-\n 6 files changed, 21 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex f4b87e9..0794703 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -243,13 +243,14 @@ static void check_unreachable_object(struct object *obj)\n \t\t\tprintf(\"dangling %s %s\\n\", typename(obj->type),\n \t\t\t       sha1_to_hex(obj->sha1));\n \t\tif (write_lost_and_found) {\n-\t\t\tconst char *filename = git_path(\"lost-found/%s/%s\",\n+\t\t\tchar *filename = git_pathdup(\"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n \t\t\t\tsha1_to_hex(obj->sha1));\n \t\t\tFILE *f;\n \n \t\t\tif (safe_create_leading_directories_const(filename)) {\n \t\t\t\terror(\"Could not create lost-found\");\n+\t\t\t\tfree(filename);\n \t\t\t\treturn;\n \t\t\t}\n \t\t\tif (!(f = fopen(filename, \"w\")))\n@@ -262,6 +263,7 @@ static void check_unreachable_object(struct object *obj)\n \t\t\tif (fclose(f))\n \t\t\t\tdie_errno(\"Could not finish '%s'\",\n \t\t\t\t\t  filename);\n+\t\t\tfree(filename);\n \t\t}\n \t\treturn;\n \t}\ndiff --git a/fast-import.c b/fast-import.c\nindex 2ad4fee..ad8848b 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -407,7 +407,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);\n \n static void write_crash_report(const char *err)\n {\n-\tconst char *loc = git_path(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n+\tchar *loc = git_pathdup(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n \tFILE *rpt = fopen(loc, \"w\");\n \tstruct branch *b;\n \tunsigned long lu;\n@@ -415,6 +415,7 @@ static void write_crash_report(const char *err)\n \n \tif (!rpt) {\n \t\terror(\"can't write crash report %s: %s\", loc, strerror(errno));\n+\t\tfree(loc);\n \t\treturn;\n \t}\n \n@@ -488,6 +489,7 @@ static void write_crash_report(const char *err)\n \tfputs(\"-------------------\\n\", rpt);\n \tfputs(\"END OF CRASH REPORT\\n\", rpt);\n \tfclose(rpt);\n+\tfree(loc);\n }\n \n static void end_packfile(void);\ndiff --git a/http-backend.c b/http-backend.c\nindex b977c00..bac40ef 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -164,7 +164,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \n static void send_local_file(const char *the_type, const char *name)\n {\n-\tconst char *p = git_path(\"%s\", name);\n+\tchar *p = git_pathdup(\"%s\", name);\n \tsize_t buf_alloc = 8192;\n \tchar *buf = xmalloc(buf_alloc);\n \tint fd;\n@@ -191,6 +191,7 @@ static void send_local_file(const char *the_type, const char *name)\n \t}\n \tclose(fd);\n \tfree(buf);\n+\tfree(p);\n }\n \n static void get_text_file(char *name)\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 0b2b82c..b3d1dab 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -295,7 +295,7 @@ static void write_buf_to_worktree(const unsigned char *obj,\n \t\t\t\t  const char *buf, unsigned long size)\n {\n \tint fd;\n-\tconst char *path = git_path(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n+\tchar *path = git_pathdup(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n \tif (safe_create_leading_directories_const(path))\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n \tif (file_exists(path))\n@@ -320,6 +320,7 @@ static void write_buf_to_worktree(const unsigned char *obj,\n \t}\n \n \tclose(fd);\n+\tfree(path);\n }\n \n static void write_note_to_worktree(const unsigned char *obj,\ndiff --git a/refs.c b/refs.c\nindex 2db2975..93b250e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)\n  */\n static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n {\n-\tconst char *packed_refs_file;\n+\tchar *packed_refs_file;\n \n \tif (*refs->name)\n-\t\tpacked_refs_file = git_path_submodule(refs->name, \"packed-refs\");\n+\t\tpacked_refs_file = git_pathdup_submodule(refs->name, \"packed-refs\");\n \telse\n-\t\tpacked_refs_file = git_path(\"packed-refs\");\n+\t\tpacked_refs_file = git_pathdup(\"packed-refs\");\n \n \tif (refs->packed &&\n \t    !stat_validity_check(&refs->packed->validity, packed_refs_file))\n@@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)\n \t\t\tfclose(f);\n \t\t}\n \t}\n+\tfree(packed_refs_file);\n \treturn refs->packed;\n }\n \n@@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,\n {\n \tint fd, len;\n \tchar buffer[128], *p;\n-\tconst char *path;\n+\tchar *path;\n \n \tif (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)\n \t\treturn -1;\n \tpath = *refs->name\n-\t\t? git_path_submodule(refs->name, \"%s\", refname)\n-\t\t: git_path(\"%s\", refname);\n+\t\t? git_pathdup_submodule(refs->name, \"%s\", refname)\n+\t\t: git_pathdup(\"%s\", refname);\n \tfd = open(path, O_RDONLY);\n+\tfree(path);\n \tif (fd < 0)\n \t\treturn resolve_gitlink_packed_ref(refs, refname, sha1);\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex d6cf849..7bb446a 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1029,10 +1029,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tif (!core_apply_sparse_checkout || !o->update)\n \t\to->skip_sparse_checkout = 1;\n \tif (!o->skip_sparse_checkout) {\n-\t\tif (add_excludes_from_file_to_list(git_path(\"info/sparse-checkout\"), \"\", 0, &el, 0) < 0)\n+\t\tchar *sparse = git_pathdup(\"info/sparse-checkout\");\n+\t\tif (add_excludes_from_file_to_list(sparse, \"\", 0, &el, 0) < 0)\n \t\t\to->skip_sparse_checkout = 1;\n \t\telse\n \t\t\to->el = &el;\n+\t\tfree(sparse);\n \t}\n \n \tmemset(&o->result, 0, sizeof(o->result));\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267713","messageId":"20150810093538.GG30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 07/17] prefer mkpathdup to mkpath in assignments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:35:38Z","receivedAt":"2015-08-10T09:35:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As with the previous commit to git_path, assigning the\nresult of mkpath is suspicious, since it is not clear\nwhether we will still depend on the value after it may have\nbeen overwritten by subsequent calls. This patch converts\nlow-hanging fruit to use mkpathdup instead of mkpath (with\nthe downside that we must remember to free the result).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/repack.c | 24 +++++++++++++-----------\n refs.c           |  6 ++++--\n 2 files changed, 17 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex af7340c..70b9b1e 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -285,8 +285,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tfailed = 0;\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tconst char *fname_old;\n-\t\t\tchar *fname;\n+\t\t\tchar *fname, *fname_old;\n \t\t\tfname = mkpathdup(\"%s/pack-%s%s\", packdir,\n \t\t\t\t\t\titem->string, exts[ext].name);\n \t\t\tif (!file_exists(fname)) {\n@@ -294,7 +293,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tfname_old = mkpath(\"%s/old-%s%s\", packdir,\n+\t\t\tfname_old = mkpathdup(\"%s/old-%s%s\", packdir,\n \t\t\t\t\t\titem->string, exts[ext].name);\n \t\t\tif (file_exists(fname_old))\n \t\t\t\tif (unlink(fname_old))\n@@ -302,10 +301,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \n \t\t\tif (!failed && rename(fname, fname_old)) {\n \t\t\t\tfree(fname);\n+\t\t\t\tfree(fname_old);\n \t\t\t\tfailed = 1;\n \t\t\t\tbreak;\n \t\t\t} else {\n \t\t\t\tstring_list_append(&rollback, fname);\n+\t\t\t\tfree(fname_old);\n \t\t\t}\n \t\t}\n \t\tif (failed)\n@@ -314,13 +315,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \tif (failed) {\n \t\tstruct string_list rollback_failure = STRING_LIST_INIT_DUP;\n \t\tfor_each_string_list_item(item, &rollback) {\n-\t\t\tconst char *fname_old;\n-\t\t\tchar *fname;\n+\t\t\tchar *fname, *fname_old;\n \t\t\tfname = mkpathdup(\"%s/%s\", packdir, item->string);\n-\t\t\tfname_old = mkpath(\"%s/old-%s\", packdir, item->string);\n+\t\t\tfname_old = mkpathdup(\"%s/old-%s\", packdir, item->string);\n \t\t\tif (rename(fname_old, fname))\n \t\t\t\tstring_list_append(&rollback_failure, fname);\n \t\t\tfree(fname);\n+\t\t\tfree(fname_old);\n \t\t}\n \n \t\tif (rollback_failure.nr) {\n@@ -368,13 +369,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t/* Remove the \"old-\" files */\n \tfor_each_string_list_item(item, &names) {\n \t\tfor (ext = 0; ext < ARRAY_SIZE(exts); ext++) {\n-\t\t\tconst char *fname;\n-\t\t\tfname = mkpath(\"%s/old-%s%s\",\n-\t\t\t\t\tpackdir,\n-\t\t\t\t\titem->string,\n-\t\t\t\t\texts[ext].name);\n+\t\t\tchar *fname;\n+\t\t\tfname = mkpathdup(\"%s/old-%s%s\",\n+\t\t\t\t\t  packdir,\n+\t\t\t\t\t  item->string,\n+\t\t\t\t\t  exts[ext].name);\n \t\t\tif (remove_path(fname))\n \t\t\t\twarning(_(\"removing '%s' failed\"), fname);\n+\t\t\tfree(fname);\n \t\t}\n \t}\n \ndiff --git a/refs.c b/refs.c\nindex 93b250e..e70941a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3380,7 +3380,7 @@ static int commit_ref_update(struct ref_lock *lock,\n int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\t  const char *logmsg)\n {\n-\tconst char *lockpath;\n+\tchar *lockpath = NULL;\n \tchar ref[1000];\n \tint fd, len, written;\n \tchar *git_HEAD = git_pathdup(\"%s\", ref_target);\n@@ -3407,7 +3407,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \t\terror(\"refname too long: %s\", refs_heads_master);\n \t\tgoto error_free_return;\n \t}\n-\tlockpath = mkpath(\"%s.lock\", git_HEAD);\n+\tlockpath = mkpathdup(\"%s.lock\", git_HEAD);\n \tfd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);\n \tif (fd < 0) {\n \t\terror(\"Unable to open %s for writing\", lockpath);\n@@ -3427,9 +3427,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,\n \terror_unlink_return:\n \t\tunlink_or_warn(lockpath);\n \terror_free_return:\n+\t\tfree(lockpath);\n \t\tfree(git_HEAD);\n \t\treturn -1;\n \t}\n+\tfree(lockpath);\n \n #ifndef NO_SYMLINK_HEAD\n \tdone:\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267714","messageId":"20150810093549.GH30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 08/17] remote.c: drop extraneous local variable from migrate_file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:35:49Z","receivedAt":"2015-08-10T09:35:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's an anti-pattern to assign the result of git_path to a\nvariable, since other calls may reuse our buffer. In this\ncase, we feed the result to unlink_or_warn immediately\nafterwards, so it's OK. However, it's nice to avoid\nassignment entirely, which makes it more obvious that\nthere's no bug.\n\nWe can just pass the result directly to unlink_or_warn,\nwhich is a known-simple function. As a bonus, the code flow\nis a little more obvious, as we eliminate an extra\nconditional (a reader does not have to wonder any more\n\"under which circumstances is 'path' set?\").\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/remote.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex cc3c741..181668d 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -581,7 +581,6 @@ static int migrate_file(struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tint i;\n-\tconst char *path = NULL;\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n \tfor (i = 0; i < remote->url_nr; i++)\n@@ -601,11 +600,9 @@ static int migrate_file(struct remote *remote)\n \t\t\treturn error(_(\"Could not append '%s' to '%s'\"),\n \t\t\t\t\tremote->fetch_refspec[i], buf.buf);\n \tif (remote->origin == REMOTE_REMOTES)\n-\t\tpath = git_path(\"remotes/%s\", remote->name);\n+\t\tunlink_or_warn(git_path(\"remotes/%s\", remote->name));\n \telse if (remote->origin == REMOTE_BRANCHES)\n-\t\tpath = git_path(\"branches/%s\", remote->name);\n-\tif (path)\n-\t\tunlink_or_warn(path);\n+\t\tunlink_or_warn(git_path(\"branches/%s\", remote->name));\n \treturn 0;\n }\n \n-- \n2.5.0.414.g670f2a4\n"},{"id":"267716","messageId":"20150810093619.GI30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 09/17] refs.c: remove extra git_path calls from read_loose_refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:36:19Z","receivedAt":"2015-08-10T09:36:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In iterating over the loose refs in \"refs/foo/\", we keep a\nrunning strbuf with \"refs/foo/one\", \"refs/foo/two\", etc. But\nwe also need to access these files in the filesystem, as\n\".git/refs/foo/one\", etc. For this latter purpose, we make a\nseries of independent calls to git_path(). These are safe\n(we only use the result to call stat()), but assigning the\nresult of git_path is a suspicious pattern that we'd rather\navoid.\n\nThis patch keeps a running buffer with \".git/refs/foo/\", and\nwe can just append/reset each directory element as we loop.\nThis matches how we handle the refnames. It should also be\nmore efficient, as we do not keep formatting the same\n\".git/refs/foo\" prefix (which can be arbitrarily deep).\n\nTechnically we are dropping a call to strbuf_cleanup() on\neach generated filename, but that's OK; it wasn't doing\nanything, as we are putting in single-level names we read\nfrom the filesystem (so it could not possibly be cleaning up\ncruft like \"./\" in this instance).\n\nA clever reader may also note that the running refname\nbuffer (\"refs/foo/\") is actually a subset of the filesystem\npath buffer (\".git/refs/foo/\"). We could get by with one\nbuffer, indexing the length of $GIT_DIR when we want the\nrefname. However, having tried this, the resulting code\nactually ends up a little more confusing, and the efficiency\nimprovement is tiny (and almost certainly dwarfed by the\nsystem calls we are making).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e70941a..06f95c4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1352,19 +1352,23 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n {\n \tstruct ref_cache *refs = dir->ref_cache;\n \tDIR *d;\n-\tconst char *path;\n \tstruct dirent *de;\n \tint dirnamelen = strlen(dirname);\n \tstruct strbuf refname;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tsize_t path_baselen;\n \n \tif (*refs->name)\n-\t\tpath = git_path_submodule(refs->name, \"%s\", dirname);\n+\t\tstrbuf_git_path_submodule(&path, refs->name, \"%s\", dirname);\n \telse\n-\t\tpath = git_path(\"%s\", dirname);\n+\t\tstrbuf_git_path(&path, \"%s\", dirname);\n+\tpath_baselen = path.len;\n \n-\td = opendir(path);\n-\tif (!d)\n+\td = opendir(path.buf);\n+\tif (!d) {\n+\t\tstrbuf_release(&path);\n \t\treturn;\n+\t}\n \n \tstrbuf_init(&refname, dirnamelen + 257);\n \tstrbuf_add(&refname, dirname, dirnamelen);\n@@ -1373,17 +1377,14 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\tunsigned char sha1[20];\n \t\tstruct stat st;\n \t\tint flag;\n-\t\tconst char *refdir;\n \n \t\tif (de->d_name[0] == '.')\n \t\t\tcontinue;\n \t\tif (ends_with(de->d_name, \".lock\"))\n \t\t\tcontinue;\n \t\tstrbuf_addstr(&refname, de->d_name);\n-\t\trefdir = *refs->name\n-\t\t\t? git_path_submodule(refs->name, \"%s\", refname.buf)\n-\t\t\t: git_path(\"%s\", refname.buf);\n-\t\tif (stat(refdir, &st) < 0) {\n+\t\tstrbuf_addstr(&path, de->d_name);\n+\t\tif (stat(path.buf, &st) < 0) {\n \t\t\t; /* silently ignore */\n \t\t} else if (S_ISDIR(st.st_mode)) {\n \t\t\tstrbuf_addch(&refname, '/');\n@@ -1430,8 +1431,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)\n \t\t\t\t\t create_ref_entry(refname.buf, sha1, flag, 0));\n \t\t}\n \t\tstrbuf_setlen(&refname, dirnamelen);\n+\t\tstrbuf_setlen(&path, path_baselen);\n \t}\n \tstrbuf_release(&refname);\n+\tstrbuf_release(&path);\n \tclosedir(d);\n }\n \n-- \n2.5.0.414.g670f2a4\n"},{"id":"267715","messageId":"20150810093627.GJ30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 10/17] path.c: drop git_path_submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:36:27Z","receivedAt":"2015-08-10T09:36:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no callers of the slightly-dangerous static-buffer\ngit_path_submodule left. Let's drop it.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h |  5 ++---\n path.c  | 10 ----------\n 2 files changed, 2 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6f74f33..7a4fa90 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -713,12 +713,11 @@ extern int check_repository_format(void);\n  * the repository directory (git_path), or in a submodule's repository\n  * directory (git_path_submodule). In all cases, note that the result\n  * may be overwritten by another call to _any_ of the functions. Consider\n- * using the safer \"dup\" or \"strbuf\" formats below.\n+ * using the safer \"dup\" or \"strbuf\" formats below (in some cases, the\n+ * unsafe versions have already been removed).\n  */\n extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern const char *git_path_submodule(const char *path, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n \n extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\ndiff --git a/path.c b/path.c\nindex 9aad9a1..94d7ec2 100644\n--- a/path.c\n+++ b/path.c\n@@ -245,16 +245,6 @@ static void do_submodule_path(struct strbuf *buf, const char *path,\n \tstrbuf_cleanup_path(buf);\n }\n \n-const char *git_path_submodule(const char *path, const char *fmt, ...)\n-{\n-\tva_list args;\n-\tstruct strbuf *buf = get_pathname();\n-\tva_start(args, fmt);\n-\tdo_submodule_path(buf, path, fmt, args);\n-\tva_end(args);\n-\treturn buf->buf;\n-}\n-\n char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n {\n \tva_list args;\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267717","messageId":"20150810093638.GK30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:36:39Z","receivedAt":"2015-08-10T09:36:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 1a83c24 (git_snpath(): retire and replace with\nstrbuf_git_path(), 2014-11-30) taught log_ref_setup and\nlog_ref_write_1 to take a strbuf parameter, rather than a\nbare string. It then makes an alias to the strbuf's \"buf\"\nfield under the original name.\n\nThis made the original diff much shorter, but the resulting\ncode is more complicated that it needs to be. Since we've\naliased the pointer, we drop our reference to the strbuf to\nensure we don't accidentally change it. But if we simply\ndrop our alias and use \"logfile.buf\" directly, we do not\nhave to worry about this aliasing. It's a larger diff, but\nthe resulting code is simpler.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 38 +++++++++++++++-----------------------\n 1 file changed, 15 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 06f95c4..3666132 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)\n  * should_autocreate_reflog returns non-zero.  Otherwise, create it\n  * regardless of the ref name.  Fill in *err and return -1 on failure.\n  */\n-static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)\n+static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n-\tchar *logfile;\n \n-\tstrbuf_git_path(sb_logfile, \"logs/%s\", refname);\n-\tlogfile = sb_logfile->buf;\n-\t/* make sure the rest of the function can't change \"logfile\" */\n-\tsb_logfile = NULL;\n+\tstrbuf_git_path(logfile, \"logs/%s\", refname);\n \tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (safe_create_leading_directories(logfile) < 0) {\n+\t\tif (safe_create_leading_directories(logfile->buf) < 0) {\n \t\t\tstrbuf_addf(err, \"unable to create directory for %s: \"\n-\t\t\t\t    \"%s\", logfile, strerror(errno));\n+\t\t\t\t    \"%s\", logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t\toflags |= O_CREAT;\n \t}\n \n-\tlogfd = open(logfile, oflags, 0666);\n+\tlogfd = open(logfile->buf, oflags, 0666);\n \tif (logfd < 0) {\n \t\tif (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))\n \t\t\treturn 0;\n \n \t\tif (errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(logfile)) {\n+\t\t\tif (remove_empty_directories(logfile->buf)) {\n \t\t\t\tstrbuf_addf(err, \"There are still logs under \"\n-\t\t\t\t\t    \"'%s'\", logfile);\n+\t\t\t\t\t    \"'%s'\", logfile->buf);\n \t\t\t\treturn -1;\n \t\t\t}\n-\t\t\tlogfd = open(logfile, oflags, 0666);\n+\t\t\tlogfd = open(logfile->buf, oflags, 0666);\n \t\t}\n \n \t\tif (logfd < 0) {\n \t\t\tstrbuf_addf(err, \"unable to append to %s: %s\",\n-\t\t\t\t    logfile, strerror(errno));\n+\t\t\t\t    logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t}\n \n-\tadjust_shared_perm(logfile);\n+\tadjust_shared_perm(logfile->buf);\n \tclose(logfd);\n \treturn 0;\n }\n@@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n \n static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \t\t\t   const unsigned char *new_sha1, const char *msg,\n-\t\t\t   struct strbuf *sb_log_file, int flags,\n+\t\t\t   struct strbuf *log_file, int flags,\n \t\t\t   struct strbuf *err)\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n-\tchar *log_file;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG);\n+\tresult = log_ref_setup(refname, log_file, err, flags & REF_FORCE_CREATE_REFLOG);\n \n \tif (result)\n \t\treturn result;\n-\tlog_file = sb_log_file->buf;\n-\t/* make sure the rest of the function can't change \"log_file\" */\n-\tsb_log_file = NULL;\n \n-\tlogfd = open(log_file, oflags);\n+\tlogfd = open(log_file->buf, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n \t\t\t\t  git_committer_info(0), msg);\n \tif (result) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file,\n+\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file->buf,\n \t\t\t    strerror(errno));\n \t\tclose(logfd);\n \t\treturn -1;\n \t}\n \tif (close(logfd)) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file,\n+\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file->buf,\n \t\t\t    strerror(errno));\n \t\treturn -1;\n \t}\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267718","messageId":"20150810093653.GL30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 12/17] refs.c: avoid repeated git_path calls in rename_tmp_log","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:36:53Z","receivedAt":"2015-08-10T09:36:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because it's not safe to store the static-buffer results of\ngit_path for a long time, we end up formatting the same\nfilename over and over. We can fix this by using a\nfunction-local strbuf to store the formatted pathname and\navoid repeating ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 3666132..61a318f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2930,9 +2930,13 @@ out:\n static int rename_tmp_log(const char *newrefname)\n {\n \tint attempts_remaining = 4;\n+\tstruct strbuf path = STRBUF_INIT;\n+\tint ret = -1;\n \n  retry:\n-\tswitch (safe_create_leading_directories_const(git_path(\"logs/%s\", newrefname))) {\n+\tstrbuf_reset(&path);\n+\tstrbuf_git_path(&path, \"logs/%s\", newrefname);\n+\tswitch (safe_create_leading_directories_const(path.buf)) {\n \tcase SCLD_OK:\n \t\tbreak; /* success */\n \tcase SCLD_VANISHED:\n@@ -2941,19 +2945,19 @@ static int rename_tmp_log(const char *newrefname)\n \t\t/* fall through */\n \tdefault:\n \t\terror(\"unable to create directory for %s\", newrefname);\n-\t\treturn -1;\n+\t\tgoto out;\n \t}\n \n-\tif (rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newrefname))) {\n+\tif (rename(git_path(TMP_RENAMED_LOG), path.buf)) {\n \t\tif ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {\n \t\t\t/*\n \t\t\t * rename(a, b) when b is an existing\n \t\t\t * directory ought to result in ISDIR, but\n \t\t\t * Solaris 5.8 gives ENOTDIR.  Sheesh.\n \t\t\t */\n-\t\t\tif (remove_empty_directories(git_path(\"logs/%s\", newrefname))) {\n+\t\t\tif (remove_empty_directories(path.buf)) {\n \t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n-\t\t\t\treturn -1;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t\tgoto retry;\n \t\t} else if (errno == ENOENT && --attempts_remaining > 0) {\n@@ -2966,10 +2970,13 @@ static int rename_tmp_log(const char *newrefname)\n \t\t} else {\n \t\t\terror(\"unable to move logfile \"TMP_RENAMED_LOG\" to logs/%s: %s\",\n \t\t\t\tnewrefname, strerror(errno));\n-\t\t\treturn -1;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\tret = 0;\n+out:\n+\tstrbuf_release(&path);\n+\treturn ret;\n }\n \n static int rename_ref_available(const char *oldname, const char *newname)\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267719","messageId":"20150810093711.GM30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 13/17] refs.c: avoid git_path assignment in lock_ref_sha1_basic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:37:12Z","receivedAt":"2015-08-10T09:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Assigning the result of git_path is a bad pattern, because\nit's not immediately obvious how long you expect the content\nto stay valid (and it may be overwritten by subsequent\ncalls). Let's use a function-local strbuf here instead,\nwhich we know is safe (we just have to remember to free it\nin all code paths).\n\nAs a bonus, we get rid of a confusing variable-reuse\n(\"ref_file\" is used for two distinct purposes).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 32 +++++++++++++++++++-------------\n 1 file changed, 19 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 61a318f..8566677 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2408,7 +2408,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t    unsigned int flags, int *type_p,\n \t\t\t\t\t    struct strbuf *err)\n {\n-\tconst char *ref_file;\n+\tstruct strbuf ref_file = STRBUF_INIT;\n+\tstruct strbuf orig_ref_file = STRBUF_INIT;\n \tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n@@ -2432,20 +2433,19 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \trefname = resolve_ref_unsafe(refname, resolve_flags,\n \t\t\t\t     lock->old_oid.hash, &type);\n \tif (!refname && errno == EISDIR) {\n-\t\t/* we are trying to lock foo but we used to\n+\t\t/*\n+\t\t * we are trying to lock foo but we used to\n \t\t * have foo/bar which now does not exist;\n \t\t * it is normal for the empty directory 'foo'\n \t\t * to remain.\n \t\t */\n-\t\tref_file = git_path(\"%s\", orig_refname);\n-\t\tif (remove_empty_directories(ref_file)) {\n+\t\tstrbuf_git_path(&orig_ref_file, \"%s\", orig_refname);\n+\t\tif (remove_empty_directories(orig_ref_file.buf)) {\n \t\t\tlast_errno = errno;\n-\n \t\t\tif (!verify_refname_available(orig_refname, extras, skip,\n \t\t\t\t\t\t      get_loose_refs(&ref_cache), err))\n \t\t\t\tstrbuf_addf(err, \"there are still refs under '%s'\",\n \t\t\t\t\t    orig_refname);\n-\n \t\t\tgoto error_return;\n \t\t}\n \t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n@@ -2485,10 +2485,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t}\n \tlock->ref_name = xstrdup(refname);\n \tlock->orig_ref_name = xstrdup(orig_refname);\n-\tref_file = git_path(\"%s\", refname);\n+\tstrbuf_git_path(&ref_file, \"%s\", refname);\n \n  retry:\n-\tswitch (safe_create_leading_directories_const(ref_file)) {\n+\tswitch (safe_create_leading_directories_const(ref_file.buf)) {\n \tcase SCLD_OK:\n \t\tbreak; /* success */\n \tcase SCLD_VANISHED:\n@@ -2497,11 +2497,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t/* fall through */\n \tdefault:\n \t\tlast_errno = errno;\n-\t\tstrbuf_addf(err, \"unable to create directory for %s\", ref_file);\n+\t\tstrbuf_addf(err, \"unable to create directory for %s\",\n+\t\t\t    ref_file.buf);\n \t\tgoto error_return;\n \t}\n \n-\tif (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {\n+\tif (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {\n \t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n@@ -2511,7 +2512,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse {\n-\t\t\tunable_to_lock_message(ref_file, errno, err);\n+\t\t\tunable_to_lock_message(ref_file.buf, errno, err);\n \t\t\tgoto error_return;\n \t\t}\n \t}\n@@ -2519,12 +2520,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tlast_errno = errno;\n \t\tgoto error_return;\n \t}\n-\treturn lock;\n+\tgoto out;\n \n  error_return:\n \tunlock_ref(lock);\n+\tlock = NULL;\n+\n+ out:\n+\tstrbuf_release(&ref_file);\n+\tstrbuf_release(&orig_ref_file);\n \terrno = last_errno;\n-\treturn NULL;\n+\treturn lock;\n }\n \n /*\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267720","messageId":"20150810093727.GN30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 14/17] refs.c: remove_empty_directories can take a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:37:27Z","receivedAt":"2015-08-10T09:37:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The first thing we do in this function is copy the input\ninto a strbuf. Of the 4 callers, 3 of them already have a\nstrbuf we could use. Let's just take the strbuf, and convert\nthe remaining caller to use a strbuf, rather than a raw\ngit_path. This is safer, anyway, as remove_dir_recursively\nis a non-trivial function that might use the pathname\nbuffers itself (this is _probably_ OK, as the likely culprit\nwould be calling resolve_gitlink_ref, but we do not pass the\nproper flags to ask it to avoid blowing away gitlinks).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 34 +++++++++++++++-------------------\n 1 file changed, 15 insertions(+), 19 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8566677..ec1d06c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2290,25 +2290,14 @@ static int verify_lock(struct ref_lock *lock,\n \treturn 0;\n }\n \n-static int remove_empty_directories(const char *file)\n+static int remove_empty_directories(struct strbuf *path)\n {\n-\t/* we want to create a file but there is a directory there;\n+\t/*\n+\t * we want to create a file but there is a directory there;\n \t * if that is an empty directory (or a directory that contains\n \t * only empty directories), remove them.\n \t */\n-\tstruct strbuf path;\n-\tint result, save_errno;\n-\n-\tstrbuf_init(&path, 20);\n-\tstrbuf_addstr(&path, file);\n-\n-\tresult = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);\n-\tsave_errno = errno;\n-\n-\tstrbuf_release(&path);\n-\terrno = save_errno;\n-\n-\treturn result;\n+\treturn remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);\n }\n \n /*\n@@ -2440,7 +2429,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t * to remain.\n \t\t */\n \t\tstrbuf_git_path(&orig_ref_file, \"%s\", orig_refname);\n-\t\tif (remove_empty_directories(orig_ref_file.buf)) {\n+\t\tif (remove_empty_directories(&orig_ref_file)) {\n \t\t\tlast_errno = errno;\n \t\t\tif (!verify_refname_available(orig_refname, extras, skip,\n \t\t\t\t\t\t      get_loose_refs(&ref_cache), err))\n@@ -2961,7 +2950,7 @@ static int rename_tmp_log(const char *newrefname)\n \t\t\t * directory ought to result in ISDIR, but\n \t\t\t * Solaris 5.8 gives ENOTDIR.  Sheesh.\n \t\t\t */\n-\t\t\tif (remove_empty_directories(path.buf)) {\n+\t\t\tif (remove_empty_directories(&path)) {\n \t\t\t\terror(\"Directory not empty: logs/%s\", newrefname);\n \t\t\t\tgoto out;\n \t\t\t}\n@@ -3046,7 +3035,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms\n \tif (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&\n \t    delete_ref(newrefname, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n-\t\t\tif (remove_empty_directories(git_path(\"%s\", newrefname))) {\n+\t\t\tstruct strbuf path = STRBUF_INIT;\n+\t\t\tint result;\n+\n+\t\t\tstrbuf_git_path(&path, \"%s\", newrefname);\n+\t\t\tresult = remove_empty_directories(&path);\n+\t\t\tstrbuf_release(&path);\n+\n+\t\t\tif (result) {\n \t\t\t\terror(\"Directory not empty: %s\", newrefname);\n \t\t\t\tgoto rollback;\n \t\t\t}\n@@ -3183,7 +3179,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str\n \t\t\treturn 0;\n \n \t\tif (errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(logfile->buf)) {\n+\t\t\tif (remove_empty_directories(logfile)) {\n \t\t\t\tstrbuf_addf(err, \"There are still logs under \"\n \t\t\t\t\t    \"'%s'\", logfile->buf);\n \t\t\t\treturn -1;\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267721","messageId":"20150810093744.GO30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 15/17] find_hook: keep our own static buffer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:37:45Z","receivedAt":"2015-08-10T09:37:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The find_hook function returns the results of git_path,\nwhich is a static buffer shared by other path-related calls.\nReturning such a buffer is slightly dangerous, because it\ncan be overwritten by seemingly unrelated functions.\n\nLet's at least keep our _own_ static buffer, so you can\nonly get in trouble by calling find_hook in quick\nsuccession, which is less likely to happen and more obvious\nto notice.\n\nWhile we're at it, let's add some documentation of the\nfunction's limitations.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n run-command.c | 10 ++++++----\n run-command.h |  5 +++++\n 2 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 4d73e90..28e1d55 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -797,11 +797,13 @@ int finish_async(struct async *async)\n \n const char *find_hook(const char *name)\n {\n-\tconst char *path = git_path(\"hooks/%s\", name);\n-\tif (access(path, X_OK) < 0)\n-\t\tpath = NULL;\n+\tstatic struct strbuf path = STRBUF_INIT;\n \n-\treturn path;\n+\tstrbuf_reset(&path);\n+\tstrbuf_git_path(&path, \"hooks/%s\", name);\n+\tif (access(path.buf, X_OK) < 0)\n+\t\treturn NULL;\n+\treturn path.buf;\n }\n \n int run_hook_ve(const char *const *env, const char *name, va_list args)\ndiff --git a/run-command.h b/run-command.h\nindex 1103805..5b4425a 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -52,6 +52,11 @@ int start_command(struct child_process *);\n int finish_command(struct child_process *);\n int run_command(struct child_process *);\n \n+/*\n+ * Returns the path to the hook file, or NULL if the hook is missing\n+ * or disabled. Note that this points to static storage that will be\n+ * overwritten by further calls to find_hook and run_hook_*.\n+ */\n extern const char *find_hook(const char *name);\n LAST_ARG_MUST_BE_NULL\n extern int run_hook_le(const char *const *env, const char *name, ...);\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267722","messageId":"20150810093754.GP30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 16/17] get_repo_path: refactor path-allocation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:37:55Z","receivedAt":"2015-08-10T09:37:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The get_repo_path function calls mkpath() and then does some\nnon-trivial operations on it, like calling\nis_git_directory() and read_gitfile(). These are actually\nOK (they do not use more pathname static buffers\nthemselves), but it takes a fair bit of work to verify.\n\nLet's use our own strbuf to store the path, and we can\nsimply reuse it for each iteration of the loop (we can even\navoid rewriting the beginning part, since we are trying a\nseries of suffixes).\n\nTo make the strbuf cleanup easier, we split out a thin\nwrapper. As a bonus, this wrapper can factor out the\ncanonicalization that happens in all of the early-return\ncode paths.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/clone.c | 43 +++++++++++++++++++++++++++++--------------\n 1 file changed, 29 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 303a3a7..5864ad1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -99,51 +99,66 @@ static const char *argv_submodule[] = {\n \t\"submodule\", \"update\", \"--init\", \"--recursive\", NULL\n };\n \n-static char *get_repo_path(const char *repo, int *is_bundle)\n+static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)\n {\n \tstatic char *suffix[] = { \"/.git\", \"\", \".git/.git\", \".git\" };\n \tstatic char *bundle_suffix[] = { \".bundle\", \"\" };\n+\tsize_t baselen = path->len;\n \tstruct stat st;\n \tint i;\n \n \tfor (i = 0; i < ARRAY_SIZE(suffix); i++) {\n-\t\tconst char *path;\n-\t\tpath = mkpath(\"%s%s\", repo, suffix[i]);\n-\t\tif (stat(path, &st))\n+\t\tstrbuf_setlen(path, baselen);\n+\t\tstrbuf_addstr(path, suffix[i]);\n+\t\tif (stat(path->buf, &st))\n \t\t\tcontinue;\n-\t\tif (S_ISDIR(st.st_mode) && is_git_directory(path)) {\n+\t\tif (S_ISDIR(st.st_mode) && is_git_directory(path->buf)) {\n \t\t\t*is_bundle = 0;\n-\t\t\treturn xstrdup(absolute_path(path));\n+\t\t\treturn path->buf;\n \t\t} else if (S_ISREG(st.st_mode) && st.st_size > 8) {\n \t\t\t/* Is it a \"gitfile\"? */\n \t\t\tchar signature[8];\n-\t\t\tint len, fd = open(path, O_RDONLY);\n+\t\t\tconst char *dst;\n+\t\t\tint len, fd = open(path->buf, O_RDONLY);\n \t\t\tif (fd < 0)\n \t\t\t\tcontinue;\n \t\t\tlen = read_in_full(fd, signature, 8);\n \t\t\tclose(fd);\n \t\t\tif (len != 8 || strncmp(signature, \"gitdir: \", 8))\n \t\t\t\tcontinue;\n-\t\t\tpath = read_gitfile(path);\n-\t\t\tif (path) {\n+\t\t\tdst = read_gitfile(path->buf);\n+\t\t\tif (dst) {\n \t\t\t\t*is_bundle = 0;\n-\t\t\t\treturn xstrdup(absolute_path(path));\n+\t\t\t\treturn dst;\n \t\t\t}\n \t\t}\n \t}\n \n \tfor (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {\n-\t\tconst char *path;\n-\t\tpath = mkpath(\"%s%s\", repo, bundle_suffix[i]);\n-\t\tif (!stat(path, &st) && S_ISREG(st.st_mode)) {\n+\t\tstrbuf_setlen(path, baselen);\n+\t\tstrbuf_addstr(path, bundle_suffix[i]);\n+\t\tif (!stat(path->buf, &st) && S_ISREG(st.st_mode)) {\n \t\t\t*is_bundle = 1;\n-\t\t\treturn xstrdup(absolute_path(path));\n+\t\t\treturn path->buf;\n \t\t}\n \t}\n \n \treturn NULL;\n }\n \n+static char *get_repo_path(const char *repo, int *is_bundle)\n+{\n+\tstruct strbuf path = STRBUF_INIT;\n+\tconst char *raw;\n+\tchar *canon;\n+\n+\tstrbuf_addstr(&path, repo);\n+\traw = get_repo_path_1(&path, is_bundle);\n+\tcanon = raw ? xstrdup(absolute_path(raw)) : NULL;\n+\tstrbuf_release(&path);\n+\treturn canon;\n+}\n+\n static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)\n {\n \tconst char *end = repo + strlen(repo), *start;\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267723","messageId":"20150810093857.GQ30981@sigill.intra.peff.net","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"[PATCH 17/17] memoize common git-path \"constant\" files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T09:38:57Z","receivedAt":"2015-08-10T09:38:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"One of the most common uses of git_path() is to pass a\nconstant, like git_path(\"MERGE_MSG\"). This has two\ndrawbacks:\n\n  1. The return value is a static buffer, and the lifetime\n     is dependent on other calls to git_path, etc.\n\n  2. There's no compile-time checking of the pathname. This\n     is OK for a one-off (after all, we have to spell it\n     correctly at least once), but many of these constant\n     strings appear throughout the code.\n\nThis patch introduces a series of functions to \"memoize\"\nthese strings, which are essentially globals for the\nlifetime of the program. We compute the value once, take\nownership of the buffer, and return the cached value for\nsubsequent calls.  cache.h provides a helper macro for\ndefining these functions as one-liners, and defines a few\ncommon ones for global use.\n\nUsing a macro is a little bit gross, but it does nicely\ndocument the purpose of the functions. If we need to touch\nthem all later (e.g., because we learned how to change the\ngit_dir variable at runtime, and need to invalidate all of\nthe stored values), it will be much easier to have the\ncomplete list.\n\nNote that the shared-global functions have separate, manual\ndeclarations. We could do something clever with the macros\n(e.g., expand it to a declaration in some places, and a\ndeclaration _and_ a definition in path.c). But there aren't\nthat many, and it's probably better to stay away from\ntoo-magical macros.\n\nLikewise, if we abandon the C preprocessor in favor of\ngenerating these with a script, we could get much fancier.\nE.g., normalizing \"FOO/BAR-BAZ\" into \"git_path_foo_bar_baz\".\nBut the small amount of saved typing is probably not worth\nthe resulting confusion to readers who want to grep for the\nfunction's definition.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis one is probably the most contentious, as it has very far-reaching\neffects. But it really reduces the number of sites that need audited by\nquite a lot.\n\n attr.c                                 |  4 +-\n bisect.c                               |  7 ++-\n branch.c                               | 14 +++---\n builtin/blame.c                        |  7 ++-\n builtin/commit.c                       | 32 ++++++-------\n builtin/fetch.c                        |  4 +-\n builtin/merge.c                        | 30 ++++++------\n builtin/reset.c                        |  2 +-\n cache.h                                | 26 ++++++++++\n contrib/examples/builtin-fetch--tool.c |  4 +-\n dir.c                                  |  4 +-\n fetch-pack.c                           |  2 +-\n path.c                                 | 10 ++++\n rerere.c                               | 19 ++++----\n sequencer.c                            | 87 ++++++++++++++++------------------\n shallow.c                              | 10 ++--\n wt-status.c                            |  8 ++--\n 17 files changed, 151 insertions(+), 119 deletions(-)\n\ndiff --git a/attr.c b/attr.c\nindex 8f2ac6c..086c08d 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -490,6 +490,8 @@ static int git_attr_system(void)\n \treturn !git_env_bool(\"GIT_ATTR_NOSYSTEM\", 0);\n }\n \n+static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)\n+\n static void bootstrap_attr_stack(void)\n {\n \tstruct attr_stack *elem;\n@@ -531,7 +533,7 @@ static void bootstrap_attr_stack(void)\n \t\tdebug_push(elem);\n \t}\n \n-\telem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);\n+\telem = read_attr_from_file(git_path_info_attributes(), 1);\n \tif (!elem)\n \t\telem = xcalloc(1, sizeof(*elem));\n \telem->origin = NULL;\ndiff --git a/bisect.c b/bisect.c\nindex 03d5cd9..7a6498c 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -420,10 +420,13 @@ static int read_bisect_refs(void)\n \treturn for_each_ref_in(\"refs/bisect/\", register_ref, NULL);\n }\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+\n static void read_bisect_paths(struct argv_array *array)\n {\n \tstruct strbuf str = STRBUF_INIT;\n-\tconst char *filename = git_path(\"BISECT_NAMES\");\n+\tconst char *filename = git_path_bisect_names();\n \tFILE *fp = fopen(filename, \"r\");\n \n \tif (!fp)\n@@ -644,7 +647,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \n static int is_expected_rev(const struct object_id *oid)\n {\n-\tconst char *filename = git_path(\"BISECT_EXPECTED_REV\");\n+\tconst char *filename = git_path_bisect_expected_rev();\n \tstruct stat st;\n \tstruct strbuf str = STRBUF_INIT;\n \tFILE *fp;\ndiff --git a/branch.c b/branch.c\nindex b002435..e283683 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -302,11 +302,11 @@ void create_branch(const char *head,\n \n void remove_branch_state(void)\n {\n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"REVERT_HEAD\"));\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_RR\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n-\tunlink(git_path(\"SQUASH_MSG\"));\n+\tunlink(git_path_cherry_pick_head());\n+\tunlink(git_path_revert_head());\n+\tunlink(git_path_merge_head());\n+\tunlink(git_path_merge_rr());\n+\tunlink(git_path_merge_msg());\n+\tunlink(git_path_merge_mode());\n+\tunlink(git_path_squash_msg());\n }\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 04e4864..4db01c1 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2227,20 +2227,19 @@ static struct commit_list **append_parent(struct commit_list **tail, const unsig\n static void append_merge_parents(struct commit_list **tail)\n {\n \tint merge_head;\n-\tconst char *merge_head_file = git_path(\"MERGE_HEAD\");\n \tstruct strbuf line = STRBUF_INIT;\n \n-\tmerge_head = open(merge_head_file, O_RDONLY);\n+\tmerge_head = open(git_path_merge_head(), O_RDONLY);\n \tif (merge_head < 0) {\n \t\tif (errno == ENOENT)\n \t\t\treturn;\n-\t\tdie(\"cannot open '%s' for reading\", merge_head_file);\n+\t\tdie(\"cannot open '%s' for reading\", git_path_merge_head());\n \t}\n \n \twhile (!strbuf_getwholeline_fd(&line, merge_head, '\\n')) {\n \t\tunsigned char sha1[20];\n \t\tif (line.len < 40 || get_sha1_hex(line.buf, sha1))\n-\t\t\tdie(\"unknown line in '%s': %s\", merge_head_file, line.buf);\n+\t\t\tdie(\"unknown line in '%s': %s\", git_path_merge_head(), line.buf);\n \t\ttail = append_parent(tail, sha1);\n \t}\n \tclose(merge_head);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7314f33..4cbd5ff 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -166,9 +166,9 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)\n \n static void determine_whence(struct wt_status *s)\n {\n-\tif (file_exists(git_path(\"MERGE_HEAD\")))\n+\tif (file_exists(git_path_merge_head()))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n+\telse if (file_exists(git_path_cherry_pick_head())) {\n \t\twhence = FROM_CHERRY_PICK;\n \t\tif (file_exists(git_path(SEQ_DIR)))\n \t\t\tsequencer_in_use = 1;\n@@ -725,12 +725,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tformat_commit_message(commit, \"fixup! %s\\n\\n\",\n \t\t\t\t      &sb, &ctx);\n \t\thook_arg1 = \"message\";\n-\t} else if (!stat(git_path(\"MERGE_MSG\"), &statbuf)) {\n-\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MSG\"), 0) < 0)\n+\t} else if (!stat(git_path_merge_msg(), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)\n \t\t\tdie_errno(_(\"could not read MERGE_MSG\"));\n \t\thook_arg1 = \"merge\";\n-\t} else if (!stat(git_path(\"SQUASH_MSG\"), &statbuf)) {\n-\t\tif (strbuf_read_file(&sb, git_path(\"SQUASH_MSG\"), 0) < 0)\n+\t} else if (!stat(git_path_squash_msg(), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)\n \t\t\tdie_errno(_(\"could not read SQUASH_MSG\"));\n \t\thook_arg1 = \"squash\";\n \t} else if (template_file) {\n@@ -1684,10 +1684,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n \t\tpptr = &commit_list_insert(current_head, pptr)->next;\n-\t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n+\t\tfp = fopen(git_path_merge_head(), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n-\t\t\t\t  git_path(\"MERGE_HEAD\"));\n+\t\t\t\t  git_path_merge_head());\n \t\twhile (strbuf_getline(&m, fp, '\\n') != EOF) {\n \t\t\tstruct commit *parent;\n \n@@ -1698,8 +1698,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n-\t\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n-\t\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\tif (!stat(git_path_merge_mode(), &statbuf)) {\n+\t\t\tif (strbuf_read_file(&sb, git_path_merge_mode(), 0) < 0)\n \t\t\t\tdie_errno(_(\"could not read MERGE_MODE\"));\n \t\t\tif (!strcmp(sb.buf, \"no-ff\"))\n \t\t\t\tallow_fast_forward = 0;\n@@ -1775,12 +1775,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t}\n \tref_transaction_free(transaction);\n \n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"REVERT_HEAD\"));\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n-\tunlink(git_path(\"SQUASH_MSG\"));\n+\tunlink(git_path_cherry_pick_head());\n+\tunlink(git_path_revert_head());\n+\tunlink(git_path_merge_head());\n+\tunlink(git_path_merge_msg());\n+\tunlink(git_path_merge_mode());\n+\tunlink(git_path_squash_msg());\n \n \tif (commit_index_files())\n \t\tdie (_(\"Repository has been updated, but unable to write\\n\"\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 34b6f5e..df16d0a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -591,7 +591,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tconst char *what, *kind;\n \tstruct ref *rm;\n \tchar *url;\n-\tconst char *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n+\tconst char *filename = dry_run ? \"/dev/null\" : git_path_fetch_head();\n \tint want_status;\n \n \tfp = fopen(filename, \"a\");\n@@ -834,7 +834,7 @@ static void check_not_current_branch(struct ref *ref_map)\n \n static int truncate_fetch_head(void)\n {\n-\tconst char *filename = git_path(\"FETCH_HEAD\");\n+\tconst char *filename = git_path_fetch_head();\n \tFILE *fp = fopen(filename, \"w\");\n \n \tif (!fp)\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 85c54dc..a0edaca 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -231,9 +231,9 @@ static struct option builtin_merge_options[] = {\n /* Cleans up metadata that is uninteresting after a succeeded merge. */\n static void drop_save(void)\n {\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n+\tunlink(git_path_merge_head());\n+\tunlink(git_path_merge_msg());\n+\tunlink(git_path_merge_mode());\n }\n \n static int save_state(unsigned char *stash)\n@@ -338,7 +338,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \tstruct pretty_print_context ctx = {0};\n \n \tprintf(_(\"Squash commit -- not updating HEAD\\n\"));\n-\tfilename = git_path(\"SQUASH_MSG\");\n+\tfilename = git_path_squash_msg();\n \tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n@@ -754,7 +754,7 @@ static void add_strategies(const char *string, unsigned attr)\n \n static void write_merge_msg(struct strbuf *msg)\n {\n-\tconst char *filename = git_path(\"MERGE_MSG\");\n+\tconst char *filename = git_path_merge_msg();\n \tint fd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"),\n@@ -766,7 +766,7 @@ static void write_merge_msg(struct strbuf *msg)\n \n static void read_merge_msg(struct strbuf *msg)\n {\n-\tconst char *filename = git_path(\"MERGE_MSG\");\n+\tconst char *filename = git_path_merge_msg();\n \tstrbuf_reset(msg);\n \tif (strbuf_read_file(msg, filename, 0) < 0)\n \t\tdie_errno(_(\"Could not read from '%s'\"), filename);\n@@ -799,10 +799,10 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \t\tstrbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);\n \twrite_merge_msg(&msg);\n \tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n-\t\t\t    git_path(\"MERGE_MSG\"), \"merge\", NULL))\n+\t\t\t    git_path_merge_msg(), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n \tif (0 < option_edit) {\n-\t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n+\t\tif (launch_editor(git_path_merge_msg(), NULL, NULL))\n \t\t\tabort_commit(remoteheads, NULL);\n \t}\n \tread_merge_msg(&msg);\n@@ -865,7 +865,7 @@ static int suggest_conflicts(void)\n \tFILE *fp;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \n-\tfilename = git_path(\"MERGE_MSG\");\n+\tfilename = git_path_merge_msg();\n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -967,7 +967,7 @@ static void write_merge_state(struct commit_list *remoteheads)\n \t\t}\n \t\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(sha1));\n \t}\n-\tfilename = git_path(\"MERGE_HEAD\");\n+\tfilename = git_path_merge_head();\n \tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -977,7 +977,7 @@ static void write_merge_state(struct commit_list *remoteheads)\n \tstrbuf_addch(&merge_msg, '\\n');\n \twrite_merge_msg(&merge_msg);\n \n-\tfilename = git_path(\"MERGE_MODE\");\n+\tfilename = git_path_merge_mode();\n \tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -1070,7 +1070,7 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge\n \tif (!merge_names)\n \t\tmerge_names = &fetch_head_file;\n \n-\tfilename = git_path(\"FETCH_HEAD\");\n+\tfilename = git_path_fetch_head();\n \tfd = open(filename, O_RDONLY);\n \tif (fd < 0)\n \t\tdie_errno(_(\"could not open '%s' for reading\"), filename);\n@@ -1204,7 +1204,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tint nargc = 2;\n \t\tconst char *nargv[] = {\"reset\", \"--merge\", NULL};\n \n-\t\tif (!file_exists(git_path(\"MERGE_HEAD\")))\n+\t\tif (!file_exists(git_path_merge_head()))\n \t\t\tdie(_(\"There is no merge to abort (MERGE_HEAD missing).\"));\n \n \t\t/* Invoke 'git reset --merge' */\n@@ -1215,7 +1215,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"merge\");\n \n-\tif (file_exists(git_path(\"MERGE_HEAD\"))) {\n+\tif (file_exists(git_path_merge_head())) {\n \t\t/*\n \t\t * There is no unmerged entry, don't advise 'git\n \t\t * add/rm <file>', just 'git commit'.\n@@ -1226,7 +1226,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"You have not concluded your merge (MERGE_HEAD exists).\"));\n \t}\n-\tif (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n+\tif (file_exists(git_path_cherry_pick_head())) {\n \t\tif (advice_resolve_conflict)\n \t\t\tdie(_(\"You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\\n\"\n \t\t\t    \"Please, commit your changes before you merge.\"));\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4c08ddc..c503e75 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -36,7 +36,7 @@ static const char *reset_type_names[] = {\n \n static inline int is_merge(void)\n {\n-\treturn !access(git_path(\"MERGE_HEAD\"), F_OK);\n+\treturn !access(git_path_merge_head(), F_OK);\n }\n \n static int reset_index(const unsigned char *sha1, int reset_type, int quiet)\ndiff --git a/cache.h b/cache.h\nindex 7a4fa90..402521e 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -736,6 +736,32 @@ extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)\n extern void report_linked_checkout_garbage(void);\n \n /*\n+ * You can define a static memoized git path like:\n+ *\n+ *    static GIT_PATH_FUNC(git_path_foo, \"FOO\");\n+ *\n+ * or use one of the global ones below.\n+ */\n+#define GIT_PATH_FUNC(func, filename) \\\n+\tconst char *func(void) \\\n+\t{ \\\n+\t\tstatic char *ret; \\\n+\t\tif (!ret) \\\n+\t\t\tret = git_pathdup(filename); \\\n+\t\treturn ret; \\\n+\t}\n+\n+const char *git_path_cherry_pick_head(void);\n+const char *git_path_revert_head(void);\n+const char *git_path_squash_msg(void);\n+const char *git_path_merge_msg(void);\n+const char *git_path_merge_rr(void);\n+const char *git_path_merge_mode(void);\n+const char *git_path_merge_head(void);\n+const char *git_path_fetch_head(void);\n+const char *git_path_shallow(void);\n+\n+/*\n  * Return the name of the file in the local object database that would\n  * be used to store a loose object with the specified sha1.  The\n  * return value is a pointer to a statically allocated buffer that is\ndiff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c\nindex ee19166..a3eb19d 100644\n--- a/contrib/examples/builtin-fetch--tool.c\n+++ b/contrib/examples/builtin-fetch--tool.c\n@@ -516,7 +516,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \n \t\tif (argc != 8)\n \t\t\treturn error(\"append-fetch-head takes 6 args\");\n-\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfilename = git_path_fetch_head();\n \t\tfp = fopen(filename, \"a\");\n \t\tif (!fp)\n \t\t\treturn error(\"cannot open %s: %s\", filename, strerror(errno));\n@@ -534,7 +534,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \n \t\tif (argc != 5)\n \t\t\treturn error(\"fetch-native-store takes 3 args\");\n-\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfilename = git_path_fetch_head();\n \t\tfp = fopen(filename, \"a\");\n \t\tif (!fp)\n \t\t\treturn error(\"cannot open %s: %s\", filename, strerror(errno));\ndiff --git a/dir.c b/dir.c\nindex 1d42811..1ac8d5d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2185,6 +2185,8 @@ int remove_dir_recursively(struct strbuf *path, int flag)\n \treturn remove_dir_recurse(path, flag, NULL);\n }\n \n+static GIT_PATH_FUNC(git_path_info_exclude, \"info/exclude\")\n+\n void setup_standard_excludes(struct dir_struct *dir)\n {\n \tconst char *path;\n@@ -2199,7 +2201,7 @@ void setup_standard_excludes(struct dir_struct *dir)\n \t\t\t\t\t dir->untracked ? &dir->ss_excludes_file : NULL);\n \n \t/* per repository user preference */\n-\tpath = git_path(\"info/exclude\");\n+\tpath = git_path_info_exclude();\n \tif (!access_or_warn(path, R_OK, 0))\n \t\tadd_excludes_from_file_1(dir, path,\n \t\t\t\t\t dir->untracked ? &dir->ss_info_exclude : NULL);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex a136772..820251a 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -948,7 +948,7 @@ static void update_shallow(struct fetch_pack_args *args,\n \n \tif (args->depth > 0 && alternate_shallow_file) {\n \t\tif (*alternate_shallow_file == '\\0') { /* --unshallow */\n-\t\t\tunlink_or_warn(git_path(\"shallow\"));\n+\t\t\tunlink_or_warn(git_path_shallow());\n \t\t\trollback_lock_file(&shallow_lock);\n \t\t} else\n \t\t\tcommit_lock_file(&shallow_lock);\ndiff --git a/path.c b/path.c\nindex 94d7ec2..95acbaf 100644\n--- a/path.c\n+++ b/path.c\n@@ -933,3 +933,13 @@ char *xdg_config_home(const char *filename)\n \t\treturn mkpathdup(\"%s/.config/git/%s\", home, filename);\n \treturn NULL;\n }\n+\n+GIT_PATH_FUNC(git_path_cherry_pick_head, \"CHERRY_PICK_HEAD\")\n+GIT_PATH_FUNC(git_path_revert_head, \"REVERT_HEAD\")\n+GIT_PATH_FUNC(git_path_squash_msg, \"SQUASH_MSG\")\n+GIT_PATH_FUNC(git_path_merge_msg, \"MERGE_MSG\")\n+GIT_PATH_FUNC(git_path_merge_rr, \"MERGE_RR\")\n+GIT_PATH_FUNC(git_path_merge_mode, \"MERGE_MODE\")\n+GIT_PATH_FUNC(git_path_merge_head, \"MERGE_HEAD\")\n+GIT_PATH_FUNC(git_path_fetch_head, \"FETCH_HEAD\")\n+GIT_PATH_FUNC(git_path_shallow, \"shallow\")\ndiff --git a/rerere.c b/rerere.c\nindex 94aea9a..6a517aa 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -20,8 +20,6 @@ static int rerere_enabled = -1;\n /* automatically update cleanly resolved paths to the index */\n static int rerere_autoupdate;\n \n-static char *merge_rr_path;\n-\n const char *rerere_path(const char *hex, const char *file)\n {\n \treturn git_path(\"rr-cache/%s/%s\", hex, file);\n@@ -37,7 +35,7 @@ static void read_rr(struct string_list *rr)\n {\n \tunsigned char sha1[20];\n \tchar buf[PATH_MAX];\n-\tFILE *in = fopen(merge_rr_path, \"r\");\n+\tFILE *in = fopen(git_path_merge_rr(), \"r\");\n \tif (!in)\n \t\treturn;\n \twhile (fread(buf, 40, 1, in) == 1) {\n@@ -577,21 +575,21 @@ static void git_rerere_config(void)\n \tgit_config(git_default_config, NULL);\n }\n \n+static GIT_PATH_FUNC(git_path_rr_cache, \"rr-cache\")\n+\n static int is_rerere_enabled(void)\n {\n-\tconst char *rr_cache;\n \tint rr_cache_exists;\n \n \tif (!rerere_enabled)\n \t\treturn 0;\n \n-\trr_cache = git_path(\"rr-cache\");\n-\trr_cache_exists = is_directory(rr_cache);\n+\trr_cache_exists = is_directory(git_path_rr_cache());\n \tif (rerere_enabled < 0)\n \t\treturn rr_cache_exists;\n \n-\tif (!rr_cache_exists && mkdir_in_gitdir(rr_cache))\n-\t\tdie(\"Could not create directory %s\", rr_cache);\n+\tif (!rr_cache_exists && mkdir_in_gitdir(git_path_rr_cache()))\n+\t\tdie(\"Could not create directory %s\", git_path_rr_cache());\n \treturn 1;\n }\n \n@@ -605,8 +603,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)\n \n \tif (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))\n \t\trerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);\n-\tmerge_rr_path = git_pathdup(\"MERGE_RR\");\n-\tfd = hold_lock_file_for_update(&write_lock, merge_rr_path,\n+\tfd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tread_rr(merge_rr);\n \treturn fd;\n@@ -741,5 +738,5 @@ void rerere_clear(struct string_list *merge_rr)\n \t\tif (!has_rerere_resolution(name))\n \t\t\tunlink_rr_item(name);\n \t}\n-\tunlink_or_warn(git_path(\"MERGE_RR\"));\n+\tunlink_or_warn(git_path_merge_rr());\n }\ndiff --git a/sequencer.c b/sequencer.c\nindex c4f4b7d..147adfa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -21,6 +21,11 @@\n const char sign_off_header[] = \"Signed-off-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n+static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)\n+static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)\n+static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)\n+static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)\n+\n static int is_rfc2822_line(const char *buf, int len)\n {\n \tint i;\n@@ -186,7 +191,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n \t\t * (typically rebase --interactive) wants to take care\n \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n \t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\tunlink(git_path_cherry_pick_head());\n \t\treturn;\n \t}\n \n@@ -467,7 +472,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tstruct commit *base, *next, *parent;\n \tconst char *base_label, *next_label;\n \tstruct commit_message msg = { NULL, NULL, NULL, NULL };\n-\tchar *defmsg = NULL;\n \tstruct strbuf msgbuf = STRBUF_INIT;\n \tint res, unborn = 0, allow;\n \n@@ -537,8 +541,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t * reverse of it if we are revert.\n \t */\n \n-\tdefmsg = git_pathdup(\"MERGE_MSG\");\n-\n \tif (opts->action == REPLAY_REVERT) {\n \t\tbase = commit;\n \t\tbase_label = msg.label;\n@@ -585,12 +587,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || opts->action == REPLAY_REVERT) {\n \t\tres = do_recursive_merge(base, next, base_label, next_label,\n \t\t\t\t\t head, &msgbuf, opts);\n-\t\twrite_message(&msgbuf, defmsg);\n+\t\twrite_message(&msgbuf, git_path_merge_msg());\n \t} else {\n \t\tstruct commit_list *common = NULL;\n \t\tstruct commit_list *remotes = NULL;\n \n-\t\twrite_message(&msgbuf, defmsg);\n+\t\twrite_message(&msgbuf, git_path_merge_msg());\n \n \t\tcommit_list_insert(base, &common);\n \t\tcommit_list_insert(next, &remotes);\n@@ -628,11 +630,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\tgoto leave;\n \t}\n \tif (!opts->no_commit)\n-\t\tres = run_git_commit(defmsg, opts, allow);\n+\t\tres = run_git_commit(git_path_merge_msg(), opts, allow);\n \n leave:\n \tfree_message(commit, &msg);\n-\tfree(defmsg);\n \n \treturn res;\n }\n@@ -756,24 +757,23 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n static void read_populate_todo(struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd, res;\n \n-\tfd = open(todo_file, O_RDONLY);\n+\tfd = open(git_path_todo_file(), O_RDONLY);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s\"), todo_file);\n+\t\tdie_errno(_(\"Could not open %s\"), git_path_todo_file());\n \tif (strbuf_read(&buf, fd, 0) < 0) {\n \t\tclose(fd);\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), todo_file);\n+\t\tdie(_(\"Could not read %s.\"), git_path_todo_file());\n \t}\n \tclose(fd);\n \n \tres = parse_insn_buffer(buf.buf, todo_list, opts);\n \tstrbuf_release(&buf);\n \tif (res)\n-\t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n+\t\tdie(_(\"Unusable instruction sheet: %s\"), git_path_todo_file());\n }\n \n static int populate_opts_cb(const char *key, const char *value, void *data)\n@@ -813,12 +813,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \n static void read_populate_opts(struct replay_opts **opts_ptr)\n {\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n-\n-\tif (!file_exists(opts_file))\n+\tif (!file_exists(git_path_opts_file()))\n \t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n+\tif (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)\n+\t\tdie(_(\"Malformed options sheet: %s\"), git_path_opts_file());\n }\n \n static void walk_revs_populate_todo(struct commit_list **todo_list,\n@@ -836,31 +834,29 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,\n \n static int create_seq_dir(void)\n {\n-\tconst char *seq_dir = git_path(SEQ_DIR);\n-\n-\tif (file_exists(seq_dir)) {\n+\tif (file_exists(git_path_seq_dir())) {\n \t\terror(_(\"a cherry-pick or revert is already in progress\"));\n \t\tadvise(_(\"try \\\"git cherry-pick (--continue | --quit | --abort)\\\"\"));\n \t\treturn -1;\n \t}\n-\telse if (mkdir(seq_dir, 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory %s\"), seq_dir);\n+\telse if (mkdir(git_path_seq_dir(), 0777) < 0)\n+\t\tdie_errno(_(\"Could not create sequencer directory %s\"),\n+\t\t\t  git_path_seq_dir());\n \treturn 0;\n }\n \n static void save_head(const char *head)\n {\n-\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s\"), head_file);\n+\t\tdie_errno(_(\"Could not write to %s\"), git_path_head_file());\n \tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), head_file);\n+\t\tdie(_(\"Error wrapping up %s.\"), git_path_head_file());\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n@@ -877,8 +873,8 @@ static int rollback_single_pick(void)\n {\n \tunsigned char head_sha1[20];\n \n-\tif (!file_exists(git_path(\"CHERRY_PICK_HEAD\")) &&\n-\t    !file_exists(git_path(\"REVERT_HEAD\")))\n+\tif (!file_exists(git_path_cherry_pick_head()) &&\n+\t    !file_exists(git_path_revert_head()))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n \tif (read_ref_full(\"HEAD\", 0, head_sha1, NULL))\n \t\treturn error(_(\"cannot resolve HEAD\"));\n@@ -889,13 +885,11 @@ static int rollback_single_pick(void)\n \n static int sequencer_rollback(struct replay_opts *opts)\n {\n-\tconst char *filename;\n \tFILE *f;\n \tunsigned char sha1[20];\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tfilename = git_path(SEQ_HEAD_FILE);\n-\tf = fopen(filename, \"r\");\n+\tf = fopen(git_path_head_file(), \"r\");\n \tif (!f && errno == ENOENT) {\n \t\t/*\n \t\t * There is no multiple-cherry-pick in progress.\n@@ -905,18 +899,18 @@ static int sequencer_rollback(struct replay_opts *opts)\n \t\treturn rollback_single_pick();\n \t}\n \tif (!f)\n-\t\treturn error(_(\"cannot open %s: %s\"), filename,\n+\t\treturn error(_(\"cannot open %s: %s\"), git_path_head_file(),\n \t\t\t\t\t\tstrerror(errno));\n \tif (strbuf_getline(&buf, f, '\\n')) {\n-\t\terror(_(\"cannot read %s: %s\"), filename, ferror(f) ?\n-\t\t\tstrerror(errno) : _(\"unexpected end of file\"));\n+\t\terror(_(\"cannot read %s: %s\"), git_path_head_file(),\n+\t\t      ferror(f) ?  strerror(errno) : _(\"unexpected end of file\"));\n \t\tfclose(f);\n \t\tgoto fail;\n \t}\n \tfclose(f);\n \tif (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\\0') {\n \t\terror(_(\"stored pre-cherry-pick HEAD file '%s' is corrupt\"),\n-\t\t\tfilename);\n+\t\t\tgit_path_head_file());\n \t\tgoto fail;\n \t}\n \tif (reset_for_rollback(sha1))\n@@ -931,28 +925,27 @@ fail:\n \n static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n {\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);\n \tif (format_todo(&buf, todo_list, opts) < 0)\n-\t\tdie(_(\"Could not format %s.\"), todo_file);\n+\t\tdie(_(\"Could not format %s.\"), git_path_todo_file());\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie_errno(_(\"Could not write to %s\"), todo_file);\n+\t\tdie_errno(_(\"Could not write to %s\"), git_path_todo_file());\n \t}\n \tif (commit_lock_file(&todo_lock) < 0) {\n \t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n+\t\tdie(_(\"Error wrapping up %s.\"), git_path_todo_file());\n \t}\n \tstrbuf_release(&buf);\n }\n \n static void save_opts(struct replay_opts *opts)\n {\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\tconst char *opts_file = git_path_opts_file();\n \n \tif (opts->no_commit)\n \t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n@@ -1013,8 +1006,8 @@ static int continue_single_pick(void)\n {\n \tconst char *argv[] = { \"commit\", NULL };\n \n-\tif (!file_exists(git_path(\"CHERRY_PICK_HEAD\")) &&\n-\t    !file_exists(git_path(\"REVERT_HEAD\")))\n+\tif (!file_exists(git_path_cherry_pick_head()) &&\n+\t    !file_exists(git_path_revert_head()))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n \treturn run_command_v_opt(argv, RUN_GIT_CMD);\n }\n@@ -1023,14 +1016,14 @@ static int sequencer_continue(struct replay_opts *opts)\n {\n \tstruct commit_list *todo_list = NULL;\n \n-\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\tif (!file_exists(git_path_todo_file()))\n \t\treturn continue_single_pick();\n \tread_populate_opts(&opts);\n \tread_populate_todo(&todo_list, opts);\n \n \t/* Verify that the conflict has been resolved */\n-\tif (file_exists(git_path(\"CHERRY_PICK_HEAD\")) ||\n-\t    file_exists(git_path(\"REVERT_HEAD\"))) {\n+\tif (file_exists(git_path_cherry_pick_head()) ||\n+\t    file_exists(git_path_revert_head())) {\n \t\tint ret = continue_single_pick();\n \t\tif (ret)\n \t\t\treturn ret;\ndiff --git a/shallow.c b/shallow.c\nindex 257d811..4ded4a8 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -48,7 +48,7 @@ int is_repository_shallow(void)\n \t\treturn is_shallow;\n \n \tif (!path)\n-\t\tpath = git_path(\"shallow\");\n+\t\tpath = git_path_shallow();\n \t/*\n \t * fetch-pack sets '--shallow-file \"\"' as an indicator that no\n \t * shallow file should be used. We could just open it and it\n@@ -142,7 +142,7 @@ static void check_shallow_file_for_update(void)\n \tif (is_shallow == -1)\n \t\tdie(\"BUG: shallow must be initialized by now\");\n \n-\tif (!stat_validity_check(&shallow_stat, git_path(\"shallow\")))\n+\tif (!stat_validity_check(&shallow_stat, git_path_shallow()))\n \t\tdie(\"shallow file has changed since we read it\");\n }\n \n@@ -261,7 +261,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(shallow_lock, git_path(\"shallow\"),\n+\tfd = hold_lock_file_for_update(shallow_lock, git_path_shallow(),\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits(&sb, 0, extra)) {\n@@ -308,7 +308,7 @@ void prune_shallow(int show_only)\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n-\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n+\tfd = hold_lock_file_for_update(&shallow_lock, git_path_shallow(),\n \t\t\t\t       LOCK_DIE_ON_ERROR);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n@@ -317,7 +317,7 @@ void prune_shallow(int show_only)\n \t\t\t\t  shallow_lock.filename.buf);\n \t\tcommit_lock_file(&shallow_lock);\n \t} else {\n-\t\tunlink(git_path(\"shallow\"));\n+\t\tunlink(git_path_shallow());\n \t\trollback_lock_file(&shallow_lock);\n \t}\n \tstrbuf_release(&sb);\ndiff --git a/wt-status.c b/wt-status.c\nindex 04e01e4..717fd48 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1171,7 +1171,7 @@ static void show_rebase_in_progress(struct wt_status *s,\n \t\t\tstatus_printf_ln(s, color,\n \t\t\t\t_(\"  (use \\\"git rebase --abort\\\" to check out the original branch)\"));\n \t\t}\n-\t} else if (state->rebase_in_progress || !stat(git_path(\"MERGE_MSG\"), &st)) {\n+\t} else if (state->rebase_in_progress || !stat(git_path_merge_msg(), &st)) {\n \t\tprint_rebase_state(s, state, color);\n \t\tif (s->hints)\n \t\t\tstatus_printf_ln(s, color,\n@@ -1368,7 +1368,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \tstruct stat st;\n \tunsigned char sha1[20];\n \n-\tif (!stat(git_path(\"MERGE_HEAD\"), &st)) {\n+\tif (!stat(git_path_merge_head(), &st)) {\n \t\tstate->merge_in_progress = 1;\n \t} else if (!stat(git_path(\"rebase-apply\"), &st)) {\n \t\tif (!stat(git_path(\"rebase-apply/applying\"), &st)) {\n@@ -1387,7 +1387,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \t\t\tstate->rebase_in_progress = 1;\n \t\tstate->branch = read_and_strip_branch(\"rebase-merge/head-name\");\n \t\tstate->onto = read_and_strip_branch(\"rebase-merge/onto\");\n-\t} else if (!stat(git_path(\"CHERRY_PICK_HEAD\"), &st) &&\n+\t} else if (!stat(git_path_cherry_pick_head(), &st) &&\n \t\t\t!get_sha1(\"CHERRY_PICK_HEAD\", sha1)) {\n \t\tstate->cherry_pick_in_progress = 1;\n \t\thashcpy(state->cherry_pick_head_sha1, sha1);\n@@ -1396,7 +1396,7 @@ void wt_status_get_state(struct wt_status_state *state,\n \t\tstate->bisect_in_progress = 1;\n \t\tstate->branch = read_and_strip_branch(\"BISECT_START\");\n \t}\n-\tif (!stat(git_path(\"REVERT_HEAD\"), &st) &&\n+\tif (!stat(git_path_revert_head(), &st) &&\n \t    !get_sha1(\"REVERT_HEAD\", sha1)) {\n \t\tstate->revert_in_progress = 1;\n \t\thashcpy(state->revert_head_sha1, sha1);\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267746","messageId":"55C87E1D.6080705@alum.mit.edu","threadId":"40042","inReplyTo":"20150810093638.GK30981@sigill.intra.peff.net","subject":"Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-10T10:34:05Z","receivedAt":"2015-08-10T10:34:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/10/2015 11:36 AM, Jeff King wrote:\n> Commit 1a83c24 (git_snpath(): retire and replace with\n> strbuf_git_path(), 2014-11-30) taught log_ref_setup and\n> log_ref_write_1 to take a strbuf parameter, rather than a\n> bare string. It then makes an alias to the strbuf's \"buf\"\n> field under the original name.\n> \n> This made the original diff much shorter, but the resulting\n> code is more complicated that it needs to be. Since we've\n> aliased the pointer, we drop our reference to the strbuf to\n> ensure we don't accidentally change it. But if we simply\n> drop our alias and use \"logfile.buf\" directly, we do not\n> have to worry about this aliasing. It's a larger diff, but\n> the resulting code is simpler.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  refs.c | 38 +++++++++++++++-----------------------\n>  1 file changed, 15 insertions(+), 23 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 06f95c4..3666132 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)\n>   * should_autocreate_reflog returns non-zero.  Otherwise, create it\n>   * regardless of the ref name.  Fill in *err and return -1 on failure.\n>   */\n> -static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)\n> +static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n>  {\n> [...]\n> @@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n>  \n>  static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n>  \t\t\t   const unsigned char *new_sha1, const char *msg,\n> -\t\t\t   struct strbuf *sb_log_file, int flags,\n> +\t\t\t   struct strbuf *log_file, int flags,\n>  \t\t\t   struct strbuf *err)\n>  {\n> [...]\n\nNice change.\n\nHow about taking this opportunity to name the parameters (\"logfile\" vs.\n\"log_file\") consistently?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"267748","messageId":"55C8937A.3080206@alum.mit.edu","threadId":"40042","inReplyTo":"20150810093857.GQ30981@sigill.intra.peff.net","subject":"Re: [PATCH 17/17] memoize common git-path \"constant\" files","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-10T12:05:14Z","receivedAt":"2015-08-10T12:05:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/10/2015 11:38 AM, Jeff King wrote:\n> [...]\n> This patch introduces a series of functions to \"memoize\"\n> these strings, which are essentially globals for the\n> lifetime of the program. We compute the value once, take\n> ownership of the buffer, and return the cached value for\n> subsequent calls.  cache.h provides a helper macro for\n> defining these functions as one-liners, and defines a few\n> common ones for global use.\n> [...]\n\nI was wondering whether this memoization could interact badly with\nupdate_common_dir(). For example, if any of the memoized functions were\ncalled before git_common_dir is initialized, then the pre-git_common_dir\nvalue would continue to be used even if git_common_dir is changed\nafterwards. But I believe it is taboo to call git_path() before\nsetup_git_env(), so I think this is not a problem.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"267749","messageId":"55C893CF.4060103@alum.mit.edu","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"Re: [PATCH 0/17] removing questionable uses of git_path","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-10T12:06:39Z","receivedAt":"2015-08-10T12:06:39Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/10/2015 11:27 AM, Jeff King wrote:\n> [...] The problem is that git_path uses a static buffer that gets overwritten\n> by subsequent calls. [...]\n> \n>   [01/17]: cache.h: clarify documentation for git_path, et al\n>   [02/17]: cache.h: complete set of git_path_submodule helpers\n>   [03/17]: t5700: modernize style\n>   [04/17]: add_to_alternates_file: don't add duplicate entries\n>   [05/17]: remove hold_lock_file_for_append\n>   [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases\n>   [07/17]: prefer mkpathdup to mkpath in assignments\n>   [08/17]: remote.c: drop extraneous local variable from migrate_file\n>   [09/17]: refs.c: remove extra git_path calls from read_loose refs\n>   [10/17]: path.c: drop git_path_submodule\n>   [11/17]: refs.c: simplify strbufs in reflog setup and writing\n>   [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log\n>   [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic\n>   [14/17]: refs.c: remove_empty_directories can take a strbuf\n>   [15/17]: find_hook: keep our own static buffer\n>   [16/17]: get_repo_path: refactor path-allocation\n>   [17/17]: memoize common git-path \"constant\" files\n\nI read through all of the patches (except for 03) and didn't see any\nproblems. Thanks, Peff, for defusing some grenades :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"267750","messageId":"20150810122637.GA32371@sigill.intra.peff.net","threadId":"40042","inReplyTo":"55C87E1D.6080705@alum.mit.edu","subject":"Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T12:26:38Z","receivedAt":"2015-08-10T12:26:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 12:34:05PM +0200, Michael Haggerty wrote:\n\n> How about taking this opportunity to name the parameters (\"logfile\" vs.\n> \"log_file\") consistently?\n\nThanks, that's a good suggestion. Here's a re-roll of this patch:\n\n-- >8 --\nSubject: [PATCH] refs.c: simplify strbufs in reflog setup and writing\n\nCommit 1a83c24 (git_snpath(): retire and replace with\nstrbuf_git_path(), 2014-11-30) taught log_ref_setup and\nlog_ref_write_1 to take a strbuf parameter, rather than a\nbare string. It then makes an alias to the strbuf's \"buf\"\nfield under the original name.\n\nThis made the original diff much shorter, but the resulting\ncode is more complicated that it needs to be. Since we've\naliased the pointer, we drop our reference to the strbuf to\nensure we don't accidentally change it. But if we simply\ndrop our alias and use \"logfile.buf\" directly, we do not\nhave to worry about this aliasing. It's a larger diff, but\nthe resulting code is simpler.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 38 +++++++++++++++-----------------------\n 1 file changed, 15 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 06f95c4..105f271 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)\n  * should_autocreate_reflog returns non-zero.  Otherwise, create it\n  * regardless of the ref name.  Fill in *err and return -1 on failure.\n  */\n-static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)\n+static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)\n {\n \tint logfd, oflags = O_APPEND | O_WRONLY;\n-\tchar *logfile;\n \n-\tstrbuf_git_path(sb_logfile, \"logs/%s\", refname);\n-\tlogfile = sb_logfile->buf;\n-\t/* make sure the rest of the function can't change \"logfile\" */\n-\tsb_logfile = NULL;\n+\tstrbuf_git_path(logfile, \"logs/%s\", refname);\n \tif (force_create || should_autocreate_reflog(refname)) {\n-\t\tif (safe_create_leading_directories(logfile) < 0) {\n+\t\tif (safe_create_leading_directories(logfile->buf) < 0) {\n \t\t\tstrbuf_addf(err, \"unable to create directory for %s: \"\n-\t\t\t\t    \"%s\", logfile, strerror(errno));\n+\t\t\t\t    \"%s\", logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t\toflags |= O_CREAT;\n \t}\n \n-\tlogfd = open(logfile, oflags, 0666);\n+\tlogfd = open(logfile->buf, oflags, 0666);\n \tif (logfd < 0) {\n \t\tif (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))\n \t\t\treturn 0;\n \n \t\tif (errno == EISDIR) {\n-\t\t\tif (remove_empty_directories(logfile)) {\n+\t\t\tif (remove_empty_directories(logfile->buf)) {\n \t\t\t\tstrbuf_addf(err, \"There are still logs under \"\n-\t\t\t\t\t    \"'%s'\", logfile);\n+\t\t\t\t\t    \"'%s'\", logfile->buf);\n \t\t\t\treturn -1;\n \t\t\t}\n-\t\t\tlogfd = open(logfile, oflags, 0666);\n+\t\t\tlogfd = open(logfile->buf, oflags, 0666);\n \t\t}\n \n \t\tif (logfd < 0) {\n \t\t\tstrbuf_addf(err, \"unable to append to %s: %s\",\n-\t\t\t\t    logfile, strerror(errno));\n+\t\t\t\t    logfile->buf, strerror(errno));\n \t\t\treturn -1;\n \t\t}\n \t}\n \n-\tadjust_shared_perm(logfile);\n+\tadjust_shared_perm(logfile->buf);\n \tclose(logfd);\n \treturn 0;\n }\n@@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n \n static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,\n \t\t\t   const unsigned char *new_sha1, const char *msg,\n-\t\t\t   struct strbuf *sb_log_file, int flags,\n+\t\t\t   struct strbuf *logfile, int flags,\n \t\t\t   struct strbuf *err)\n {\n \tint logfd, result, oflags = O_APPEND | O_WRONLY;\n-\tchar *log_file;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n \n-\tresult = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG);\n+\tresult = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);\n \n \tif (result)\n \t\treturn result;\n-\tlog_file = sb_log_file->buf;\n-\t/* make sure the rest of the function can't change \"log_file\" */\n-\tsb_log_file = NULL;\n \n-\tlogfd = open(log_file, oflags);\n+\tlogfd = open(logfile->buf, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n \tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n \t\t\t\t  git_committer_info(0), msg);\n \tif (result) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file,\n+\t\tstrbuf_addf(err, \"unable to append to %s: %s\", logfile->buf,\n \t\t\t    strerror(errno));\n \t\tclose(logfd);\n \t\treturn -1;\n \t}\n \tif (close(logfd)) {\n-\t\tstrbuf_addf(err, \"unable to append to %s: %s\", log_file,\n+\t\tstrbuf_addf(err, \"unable to append to %s: %s\", logfile->buf,\n \t\t\t    strerror(errno));\n \t\treturn -1;\n \t}\n-- \n2.5.0.414.g670f2a4\n"},{"id":"267752","messageId":"20150810123034.GB32371@sigill.intra.peff.net","threadId":"40042","inReplyTo":"55C8937A.3080206@alum.mit.edu","subject":"Re: [PATCH 17/17] memoize common git-path \"constant\" files","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T12:30:34Z","receivedAt":"2015-08-10T12:30:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 02:05:14PM +0200, Michael Haggerty wrote:\n\n> I was wondering whether this memoization could interact badly with\n> update_common_dir(). For example, if any of the memoized functions were\n> called before git_common_dir is initialized, then the pre-git_common_dir\n> value would continue to be used even if git_common_dir is changed\n> afterwards. But I believe it is taboo to call git_path() before\n> setup_git_env(), so I think this is not a problem.\n\nCalling git_path() actually ends up in setup_git_env(), because we\nlazily call it from get_git_dir(). So if somebody tries to tweak\ncommon_dir stuff too late, we could in theory cache a bogus value; but\nthe real problem is generating the bogosity in the first place.\n\n-Peff\n"},{"id":"267778","messageId":"xmqqr3nbuk23.fsf@gitster.dls.corp.google.com","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"Re: [PATCH 0/17] removing questionable uses of git_path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T17:31:32Z","receivedAt":"2015-08-10T17:31:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The problem is that git_path uses a static buffer that gets overwritten\n> by subsequent calls.\n\nAs the rotating static buffer pattern used in get_pathname() was\nmodeled after sha1_to_hex(), we have the same issue there.  They are\ntroubles waiting to happen unless the callers are careful.\n\n> producing a fairly tame-looking set of function calls. It's OK to pass\n> the result of git_path() to a system call, or something that is a thin\n> wrapper around one (e.g., strbuf_read_file).\n\nThat is a short and good rule to follow, but the problem is that not\neverybody has good taste to interpret the above rule and apply it with\nan eye toward maintainability X-<.\n\n> Along the way, there are a few cleanups (e.g., I polished off the recent\n> hold_lock_file_for_append topic which was on the list, as it had some\n> problematic calls).\n>\n>   [01/17]: cache.h: clarify documentation for git_path, et al\n>   [02/17]: cache.h: complete set of git_path_submodule helpers\n>   [03/17]: t5700: modernize style\n>   [04/17]: add_to_alternates_file: don't add duplicate entries\n>   [05/17]: remove hold_lock_file_for_append\n>   [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases\n>   [07/17]: prefer mkpathdup to mkpath in assignments\n>   [08/17]: remote.c: drop extraneous local variable from migrate_file\n>   [09/17]: refs.c: remove extra git_path calls from read_loose refs\n>   [10/17]: path.c: drop git_path_submodule\n>   [11/17]: refs.c: simplify strbufs in reflog setup and writing\n>   [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log\n>   [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic\n>   [14/17]: refs.c: remove_empty_directories can take a strbuf\n>   [15/17]: find_hook: keep our own static buffer\n>   [16/17]: get_repo_path: refactor path-allocation\n>   [17/17]: memoize common git-path \"constant\" files\n\nNice.  Thanks.\n"},{"id":"267781","messageId":"20150810174743.GB20546@sigill.intra.peff.net","threadId":"40042","inReplyTo":"xmqqr3nbuk23.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/17] removing questionable uses of git_path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-10T17:47:43Z","receivedAt":"2015-08-10T17:47:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 10:31:32AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The problem is that git_path uses a static buffer that gets overwritten\n> > by subsequent calls.\n> \n> As the rotating static buffer pattern used in get_pathname() was\n> modeled after sha1_to_hex(), we have the same issue there.  They are\n> troubles waiting to happen unless the callers are careful.\n\nYeah, I think Michael mentioned that to me off-list. I don't _think_\nsha1_to_hex is nearly such a problem, because we tend to store sha1s in\ntheir binary form. So sha1_to_hex is almost always an argument to\nfprintf() or similar.\n\nOf course there are some exceptions. :) I notice we pass one return\nvalue to add_pending_object, which was almost certainly horribly broken\nbefore 31faeb2 started strdup'ing object_array names.\n\nSo certainly it would be nice to audit them all, but there are over 600\ncalls. Given the likelihood of finding a useful bug, I'm not sure it's\nthe greatest use of developer time.\n\n> > producing a fairly tame-looking set of function calls. It's OK to pass\n> > the result of git_path() to a system call, or something that is a thin\n> > wrapper around one (e.g., strbuf_read_file).\n> \n> That is a short and good rule to follow, but the problem is that not\n> everybody has good taste to interpret the above rule and apply it with\n> an eye toward maintainability X-<.\n\nYeah. Part of me wants to eradicate git_path entirely. My series takes\nout over half of the calls, but there are still close to 100, I think.\nI think it would make the code worse to convert all of them naively. We\ncould provide format-aware wrappers for some filesystem functions, like:\n\n  git_stat(&st, \"%s\", refname);\n\nor something. That feels horribly coupled compared to:\n\n  stat(git_path(\"%s\", refname), &st);\n\nbut it makes it very clear what the memory ownership/lifetime rules are.\n\nAnyway, part of my goal with the series was not just to clean up the\nexisting suspicious spots, but to raise awareness of the issue. At least\nfor me, I know it's something I will look for more carefully when\nreviewing patches. Once bitten, twice shy. :)\n\n-Peff\n"},{"id":"267798","messageId":"xmqqk2t2u5y9.fsf@gitster.dls.corp.google.com","threadId":"40042","inReplyTo":"20150810093514.GE30981@sigill.intra.peff.net","subject":"Re: [PATCH 05/17] remove hold_lock_file_for_append","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T22:36:14Z","receivedAt":"2015-08-10T22:36:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> No users of hold_lock_file_for_append remain, so remove it.\n\nThis does not seem to have anything to do with rotating static buffers\nused in get_pathname(); the only effect it has is to conflict heavily\nwith Michael's tempfile topic X-<.\n\nPerhaps this should be part of Michael's tempfile topic?\n"},{"id":"267802","messageId":"xmqqfv3qu5a7.fsf@gitster.dls.corp.google.com","threadId":"40042","inReplyTo":"20150810093627.GJ30981@sigill.intra.peff.net","subject":"Re: [PATCH 10/17] path.c: drop git_path_submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T22:50:40Z","receivedAt":"2015-08-10T22:50:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are no callers of the slightly-dangerous static-buffer\n> git_path_submodule left. Let's drop it.\n\nThere are a few callers added on 'pu', though.\n"},{"id":"267803","messageId":"xmqqbneeu4ys.fsf@gitster.dls.corp.google.com","threadId":"40042","inReplyTo":"xmqqfv3qu5a7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/17] path.c: drop git_path_submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T22:57:31Z","receivedAt":"2015-08-10T22:57:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> There are no callers of the slightly-dangerous static-buffer\n>> git_path_submodule left. Let's drop it.\n>\n> There are a few callers added on 'pu', though.\n\nActually there is only one.  Here is a proposed evil merge.\n\ndiff --git a/submodule.c b/submodule.c\nindex dfe8b7b..7ab08a1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)\n static int add_submodule_odb(const char *path)\n {\n \tstruct alternate_object_database *alt_odb;\n-\tconst char *objects_directory;\n+\tstruct strbuf objects_directory = STRBUF_INIT;\n \tint ret = 0;\n \n-\tobjects_directory = git_path_submodule(path, \"objects/\");\n-\tif (!is_directory(objects_directory)) {\n+\tstrbuf_git_path_submodule(&objects_directory, \"objects/\", \"%s\", path);\n+\tif (!is_directory(objects_directory.buf)) {\n \t\tret = -1;\n \t\tgoto done;\n \t}\n \n \t/* avoid adding it twice */\n \tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n-\t\tif (alt_odb->name - alt_odb->base == strlen(objects_directory) &&\n-\t\t\t\t!strcmp(alt_odb->base, objects_directory))\n+\t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n+\t\t\t\t!strcmp(alt_odb->base, objects_directory.buf))\n \t\t\tgoto done;\n \n-\talt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));\n+\talt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));\n \talt_odb->next = alt_odb_list;\n-\tstrcpy(alt_odb->base, objects_directory);\n-\talt_odb->name = alt_odb->base + strlen(objects_directory);\n+\tstrcpy(alt_odb->base, objects_directory.buf);\n+\talt_odb->name = alt_odb->base + objects_directory.len;\n \talt_odb->name[2] = '/';\n \talt_odb->name[40] = '\\0';\n \talt_odb->name[41] = '\\0';\n \talt_odb_list = alt_odb;\n \n \t/* add possible alternates from the submodule */\n-\tread_info_alternates(objects_directory, 0);\n+\tread_info_alternates(objects_directory.buf, 0);\n \tprepare_alt_odb();\n done:\n+\tstrbuf_release(&objects_directory);\n \treturn ret;\n }\n \n"},{"id":"267805","messageId":"xmqq7fp2u2fm.fsf@gitster.dls.corp.google.com","threadId":"40042","inReplyTo":"xmqqbneeu4ys.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/17] path.c: drop git_path_submodule","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-08-10T23:52:13Z","receivedAt":"2015-08-10T23:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Jeff King <peff@peff.net> writes:\n>>\n>>> There are no callers of the slightly-dangerous static-buffer\n>>> git_path_submodule left. Let's drop it.\n>>\n>> There are a few callers added on 'pu', though.\n>\n> Actually there is only one.  Here is a proposed evil merge.\n\nSorry, that didn't work X-<.\n\ndiff --git a/submodule.c b/submodule.c\nindex dfe8b7b..0cdaeb8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -120,10 +120,10 @@ void stage_updated_gitmodules(void)\n static int add_submodule_odb(const char *path)\n {\n \tstruct alternate_object_database *alt_odb;\n-\tconst char *objects_directory;\n+\tchar *objects_directory;\n \tint ret = 0;\n \n-\tobjects_directory = git_path_submodule(path, \"objects/\");\n+\tobjects_directory = git_pathdup_submodule(path, \"objects/\");\n \tif (!is_directory(objects_directory)) {\n \t\tret = -1;\n \t\tgoto done;\n@@ -148,6 +148,7 @@ static int add_submodule_odb(const char *path)\n \tread_info_alternates(objects_directory, 0);\n \tprepare_alt_odb();\n done:\n+\tfree(objects_directory);\n \treturn ret;\n }\n \n"},{"id":"267811","messageId":"55C97354.7080008@alum.mit.edu","threadId":"40042","inReplyTo":"20150810093446.GD30981@sigill.intra.peff.net","subject":"Re: [PATCH 04/17] add_to_alternates_file: don't add duplicate entries","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-08-11T04:00:20Z","receivedAt":"2015-08-11T04:00:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/10/2015 11:34 AM, Jeff King wrote:\n> The add_to_alternates_file function blindly uses\n> hold_lock_file_for_append to copy the existing contents, and\n> then adds the new line to it. This has two minor problems:\n> \n>   1. We might add duplicate entries, which are ugly and\n>      inefficient.\n> \n>   2. We do not check that the file ends with a newline, in\n>      which case we would bogusly append to the final line.\n>      This is quite unlikely in practice, though, as we call\n>      this function only from git-clone, so presumably we are\n>      the only writers of the file (and we always add a\n>      newline).\n> \n> Instead of using hold_lock_file_for_append, let's copy the\n> file line by line, which ensures all records are properly\n> terminated. If we see an extra line, we can simply abort the\n> update (there is no point in even copying the rest, as we\n> know that it would be identical to the original).\n\nDo we have reason to expect that a lot of people have alternates files\nthat already contain duplicate lines? You say that this function is only\ncalled from `git clone`, so I guess the answer is \"no\".\n\nBut if I'm wrong, it might be friendly to de-dup the existing lines\nwhile copying them.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"267817","messageId":"20150811093809.GA10238@sigill.intra.peff.net","threadId":"40042","inReplyTo":"xmqqk2t2u5y9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 05/17] remove hold_lock_file_for_append","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T09:38:09Z","receivedAt":"2015-08-11T09:38:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 03:36:14PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > No users of hold_lock_file_for_append remain, so remove it.\n> \n> This does not seem to have anything to do with rotating static buffers\n> used in get_pathname(); the only effect it has is to conflict heavily\n> with Michael's tempfile topic X-<.\n\nYeah, the first patch (to drop the final caller) is why I stuck it in\nthis series, and I did not want to forget the rest of the topic that Jim\nworked on.\n\n> Perhaps this should be part of Michael's tempfile topic?\n\nYes, I think that is OK. We can keep the first patch (to\nadd_to_alternates_file) here, and do the other one later on top of\nMichael's topic.\n\n-Peff\n"},{"id":"267818","messageId":"20150811095318.GB10238@sigill.intra.peff.net","threadId":"40042","inReplyTo":"xmqqbneeu4ys.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 10/17] path.c: drop git_path_submodule","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T09:53:18Z","receivedAt":"2015-08-11T09:53:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 10, 2015 at 03:57:31PM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Jeff King <peff@peff.net> writes:\n> >\n> >> There are no callers of the slightly-dangerous static-buffer\n> >> git_path_submodule left. Let's drop it.\n> >\n> > There are a few callers added on 'pu', though.\n> \n> Actually there is only one.  Here is a proposed evil merge.\n\nThanks for catching. I (obviously) only checked against 'next' and not\n'pu'. Rather than the evil merge, we could also just drop this patch.\nAnd then either leave it be, or fix this one up as a separate topic once\nmerged. Though it really looks like this call site is potentially\ndangerous, with the assignment (I think it is OK, though, because\nread_info_alternates does not use git_path itself).\n\n> diff --git a/submodule.c b/submodule.c\n> index dfe8b7b..7ab08a1 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)\n>  static int add_submodule_odb(const char *path)\n> [...]\n>  {\n>  \tstruct alternate_object_database *alt_odb;\n> -\tconst char *objects_directory;\n> +\tstruct strbuf objects_directory = STRBUF_INIT;\n>  \tint ret = 0;\n>  \n> -\tobjects_directory = git_path_submodule(path, \"objects/\");\n> -\tif (!is_directory(objects_directory)) {\n> +\tstrbuf_git_path_submodule(&objects_directory, \"objects/\", \"%s\", path);\n> +\tif (!is_directory(objects_directory.buf)) {\n>  \t\tret = -1;\n>  \t\tgoto done;\n>  \t}\n\nHmph, the change in 6659e74 would have been a lot nicer if\nstrbuf_git_path_submodule were already available. Most of what you are\ndoing here is undoing that commit's strbuf/buf transition. :-/\n\n>  \t/* avoid adding it twice */\n>  \tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n> -\t\tif (alt_odb->name - alt_odb->base == strlen(objects_directory) &&\n> -\t\t\t\t!strcmp(alt_odb->base, objects_directory))\n> +\t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n> +\t\t\t\t!strcmp(alt_odb->base, objects_directory.buf))\n>  \t\t\tgoto done;\n\nNot really relevant to what we're talking about here, but this is the\nfirst time I've lookd at add_submodule_odb. It really looks like the\nwhole second half of the function could be replaced with a call to\nlink_alt_odb_entry.\n\n-Peff\n"},{"id":"267819","messageId":"20150811095435.GC10238@sigill.intra.peff.net","threadId":"40042","inReplyTo":"55C97354.7080008@alum.mit.edu","subject":"Re: [PATCH 04/17] add_to_alternates_file: don't add duplicate entries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-08-11T09:54:35Z","receivedAt":"2015-08-11T09:54:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 11, 2015 at 06:00:20AM +0200, Michael Haggerty wrote:\n\n> > Instead of using hold_lock_file_for_append, let's copy the\n> > file line by line, which ensures all records are properly\n> > terminated. If we see an extra line, we can simply abort the\n> > update (there is no point in even copying the rest, as we\n> > know that it would be identical to the original).\n> \n> Do we have reason to expect that a lot of people have alternates files\n> that already contain duplicate lines? You say that this function is only\n> called from `git clone`, so I guess the answer is \"no\".\n> \n> But if I'm wrong, it might be friendly to de-dup the existing lines\n> while copying them.\n\nYou're right; this is not something we expect. There's not really any\nway to get an alternates file inside the running clone, except by\nputting it in your \"templates/\" directory.\n\nSo I think it is OK to not worry about cleaning up an existing mess.\n\n-Peff\n"},{"id":"268119","messageId":"CACsJy8DPX2Z8zsRjsWoXLMN4V5=HFN7mZSiWPcrUvuyTte3UKA@mail.gmail.com","threadId":"40042","inReplyTo":"20150810092731.GA9027@sigill.intra.peff.net","subject":"Re: [PATCH 0/17] removing questionable uses of git_path","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-08-15T09:05:27Z","receivedAt":"2015-08-15T09:05:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Aug 10, 2015 at 4:27 PM, Jeff King <peff@peff.net> wrote:\n> The problem is that git_path uses a static buffer that gets overwritten\n> by subsequent calls.\n\nresolve_ref() was in the same boat, then we renamed it to\nresolve_ref_unsafe(), I believe with an intention to eventually kill\nit. But it lives on anyway. I wanted to suggest renaming git_path() to\ngit_path_unsafe(). But I'm not sure if that will catch reviewer's eyes\nwhen new call sites are added. Probably not..\n-- \nDuy\n"}]}