[PATCH 0/5] Sequencer: working around historical mistakes

[PATCH 0/5] Sequencer: working around historical mistakes

[Ramkumar Ramachandra]

Hi,

As described in the discussion following $gmane/179304/focus=179383,
we have decided to handle historical hacks in the sequencer itself.
This series that follows is one step in the right direction.

- Part 1/5 makes the gigantic move required to create the sequencer.
If you need an excuse to celebrate, wait till this gets merged :)
- Part 5/5 can be considered as the "ultimate objective" of the
series.  I first wrote this part, and then wrote the other parts to
make tests pass.
- Parts 3/5 and 4/5 are ugly!  Causes heartburn.

Immediate shortcomings of this iteration:
1. No tests yet.  I want to see if it's possible to make this less
ugly first.
2. This series depends on rr/revert-cherry-pick, but doesn't apply to
the current 'next'- sorry, rebasing is a massive pita due to 1/5.

Thanks for reading.

-- Ram

Ramkumar Ramachandra (5):
  sequencer: factor code out of revert builtin
  sequencer: remove CHERRY_PICK_HEAD with sequencer state
  sequencer: sequencer state is useless without todo
  sequencer: handle single commit pick separately
  sequencer: revert d3f4628e

 builtin/revert.c                |  821 +--------------------------------------
 sequencer.c                     |  832 ++++++++++++++++++++++++++++++++++++++-
 sequencer.h                     |   26 ++
 t/t3510-cherry-pick-sequence.sh |   24 --
 4 files changed, 847 insertions(+), 856 deletions(-)

-- 
1.7.6.351.gb35ac.dirty

[PATCH 1/5] sequencer: factor code out of revert builtin

[Ramkumar Ramachandra]

Start building the generalized sequencer by moving code from revert.c
into sequencer.c and sequencer.h.  Make the builtin responsible only
for command-line parsing, and expose a new sequencer_pick_revisions()
to do the actual work of sequencing commits.

This is intended to be almost a pure code movement patch with no
functional changes.  Check with:

  $ git blame -s -CCC HEAD^..HEAD -- sequencer.c | grep -C3 '^[^^]'

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/revert.c |  821 +-----------------------------------------------------
 sequencer.c      |  802 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 sequencer.h      |   26 ++
 3 files changed, 828 insertions(+), 821 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index df9459b..c272920 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
 #include "cache.h"
 #include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
 #include "parse-options.h"
-#include "cache-tree.h"
 #include "diff.h"
 #include "revision.h"
 #include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
 #include "sequencer.h"
 
 /*
@@ -39,40 +29,11 @@ static const char * const cherry_pick_usage[] = {
 	NULL
 };
 
-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
-
-struct replay_opts {
-	enum replay_action action;
-	enum replay_subcommand subcommand;
-
-	/* Boolean options */
-	int edit;
-	int record_origin;
-	int no_commit;
-	int signoff;
-	int allow_ff;
-	int allow_rerere_auto;
-
-	int mainline;
-
-	/* Merge strategy */
-	const char *strategy;
-	const char **xopts;
-	size_t xopts_nr, xopts_alloc;
-
-	/* Only used by REPLAY_NONE */
-	struct rev_info *revs;
-};
-
-#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-
 static const char *action_name(const struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
 }
 
-static char *get_encoding(const char *message);
-
 static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
 {
 	return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;
@@ -222,784 +183,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		usage_with_options(usage_str, options);
 }
 
-struct commit_message {
-	char *parent_label;
-	const char *label;
-	const char *subject;
-	char *reencoded_message;
-	const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
-	const char *encoding;
-	const char *abbrev, *subject;
-	int abbrev_len, subject_len;
-	char *q;
-
-	if (!commit->buffer)
-		return -1;
-	encoding = get_encoding(commit->buffer);
-	if (!encoding)
-		encoding = "UTF-8";
-	if (!git_commit_encoding)
-		git_commit_encoding = "UTF-8";
-
-	out->reencoded_message = NULL;
-	out->message = commit->buffer;
-	if (strcmp(encoding, git_commit_encoding))
-		out->reencoded_message = reencode_string(commit->buffer,
-					git_commit_encoding, encoding);
-	if (out->reencoded_message)
-		out->message = out->reencoded_message;
-
-	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
-	abbrev_len = strlen(abbrev);
-
-	subject_len = find_commit_subject(out->message, &subject);
-
-	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
-			      strlen("... ") + subject_len + 1);
-	q = out->parent_label;
-	q = mempcpy(q, "parent of ", strlen("parent of "));
-	out->label = q;
-	q = mempcpy(q, abbrev, abbrev_len);
-	q = mempcpy(q, "... ", strlen("... "));
-	out->subject = q;
-	q = mempcpy(q, subject, subject_len);
-	*q = '\0';
-	return 0;
-}
-
-static void free_message(struct commit_message *msg)
-{
-	free(msg->parent_label);
-	free(msg->reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
-	const char *p = message, *eol;
-
-	while (*p && *p != '\n') {
-		for (eol = p + 1; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
-		if (!prefixcmp(p, "encoding ")) {
-			char *result = xmalloc(eol - 8 - p);
-			strlcpy(result, p + 9, eol - 8 - p);
-			return result;
-		}
-		p = eol;
-		if (*p == '\n')
-			p++;
-	}
-	return NULL;
-}
-
-static void write_cherry_pick_head(struct commit *commit)
-{
-	int fd;
-	struct strbuf buf = STRBUF_INIT;
-
-	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
-	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("CHERRY_PICK_HEAD"));
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
-	strbuf_release(&buf);
-}
-
-static void print_advice(int show_hint)
-{
-	char *msg = getenv("GIT_CHERRY_PICK_HELP");
-
-	if (msg) {
-		fprintf(stderr, "%s\n", msg);
-		/*
-		 * A conflict has occured but the porcelain
-		 * (typically rebase --interactive) wants to take care
-		 * of the commit itself so remove CHERRY_PICK_HEAD
-		 */
-		unlink(git_path("CHERRY_PICK_HEAD"));
-		return;
-	}
-
-	if (show_hint) {
-		advise("after resolving the conflicts, mark the corrected paths");
-		advise("with 'git add <paths>' or 'git rm <paths>'");
-		advise("and commit the result with 'git commit'");
-	}
-}
-
-static void write_message(struct strbuf *msgbuf, const char *filename)
-{
-	static struct lock_file msg_file;
-
-	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
-					       LOCK_DIE_ON_ERROR);
-	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
-		die_errno(_("Could not write to %s."), filename);
-	strbuf_release(msgbuf);
-	if (commit_lock_file(&msg_file) < 0)
-		die(_("Error wrapping up %s"), filename);
-}
-
-static struct tree *empty_tree(void)
-{
-	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
-}
-
-static int error_dirty_index(struct replay_opts *opts)
-{
-	if (read_cache_unmerged())
-		return error_resolve_conflict(action_name(opts));
-
-	/* Different translation strings for cherry-pick and revert */
-	if (opts->action == REPLAY_PICK)
-		error(_("Your local changes would be overwritten by cherry-pick."));
-	else
-		error(_("Your local changes would be overwritten by revert."));
-
-	if (advice_commit_before_merge)
-		advise(_("Commit your changes or stash them to proceed."));
-	return -1;
-}
-
-static int fast_forward_to(const unsigned char *to, const unsigned char *from)
-{
-	struct ref_lock *ref_lock;
-
-	read_cache();
-	if (checkout_fast_forward(from, to))
-		exit(1); /* the callee should have complained already */
-	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
-	return write_ref_sha1(ref_lock, to, "cherry-pick");
-}
-
-static int do_recursive_merge(struct commit *base, struct commit *next,
-			      const char *base_label, const char *next_label,
-			      unsigned char *head, struct strbuf *msgbuf,
-			      struct replay_opts *opts)
-{
-	struct merge_options o;
-	struct tree *result, *next_tree, *base_tree, *head_tree;
-	int clean, index_fd;
-	const char **xopt;
-	static struct lock_file index_lock;
-
-	index_fd = hold_locked_index(&index_lock, 1);
-
-	read_cache();
-
-	init_merge_options(&o);
-	o.ancestor = base ? base_label : "(empty tree)";
-	o.branch1 = "HEAD";
-	o.branch2 = next ? next_label : "(empty tree)";
-
-	head_tree = parse_tree_indirect(head);
-	next_tree = next ? next->tree : empty_tree();
-	base_tree = base ? base->tree : empty_tree();
-
-	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
-		parse_merge_opt(&o, *xopt);
-
-	clean = merge_trees(&o,
-			    head_tree,
-			    next_tree, base_tree, &result);
-
-	if (active_cache_changed &&
-	    (write_cache(index_fd, active_cache, active_nr) ||
-	     commit_locked_index(&index_lock)))
-		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
-		die(_("%s: Unable to write new index file"), action_name(opts));
-	rollback_lock_file(&index_lock);
-
-	if (!clean) {
-		int i;
-		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
-		for (i = 0; i < active_nr;) {
-			struct cache_entry *ce = active_cache[i++];
-			if (ce_stage(ce)) {
-				strbuf_addch(msgbuf, '\t');
-				strbuf_addstr(msgbuf, ce->name);
-				strbuf_addch(msgbuf, '\n');
-				while (i < active_nr && !strcmp(ce->name,
-						active_cache[i]->name))
-					i++;
-			}
-		}
-	}
-
-	return !clean;
-}
-
-/*
- * If we are cherry-pick, and if the merge did not result in
- * hand-editing, we will hit this commit and inherit the original
- * author date and name.
- * If we are revert, or if our cherry-pick results in a hand merge,
- * we had better say that the current user is responsible for that.
- */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
-{
-	/* 6 is max possible length of our args array including NULL */
-	const char *args[6];
-	int i = 0;
-
-	args[i++] = "commit";
-	args[i++] = "-n";
-	if (opts->signoff)
-		args[i++] = "-s";
-	if (!opts->edit) {
-		args[i++] = "-F";
-		args[i++] = defmsg;
-	}
-	args[i] = NULL;
-
-	return run_command_v_opt(args, RUN_GIT_CMD);
-}
-
-static int do_pick_commit(struct commit *commit, enum replay_action action,
-			struct replay_opts *opts)
-{
-	unsigned char head[20];
-	struct commit *base, *next, *parent;
-	const char *base_label, *next_label;
-	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
-	char *defmsg = NULL;
-	struct strbuf msgbuf = STRBUF_INIT;
-	int res;
-
-	if (opts->no_commit) {
-		/*
-		 * We do not intend to commit immediately.  We just want to
-		 * merge the differences in, so let's compute the tree
-		 * that represents the "current" state for merge-recursive
-		 * to work on.
-		 */
-		if (write_cache_as_tree(head, 0, NULL))
-			die (_("Your index file is unmerged."));
-	} else {
-		if (get_sha1("HEAD", head))
-			return error(_("You do not have a valid HEAD"));
-		if (index_differs_from("HEAD", 0))
-			return error_dirty_index(opts);
-	}
-	discard_cache();
-
-	if (!commit->parents) {
-		parent = NULL;
-	}
-	else if (commit->parents->next) {
-		/* Reverting or cherry-picking a merge commit */
-		int cnt;
-		struct commit_list *p;
-
-		if (!opts->mainline)
-			return error(_("Commit %s is a merge but no -m option was given."),
-				sha1_to_hex(commit->object.sha1));
-
-		for (cnt = 1, p = commit->parents;
-		     cnt != opts->mainline && p;
-		     cnt++)
-			p = p->next;
-		if (cnt != opts->mainline || !p)
-			return error(_("Commit %s does not have parent %d"),
-				sha1_to_hex(commit->object.sha1), opts->mainline);
-		parent = p->item;
-	} else if (0 < opts->mainline)
-		return error(_("Mainline was specified but commit %s is not a merge."),
-			sha1_to_hex(commit->object.sha1));
-	else
-		parent = commit->parents->item;
-
-	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
-		return fast_forward_to(commit->object.sha1, head);
-
-	if (parent && parse_commit(parent) < 0)
-		/* TRANSLATORS: The first %s will be "revert" or
-		   "cherry-pick", the second %s a SHA1 */
-		return error(_("%s: cannot parse parent commit %s"),
-			action_name(opts), sha1_to_hex(parent->object.sha1));
-
-	if (get_message(commit, &msg) != 0)
-		return error(_("Cannot get commit message for %s"),
-			sha1_to_hex(commit->object.sha1));
-
-	/*
-	 * "commit" is an existing commit.  We would want to apply
-	 * the difference it introduces since its first parent "prev"
-	 * on top of the current HEAD if we are cherry-pick.  Or the
-	 * reverse of it if we are revert.
-	 */
-
-	defmsg = git_pathdup("MERGE_MSG");
-
-	if (action == REPLAY_REVERT) {
-		base = commit;
-		base_label = msg.label;
-		next = parent;
-		next_label = msg.parent_label;
-		strbuf_addstr(&msgbuf, "Revert \"");
-		strbuf_addstr(&msgbuf, msg.subject);
-		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
-		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-
-		if (commit->parents && commit->parents->next) {
-			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
-		}
-		strbuf_addstr(&msgbuf, ".\n");
-	} else {
-		const char *p;
-
-		base = parent;
-		base_label = msg.parent_label;
-		next = commit;
-		next_label = msg.label;
-
-		/*
-		 * Append the commit log message to msgbuf; it starts
-		 * after the tree, parent, author, committer
-		 * information followed by "\n\n".
-		 */
-		p = strstr(msg.message, "\n\n");
-		if (p) {
-			p += 2;
-			strbuf_addstr(&msgbuf, p);
-		}
-
-		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
-			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
-	}
-
-	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
-		res = do_recursive_merge(base, next, base_label, next_label,
-					 head, &msgbuf, opts);
-		write_message(&msgbuf, defmsg);
-	} else {
-		struct commit_list *common = NULL;
-		struct commit_list *remotes = NULL;
-
-		write_message(&msgbuf, defmsg);
-
-		commit_list_insert(base, &common);
-		commit_list_insert(next, &remotes);
-		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
-					common, sha1_to_hex(head), remotes);
-		free_commit_list(common);
-		free_commit_list(remotes);
-	}
-
-	/*
-	 * If the merge was clean or if it failed due to conflict, we write
-	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
-	 * However, if the merge did not even start, then we don't want to
-	 * write it at all.
-	 */
-	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
-		write_cherry_pick_head(commit);
-
-	if (res) {
-		error(action == REPLAY_REVERT
-		      ? _("could not revert %s... %s")
-		      : _("could not apply %s... %s"),
-		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
-		      msg.subject);
-		print_advice(res == 1);
-		rerere(opts->allow_rerere_auto);
-	} else {
-		if (!opts->no_commit)
-			res = run_git_commit(defmsg, opts);
-	}
-
-	free_message(&msg);
-	free(defmsg);
-
-	return res;
-}
-
-static void prepare_revs(struct replay_opts *opts)
-{
-	if (opts->action != REPLAY_REVERT)
-		opts->revs->reverse ^= 1;
-
-	if (prepare_revision_walk(opts->revs))
-		die(_("revision walk setup failed"));
-
-	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
-}
-
-static void read_and_refresh_cache(struct replay_opts *opts)
-{
-	static struct lock_file index_lock;
-	int index_fd = hold_locked_index(&index_lock, 0);
-	if (read_index_preload(&the_index, NULL) < 0)
-		die(_("git %s: failed to read the index"), action_name(opts));
-	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
-	if (the_index.cache_changed) {
-		if (write_index(&the_index, index_fd) ||
-		    commit_locked_index(&index_lock))
-			die(_("git %s: failed to refresh the index"), action_name(opts));
-	}
-	rollback_lock_file(&index_lock);
-}
-
-/*
- * Append a commit to the end of the commit_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty commit_list, and is updated to point to the "next" field of
- * the last item on the list as new commits are appended.
- *
- * Usage example:
- *
- *     struct commit_list *list;
- *     struct commit_list **next = &list;
- *
- *     next = commit_list_append(c1, next);
- *     next = commit_list_append(c2, next);
- *     assert(commit_list_count(list) == 2);
- *     return list;
- */
-static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
-						struct commit *operand,
-						struct replay_insn_list **next)
-{
-	struct replay_insn_list *new = xmalloc(sizeof(*new));
-	new->action = action;
-	new->operand = operand;
-	*next = new;
-	new->next = NULL;
-	return &new->next;
-}
-
-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
-{
-	struct replay_insn_list *cur;
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		const char *sha1_abbrev, *action_str, *subject;
-		int subject_len;
-
-		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
-		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
-		subject_len = find_commit_subject(cur->operand->buffer, &subject);
-		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
-			subject_len, subject);
-	}
-	return 0;
-}
-
-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
-{
-	unsigned char commit_sha1[20];
-	char *end_of_object_name;
-	int saved, status;
-
-	if (!prefixcmp(bol, "pick ")) {
-		item->action = REPLAY_PICK;
-		bol += strlen("pick ");
-	} else if (!prefixcmp(bol, "revert ")) {
-		item->action = REPLAY_REVERT;
-		bol += strlen("revert ");
-	} else {
-		size_t len = strchrnul(bol, '\n') - bol;
-		if (len > 255)
-			len = 255;
-		return error(_("Unrecognized action: %.*s"), (int)len, bol);
-	}
-
-	end_of_object_name = bol + strcspn(bol, " \n");
-	saved = *end_of_object_name;
-	*end_of_object_name = '\0';
-	status = get_sha1(bol, commit_sha1);
-	*end_of_object_name = saved;
-
-	if (status < 0)
-		return error(_("Malformed object name: %s"), bol);
-
-	item->operand = lookup_commit_reference(commit_sha1);
-	if (!item->operand)
-		return error(_("Not a valid commit: %s"), bol);
-
-	item->next = NULL;
-	return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
-	struct replay_insn_list **next = todo_list;
-	struct replay_insn_list item = {0, NULL, NULL};
-	char *p = buf;
-	int i;
-
-	for (i = 1; *p; i++) {
-		char *eol = strchrnul(p, '\n');
-		if (parse_insn_line(p, eol, &item) < 0)
-			return error(_("on line %d."), i);
-		next = replay_insn_list_append(item.action, item.operand, next);
-		p = *eol ? eol + 1 : eol;
-	}
-	if (!*todo_list)
-		return error(_("No commits parsed."));
-	return 0;
-}
-
-static void read_populate_todo(struct replay_insn_list **todo_list)
-{
-	const char *todo_file = git_path(SEQ_TODO_FILE);
-	struct strbuf buf = STRBUF_INIT;
-	int fd, res;
-
-	fd = open(todo_file, O_RDONLY);
-	if (fd < 0)
-		die_errno(_("Could not open %s."), todo_file);
-	if (strbuf_read(&buf, fd, 0) < 0) {
-		close(fd);
-		strbuf_release(&buf);
-		die(_("Could not read %s."), todo_file);
-	}
-	close(fd);
-
-	res = parse_insn_buffer(buf.buf, todo_list);
-	strbuf_release(&buf);
-	if (res)
-		die(_("Unusable instruction sheet: %s"), todo_file);
-}
-
-static int populate_opts_cb(const char *key, const char *value, void *data)
-{
-	struct replay_opts *opts = data;
-	int error_flag = 1;
-
-	if (!value)
-		error_flag = 0;
-	else if (!strcmp(key, "options.no-commit"))
-		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.edit"))
-		opts->edit = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.signoff"))
-		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.record-origin"))
-		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.allow-ff"))
-		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
-	else if (!strcmp(key, "options.mainline"))
-		opts->mainline = git_config_int(key, value);
-	else if (!strcmp(key, "options.strategy"))
-		git_config_string(&opts->strategy, key, value);
-	else if (!strcmp(key, "options.strategy-option")) {
-		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
-		opts->xopts[opts->xopts_nr++] = xstrdup(value);
-	} else
-		return error(_("Invalid key: %s"), key);
-
-	if (!error_flag)
-		return error(_("Invalid value for %s: %s"), key, value);
-
-	return 0;
-}
-
-static void read_populate_opts(struct replay_opts **opts_ptr)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (!file_exists(opts_file))
-		return;
-	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
-		die(_("Malformed options sheet: %s"), opts_file);
-}
-
-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
-				struct replay_opts *opts)
-{
-	struct commit *commit;
-	struct replay_insn_list **next;
-
-	prepare_revs(opts);
-
-	next = todo_list;
-	while ((commit = get_revision(opts->revs)))
-		next = replay_insn_list_append(opts->action, commit, next);
-}
-
-static int create_seq_dir(void)
-{
-	const char *seq_dir = git_path(SEQ_DIR);
-
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
-		die_errno(_("Could not create sequencer directory '%s'."), 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);
-	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);
-	if (commit_lock_file(&head_lock) < 0)
-		die(_("Error wrapping up %s."), head_file);
-}
-
-static void save_todo(struct replay_insn_list *todo_list)
-{
-	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);
-	if (format_todo(&buf, todo_list) < 0)
-		die(_("Could not format %s."), todo_file);
-	if (write_in_full(fd, buf.buf, buf.len) < 0) {
-		strbuf_release(&buf);
-		die_errno(_("Could not write to %s."), todo_file);
-	}
-	if (commit_lock_file(&todo_lock) < 0) {
-		strbuf_release(&buf);
-		die(_("Error wrapping up %s."), todo_file);
-	}
-	strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
-
-	if (opts->no_commit)
-		git_config_set_in_file(opts_file, "options.no-commit", "true");
-	if (opts->edit)
-		git_config_set_in_file(opts_file, "options.edit", "true");
-	if (opts->signoff)
-		git_config_set_in_file(opts_file, "options.signoff", "true");
-	if (opts->record_origin)
-		git_config_set_in_file(opts_file, "options.record-origin", "true");
-	if (opts->allow_ff)
-		git_config_set_in_file(opts_file, "options.allow-ff", "true");
-	if (opts->mainline) {
-		struct strbuf buf = STRBUF_INIT;
-		strbuf_addf(&buf, "%d", opts->mainline);
-		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
-		strbuf_release(&buf);
-	}
-	if (opts->strategy)
-		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
-	if (opts->xopts) {
-		int i;
-		for (i = 0; i < opts->xopts_nr; i++)
-			git_config_set_multivar_in_file(opts_file,
-							"options.strategy-option",
-							opts->xopts[i], "^$", 0);
-	}
-}
-
-static int pick_commits(struct replay_insn_list *todo_list,
-			struct replay_opts *opts)
-{
-	struct replay_insn_list *cur;
-	int res;
-
-	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	if (opts->allow_ff)
-		assert(!(opts->signoff || opts->no_commit ||
-				opts->record_origin || opts->edit));
-	read_and_refresh_cache(opts);
-
-	for (cur = todo_list; cur; cur = cur->next) {
-		save_todo(cur);
-		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
-			return res;
-		}
-	}
-
-	/*
-	 * Sequence of picks finished successfully; cleanup by
-	 * removing the .git/sequencer directory
-	 */
-	remove_sequencer_state(1);
-	return 0;
-}
-
-static int pick_revisions(struct replay_opts *opts)
-{
-	struct replay_insn_list *todo_list = NULL;
-	unsigned char sha1[20];
-
-	if (opts->subcommand == REPLAY_NONE)
-		assert(opts->revs);
-
-	read_and_refresh_cache(opts);
-
-	/*
-	 * Decide what to do depending on the arguments; a fresh
-	 * cherry-pick should be handled differently from an existing
-	 * one that is being continued
-	 */
-	if (opts->subcommand == REPLAY_RESET) {
-		remove_sequencer_state(1);
-		return 0;
-	} else if (opts->subcommand == REPLAY_CONTINUE) {
-		if (!file_exists(git_path(SEQ_TODO_FILE)))
-			goto error;
-		read_populate_opts(&opts);
-		read_populate_todo(&todo_list);
-
-		/* Verify that the conflict has been resolved */
-		if (!index_differs_from("HEAD", 0))
-			todo_list = todo_list->next;
-	} else {
-		/*
-		 * Start a new cherry-pick/ revert sequence; but
-		 * first, make sure that an existing one isn't in
-		 * progress
-		 */
-
-		walk_revs_populate_todo(&todo_list, opts);
-		if (create_seq_dir() < 0) {
-			error(_("A cherry-pick or revert is in progress."));
-			advise(_("Use --continue to continue the operation"));
-			advise(_("or --reset to forget about it"));
-			return -1;
-		}
-		if (get_sha1("HEAD", sha1)) {
-			if (opts->action == REPLAY_REVERT)
-				return error(_("Can't revert as initial commit"));
-			return error(_("Can't cherry-pick into empty head"));
-		}
-		save_head(sha1_to_hex(sha1));
-		save_opts(opts);
-	}
-	return pick_commits(todo_list, opts);
-error:
-	return error(_("No %s in progress"), action_name(opts));
-}
-
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	struct replay_opts opts;
@@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_REVERT;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("revert failed"));
 	return res;
@@ -1026,7 +209,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
 	opts.action = REPLAY_PICK;
 	git_config(git_default_config, NULL);
 	parse_args(argc, argv, &opts);
-	res = pick_revisions(&opts);
+	res = sequencer_pick_revisions(&opts);
 	if (res < 0)
 		die(_("cherry-pick failed"));
 	return res;
diff --git a/sequencer.c b/sequencer.c
index bc2c046..87f146b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,27 @@
 #include "cache.h"
-#include "sequencer.h"
-#include "strbuf.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
 #include "dir.h"
+#include "sequencer.h"
+
+#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+
+static const char *action_name(const struct replay_opts *opts)
+{
+	return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick";
+}
+
+static char *get_encoding(const char *message);
 
 void remove_sequencer_state(int aggressive)
 {
@@ -17,3 +37,781 @@ void remove_sequencer_state(int aggressive)
 	strbuf_release(&seq_dir);
 	strbuf_release(&seq_old_dir);
 }
+
+struct commit_message {
+	char *parent_label;
+	const char *label;
+	const char *subject;
+	char *reencoded_message;
+	const char *message;
+};
+
+static int get_message(struct commit *commit, struct commit_message *out)
+{
+	const char *encoding;
+	const char *abbrev, *subject;
+	int abbrev_len, subject_len;
+	char *q;
+
+	if (!commit->buffer)
+		return -1;
+	encoding = get_encoding(commit->buffer);
+	if (!encoding)
+		encoding = "UTF-8";
+	if (!git_commit_encoding)
+		git_commit_encoding = "UTF-8";
+
+	out->reencoded_message = NULL;
+	out->message = commit->buffer;
+	if (strcmp(encoding, git_commit_encoding))
+		out->reencoded_message = reencode_string(commit->buffer,
+					git_commit_encoding, encoding);
+	if (out->reencoded_message)
+		out->message = out->reencoded_message;
+
+	abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+	abbrev_len = strlen(abbrev);
+
+	subject_len = find_commit_subject(out->message, &subject);
+
+	out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+			      strlen("... ") + subject_len + 1);
+	q = out->parent_label;
+	q = mempcpy(q, "parent of ", strlen("parent of "));
+	out->label = q;
+	q = mempcpy(q, abbrev, abbrev_len);
+	q = mempcpy(q, "... ", strlen("... "));
+	out->subject = q;
+	q = mempcpy(q, subject, subject_len);
+	*q = '\0';
+	return 0;
+}
+
+static void free_message(struct commit_message *msg)
+{
+	free(msg->parent_label);
+	free(msg->reencoded_message);
+}
+
+static char *get_encoding(const char *message)
+{
+	const char *p = message, *eol;
+
+	while (*p && *p != '\n') {
+		for (eol = p + 1; *eol && *eol != '\n'; eol++)
+			; /* do nothing */
+		if (!prefixcmp(p, "encoding ")) {
+			char *result = xmalloc(eol - 8 - p);
+			strlcpy(result, p + 9, eol - 8 - p);
+			return result;
+		}
+		p = eol;
+		if (*p == '\n')
+			p++;
+	}
+	return NULL;
+}
+
+static void write_cherry_pick_head(struct commit *commit)
+{
+	int fd;
+	struct strbuf buf = STRBUF_INIT;
+
+	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+
+	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
+	if (fd < 0)
+		die_errno(_("Could not open '%s' for writing"),
+			  git_path("CHERRY_PICK_HEAD"));
+	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
+		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
+	strbuf_release(&buf);
+}
+
+static void print_advice(int show_hint)
+{
+	char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+	if (msg) {
+		fprintf(stderr, "%s\n", msg);
+		/*
+		 * A conflict has occured but the porcelain
+		 * (typically rebase --interactive) wants to take care
+		 * of the commit itself so remove CHERRY_PICK_HEAD
+		 */
+		unlink(git_path("CHERRY_PICK_HEAD"));
+		return;
+	}
+
+	if (show_hint) {
+		advise("after resolving the conflicts, mark the corrected paths");
+		advise("with 'git add <paths>' or 'git rm <paths>'");
+		advise("and commit the result with 'git commit'");
+	}
+}
+
+static void write_message(struct strbuf *msgbuf, const char *filename)
+{
+	static struct lock_file msg_file;
+
+	int msg_fd = hold_lock_file_for_update(&msg_file, filename,
+					       LOCK_DIE_ON_ERROR);
+	if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+		die_errno(_("Could not write to %s."), filename);
+	strbuf_release(msgbuf);
+	if (commit_lock_file(&msg_file) < 0)
+		die(_("Error wrapping up %s"), filename);
+}
+
+static struct tree *empty_tree(void)
+{
+	return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+}
+
+static int error_dirty_index(struct replay_opts *opts)
+{
+	if (read_cache_unmerged())
+		return error_resolve_conflict(action_name(opts));
+
+	/* Different translation strings for cherry-pick and revert */
+	if (opts->action == REPLAY_PICK)
+		error(_("Your local changes would be overwritten by cherry-pick."));
+	else
+		error(_("Your local changes would be overwritten by revert."));
+
+	if (advice_commit_before_merge)
+		advise(_("Commit your changes or stash them to proceed."));
+	return -1;
+}
+
+static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+{
+	struct ref_lock *ref_lock;
+
+	read_cache();
+	if (checkout_fast_forward(from, to))
+		exit(1); /* the callee should have complained already */
+	ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+	return write_ref_sha1(ref_lock, to, "cherry-pick");
+}
+
+static int do_recursive_merge(struct commit *base, struct commit *next,
+			      const char *base_label, const char *next_label,
+			      unsigned char *head, struct strbuf *msgbuf,
+			      struct replay_opts *opts)
+{
+	struct merge_options o;
+	struct tree *result, *next_tree, *base_tree, *head_tree;
+	int clean, index_fd;
+	const char **xopt;
+	static struct lock_file index_lock;
+
+	index_fd = hold_locked_index(&index_lock, 1);
+
+	read_cache();
+
+	init_merge_options(&o);
+	o.ancestor = base ? base_label : "(empty tree)";
+	o.branch1 = "HEAD";
+	o.branch2 = next ? next_label : "(empty tree)";
+
+	head_tree = parse_tree_indirect(head);
+	next_tree = next ? next->tree : empty_tree();
+	base_tree = base ? base->tree : empty_tree();
+
+	for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
+		parse_merge_opt(&o, *xopt);
+
+	clean = merge_trees(&o,
+			    head_tree,
+			    next_tree, base_tree, &result);
+
+	if (active_cache_changed &&
+	    (write_cache(index_fd, active_cache, active_nr) ||
+	     commit_locked_index(&index_lock)))
+		/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+		die(_("%s: Unable to write new index file"), action_name(opts));
+	rollback_lock_file(&index_lock);
+
+	if (!clean) {
+		int i;
+		strbuf_addstr(msgbuf, "\nConflicts:\n\n");
+		for (i = 0; i < active_nr;) {
+			struct cache_entry *ce = active_cache[i++];
+			if (ce_stage(ce)) {
+				strbuf_addch(msgbuf, '\t');
+				strbuf_addstr(msgbuf, ce->name);
+				strbuf_addch(msgbuf, '\n');
+				while (i < active_nr && !strcmp(ce->name,
+						active_cache[i]->name))
+					i++;
+			}
+		}
+	}
+
+	return !clean;
+}
+
+/*
+ * If we are cherry-pick, and if the merge did not result in
+ * hand-editing, we will hit this commit and inherit the original
+ * author date and name.
+ * If we are revert, or if our cherry-pick results in a hand merge,
+ * we had better say that the current user is responsible for that.
+ */
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+{
+	/* 6 is max possible length of our args array including NULL */
+	const char *args[6];
+	int i = 0;
+
+	args[i++] = "commit";
+	args[i++] = "-n";
+	if (opts->signoff)
+		args[i++] = "-s";
+	if (!opts->edit) {
+		args[i++] = "-F";
+		args[i++] = defmsg;
+	}
+	args[i] = NULL;
+
+	return run_command_v_opt(args, RUN_GIT_CMD);
+}
+
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+			struct replay_opts *opts)
+{
+	unsigned char head[20];
+	struct commit *base, *next, *parent;
+	const char *base_label, *next_label;
+	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+	char *defmsg = NULL;
+	struct strbuf msgbuf = STRBUF_INIT;
+	int res;
+
+	if (opts->no_commit) {
+		/*
+		 * We do not intend to commit immediately.  We just want to
+		 * merge the differences in, so let's compute the tree
+		 * that represents the "current" state for merge-recursive
+		 * to work on.
+		 */
+		if (write_cache_as_tree(head, 0, NULL))
+			die (_("Your index file is unmerged."));
+	} else {
+		if (get_sha1("HEAD", head))
+			return error(_("You do not have a valid HEAD"));
+		if (index_differs_from("HEAD", 0))
+			return error_dirty_index(opts);
+	}
+	discard_cache();
+
+	if (!commit->parents) {
+		parent = NULL;
+	}
+	else if (commit->parents->next) {
+		/* Reverting or cherry-picking a merge commit */
+		int cnt;
+		struct commit_list *p;
+
+		if (!opts->mainline)
+			return error(_("Commit %s is a merge but no -m option was given."),
+				sha1_to_hex(commit->object.sha1));
+
+		for (cnt = 1, p = commit->parents;
+		     cnt != opts->mainline && p;
+		     cnt++)
+			p = p->next;
+		if (cnt != opts->mainline || !p)
+			return error(_("Commit %s does not have parent %d"),
+				sha1_to_hex(commit->object.sha1), opts->mainline);
+		parent = p->item;
+	} else if (0 < opts->mainline)
+		return error(_("Mainline was specified but commit %s is not a merge."),
+			sha1_to_hex(commit->object.sha1));
+	else
+		parent = commit->parents->item;
+
+	if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
+		return fast_forward_to(commit->object.sha1, head);
+
+	if (parent && parse_commit(parent) < 0)
+		/* TRANSLATORS: The first %s will be "revert" or
+		   "cherry-pick", the second %s a SHA1 */
+		return error(_("%s: cannot parse parent commit %s"),
+			action_name(opts), sha1_to_hex(parent->object.sha1));
+
+	if (get_message(commit, &msg) != 0)
+		return error(_("Cannot get commit message for %s"),
+			sha1_to_hex(commit->object.sha1));
+
+	/*
+	 * "commit" is an existing commit.  We would want to apply
+	 * the difference it introduces since its first parent "prev"
+	 * on top of the current HEAD if we are cherry-pick.  Or the
+	 * reverse of it if we are revert.
+	 */
+
+	defmsg = git_pathdup("MERGE_MSG");
+
+	if (action == REPLAY_REVERT) {
+		base = commit;
+		base_label = msg.label;
+		next = parent;
+		next_label = msg.parent_label;
+		strbuf_addstr(&msgbuf, "Revert \"");
+		strbuf_addstr(&msgbuf, msg.subject);
+		strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+		strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+
+		if (commit->parents && commit->parents->next) {
+			strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
+		}
+		strbuf_addstr(&msgbuf, ".\n");
+	} else {
+		const char *p;
+
+		base = parent;
+		base_label = msg.parent_label;
+		next = commit;
+		next_label = msg.label;
+
+		/*
+		 * Append the commit log message to msgbuf; it starts
+		 * after the tree, parent, author, committer
+		 * information followed by "\n\n".
+		 */
+		p = strstr(msg.message, "\n\n");
+		if (p) {
+			p += 2;
+			strbuf_addstr(&msgbuf, p);
+		}
+
+		if (opts->record_origin) {
+			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+			strbuf_addstr(&msgbuf, ")\n");
+		}
+	}
+
+	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
+		res = do_recursive_merge(base, next, base_label, next_label,
+					 head, &msgbuf, opts);
+		write_message(&msgbuf, defmsg);
+	} else {
+		struct commit_list *common = NULL;
+		struct commit_list *remotes = NULL;
+
+		write_message(&msgbuf, defmsg);
+
+		commit_list_insert(base, &common);
+		commit_list_insert(next, &remotes);
+		res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+					common, sha1_to_hex(head), remotes);
+		free_commit_list(common);
+		free_commit_list(remotes);
+	}
+
+	/*
+	 * If the merge was clean or if it failed due to conflict, we write
+	 * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
+	 * However, if the merge did not even start, then we don't want to
+	 * write it at all.
+	 */
+	if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
+		write_cherry_pick_head(commit);
+
+	if (res) {
+		error(action == REPLAY_REVERT
+		      ? _("could not revert %s... %s")
+		      : _("could not apply %s... %s"),
+		      find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+		      msg.subject);
+		print_advice(res == 1);
+		rerere(opts->allow_rerere_auto);
+	} else {
+		if (!opts->no_commit)
+			res = run_git_commit(defmsg, opts);
+	}
+
+	free_message(&msg);
+	free(defmsg);
+
+	return res;
+}
+
+static void prepare_revs(struct replay_opts *opts)
+{
+	if (opts->action != REPLAY_REVERT)
+		opts->revs->reverse ^= 1;
+
+	if (prepare_revision_walk(opts->revs))
+		die(_("revision walk setup failed"));
+
+	if (!opts->revs->commits)
+		die(_("empty commit set passed"));
+}
+
+static void read_and_refresh_cache(struct replay_opts *opts)
+{
+	static struct lock_file index_lock;
+	int index_fd = hold_locked_index(&index_lock, 0);
+	if (read_index_preload(&the_index, NULL) < 0)
+		die(_("git %s: failed to read the index"), action_name(opts));
+	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+	if (the_index.cache_changed) {
+		if (write_index(&the_index, index_fd) ||
+		    commit_locked_index(&index_lock))
+			die(_("git %s: failed to refresh the index"), action_name(opts));
+	}
+	rollback_lock_file(&index_lock);
+}
+
+/*
+ * Append a commit to the end of the commit_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty commit_list, and is updated to point to the "next" field of
+ * the last item on the list as new commits are appended.
+ *
+ * Usage example:
+ *
+ *     struct commit_list *list;
+ *     struct commit_list **next = &list;
+ *
+ *     next = commit_list_append(c1, next);
+ *     next = commit_list_append(c2, next);
+ *     assert(commit_list_count(list) == 2);
+ *     return list;
+ */
+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,
+						struct commit *operand,
+						struct replay_insn_list **next)
+{
+	struct replay_insn_list *new = xmalloc(sizeof(*new));
+	new->action = action;
+	new->operand = operand;
+	*next = new;
+	new->next = NULL;
+	return &new->next;
+}
+
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+	struct replay_insn_list *cur;
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		const char *sha1_abbrev, *action_str, *subject;
+		int subject_len;
+
+		action_str = cur->action == REPLAY_REVERT ? "revert" : "pick";
+		sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+		subject_len = find_commit_subject(cur->operand->buffer, &subject);
+		strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+			subject_len, subject);
+	}
+	return 0;
+}
+
+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
+{
+	unsigned char commit_sha1[20];
+	char *end_of_object_name;
+	int saved, status;
+
+	if (!prefixcmp(bol, "pick ")) {
+		item->action = REPLAY_PICK;
+		bol += strlen("pick ");
+	} else if (!prefixcmp(bol, "revert ")) {
+		item->action = REPLAY_REVERT;
+		bol += strlen("revert ");
+	} else {
+		size_t len = strchrnul(bol, '\n') - bol;
+		if (len > 255)
+			len = 255;
+		return error(_("Unrecognized action: %.*s"), (int)len, bol);
+	}
+
+	end_of_object_name = bol + strcspn(bol, " \n");
+	saved = *end_of_object_name;
+	*end_of_object_name = '\0';
+	status = get_sha1(bol, commit_sha1);
+	*end_of_object_name = saved;
+
+	if (status < 0)
+		return error(_("Malformed object name: %s"), bol);
+
+	item->operand = lookup_commit_reference(commit_sha1);
+	if (!item->operand)
+		return error(_("Not a valid commit: %s"), bol);
+
+	item->next = NULL;
+	return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+	struct replay_insn_list **next = todo_list;
+	struct replay_insn_list item = {0, NULL, NULL};
+	char *p = buf;
+	int i;
+
+	for (i = 1; *p; i++) {
+		char *eol = strchrnul(p, '\n');
+		if (parse_insn_line(p, eol, &item) < 0)
+			return error(_("on line %d."), i);
+		next = replay_insn_list_append(item.action, item.operand, next);
+		p = *eol ? eol + 1 : eol;
+	}
+	if (!*todo_list)
+		return error(_("No commits parsed."));
+	return 0;
+}
+
+static void read_populate_todo(struct replay_insn_list **todo_list)
+{
+	const char *todo_file = git_path(SEQ_TODO_FILE);
+	struct strbuf buf = STRBUF_INIT;
+	int fd, res;
+
+	fd = open(todo_file, O_RDONLY);
+	if (fd < 0)
+		die_errno(_("Could not open %s."), todo_file);
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		die(_("Could not read %s."), todo_file);
+	}
+	close(fd);
+
+	res = parse_insn_buffer(buf.buf, todo_list);
+	strbuf_release(&buf);
+	if (res)
+		die(_("Unusable instruction sheet: %s"), todo_file);
+}
+
+static int populate_opts_cb(const char *key, const char *value, void *data)
+{
+	struct replay_opts *opts = data;
+	int error_flag = 1;
+
+	if (!value)
+		error_flag = 0;
+	else if (!strcmp(key, "options.no-commit"))
+		opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.edit"))
+		opts->edit = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.signoff"))
+		opts->signoff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.record-origin"))
+		opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.allow-ff"))
+		opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+	else if (!strcmp(key, "options.mainline"))
+		opts->mainline = git_config_int(key, value);
+	else if (!strcmp(key, "options.strategy"))
+		git_config_string(&opts->strategy, key, value);
+	else if (!strcmp(key, "options.strategy-option")) {
+		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+		opts->xopts[opts->xopts_nr++] = xstrdup(value);
+	} else
+		return error(_("Invalid key: %s"), key);
+
+	if (!error_flag)
+		return error(_("Invalid value for %s: %s"), key, value);
+
+	return 0;
+}
+
+static void read_populate_opts(struct replay_opts **opts_ptr)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (!file_exists(opts_file))
+		return;
+	if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+		die(_("Malformed options sheet: %s"), opts_file);
+}
+
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
+				struct replay_opts *opts)
+{
+	struct commit *commit;
+	struct replay_insn_list **next;
+
+	prepare_revs(opts);
+
+	next = todo_list;
+	while ((commit = get_revision(opts->revs)))
+		next = replay_insn_list_append(opts->action, commit, next);
+}
+
+static int create_seq_dir(void)
+{
+	const char *seq_dir = git_path(SEQ_DIR);
+
+	if (file_exists(seq_dir))
+		return error(_("%s already exists."), seq_dir);
+	else if (mkdir(seq_dir, 0777) < 0)
+		die_errno(_("Could not create sequencer directory '%s'."), 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);
+	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);
+	if (commit_lock_file(&head_lock) < 0)
+		die(_("Error wrapping up %s."), head_file);
+}
+
+static void save_todo(struct replay_insn_list *todo_list)
+{
+	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);
+	if (format_todo(&buf, todo_list) < 0)
+		die(_("Could not format %s."), todo_file);
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		strbuf_release(&buf);
+		die_errno(_("Could not write to %s."), todo_file);
+	}
+	if (commit_lock_file(&todo_lock) < 0) {
+		strbuf_release(&buf);
+		die(_("Error wrapping up %s."), todo_file);
+	}
+	strbuf_release(&buf);
+}
+
+static void save_opts(struct replay_opts *opts)
+{
+	const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+	if (opts->no_commit)
+		git_config_set_in_file(opts_file, "options.no-commit", "true");
+	if (opts->edit)
+		git_config_set_in_file(opts_file, "options.edit", "true");
+	if (opts->signoff)
+		git_config_set_in_file(opts_file, "options.signoff", "true");
+	if (opts->record_origin)
+		git_config_set_in_file(opts_file, "options.record-origin", "true");
+	if (opts->allow_ff)
+		git_config_set_in_file(opts_file, "options.allow-ff", "true");
+	if (opts->mainline) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "%d", opts->mainline);
+		git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+		strbuf_release(&buf);
+	}
+	if (opts->strategy)
+		git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+	if (opts->xopts) {
+		int i;
+		for (i = 0; i < opts->xopts_nr; i++)
+			git_config_set_multivar_in_file(opts_file,
+							"options.strategy-option",
+							opts->xopts[i], "^$", 0);
+	}
+}
+
+static int pick_commits(struct replay_insn_list *todo_list,
+			struct replay_opts *opts)
+{
+	struct replay_insn_list *cur;
+	int res;
+
+	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+	if (opts->allow_ff)
+		assert(!(opts->signoff || opts->no_commit ||
+				opts->record_origin || opts->edit));
+	read_and_refresh_cache(opts);
+
+	for (cur = todo_list; cur; cur = cur->next) {
+		save_todo(cur);
+		res = do_pick_commit(cur->operand, cur->action, opts);
+		if (res) {
+			if (!cur->next)
+				/*
+				 * An error was encountered while
+				 * picking the last commit; the
+				 * sequencer state is useless now --
+				 * the user simply needs to resolve
+				 * the conflict and commit
+				 */
+				remove_sequencer_state(0);
+			return res;
+		}
+	}
+
+	/*
+	 * Sequence of picks finished successfully; cleanup by
+	 * removing the .git/sequencer directory
+	 */
+	remove_sequencer_state(1);
+	return 0;
+}
+
+int sequencer_pick_revisions(struct replay_opts *opts)
+{
+	struct replay_insn_list *todo_list = NULL;
+	unsigned char sha1[20];
+
+	if (opts->subcommand == REPLAY_NONE)
+		assert(opts->revs);
+
+	read_and_refresh_cache(opts);
+
+	/*
+	 * Decide what to do depending on the arguments; a fresh
+	 * cherry-pick should be handled differently from an existing
+	 * one that is being continued
+	 */
+	if (opts->subcommand == REPLAY_RESET) {
+		remove_sequencer_state(1);
+		return 0;
+	} else if (opts->subcommand == REPLAY_CONTINUE) {
+		if (!file_exists(git_path(SEQ_TODO_FILE)))
+			goto error;
+		read_populate_opts(&opts);
+		read_populate_todo(&todo_list);
+
+		/* Verify that the conflict has been resolved */
+		if (!index_differs_from("HEAD", 0))
+			todo_list = todo_list->next;
+	} else {
+		/*
+		 * Start a new cherry-pick/ revert sequence; but
+		 * first, make sure that an existing one isn't in
+		 * progress
+		 */
+
+		walk_revs_populate_todo(&todo_list, opts);
+		if (create_seq_dir() < 0) {
+			error(_("A cherry-pick or revert is in progress."));
+			advise(_("Use --continue to continue the operation"));
+			advise(_("or --reset to forget about it"));
+			return -1;
+		}
+		if (get_sha1("HEAD", sha1)) {
+			if (opts->action == REPLAY_REVERT)
+				return error(_("Can't revert as initial commit"));
+			return error(_("Can't cherry-pick into empty head"));
+		}
+		save_head(sha1_to_hex(sha1));
+		save_opts(opts);
+	}
+	return pick_commits(todo_list, opts);
+error:
+	return error(_("No %s in progress"), action_name(opts));
+}
diff --git a/sequencer.h b/sequencer.h
index f4db257..92b2d63 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -8,6 +8,30 @@
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
 enum replay_action { REPLAY_REVERT, REPLAY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
+
+struct replay_opts {
+	enum replay_action action;
+	enum replay_subcommand subcommand;
+
+	/* Boolean options */
+	int edit;
+	int record_origin;
+	int no_commit;
+	int signoff;
+	int allow_ff;
+	int allow_rerere_auto;
+
+	int mainline;
+
+	/* Merge strategy */
+	const char *strategy;
+	const char **xopts;
+	size_t xopts_nr, xopts_alloc;
+
+	/* Only used by REPLAY_NONE */
+	struct rev_info *revs;
+};
 
 struct replay_insn_list {
 	enum replay_action action;
@@ -25,4 +49,6 @@ struct replay_insn_list {
  */
 void remove_sequencer_state(int aggressive);
 
+int sequencer_pick_revisions(struct replay_opts *opts);
+
 #endif
-- 
1.7.6.351.gb35ac.dirty

[PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state

[Ramkumar Ramachandra]

Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when
invoked aggressively, since we want to treat it as part of the
sequencer state now.  While at it, make some minor improvements to the
function.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c |   27 ++++++++++++++++-----------
 1 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 87f146b..e566043 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -25,17 +25,22 @@ static char *get_encoding(const char *message);
 
 void remove_sequencer_state(int aggressive)
 {
-	struct strbuf seq_dir = STRBUF_INIT;
-	struct strbuf seq_old_dir = STRBUF_INIT;
-
-	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
-	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
-	remove_dir_recursively(&seq_old_dir, 0);
-	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
-	if (aggressive)
-		remove_dir_recursively(&seq_old_dir, 0);
-	strbuf_release(&seq_dir);
-	strbuf_release(&seq_old_dir);
+	const char *seq_dir = git_path(SEQ_DIR);
+	const char *seq_old_dir = git_path(SEQ_OLD_DIR);
+	const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");
+	struct strbuf seq_dir_buf = STRBUF_INIT;
+	struct strbuf seq_old_dir_buf = STRBUF_INIT;
+
+	strbuf_addf(&seq_dir_buf, "%s", seq_dir);
+	strbuf_addf(&seq_old_dir_buf, "%s", seq_old_dir);
+	remove_dir_recursively(&seq_old_dir_buf, 0);
+	rename(seq_dir, seq_old_dir);
+	if (aggressive) {
+		remove_dir_recursively(&seq_old_dir_buf, 0);
+		unlink(cherry_pick_head);
+	}
+	strbuf_release(&seq_dir_buf);
+	strbuf_release(&seq_old_dir_buf);
 }
 
 struct commit_message {
-- 
1.7.6.351.gb35ac.dirty

[PATCH 3/5] sequencer: sequencer state is useless without todo

[Ramkumar Ramachandra]

Later in the series, we will not write '.git/sequencer/todo' for a
single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains
this information.  So, stomp the sequencer state in create_seq_state()
unless the todo file is present.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e566043..517eb23 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
 
 static int create_seq_dir(void)
 {
+	const char *todo_file = git_path(SEQ_TODO_FILE);
 	const char *seq_dir = git_path(SEQ_DIR);
 
-	if (file_exists(seq_dir))
-		return error(_("%s already exists."), seq_dir);
-	else if (mkdir(seq_dir, 0777) < 0)
+	if (file_exists(todo_file))
+		return error(_("%s already exists."), todo_file);
+
+	/* If todo_file doesn't exist, discard sequencer state */
+	remove_sequencer_state(1);
+	if (mkdir(seq_dir, 0777) < 0)
 		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
 	return 0;
 }
-- 
1.7.6.351.gb35ac.dirty

[PATCH 4/5] sequencer: handle single commit pick separately

[Ramkumar Ramachandra]

Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already
contains this information.  However, '.git/sequencer/opts' and
'.git/sequencer/head' are required to support '--reset' and
'--continue' operations.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 517eb23..6762ceb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,
 				opts->record_origin || opts->edit));
 	read_and_refresh_cache(opts);
 
+	/*
+	 * Backward compatibility hack: when only a single commit is
+	 * picked, don't save_todo(), because CHERRY_PICK_HEAD will
+	 * contain this information anyway.
+	 */
+	if (opts->subcommand == REPLAY_NONE &&
+		todo_list->next == NULL && todo_list->action == REPLAY_PICK)
+		return do_pick_commit(todo_list->operand, REPLAY_PICK, opts);
+
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur);
 		res = do_pick_commit(cur->operand, cur->action, opts);
-- 
1.7.6.351.gb35ac.dirty

[PATCH 5/5] sequencer: revert d3f4628e

[Ramkumar Ramachandra]

Revert d3f4628e (revert: Remove sequencer state when no commits are
pending, 2011-06-06), because this is not the right approach.  Instead
of increasing coupling between the sequencer and 'git commit', a
unified '--continue' that invokes 'git commit' on behalf of the
end-user is preferred.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 sequencer.c                     |   12 +-----------
 t/t3510-cherry-pick-sequence.sh |   24 ------------------------
 2 files changed, 1 insertions(+), 35 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 6762ceb..7caa550 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -758,18 +758,8 @@ static int pick_commits(struct replay_insn_list *todo_list,
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur);
 		res = do_pick_commit(cur->operand, cur->action, opts);
-		if (res) {
-			if (!cur->next)
-				/*
-				 * An error was encountered while
-				 * picking the last commit; the
-				 * sequencer state is useless now --
-				 * the user simply needs to resolve
-				 * the conflict and commit
-				 */
-				remove_sequencer_state(0);
+		if (res)
 			return res;
-		}
 	}
 
 	/*
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 4b12244..b30f13a 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -85,30 +85,6 @@ test_expect_success '--reset cleans up sequencer state' '
 	test_path_is_missing .git/sequencer
 '
 
-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
-	pristine_detach initial &&
-	test_must_fail git cherry-pick base..picked &&
-	test_path_is_missing .git/sequencer &&
-	echo "resolved" >foo &&
-	git add foo &&
-	git commit &&
-	{
-		git rev-list HEAD |
-		git diff-tree --root --stdin |
-		sed "s/$_x40/OBJID/g"
-	} >actual &&
-	cat >expect <<-\EOF &&
-	OBJID
-	:100644 100644 OBJID OBJID M	foo
-	OBJID
-	:100644 100644 OBJID OBJID M	unrelated
-	OBJID
-	:000000 100644 OBJID OBJID A	foo
-	:000000 100644 OBJID OBJID A	unrelated
-	EOF
-	test_cmp expect actual
-'
-
 test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '
 	pristine_detach initial &&
 	test_must_fail git cherry-pick base..anotherpick &&
-- 
1.7.6.351.gb35ac.dirty

Re: [PATCH 0/5] Sequencer: working around historical mistakes

[Jonathan Nieder]

Hey,

Ramkumar Ramachandra wrote:

> As described in the discussion following $gmane/179304/focus=179383,
> we have decided to handle historical hacks in the sequencer itself.
> This series that follows is one step in the right direction.

I'm not sure what the above means.  But let's see what the patches
say. :)

[...]
> 2. This series depends on rr/revert-cherry-pick, but doesn't apply to
> the current 'next'- sorry, rebasing is a massive pita due to 1/5.

Shouldn't it be based against rr/revert-cherry-pick, rather than
"next" which is more of a moving target?

Thanks,
Jonathan

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Start building the generalized sequencer by moving code from revert.c
> into sequencer.c and sequencer.h.  Make the builtin responsible only
> for command-line parsing, and expose a new sequencer_pick_revisions()
> to do the actual work of sequencing commits.
>
> This is intended to be almost a pure code movement patch with no
> functional changes.  Check with:

Do I understand correctly that the purpose of this patch is to expose
some functions through the "sequencer.h" API, which patches later in
the series will use?  Which functions?  What is this generalized
sequencer which we are starting to build?  Why should I be happy about
(or care about, for that matter) code having moved from one source
file to another?

Rule of thumb for commit messages: after reading a commit message, I
should be able to predict what the patch will do, without reading the
patch.

I am guessing the above description started sane and then went through
a few revisions without a person reading it all the way through again.
Please consider just rewriting it.

[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1,19 +1,9 @@
>  #include "cache.h"
>  #include "builtin.h"
> -#include "object.h"
> -#include "commit.h"
> -#include "tag.h"
> -#include "run-command.h"
> -#include "exec_cmd.h"
> -#include "utf8.h"
>  #include "parse-options.h"
> -#include "cache-tree.h"
>  #include "diff.h"
>  #include "revision.h"
>  #include "rerere.h"
> -#include "merge-recursive.h"
> -#include "refs.h"
> -#include "dir.h"
>  #include "sequencer.h"

Hoorah!

[snipping lots of deletion of code from builtin/revert.c]
> @@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>  	opts.action = REPLAY_REVERT;
>  	git_config(git_default_config, NULL);
>  	parse_args(argc, argv, &opts);
> -	res = pick_revisions(&opts);
> +	res = sequencer_pick_revisions(&opts);

The new sequencer_pick_revisions is just a new name for the old
pick_revisions.  Sane, but probably worth mentioning in the log
message.

[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,7 +1,27 @@
>  #include "cache.h"
> +#include "object.h"
> +#include "commit.h"
> +#include "tag.h"
> +#include "run-command.h"
> +#include "exec_cmd.h"
> +#include "utf8.h"
> +#include "cache-tree.h"
> +#include "diff.h"
> +#include "revision.h"
> +#include "rerere.h"
> +#include "merge-recursive.h"
> +#include "refs.h"
> -#include "sequencer.h"
> -#include "strbuf.h"
>  #include "dir.h"
> +#include "sequencer.h"

Why did sequencer.h move to after dir.h?  Wow, we use a lot of headers
here --- I wonder if there are some pieces that could be split out
(that's not due to your patch, though).

[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -8,6 +8,30 @@
>  #define SEQ_OPTS_FILE	"sequencer/opts"
>  
>  enum replay_action { REPLAY_REVERT, REPLAY_PICK };
> +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
> +
> +struct replay_opts {
[...]
> @@ -25,4 +49,6 @@ struct replay_insn_list {
>   */
>  void remove_sequencer_state(int aggressive);
>  
> +int sequencer_pick_revisions(struct replay_opts *opts);

Ah, so this moves most of the logic of "git cherry-pick" to the sequencer
but the only new API that needs to be exposed is pick_revisions().  The
calling sequence looks like this:

	memset(&opts, o, sizeof(opts));
	opts.action = REPLAY_PICK;
	opts.revs = xmalloc(sizeof(*opts.revs));

	init_revisions(opts.revs);
	add_pending_object / setup_revisions / etc

	sequencer_pick_revisions(&opts);

The small exposed interface makes this a relatively uninvasive patch,
and the immediate advantage is that we plan to reuse some of the
functionality used in pick_revisions() in other, new APIs to be used
by commands other than cherry-pick.  No functional change yet intended.

Except for the commit message, looks reasonable (though I haven't
tried the "git blame" magic to check the code movement part).  Thanks.

Re: [PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when
> invoked aggressively, since we want to treat it as part of the
> sequencer state now.  While at it, make some minor improvements to the
> function.

What does it mean to invoke a function aggressively?  What is the
nature of these minor improvements (are they behavior changes or just
cleanups)?  (Remember, the reader hasn't seen the patch yet.)

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -25,17 +25,22 @@ static char *get_encoding(const char *message);
>  
>  void remove_sequencer_state(int aggressive)
>  {
> +	const char *seq_dir = git_path(SEQ_DIR);
> +	const char *seq_old_dir = git_path(SEQ_OLD_DIR);
> +	const char *cherry_pick_head = git_path("CHERRY_PICK_HEAD");

If there were just two more like this, the behavior would change
completely.  Scary.  Are these temporary variables needed?

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Later in the series, we will not write '.git/sequencer/todo' for a
> single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains
> this information.  So, stomp the sequencer state in create_seq_state()
> unless the todo file is present.

What problem does this solve?  How does it solve it?  What does it
mean to stomp?

The usual commit-message debugging strategy applies here: imagine you
are a BIOS clone manufacturer, and for legal reasons you are not
allowed to read this part of the git implementation embedded in the
standard BIOS.  However, you are allowed to read the commit message,
and if that message is clear enough, it will explain the purpose and
behavior of that code and you will be able to implement a compatible
implementation addressing the same problem without scratching your
head too much.

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
>  
>  static int create_seq_dir(void)
>  {
> +	const char *todo_file = git_path(SEQ_TODO_FILE);
>  	const char *seq_dir = git_path(SEQ_DIR);

Scary idiom.

> -	if (file_exists(seq_dir))
> -		return error(_("%s already exists."), seq_dir);
> -	else if (mkdir(seq_dir, 0777) < 0)
> +	if (file_exists(todo_file))
> +		return error(_("%s already exists."), todo_file);
> +
> +	/* If todo_file doesn't exist, discard sequencer state */
> +	remove_sequencer_state(1);
> +	if (mkdir(seq_dir, 0777) < 0)
>  		die_errno(_("Could not create sequencer directory '%s'."), seq_dir);

I guess this patch would make more sense after patch 4.

Re: [PATCH 4/5] sequencer: handle single commit pick separately

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already
> contains this information.  However, '.git/sequencer/opts' and
> '.git/sequencer/head' are required to support '--reset' and
> '--continue' operations.

This is meant as a signal to later "git cherry-pick" commands that it
is okay to forget about the cherry-pick, right?  How is the reader
supposed to know that?  Say so!

By the way, it's not clear to me yet whether the resulting UI would be
more pleasant or not.  What is the expected calling sequence?  Any odd
corners of behavior changing?  What happens if I do

	git cherry-pick foo; # conflicts!
	git cherry-pick bar; # just ignore them

or

	git cherry-pick foo; # conflicts!  but resolved in index by rerere
	git checkout something-else

Is there any potential downside to the change?

[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,
>  				opts->record_origin || opts->edit));
>  	read_and_refresh_cache(opts);
>  
> +	/*
> +	 * Backward compatibility hack: when only a single commit is
> +	 * picked, don't save_todo(), because CHERRY_PICK_HEAD will
> +	 * contain this information anyway.
> +	 */

How does saving disk space by avoiding saving redundant information
affect backward compatibility?  I'm not sure what this comment is
trying to say.

> +	if (opts->subcommand == REPLAY_NONE &&
> +		todo_list->next == NULL && todo_list->action == REPLAY_PICK)
> +		return do_pick_commit(todo_list->operand, REPLAY_PICK, opts);
> +
>  	for (cur = todo_list; cur; cur = cur->next) {

Re: [PATCH 5/5] sequencer: revert d3f4628e

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Revert d3f4628e (revert: Remove sequencer state when no commits are
> pending, 2011-06-06), because this is not the right approach.  Instead
> of increasing coupling between the sequencer and 'git commit', a
> unified '--continue' that invokes 'git commit' on behalf of the
> end-user is preferred.

Forgive me for forgetting: what is the problem that d3f4628e was going
to resolve (i.e., right approach to what)?  What is this increased
coupling, and why do we want to avoid it?  Is "to prefer" another word
for "to implement"?  Who is being united by this new --continue
switch?

Is this patch just reverting a previous patch?  If so, why doesn't the
commit message use the usual format

	Revert "<commit message>"

	This reverts commit <unabbreviated object name>.

	<explanation>

?

>  sequencer.c                     |   12 +-----------
>  t/t3510-cherry-pick-sequence.sh |   24 ------------------------
>  2 files changed, 1 insertions(+), 35 deletions(-)

When changing behavior, it's more comforting to modify tests to describe
the new behavior than to just get rid of them. :)

To sum matters up: with a new commit message, patch 1 seems likely to
be ready.  Patches 2-5 seem to need more work --- it's not clear to me
yet what they are supposed to do.

Hope that helps,
Jonathan

Re: [PATCH 5/5] sequencer: revert d3f4628e

[Junio C Hamano]

Thanks for detailed reviews, Jonathan; looking forward for a re-roll, as I
think the general direction the series seems to be aiming to go is good.

Re: [PATCH 5/5] sequencer: revert d3f4628e

[Ramkumar Ramachandra]

Hi Jonathan and Junio,

Jonathan Nieder writes:
> [...]

Junio C Hamano writes:
> Thanks for detailed reviews, Jonathan; looking forward for a re-roll, as I
> think the general direction the series seems to be aiming to go is good.

Thanks for the early feedback!  I'll polish the series this week.

-- Ram

Re: [PATCH 5/5] sequencer: revert d3f4628e

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Is this patch just reverting a previous patch?  If so, why doesn't the
> commit message use the usual format
>
>        Revert "<commit message>"
>
>        This reverts commit <unabbreviated object name>.
>
>        <explanation>
> [...]

I'd have loved to use 'git revert d3f4628e', but that ends up creating
a lot more work: recall the big move made by 1/5?  I'm trying to
"effectively port the inverse of the changes made by d3f4628e in
revert.c to sequencer.c" -- would you still like to see a git-revert
style commit message?  Don't you think it'll be misleading?

Sorry about the shoddy commit messages though: I'm polishing the
series now that I'm convinced that it's heading in the right
direction.  Hopefully, I'll have more to show soon.

Thanks.

-- Ram

Re: [PATCH 5/5] sequencer: revert d3f4628e

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> I'm trying to
> "effectively port the inverse of the changes made by d3f4628e in
> revert.c to sequencer.c" -- would you still like to see a git-revert
> style commit message?  Don't you think it'll be misleading?

My main complaint is that the subject line (and then the body) didn't
tell me what effect the patch would have in a self-contained way.

I don't think a git-revert style commit message would be misleading.
Couldn't you avoid confusing people by providing the relevant
information directly?  "This commit was not made with 'git revert',
since there has been too much code reorganization in the meantime;
instead, I applied the inverse of the changes made by d3f4628e by
hand.  This patch also tweaks the test added in that commit instead of
removing it."

> Sorry about the shoddy commit messages though: I'm polishing the
> series now that I'm convinced that it's heading in the right
> direction.  Hopefully, I'll have more to show soon.

Thanks.  I'll try not to be distracted and to just focus on the code
for the next round.

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Ramkumar Ramachandra]

Hi,

Small note.

Jonathan Nieder writes:
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1,7 +1,27 @@
>>  #include "cache.h"
>> +#include "object.h"
>> +#include "commit.h"
>> +#include "tag.h"
>> +#include "run-command.h"
>> +#include "exec_cmd.h"
>> +#include "utf8.h"
>> +#include "cache-tree.h"
>> +#include "diff.h"
>> +#include "revision.h"
>> +#include "rerere.h"
>> +#include "merge-recursive.h"
>> +#include "refs.h"
>> -#include "sequencer.h"
>> -#include "strbuf.h"
>>  #include "dir.h"
>> +#include "sequencer.h"
>
> Why did sequencer.h move to after dir.h?

1. I like the convention of including the "foo.h" as the last header
in "foo.c".  I suppose it has to do with the way I include standard
headers in my own code (for pet projects).
2. I didn't want to include many of these headers in sequencer.h again
-- it uses a lot of these data types.  I've noticed that ordering of
header inclusion is important in many parts of Git, so the convention
just stuck.

Thanks.

-- Ram

Re: [PATCH 0/5] Sequencer: working around historical mistakes

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder writes:
> Shouldn't it be based against rr/revert-cherry-pick, rather than
> "next" which is more of a moving target?

When I read this, I went: "Now why didn't I think of that?"

Thanks.

-- Ram

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> [...]
> The usual commit-message debugging strategy applies here: imagine you
> are a BIOS clone manufacturer, and for legal reasons you are not
> allowed to read this part of the git implementation embedded in the
> standard BIOS.  However, you are allowed to read the commit message,
> and if that message is clear enough, it will explain the purpose and
> behavior of that code and you will be able to implement a compatible
> implementation addressing the same problem without scratching your
> head too much.

Ah, it helps to think about commit messages like this.  Thanks.

>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
>>
>>  static int create_seq_dir(void)
>>  {
>> +     const char *todo_file = git_path(SEQ_TODO_FILE);
>>       const char *seq_dir = git_path(SEQ_DIR);
>
> Scary idiom.

What's scary about it?

-- Ram

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>>>  static int create_seq_dir(void)
>>>  {
>>> +     const char *todo_file = git_path(SEQ_TODO_FILE);
>>>       const char *seq_dir = git_path(SEQ_DIR);
>>
>> Scary idiom.
>
> What's scary about it?

The next person who copies and pastes this code to other codepaths without
thinking that the return value of git_path() is ephemeral and may need to
be saved away depending on what goes between its assignment and its use.

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> Why did sequencer.h move to after dir.h?
>
> 1. I like the convention of including the "foo.h" as the last header
> in "foo.c".

I do not think it is a good convention. The implementation of "foo.c" may
need to include many other headers for its own use of other APIs, and the
declarations in "foo.h" may depend on some types declared in some of them,
but by definition the latter is a subset of the former. Having "foo.h" at
the end of "foo.c" makes it difficult for others to tell between the two.

A user of foo.h API should need to include only git-compat-util.h and
foo.h to be able to use foo.h API in the ideal world, even though it may
need to include other headers to use other APIs defined in them.

A workable alternative in a world that is not so perfect for a user of
"foo.h" API is to include git-compat-util.h and what "foo.h" needs before
including "foo.h" and then other headers it needs. I think the current
source code takes this approach.

With that observation, it would probably make more sense if "foo.c"
included the headers in the following order:

 - git-compat-util.h (or the prominent ones like "cache.h" that is known
   to include it at the beginning);
 - Anything the declarations in "foo.h" depends on;
 - "foo.h" itself; and finally
 - Other headers that "foo.c" implementation needs.

That way, people who want to use "foo.h" can guess what needs to be
included before using "foo.h" a lot more easily.

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Ramkumar Ramachandra]

Hi Junio,

Junio C Hamano writes:
> [...]
> With that observation, it would probably make more sense if "foo.c"
> included the headers in the following order:
>
>  - git-compat-util.h (or the prominent ones like "cache.h" that is known
>   to include it at the beginning);
>  - Anything the declarations in "foo.h" depends on;
>  - "foo.h" itself; and finally
>  - Other headers that "foo.c" implementation needs.
>
> That way, people who want to use "foo.h" can guess what needs to be
> included before using "foo.h" a lot more easily.

That's a good rule-of-thumb.  Thanks :)

-- Ram

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Ramkumar Ramachandra]

Hi,

Junio C Hamano wrote:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
>
>>>>  static int create_seq_dir(void)
>>>>  {
>>>> +     const char *todo_file = git_path(SEQ_TODO_FILE);
>>>>       const char *seq_dir = git_path(SEQ_DIR);
>>>
>>> Scary idiom.
>>
>> What's scary about it?
>
> The next person who copies and pastes this code to other codepaths without
> thinking that the return value of git_path() is ephemeral and may need to
> be saved away depending on what goes between its assignment and its use.

Yeah, git_path() writes to one of the four static buffers in
path.c:get_pathname().  Which brings me to: what should (can) we do
about it?  Explicitly xmalloc()'ing and free()'ing a tiny path buffer
is an overkill, so I'm thinking more on the lines of good
documentation.  I've been guilty of misusing git_path() blindly in the
past myself.

Thanks.

-- Ram

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Miles Bader]

Ramkumar Ramachandra <artagnon@gmail.com> writes:
> Junio C Hamano writes:
>> [...]
>> With that observation, it would probably make more sense if "foo.c"
>> included the headers in the following order:
>>
>>  - Anything the declarations in "foo.h" depends on;
>>  - "foo.h" itself; and finally
>>  - Other headers that "foo.c" implementation needs.
>>
>> That way, people who want to use "foo.h" can guess what needs to be
>> included before using "foo.h" a lot more easily.
>
> That's a good rule-of-thumb.  Thanks :)

Does git not use the common practice of self-contained headers?

-miles

-- 
Fast, small, soon; pick any 2.

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

[Jonathan Nieder]

Miles Bader wrote:

> Does git not use the common practice of self-contained headers?

It usually does, with two exceptions.

Headers do not usually include git-compat-util.h directly, which is a
good thing, since it reminds callers to include git-compat-util.h
before anything else.

Headers might sometimes forget to declare types defined in cache.h,
which would be a mistake.  For example, in branch.h we see:

 int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);

Which means the following code does not type-check:

 #include "git-compat-util.h"
 #include "branch.h"
 #include "strbuf.h"

 int demo(const char *name, struct strbuf *ref)
 {
	return validate_new_branchname(name, ref, 0, 0);
 }

Reordering the #includes to put strbuf.h before branch.h is a possible
workaround.  Adding the missing forward declaration is better:

diff --git i/branch.h w/branch.h
index 1285158d..d5240a20 100644
--- i/branch.h
+++ w/branch.h
@@ -1,6 +1,9 @@
 #ifndef BRANCH_H
 #define BRANCH_H
 
+struct strbuf;
+enum branch_track;
+
 /* Functions for acting on the information about branches. */
 
 /*
-- 

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Yeah, git_path() writes to one of the four static buffers in
> path.c:get_pathname().  Which brings me to: what should (can) we do
> about it?

Just use a sane idiom.  Which means: as few git_path() values in
flight at a time as possible.

In other words, do not save the git_path() result in a variable, but
pass it directly to whatever computation needs to use it.

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>
>> Yeah, git_path() writes to one of the four static buffers in
>> path.c:get_pathname().  Which brings me to: what should (can) we do
>> about it?
>
> Just use a sane idiom.  Which means: as few git_path() values in
> flight at a time as possible.
>
> In other words, do not save the git_path() result in a variable, but
> pass it directly to whatever computation needs to use it.

Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Ramkumar Ramachandra]

Hi,

Jonathan Nieder wrote:
> Just use a sane idiom.  Which means: as few git_path() values in
> flight at a time as possible.

Makes sense, thanks.

Junio C Hamano wrote:
> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436

I noticed that sha1_to_hex() also operates like this.  The
resolve_ref() function is really important, but using the same
technique for these tiny functions is probably an overkill; something
in `Documentation/technical` perhaps?

Thanks.

-- Ram

Re: [PATCH 3/5] sequencer: sequencer state is useless without todo

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> I noticed that sha1_to_hex() also operates like this.

A function to externalize our internal representation like sha1_to_hex()
is not such a big problem in practice, as the lifetime of its result is
inherently much shorter.

Anybody sane with a datum that eventually needs to be externalized will
keep it in its internal representation as long as possible, and then call
such an internal-to-external function just before it becomes absolutely
necessary to externalize it (e.g. calling printf(), packet_write(), etc).
This is because the whole point of having an internal representation
(e.g. when our code talks about an object name, we always use "unsigned
char[20]") is so that all of our functions can use that representation to
pass it around. It would be insane to call such a function earlier than
necessary, having to pass external representation around.

On the other hand, resolve_ref() is an interface to canonicalize external
representation into a form suitable to be kept and passed around as its
internal representation. The lifetime of its result fundamentally has to
be a lot longer than that of functions that work in the opposite
direction, e.g. sha1_to_hex().

[PATCH 0/3] avoiding unintended consequences of git_path() usage

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Junio C Hamano wrote:

>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436
>
> I noticed that sha1_to_hex() also operates like this.  The
> resolve_ref() function is really important, but using the same
> technique for these tiny functions is probably an overkill

I don't follow.  Do you mean that not being confusing is overkill,
because the function is small that no one will bother to look up the
right semantics?  Wait, that sentence didn't come out the way I
wanted. ;-)

Jokes aside, here's a rough series to do the git_path ->
git_path_unsafe renaming.  While writing it, I noticed a couple of
bugs, hence the two patches before the last one.  Patch 2 is the more
interesting one.

Patches are against "master", but patch 2 probably should be thought
of as being against maint-1.7.6.  Improvements welcome, as always.

Thanks,

Jonathan Nieder (3):
  do not let git_path clobber errno when reporting errors
  Bigfile: dynamically allocate buffer for marks file name
  rename git_path() to git_path_unsafe()

 Documentation/technical/api-string-list.txt |    5 +-
 attr.c                                      |    2 +-
 bisect.c                                    |    8 ++--
 branch.c                                    |   12 ++--
 builtin/add.c                               |    2 +-
 builtin/commit.c                            |   57 ++++++++++++-----------
 builtin/config.c                            |    4 +-
 builtin/fetch-pack.c                        |    4 +-
 builtin/fetch.c                             |    5 +-
 builtin/fsck.c                              |    2 +-
 builtin/init-db.c                           |   12 ++--
 builtin/merge.c                             |   67 +++++++++++++++------------
 builtin/notes.c                             |    2 +-
 builtin/remote.c                            |    6 +-
 builtin/reset.c                             |    2 +-
 builtin/revert.c                            |   25 +++++-----
 cache.h                                     |    3 +-
 contrib/examples/builtin-fetch--tool.c      |    4 +-
 dir.c                                       |    2 +-
 fast-import.c                               |    2 +-
 http-backend.c                              |    2 +-
 notes-merge.c                               |   22 +++++----
 pack-refs.c                                 |    6 +-
 path.c                                      |    2 +-
 refs.c                                      |   51 +++++++++++---------
 remote.c                                    |    4 +-
 rerere.c                                    |   12 ++--
 run-command.c                               |    4 +-
 sequencer.c                                 |    6 +-
 server-info.c                               |    2 +-
 sha1_file.c                                 |   22 ++++++--
 shallow.c                                   |    2 +-
 transport.c                                 |    4 +-
 unpack-trees.c                              |    2 +-
 34 files changed, 200 insertions(+), 167 deletions(-)

-- 
1.7.8.rc0

[PATCH 1/3] do not let git_path clobber errno when reporting errors

[Jonathan Nieder]

Because git_path() calls vsnprintf(), code like

	fd = open(git_path("SQUASH_MSG"), O_WRONLY | O_CREAT, 0666);
	die_errno(_("Could not write to '%s'"), git_path("SQUASH_MSG"));

can end up printing an error indicator from vsnprintf() instead of
open() by mistake.  Store the path we are trying to write to in a
temporary variable and pass _that_ to die_errno(), so the messages
written by git cherry-pick/revert and git merge can avoid this source
of confusion.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/merge.c  |   49 +++++++++++++++++++++++++++++--------------------
 builtin/revert.c |    9 +++++----
 2 files changed, 34 insertions(+), 24 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index dffd5ec1..2870a6af 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -316,13 +316,15 @@ static void squash_message(struct commit *commit)
 	struct rev_info rev;
 	struct strbuf out = STRBUF_INIT;
 	struct commit_list *j;
+	const char *filename;
 	int fd;
 	struct pretty_print_context ctx = {0};
 
 	printf(_("Squash commit -- not updating HEAD\n"));
-	fd = open(git_path("SQUASH_MSG"), O_WRONLY | O_CREAT, 0666);
+	filename = git_path("SQUASH_MSG");
+	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
-		die_errno(_("Could not write to '%s'"), git_path("SQUASH_MSG"));
+		die_errno(_("Could not write to '%s'"), filename);
 
 	init_revisions(&rev, NULL);
 	rev.ignore_merges = 1;
@@ -492,14 +494,16 @@ static void merge_name(const char *remote, struct strbuf *msg)
 
 	if (!strcmp(remote, "FETCH_HEAD") &&
 			!access(git_path("FETCH_HEAD"), R_OK)) {
+		const char *filename;
 		FILE *fp;
 		struct strbuf line = STRBUF_INIT;
 		char *ptr;
 
-		fp = fopen(git_path("FETCH_HEAD"), "r");
+		filename = git_path("FETCH_HEAD");
+		fp = fopen(filename, "r");
 		if (!fp)
 			die_errno(_("could not open '%s' for reading"),
-				  git_path("FETCH_HEAD"));
+				  filename);
 		strbuf_getline(&line, fp, '\n');
 		fclose(fp);
 		ptr = strstr(line.buf, "\tnot-for-merge\t");
@@ -847,20 +851,22 @@ static void add_strategies(const char *string, unsigned attr)
 
 static void write_merge_msg(struct strbuf *msg)
 {
-	int fd = open(git_path("MERGE_MSG"), O_WRONLY | O_CREAT, 0666);
+	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"),
-			  git_path("MERGE_MSG"));
+			  filename);
 	if (write_in_full(fd, msg->buf, msg->len) != msg->len)
-		die_errno(_("Could not write to '%s'"), git_path("MERGE_MSG"));
+		die_errno(_("Could not write to '%s'"), filename);
 	close(fd);
 }
 
 static void read_merge_msg(struct strbuf *msg)
 {
+	const char *filename = git_path("MERGE_MSG");
 	strbuf_reset(msg);
-	if (strbuf_read_file(msg, git_path("MERGE_MSG"), 0) < 0)
-		die_errno(_("Could not read from '%s'"), git_path("MERGE_MSG"));
+	if (strbuf_read_file(msg, filename, 0) < 0)
+		die_errno(_("Could not read from '%s'"), filename);
 }
 
 static void write_merge_state(void);
@@ -948,13 +954,14 @@ static int finish_automerge(struct commit *head,
 
 static int suggest_conflicts(int renormalizing)
 {
+	const char *filename;
 	FILE *fp;
 	int pos;
 
-	fp = fopen(git_path("MERGE_MSG"), "a");
+	filename = git_path("MERGE_MSG");
+	fp = fopen(filename, "a");
 	if (!fp)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("MERGE_MSG"));
+		die_errno(_("Could not open '%s' for writing"), filename);
 	fprintf(fp, "\nConflicts:\n");
 	for (pos = 0; pos < active_nr; pos++) {
 		struct cache_entry *ce = active_cache[pos];
@@ -1046,6 +1053,7 @@ static int setup_with_upstream(const char ***argv)
 
 static void write_merge_state(void)
 {
+	const char *filename;
 	int fd;
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;
@@ -1053,24 +1061,25 @@ static void write_merge_state(void)
 	for (j = remoteheads; j; j = j->next)
 		strbuf_addf(&buf, "%s\n",
 			sha1_to_hex(j->item->object.sha1));
-	fd = open(git_path("MERGE_HEAD"), O_WRONLY | O_CREAT, 0666);
+	filename = git_path("MERGE_HEAD");
+	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("MERGE_HEAD"));
+		die_errno(_("Could not open '%s' for writing"), filename);
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-		die_errno(_("Could not write to '%s'"), git_path("MERGE_HEAD"));
+		die_errno(_("Could not write to '%s'"), filename);
 	close(fd);
 	strbuf_addch(&merge_msg, '\n');
 	write_merge_msg(&merge_msg);
-	fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT | O_TRUNC, 0666);
+
+	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"),
-			  git_path("MERGE_MODE"));
+		die_errno(_("Could not open '%s' for writing"), filename);
 	strbuf_reset(&buf);
 	if (!allow_fast_forward)
 		strbuf_addf(&buf, "no-ff");
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-		die_errno(_("Could not write to '%s'"), git_path("MERGE_MODE"));
+		die_errno(_("Could not write to '%s'"), filename);
 	close(fd);
 }
 
diff --git a/builtin/revert.c b/builtin/revert.c
index 87df70ed..985f95b0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -288,17 +288,18 @@ static char *get_encoding(const char *message)
 
 static void write_cherry_pick_head(struct commit *commit)
 {
+	const char *filename;
 	int fd;
 	struct strbuf buf = STRBUF_INIT;
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
 
-	fd = open(git_path("CHERRY_PICK_HEAD"), O_WRONLY | O_CREAT, 0666);
+	filename = git_path("CHERRY_PICK_HEAD");
+	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  git_path("CHERRY_PICK_HEAD"));
+		die_errno(_("Could not open '%s' for writing"), filename);
 	if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
-		die_errno(_("Could not write to '%s'"), git_path("CHERRY_PICK_HEAD"));
+		die_errno(_("Could not write to '%s'"), filename);
 	strbuf_release(&buf);
 }
 
-- 
1.7.8.rc0

[PATCH 2/3] Bigfile: dynamically allocate buffer for marks file name

[Jonathan Nieder]

This prevents a buffer overrun that could otherwise be triggered by
creating a .git file with a long destination path and trying to "git
add" a file larger than the big-file threshold (which defaults to 512
MiB), ever since v1.7.6-rc0~31^2 (Bigfile: teach "git add" to send a
large file straight to a pack, 2011-05-08).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sha1_file.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 27f3b9b2..86705bc9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2697,20 +2697,28 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,
 			unsigned flags)
 {
 	struct child_process fast_import;
-	char export_marks[512];
-	const char *argv[] = { "fast-import", "--quiet", export_marks, NULL };
-	char tmpfile[512];
+	const char *argv[4];	/* command, two args, NULL */
+	const char **arg;
+	struct strbuf export_marks = STRBUF_INIT;
+	char *tmpfile;
 	char fast_import_cmd[512];
 	char buf[512];
 	int len, tmpfd;
 
-	strcpy(tmpfile, git_path("hashstream_XXXXXX"));
+	strbuf_addstr(&export_marks, "--export-marks=");
+	strbuf_addstr(&export_marks, git_path("hashstream_XXXXXX"));
+	tmpfile = export_marks.buf + strlen("--export-marks=");
 	tmpfd = git_mkstemp_mode(tmpfile, 0600);
 	if (tmpfd < 0)
 		die_errno("cannot create tempfile: %s", tmpfile);
 	if (close(tmpfd))
 		die_errno("cannot close tempfile: %s", tmpfile);
-	sprintf(export_marks, "--export-marks=%s", tmpfile);
+
+	arg = argv;
+	*arg++ = "fast-import";
+	*arg++ = "--quiet";
+	*arg++ = export_marks.buf;
+	*arg++ = NULL;
 
 	memset(&fast_import, 0, sizeof(fast_import));
 	fast_import.in = -1;
@@ -2754,6 +2762,8 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,
 	    memcmp(":1 ", buf, 3) ||
 	    get_sha1_hex(buf + 3, sha1))
 		die_errno("index-stream: unexpected fast-import mark: <%s>", buf);
+
+	strbuf_release(&export_marks);
 	return 0;
 }
 
-- 
1.7.8.rc0

[PATCH 3/3] rename git_path() to git_path_unsafe()

[Jonathan Nieder]

git_path() stores its result to one of a rotating collection of four
static buffers.  If more than 4 git_path() results are in play, the
result can be a little unpleasant, as each call clobbers the return
value from previous calls.

Therefore callers should be careful not to assign the return value
from git_path() to a long-lived variable.  Rename the function to
git_path_unsafe() as a reminder.

Mechanics: This patch only makes three kinds of changes:

 1) changing git_path(foo) to git_path_unsafe(foo)
 2) changing xstrdup(git_path(foo)) to git_pathdup(foo)
 3) rewrapping lines that were made longer by (1)

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 Documentation/technical/api-string-list.txt |    5 +-
 attr.c                                      |    2 +-
 bisect.c                                    |    8 ++--
 branch.c                                    |   12 +++---
 builtin/add.c                               |    2 +-
 builtin/commit.c                            |   57 ++++++++++++++-------------
 builtin/config.c                            |    4 +-
 builtin/fetch-pack.c                        |    4 +-
 builtin/fetch.c                             |    5 +-
 builtin/fsck.c                              |    2 +-
 builtin/init-db.c                           |   12 +++---
 builtin/merge.c                             |   32 +++++++-------
 builtin/notes.c                             |    2 +-
 builtin/remote.c                            |    6 +-
 builtin/reset.c                             |    2 +-
 builtin/revert.c                            |   18 ++++----
 cache.h                                     |    3 +-
 contrib/examples/builtin-fetch--tool.c      |    4 +-
 dir.c                                       |    2 +-
 fast-import.c                               |    2 +-
 http-backend.c                              |    2 +-
 notes-merge.c                               |   22 ++++++-----
 pack-refs.c                                 |    6 +-
 path.c                                      |    2 +-
 refs.c                                      |   51 +++++++++++++-----------
 remote.c                                    |    4 +-
 rerere.c                                    |   12 +++---
 run-command.c                               |    4 +-
 sequencer.c                                 |    6 +-
 server-info.c                               |    2 +-
 sha1_file.c                                 |    4 +-
 shallow.c                                   |    2 +-
 transport.c                                 |    4 +-
 unpack-trees.c                              |    2 +-
 34 files changed, 160 insertions(+), 147 deletions(-)

diff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt
index ce24eb96..446a51ab 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -13,8 +13,9 @@ The caller:
 
 . Initializes the members. You might want to set the flag `strdup_strings`
   if the strings should be strdup()ed. For example, this is necessary
-  when you add something like git_path("..."), since that function returns
-  a static buffer that will change with the next call to git_path().
+  when you add something like git_path_unsafe("..."), since that function
+  returns a static buffer that will change with the next call to
+  git_path_unsafe().
 +
 If you need something advanced, you can manually malloc() the `items`
 member (you need this if you add things later) and you should set the
diff --git a/attr.c b/attr.c
index 76b079f0..afdd6d24 100644
--- a/attr.c
+++ b/attr.c
@@ -529,7 +529,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_unsafe(INFOATTRIBUTES_FILE), 1);
 		if (!elem)
 			elem = xcalloc(1, sizeof(*elem));
 		elem->origin = NULL;
diff --git a/bisect.c b/bisect.c
index 6e186e29..315d22e6 100644
--- a/bisect.c
+++ b/bisect.c
@@ -422,7 +422,7 @@ static int read_bisect_refs(void)
 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_unsafe("BISECT_NAMES");
 	FILE *fp = fopen(filename, "r");
 
 	if (!fp)
@@ -643,7 +643,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 
 static int is_expected_rev(const unsigned char *sha1)
 {
-	const char *filename = git_path("BISECT_EXPECTED_REV");
+	const char *filename = git_path_unsafe("BISECT_EXPECTED_REV");
 	struct stat st;
 	struct strbuf str = STRBUF_INIT;
 	FILE *fp;
@@ -668,7 +668,7 @@ static int is_expected_rev(const unsigned char *sha1)
 static void mark_expected_rev(char *bisect_rev_hex)
 {
 	int len = strlen(bisect_rev_hex);
-	const char *filename = git_path("BISECT_EXPECTED_REV");
+	const char *filename = git_path_unsafe("BISECT_EXPECTED_REV");
 	int fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
 
 	if (fd < 0)
@@ -833,7 +833,7 @@ static int check_ancestors(const char *prefix)
  */
 static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)
 {
-	const char *filename = git_path("BISECT_ANCESTORS_OK");
+	const char *filename = git_path_unsafe("BISECT_ANCESTORS_OK");
 	struct stat st;
 	int fd;
 
diff --git a/branch.c b/branch.c
index d8098762..5a3faa10 100644
--- a/branch.c
+++ b/branch.c
@@ -240,11 +240,11 @@ void create_branch(const char *head,
 
 void remove_branch_state(void)
 {
-	unlink(git_path("CHERRY_PICK_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_unsafe("CHERRY_PICK_HEAD"));
+	unlink(git_path_unsafe("MERGE_HEAD"));
+	unlink(git_path_unsafe("MERGE_RR"));
+	unlink(git_path_unsafe("MERGE_MSG"));
+	unlink(git_path_unsafe("MERGE_MODE"));
+	unlink(git_path_unsafe("SQUASH_MSG"));
 	remove_sequencer_state(0);
 }
diff --git a/builtin/add.c b/builtin/add.c
index c59b0c98..0aabb61d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -259,7 +259,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
 
 static int edit_patch(int argc, const char **argv, const char *prefix)
 {
-	char *file = xstrdup(git_path("ADD_EDIT.patch"));
+	char *file = git_pathdup("ADD_EDIT.patch");
 	const char *apply_argv[] = { "apply", "--recount", "--cached",
 		NULL, NULL };
 	struct child_process child;
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d18..e9aa5e75 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -178,9 +178,9 @@ static struct option builtin_commit_options[] = {
 
 static void determine_whence(struct wt_status *s)
 {
-	if (file_exists(git_path("MERGE_HEAD")))
+	if (file_exists(git_path_unsafe("MERGE_HEAD")))
 		whence = FROM_MERGE;
-	else if (file_exists(git_path("CHERRY_PICK_HEAD")))
+	else if (file_exists(git_path_unsafe("CHERRY_PICK_HEAD")))
 		whence = FROM_CHERRY_PICK;
 	else
 		whence = FROM_COMMIT;
@@ -465,8 +465,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,
 		die(_("unable to write new_index file"));
 
 	fd = hold_lock_file_for_update(&false_lock,
-				       git_path("next-index-%"PRIuMAX,
-						(uintmax_t) getpid()),
+				       git_path_unsafe("next-index-%"PRIuMAX,
+							(uintmax_t) getpid()),
 				       LOCK_DIE_ON_ERROR);
 
 	create_base_index(current_head);
@@ -691,12 +691,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_unsafe("MERGE_MSG"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path_unsafe("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_unsafe("SQUASH_MSG"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path_unsafe("SQUASH_MSG"), 0) < 0)
 			die_errno(_("could not read SQUASH_MSG"));
 		hook_arg1 = "squash";
 	} else if (template_file) {
@@ -727,9 +727,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	s->fp = fopen(git_path(commit_editmsg), "w");
+	s->fp = fopen(git_path_unsafe(commit_editmsg), "w");
 	if (s->fp == NULL)
-		die_errno(_("could not open '%s'"), git_path(commit_editmsg));
+		die_errno(_("could not open '%s'"),
+			  git_path_unsafe(commit_editmsg));
 
 	if (clean_message_contents)
 		stripspace(&sb, 0);
@@ -773,7 +774,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				"and try again.\n"
 				""),
 				whence_s(),
-				git_path(whence == FROM_MERGE
+				git_path_unsafe(whence == FROM_MERGE
 					 ? "MERGE_HEAD"
 					 : "CHERRY_PICK_HEAD"));
 
@@ -870,8 +871,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		return 0;
 	}
 
-	if (run_hook(index_file, "prepare-commit-msg",
-		     git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))
+	if (run_hook(index_file,
+		     "prepare-commit-msg", git_path_unsafe(commit_editmsg),
+		     hook_arg1, hook_arg2, NULL))
 		return 0;
 
 	if (use_editor) {
@@ -879,7 +881,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		const char *env[2] = { NULL };
 		env[0] =  index;
 		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-		if (launch_editor(git_path(commit_editmsg), NULL, env)) {
+		if (launch_editor(git_path_unsafe(commit_editmsg), NULL, env)) {
 			fprintf(stderr,
 			_("Please supply the message using either -m or -F option.\n"));
 			exit(1);
@@ -887,7 +889,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 
 	if (!no_verify &&
-	    run_hook(index_file, "commit-msg", git_path(commit_editmsg), NULL)) {
+	    run_hook(index_file,
+		     "commit-msg", git_path_unsafe(commit_editmsg), NULL)) {
 		return 0;
 	}
 
@@ -1347,10 +1350,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	int code;
 	size_t n;
 
-	if (access(git_path(post_rewrite_hook), X_OK) < 0)
+	if (access(git_path_unsafe(post_rewrite_hook), X_OK) < 0)
 		return 0;
 
-	argv[0] = git_path(post_rewrite_hook);
+	argv[0] = git_path_unsafe(post_rewrite_hook);
 	argv[1] = "amend";
 	argv[2] = NULL;
 
@@ -1431,10 +1434,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_unsafe("MERGE_HEAD"), "r");
 		if (fp == NULL)
 			die_errno(_("could not open '%s' for reading"),
-				  git_path("MERGE_HEAD"));
+				  git_path_unsafe("MERGE_HEAD"));
 		while (strbuf_getline(&m, fp, '\n') != EOF) {
 			unsigned char sha1[20];
 			if (get_sha1_hex(m.buf, sha1) < 0)
@@ -1444,8 +1447,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_unsafe("MERGE_MODE"), &statbuf)) {
+			if (strbuf_read_file(&sb, git_path_unsafe("MERGE_MODE"), 0) < 0)
 				die_errno(_("could not read MERGE_MODE"));
 			if (!strcmp(sb.buf, "no-ff"))
 				allow_fast_forward = 0;
@@ -1462,7 +1465,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 
 	/* Finally, get the commit message */
 	strbuf_reset(&sb);
-	if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {
+	if (strbuf_read_file(&sb, git_path_unsafe(commit_editmsg), 0) < 0) {
 		int saved_errno = errno;
 		rollback_index_files();
 		die(_("could not read commit message: %s"), strerror(saved_errno));
@@ -1513,11 +1516,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		die(_("cannot update HEAD ref"));
 	}
 
-	unlink(git_path("CHERRY_PICK_HEAD"));
-	unlink(git_path("MERGE_HEAD"));
-	unlink(git_path("MERGE_MSG"));
-	unlink(git_path("MERGE_MODE"));
-	unlink(git_path("SQUASH_MSG"));
+	unlink(git_path_unsafe("CHERRY_PICK_HEAD"));
+	unlink(git_path_unsafe("MERGE_HEAD"));
+	unlink(git_path_unsafe("MERGE_MSG"));
+	unlink(git_path_unsafe("MERGE_MODE"));
+	unlink(git_path_unsafe("SQUASH_MSG"));
 
 	if (commit_index_files())
 		die (_("Repository has been updated, but unable to write\n"
diff --git a/builtin/config.c b/builtin/config.c
index 0315ad76..407d7ca8 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -434,8 +434,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 			die("not in a git directory");
 		git_config(git_default_config, NULL);
 		launch_editor(config_exclusive_filename ?
-			      config_exclusive_filename : git_path("config"),
-			      NULL, NULL);
+			      config_exclusive_filename :
+			      git_path_unsafe("config"), NULL, NULL);
 	}
 	else if (actions == ACTION_SET) {
 		int ret;
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb0..f8d0954c 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1026,7 +1026,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 	if (&args != my_args)
 		memcpy(&args, my_args, sizeof(args));
 	if (args.depth > 0) {
-		if (stat(git_path("shallow"), &st))
+		if (stat(git_path_unsafe("shallow"), &st))
 			st.st_mtime = 0;
 	}
 
@@ -1041,7 +1041,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 	if (args.depth > 0) {
 		struct cache_time mtime;
 		struct strbuf sb = STRBUF_INIT;
-		char *shallow = git_path("shallow");
+		char *shallow = git_path_unsafe("shallow");
 		int fd;
 
 		mtime.sec = st.st_mtime;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 91731b90..ccbe63c7 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -367,7 +367,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
-	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	char *url;
+	char *filename = dry_run ? "/dev/null" : git_path_unsafe("FETCH_HEAD");
 
 	fp = fopen(filename, "a");
 	if (!fp)
@@ -647,7 +648,7 @@ static void check_not_current_branch(struct ref *ref_map)
 
 static int truncate_fetch_head(void)
 {
-	char *filename = git_path("FETCH_HEAD");
+	char *filename = git_path_unsafe("FETCH_HEAD");
 	FILE *fp = fopen(filename, "w");
 
 	if (!fp)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index df1a88b5..f8429f55 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -215,7 +215,7 @@ 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) {
-			char *filename = git_path("lost-found/%s/%s",
+			char *filename = git_path_unsafe("lost-found/%s/%s",
 				obj->type == OBJ_COMMIT ? "commit" : "other",
 				sha1_to_hex(obj->sha1));
 			FILE *f;
diff --git a/builtin/init-db.c b/builtin/init-db.c
index d07554c8..7ae89f85 100644
--- a/builtin/init-db.c
+++ b/builtin/init-db.c
@@ -198,9 +198,9 @@ static int create_default_files(const char *template_path)
 	/*
 	 * Create .git/refs/{heads,tags}
 	 */
-	safe_create_dir(git_path("refs"), 1);
-	safe_create_dir(git_path("refs/heads"), 1);
-	safe_create_dir(git_path("refs/tags"), 1);
+	safe_create_dir(git_path_unsafe("refs"), 1);
+	safe_create_dir(git_path_unsafe("refs/heads"), 1);
+	safe_create_dir(git_path_unsafe("refs/tags"), 1);
 
 	/* Just look for `init.templatedir` */
 	git_config(git_init_db_config, NULL);
@@ -224,9 +224,9 @@ static int create_default_files(const char *template_path)
 	 */
 	if (shared_repository) {
 		adjust_shared_perm(get_git_dir());
-		adjust_shared_perm(git_path("refs"));
-		adjust_shared_perm(git_path("refs/heads"));
-		adjust_shared_perm(git_path("refs/tags"));
+		adjust_shared_perm(git_path_unsafe("refs"));
+		adjust_shared_perm(git_path_unsafe("refs/heads"));
+		adjust_shared_perm(git_path_unsafe("refs/tags"));
 	}
 
 	/*
diff --git a/builtin/merge.c b/builtin/merge.c
index 2870a6af..3e75c30b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -213,9 +213,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_unsafe("MERGE_HEAD"));
+	unlink(git_path_unsafe("MERGE_MSG"));
+	unlink(git_path_unsafe("MERGE_MODE"));
 }
 
 static int save_state(unsigned char *stash)
@@ -321,7 +321,7 @@ static void squash_message(struct commit *commit)
 	struct pretty_print_context ctx = {0};
 
 	printf(_("Squash commit -- not updating HEAD\n"));
-	filename = git_path("SQUASH_MSG");
+	filename = git_path_unsafe("SQUASH_MSG");
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not write to '%s'"), filename);
@@ -493,13 +493,13 @@ static void merge_name(const char *remote, struct strbuf *msg)
 	}
 
 	if (!strcmp(remote, "FETCH_HEAD") &&
-			!access(git_path("FETCH_HEAD"), R_OK)) {
+			!access(git_path_unsafe("FETCH_HEAD"), R_OK)) {
 		const char *filename;
 		FILE *fp;
 		struct strbuf line = STRBUF_INIT;
 		char *ptr;
 
-		filename = git_path("FETCH_HEAD");
+		filename = git_path_unsafe("FETCH_HEAD");
 		fp = fopen(filename, "r");
 		if (!fp)
 			die_errno(_("could not open '%s' for reading"),
@@ -851,7 +851,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_unsafe("MERGE_MSG");
 	int fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"),
@@ -863,7 +863,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_unsafe("MERGE_MSG");
 	strbuf_reset(msg);
 	if (strbuf_read_file(msg, filename, 0) < 0)
 		die_errno(_("Could not read from '%s'"), filename);
@@ -887,9 +887,9 @@ static void prepare_to_commit(void)
 	strbuf_addch(&msg, '\n');
 	write_merge_msg(&msg);
 	run_hook(get_index_file(), "prepare-commit-msg",
-		 git_path("MERGE_MSG"), "merge", NULL, NULL);
+		 git_path_unsafe("MERGE_MSG"), "merge", NULL, NULL);
 	if (option_edit) {
-		if (launch_editor(git_path("MERGE_MSG"), NULL, NULL))
+		if (launch_editor(git_path_unsafe("MERGE_MSG"), NULL, NULL))
 			abort_commit(NULL);
 	}
 	read_merge_msg(&msg);
@@ -958,7 +958,7 @@ static int suggest_conflicts(int renormalizing)
 	FILE *fp;
 	int pos;
 
-	filename = git_path("MERGE_MSG");
+	filename = git_path_unsafe("MERGE_MSG");
 	fp = fopen(filename, "a");
 	if (!fp)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -1061,7 +1061,7 @@ static void write_merge_state(void)
 	for (j = remoteheads; j; j = j->next)
 		strbuf_addf(&buf, "%s\n",
 			sha1_to_hex(j->item->object.sha1));
-	filename = git_path("MERGE_HEAD");
+	filename = git_path_unsafe("MERGE_HEAD");
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -1071,7 +1071,7 @@ static void write_merge_state(void)
 	strbuf_addch(&merge_msg, '\n');
 	write_merge_msg(&merge_msg);
 
-	filename = git_path("MERGE_MODE");
+	filename = git_path_unsafe("MERGE_MODE");
 	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -1126,7 +1126,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_unsafe("MERGE_HEAD")))
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
 		/* Invoke 'git reset --merge' */
@@ -1136,7 +1136,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_unsafe("MERGE_HEAD"))) {
 		/*
 		 * There is no unmerged entry, don't advise 'git
 		 * add/rm <file>', just 'git commit'.
@@ -1147,7 +1147,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_unsafe("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 can merge."));
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437db..c037afe6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -944,7 +944,7 @@ static int merge(int argc, const char **argv, const char *prefix)
 		printf("Automatic notes merge failed. Fix conflicts in %s and "
 		       "commit the result with 'git notes merge --commit', or "
 		       "abort the merge with 'git notes merge --abort'.\n",
-		       git_path(NOTES_MERGE_WORKTREE));
+		       git_path_unsafe(NOTES_MERGE_WORKTREE));
 	}
 
 	free_notes(t);
diff --git a/builtin/remote.c b/builtin/remote.c
index c8106438..c7032125 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -546,7 +546,7 @@ static int add_branch_for_removal(const char *refname,
 
 	/* make sure that symrefs are deleted */
 	if (flags & REF_ISSYMREF)
-		return unlink(git_path("%s", refname));
+		return unlink(git_path_unsafe("%s", refname));
 
 	item = string_list_append(branches->branches, refname);
 	item->util = xmalloc(20);
@@ -608,9 +608,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);
+		path = git_path_unsafe("remotes/%s", remote->name);
 	else if (remote->origin == REMOTE_BRANCHES)
-		path = git_path("branches/%s", remote->name);
+		path = git_path_unsafe("branches/%s", remote->name);
 	if (path)
 		unlink_or_warn(path);
 	return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 811e8e25..18bacdac 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -35,7 +35,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_unsafe("MERGE_HEAD"), F_OK);
 }
 
 static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)
diff --git a/builtin/revert.c b/builtin/revert.c
index 985f95b0..09a062c6 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -294,7 +294,7 @@ static void write_cherry_pick_head(struct commit *commit)
 
 	strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
 
-	filename = git_path("CHERRY_PICK_HEAD");
+	filename = git_path_unsafe("CHERRY_PICK_HEAD");
 	fd = open(filename, O_WRONLY | O_CREAT, 0666);
 	if (fd < 0)
 		die_errno(_("Could not open '%s' for writing"), filename);
@@ -314,7 +314,7 @@ static void print_advice(int show_hint)
 		 * (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_unsafe("CHERRY_PICK_HEAD"));
 		return;
 	}
 
@@ -762,7 +762,7 @@ 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);
+	const char *todo_file = git_path_unsafe(SEQ_TODO_FILE);
 	struct strbuf buf = STRBUF_INIT;
 	int fd, res;
 
@@ -817,7 +817,7 @@ 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);
+	const char *opts_file = git_path_unsafe(SEQ_OPTS_FILE);
 
 	if (!file_exists(opts_file))
 		return;
@@ -841,7 +841,7 @@ 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);
+	const char *seq_dir = git_path_unsafe(SEQ_DIR);
 
 	if (file_exists(seq_dir))
 		return error(_("%s already exists."), seq_dir);
@@ -852,7 +852,7 @@ static int create_seq_dir(void)
 
 static void save_head(const char *head)
 {
-	const char *head_file = git_path(SEQ_HEAD_FILE);
+	const char *head_file = git_path_unsafe(SEQ_HEAD_FILE);
 	static struct lock_file head_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
@@ -867,7 +867,7 @@ static void save_head(const char *head)
 
 static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 {
-	const char *todo_file = git_path(SEQ_TODO_FILE);
+	const char *todo_file = git_path_unsafe(SEQ_TODO_FILE);
 	static struct lock_file todo_lock;
 	struct strbuf buf = STRBUF_INIT;
 	int fd;
@@ -888,7 +888,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
 
 static void save_opts(struct replay_opts *opts)
 {
-	const char *opts_file = git_path(SEQ_OPTS_FILE);
+	const char *opts_file = git_path_unsafe(SEQ_OPTS_FILE);
 
 	if (opts->no_commit)
 		git_config_set_in_file(opts_file, "options.no-commit", "true");
@@ -969,7 +969,7 @@ static int pick_revisions(struct replay_opts *opts)
 		remove_sequencer_state(1);
 		return 0;
 	} else if (opts->subcommand == REPLAY_CONTINUE) {
-		if (!file_exists(git_path(SEQ_TODO_FILE)))
+		if (!file_exists(git_path_unsafe(SEQ_TODO_FILE)))
 			goto error;
 		read_populate_opts(&opts);
 		read_populate_todo(&todo_list, opts);
diff --git a/cache.h b/cache.h
index 2e6ad360..7fb85445 100644
--- a/cache.h
+++ b/cache.h
@@ -662,7 +662,8 @@ extern char *git_pathdup(const char *fmt, ...)
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+extern char *git_path_unsafe(const char *fmt, ...)
+	__attribute__((format (printf, 1, 2)));
 extern char *git_path_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 
diff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c
index 3140e405..ea18fdac 100644
--- a/contrib/examples/builtin-fetch--tool.c
+++ b/contrib/examples/builtin-fetch--tool.c
@@ -515,7 +515,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_unsafe("FETCH_HEAD");
 		fp = fopen(filename, "a");
 		if (!fp)
 			return error("cannot open %s: %s\n", filename, strerror(errno));
@@ -533,7 +533,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_unsafe("FETCH_HEAD");
 		fp = fopen(filename, "a");
 		if (!fp)
 			return error("cannot open %s: %s\n", filename, strerror(errno));
diff --git a/dir.c b/dir.c
index 6c0d7825..94662509 100644
--- a/dir.c
+++ b/dir.c
@@ -1224,7 +1224,7 @@ void setup_standard_excludes(struct dir_struct *dir)
 	const char *path;
 
 	dir->exclude_per_dir = ".gitignore";
-	path = git_path("info/exclude");
+	path = git_path_unsafe("info/exclude");
 	if (!access(path, R_OK))
 		add_excludes_from_file(dir, path);
 	if (excludes_file && !access(excludes_file, R_OK))
diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c4..04bcd353 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -403,7 +403,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
 
 static void write_crash_report(const char *err)
 {
-	char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
+	char *loc = git_path_unsafe("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
 	FILE *rpt = fopen(loc, "w");
 	struct branch *b;
 	unsigned long lu;
diff --git a/http-backend.c b/http-backend.c
index 59ad7da6..7169e040 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -161,7 +161,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);
+	const char *p = git_path_unsafe("%s", name);
 	size_t buf_alloc = 8192;
 	char *buf = xmalloc(buf_alloc);
 	int fd;
diff --git a/notes-merge.c b/notes-merge.c
index e9e41993..0b49e8ad 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -275,35 +275,37 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 		 * Must establish NOTES_MERGE_WORKTREE.
 		 * Abort if NOTES_MERGE_WORKTREE already exists
 		 */
-		if (file_exists(git_path(NOTES_MERGE_WORKTREE))) {
+		if (file_exists(git_path_unsafe(NOTES_MERGE_WORKTREE))) {
 			if (advice_resolve_conflict)
 				die("You have not concluded your previous "
 				    "notes merge (%s exists).\nPlease, use "
 				    "'git notes merge --commit' or 'git notes "
 				    "merge --abort' to commit/abort the "
 				    "previous merge before you start a new "
-				    "notes merge.", git_path("NOTES_MERGE_*"));
+				    "notes merge.",
+				    git_path_unsafe("NOTES_MERGE_*"));
 			else
 				die("You have not concluded your notes merge "
-				    "(%s exists).", git_path("NOTES_MERGE_*"));
+				    "(%s exists).",
+				    git_path_unsafe("NOTES_MERGE_*"));
 		}
 
-		if (safe_create_leading_directories(git_path(
+		if (safe_create_leading_directories(git_path_unsafe(
 				NOTES_MERGE_WORKTREE "/.test")))
 			die_errno("unable to create directory %s",
-				  git_path(NOTES_MERGE_WORKTREE));
+				  git_path_unsafe(NOTES_MERGE_WORKTREE));
 		o->has_worktree = 1;
-	} else if (!file_exists(git_path(NOTES_MERGE_WORKTREE)))
+	} else if (!file_exists(git_path_unsafe(NOTES_MERGE_WORKTREE)))
 		/* NOTES_MERGE_WORKTREE should already be established */
 		die("missing '%s'. This should not happen",
-		    git_path(NOTES_MERGE_WORKTREE));
+		    git_path_unsafe(NOTES_MERGE_WORKTREE));
 }
 
 static void write_buf_to_worktree(const unsigned char *obj,
 				  const char *buf, unsigned long size)
 {
 	int fd;
-	char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
+	char *path = git_path_unsafe(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
 	if (safe_create_leading_directories(path))
 		die_errno("unable to create directory for '%s'", path);
 	if (file_exists(path))
@@ -681,7 +683,7 @@ int notes_merge_commit(struct notes_merge_options *o,
 	 * Finally store the new commit object SHA1 into 'result_sha1'.
 	 */
 	struct dir_struct dir;
-	char *path = xstrdup(git_path(NOTES_MERGE_WORKTREE "/"));
+	char *path = git_pathdup(NOTES_MERGE_WORKTREE "/");
 	int path_len = strlen(path), i;
 	const char *msg = strstr(partial_commit->buffer, "\n\n");
 
@@ -731,7 +733,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));
+	strbuf_addstr(&buf, git_path_unsafe(NOTES_MERGE_WORKTREE));
 	OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
 	ret = remove_dir_recursively(&buf, 0);
 	strbuf_release(&buf);
diff --git a/pack-refs.c b/pack-refs.c
index 23bbd00e..9557b063 100644
--- a/pack-refs.c
+++ b/pack-refs.c
@@ -86,7 +86,7 @@ static void try_remove_empty_parents(char *name)
 		if (q == p)
 			break;
 		*q = '\0';
-		if (rmdir(git_path("%s", name)))
+		if (rmdir(git_path_unsafe("%s", name)))
 			break;
 	}
 }
@@ -97,7 +97,7 @@ static void prune_ref(struct ref_to_prune *r)
 	struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
 
 	if (lock) {
-		unlink_or_warn(git_path("%s", r->name));
+		unlink_or_warn(git_path_unsafe("%s", r->name));
 		unlock_ref(lock);
 		try_remove_empty_parents(r->name);
 	}
@@ -121,7 +121,7 @@ int pack_refs(unsigned int flags)
 	memset(&cbdata, 0, sizeof(cbdata));
 	cbdata.flags = flags;
 
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"),
+	fd = hold_lock_file_for_update(&packed, git_path_unsafe("packed-refs"),
 				       LOCK_DIE_ON_ERROR);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
diff --git a/path.c b/path.c
index b6f71d10..0611b7be 100644
--- a/path.c
+++ b/path.c
@@ -101,7 +101,7 @@ char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
-char *git_path(const char *fmt, ...)
+char *git_path_unsafe(const char *fmt, ...)
 {
 	const char *git_dir = get_git_dir();
 	char *pathname = get_pathname();
diff --git a/refs.c b/refs.c
index e69ba26b..e527c7b7 100644
--- a/refs.c
+++ b/refs.c
@@ -268,7 +268,7 @@ static struct ref_array *get_packed_refs(const char *submodule)
 		if (submodule)
 			packed_refs_file = git_path_submodule(submodule, "packed-refs");
 		else
-			packed_refs_file = git_path("packed-refs");
+			packed_refs_file = git_path_unsafe("packed-refs");
 		f = fopen(packed_refs_file, "r");
 		if (f) {
 			read_packed_refs(f, &refs->packed);
@@ -288,7 +288,7 @@ static void get_ref_dir(const char *submodule, const char *base,
 	if (submodule)
 		path = git_path_submodule(submodule, "%s", base);
 	else
-		path = git_path("%s", base);
+		path = git_path_unsafe("%s", base);
 
 
 	dir = opendir(path);
@@ -319,7 +319,7 @@ static void get_ref_dir(const char *submodule, const char *base,
 			memcpy(ref + baselen, de->d_name, namelen+1);
 			refdir = submodule
 				? git_path_submodule(submodule, "%s", ref)
-				: git_path("%s", ref);
+				: git_path_unsafe("%s", ref);
 			if (stat(refdir, &st) < 0)
 				continue;
 			if (S_ISDIR(st.st_mode)) {
@@ -1146,11 +1146,11 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		ref = resolve_ref(path, hash, 1, NULL);
 		if (!ref)
 			continue;
-		if (!stat(git_path("logs/%s", path), &st) &&
+		if (!stat(git_path_unsafe("logs/%s", path), &st) &&
 		    S_ISREG(st.st_mode))
 			it = path;
 		else if (strcmp(ref, path) &&
-			 !stat(git_path("logs/%s", ref), &st) &&
+			 !stat(git_path_unsafe("logs/%s", ref), &st) &&
 			 S_ISREG(st.st_mode))
 			it = ref;
 		else
@@ -1186,7 +1186,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 		 * it is normal for the empty directory 'foo'
 		 * to remain.
 		 */
-		ref_file = git_path("%s", orig_ref);
+		ref_file = git_path_unsafe("%s", orig_ref);
 		if (remove_empty_directories(ref_file)) {
 			last_errno = errno;
 			error("there are still refs under '%s'", orig_ref);
@@ -1223,7 +1223,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	}
 	lock->ref_name = xstrdup(ref);
 	lock->orig_ref_name = xstrdup(orig_ref);
-	ref_file = git_path("%s", ref);
+	ref_file = git_path_unsafe("%s", ref);
 	if (missing)
 		lock->force_write = 1;
 	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
@@ -1272,9 +1272,10 @@ static int repack_without_ref(const char *refname)
 	ref = search_ref_array(packed, refname);
 	if (ref == NULL)
 		return 0;
-	fd = hold_lock_file_for_update(&packlock, git_path("packed-refs"), 0);
+	fd = hold_lock_file_for_update(&packlock,
+				       git_path_unsafe("packed-refs"), 0);
 	if (fd < 0) {
-		unable_to_lock_error(git_path("packed-refs"), errno);
+		unable_to_lock_error(git_path_unsafe("packed-refs"), errno);
 		return error("cannot delete '%s' from packed refs", refname);
 	}
 
@@ -1313,7 +1314,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 			lock->lk->filename[i] = 0;
 			path = lock->lk->filename;
 		} else {
-			path = git_path("%s", refname);
+			path = git_path_unsafe("%s", refname);
 		}
 		err = unlink_or_warn(path);
 		if (err && errno != ENOENT)
@@ -1328,7 +1329,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	 */
 	ret |= repack_without_ref(refname);
 
-	unlink_or_warn(git_path("logs/%s", lock->ref_name));
+	unlink_or_warn(git_path_unsafe("logs/%s", lock->ref_name));
 	invalidate_ref_cache(NULL);
 	unlock_ref(lock);
 	return ret;
@@ -1349,7 +1350,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	int flag = 0, logmoved = 0;
 	struct ref_lock *lock;
 	struct stat loginfo;
-	int log = !lstat(git_path("logs/%s", oldref), &loginfo);
+	int log = !lstat(git_path_unsafe("logs/%s", oldref), &loginfo);
 	const char *symref = NULL;
 
 	if (log && S_ISLNK(loginfo.st_mode))
@@ -1368,7 +1369,8 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	if (!is_refname_available(newref, oldref, get_loose_refs(NULL), 0))
 		return 1;
 
-	if (log && rename(git_path("logs/%s", oldref), git_path(TMP_RENAMED_LOG)))
+	if (log && rename(git_path_unsafe("logs/%s", oldref),
+			  git_path_unsafe(TMP_RENAMED_LOG)))
 		return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
 			oldref, strerror(errno));
 
@@ -1379,7 +1381,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 
 	if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
 		if (errno==EISDIR) {
-			if (remove_empty_directories(git_path("%s", newref))) {
+			if (remove_empty_directories(git_path_unsafe("%s", newref))) {
 				error("Directory not empty: %s", newref);
 				goto rollback;
 			}
@@ -1389,20 +1391,21 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		}
 	}
 
-	if (log && safe_create_leading_directories(git_path("logs/%s", newref))) {
+	if (log && safe_create_leading_directories(git_path_unsafe("logs/%s", newref))) {
 		error("unable to create directory for %s", newref);
 		goto rollback;
 	}
 
  retry:
-	if (log && rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", newref))) {
+	if (log && rename(git_path_unsafe(TMP_RENAMED_LOG),
+			  git_path_unsafe("logs/%s", newref))) {
 		if (errno==EISDIR || errno==ENOTDIR) {
 			/*
 			 * 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", newref))) {
+			if (remove_empty_directories(git_path_unsafe("logs/%s", newref))) {
 				error("Directory not empty: logs/%s", newref);
 				goto rollback;
 			}
@@ -1444,11 +1447,13 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	log_all_ref_updates = flag;
 
  rollbacklog:
-	if (logmoved && rename(git_path("logs/%s", newref), git_path("logs/%s", oldref)))
+	if (logmoved && rename(git_path_unsafe("logs/%s", newref),
+			       git_path_unsafe("logs/%s", oldref)))
 		error("unable to restore logfile %s from %s: %s",
 			oldref, newref, strerror(errno));
 	if (!logmoved && log &&
-	    rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldref)))
+	    rename(git_path_unsafe(TMP_RENAMED_LOG),
+		   git_path_unsafe("logs/%s", oldref)))
 		error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
 			oldref, strerror(errno));
 
@@ -1741,7 +1746,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *
 	void *log_mapped;
 	size_t mapsz;
 
-	logfile = git_path("logs/%s", ref);
+	logfile = git_path_unsafe("logs/%s", ref);
 	logfd = open(logfile, O_RDONLY, 0);
 	if (logfd < 0)
 		die_errno("Unable to read log '%s'", logfile);
@@ -1841,7 +1846,7 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,
 	struct strbuf sb = STRBUF_INIT;
 	int ret = 0;
 
-	logfile = git_path("logs/%s", ref);
+	logfile = git_path_unsafe("logs/%s", ref);
 	logfp = fopen(logfile, "r");
 	if (!logfp)
 		return -1;
@@ -1899,7 +1904,7 @@ int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)
 
 static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)
 {
-	DIR *dir = opendir(git_path("logs/%s", base));
+	DIR *dir = opendir(git_path_unsafe("logs/%s", base));
 	int retval = 0;
 
 	if (dir) {
@@ -1923,7 +1928,7 @@ static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)
 			if (has_extension(de->d_name, ".lock"))
 				continue;
 			memcpy(log + baselen, de->d_name, namelen+1);
-			if (stat(git_path("logs/%s", log), &st) < 0)
+			if (stat(git_path_unsafe("logs/%s", log), &st) < 0)
 				continue;
 			if (S_ISDIR(st.st_mode)) {
 				retval = do_for_each_reflog(log, fn, cb_data);
diff --git a/remote.c b/remote.c
index e2ef9911..bb85b326 100644
--- a/remote.c
+++ b/remote.c
@@ -224,7 +224,7 @@ static void add_instead_of(struct rewrite *rewrite, const char *instead_of)
 
 static void read_remotes_file(struct remote *remote)
 {
-	FILE *f = fopen(git_path("remotes/%s", remote->name), "r");
+	FILE *f = fopen(git_path_unsafe("remotes/%s", remote->name), "r");
 
 	if (!f)
 		return;
@@ -275,7 +275,7 @@ static void read_branches_file(struct remote *remote)
 	char *frag;
 	struct strbuf branch = STRBUF_INIT;
 	int n = slash ? slash - remote->name : 1000;
-	FILE *f = fopen(git_path("branches/%.*s", n, remote->name), "r");
+	FILE *f = fopen(git_path_unsafe("branches/%.*s", n, remote->name), "r");
 	char *s, *p;
 	int len;
 
diff --git a/rerere.c b/rerere.c
index dcb525a4..61f60701 100644
--- a/rerere.c
+++ b/rerere.c
@@ -22,7 +22,7 @@ static char *merge_rr_path;
 
 const char *rerere_path(const char *hex, const char *file)
 {
-	return git_path("rr-cache/%s/%s", hex, file);
+	return git_path_unsafe("rr-cache/%s/%s", hex, file);
 }
 
 int has_rerere_resolution(const char *hex)
@@ -524,7 +524,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)
 				continue;
 			hex = xstrdup(sha1_to_hex(sha1));
 			string_list_insert(rr, path)->util = hex;
-			if (mkdir(git_path("rr-cache/%s", hex), 0755))
+			if (mkdir(git_path_unsafe("rr-cache/%s", hex), 0755))
 				continue;
 			handle_file(path, NULL, rerere_path(hex, "preimage"));
 			fprintf(stderr, "Recorded preimage for '%s'\n", path);
@@ -591,7 +591,7 @@ static int is_rerere_enabled(void)
 	if (!rerere_enabled)
 		return 0;
 
-	rr_cache = git_path("rr-cache");
+	rr_cache = git_path_unsafe("rr-cache");
 	rr_cache_exists = is_directory(rr_cache);
 	if (rerere_enabled < 0)
 		return rr_cache_exists;
@@ -695,7 +695,7 @@ static void unlink_rr_item(const char *name)
 	unlink(rerere_path(name, "thisimage"));
 	unlink(rerere_path(name, "preimage"));
 	unlink(rerere_path(name, "postimage"));
-	rmdir(git_path("rr-cache/%s", name));
+	rmdir(git_path_unsafe("rr-cache/%s", name));
 }
 
 struct rerere_gc_config_cb {
@@ -726,7 +726,7 @@ void rerere_gc(struct string_list *rr)
 	struct rerere_gc_config_cb cf = { 15, 60 };
 
 	git_config(git_rerere_gc_config, &cf);
-	dir = opendir(git_path("rr-cache"));
+	dir = opendir(git_path_unsafe("rr-cache"));
 	if (!dir)
 		die_errno("unable to open rr-cache directory");
 	while ((e = readdir(dir))) {
@@ -760,5 +760,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_unsafe("MERGE_RR"));
 }
diff --git a/run-command.c b/run-command.c
index 1c510438..598e41dd 100644
--- a/run-command.c
+++ b/run-command.c
@@ -612,11 +612,11 @@ int run_hook(const char *index_file, const char *name, ...)
 	va_list args;
 	int ret;
 
-	if (access(git_path("hooks/%s", name), X_OK) < 0)
+	if (access(git_path_unsafe("hooks/%s", name), X_OK) < 0)
 		return 0;
 
 	va_start(args, name);
-	argv_array_push(&argv, git_path("hooks/%s", name));
+	argv_array_push(&argv, git_path_unsafe("hooks/%s", name));
 	while ((p = va_arg(args, const char *)))
 		argv_array_push(&argv, p);
 	va_end(args);
diff --git a/sequencer.c b/sequencer.c
index bc2c046a..2e29152c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -8,10 +8,10 @@ void remove_sequencer_state(int aggressive)
 	struct strbuf seq_dir = STRBUF_INIT;
 	struct strbuf seq_old_dir = STRBUF_INIT;
 
-	strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
-	strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
+	strbuf_addf(&seq_dir, "%s", git_path_unsafe(SEQ_DIR));
+	strbuf_addf(&seq_old_dir, "%s", git_path_unsafe(SEQ_OLD_DIR));
 	remove_dir_recursively(&seq_old_dir, 0);
-	rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
+	rename(git_path_unsafe(SEQ_DIR), git_path_unsafe(SEQ_OLD_DIR));
 	if (aggressive)
 		remove_dir_recursively(&seq_old_dir, 0);
 	strbuf_release(&seq_dir);
diff --git a/server-info.c b/server-info.c
index 9ec744e9..348a3447 100644
--- a/server-info.c
+++ b/server-info.c
@@ -243,7 +243,7 @@ int update_server_info(int force)
 	errs = errs | update_info_packs(force);
 
 	/* remove leftover rev-cache file if there is any */
-	unlink_or_warn(git_path("info/rev-cache"));
+	unlink_or_warn(git_path_unsafe("info/rev-cache"));
 
 	return errs;
 }
diff --git a/sha1_file.c b/sha1_file.c
index 86705bc9..ba7eca89 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -382,7 +382,7 @@ static 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);
+	int fd = hold_lock_file_for_append(lock, git_path_unsafe("objects/info/alternates"), LOCK_DIE_ON_ERROR);
 	char *alt = mkpath("%s\n", reference);
 	write_or_die(fd, alt, strlen(alt));
 	if (commit_lock_file(lock))
@@ -2706,7 +2706,7 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,
 	int len, tmpfd;
 
 	strbuf_addstr(&export_marks, "--export-marks=");
-	strbuf_addstr(&export_marks, git_path("hashstream_XXXXXX"));
+	strbuf_addstr(&export_marks, git_path_unsafe("hashstream_XXXXXX"));
 	tmpfile = export_marks.buf + strlen("--export-marks=");
 	tmpfd = git_mkstemp_mode(tmpfile, 0600);
 	if (tmpfd < 0)
diff --git a/shallow.c b/shallow.c
index a0363dea..d721397a 100644
--- a/shallow.c
+++ b/shallow.c
@@ -25,7 +25,7 @@ int is_repository_shallow(void)
 	if (is_shallow >= 0)
 		return is_shallow;
 
-	fp = fopen(git_path("shallow"), "r");
+	fp = fopen(git_path_unsafe("shallow"), "r");
 	if (!fp) {
 		is_shallow = 0;
 		return is_shallow;
diff --git a/transport.c b/transport.c
index 51814b5d..cc0ca04c 100644
--- a/transport.c
+++ b/transport.c
@@ -203,7 +203,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)
 
 	/* copy the refs to the temporary directory */
 
-	strbuf_addstr(&temp_dir, git_path("rsync-refs-XXXXXX"));
+	strbuf_addstr(&temp_dir, git_path_unsafe("rsync-refs-XXXXXX"));
 	if (!mkdtemp(temp_dir.buf))
 		die_errno ("Could not make temporary directory");
 	temp_dir_len = temp_dir.len;
@@ -366,7 +366,7 @@ static int rsync_transport_push(struct transport *transport,
 
 	/* copy the refs to the temporary directory; they could be packed. */
 
-	strbuf_addstr(&temp_dir, git_path("rsync-refs-XXXXXX"));
+	strbuf_addstr(&temp_dir, git_path_unsafe("rsync-refs-XXXXXX"));
 	if (!mkdtemp(temp_dir.buf))
 		die_errno ("Could not make temporary directory");
 	strbuf_addch(&temp_dir, '/');
diff --git a/unpack-trees.c b/unpack-trees.c
index 8282f5e5..44f408b8 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1010,7 +1010,7 @@ 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, NULL, &el, 0) < 0)
+		if (add_excludes_from_file_to_list(git_path_unsafe("info/sparse-checkout"), "", 0, NULL, &el, 0) < 0)
 			o->skip_sparse_checkout = 1;
 		else
 			o->el = &el;
-- 
1.7.8.rc0

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Nguyen Thai Ngoc Duy]

2011/11/16 Jonathan Nieder <jrnieder@gmail.com>:
> Ramkumar Ramachandra wrote:
>> Junio C Hamano wrote:
>
>>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436
>>
>> I noticed that sha1_to_hex() also operates like this.  The
>> resolve_ref() function is really important, but using the same
>> technique for these tiny functions is probably an overkill
>
> I don't follow.  Do you mean that not being confusing is overkill,
> because the function is small that no one will bother to look up the
> right semantics?  Wait, that sentence didn't come out the way I
> wanted. ;-)
>
> Jokes aside, here's a rough series to do the git_path ->
> git_path_unsafe renaming.  While writing it, I noticed a couple of
> bugs, hence the two patches before the last one.  Patch 2 is the more
> interesting one.

Or perhaps
 - kill git_path(const char *fmt, ...) in favor of git_pathdup() companion
 - git_path(const char *path) maintains a small hash table to keep
track of all returned strings based with "path" as key.

Out of 142 git_path() calls in my tree, 97 of them are in form
git_path("some static string"). git_path() could learn to keep track
of all generated strings while keep it convenient to use. I suspect
with some macro magic, we can keep track of generated strings without
a hash table.
-- 
Duy

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Nguyen Thai Ngoc Duy]

On Wed, Nov 16, 2011 at 3:37 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> I suspect
> with some macro magic, we can keep track of generated strings without
> a hash table.

I misremembered. There is an operator to convert symbol to string, not
the other way around.
-- 
Duy

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Junio C Hamano wrote:
>
>>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436
>>
>> I noticed that sha1_to_hex() also operates like this.  The
>> resolve_ref() function is really important, but using the same
>> technique for these tiny functions is probably an overkill
>
> I don't follow.  Do you mean that not being confusing is overkill,
> because the function is small that no one will bother to look up the
> right semantics?  Wait, that sentence didn't come out the way I
> wanted. ;-)

I meant overkill in terms of the work required and code churn.
Ofcourse, I'd have been more than happy to see it being implemented-
and you've actually done it now! :) Nguyễn has a more fancy solution,
though I'm quite happy with this as it is.

Finally, for all the times I've fumbled in git_path() usage:
Liked-by: Ramkumar Ramachandra <artagnon@gmail.com>

Thanks for working on this.

-- Ram

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Jonathan Nieder]

Nguyen Thai Ngoc Duy wrote:

> Or perhaps
[...]
>  - git_path(const char *path) maintains a small hash table to keep
> track of all returned strings based with "path" as key.
>
> Out of 142 git_path() calls in my tree, 97 of them are in form
> git_path("some static string").

The main bit I dislike about patch 3/3 is that constructs like
'unlink(git_path("MERGE_HEAD"));' are not actually unsafe, unless they
happen to sit in the middle of an unsafe

	const char *filename = git_path(foo);
	int fd;

	call_a_function_i_dont_control();
	fd = open(filename, O_CREAT|O_WRONLY|O_TRUNC, 0600);

sequence.  Lacks that feeling of truth in advertising.  And on the
other hand that this doesn't help with thread-safety at all.

I think if I ran the world, the fundamental operation would be
strbuf_addpath().  Unlike git_pathdup(), this lets callers avoid some
allocation churn if they are in the middle of a loop.

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Nguyen Thai Ngoc Duy]

On Wed, Nov 16, 2011 at 3:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>
>> Or perhaps
> [...]
>>  - git_path(const char *path) maintains a small hash table to keep
>> track of all returned strings based with "path" as key.
>>
>> Out of 142 git_path() calls in my tree, 97 of them are in form
>> git_path("some static string").
>
> The main bit I dislike about patch 3/3 is that constructs like
> 'unlink(git_path("MERGE_HEAD"));' are not actually unsafe

Well, we can create wrappers (e.g. repo_unlink(const char *) that
calls git_path internally). According to grep/sed these functions are
used in form xxx(git_path(xxx))

     16 unlink
      8 file_exists
      7 stat
      6 fopen
      5 rename
      5 open
      4 unlink_or_warn
      3 safe_create_dir
      3 adjust_shared_perm
      3 access
      2 xstrdup
      2 safe_create_leading_directories
      2 rmdir
      2 remove_empty_directories
      2 opendir
      1 unable_to_lock_error
      1 read_attr_from_file
      1 mkdir
      1 lstat
      1 launch_editor
      1 add_excludes_from_file_to_list

By creating wrappers for unlink, file_exists, stat, fopen, rename and
open we can safely avoid git_pathdup()/free() in 42 places.
-- 
Duy

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Nguyen Thai Ngoc Duy]

2011/11/16 Jonathan Nieder <jrnieder@gmail.com>:
> Jokes aside, here's a rough series to do the git_path ->
> git_path_unsafe renaming.  While writing it, I noticed a couple of
> bugs, hence the two patches before the last one.  Patch 2 is the more
> interesting one.

Another approach is do nothing and leave it for a static analysis tool
to detect potential problems. I'm looking at sparse at the moment,
although I know nothing about it to say if it can or cannot detect
such problems. We can at least make sparse detect return value from
git_path() being passed to an unsafe function, I think.
-- 
Duy

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Michael Haggerty]

On 11/16/2011 02:33 PM, Nguyen Thai Ngoc Duy wrote:
> 2011/11/16 Jonathan Nieder <jrnieder@gmail.com>:
>> Jokes aside, here's a rough series to do the git_path ->
>> git_path_unsafe renaming.  While writing it, I noticed a couple of
>> bugs, hence the two patches before the last one.  Patch 2 is the more
>> interesting one.
> 
> Another approach is do nothing and leave it for a static analysis tool
> to detect potential problems. I'm looking at sparse at the moment,
> although I know nothing about it to say if it can or cannot detect
> such problems. We can at least make sparse detect return value from
> git_path() being passed to an unsafe function, I think.

For the cases when static analysis doesn't suffice, recently I posted
some patches that make it possible for debug a problem that results from
the use of a "stale" buffer [1].  But having myself also been bitten by
this problem, I'd also be in favor of a more systematic solution, even
if it has a small runtime cost.  After all, most of the time the
filename created by git_path() is going to be passed to the kernel a
moment later, which will usually be vastly slower than an extra malloc/free.

Michael

[1] http://comments.gmane.org/gmane.comp.version-control.git/182209

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH/RFC] introduce strbuf_addpath()

[Jonathan Nieder]

strbuf_addpath() is like git_path_unsafe(), except instead of
returning its own buffer, it appends its result to a buffer provided
by the caller.

Benefits:

 - Since it uses a caller-supplied buffer, unlike git_path_unsafe(),
   there is no risk that one call will clobber the result from
   another.

 - Unlike git_pathdup(), it does not need to waste time allocating
   memory in the middle of your tight loop over refs.

 - The size of the result is not limited to PATH_MAX.

Caveat: the size of its result is not limited to PATH_MAX.  Existing
code might be relying on git_path*() to produce a result that is safe
to copy to a PATH_MAX-sized buffer.  Be careful.

This patch introduces the strbuf_addpath() function and converts a few
existing users of the strbuf_addstr(git_path(...)) idiom to
demonstrate the API.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Jonathan Nieder wrote:

> I think if I ran the world, the fundamental operation would be
> strbuf_addpath().

Like this, maybe.

In these v1.7.8-rc2 days, you should probably spend your time
reviewing patch 2/3 of the previous series, though. ;-)

 cache.h       |    3 +++
 notes-merge.c |    2 +-
 path.c        |   34 ++++++++++++++++++++++++++++++++++
 sha1_file.c   |    2 +-
 transport.c   |    4 ++--
 5 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/cache.h b/cache.h
index 7fb85445..33d7d147 100644
--- a/cache.h
+++ b/cache.h
@@ -659,6 +659,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 	__attribute__((format (printf, 3, 4)));
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
+extern void strbuf_addpath(struct strbuf *sb, const char *fmt, ...)
+	__attribute__((format (printf, 2, 3)));
 
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
diff --git a/notes-merge.c b/notes-merge.c
index 0b49e8ad..738442de 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -733,7 +733,7 @@ int notes_merge_abort(struct notes_merge_options *o)
 	struct strbuf buf = STRBUF_INIT;
 	int ret;
 
-	strbuf_addstr(&buf, git_path_unsafe(NOTES_MERGE_WORKTREE));
+	strbuf_addpath(&buf, NOTES_MERGE_WORKTREE);
 	OUTPUT(o, 3, "Removing notes merge worktree at %s", buf.buf);
 	ret = remove_dir_recursively(&buf, 0);
 	strbuf_release(&buf);
diff --git a/path.c b/path.c
index 0611b7be..28d6326d 100644
--- a/path.c
+++ b/path.c
@@ -33,6 +33,20 @@ static char *cleanup_path(char *path)
 	return path;
 }
 
+static void strbuf_cleanup_path(struct strbuf *sb, size_t pos)
+{
+	char *newstart;
+
+	/*
+	 * cleanup_path expects to be acting on a static buffer,
+	 * so it modifies its argument in place and returns
+	 * a pointer to the new start of the path.
+	 */
+	newstart = cleanup_path(sb->buf + pos);
+	strbuf_remove(sb, pos, newstart - sb->buf - pos);
+	strbuf_setlen(sb, strlen(sb->buf));
+}
+
 char *mksnpath(char *buf, size_t n, const char *fmt, ...)
 {
 	va_list args;
@@ -68,6 +82,18 @@ bad:
 	return buf;
 }
 
+static void strbuf_vaddpath(struct strbuf *sb, const char *fmt, va_list args)
+{
+	const char *git_dir = get_git_dir();
+	size_t pos = sb->len;
+
+	strbuf_addstr(sb, git_dir);
+	if (pos < sb->len && !is_dir_sep(sb->buf[sb->len - 1]))
+		strbuf_addch(sb, '/');
+	strbuf_vaddf(sb, fmt, args);
+	strbuf_cleanup_path(sb, pos);
+}
+
 char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 {
 	va_list args;
@@ -87,6 +113,14 @@ char *git_pathdup(const char *fmt, ...)
 	return xstrdup(path);
 }
 
+void strbuf_addpath(struct strbuf *sb, const char *fmt, ...)
+{
+	va_list args;
+	va_start(args, fmt);
+	strbuf_vaddpath(sb, fmt, args);
+	va_end(args);
+}
+
 char *mkpath(const char *fmt, ...)
 {
 	va_list args;
diff --git a/sha1_file.c b/sha1_file.c
index ba7eca89..315d1004 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2706,7 +2706,7 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,
 	int len, tmpfd;
 
 	strbuf_addstr(&export_marks, "--export-marks=");
-	strbuf_addstr(&export_marks, git_path_unsafe("hashstream_XXXXXX"));
+	strbuf_addpath(&export_marks, "hashstream_XXXXXX");
 	tmpfile = export_marks.buf + strlen("--export-marks=");
 	tmpfd = git_mkstemp_mode(tmpfile, 0600);
 	if (tmpfd < 0)
diff --git a/transport.c b/transport.c
index cc0ca04c..f5c95b40 100644
--- a/transport.c
+++ b/transport.c
@@ -203,7 +203,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)
 
 	/* copy the refs to the temporary directory */
 
-	strbuf_addstr(&temp_dir, git_path_unsafe("rsync-refs-XXXXXX"));
+	strbuf_addpath(&temp_dir, "rsync-refs-XXXXXX");
 	if (!mkdtemp(temp_dir.buf))
 		die_errno ("Could not make temporary directory");
 	temp_dir_len = temp_dir.len;
@@ -366,7 +366,7 @@ static int rsync_transport_push(struct transport *transport,
 
 	/* copy the refs to the temporary directory; they could be packed. */
 
-	strbuf_addstr(&temp_dir, git_path_unsafe("rsync-refs-XXXXXX"));
+	strbuf_addpath(&temp_dir, "rsync-refs-XXXXXX");
 	if (!mkdtemp(temp_dir.buf))
 		die_errno ("Could not make temporary directory");
 	strbuf_addch(&temp_dir, '/');
-- 
1.7.8.rc2

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> ... git_path() could learn to keep track
> of all generated strings while keep it convenient to use.

That certainly sounds an interesting approach.

Re: [PATCH 3/3] rename git_path() to git_path_unsafe()

[Junio C Hamano]

Given that other functions like real_path() and mkpath() share the same
"perishable, use it immediately" property, and also git_path() is such a
short and sweet name, I am beginning to think that we probably should
leave these alone but document that *path() are "unsafe" somewhere and
just add *path_cpy() or your strbuf_addpath() function.

In any case, I do not like seeing many list regulars throwing too many
non-regression-fix patches during prerelease freeze period on the
list. Continuing development for the next cycle is encouraged and trying
to do so using workflows that you do not usually use is even more
encouraged, though. You would make more use of the release candidate Git
for such activities, and may uncover regressions before the final.

Thanks.

Re: [PATCH 3/3] rename git_path() to git_path_unsafe()

[Jonathan Nieder]

Junio C Hamano wrote:

> In any case, I do not like seeing many list regulars throwing too many
> non-regression-fix patches during prerelease freeze period on the
> list.

Fine, but what about the buffer overflow (not an incredibly recent
regression, but certainly a fix) addressed in patch 2 of the series?

Re: [PATCH/RFC] introduce strbuf_addpath()

[Nguyen Thai Ngoc Duy]

On Wed, Nov 16, 2011 at 03:50:04PM -0600, Jonathan Nieder wrote:
> strbuf_addpath() is like git_path_unsafe(), except instead of
> returning its own buffer, it appends its result to a buffer provided
> by the caller.
> 
> Benefits:
> 
>  - Since it uses a caller-supplied buffer, unlike git_path_unsafe(),
>    there is no risk that one call will clobber the result from
>    another.
> 
>  - Unlike git_pathdup(), it does not need to waste time allocating
>    memory in the middle of your tight loop over refs.
> 
>  - The size of the result is not limited to PATH_MAX.
> 
> Caveat: the size of its result is not limited to PATH_MAX.  Existing
> code might be relying on git_path*() to produce a result that is safe
> to copy to a PATH_MAX-sized buffer.  Be careful.
> 
> This patch introduces the strbuf_addpath() function and converts a few
> existing users of the strbuf_addstr(git_path(...)) idiom to
> demonstrate the API.
> 
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Jonathan Nieder wrote:
> 
> > I think if I ran the world, the fundamental operation would be
> > strbuf_addpath().
> 
> Like this, maybe.
> 


If I ran the world, I would change get_pathname() to return "struct
strbuf *" instead and change "char *git_path(..)" to "const char *git_path(...)".
Code paths that want to modify git_path() return value could
just use strbuf_addpath()

I did try to turn git_path to return const char *, the following patch
seems to make it build without any warnings

-- 8< --
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index c6bc8eb..fd7c682 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -1041,7 +1041,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,
 	if (args.depth > 0) {
 		struct cache_time mtime;
 		struct strbuf sb = STRBUF_INIT;
-		char *shallow = git_path("shallow");
+		const char *shallow = git_path("shallow");
 		int fd;
 
 		mtime.sec = st.st_mtime;
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 91731b9..261f1a0 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -367,7 +367,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	char note[1024];
 	const char *what, *kind;
 	struct ref *rm;
-	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	const char *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	char *url;
 
 	fp = fopen(filename, "a");
 	if (!fp)
@@ -647,7 +648,7 @@ static void check_not_current_branch(struct ref *ref_map)
 
 static int truncate_fetch_head(void)
 {
-	char *filename = git_path("FETCH_HEAD");
+	const char *filename = git_path("FETCH_HEAD");
 	FILE *fp = fopen(filename, "w");
 
 	if (!fp)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0e0e17a..f9624d7 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -215,12 +215,12 @@ 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) {
-			char *filename = git_path("lost-found/%s/%s",
+			const char *filename = git_path("lost-found/%s/%s",
 				obj->type == OBJ_COMMIT ? "commit" : "other",
 				sha1_to_hex(obj->sha1));
 			FILE *f;
 
-			if (safe_create_leading_directories(filename)) {
+			if (safe_create_leading_directories_const(filename)) {
 				error("Could not create lost-found");
 				return;
 			}
diff --git a/builtin/remote.c b/builtin/remote.c
index 583eec9..0662d37 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -587,7 +587,7 @@ static int migrate_file(struct remote *remote)
 {
 	struct strbuf buf = STRBUF_INIT;
 	int i;
-	char *path = NULL;
+	const char *path = NULL;
 
 	strbuf_addf(&buf, "remote.%s.url", remote->name);
 	for (i = 0; i < remote->url_nr; i++)
diff --git a/fast-import.c b/fast-import.c
index 8d8ea3c..4293a9f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -403,7 +403,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);
 
 static void write_crash_report(const char *err)
 {
-	char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
+	const char *loc = git_path("fast_import_crash_%"PRIuMAX, (uintmax_t) getpid());
 	FILE *rpt = fopen(loc, "w");
 	struct branch *b;
 	unsigned long lu;
diff --git a/notes-merge.c b/notes-merge.c
index e33c2c9..8da2e0a 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -288,7 +288,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)
 				    "(%s exists).", git_path("NOTES_MERGE_*"));
 		}
 
-		if (safe_create_leading_directories(git_path(
+		if (safe_create_leading_directories_const(git_path(
 				NOTES_MERGE_WORKTREE "/.test")))
 			die_errno("unable to create directory %s",
 				  git_path(NOTES_MERGE_WORKTREE));
@@ -303,8 +303,8 @@ static void write_buf_to_worktree(const unsigned char *obj,
 				  const char *buf, unsigned long size)
 {
 	int fd;
-	char *path = git_path(NOTES_MERGE_WORKTREE "/%s", sha1_to_hex(obj));
-	if (safe_create_leading_directories(path))
+	const char *path = git_path(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))
 		die("found existing file at '%s'", path);
diff --git a/refs.c b/refs.c
index 62d8a37..7469cf1 100644
--- a/refs.c
+++ b/refs.c
@@ -1189,7 +1189,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 
 static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int flags, int *type_p)
 {
-	char *ref_file;
+	const char *ref_file;
 	const char *orig_ref = ref;
 	struct ref_lock *lock;
 	int last_errno = 0;
@@ -1250,7 +1250,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	if ((flags & REF_NODEREF) && (type & REF_ISSYMREF))
 		lock->force_write = 1;
 
-	if (safe_create_leading_directories(ref_file)) {
+	if (safe_create_leading_directories_const(ref_file)) {
 		last_errno = errno;
 		error("unable to create directory for %s", ref_file);
 		goto error_return;
@@ -1411,7 +1411,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 		}
 	}
 
-	if (log && safe_create_leading_directories(git_path("logs/%s", newref))) {
+	if (log && safe_create_leading_directories_const(git_path("logs/%s", newref))) {
 		error("unable to create directory for %s", newref);
 		goto rollback;
 	}
-- 8< --

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Nguyen Thai Ngoc Duy]

On Wed, Nov 16, 2011 at 01:59:55AM -0600, Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
> > Junio C Hamano wrote:
> 
> >> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436
> >
> > I noticed that sha1_to_hex() also operates like this.  The
> > resolve_ref() function is really important, but using the same
> > technique for these tiny functions is probably an overkill
> 
> I don't follow.  Do you mean that not being confusing is overkill,
> because the function is small that no one will bother to look up the
> right semantics?  Wait, that sentence didn't come out the way I
> wanted. ;-)
> 
> Jokes aside, here's a rough series to do the git_path ->
> git_path_unsafe renaming.  While writing it, I noticed a couple of
> bugs, hence the two patches before the last one.  Patch 2 is the more
> interesting one.

Or perhaps we can use per-file buffer rings instead of a global one.
This means git_path() can only interfere another one in the same file,
making the interaction simpler and hopefully simple enough for reviewers
to catch 90% bugs, therefore safe enough to avoid the _unsafe suffix.

Adding static variable declaration in cache.h is ugly, but that could be
moved to a separate header file.

diff --git a/cache.h b/cache.h
index 2e6ad36..437bc3a 100644
--- a/cache.h
+++ b/cache.h
@@ -660,9 +660,13 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)
 extern char *git_pathdup(const char *fmt, ...)
 	__attribute__((format (printf, 1, 2)));
 
+#define git_path(...) git_path_1(pathname_array[3 & ++pathname_index], __VA_ARGS__)
+static char pathname_array[4][PATH_MAX];
+static int pathname_index;
+
 /* Return a statically allocated filename matching the sha1 signature */
 extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));
+extern char *git_path_1(char *pathname, const char *fmt, ...) __attribute__((format (printf, 2, 3)));
 extern char *git_path_submodule(const char *path, const char *fmt, ...)
 	__attribute__((format (printf, 2, 3)));
 
diff --git a/path.c b/path.c
index b6f71d1..3c95db1 100644
--- a/path.c
+++ b/path.c
@@ -101,10 +101,9 @@ char *mkpath(const char *fmt, ...)
 	return cleanup_path(pathname);
 }
 
-char *git_path(const char *fmt, ...)
+char *git_path_1(char *pathname, const char *fmt, ...)
 {
 	const char *git_dir = get_git_dir();
-	char *pathname = get_pathname();
 	va_list args;
 	unsigned len;
 

Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage

[Ramsay Jones]

Nguyen Thai Ngoc Duy wrote:
> On Wed, Nov 16, 2011 at 3:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Nguyen Thai Ngoc Duy wrote:
>>
>>> Or perhaps
>> [...]
>>>  - git_path(const char *path) maintains a small hash table to keep
>>> track of all returned strings based with "path" as key.
>>>
>>> Out of 142 git_path() calls in my tree, 97 of them are in form
>>> git_path("some static string").
>> The main bit I dislike about patch 3/3 is that constructs like
>> 'unlink(git_path("MERGE_HEAD"));' are not actually unsafe
> 
> Well, we can create wrappers (e.g. repo_unlink(const char *) that
> calls git_path internally). According to grep/sed these functions are
> used in form xxx(git_path(xxx))
> 
>      16 unlink
>       8 file_exists
>       7 stat
>       6 fopen
>       5 rename
>       5 open
>       4 unlink_or_warn
>       3 safe_create_dir
>       3 adjust_shared_perm
>       3 access
>       2 xstrdup
>       2 safe_create_leading_directories

This one at least, maybe others, is unsafe on cygwin. Indeed it causes
a test failure in t3200-branch.sh; patch is on it's way ...

ATB,
Ramsay Jones