threads / patch / 45764

patch, 6 partsremoving more calls to git_path()

Subject: [PATCH 0/6] removing more calls to git_path()

## tl;dr

8 messages between Apr 20, 2017 and Apr 21, 2017. Diffs are folded; open one to read it.

replies: 7people: 2as markdown or json

Jeff King· Apr 20, 2017, 21:07 UTC · lore
After the discussion about the git_path()-related bug in:
  http://public-inbox.org/git/20170329080820.8084-1-chriscool@tuxfamily.org/

I looked over some of the calls. When I introduced git_pathdup() a few years ago I converted most of the dangerous git_path() calls. But there were still some left, and of course new ones have been added. This patch cleans up some of the low-hanging fruit.

There's probably more that could be done, but this is just what I happened to produce when looking through some of the cases the other day. I don't think any of these is fixing a triggerable bug; they can all be considered cleanup.

  [1/6]: bisect: add git_path_bisect_terms helper
  [2/6]: branch: add edit_description() helper
  [3/6]: use git_path_* helper functions
  [4/6]: replace xstrdup(git_path(...)) with git_pathdup(...)
  [5/6]: replace strbuf_addstr(git_path()) with git_path_buf()
  [6/6]: am: drop "dir" parameter from am_state_init
 bisect.c           |  3 ++-
 builtin/am.c       | 10 ++++------
 builtin/branch.c   |  6 +++---
 builtin/commit.c   |  6 +++---
 builtin/config.c   |  5 +++--
 builtin/pull.c     |  4 ++--
 builtin/worktree.c |  6 ++----
 fast-import.c      |  2 +-
 notes-merge.c      |  4 ++--
 sequencer.c        | 12 ++++++------
 10 files changed, 28 insertions(+), 30 deletions(-)
Jeff King· Apr 20, 2017, 21:08 UTC · re: Jeff King · lore

[PATCH 1/6] bisect: add git_path_bisect_terms helper

This avoids using the dangerous git_path(). Right now there's only one call site (because the writing half is still part of the shell script), but it may come in handy in the future as more of bisect is written in C. It also matches how we access the other BISECT_* files.

Signed-off-by: Jeff King <peff@peff.net>
---
 bisect.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to bisect.c +2 −1
diff --git a/bisect.c b/bisect.c
index 03af06c66..08c9fb726 100644
--- a/bisect.c
+++ b/bisect.c
@@ -432,6 +432,7 @@ static int read_bisect_refs(void)
 
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
+static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 
 static void read_bisect_paths(struct argv_array *array)
 {
@@ -906,7 +907,7 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
 void read_bisect_terms(const char **read_bad, const char **read_good)
 {
 	struct strbuf str = STRBUF_INIT;
-	const char *filename = git_path("BISECT_TERMS");
+	const char *filename = git_path_bisect_terms();
 	FILE *fp = fopen(filename, "r");
 
 	if (!fp) {
-- 
2.13.0.rc0.363.g8726c260e
Jeff King· Apr 20, 2017, 21:08 UTC · re: Jeff King · lore

[PATCH 2/6] branch: add edit_description() helper

Rather than have a variable with a short name that is fed to git_path(), let's add a helper function that returns the full path. This avoids the dangerous git_path() function.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to builtin/branch.c +3 −3
diff --git a/builtin/branch.c b/builtin/branch.c
index 0552c42ad..48a513a84 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -504,7 +504,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
 	strbuf_release(&newsection);
 }
 
-static const char edit_description[] = "BRANCH_DESCRIPTION";
+static GIT_PATH_FUNC(edit_description, "EDIT_DESCRIPTION")
 
 static int edit_branch_description(const char *branch_name)
 {
@@ -519,9 +519,9 @@ static int edit_branch_description(const char *branch_name)
 		      "  %s\n"
 		      "Lines starting with '%c' will be stripped.\n"),
 		    branch_name, comment_line_char);
-	write_file_buf(git_path(edit_description), buf.buf, buf.len);
+	write_file_buf(edit_description(), buf.buf, buf.len);
 	strbuf_reset(&buf);
-	if (launch_editor(git_path(edit_description), &buf, NULL)) {
+	if (launch_editor(edit_description(), &buf, NULL)) {
 		strbuf_release(&buf);
 		return -1;
 	}
-- 
2.13.0.rc0.363.g8726c260e
Jeff King· Apr 20, 2017, 21:08 UTC · re: Jeff King · lore

[PATCH 3/6] use git_path_* helper functions

Long ago we added functions like git_path_merge_msg() to replace the more dangerous git_path("MERGE_MSG"). Over time some new calls to the latter have crept it. Let's convert them to use the safer form.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/commit.c |  6 +++---
 builtin/pull.c   |  4 ++--
 sequencer.c      | 12 ++++++------
 3 files changed, 11 insertions(+), 11 deletions(-)
Show changes to 3 files +11 −11

builtin/commit.c, builtin/pull.c, sequencer.c

diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc51..98927d962 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -821,9 +821,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 					"If this is not correct, please remove the file\n"
 					"	%s\n"
 					"and try again.\n"),
-				git_path(whence == FROM_MERGE
-					 ? "MERGE_HEAD"
-					 : "CHERRY_PICK_HEAD"));
+				whence == FROM_MERGE ?
+					git_path_merge_head() :
+					git_path_cherry_pick_head());
 		}
 
 		fprintf(s->fp, "\n");
diff --git a/builtin/pull.c b/builtin/pull.c
index d8aa26d8a..dd1a4a94e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -332,7 +332,7 @@ static int git_pull_config(const char *var, const char *value, void *cb)
  */
 static void get_merge_heads(struct oid_array *merge_heads)
 {
-	const char *filename = git_path("FETCH_HEAD");
+	const char *filename = git_path_fetch_head();
 	FILE *fp;
 	struct strbuf sb = STRBUF_INIT;
 	struct object_id oid;
@@ -791,7 +791,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (read_cache_unmerged())
 		die_resolve_conflict("pull");
 
-	if (file_exists(git_path("MERGE_HEAD")))
+	if (file_exists(git_path_merge_head()))
 		die_conclude_merge();
 
 	if (get_oid("HEAD", &orig_head))
diff --git a/sequencer.c b/sequencer.c
index 77afecaeb..130cc868e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1065,12 +1065,12 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,
 			flags |= CLEANUP_MSG;
 			msg_file = rebase_path_fixup_msg();
 		} else {
-			const char *dest = git_path("SQUASH_MSG");
+			const char *dest = git_path_squash_msg();
 			unlink(dest);
 			if (copy_file(dest, rebase_path_squash_msg(), 0666))
 				return error(_("could not rename '%s' to '%s'"),
 					     rebase_path_squash_msg(), dest);
-			unlink(git_path("MERGE_MSG"));
+			unlink(git_path_merge_msg());
 			msg_file = dest;
 			flags |= EDIT_MSG;
 		}
@@ -1820,10 +1820,10 @@ static int error_failed_squash(struct commit *commit,
 		return error(_("could not rename '%s' to '%s'"),
 			rebase_path_squash_msg(), rebase_path_message());
 	unlink(rebase_path_fixup_msg());
-	unlink(git_path("MERGE_MSG"));
-	if (copy_file(git_path("MERGE_MSG"), rebase_path_message(), 0666))
+	unlink(git_path_merge_msg());
+	if (copy_file(git_path_merge_msg(), rebase_path_message(), 0666))
 		return error(_("could not copy '%s' to '%s'"),
-			     rebase_path_message(), git_path("MERGE_MSG"));
+			     rebase_path_message(), git_path_merge_msg());
 	return error_with_patch(commit, subject, subject_len, opts, 1, 0);
 }
 
@@ -2167,7 +2167,7 @@ static int commit_staged_changes(struct replay_opts *opts)
 	if (has_unstaged_changes(1))
 		return error(_("cannot rebase: You have unstaged changes."));
 	if (!has_uncommitted_changes(0)) {
-		const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+		const char *cherry_pick_head = git_path_cherry_pick_head();
 
 		if (file_exists(cherry_pick_head) && unlink(cherry_pick_head))
 			return error(_("could not remove CHERRY_PICK_HEAD"));
-- 
2.13.0.rc0.363.g8726c260e
Jeff King· Apr 20, 2017, 21:09 UTC · re: Jeff King · lore

[PATCH 4/6] replace xstrdup(git_path(...)) with git_pathdup(...)

It's more efficient to use git_pathdup(), as it skips an extra copy of the path. And by removing some calls to git_path(), it makes it easier to audit for dangerous uses.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/config.c | 5 +++--
 fast-import.c    | 2 +-
 2 files changed, 4 insertions(+), 3 deletions(-)
Show changes to 2 files +4 −3

builtin/config.c, fast-import.c

diff --git a/builtin/config.c b/builtin/config.c
index 4f49a0edb..3407a8b87 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -597,8 +597,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 		if (given_config_source.blob)
 			die("editing blobs is not supported");
 		git_config(git_default_config, NULL);
-		config_file = xstrdup(given_config_source.file ?
-				      given_config_source.file : git_path("config"));
+		config_file = given_config_source.file ?
+				xstrdup(given_config_source.file) :
+				git_pathdup("config");
 		if (use_global_config) {
 			int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
 			if (fd >= 0) {
diff --git a/fast-import.c b/fast-import.c
index 1ea3a0860..cf58f875b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3202,7 +3202,7 @@ static char* make_fast_import_path(const char *path)
 {
 	if (!relative_marks_paths || is_absolute_path(path))
 		return xstrdup(path);
-	return xstrdup(git_path("info/fast-import/%s", path));
+	return git_pathdup("info/fast-import/%s", path);
 }
 
 static void option_import_marks(const char *marks,
-- 
2.13.0.rc0.363.g8726c260e
Jeff King· Apr 20, 2017, 21:09 UTC · re: Jeff King · lore

[PATCH 5/6] replace strbuf_addstr(git_path()) with git_path_buf()

Writing directly into the strbuf avoids a useless copy of the data, and dropping calls to git_path() makes it easier to audit for dangerous calls.

Note that git_path() does an implicit strbuf_reset(), but in each of these cases we were either already doing that reset, or writing into a fresh strbuf anyway.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 6 ++----
 notes-merge.c      | 4 ++--
 2 files changed, 4 insertions(+), 6 deletions(-)
Show changes to 2 files +4 −6

builtin/worktree.c, notes-merge.c

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9993ded41..57caa0855 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -106,8 +106,7 @@ static void prune_worktrees(void)
 			printf("%s\n", reason.buf);
 		if (show_only)
 			continue;
-		strbuf_reset(&path);
-		strbuf_addstr(&path, git_path("worktrees/%s", d->d_name));
+		git_path_buf(&path, "worktrees/%s", d->d_name);
 		ret = remove_dir_recursively(&path, 0);
 		if (ret < 0 && errno == ENOTDIR)
 			ret = unlink(path.buf);
@@ -215,8 +214,7 @@ static int add_worktree(const char *path, const char *refname,
 	}
 
 	name = worktree_basename(path, &len);
-	strbuf_addstr(&sb_repo,
-		      git_path("worktrees/%.*s", (int)(path + len - name), name));
+	git_path_buf(&sb_repo, "worktrees/%.*s", (int)(path + len - name), name);
 	len = sb_repo.len;
 	if (safe_create_leading_directories_const(sb_repo.buf))
 		die_errno(_("could not create leading directories of '%s'"),
diff --git a/notes-merge.c b/notes-merge.c
index 5998605ac..32caaaff7 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -676,7 +676,7 @@ int notes_merge_commit(struct notes_merge_options *o,
 	const char *msg = strstr(buffer, "\n\n");
 	int baselen;
 
-	strbuf_addstr(&path, git_path(NOTES_MERGE_WORKTREE));
+	git_path_buf(&path, NOTES_MERGE_WORKTREE);
 	if (o->verbosity >= 3)
 		printf("Committing notes in notes merge worktree at %s\n",
 			path.buf);
@@ -741,7 +741,7 @@ int notes_merge_abort(struct notes_merge_options *o)
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 
-	strbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));
+	git_path_buf(&buf, NOTES_MERGE_WORKTREE);
 	if (o->verbosity >= 3)
 		printf("Removing notes merge worktree at %s/*\n", buf.buf);
 	ret = remove_dir_recursively(&buf, REMOVE_DIR_KEEP_TOPLEVEL);
-- 
2.13.0.rc0.363.g8726c260e
Jeff King· Apr 20, 2017, 21:09 UTC · re: Jeff King · lore

[PATCH 6/6] am: drop "dir" parameter from am_state_init

The only caller of this function passes in a static buffer returned from git_path(). This looks dangerous at first glance, but turns out to be OK because the first thing we do is xstrdup() the result.

Let's turn this into a git_pathdup(). That's slightly more efficient (no extra copy), and makes it easier to audit for dangerous git_path() invocations.

Since there's only a single caller, let's just set this default path inside the init function. That makes the memory ownership clear.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)
Show changes to builtin/am.c +4 −6
diff --git a/builtin/am.c b/builtin/am.c
index f7a7a971f..97849d4dc 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -134,17 +134,15 @@ struct am_state {
 };
 
 /**
- * Initializes am_state with the default values. The state directory is set to
- * dir.
+ * Initializes am_state with the default values.
  */
-static void am_state_init(struct am_state *state, const char *dir)
+static void am_state_init(struct am_state *state)
 {
 	int gpgsign;
 
 	memset(state, 0, sizeof(*state));
 
-	assert(dir);
-	state->dir = xstrdup(dir);
+	state->dir = git_pathdup("rebase-apply");
 
 	state->prec = 4;
 
@@ -2322,7 +2320,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 
 	git_config(git_am_config, NULL);
 
-	am_state_init(&state, git_path("rebase-apply"));
+	am_state_init(&state);
 
 	in_progress = am_in_progress(&state);
 	if (in_progress)
-- 
2.13.0.rc0.363.g8726c260e

← back to recent threads