threads / patch / 40042

patch, 17 partsremoving questionable uses of git_path

Subject: [PATCH 0/17] removing questionable uses of git_path

## tl;dr

34 messages between Aug 10, 2015 and Aug 15, 2015. Diffs are folded; open one to read it.

replies: 33people: 4as markdown or json

Jeff King· Aug 10, 2015, 09:27 UTC · lore

Recently Michael and I were working on a patch series (not yet published), which did something like:

  const char *path = git_path("foo");
  ... do stuff with path ...
  for_each_ref(some_callback, NULL);
  ... do some other stuff ...
  unlink(path);

Clever readers may have spotted the bug immediately, but we did not, until we found that random loose refs were being deleted from the repository.

The problem is that git_path uses a static buffer that gets overwritten by subsequent calls. The ref code uses it to iterate over all of the loose refs in a directory, so our original path is trashed before for_each_ref returns. Except to make it even more exciting, git_path actually has a ring of _four_ buffers, so any trivial test you write will probably work just fine; it's only when you use a real repository that it causes problems (and then, only if the code path is such that the loose refs were not previously accessed and cached!).

Michael likened git_path to "a hand-grenade with the pin pulled out", and I tend to agree. On the other hand, it's pretty darn useful to be able to get a quick path without having to deal with memory allocation and ownership. This patch series tries to document the danger, and remove some of the more questionable uses. I don't know whether this is fixing any actual latent bugs; I traced a number of the code paths manually, but never found a bug. There were some near misses, though, which make me believe that seemingly-unrelated refactoring could introduce a bug.

I stopped short of trying to eradicate git_path entirely, and settled for:

  git grep -E '[^_](git_|mk)path\('

producing a fairly tame-looking set of function calls. It's OK to pass the result of git_path() to a system call, or something that is a thin wrapper around one (e.g., strbuf_read_file).

I think this takes us most of the way there. I left out a few cases where introducing allocations would have been awkward, and I verified that there were no bugs (e.g., rerere_path). And I left out a few spots that conflict with topics in "next" (and luckily, in all cases what is in next makes the problem go away, so we do not have to follow-up for those sites).

Along the way, there are a few cleanups (e.g., I polished off the recent hold_lock_file_for_append topic which was on the list, as it had some problematic calls).

  [01/17]: cache.h: clarify documentation for git_path, et al
  [02/17]: cache.h: complete set of git_path_submodule helpers
  [03/17]: t5700: modernize style
  [04/17]: add_to_alternates_file: don't add duplicate entries
  [05/17]: remove hold_lock_file_for_append
  [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases
  [07/17]: prefer mkpathdup to mkpath in assignments
  [08/17]: remote.c: drop extraneous local variable from migrate_file
  [09/17]: refs.c: remove extra git_path calls from read_loose refs
  [10/17]: path.c: drop git_path_submodule
  [11/17]: refs.c: simplify strbufs in reflog setup and writing
  [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log
  [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic
  [14/17]: refs.c: remove_empty_directories can take a strbuf
  [15/17]: find_hook: keep our own static buffer
  [16/17]: get_repo_path: refactor path-allocation
  [17/17]: memoize common git-path "constant" files
-Peff
Jeff King· Aug 10, 2015, 09:32 UTC · re: Jeff King · lore

[PATCH 01/17] cache.h: clarify documentation for git_path, et al

The comment above these functions actually describes sha1_file_name, and comes from the very first revision of git. Commit 723c31f (Add "git_path()" and "head_ref()" helper functions., 2005-07-05) added git_path, pushing the comment away from the function it describes; later commits added more functions in this block.

Let's fix the comment to describe these related functions in more detail. Let's also make sure to point out their safer alternatives (and move those alternatives below, which makes more sense when reading the file).

Note that we do not need to move the existing comment to sha1_file_name. Commit d40d535 (sha1_file.c: document a bunch of functions defined in the file, 2014-02-21) already added a much more descriptive comment to it.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)
Show changes to cache.h +12 −5
diff --git a/cache.h b/cache.h
index 6bb7119..8db884e 100644
--- a/cache.h
+++ b/cache.h
@@ -708,6 +708,18 @@ extern int check_repository_format(void);
 #define DATA_CHANGED    0x0020
 #define TYPE_CHANGED    0x0040
 
+/*
+ * Return a statically allocated filename, either generically (mkpath), in
+ * the repository directory (git_path), or in a submodule's repository
+ * directory (git_path_submodule). In all cases, note that the result
+ * may be overwritten by another call to _any_ of the functions. Consider
+ * using the safer "dup" or "strbuf" formats below.
+ */
+extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+extern const char *git_path_submodule(const char *path, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
+
 extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
@@ -717,11 +729,6 @@ extern char *git_pathdup(const char *fmt, ...)
 extern char *mkpathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 
-/* Return a statically allocated filename matching the sha1 signature */
-extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern const char *git_path_submodule(const char *path, const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
 extern void report_linked_checkout_garbage(void);
 
 /*

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:32 UTC · re: Jeff King · lore

[PATCH 02/17] cache.h: complete set of git_path_submodule helpers

The git_path function has "git_pathdup" and "strbuf_git_path" variants, but git_submodule_path only comes in the dangerous, static-buffer variant. That makes refactoring callers to use the safer functions hard (since they don't exist).

Since we're already using a strbuf behind the scenes, it's easy to expose all three of these interfaces with thin wrappers.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  5 +++++
 path.c  | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)
Show changes to 2 files +35 −5

cache.h, path.c

diff --git a/cache.h b/cache.h
index 8db884e..6f74f33 100644
--- a/cache.h
+++ b/cache.h
@@ -724,10 +724,15 @@ extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern void strbuf_git_path(struct strbuf *sb, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
+extern void strbuf_git_path_submodule(struct strbuf *sb, const char *path,
+				      const char *fmt, ...)
+	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 extern char *mkpathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
 
 extern void report_linked_checkout_garbage(void);
 
diff --git a/path.c b/path.c
index 10f4cbf..9aad9a1 100644
--- a/path.c
+++ b/path.c
@@ -224,11 +224,10 @@ const char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname->buf);
 }
 
-const char *git_path_submodule(const char *path, const char *fmt, ...)
+static void do_submodule_path(struct strbuf *buf, const char *path,
+			      const char *fmt, va_list args)
 {
-	struct strbuf *buf = get_pathname();
 	const char *git_dir;
-	va_list args;
 
 	strbuf_addstr(buf, path);
 	if (buf->len && buf->buf[buf->len - 1] != '/')
@@ -242,13 +241,39 @@ const char *git_path_submodule(const char *path, const char *fmt, ...)
 	}
 	strbuf_addch(buf, '/');
 
-	va_start(args, fmt);
 	strbuf_vaddf(buf, fmt, args);
-	va_end(args);
 	strbuf_cleanup_path(buf);
+}
+
+const char *git_path_submodule(const char *path, const char *fmt, ...)
+{
+	va_list args;
+	struct strbuf *buf = get_pathname();
+	va_start(args, fmt);
+	do_submodule_path(buf, path, fmt, args);
+	va_end(args);
 	return buf->buf;
 }
 
+char *git_pathdup_submodule(const char *path, const char *fmt, ...)
+{
+	va_list args;
+	struct strbuf buf = STRBUF_INIT;
+	va_start(args, fmt);
+	do_submodule_path(&buf, path, fmt, args);
+	va_end(args);
+	return strbuf_detach(&buf, NULL);
+}
+
+void strbuf_git_path_submodule(struct strbuf *buf, const char *path,
+			       const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	do_submodule_path(buf, path, fmt, args);
+	va_end(args);
+}
+
 int validate_headref(const char *path)
 {
 	struct stat st;

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:32 UTC · re: Jeff King · lore

[PATCH 03/17] t5700: modernize style

The early part of this test is rather old, and does not follow our usual style guidelines. In particular:

  - the tests liberally chdir, and expect out-of-test "cd"
    commands to return them to a sane state
  - test commands aren't indented at all
  - there are a lot of minor formatting nits, like the
    opening quote of the test block on the wrong line,
    spaces after ">", etc

This patch fixes the style issues, and uses a few helper functions, along with subshells and "git -C", to avoid changing the cwd of the main script.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t5700-clone-reference.sh | 193 +++++++++++++++++++--------------------------
 1 file changed, 81 insertions(+), 112 deletions(-)
Show changes to t/t5700-clone-reference.sh +81 −112
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 3e783fc..51d131a 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -10,49 +10,51 @@ base_dir=`pwd`
 
 U=$base_dir/UPLOAD_LOG
 
-test_expect_success 'preparing first repository' \
-'test_create_repo A && cd A &&
-echo first > file1 &&
-git add file1 &&
-git commit -m initial'
-
-cd "$base_dir"
-
-test_expect_success 'preparing second repository' \
-'git clone A B && cd B &&
-echo second > file2 &&
-git add file2 &&
-git commit -m addition &&
-git repack -a -d &&
-git prune'
-
-cd "$base_dir"
-
-test_expect_success 'cloning with reference (-l -s)' \
-'git clone -l -s --reference B A C'
-
-cd "$base_dir"
-
-test_expect_success 'existence of info/alternates' \
-'test_line_count = 2 C/.git/objects/info/alternates'
-
-cd "$base_dir"
+# create a commit in repo $1 with name $2
+commit_in () {
+	(
+		cd "$1" &&
+		echo "$2" >"$2" &&
+		git add "$2" &&
+		git commit -m "$2"
+	)
+}
+
+# check that there are $2 loose objects in repo $1
+test_objcount () {
+	echo "$2" >expect &&
+	git -C "$1" count-objects >actual.raw &&
+	cut -d' ' -f1 <actual.raw >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'preparing first repository' '
+	test_create_repo A &&
+	commit_in A file1
+'
 
-test_expect_success 'pulling from reference' \
-'cd C &&
-git pull ../B master'
+test_expect_success 'preparing second repository' '
+	git clone A B &&
+	commit_in B file2 &&
+	git -C B repack -ad &&
+	git -C B prune
+'
 
-cd "$base_dir"
+test_expect_success 'cloning with reference (-l -s)' '
+	git clone -l -s --reference B A C
+'
 
-test_expect_success 'that reference gets used' \
-'cd C &&
-echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-test_cmp expected current'
+test_expect_success 'existence of info/alternates' '
+	test_line_count = 2 C/.git/objects/info/alternates
+'
 
-cd "$base_dir"
+test_expect_success 'pulling from reference' '
+	git -C C pull ../B master
+'
 
-rm -f "$U.D"
+test_expect_success 'that reference gets used' '
+	test_objcount C 0
+'
 
 test_expect_success 'cloning with reference (no -l -s)' '
 	GIT_TRACE_PACKET=$U.D git clone --reference B "file://$(pwd)/A" D
@@ -63,95 +65,64 @@ test_expect_success 'fetched no objects' '
 	! grep " want" "$U.D"
 '
 
-cd "$base_dir"
-
-test_expect_success 'existence of info/alternates' \
-'test_line_count = 1 D/.git/objects/info/alternates'
-
-cd "$base_dir"
-
-test_expect_success 'pulling from reference' \
-'cd D && git pull ../B master'
-
-cd "$base_dir"
-
-test_expect_success 'that reference gets used' \
-'cd D && echo "0 objects, 0 kilobytes" > expected &&
-git count-objects > current &&
-test_cmp expected current'
-
-cd "$base_dir"
+test_expect_success 'existence of info/alternates' '
+	test_line_count = 1 D/.git/objects/info/alternates
+'
 
-test_expect_success 'updating origin' \
-'cd A &&
-echo third > file3 &&
-git add file3 &&
-git commit -m update &&
-git repack -a -d &&
-git prune'
+test_expect_success 'pulling from reference' '
+	git -C D pull ../B master
+'
 
-cd "$base_dir"
+test_expect_success 'that reference gets used' '
+	test_objcount D 0
+'
 
-test_expect_success 'pulling changes from origin' \
-'cd C &&
-git pull origin'
+test_expect_success 'updating origin' '
+	commit_in A file3 &&
+	git -C A repack -ad &&
+	git -C A prune
+'
 
-cd "$base_dir"
+test_expect_success 'pulling changes from origin' '
+	git -C C pull origin
+'
 
 # the 2 local objects are commit and tree from the merge
-test_expect_success 'that alternate to origin gets used' \
-'cd C &&
-echo "2 objects" > expected &&
-git count-objects | cut -d, -f1 > current &&
-test_cmp expected current'
-
-cd "$base_dir"
-
-test_expect_success 'pulling changes from origin' \
-'cd D &&
-git pull origin'
+test_expect_success 'that alternate to origin gets used' '
+	test_objcount C 2
+'
 
-cd "$base_dir"
+test_expect_success 'pulling changes from origin' '
+	git -C D pull origin
+'
 
 # the 5 local objects are expected; file3 blob, commit in A to add it
 # and its tree, and 2 are our tree and the merge commit.
-test_expect_success 'check objects expected to exist locally' \
-'cd D &&
-echo "5 objects" > expected &&
-git count-objects | cut -d, -f1 > current &&
-test_cmp expected current'
-
-cd "$base_dir"
-
-test_expect_success 'preparing alternate repository #1' \
-'test_create_repo F && cd F &&
-echo first > file1 &&
-git add file1 &&
-git commit -m initial'
-
-cd "$base_dir"
-
-test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' \
-'git clone F G && cd F &&
-echo second > file2 &&
-git add file2 &&
-git commit -m addition'
-
-cd "$base_dir"
+test_expect_success 'check objects expected to exist locally' '
+	test_objcount D 5
+'
 
-test_expect_success 'cloning alternate repo #1, using #2 as reference' \
-'git clone --reference G F H'
+test_expect_success 'preparing alternate repository #1' '
+	test_create_repo F &&
+	commit_in F file1
+'
 
-cd "$base_dir"
+test_expect_success 'cloning alternate repo #2 and adding changes to repo #1' '
+	git clone F G &&
+	commit_in F file2
+'
 
-test_expect_success 'cloning with reference being subset of source (-l -s)' \
-'git clone -l -s --reference A B E'
+test_expect_success 'cloning alternate repo #1, using #2 as reference' '
+	git clone --reference G F H
+'
 
-cd "$base_dir"
+test_expect_success 'cloning with reference being subset of source (-l -s)' '
+	git clone -l -s --reference A B E
+'
 
 test_expect_success 'clone with reference from a tagged repository' '
 	(
-		cd A && git tag -a -m 'tagged' HEAD
+		cd A && git tag -a -m tagged HEAD
 	) &&
 	git clone --reference=A A I
 '
@@ -168,8 +139,6 @@ test_expect_success 'prepare branched repository' '
 	)
 '
 
-rm -f "$U.K"
-
 test_expect_success 'fetch with incomplete alternates' '
 	git init K &&
 	echo "$base_dir/A/.git/objects" >K/.git/objects/info/alternates &&

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:34 UTC · re: Jeff King · lore

[PATCH 04/17] add_to_alternates_file: don't add duplicate entries

The add_to_alternates_file function blindly uses hold_lock_file_for_append to copy the existing contents, and then adds the new line to it. This has two minor problems:

  1. We might add duplicate entries, which are ugly and
     inefficient.
  2. We do not check that the file ends with a newline, in
     which case we would bogusly append to the final line.
     This is quite unlikely in practice, though, as we call
     this function only from git-clone, so presumably we are
     the only writers of the file (and we always add a
     newline).

Instead of using hold_lock_file_for_append, let's copy the file line by line, which ensures all records are properly terminated. If we see an extra line, we can simply abort the update (there is no point in even copying the rest, as we know that it would be identical to the original).

As a bonus, we also get rid of some calls to the static-buffer mkpath and git_path functions.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a polishing of the thread at:
  http://thread.gmane.org/gmane.comp.version-control.git/270341
 sha1_file.c                | 47 +++++++++++++++++++++++++++++++++++++++-------
 t/t5700-clone-reference.sh |  5 +++++
 2 files changed, 45 insertions(+), 7 deletions(-)
Show changes to 2 files +45 −7

sha1_file.c, t/t5700-clone-reference.sh

diff --git a/sha1_file.c b/sha1_file.c
index 1cee438..3400b8b 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -404,13 +404,46 @@ void read_info_alternates(const char * relative_base, int depth)
 void add_to_alternates_file(const char *reference)
 {
 	struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));
-	int fd = hold_lock_file_for_append(lock, git_path("objects/info/alternates"), LOCK_DIE_ON_ERROR);
-	const char *alt = mkpath("%s\n", reference);
-	write_or_die(fd, alt, strlen(alt));
-	if (commit_lock_file(lock))
-		die("could not close alternates file");
-	if (alt_odb_tail)
-		link_alt_odb_entries(alt, strlen(alt), '\n', NULL, 0);
+	char *alts = git_pathdup("objects/info/alternates");
+	FILE *in, *out;
+
+	hold_lock_file_for_update(lock, alts, LOCK_DIE_ON_ERROR);
+	out = fdopen_lock_file(lock, "w");
+	if (!out)
+		die_errno("unable to fdopen alternates lockfile");
+
+	in = fopen(alts, "r");
+	if (in) {
+		struct strbuf line = STRBUF_INIT;
+		int found = 0;
+
+		while (strbuf_getline(&line, in, '\n') != EOF) {
+			if (!strcmp(reference, line.buf)) {
+				found = 1;
+				break;
+			}
+			fprintf_or_die(out, "%s\n", line.buf);
+		}
+
+		strbuf_release(&line);
+		fclose(in);
+
+		if (found) {
+			rollback_lock_file(lock);
+			lock = NULL;
+		}
+	}
+	else if (errno != ENOENT)
+		die_errno("unable to read alternates file");
+
+	if (lock) {
+		fprintf_or_die(out, "%s\n", reference);
+		if (commit_lock_file(lock))
+			die_errno("unable to move new alternates file into place");
+		if (alt_odb_tail)
+			link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
+	}
+	free(alts);
 }
 
 int foreach_alt_odb(alt_odb_fn fn, void *cb)
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 51d131a..ef1779f 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -120,6 +120,11 @@ test_expect_success 'cloning with reference being subset of source (-l -s)' '
 	git clone -l -s --reference A B E
 '
 
+test_expect_success 'cloning with multiple references drops duplicates' '
+	git clone -s --reference B --reference A --reference B A dups &&
+	test_line_count = 2 dups/.git/objects/info/alternates
+'
+
 test_expect_success 'clone with reference from a tagged repository' '
 	(
 		cd A && git tag -a -m tagged HEAD

-- 
2.5.0.414.g670f2a4
Michael Haggerty· Aug 11, 2015, 04:00 UTC · re: Jeff King · lore

Re: [PATCH 04/17] add_to_alternates_file: don't add duplicate entries

On 08/10/2015 11:34 AM, Jeff King wrote:
Show 19 quoted lines
> The add_to_alternates_file function blindly uses
> hold_lock_file_for_append to copy the existing contents, and
> then adds the new line to it. This has two minor problems:
> 
>   1. We might add duplicate entries, which are ugly and
>      inefficient.
> 
>   2. We do not check that the file ends with a newline, in
>      which case we would bogusly append to the final line.
>      This is quite unlikely in practice, though, as we call
>      this function only from git-clone, so presumably we are
>      the only writers of the file (and we always add a
>      newline).
> 
> Instead of using hold_lock_file_for_append, let's copy the
> file line by line, which ensures all records are properly
> terminated. If we see an extra line, we can simply abort the
> update (there is no point in even copying the rest, as we
> know that it would be identical to the original).

Do we have reason to expect that a lot of people have alternates files that already contain duplicate lines? You say that this function is only called from `git clone`, so I guess the answer is "no".

But if I'm wrong, it might be friendly to de-dup the existing lines while copying them.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Jeff King· Aug 11, 2015, 09:54 UTC · re: Michael Haggerty · lore

Re: [PATCH 04/17] add_to_alternates_file: don't add duplicate entries

On Tue, Aug 11, 2015 at 06:00:20AM +0200, Michael Haggerty wrote:
Show 12 quoted lines
> > Instead of using hold_lock_file_for_append, let's copy the
> > file line by line, which ensures all records are properly
> > terminated. If we see an extra line, we can simply abort the
> > update (there is no point in even copying the rest, as we
> > know that it would be identical to the original).
> 
> Do we have reason to expect that a lot of people have alternates files
> that already contain duplicate lines? You say that this function is only
> called from `git clone`, so I guess the answer is "no".
> 
> But if I'm wrong, it might be friendly to de-dup the existing lines
> while copying them.

You're right; this is not something we expect. There's not really any way to get an alternates file inside the running clone, except by putting it in your "templates/" directory.

So I think it is OK to not worry about cleaning up an existing mess.
-Peff
Jeff King· Aug 10, 2015, 09:35 UTC · re: Jeff King · lore

[PATCH 05/17] remove hold_lock_file_for_append

From: Jim Hill <gjthill@gmail.com>
No users of hold_lock_file_for_append remain, so remove it.

hold_lock_file_for_append copies its target file internally. This makes it too heavyweight for true append-only logging and too limited for anything else (which probably wants to process the contents). It shouldn't be used.

[jk: tweaked commit message, and dropped declaration and
     documentation, too]
Signed-off-by: Jim Hill <gjthill@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
And this is the obvious continuation of the last patch.
 Documentation/technical/api-lockfile.txt | 26 ++++++++--------------
 lockfile.c                               | 38 --------------------------------
 lockfile.h                               |  7 ++----
 3 files changed, 11 insertions(+), 60 deletions(-)
Show changes to 3 files +11 −60

Documentation/technical/api-lockfile.txt, lockfile.c, lockfile.h

diff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt
index 93b5f23..0f5c481 100644
--- a/Documentation/technical/api-lockfile.txt
+++ b/Documentation/technical/api-lockfile.txt
@@ -40,7 +40,7 @@ The caller:
 
 * Attempts to create a lockfile by passing that variable and the path
   of the final destination (e.g. `$GIT_DIR/index`) to
-  `hold_lock_file_for_update` or `hold_lock_file_for_append`.
+  `hold_lock_file_for_update`.
 
 * Writes new content for the destination file by either:
 
@@ -64,8 +64,7 @@ When finished writing, the caller can:
 
 Even after the lockfile is committed or rolled back, the `lock_file`
 object must not be freed or altered by the caller. However, it may be
-reused; just pass it to another call of `hold_lock_file_for_update` or
-`hold_lock_file_for_append`.
+reused; just pass it to another call of `hold_lock_file_for_update`.
 
 If the program exits before you have called one of `commit_lock_file`,
 `commit_lock_file_to`, `rollback_lock_file`, or `close_lock_file`, an
@@ -111,8 +110,7 @@ appropriately, do their best to roll back the lockfile, and return -1.
 Flags
 -----
 
-The following flags can be passed to `hold_lock_file_for_update` or
-`hold_lock_file_for_append`:
+The following flags can be passed to `hold_lock_file_for_update`:
 
 LOCK_NO_DEREF::
 
@@ -141,12 +139,6 @@ hold_lock_file_for_update::
 	above). Attempt to create a lockfile for the destination and
 	return the file descriptor for writing to the file.
 
-hold_lock_file_for_append::
-
-	Like `hold_lock_file_for_update`, but before returning copy
-	the existing contents of the file (if any) to the lockfile and
-	position its write pointer at the end of the file.
-
 fdopen_lock_file::
 
 	Associate a stdio stream with the lockfile. Return NULL
@@ -162,8 +154,8 @@ get_locked_file_path::
 commit_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized with an
-	earlier call to `hold_lock_file_for_update` or
-	`hold_lock_file_for_append`, close the file descriptor, and
+	earlier call to `hold_lock_file_for_update`,
+	close the file descriptor, and
 	rename the lockfile to its final destination. Return 0 upon
 	success. On failure, roll back the lock file and return -1,
 	with `errno` set to the value from the failing call to
@@ -180,8 +172,8 @@ commit_lock_file_to::
 rollback_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized with an
-	earlier call to `hold_lock_file_for_update` or
-	`hold_lock_file_for_append`, close the file descriptor and
+	earlier call to `hold_lock_file_for_update`,
+	close the file descriptor and
 	remove the lockfile. It is a NOOP to call
 	`rollback_lock_file()` for a `lock_file` object that has
 	already been committed or rolled back.
@@ -189,8 +181,8 @@ rollback_lock_file::
 close_lock_file::
 
 	Take a pointer to the `struct lock_file` initialized with an
-	earlier call to `hold_lock_file_for_update` or
-	`hold_lock_file_for_append`. Close the file descriptor (and
+	earlier call to `hold_lock_file_for_update`.
+	Close the file descriptor (and
 	the file pointer if it has been opened using
 	`fdopen_lock_file`). Return 0 upon success. On failure to
 	`close(2)`, return a negative value and roll back the lock
diff --git a/lockfile.c b/lockfile.c
index 993bb82..b1ceec6 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -249,44 +249,6 @@ int hold_lock_file_for_update_timeout(struct lock_file *lk, const char *path,
 	return fd;
 }
 
-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)
-{
-	int fd, orig_fd;
-
-	fd = lock_file(lk, path, flags);
-	if (fd < 0) {
-		if (flags & LOCK_DIE_ON_ERROR)
-			unable_to_lock_die(path, errno);
-		return fd;
-	}
-
-	orig_fd = open(path, O_RDONLY);
-	if (orig_fd < 0) {
-		if (errno != ENOENT) {
-			int save_errno = errno;
-
-			if (flags & LOCK_DIE_ON_ERROR)
-				die("cannot open '%s' for copying", path);
-			rollback_lock_file(lk);
-			error("cannot open '%s' for copying", path);
-			errno = save_errno;
-			return -1;
-		}
-	} else if (copy_fd(orig_fd, fd)) {
-		int save_errno = errno;
-
-		if (flags & LOCK_DIE_ON_ERROR)
-			die("failed to prepare '%s' for appending", path);
-		close(orig_fd);
-		rollback_lock_file(lk);
-		errno = save_errno;
-		return -1;
-	} else {
-		close(orig_fd);
-	}
-	return fd;
-}
-
 FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)
 {
 	if (!lk->active)
diff --git a/lockfile.h b/lockfile.h
index b4abc61..1373f5c 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -27,8 +27,8 @@
  *   soon as the object is used in any way, it is irrevocably
  *   registered in the lock_file_list, and on_list is set.
  *
- * - Locked, lockfile open (after hold_lock_file_for_update(),
- *   hold_lock_file_for_append(), or reopen_lock_file()). In this
+ * - Locked, lockfile open (after hold_lock_file_for_update()
+ *   or reopen_lock_file()). In this
  *   state:
  *   - the lockfile exists
  *   - active is set
@@ -85,9 +85,6 @@ static inline int hold_lock_file_for_update(
 	return hold_lock_file_for_update_timeout(lk, path, flags, 0);
 }
 
-extern int hold_lock_file_for_append(struct lock_file *lk, const char *path,
-				     int flags);
-
 extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);
 extern char *get_locked_file_path(struct lock_file *);
 extern int commit_lock_file_to(struct lock_file *, const char *path);

-- 
2.5.0.414.g670f2a4
Junio C Hamano· Aug 10, 2015, 22:36 UTC · re: Jeff King · lore

Re: [PATCH 05/17] remove hold_lock_file_for_append

Jeff King <peff@peff.net> writes:
> No users of hold_lock_file_for_append remain, so remove it.

This does not seem to have anything to do with rotating static buffers used in get_pathname(); the only effect it has is to conflict heavily with Michael's tempfile topic X-<.

Perhaps this should be part of Michael's tempfile topic?
Jeff King· Aug 11, 2015, 09:38 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/17] remove hold_lock_file_for_append

On Mon, Aug 10, 2015 at 03:36:14PM -0700, Junio C Hamano wrote:
Show 7 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > No users of hold_lock_file_for_append remain, so remove it.
> 
> This does not seem to have anything to do with rotating static buffers
> used in get_pathname(); the only effect it has is to conflict heavily
> with Michael's tempfile topic X-<.

Yeah, the first patch (to drop the final caller) is why I stuck it in this series, and I did not want to forget the rest of the topic that Jim worked on.

> Perhaps this should be part of Michael's tempfile topic?

Yes, I think that is OK. We can keep the first patch (to add_to_alternates_file) here, and do the other one later on top of Michael's topic.

-Peff
Jeff King· Aug 10, 2015, 09:35 UTC · re: Jeff King · lore

[PATCH 06/17] prefer git_pathdup to git_path in some possibly-dangerous cases

Because git_path uses a static buffer that is shared with calls to git_path, mkpath, etc, it can be dangerous to assign the result to a variable or pass it to a non-trivial function. The value may change unexpectedly due to other calls.

None of the cases changed here has a known bug, but they're worth converting away from git_path because:

  1. It's easy to use git_pathdup in these cases.
  2. They use constructs (like assignment) that make it
     hard to tell whether they're safe or not.

The extra malloc overhead should be trivial, as an allocation should be an order of magnitude cheaper than a system call (which we are clearly about to make, since we are constructing a filename). The real cost is that we must remember to free the result.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fsck.c |  4 +++-
 fast-import.c  |  4 +++-
 http-backend.c |  3 ++-
 notes-merge.c  |  3 ++-
 refs.c         | 14 ++++++++------
 unpack-trees.c |  4 +++-
 6 files changed, 21 insertions(+), 11 deletions(-)
Show changes to 6 files +21 −11

builtin/fsck.c, fast-import.c, http-backend.c, notes-merge.c, refs.c, unpack-trees.c

diff --git a/builtin/fsck.c b/builtin/fsck.c
index f4b87e9..0794703 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -243,13 +243,14 @@ static void check_unreachable_object(struct object *obj)
 			printf("dangling %s %s\n", typename(obj->type),
 			       sha1_to_hex(obj->sha1));
 		if (write_lost_and_found) {
-			const char *filename = git_path("lost-found/%s/%s",
+			char *filename = git_pathdup("lost-found/%s/%s",
 				obj->type == OBJ_COMMIT ? "commit" : "other",
 				sha1_to_hex(obj->sha1));
 			FILE *f;
 
 			if (safe_create_leading_directories_const(filename)) {
 				error("Could not create lost-found");
+				free(filename);
 				return;
 			}
 			if (!(f = fopen(filename, "w")))
@@ -262,6 +263,7 @@ static void check_unreachable_object(struct object *obj)
 			if (fclose(f))
 				die_errno("Could not finish '%s'",
 					  filename);
+			free(filename);
 		}
 		return;
 	}
diff --git a/fast-import.c b/fast-import.c
index 2ad4fee..ad8848b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -407,7 +407,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
 
 static void write_crash_report(const char *err)
 {
-	const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
+	char *loc = git_pathdup("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
 	FILE *rpt = fopen(loc, "w");
 	struct branch *b;
 	unsigned long lu;
@@ -415,6 +415,7 @@ static void write_crash_report(const char *err)
 
 	if (!rpt) {
 		error("can't write crash report %s: %s", loc, strerror(errno));
+		free(loc);
 		return;
 	}
 
@@ -488,6 +489,7 @@ static void write_crash_report(const char *err)
 	fputs("-------------------\n", rpt);
 	fputs("END OF CRASH REPORT\n", rpt);
 	fclose(rpt);
+	free(loc);
 }
 
 static void end_packfile(void);
diff --git a/http-backend.c b/http-backend.c
index b977c00..bac40ef 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -164,7 +164,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)
 
 static void send_local_file(const char *the_type, const char *name)
 {
-	const char *p = git_path("%s", name);
+	char *p = git_pathdup("%s", name);
 	size_t buf_alloc = 8192;
 	char *buf = xmalloc(buf_alloc);
 	int fd;
@@ -191,6 +191,7 @@ static void send_local_file(const char *the_type, const char *name)
 	}
 	close(fd);
 	free(buf);
+	free(p);
 }
 
 static void get_text_file(char *name)
diff --git a/notes-merge.c b/notes-merge.c
index 0b2b82c..b3d1dab 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -295,7 +295,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
 				  const char *buf, unsigned long size)
 {
 	int fd;
-	const char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+	char *path = git_pathdup(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
 	if (safe_create_leading_directories_const(path))
 		die_errno("unable to create directory for '%s'", path);
 	if (file_exists(path))
@@ -320,6 +320,7 @@ static void write_buf_to_worktree(const unsigned char *obj,
 	}
 
 	close(fd);
+	free(path);
 }
 
 static void write_note_to_worktree(const unsigned char *obj,
diff --git a/refs.c b/refs.c
index 2db2975..93b250e 100644
--- a/refs.c
+++ b/refs.c
@@ -1288,12 +1288,12 @@ static void read_packed_refs(FILE *f, struct ref_dir *dir)
  */
 static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 {
-	const char *packed_refs_file;
+	char *packed_refs_file;
 
 	if (*refs->name)
-		packed_refs_file = git_path_submodule(refs->name, "packed-refs");
+		packed_refs_file = git_pathdup_submodule(refs->name, "packed-refs");
 	else
-		packed_refs_file = git_path("packed-refs");
+		packed_refs_file = git_pathdup("packed-refs");
 
 	if (refs->packed &&
 	    !stat_validity_check(&refs->packed->validity, packed_refs_file))
@@ -1312,6 +1312,7 @@ static struct packed_ref_cache *get_packed_ref_cache(struct ref_cache *refs)
 			fclose(f);
 		}
 	}
+	free(packed_refs_file);
 	return refs->packed;
 }
 
@@ -1481,14 +1482,15 @@ static int resolve_gitlink_ref_recursive(struct ref_cache *refs,
 {
 	int fd, len;
 	char buffer[128], *p;
-	const char *path;
+	char *path;
 
 	if (recursion > MAXDEPTH || strlen(refname) > MAXREFLEN)
 		return -1;
 	path = *refs->name
-		? git_path_submodule(refs->name, "%s", refname)
-		: git_path("%s", refname);
+		? git_pathdup_submodule(refs->name, "%s", refname)
+		: git_pathdup("%s", refname);
 	fd = open(path, O_RDONLY);
+	free(path);
 	if (fd < 0)
 		return resolve_gitlink_packed_ref(refs, refname, sha1);
 
diff --git a/unpack-trees.c b/unpack-trees.c
index d6cf849..7bb446a 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1029,10 +1029,12 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 	if (!core_apply_sparse_checkout || !o->update)
 		o->skip_sparse_checkout = 1;
 	if (!o->skip_sparse_checkout) {
-		if (add_excludes_from_file_to_list(git_path("info/sparse-checkout"), "", 0, &el, 0) < 0)
+		char *sparse = git_pathdup("info/sparse-checkout");
+		if (add_excludes_from_file_to_list(sparse, "", 0, &el, 0) < 0)
 			o->skip_sparse_checkout = 1;
 		else
 			o->el = &el;
+		free(sparse);
 	}
 
 	memset(&o->result, 0, sizeof(o->result));

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:35 UTC · re: Jeff King · lore

[PATCH 07/17] prefer mkpathdup to mkpath in assignments

As with the previous commit to git_path, assigning the result of mkpath is suspicious, since it is not clear whether we will still depend on the value after it may have been overwritten by subsequent calls. This patch converts low-hanging fruit to use mkpathdup instead of mkpath (with the downside that we must remember to free the result).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/repack.c | 24 +++++++++++++-----------
 refs.c           |  6 ++++--
 2 files changed, 17 insertions(+), 13 deletions(-)
Show changes to 2 files +17 −13

builtin/repack.c, refs.c

diff --git a/builtin/repack.c b/builtin/repack.c
index af7340c..70b9b1e 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -285,8 +285,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	failed = 0;
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
-			const char *fname_old;
-			char *fname;
+			char *fname, *fname_old;
 			fname = mkpathdup("%s/pack-%s%s", packdir,
 						item->string, exts[ext].name);
 			if (!file_exists(fname)) {
@@ -294,7 +293,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 				continue;
 			}
 
-			fname_old = mkpath("%s/old-%s%s", packdir,
+			fname_old = mkpathdup("%s/old-%s%s", packdir,
 						item->string, exts[ext].name);
 			if (file_exists(fname_old))
 				if (unlink(fname_old))
@@ -302,10 +301,12 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 
 			if (!failed && rename(fname, fname_old)) {
 				free(fname);
+				free(fname_old);
 				failed = 1;
 				break;
 			} else {
 				string_list_append(&rollback, fname);
+				free(fname_old);
 			}
 		}
 		if (failed)
@@ -314,13 +315,13 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	if (failed) {
 		struct string_list rollback_failure = STRING_LIST_INIT_DUP;
 		for_each_string_list_item(item, &rollback) {
-			const char *fname_old;
-			char *fname;
+			char *fname, *fname_old;
 			fname = mkpathdup("%s/%s", packdir, item->string);
-			fname_old = mkpath("%s/old-%s", packdir, item->string);
+			fname_old = mkpathdup("%s/old-%s", packdir, item->string);
 			if (rename(fname_old, fname))
 				string_list_append(&rollback_failure, fname);
 			free(fname);
+			free(fname_old);
 		}
 
 		if (rollback_failure.nr) {
@@ -368,13 +369,14 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 	/* Remove the "old-" files */
 	for_each_string_list_item(item, &names) {
 		for (ext = 0; ext < ARRAY_SIZE(exts); ext++) {
-			const char *fname;
-			fname = mkpath("%s/old-%s%s",
-					packdir,
-					item->string,
-					exts[ext].name);
+			char *fname;
+			fname = mkpathdup("%s/old-%s%s",
+					  packdir,
+					  item->string,
+					  exts[ext].name);
 			if (remove_path(fname))
 				warning(_("removing '%s' failed"), fname);
+			free(fname);
 		}
 	}
 
diff --git a/refs.c b/refs.c
index 93b250e..e70941a 100644
--- a/refs.c
+++ b/refs.c
@@ -3380,7 +3380,7 @@ static int commit_ref_update(struct ref_lock *lock,
 int create_symref(const char *ref_target, const char *refs_heads_master,
 		  const char *logmsg)
 {
-	const char *lockpath;
+	char *lockpath = NULL;
 	char ref[1000];
 	int fd, len, written;
 	char *git_HEAD = git_pathdup("%s", ref_target);
@@ -3407,7 +3407,7 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 		error("refname too long: %s", refs_heads_master);
 		goto error_free_return;
 	}
-	lockpath = mkpath("%s.lock", git_HEAD);
+	lockpath = mkpathdup("%s.lock", git_HEAD);
 	fd = open(lockpath, O_CREAT | O_EXCL | O_WRONLY, 0666);
 	if (fd < 0) {
 		error("Unable to open %s for writing", lockpath);
@@ -3427,9 +3427,11 @@ int create_symref(const char *ref_target, const char *refs_heads_master,
 	error_unlink_return:
 		unlink_or_warn(lockpath);
 	error_free_return:
+		free(lockpath);
 		free(git_HEAD);
 		return -1;
 	}
+	free(lockpath);
 
 #ifndef NO_SYMLINK_HEAD
 	done:

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:35 UTC · re: Jeff King · lore

[PATCH 08/17] remote.c: drop extraneous local variable from migrate_file

It's an anti-pattern to assign the result of git_path to a variable, since other calls may reuse our buffer. In this case, we feed the result to unlink_or_warn immediately afterwards, so it's OK. However, it's nice to avoid assignment entirely, which makes it more obvious that there's no bug.

We can just pass the result directly to unlink_or_warn, which is a known-simple function. As a bonus, the code flow is a little more obvious, as we eliminate an extra conditional (a reader does not have to wonder any more "under which circumstances is 'path' set?").

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
Show changes to builtin/remote.c +2 −5
diff --git a/builtin/remote.c b/builtin/remote.c
index cc3c741..181668d 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -581,7 +581,6 @@ static int migrate_file(struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int i;
-	const char *path = NULL;
 
 	strbuf_addf(&buf, "remote.%s.url", remote->name);
 	for (i = 0; i < remote->url_nr; i++)
@@ -601,11 +600,9 @@ static int migrate_file(struct remote *remote)
 			return error(_("Could not append '%s' to '%s'"),
 					remote->fetch_refspec[i], buf.buf);
 	if (remote->origin == REMOTE_REMOTES)
-		path = git_path("remotes/%s", remote->name);
+		unlink_or_warn(git_path("remotes/%s", remote->name));
 	else if (remote->origin == REMOTE_BRANCHES)
-		path = git_path("branches/%s", remote->name);
-	if (path)
-		unlink_or_warn(path);
+		unlink_or_warn(git_path("branches/%s", remote->name));
 	return 0;
 }
 

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:36 UTC · re: Jeff King · lore

[PATCH 09/17] refs.c: remove extra git_path calls from read_loose_refs

In iterating over the loose refs in "refs/foo/", we keep a running strbuf with "refs/foo/one", "refs/foo/two", etc. But we also need to access these files in the filesystem, as ".git/refs/foo/one", etc. For this latter purpose, we make a series of independent calls to git_path(). These are safe (we only use the result to call stat()), but assigning the result of git_path is a suspicious pattern that we'd rather avoid.

This patch keeps a running buffer with ".git/refs/foo/", and we can just append/reset each directory element as we loop. This matches how we handle the refnames. It should also be more efficient, as we do not keep formatting the same ".git/refs/foo" prefix (which can be arbitrarily deep).

Technically we are dropping a call to strbuf_cleanup() on each generated filename, but that's OK; it wasn't doing anything, as we are putting in single-level names we read from the filesystem (so it could not possibly be cleaning up cruft like "./" in this instance).

A clever reader may also note that the running refname buffer ("refs/foo/") is actually a subset of the filesystem path buffer (".git/refs/foo/"). We could get by with one buffer, indexing the length of $GIT_DIR when we want the refname. However, having tried this, the resulting code actually ends up a little more confusing, and the efficiency improvement is tiny (and almost certainly dwarfed by the system calls we are making).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)
Show changes to refs.c +13 −10
diff --git a/refs.c b/refs.c
index e70941a..06f95c4 100644
--- a/refs.c
+++ b/refs.c
@@ -1352,19 +1352,23 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 {
 	struct ref_cache *refs = dir->ref_cache;
 	DIR *d;
-	const char *path;
 	struct dirent *de;
 	int dirnamelen = strlen(dirname);
 	struct strbuf refname;
+	struct strbuf path = STRBUF_INIT;
+	size_t path_baselen;
 
 	if (*refs->name)
-		path = git_path_submodule(refs->name, "%s", dirname);
+		strbuf_git_path_submodule(&path, refs->name, "%s", dirname);
 	else
-		path = git_path("%s", dirname);
+		strbuf_git_path(&path, "%s", dirname);
+	path_baselen = path.len;
 
-	d = opendir(path);
-	if (!d)
+	d = opendir(path.buf);
+	if (!d) {
+		strbuf_release(&path);
 		return;
+	}
 
 	strbuf_init(&refname, dirnamelen + 257);
 	strbuf_add(&refname, dirname, dirnamelen);
@@ -1373,17 +1377,14 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 		unsigned char sha1[20];
 		struct stat st;
 		int flag;
-		const char *refdir;
 
 		if (de->d_name[0] == '.')
 			continue;
 		if (ends_with(de->d_name, ".lock"))
 			continue;
 		strbuf_addstr(&refname, de->d_name);
-		refdir = *refs->name
-			? git_path_submodule(refs->name, "%s", refname.buf)
-			: git_path("%s", refname.buf);
-		if (stat(refdir, &st) < 0) {
+		strbuf_addstr(&path, de->d_name);
+		if (stat(path.buf, &st) < 0) {
 			; /* silently ignore */
 		} else if (S_ISDIR(st.st_mode)) {
 			strbuf_addch(&refname, '/');
@@ -1430,8 +1431,10 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir)
 					 create_ref_entry(refname.buf, sha1, flag, 0));
 		}
 		strbuf_setlen(&refname, dirnamelen);
+		strbuf_setlen(&path, path_baselen);
 	}
 	strbuf_release(&refname);
+	strbuf_release(&path);
 	closedir(d);
 }
 

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:36 UTC · re: Jeff King · lore

[PATCH 10/17] path.c: drop git_path_submodule

There are no callers of the slightly-dangerous static-buffer git_path_submodule left. Let's drop it.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h |  5 ++---
 path.c  | 10 ----------
 2 files changed, 2 insertions(+), 13 deletions(-)
Show changes to 2 files +2 −13

cache.h, path.c

diff --git a/cache.h b/cache.h
index 6f74f33..7a4fa90 100644
--- a/cache.h
+++ b/cache.h
@@ -713,12 +713,11 @@ extern int check_repository_format(void);
  * the repository directory (git_path), or in a submodule's repository
  * directory (git_path_submodule). In all cases, note that the result
  * may be overwritten by another call to _any_ of the functions. Consider
- * using the safer "dup" or "strbuf" formats below.
+ * using the safer "dup" or "strbuf" formats below (in some cases, the
+ * unsafe versions have already been removed).
  */
 extern const char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
 extern const char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern const char *git_path_submodule(const char *path, const char *fmt, ...)
-	__attribute__((format (printf, 2, 3)));
 
 extern char *mksnpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
diff --git a/path.c b/path.c
index 9aad9a1..94d7ec2 100644
--- a/path.c
+++ b/path.c
@@ -245,16 +245,6 @@ static void do_submodule_path(struct strbuf *buf, const char *path,
 	strbuf_cleanup_path(buf);
 }
 
-const char *git_path_submodule(const char *path, const char *fmt, ...)
-{
-	va_list args;
-	struct strbuf *buf = get_pathname();
-	va_start(args, fmt);
-	do_submodule_path(buf, path, fmt, args);
-	va_end(args);
-	return buf->buf;
-}
-
 char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 {
 	va_list args;

-- 
2.5.0.414.g670f2a4
Junio C Hamano· Aug 10, 2015, 22:50 UTC · re: Jeff King · lore

Re: [PATCH 10/17] path.c: drop git_path_submodule

Jeff King <peff@peff.net> writes:
> There are no callers of the slightly-dangerous static-buffer
> git_path_submodule left. Let's drop it.
There are a few callers added on 'pu', though.
Junio C Hamano· Aug 10, 2015, 22:57 UTC · re: Junio C Hamano · lore

Re: [PATCH 10/17] path.c: drop git_path_submodule

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> Jeff King <peff@peff.net> writes:
>
>> There are no callers of the slightly-dangerous static-buffer
>> git_path_submodule left. Let's drop it.
>
> There are a few callers added on 'pu', though.
Actually there is only one.  Here is a proposed evil merge.
Show changes to submodule.c +10 −9
diff --git a/submodule.c b/submodule.c
index dfe8b7b..7ab08a1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)
 static int add_submodule_odb(const char *path)
 {
 	struct alternate_object_database *alt_odb;
-	const char *objects_directory;
+	struct strbuf objects_directory = STRBUF_INIT;
 	int ret = 0;
 
-	objects_directory = git_path_submodule(path, "objects/");
-	if (!is_directory(objects_directory)) {
+	strbuf_git_path_submodule(&objects_directory, "objects/", "%s", path);
+	if (!is_directory(objects_directory.buf)) {
 		ret = -1;
 		goto done;
 	}
 
 	/* avoid adding it twice */
 	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
-		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
-				!strcmp(alt_odb->base, objects_directory))
+		if (alt_odb->name - alt_odb->base == objects_directory.len &&
+				!strcmp(alt_odb->base, objects_directory.buf))
 			goto done;
 
-	alt_odb = xmalloc(strlen(objects_directory) + 42 + sizeof(*alt_odb));
+	alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
 	alt_odb->next = alt_odb_list;
-	strcpy(alt_odb->base, objects_directory);
-	alt_odb->name = alt_odb->base + strlen(objects_directory);
+	strcpy(alt_odb->base, objects_directory.buf);
+	alt_odb->name = alt_odb->base + objects_directory.len;
 	alt_odb->name[2] = '/';
 	alt_odb->name[40] = '\0';
 	alt_odb->name[41] = '\0';
 	alt_odb_list = alt_odb;
 
 	/* add possible alternates from the submodule */
-	read_info_alternates(objects_directory, 0);
+	read_info_alternates(objects_directory.buf, 0);
 	prepare_alt_odb();
 done:
+	strbuf_release(&objects_directory);
 	return ret;
 }
Junio C Hamano· Aug 10, 2015, 23:52 UTC · re: Junio C Hamano · lore

Re: [PATCH 10/17] path.c: drop git_path_submodule

Junio C Hamano <gitster@pobox.com> writes:
Show 10 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> There are no callers of the slightly-dangerous static-buffer
>>> git_path_submodule left. Let's drop it.
>>
>> There are a few callers added on 'pu', though.
>
> Actually there is only one.  Here is a proposed evil merge.
Sorry, that didn't work X-<.
Show changes to submodule.c +3 −2
diff --git a/submodule.c b/submodule.c
index dfe8b7b..0cdaeb8 100644
--- a/submodule.c
+++ b/submodule.c
@@ -120,10 +120,10 @@ void stage_updated_gitmodules(void)
 static int add_submodule_odb(const char *path)
 {
 	struct alternate_object_database *alt_odb;
-	const char *objects_directory;
+	char *objects_directory;
 	int ret = 0;
 
-	objects_directory = git_path_submodule(path, "objects/");
+	objects_directory = git_pathdup_submodule(path, "objects/");
 	if (!is_directory(objects_directory)) {
 		ret = -1;
 		goto done;
@@ -148,6 +148,7 @@ static int add_submodule_odb(const char *path)
 	read_info_alternates(objects_directory, 0);
 	prepare_alt_odb();
 done:
+	free(objects_directory);
 	return ret;
 }
Jeff King· Aug 11, 2015, 09:53 UTC · re: Junio C Hamano · lore

Re: [PATCH 10/17] path.c: drop git_path_submodule

On Mon, Aug 10, 2015 at 03:57:31PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Jeff King <peff@peff.net> writes:
> >
> >> There are no callers of the slightly-dangerous static-buffer
> >> git_path_submodule left. Let's drop it.
> >
> > There are a few callers added on 'pu', though.
> 
> Actually there is only one.  Here is a proposed evil merge.

Thanks for catching. I (obviously) only checked against 'next' and not 'pu'. Rather than the evil merge, we could also just drop this patch. And then either leave it be, or fix this one up as a separate topic once merged. Though it really looks like this call site is potentially dangerous, with the assignment (I think it is OK, though, because read_info_alternates does not use git_path itself).

Show 20 quoted lines
> diff --git a/submodule.c b/submodule.c
> index dfe8b7b..7ab08a1 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)
>  static int add_submodule_odb(const char *path)
> [...]
>  {
>  	struct alternate_object_database *alt_odb;
> -	const char *objects_directory;
> +	struct strbuf objects_directory = STRBUF_INIT;
>  	int ret = 0;
>  
> -	objects_directory = git_path_submodule(path, "objects/");
> -	if (!is_directory(objects_directory)) {
> +	strbuf_git_path_submodule(&objects_directory, "objects/", "%s", path);
> +	if (!is_directory(objects_directory.buf)) {
>  		ret = -1;
>  		goto done;
>  	}

Hmph, the change in 6659e74 would have been a lot nicer if strbuf_git_path_submodule were already available. Most of what you are doing here is undoing that commit's strbuf/buf transition. :-/

Show 7 quoted lines
>  	/* avoid adding it twice */
>  	for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
> -		if (alt_odb->name - alt_odb->base == strlen(objects_directory) &&
> -				!strcmp(alt_odb->base, objects_directory))
> +		if (alt_odb->name - alt_odb->base == objects_directory.len &&
> +				!strcmp(alt_odb->base, objects_directory.buf))
>  			goto done;

Not really relevant to what we're talking about here, but this is the first time I've lookd at add_submodule_odb. It really looks like the whole second half of the function could be replaced with a call to link_alt_odb_entry.

-Peff
Jeff King· Aug 10, 2015, 09:36 UTC · re: Jeff King · lore

[PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing

Commit 1a83c24 (git_snpath(): retire and replace with strbuf_git_path(), 2014-11-30) taught log_ref_setup and log_ref_write_1 to take a strbuf parameter, rather than a bare string. It then makes an alias to the strbuf's "buf" field under the original name.

This made the original diff much shorter, but the resulting code is more complicated that it needs to be. Since we've aliased the pointer, we drop our reference to the strbuf to ensure we don't accidentally change it. But if we simply drop our alias and use "logfile.buf" directly, we do not have to worry about this aliasing. It's a larger diff, but the resulting code is simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)
Show changes to refs.c +15 −23
diff --git a/refs.c b/refs.c
index 06f95c4..3666132 100644
--- a/refs.c
+++ b/refs.c
@@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)
  * should_autocreate_reflog returns non-zero.  Otherwise, create it
  * regardless of the ref name.  Fill in *err and return -1 on failure.
  */
-static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
-	char *logfile;
 
-	strbuf_git_path(sb_logfile, "logs/%s", refname);
-	logfile = sb_logfile->buf;
-	/* make sure the rest of the function can't change "logfile" */
-	sb_logfile = NULL;
+	strbuf_git_path(logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (safe_create_leading_directories(logfile) < 0) {
+		if (safe_create_leading_directories(logfile->buf) < 0) {
 			strbuf_addf(err, "unable to create directory for %s: "
-				    "%s", logfile, strerror(errno));
+				    "%s", logfile->buf, strerror(errno));
 			return -1;
 		}
 		oflags |= O_CREAT;
 	}
 
-	logfd = open(logfile, oflags, 0666);
+	logfd = open(logfile->buf, oflags, 0666);
 	if (logfd < 0) {
 		if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
 			return 0;
 
 		if (errno == EISDIR) {
-			if (remove_empty_directories(logfile)) {
+			if (remove_empty_directories(logfile->buf)) {
 				strbuf_addf(err, "There are still logs under "
-					    "'%s'", logfile);
+					    "'%s'", logfile->buf);
 				return -1;
 			}
-			logfd = open(logfile, oflags, 0666);
+			logfd = open(logfile->buf, oflags, 0666);
 		}
 
 		if (logfd < 0) {
 			strbuf_addf(err, "unable to append to %s: %s",
-				    logfile, strerror(errno));
+				    logfile->buf, strerror(errno));
 			return -1;
 		}
 	}
 
-	adjust_shared_perm(logfile);
+	adjust_shared_perm(logfile->buf);
 	close(logfd);
 	return 0;
 }
@@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 
 static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
-			   struct strbuf *sb_log_file, int flags,
+			   struct strbuf *log_file, int flags,
 			   struct strbuf *err)
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
-	char *log_file;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refname, log_file, err, flags & REF_FORCE_CREATE_REFLOG);
 
 	if (result)
 		return result;
-	log_file = sb_log_file->buf;
-	/* make sure the rest of the function can't change "log_file" */
-	sb_log_file = NULL;
 
-	logfd = open(log_file, oflags);
+	logfd = open(log_file->buf, oflags);
 	if (logfd < 0)
 		return 0;
 	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
 				  git_committer_info(0), msg);
 	if (result) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", log_file->buf,
 			    strerror(errno));
 		close(logfd);
 		return -1;
 	}
 	if (close(logfd)) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", log_file->buf,
 			    strerror(errno));
 		return -1;
 	}

-- 
2.5.0.414.g670f2a4
Michael Haggerty· Aug 10, 2015, 10:34 UTC · re: Jeff King · lore

Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing

On 08/10/2015 11:36 AM, Jeff King wrote:
Show 40 quoted lines
> Commit 1a83c24 (git_snpath(): retire and replace with
> strbuf_git_path(), 2014-11-30) taught log_ref_setup and
> log_ref_write_1 to take a strbuf parameter, rather than a
> bare string. It then makes an alias to the strbuf's "buf"
> field under the original name.
> 
> This made the original diff much shorter, but the resulting
> code is more complicated that it needs to be. Since we've
> aliased the pointer, we drop our reference to the strbuf to
> ensure we don't accidentally change it. But if we simply
> drop our alias and use "logfile.buf" directly, we do not
> have to worry about this aliasing. It's a larger diff, but
> the resulting code is simpler.
> 
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  refs.c | 38 +++++++++++++++-----------------------
>  1 file changed, 15 insertions(+), 23 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index 06f95c4..3666132 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)
>   * should_autocreate_reflog returns non-zero.  Otherwise, create it
>   * regardless of the ref name.  Fill in *err and return -1 on failure.
>   */
> -static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
> +static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
>  {
> [...]
> @@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
>  
>  static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
>  			   const unsigned char *new_sha1, const char *msg,
> -			   struct strbuf *sb_log_file, int flags,
> +			   struct strbuf *log_file, int flags,
>  			   struct strbuf *err)
>  {
> [...]
Nice change.

How about taking this opportunity to name the parameters ("logfile" vs. "log_file") consistently?

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Jeff King· Aug 10, 2015, 12:26 UTC · re: Michael Haggerty · lore

Re: [PATCH 11/17] refs.c: simplify strbufs in reflog setup and writing

On Mon, Aug 10, 2015 at 12:34:05PM +0200, Michael Haggerty wrote:
> How about taking this opportunity to name the parameters ("logfile" vs.
> "log_file") consistently?
Thanks, that's a good suggestion. Here's a re-roll of this patch:
-- >8 --
Subject: [PATCH] refs.c: simplify strbufs in reflog setup and writing

Commit 1a83c24 (git_snpath(): retire and replace with strbuf_git_path(), 2014-11-30) taught log_ref_setup and log_ref_write_1 to take a strbuf parameter, rather than a bare string. It then makes an alias to the strbuf's "buf" field under the original name.

This made the original diff much shorter, but the resulting code is more complicated that it needs to be. Since we've aliased the pointer, we drop our reference to the strbuf to ensure we don't accidentally change it. But if we simply drop our alias and use "logfile.buf" directly, we do not have to worry about this aliasing. It's a larger diff, but the resulting code is simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)
Show changes to refs.c +15 −23
diff --git a/refs.c b/refs.c
index 06f95c4..105f271 100644
--- a/refs.c
+++ b/refs.c
@@ -3150,46 +3150,42 @@ static int should_autocreate_reflog(const char *refname)
  * should_autocreate_reflog returns non-zero.  Otherwise, create it
  * regardless of the ref name.  Fill in *err and return -1 on failure.
  */
-static int log_ref_setup(const char *refname, struct strbuf *sb_logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
 {
 	int logfd, oflags = O_APPEND | O_WRONLY;
-	char *logfile;
 
-	strbuf_git_path(sb_logfile, "logs/%s", refname);
-	logfile = sb_logfile->buf;
-	/* make sure the rest of the function can't change "logfile" */
-	sb_logfile = NULL;
+	strbuf_git_path(logfile, "logs/%s", refname);
 	if (force_create || should_autocreate_reflog(refname)) {
-		if (safe_create_leading_directories(logfile) < 0) {
+		if (safe_create_leading_directories(logfile->buf) < 0) {
 			strbuf_addf(err, "unable to create directory for %s: "
-				    "%s", logfile, strerror(errno));
+				    "%s", logfile->buf, strerror(errno));
 			return -1;
 		}
 		oflags |= O_CREAT;
 	}
 
-	logfd = open(logfile, oflags, 0666);
+	logfd = open(logfile->buf, oflags, 0666);
 	if (logfd < 0) {
 		if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
 			return 0;
 
 		if (errno == EISDIR) {
-			if (remove_empty_directories(logfile)) {
+			if (remove_empty_directories(logfile->buf)) {
 				strbuf_addf(err, "There are still logs under "
-					    "'%s'", logfile);
+					    "'%s'", logfile->buf);
 				return -1;
 			}
-			logfd = open(logfile, oflags, 0666);
+			logfd = open(logfile->buf, oflags, 0666);
 		}
 
 		if (logfd < 0) {
 			strbuf_addf(err, "unable to append to %s: %s",
-				    logfile, strerror(errno));
+				    logfile->buf, strerror(errno));
 			return -1;
 		}
 	}
 
-	adjust_shared_perm(logfile);
+	adjust_shared_perm(logfile->buf);
 	close(logfd);
 	return 0;
 }
@@ -3233,36 +3229,32 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
 
 static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
 			   const unsigned char *new_sha1, const char *msg,
-			   struct strbuf *sb_log_file, int flags,
+			   struct strbuf *logfile, int flags,
 			   struct strbuf *err)
 {
 	int logfd, result, oflags = O_APPEND | O_WRONLY;
-	char *log_file;
 
 	if (log_all_ref_updates < 0)
 		log_all_ref_updates = !is_bare_repository();
 
-	result = log_ref_setup(refname, sb_log_file, err, flags & REF_FORCE_CREATE_REFLOG);
+	result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
 
 	if (result)
 		return result;
-	log_file = sb_log_file->buf;
-	/* make sure the rest of the function can't change "log_file" */
-	sb_log_file = NULL;
 
-	logfd = open(log_file, oflags);
+	logfd = open(logfile->buf, oflags);
 	if (logfd < 0)
 		return 0;
 	result = log_ref_write_fd(logfd, old_sha1, new_sha1,
 				  git_committer_info(0), msg);
 	if (result) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
 			    strerror(errno));
 		close(logfd);
 		return -1;
 	}
 	if (close(logfd)) {
-		strbuf_addf(err, "unable to append to %s: %s", log_file,
+		strbuf_addf(err, "unable to append to %s: %s", logfile->buf,
 			    strerror(errno));
 		return -1;
 	}

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:36 UTC · re: Jeff King · lore

[PATCH 12/17] refs.c: avoid repeated git_path calls in rename_tmp_log

Because it's not safe to store the static-buffer results of git_path for a long time, we end up formatting the same filename over and over. We can fix this by using a function-local strbuf to store the formatted pathname and avoid repeating ourselves.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)
Show changes to refs.c +14 −7
diff --git a/refs.c b/refs.c
index 3666132..61a318f 100644
--- a/refs.c
+++ b/refs.c
@@ -2930,9 +2930,13 @@ out:
 static int rename_tmp_log(const char *newrefname)
 {
 	int attempts_remaining = 4;
+	struct strbuf path = STRBUF_INIT;
+	int ret = -1;
 
  retry:
-	switch (safe_create_leading_directories_const(git_path("logs/%s", newrefname))) {
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "logs/%s", newrefname);
+	switch (safe_create_leading_directories_const(path.buf)) {
 	case SCLD_OK:
 		break; /* success */
 	case SCLD_VANISHED:
@@ -2941,19 +2945,19 @@ static int rename_tmp_log(const char *newrefname)
 		/* fall through */
 	default:
 		error("unable to create directory for %s", newrefname);
-		return -1;
+		goto out;
 	}
 
-	if (rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newrefname))) {
+	if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
 		if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
 			/*
 			 * rename(a, b) when b is an existing
 			 * directory ought to result in ISDIR, but
 			 * Solaris 5.8 gives ENOTDIR.  Sheesh.
 			 */
-			if (remove_empty_directories(git_path("logs/%s", newrefname))) {
+			if (remove_empty_directories(path.buf)) {
 				error("Directory not empty: logs/%s", newrefname);
-				return -1;
+				goto out;
 			}
 			goto retry;
 		} else if (errno == ENOENT && --attempts_remaining > 0) {
@@ -2966,10 +2970,13 @@ static int rename_tmp_log(const char *newrefname)
 		} else {
 			error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
 				newrefname, strerror(errno));
-			return -1;
+			goto out;
 		}
 	}
-	return 0;
+	ret = 0;
+out:
+	strbuf_release(&path);
+	return ret;
 }
 
 static int rename_ref_available(const char *oldname, const char *newname)

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:37 UTC · re: Jeff King · lore

[PATCH 13/17] refs.c: avoid git_path assignment in lock_ref_sha1_basic

Assigning the result of git_path is a bad pattern, because it's not immediately obvious how long you expect the content to stay valid (and it may be overwritten by subsequent calls). Let's use a function-local strbuf here instead, which we know is safe (we just have to remember to free it in all code paths).

As a bonus, we get rid of a confusing variable-reuse ("ref_file" is used for two distinct purposes).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)
Show changes to refs.c +19 −13
diff --git a/refs.c b/refs.c
index 61a318f..8566677 100644
--- a/refs.c
+++ b/refs.c
@@ -2408,7 +2408,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 					    unsigned int flags, int *type_p,
 					    struct strbuf *err)
 {
-	const char *ref_file;
+	struct strbuf ref_file = STRBUF_INIT;
+	struct strbuf orig_ref_file = STRBUF_INIT;
 	const char *orig_refname = refname;
 	struct ref_lock *lock;
 	int last_errno = 0;
@@ -2432,20 +2433,19 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	refname = resolve_ref_unsafe(refname, resolve_flags,
 				     lock->old_oid.hash, &type);
 	if (!refname && errno == EISDIR) {
-		/* we are trying to lock foo but we used to
+		/*
+		 * we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
 		 * it is normal for the empty directory 'foo'
 		 * to remain.
 		 */
-		ref_file = git_path("%s", orig_refname);
-		if (remove_empty_directories(ref_file)) {
+		strbuf_git_path(&orig_ref_file, "%s", orig_refname);
+		if (remove_empty_directories(orig_ref_file.buf)) {
 			last_errno = errno;
-
 			if (!verify_refname_available(orig_refname, extras, skip,
 						      get_loose_refs(&ref_cache), err))
 				strbuf_addf(err, "there are still refs under '%s'",
 					    orig_refname);
-
 			goto error_return;
 		}
 		refname = resolve_ref_unsafe(orig_refname, resolve_flags,
@@ -2485,10 +2485,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 	}
 	lock->ref_name = xstrdup(refname);
 	lock->orig_ref_name = xstrdup(orig_refname);
-	ref_file = git_path("%s", refname);
+	strbuf_git_path(&ref_file, "%s", refname);
 
  retry:
-	switch (safe_create_leading_directories_const(ref_file)) {
+	switch (safe_create_leading_directories_const(ref_file.buf)) {
 	case SCLD_OK:
 		break; /* success */
 	case SCLD_VANISHED:
@@ -2497,11 +2497,12 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		/* fall through */
 	default:
 		last_errno = errno;
-		strbuf_addf(err, "unable to create directory for %s", ref_file);
+		strbuf_addf(err, "unable to create directory for %s",
+			    ref_file.buf);
 		goto error_return;
 	}
 
-	if (hold_lock_file_for_update(lock->lk, ref_file, lflags) < 0) {
+	if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {
 		last_errno = errno;
 		if (errno == ENOENT && --attempts_remaining > 0)
 			/*
@@ -2511,7 +2512,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 			 */
 			goto retry;
 		else {
-			unable_to_lock_message(ref_file, errno, err);
+			unable_to_lock_message(ref_file.buf, errno, err);
 			goto error_return;
 		}
 	}
@@ -2519,12 +2520,17 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		last_errno = errno;
 		goto error_return;
 	}
-	return lock;
+	goto out;
 
  error_return:
 	unlock_ref(lock);
+	lock = NULL;
+
+ out:
+	strbuf_release(&ref_file);
+	strbuf_release(&orig_ref_file);
 	errno = last_errno;
-	return NULL;
+	return lock;
 }
 
 /*

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:37 UTC · re: Jeff King · lore

[PATCH 14/17] refs.c: remove_empty_directories can take a strbuf

The first thing we do in this function is copy the input into a strbuf. Of the 4 callers, 3 of them already have a strbuf we could use. Let's just take the strbuf, and convert the remaining caller to use a strbuf, rather than a raw git_path. This is safer, anyway, as remove_dir_recursively is a non-trivial function that might use the pathname buffers itself (this is _probably_ OK, as the likely culprit would be calling resolve_gitlink_ref, but we do not pass the proper flags to ask it to avoid blowing away gitlinks).

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)
Show changes to refs.c +15 −19
diff --git a/refs.c b/refs.c
index 8566677..ec1d06c 100644
--- a/refs.c
+++ b/refs.c
@@ -2290,25 +2290,14 @@ static int verify_lock(struct ref_lock *lock,
 	return 0;
 }
 
-static int remove_empty_directories(const char *file)
+static int remove_empty_directories(struct strbuf *path)
 {
-	/* we want to create a file but there is a directory there;
+	/*
+	 * we want to create a file but there is a directory there;
 	 * if that is an empty directory (or a directory that contains
 	 * only empty directories), remove them.
 	 */
-	struct strbuf path;
-	int result, save_errno;
-
-	strbuf_init(&path, 20);
-	strbuf_addstr(&path, file);
-
-	result = remove_dir_recursively(&path, REMOVE_DIR_EMPTY_ONLY);
-	save_errno = errno;
-
-	strbuf_release(&path);
-	errno = save_errno;
-
-	return result;
+	return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);
 }
 
 /*
@@ -2440,7 +2429,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,
 		 * to remain.
 		 */
 		strbuf_git_path(&orig_ref_file, "%s", orig_refname);
-		if (remove_empty_directories(orig_ref_file.buf)) {
+		if (remove_empty_directories(&orig_ref_file)) {
 			last_errno = errno;
 			if (!verify_refname_available(orig_refname, extras, skip,
 						      get_loose_refs(&ref_cache), err))
@@ -2961,7 +2950,7 @@ static int rename_tmp_log(const char *newrefname)
 			 * directory ought to result in ISDIR, but
 			 * Solaris 5.8 gives ENOTDIR.  Sheesh.
 			 */
-			if (remove_empty_directories(path.buf)) {
+			if (remove_empty_directories(&path)) {
 				error("Directory not empty: logs/%s", newrefname);
 				goto out;
 			}
@@ -3046,7 +3035,14 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms
 	if (!read_ref_full(newrefname, RESOLVE_REF_READING, sha1, NULL) &&
 	    delete_ref(newrefname, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
-			if (remove_empty_directories(git_path("%s", newrefname))) {
+			struct strbuf path = STRBUF_INIT;
+			int result;
+
+			strbuf_git_path(&path, "%s", newrefname);
+			result = remove_empty_directories(&path);
+			strbuf_release(&path);
+
+			if (result) {
 				error("Directory not empty: %s", newrefname);
 				goto rollback;
 			}
@@ -3183,7 +3179,7 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
 			return 0;
 
 		if (errno == EISDIR) {
-			if (remove_empty_directories(logfile->buf)) {
+			if (remove_empty_directories(logfile)) {
 				strbuf_addf(err, "There are still logs under "
 					    "'%s'", logfile->buf);
 				return -1;

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:37 UTC · re: Jeff King · lore

[PATCH 15/17] find_hook: keep our own static buffer

The find_hook function returns the results of git_path, which is a static buffer shared by other path-related calls. Returning such a buffer is slightly dangerous, because it can be overwritten by seemingly unrelated functions.

Let's at least keep our _own_ static buffer, so you can only get in trouble by calling find_hook in quick succession, which is less likely to happen and more obvious to notice.

While we're at it, let's add some documentation of the function's limitations.

Signed-off-by: Jeff King <peff@peff.net>
---
 run-command.c | 10 ++++++----
 run-command.h |  5 +++++
 2 files changed, 11 insertions(+), 4 deletions(-)
Show changes to 2 files +11 −4

run-command.c, run-command.h

diff --git a/run-command.c b/run-command.c
index 4d73e90..28e1d55 100644
--- a/run-command.c
+++ b/run-command.c
@@ -797,11 +797,13 @@ int finish_async(struct async *async)
 
 const char *find_hook(const char *name)
 {
-	const char *path = git_path("hooks/%s", name);
-	if (access(path, X_OK) < 0)
-		path = NULL;
+	static struct strbuf path = STRBUF_INIT;
 
-	return path;
+	strbuf_reset(&path);
+	strbuf_git_path(&path, "hooks/%s", name);
+	if (access(path.buf, X_OK) < 0)
+		return NULL;
+	return path.buf;
 }
 
 int run_hook_ve(const char *const *env, const char *name, va_list args)
diff --git a/run-command.h b/run-command.h
index 1103805..5b4425a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -52,6 +52,11 @@ int start_command(struct child_process *);
 int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
+/*
+ * Returns the path to the hook file, or NULL if the hook is missing
+ * or disabled. Note that this points to static storage that will be
+ * overwritten by further calls to find_hook and run_hook_*.
+ */
 extern const char *find_hook(const char *name);
 LAST_ARG_MUST_BE_NULL
 extern int run_hook_le(const char *const *env, const char *name, ...);

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:37 UTC · re: Jeff King · lore

[PATCH 16/17] get_repo_path: refactor path-allocation

The get_repo_path function calls mkpath() and then does some non-trivial operations on it, like calling is_git_directory() and read_gitfile(). These are actually OK (they do not use more pathname static buffers themselves), but it takes a fair bit of work to verify.

Let's use our own strbuf to store the path, and we can simply reuse it for each iteration of the loop (we can even avoid rewriting the beginning part, since we are trying a series of suffixes).

To make the strbuf cleanup easier, we split out a thin wrapper. As a bonus, this wrapper can factor out the canonicalization that happens in all of the early-return code paths.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)
Show changes to builtin/clone.c +29 −14
diff --git a/builtin/clone.c b/builtin/clone.c
index 303a3a7..5864ad1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -99,51 +99,66 @@ static const char *argv_submodule[] = {
 	"submodule", "update", "--init", "--recursive", NULL
 };
 
-static char *get_repo_path(const char *repo, int *is_bundle)
+static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
 	static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
 	static char *bundle_suffix[] = { ".bundle", "" };
+	size_t baselen = path->len;
 	struct stat st;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(suffix); i++) {
-		const char *path;
-		path = mkpath("%s%s", repo, suffix[i]);
-		if (stat(path, &st))
+		strbuf_setlen(path, baselen);
+		strbuf_addstr(path, suffix[i]);
+		if (stat(path->buf, &st))
 			continue;
-		if (S_ISDIR(st.st_mode) && is_git_directory(path)) {
+		if (S_ISDIR(st.st_mode) && is_git_directory(path->buf)) {
 			*is_bundle = 0;
-			return xstrdup(absolute_path(path));
+			return path->buf;
 		} else if (S_ISREG(st.st_mode) && st.st_size > 8) {
 			/* Is it a "gitfile"? */
 			char signature[8];
-			int len, fd = open(path, O_RDONLY);
+			const char *dst;
+			int len, fd = open(path->buf, O_RDONLY);
 			if (fd < 0)
 				continue;
 			len = read_in_full(fd, signature, 8);
 			close(fd);
 			if (len != 8 || strncmp(signature, "gitdir: ", 8))
 				continue;
-			path = read_gitfile(path);
-			if (path) {
+			dst = read_gitfile(path->buf);
+			if (dst) {
 				*is_bundle = 0;
-				return xstrdup(absolute_path(path));
+				return dst;
 			}
 		}
 	}
 
 	for (i = 0; i < ARRAY_SIZE(bundle_suffix); i++) {
-		const char *path;
-		path = mkpath("%s%s", repo, bundle_suffix[i]);
-		if (!stat(path, &st) && S_ISREG(st.st_mode)) {
+		strbuf_setlen(path, baselen);
+		strbuf_addstr(path, bundle_suffix[i]);
+		if (!stat(path->buf, &st) && S_ISREG(st.st_mode)) {
 			*is_bundle = 1;
-			return xstrdup(absolute_path(path));
+			return path->buf;
 		}
 	}
 
 	return NULL;
 }
 
+static char *get_repo_path(const char *repo, int *is_bundle)
+{
+	struct strbuf path = STRBUF_INIT;
+	const char *raw;
+	char *canon;
+
+	strbuf_addstr(&path, repo);
+	raw = get_repo_path_1(&path, is_bundle);
+	canon = raw ? xstrdup(absolute_path(raw)) : NULL;
+	strbuf_release(&path);
+	return canon;
+}
+
 static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 {
 	const char *end = repo + strlen(repo), *start;

-- 
2.5.0.414.g670f2a4
Jeff King· Aug 10, 2015, 09:38 UTC · re: Jeff King · lore

[PATCH 17/17] memoize common git-path "constant" files

One of the most common uses of git_path() is to pass a constant, like git_path("MERGE_MSG"). This has two drawbacks:

  1. The return value is a static buffer, and the lifetime
     is dependent on other calls to git_path, etc.
  2. There's no compile-time checking of the pathname. This
     is OK for a one-off (after all, we have to spell it
     correctly at least once), but many of these constant
     strings appear throughout the code.

This patch introduces a series of functions to "memoize" these strings, which are essentially globals for the lifetime of the program. We compute the value once, take ownership of the buffer, and return the cached value for subsequent calls. cache.h provides a helper macro for defining these functions as one-liners, and defines a few common ones for global use.

Using a macro is a little bit gross, but it does nicely document the purpose of the functions. If we need to touch them all later (e.g., because we learned how to change the git_dir variable at runtime, and need to invalidate all of the stored values), it will be much easier to have the complete list.

Note that the shared-global functions have separate, manual declarations. We could do something clever with the macros (e.g., expand it to a declaration in some places, and a declaration _and_ a definition in path.c). But there aren't that many, and it's probably better to stay away from too-magical macros.

Likewise, if we abandon the C preprocessor in favor of generating these with a script, we could get much fancier. E.g., normalizing "FOO/BAR-BAZ" into "git_path_foo_bar_baz". But the small amount of saved typing is probably not worth the resulting confusion to readers who want to grep for the function's definition.

Signed-off-by: Jeff King <peff@peff.net>
---
This one is probably the most contentious, as it has very far-reaching
effects. But it really reduces the number of sites that need audited by
quite a lot.
 attr.c                                 |  4 +-
 bisect.c                               |  7 ++-
 branch.c                               | 14 +++---
 builtin/blame.c                        |  7 ++-
 builtin/commit.c                       | 32 ++++++-------
 builtin/fetch.c                        |  4 +-
 builtin/merge.c                        | 30 ++++++------
 builtin/reset.c                        |  2 +-
 cache.h                                | 26 ++++++++++
 contrib/examples/builtin-fetch--tool.c |  4 +-
 dir.c                                  |  4 +-
 fetch-pack.c                           |  2 +-
 path.c                                 | 10 ++++
 rerere.c                               | 19 ++++----
 sequencer.c                            | 87 ++++++++++++++++------------------
 shallow.c                              | 10 ++--
 wt-status.c                            |  8 ++--
 17 files changed, 151 insertions(+), 119 deletions(-)
Show changes to 17 files +151 −119

attr.c, bisect.c, branch.c, builtin/blame.c, builtin/commit.c, builtin/fetch.c, builtin/merge.c, builtin/reset.c, cache.h, contrib/examples/builtin-fetch--tool.c, dir.c, fetch-pack.c, path.c, rerere.c, sequencer.c, shallow.c, wt-status.c

diff --git a/attr.c b/attr.c
index 8f2ac6c..086c08d 100644
--- a/attr.c
+++ b/attr.c
@@ -490,6 +490,8 @@ static int git_attr_system(void)
 	return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
 }
 
+static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
+
 static void bootstrap_attr_stack(void)
 {
 	struct attr_stack *elem;
@@ -531,7 +533,7 @@ static void bootstrap_attr_stack(void)
 		debug_push(elem);
 	}
 
-	elem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);
+	elem = read_attr_from_file(git_path_info_attributes(), 1);
 	if (!elem)
 		elem = xcalloc(1, sizeof(*elem));
 	elem->origin = NULL;
diff --git a/bisect.c b/bisect.c
index 03d5cd9..7a6498c 100644
--- a/bisect.c
+++ b/bisect.c
@@ -420,10 +420,13 @@ static int read_bisect_refs(void)
 	return for_each_ref_in("refs/bisect/", register_ref, NULL);
 }
 
+static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
+static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+
 static void read_bisect_paths(struct argv_array *array)
 {
 	struct strbuf str = STRBUF_INIT;
-	const char *filename = git_path("BISECT_NAMES");
+	const char *filename = git_path_bisect_names();
 	FILE *fp = fopen(filename, "r");
 
 	if (!fp)
@@ -644,7 +647,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 
 static int is_expected_rev(const struct object_id *oid)
 {
-	const char *filename = git_path("BISECT_EXPECTED_REV");
+	const char *filename = git_path_bisect_expected_rev();
 	struct stat st;
 	struct strbuf str = STRBUF_INIT;
 	FILE *fp;
diff --git a/branch.c b/branch.c
index b002435..e283683 100644
--- a/branch.c
+++ b/branch.c
@@ -302,11 +302,11 @@ void create_branch(const char *head,
 
 void remove_branch_state(void)
 {
-	unlink(git_path("CHERRY_PICK_HEAD"));
-	unlink(git_path("REVERT_HEAD"));
-	unlink(git_path("MERGE_HEAD"));
-	unlink(git_path("MERGE_RR"));
-	unlink(git_path("MERGE_MSG"));
-	unlink(git_path("MERGE_MODE"));
-	unlink(git_path("SQUASH_MSG"));
+	unlink(git_path_cherry_pick_head());
+	unlink(git_path_revert_head());
+	unlink(git_path_merge_head());
+	unlink(git_path_merge_rr());
+	unlink(git_path_merge_msg());
+	unlink(git_path_merge_mode());
+	unlink(git_path_squash_msg());
 }
diff --git a/builtin/blame.c b/builtin/blame.c
index 04e4864..4db01c1 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2227,20 +2227,19 @@ static struct commit_list **append_parent(struct commit_list **tail, const unsig
 static void append_merge_parents(struct commit_list **tail)
 {
 	int merge_head;
-	const char *merge_head_file = git_path("MERGE_HEAD");
 	struct strbuf line = STRBUF_INIT;
 
-	merge_head = open(merge_head_file, O_RDONLY);
+	merge_head = open(git_path_merge_head(), O_RDONLY);
 	if (merge_head < 0) {
 		if (errno == ENOENT)
 			return;
-		die("cannot open '%s' for reading", merge_head_file);
+		die("cannot open '%s' for reading", git_path_merge_head());
 	}
 
 	while (!strbuf_getwholeline_fd(&line, merge_head, '\n')) {
 		unsigned char sha1[20];
 		if (line.len < 40 || get_sha1_hex(line.buf, sha1))
-			die("unknown line in '%s': %s", merge_head_file, line.buf);
+			die("unknown line in '%s': %s", git_path_merge_head(), line.buf);
 		tail = append_parent(tail, sha1);
 	}
 	close(merge_head);
diff --git a/builtin/commit.c b/builtin/commit.c
index 7314f33..4cbd5ff 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -166,9 +166,9 @@ static int opt_parse_m(const struct option *opt, const char *arg, int unset)
 
 static void determine_whence(struct wt_status *s)
 {
-	if (file_exists(git_path("MERGE_HEAD")))
+	if (file_exists(git_path_merge_head()))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
+	else if (file_exists(git_path_cherry_pick_head())) {
 		whence = FROM_CHERRY_PICK;
 		if (file_exists(git_path(SEQ_DIR)))
 			sequencer_in_use = 1;
@@ -725,12 +725,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		format_commit_message(commit, "fixup! %s\n\n",
 				      &sb, &ctx);
 		hook_arg1 = "message";
-	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
-		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
+	} else if (!stat(git_path_merge_msg(), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path_merge_msg(), 0) < 0)
 			die_errno(_("could not read MERGE_MSG"));
 		hook_arg1 = "merge";
-	} else if (!stat(git_path("SQUASH_MSG"), &statbuf)) {
-		if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0)
+	} else if (!stat(git_path_squash_msg(), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path_squash_msg(), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
 		hook_arg1 = "squash";
 	} else if (template_file) {
@@ -1684,10 +1684,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		if (!reflog_msg)
 			reflog_msg = "commit (merge)";
 		pptr = &commit_list_insert(current_head, pptr)->next;
-		fp = fopen(git_path("MERGE_HEAD"), "r");
+		fp = fopen(git_path_merge_head(), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
-				  git_path("MERGE_HEAD"));
+				  git_path_merge_head());
 		while (strbuf_getline(&m, fp, '\n') != EOF) {
 			struct commit *parent;
 
@@ -1698,8 +1698,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		}
 		fclose(fp);
 		strbuf_release(&m);
-		if (!stat(git_path("MERGE_MODE"), &statbuf)) {
-			if (strbuf_read_file(&sb, git_path("MERGE_MODE"), 0) < 0)
+		if (!stat(git_path_merge_mode(), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_merge_mode(), 0) < 0)
 				die_errno(_("could not read MERGE_MODE"));
 			if (!strcmp(sb.buf, "no-ff"))
 				allow_fast_forward = 0;
@@ -1775,12 +1775,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	}
 	ref_transaction_free(transaction);
 
-	unlink(git_path("CHERRY_PICK_HEAD"));
-	unlink(git_path("REVERT_HEAD"));
-	unlink(git_path("MERGE_HEAD"));
-	unlink(git_path("MERGE_MSG"));
-	unlink(git_path("MERGE_MODE"));
-	unlink(git_path("SQUASH_MSG"));
+	unlink(git_path_cherry_pick_head());
+	unlink(git_path_revert_head());
+	unlink(git_path_merge_head());
+	unlink(git_path_merge_msg());
+	unlink(git_path_merge_mode());
+	unlink(git_path_squash_msg());
 
 	if (commit_index_files())
 		die (_("Repository has been updated, but unable to write\n"
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 34b6f5e..df16d0a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -591,7 +591,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	const char *what, *kind;
 	struct ref *rm;
 	char *url;
-	const char *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	const char *filename = dry_run ? "/dev/null" : git_path_fetch_head();
 	int want_status;
 
 	fp = fopen(filename, "a");
@@ -834,7 +834,7 @@ static void check_not_current_branch(struct ref *ref_map)
 
 static int truncate_fetch_head(void)
 {
-	const char *filename = git_path("FETCH_HEAD");
+	const char *filename = git_path_fetch_head();
 	FILE *fp = fopen(filename, "w");
 
 	if (!fp)
diff --git a/builtin/merge.c b/builtin/merge.c
index 85c54dc..a0edaca 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -231,9 +231,9 @@ static struct option builtin_merge_options[] = {
 /* Cleans up metadata that is uninteresting after a succeeded merge. */
 static void drop_save(void)
 {
-	unlink(git_path("MERGE_HEAD"));
-	unlink(git_path("MERGE_MSG"));
-	unlink(git_path("MERGE_MODE"));
+	unlink(git_path_merge_head());
+	unlink(git_path_merge_msg());
+	unlink(git_path_merge_mode());
 }
 
 static int save_state(unsigned char *stash)
@@ -338,7 +338,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
 	struct pretty_print_context ctx = {0};
 
 	printf(_("Squash commit -- not updating HEAD\n"));
-	filename = git_path("SQUASH_MSG");
+	filename = git_path_squash_msg();
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not write to '%s'"), filename);
@@ -754,7 +754,7 @@ static void add_strategies(const char *string, unsigned attr)
 
 static void write_merge_msg(struct strbuf *msg)
 {
-	const char *filename = git_path("MERGE_MSG");
+	const char *filename = git_path_merge_msg();
 	int fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"),
@@ -766,7 +766,7 @@ static void write_merge_msg(struct strbuf *msg)
 
 static void read_merge_msg(struct strbuf *msg)
 {
-	const char *filename = git_path("MERGE_MSG");
+	const char *filename = git_path_merge_msg();
 	strbuf_reset(msg);
 	if (strbuf_read_file(msg, filename, 0) < 0)
 		die_errno(_("Could not read from '%s'"), filename);
@@ -799,10 +799,10 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
 	write_merge_msg(&msg);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
-			    git_path("MERGE_MSG"), "merge", NULL))
+			    git_path_merge_msg(), "merge", NULL))
 		abort_commit(remoteheads, NULL);
 	if (0 < option_edit) {
-		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
+		if (launch_editor(git_path_merge_msg(), NULL, NULL))
 			abort_commit(remoteheads, NULL);
 	}
 	read_merge_msg(&msg);
@@ -865,7 +865,7 @@ static int suggest_conflicts(void)
 	FILE *fp;
 	struct strbuf msgbuf = STRBUF_INIT;
 
-	filename = git_path("MERGE_MSG");
+	filename = git_path_merge_msg();
 	fp = fopen(filename, "a");
 	if (!fp)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -967,7 +967,7 @@ static void write_merge_state(struct commit_list *remoteheads)
 		}
 		strbuf_addf(&buf, "%s\n", sha1_to_hex(sha1));
 	}
-	filename = git_path("MERGE_HEAD");
+	filename = git_path_merge_head();
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -977,7 +977,7 @@ static void write_merge_state(struct commit_list *remoteheads)
 	strbuf_addch(&merge_msg, '\n');
 	write_merge_msg(&merge_msg);
 
-	filename = git_path("MERGE_MODE");
+	filename = git_path_merge_mode();
 	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -1070,7 +1070,7 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge
 	if (!merge_names)
 		merge_names = &fetch_head_file;
 
-	filename = git_path("FETCH_HEAD");
+	filename = git_path_fetch_head();
 	fd = open(filename, O_RDONLY);
 	if (fd < 0)
 		die_errno(_("could not open '%s' for reading"), filename);
@@ -1204,7 +1204,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		int nargc = 2;
 		const char *nargv[] = {"reset", "--merge", NULL};
 
-		if (!file_exists(git_path("MERGE_HEAD")))
+		if (!file_exists(git_path_merge_head()))
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
 		/* Invoke 'git reset --merge' */
@@ -1215,7 +1215,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (read_cache_unmerged())
 		die_resolve_conflict("merge");
 
-	if (file_exists(git_path("MERGE_HEAD"))) {
+	if (file_exists(git_path_merge_head())) {
 		/*
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
@@ -1226,7 +1226,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			die(_("You have not concluded your merge (MERGE_HEAD exists)."));
 	}
-	if (file_exists(git_path("CHERRY_PICK_HEAD"))) {
+	if (file_exists(git_path_cherry_pick_head())) {
 		if (advice_resolve_conflict)
 			die(_("You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\n"
 			    "Please, commit your changes before you merge."));
diff --git a/builtin/reset.c b/builtin/reset.c
index 4c08ddc..c503e75 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -36,7 +36,7 @@ static const char *reset_type_names[] = {
 
 static inline int is_merge(void)
 {
-	return !access(git_path("MERGE_HEAD"), F_OK);
+	return !access(git_path_merge_head(), F_OK);
 }
 
 static int reset_index(const unsigned char *sha1, int reset_type, int quiet)
diff --git a/cache.h b/cache.h
index 7a4fa90..402521e 100644
--- a/cache.h
+++ b/cache.h
@@ -736,6 +736,32 @@ extern char *git_pathdup_submodule(const char *path, const char *fmt, ...)
 extern void report_linked_checkout_garbage(void);
 
 /*
+ * You can define a static memoized git path like:
+ *
+ *    static GIT_PATH_FUNC(git_path_foo, "FOO");
+ *
+ * or use one of the global ones below.
+ */
+#define GIT_PATH_FUNC(func, filename) \
+	const char *func(void) \
+	{ \
+		static char *ret; \
+		if (!ret) \
+			ret = git_pathdup(filename); \
+		return ret; \
+	}
+
+const char *git_path_cherry_pick_head(void);
+const char *git_path_revert_head(void);
+const char *git_path_squash_msg(void);
+const char *git_path_merge_msg(void);
+const char *git_path_merge_rr(void);
+const char *git_path_merge_mode(void);
+const char *git_path_merge_head(void);
+const char *git_path_fetch_head(void);
+const char *git_path_shallow(void);
+
+/*
  * Return the name of the file in the local object database that would
  * be used to store a loose object with the specified sha1.  The
  * return value is a pointer to a statically allocated buffer that is
diff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c
index ee19166..a3eb19d 100644
--- a/contrib/examples/builtin-fetch--tool.c
+++ b/contrib/examples/builtin-fetch--tool.c
@@ -516,7 +516,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 
 		if (argc != 8)
 			return error("append-fetch-head takes 6 args");
-		filename = git_path("FETCH_HEAD");
+		filename = git_path_fetch_head();
 		fp = fopen(filename, "a");
 		if (!fp)
 			return error("cannot open %s: %s", filename, strerror(errno));
@@ -534,7 +534,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)
 
 		if (argc != 5)
 			return error("fetch-native-store takes 3 args");
-		filename = git_path("FETCH_HEAD");
+		filename = git_path_fetch_head();
 		fp = fopen(filename, "a");
 		if (!fp)
 			return error("cannot open %s: %s", filename, strerror(errno));
diff --git a/dir.c b/dir.c
index 1d42811..1ac8d5d 100644
--- a/dir.c
+++ b/dir.c
@@ -2185,6 +2185,8 @@ int remove_dir_recursively(struct strbuf *path, int flag)
 	return remove_dir_recurse(path, flag, NULL);
 }
 
+static GIT_PATH_FUNC(git_path_info_exclude, "info/exclude")
+
 void setup_standard_excludes(struct dir_struct *dir)
 {
 	const char *path;
@@ -2199,7 +2201,7 @@ void setup_standard_excludes(struct dir_struct *dir)
 					 dir->untracked ? &dir->ss_excludes_file : NULL);
 
 	/* per repository user preference */
-	path = git_path("info/exclude");
+	path = git_path_info_exclude();
 	if (!access_or_warn(path, R_OK, 0))
 		add_excludes_from_file_1(dir, path,
 					 dir->untracked ? &dir->ss_info_exclude : NULL);
diff --git a/fetch-pack.c b/fetch-pack.c
index a136772..820251a 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -948,7 +948,7 @@ static void update_shallow(struct fetch_pack_args *args,
 
 	if (args->depth > 0 && alternate_shallow_file) {
 		if (*alternate_shallow_file == '\0') { /* --unshallow */
-			unlink_or_warn(git_path("shallow"));
+			unlink_or_warn(git_path_shallow());
 			rollback_lock_file(&shallow_lock);
 		} else
 			commit_lock_file(&shallow_lock);
diff --git a/path.c b/path.c
index 94d7ec2..95acbaf 100644
--- a/path.c
+++ b/path.c
@@ -933,3 +933,13 @@ char *xdg_config_home(const char *filename)
 		return mkpathdup("%s/.config/git/%s", home, filename);
 	return NULL;
 }
+
+GIT_PATH_FUNC(git_path_cherry_pick_head, "CHERRY_PICK_HEAD")
+GIT_PATH_FUNC(git_path_revert_head, "REVERT_HEAD")
+GIT_PATH_FUNC(git_path_squash_msg, "SQUASH_MSG")
+GIT_PATH_FUNC(git_path_merge_msg, "MERGE_MSG")
+GIT_PATH_FUNC(git_path_merge_rr, "MERGE_RR")
+GIT_PATH_FUNC(git_path_merge_mode, "MERGE_MODE")
+GIT_PATH_FUNC(git_path_merge_head, "MERGE_HEAD")
+GIT_PATH_FUNC(git_path_fetch_head, "FETCH_HEAD")
+GIT_PATH_FUNC(git_path_shallow, "shallow")
diff --git a/rerere.c b/rerere.c
index 94aea9a..6a517aa 100644
--- a/rerere.c
+++ b/rerere.c
@@ -20,8 +20,6 @@ static int rerere_enabled = -1;
 /* automatically update cleanly resolved paths to the index */
 static int rerere_autoupdate;
 
-static char *merge_rr_path;
-
 const char *rerere_path(const char *hex, const char *file)
 {
 	return git_path("rr-cache/%s/%s", hex, file);
@@ -37,7 +35,7 @@ static void read_rr(struct string_list *rr)
 {
 	unsigned char sha1[20];
 	char buf[PATH_MAX];
-	FILE *in = fopen(merge_rr_path, "r");
+	FILE *in = fopen(git_path_merge_rr(), "r");
 	if (!in)
 		return;
 	while (fread(buf, 40, 1, in) == 1) {
@@ -577,21 +575,21 @@ static void git_rerere_config(void)
 	git_config(git_default_config, NULL);
 }
 
+static GIT_PATH_FUNC(git_path_rr_cache, "rr-cache")
+
 static int is_rerere_enabled(void)
 {
-	const char *rr_cache;
 	int rr_cache_exists;
 
 	if (!rerere_enabled)
 		return 0;
 
-	rr_cache = git_path("rr-cache");
-	rr_cache_exists = is_directory(rr_cache);
+	rr_cache_exists = is_directory(git_path_rr_cache());
 	if (rerere_enabled < 0)
 		return rr_cache_exists;
 
-	if (!rr_cache_exists && mkdir_in_gitdir(rr_cache))
-		die("Could not create directory %s", rr_cache);
+	if (!rr_cache_exists && mkdir_in_gitdir(git_path_rr_cache()))
+		die("Could not create directory %s", git_path_rr_cache());
 	return 1;
 }
 
@@ -605,8 +603,7 @@ int setup_rerere(struct string_list *merge_rr, int flags)
 
 	if (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))
 		rerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);
-	merge_rr_path = git_pathdup("MERGE_RR");
-	fd = hold_lock_file_for_update(&write_lock, merge_rr_path,
+	fd = hold_lock_file_for_update(&write_lock, git_path_merge_rr(),
 				       LOCK_DIE_ON_ERROR);
 	read_rr(merge_rr);
 	return fd;
@@ -741,5 +738,5 @@ void rerere_clear(struct string_list *merge_rr)
 		if (!has_rerere_resolution(name))
 			unlink_rr_item(name);
 	}
-	unlink_or_warn(git_path("MERGE_RR"));
+	unlink_or_warn(git_path_merge_rr());
 }
diff --git a/sequencer.c b/sequencer.c
index c4f4b7d..147adfa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -21,6 +21,11 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
+static GIT_PATH_FUNC(git_path_todo_file, SEQ_TODO_FILE)
+static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
+static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
+static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
+
 static int is_rfc2822_line(const char *buf, int len)
 {
 	int i;
@@ -186,7 +191,7 @@ static void print_advice(int show_hint, struct replay_opts *opts)
 		 * (typically rebase --interactive) wants to take care
 		 * of the commit itself so remove CHERRY_PICK_HEAD
 		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
+		unlink(git_path_cherry_pick_head());
 		return;
 	}
 
@@ -467,7 +472,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	struct commit *base, *next, *parent;
 	const char *base_label, *next_label;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
-	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 	int res, unborn = 0, allow;
 
@@ -537,8 +541,6 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	 * reverse of it if we are revert.
 	 */
 
-	defmsg = git_pathdup("MERGE_MSG");
-
 	if (opts->action == REPLAY_REVERT) {
 		base = commit;
 		base_label = msg.label;
@@ -585,12 +587,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
 		res = do_recursive_merge(base, next, base_label, next_label,
 					 head, &msgbuf, opts);
-		write_message(&msgbuf, defmsg);
+		write_message(&msgbuf, git_path_merge_msg());
 	} else {
 		struct commit_list *common = NULL;
 		struct commit_list *remotes = NULL;
 
-		write_message(&msgbuf, defmsg);
+		write_message(&msgbuf, git_path_merge_msg());
 
 		commit_list_insert(base, &common);
 		commit_list_insert(next, &remotes);
@@ -628,11 +630,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		goto leave;
 	}
 	if (!opts->no_commit)
-		res = run_git_commit(defmsg, opts, allow);
+		res = run_git_commit(git_path_merge_msg(), opts, allow);
 
 leave:
 	free_message(commit, &msg);
-	free(defmsg);
 
 	return res;
 }
@@ -756,24 +757,23 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
 static void read_populate_todo(struct commit_list **todo_list,
 			struct replay_opts *opts)
 {
-	const char *todo_file = git_path(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
 	int fd, res;
 
-	fd = open(todo_file, O_RDONLY);
+	fd = open(git_path_todo_file(), O_RDONLY);
 	if (fd < 0)
-		die_errno(_("Could not open %s"), todo_file);
+		die_errno(_("Could not open %s"), git_path_todo_file());
 	if (strbuf_read(&buf, fd, 0) < 0) {
 		close(fd);
 		strbuf_release(&buf);
-		die(_("Could not read %s."), todo_file);
+		die(_("Could not read %s."), git_path_todo_file());
 	}
 	close(fd);
 
 	res = parse_insn_buffer(buf.buf, todo_list, opts);
 	strbuf_release(&buf);
 	if (res)
-		die(_("Unusable instruction sheet: %s"), todo_file);
+		die(_("Unusable instruction sheet: %s"), git_path_todo_file());
 }
 
 static int populate_opts_cb(const char *key, const char *value, void *data)
@@ -813,12 +813,10 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 
 static void read_populate_opts(struct replay_opts **opts_ptr)
 {
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (!file_exists(opts_file))
+	if (!file_exists(git_path_opts_file()))
 		return;
-	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), opts_file);
+	if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts_ptr) < 0)
+		die(_("Malformed options sheet: %s"), git_path_opts_file());
 }
 
 static void walk_revs_populate_todo(struct commit_list **todo_list,
@@ -836,31 +834,29 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,
 
 static int create_seq_dir(void)
 {
-	const char *seq_dir = git_path(SEQ_DIR);
-
-	if (file_exists(seq_dir)) {
+	if (file_exists(git_path_seq_dir())) {
 		error(_("a cherry-pick or revert is already in progress"));
 		advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
 		return -1;
 	}
-	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory %s"), seq_dir);
+	else if (mkdir(git_path_seq_dir(), 0777) < 0)
+		die_errno(_("Could not create sequencer directory %s"),
+			  git_path_seq_dir());
 	return 0;
 }
 
 static void save_head(const char *head)
 {
-	const char *head_file = git_path(SEQ_HEAD_FILE);
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&head_lock, git_path_head_file(), LOCK_DIE_ON_ERROR);
 	strbuf_addf(&buf, "%s\n", head);
 	if (write_in_full(fd, buf.buf, buf.len) < 0)
-		die_errno(_("Could not write to %s"), head_file);
+		die_errno(_("Could not write to %s"), git_path_head_file());
 	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), head_file);
+		die(_("Error wrapping up %s."), git_path_head_file());
 }
 
 static int reset_for_rollback(const unsigned char *sha1)
@@ -877,8 +873,8 @@ static int rollback_single_pick(void)
 {
 	unsigned char head_sha1[20];
 
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
+	if (!file_exists(git_path_cherry_pick_head()) &&
+	    !file_exists(git_path_revert_head()))
 		return error(_("no cherry-pick or revert in progress"));
 	if (read_ref_full("HEAD", 0, head_sha1, NULL))
 		return error(_("cannot resolve HEAD"));
@@ -889,13 +885,11 @@ static int rollback_single_pick(void)
 
 static int sequencer_rollback(struct replay_opts *opts)
 {
-	const char *filename;
 	FILE *f;
 	unsigned char sha1[20];
 	struct strbuf buf = STRBUF_INIT;
 
-	filename = git_path(SEQ_HEAD_FILE);
-	f = fopen(filename, "r");
+	f = fopen(git_path_head_file(), "r");
 	if (!f && errno == ENOENT) {
 		/*
 		 * There is no multiple-cherry-pick in progress.
@@ -905,18 +899,18 @@ static int sequencer_rollback(struct replay_opts *opts)
 		return rollback_single_pick();
 	}
 	if (!f)
-		return error(_("cannot open %s: %s"), filename,
+		return error(_("cannot open %s: %s"), git_path_head_file(),
 						strerror(errno));
 	if (strbuf_getline(&buf, f, '\n')) {
-		error(_("cannot read %s: %s"), filename, ferror(f) ?
-			strerror(errno) : _("unexpected end of file"));
+		error(_("cannot read %s: %s"), git_path_head_file(),
+		      ferror(f) ?  strerror(errno) : _("unexpected end of file"));
 		fclose(f);
 		goto fail;
 	}
 	fclose(f);
 	if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
 		error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
-			filename);
+			git_path_head_file());
 		goto fail;
 	}
 	if (reset_for_rollback(sha1))
@@ -931,28 +925,27 @@ fail:
 
 static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
-	const char *todo_file = git_path(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+	fd = hold_lock_file_for_update(&todo_lock, git_path_todo_file(), LOCK_DIE_ON_ERROR);
 	if (format_todo(&buf, todo_list, opts) < 0)
-		die(_("Could not format %s."), todo_file);
+		die(_("Could not format %s."), git_path_todo_file());
 	if (write_in_full(fd, buf.buf, buf.len) < 0) {
 		strbuf_release(&buf);
-		die_errno(_("Could not write to %s"), todo_file);
+		die_errno(_("Could not write to %s"), git_path_todo_file());
 	}
 	if (commit_lock_file(&todo_lock) < 0) {
 		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), todo_file);
+		die(_("Error wrapping up %s."), git_path_todo_file());
 	}
 	strbuf_release(&buf);
 }
 
 static void save_opts(struct replay_opts *opts)
 {
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
+	const char *opts_file = git_path_opts_file();
 
 	if (opts->no_commit)
 		git_config_set_in_file(opts_file, "options.no-commit", "true");
@@ -1013,8 +1006,8 @@ static int continue_single_pick(void)
 {
 	const char *argv[] = { "commit", NULL };
 
-	if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
-	    !file_exists(git_path("REVERT_HEAD")))
+	if (!file_exists(git_path_cherry_pick_head()) &&
+	    !file_exists(git_path_revert_head()))
 		return error(_("no cherry-pick or revert in progress"));
 	return run_command_v_opt(argv, RUN_GIT_CMD);
 }
@@ -1023,14 +1016,14 @@ static int sequencer_continue(struct replay_opts *opts)
 {
 	struct commit_list *todo_list = NULL;
 
-	if (!file_exists(git_path(SEQ_TODO_FILE)))
+	if (!file_exists(git_path_todo_file()))
 		return continue_single_pick();
 	read_populate_opts(&opts);
 	read_populate_todo(&todo_list, opts);
 
 	/* Verify that the conflict has been resolved */
-	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
-	    file_exists(git_path("REVERT_HEAD"))) {
+	if (file_exists(git_path_cherry_pick_head()) ||
+	    file_exists(git_path_revert_head())) {
 		int ret = continue_single_pick();
 		if (ret)
 			return ret;
diff --git a/shallow.c b/shallow.c
index 257d811..4ded4a8 100644
--- a/shallow.c
+++ b/shallow.c
@@ -48,7 +48,7 @@ int is_repository_shallow(void)
 		return is_shallow;
 
 	if (!path)
-		path = git_path("shallow");
+		path = git_path_shallow();
 	/*
 	 * fetch-pack sets '--shallow-file ""' as an indicator that no
 	 * shallow file should be used. We could just open it and it
@@ -142,7 +142,7 @@ static void check_shallow_file_for_update(void)
 	if (is_shallow == -1)
 		die("BUG: shallow must be initialized by now");
 
-	if (!stat_validity_check(&shallow_stat, git_path("shallow")))
+	if (!stat_validity_check(&shallow_stat, git_path_shallow()))
 		die("shallow file has changed since we read it");
 }
 
@@ -261,7 +261,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,
 	struct strbuf sb = STRBUF_INIT;
 	int fd;
 
-	fd = hold_lock_file_for_update(shallow_lock, git_path("shallow"),
+	fd = hold_lock_file_for_update(shallow_lock, git_path_shallow(),
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update();
 	if (write_shallow_commits(&sb, 0, extra)) {
@@ -308,7 +308,7 @@ void prune_shallow(int show_only)
 		strbuf_release(&sb);
 		return;
 	}
-	fd = hold_lock_file_for_update(&shallow_lock, git_path("shallow"),
+	fd = hold_lock_file_for_update(&shallow_lock, git_path_shallow(),
 				       LOCK_DIE_ON_ERROR);
 	check_shallow_file_for_update();
 	if (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {
@@ -317,7 +317,7 @@ void prune_shallow(int show_only)
 				  shallow_lock.filename.buf);
 		commit_lock_file(&shallow_lock);
 	} else {
-		unlink(git_path("shallow"));
+		unlink(git_path_shallow());
 		rollback_lock_file(&shallow_lock);
 	}
 	strbuf_release(&sb);
diff --git a/wt-status.c b/wt-status.c
index 04e01e4..717fd48 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1171,7 +1171,7 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git rebase --abort\" to check out the original branch)"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path("MERGE_MSG"), &st)) {
+	} else if (state->rebase_in_progress || !stat(git_path_merge_msg(), &st)) {
 		print_rebase_state(s, state, color);
 		if (s->hints)
 			status_printf_ln(s, color,
@@ -1368,7 +1368,7 @@ void wt_status_get_state(struct wt_status_state *state,
 	struct stat st;
 	unsigned char sha1[20];
 
-	if (!stat(git_path("MERGE_HEAD"), &st)) {
+	if (!stat(git_path_merge_head(), &st)) {
 		state->merge_in_progress = 1;
 	} else if (!stat(git_path("rebase-apply"), &st)) {
 		if (!stat(git_path("rebase-apply/applying"), &st)) {
@@ -1387,7 +1387,7 @@ void wt_status_get_state(struct wt_status_state *state,
 			state->rebase_in_progress = 1;
 		state->branch = read_and_strip_branch("rebase-merge/head-name");
 		state->onto = read_and_strip_branch("rebase-merge/onto");
-	} else if (!stat(git_path("CHERRY_PICK_HEAD"), &st) &&
+	} else if (!stat(git_path_cherry_pick_head(), &st) &&
 			!get_sha1("CHERRY_PICK_HEAD", sha1)) {
 		state->cherry_pick_in_progress = 1;
 		hashcpy(state->cherry_pick_head_sha1, sha1);
@@ -1396,7 +1396,7 @@ void wt_status_get_state(struct wt_status_state *state,
 		state->bisect_in_progress = 1;
 		state->branch = read_and_strip_branch("BISECT_START");
 	}
-	if (!stat(git_path("REVERT_HEAD"), &st) &&
+	if (!stat(git_path_revert_head(), &st) &&
 	    !get_sha1("REVERT_HEAD", sha1)) {
 		state->revert_in_progress = 1;
 		hashcpy(state->revert_head_sha1, sha1);

-- 
2.5.0.414.g670f2a4
Michael Haggerty· Aug 10, 2015, 12:05 UTC · re: Jeff King · lore

Re: [PATCH 17/17] memoize common git-path "constant" files

On 08/10/2015 11:38 AM, Jeff King wrote:
Show 9 quoted lines
> [...]
> This patch introduces a series of functions to "memoize"
> these strings, which are essentially globals for the
> lifetime of the program. We compute the value once, take
> ownership of the buffer, and return the cached value for
> subsequent calls.  cache.h provides a helper macro for
> defining these functions as one-liners, and defines a few
> common ones for global use.
> [...]

I was wondering whether this memoization could interact badly with update_common_dir(). For example, if any of the memoized functions were called before git_common_dir is initialized, then the pre-git_common_dir value would continue to be used even if git_common_dir is changed afterwards. But I believe it is taboo to call git_path() before setup_git_env(), so I think this is not a problem.

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Jeff King· Aug 10, 2015, 12:30 UTC · re: Michael Haggerty · lore

Re: [PATCH 17/17] memoize common git-path "constant" files

On Mon, Aug 10, 2015 at 02:05:14PM +0200, Michael Haggerty wrote:
Show 6 quoted lines
> I was wondering whether this memoization could interact badly with
> update_common_dir(). For example, if any of the memoized functions were
> called before git_common_dir is initialized, then the pre-git_common_dir
> value would continue to be used even if git_common_dir is changed
> afterwards. But I believe it is taboo to call git_path() before
> setup_git_env(), so I think this is not a problem.

Calling git_path() actually ends up in setup_git_env(), because we lazily call it from get_git_dir(). So if somebody tries to tweak common_dir stuff too late, we could in theory cache a bogus value; but the real problem is generating the bogosity in the first place.

-Peff
Michael Haggerty· Aug 10, 2015, 12:06 UTC · re: Jeff King · lore

Re: [PATCH 0/17] removing questionable uses of git_path

On 08/10/2015 11:27 AM, Jeff King wrote:
Show 20 quoted lines
> [...] The problem is that git_path uses a static buffer that gets overwritten
> by subsequent calls. [...]
> 
>   [01/17]: cache.h: clarify documentation for git_path, et al
>   [02/17]: cache.h: complete set of git_path_submodule helpers
>   [03/17]: t5700: modernize style
>   [04/17]: add_to_alternates_file: don't add duplicate entries
>   [05/17]: remove hold_lock_file_for_append
>   [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases
>   [07/17]: prefer mkpathdup to mkpath in assignments
>   [08/17]: remote.c: drop extraneous local variable from migrate_file
>   [09/17]: refs.c: remove extra git_path calls from read_loose refs
>   [10/17]: path.c: drop git_path_submodule
>   [11/17]: refs.c: simplify strbufs in reflog setup and writing
>   [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log
>   [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic
>   [14/17]: refs.c: remove_empty_directories can take a strbuf
>   [15/17]: find_hook: keep our own static buffer
>   [16/17]: get_repo_path: refactor path-allocation
>   [17/17]: memoize common git-path "constant" files

I read through all of the patches (except for 03) and didn't see any problems. Thanks, Peff, for defusing some grenades :-)

Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
Junio C Hamano· Aug 10, 2015, 17:31 UTC · re: Jeff King · lore

Re: [PATCH 0/17] removing questionable uses of git_path

Jeff King <peff@peff.net> writes:
> The problem is that git_path uses a static buffer that gets overwritten
> by subsequent calls.

As the rotating static buffer pattern used in get_pathname() was modeled after sha1_to_hex(), we have the same issue there. They are troubles waiting to happen unless the callers are careful.

> producing a fairly tame-looking set of function calls. It's OK to pass
> the result of git_path() to a system call, or something that is a thin
> wrapper around one (e.g., strbuf_read_file).

That is a short and good rule to follow, but the problem is that not everybody has good taste to interpret the above rule and apply it with an eye toward maintainability X-<.

Show 21 quoted lines
> Along the way, there are a few cleanups (e.g., I polished off the recent
> hold_lock_file_for_append topic which was on the list, as it had some
> problematic calls).
>
>   [01/17]: cache.h: clarify documentation for git_path, et al
>   [02/17]: cache.h: complete set of git_path_submodule helpers
>   [03/17]: t5700: modernize style
>   [04/17]: add_to_alternates_file: don't add duplicate entries
>   [05/17]: remove hold_lock_file_for_append
>   [06/17]: prefer git_pathdup to git_path in some possibly-dangerous cases
>   [07/17]: prefer mkpathdup to mkpath in assignments
>   [08/17]: remote.c: drop extraneous local variable from migrate_file
>   [09/17]: refs.c: remove extra git_path calls from read_loose refs
>   [10/17]: path.c: drop git_path_submodule
>   [11/17]: refs.c: simplify strbufs in reflog setup and writing
>   [12/17]: refs.c: avoid repeated git_path calls in rename_tmp_log
>   [13/17]: refs.c: avoid git_path assignment in lock_ref_sha1_basic
>   [14/17]: refs.c: remove_empty_directories can take a strbuf
>   [15/17]: find_hook: keep our own static buffer
>   [16/17]: get_repo_path: refactor path-allocation
>   [17/17]: memoize common git-path "constant" files
Nice.  Thanks.
Jeff King· Aug 10, 2015, 17:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/17] removing questionable uses of git_path

On Mon, Aug 10, 2015 at 10:31:32AM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> Jeff King <peff@peff.net> writes:
> 
> > The problem is that git_path uses a static buffer that gets overwritten
> > by subsequent calls.
> 
> As the rotating static buffer pattern used in get_pathname() was
> modeled after sha1_to_hex(), we have the same issue there.  They are
> troubles waiting to happen unless the callers are careful.

Yeah, I think Michael mentioned that to me off-list. I don't _think_ sha1_to_hex is nearly such a problem, because we tend to store sha1s in their binary form. So sha1_to_hex is almost always an argument to fprintf() or similar.

Of course there are some exceptions. :) I notice we pass one return value to add_pending_object, which was almost certainly horribly broken before 31faeb2 started strdup'ing object_array names.

So certainly it would be nice to audit them all, but there are over 600 calls. Given the likelihood of finding a useful bug, I'm not sure it's the greatest use of developer time.

Show 7 quoted lines
> > producing a fairly tame-looking set of function calls. It's OK to pass
> > the result of git_path() to a system call, or something that is a thin
> > wrapper around one (e.g., strbuf_read_file).
> 
> That is a short and good rule to follow, but the problem is that not
> everybody has good taste to interpret the above rule and apply it with
> an eye toward maintainability X-<.

Yeah. Part of me wants to eradicate git_path entirely. My series takes out over half of the calls, but there are still close to 100, I think. I think it would make the code worse to convert all of them naively. We could provide format-aware wrappers for some filesystem functions, like:

  git_stat(&st, "%s", refname);
or something. That feels horribly coupled compared to:
  stat(git_path("%s", refname), &st);
but it makes it very clear what the memory ownership/lifetime rules are.

Anyway, part of my goal with the series was not just to clean up the existing suspicious spots, but to raise awareness of the issue. At least for me, I know it's something I will look for more carefully when reviewing patches. Once bitten, twice shy. :)

-Peff
Duy Nguyen· Aug 15, 2015, 09:05 UTC · re: Jeff King · lore

Re: [PATCH 0/17] removing questionable uses of git_path

On Mon, Aug 10, 2015 at 4:27 PM, Jeff King <peff@peff.net> wrote:
> The problem is that git_path uses a static buffer that gets overwritten
> by subsequent calls.

resolve_ref() was in the same boat, then we renamed it to resolve_ref_unsafe(), I believe with an intention to eventually kill it. But it lives on anyway. I wanted to suggest renaming git_path() to git_path_unsafe(). But I'm not sure if that will catch reviewer's eyes when new call sites are added. Probably not..

-- 
Duy

← back to recent threads