{"thread":{"id":"28854","subject":"[PATCH 0/5] Sequencer: working around historical mistakes","startedAt":"2011-11-05T16:29:41Z","lastAt":"2011-11-19T19:25:33Z","messageCount":47,"participants":["Ramkumar Ramachandra","Jonathan Nieder","Junio C Hamano","Miles Bader","Nguyen Thai Ngoc Duy","Michael Haggerty","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"178910","messageId":"1320510586-3940-1-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":null,"subject":"[PATCH 0/5] Sequencer: working around historical mistakes","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:41Z","receivedAt":"2011-11-05T16:29:41Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nAs described in the discussion following $gmane/179304/focus=179383,\nwe have decided to handle historical hacks in the sequencer itself.\nThis series that follows is one step in the right direction.\n\n- Part 1/5 makes the gigantic move required to create the sequencer.\nIf you need an excuse to celebrate, wait till this gets merged :)\n- Part 5/5 can be considered as the \"ultimate objective\" of the\nseries. I first wrote this part, and then wrote the other parts to\nmake tests pass.\n- Parts 3/5 and 4/5 are ugly! Causes heartburn.\n\nImmediate shortcomings of this iteration:\n1. No tests yet. I want to see if it's possible to make this less\nugly first.\n2. This series depends on rr/revert-cherry-pick, but doesn't apply to\nthe current 'next'- sorry, rebasing is a massive pita due to 1/5.\n\nThanks for reading.\n\n-- Ram\n\nRamkumar Ramachandra (5):\n sequencer: factor code out of revert builtin\n sequencer: remove CHERRY_PICK_HEAD with sequencer state\n sequencer: sequencer state is useless without todo\n sequencer: handle single commit pick separately\n sequencer: revert d3f4628e\n\n builtin/revert.c | 821 +--------------------------------------\n sequencer.c | 832 ++++++++++++++++++++++++++++++++++++++-\n sequencer.h | 26 ++\n t/t3510-cherry-pick-sequence.sh | 24 --\n 4 files changed, 847 insertions(+), 856 deletions(-)\n\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178912","messageId":"1320510586-3940-2-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:42Z","receivedAt":"2011-11-05T16:29:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Start building the generalized sequencer by moving code from revert.c\ninto sequencer.c and sequencer.h. Make the builtin responsible only\nfor command-line parsing, and expose a new sequencer_pick_revisions()\nto do the actual work of sequencing commits.\n\nThis is intended to be almost a pure code movement patch with no\nfunctional changes. Check with:\n\n $ git blame -s -CCC HEAD^..HEAD -- sequencer.c | grep -C3 '^[^^]'\n\nSigned-off-by: Ramkumar Ramachandra \n---\n builtin/revert.c | 821 +-----------------------------------------------------\n sequencer.c | 802 ++++++++++++++++++++++++++++++++++++++++++++++++++++-\n sequencer.h | 26 ++\n 3 files changed, 828 insertions(+), 821 deletions(-)\n\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex df9459b..c272920 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -1,19 +1,9 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n-#include \"object.h\"\n-#include \"commit.h\"\n-#include \"tag.h\"\n-#include \"run-command.h\"\n-#include \"exec_cmd.h\"\n-#include \"utf8.h\"\n #include \"parse-options.h\"\n-#include \"cache-tree.h\"\n #include \"diff.h\"\n #include \"revision.h\"\n #include \"rerere.h\"\n-#include \"merge-recursive.h\"\n-#include \"refs.h\"\n-#include \"dir.h\"\n #include \"sequencer.h\"\n \n /*\n@@ -39,40 +29,11 @@ static const char * const cherry_pick_usage[] = {\n \tNULL\n };\n \n-enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n-\n-struct replay_opts {\n-\tenum replay_action action;\n-\tenum replay_subcommand subcommand;\n-\n-\t/* Boolean options */\n-\tint edit;\n-\tint record_origin;\n-\tint no_commit;\n-\tint signoff;\n-\tint allow_ff;\n-\tint allow_rerere_auto;\n-\n-\tint mainline;\n-\n-\t/* Merge strategy */\n-\tconst char *strategy;\n-\tconst char **xopts;\n-\tsize_t xopts_nr, xopts_alloc;\n-\n-\t/* Only used by REPLAY_NONE */\n-\tstruct rev_info *revs;\n-};\n-\n-#define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n-\n static const char *action_name(const struct replay_opts *opts)\n {\n \treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n }\n \n-static char *get_encoding(const char *message);\n-\n static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)\n {\n \treturn opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage;\n@@ -222,784 +183,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)\n \t\tusage_with_options(usage_str, options);\n }\n \n-struct commit_message {\n-\tchar *parent_label;\n-\tconst char *label;\n-\tconst char *subject;\n-\tchar *reencoded_message;\n-\tconst char *message;\n-};\n-\n-static int get_message(struct commit *commit, struct commit_message *out)\n-{\n-\tconst char *encoding;\n-\tconst char *abbrev, *subject;\n-\tint abbrev_len, subject_len;\n-\tchar *q;\n-\n-\tif (!commit->buffer)\n-\t\treturn -1;\n-\tencoding = get_encoding(commit->buffer);\n-\tif (!encoding)\n-\t\tencoding = \"UTF-8\";\n-\tif (!git_commit_encoding)\n-\t\tgit_commit_encoding = \"UTF-8\";\n-\n-\tout->reencoded_message = NULL;\n-\tout->message = commit->buffer;\n-\tif (strcmp(encoding, git_commit_encoding))\n-\t\tout->reencoded_message = reencode_string(commit->buffer,\n-\t\t\t\t\tgit_commit_encoding, encoding);\n-\tif (out->reencoded_message)\n-\t\tout->message = out->reencoded_message;\n-\n-\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n-\tabbrev_len = strlen(abbrev);\n-\n-\tsubject_len = find_commit_subject(out->message, &subject);\n-\n-\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n-\t\t\t strlen(\"... \") + subject_len + 1);\n-\tq = out->parent_label;\n-\tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n-\tout->label = q;\n-\tq = mempcpy(q, abbrev, abbrev_len);\n-\tq = mempcpy(q, \"... \", strlen(\"... \"));\n-\tout->subject = q;\n-\tq = mempcpy(q, subject, subject_len);\n-\t*q = '\\0';\n-\treturn 0;\n-}\n-\n-static void free_message(struct commit_message *msg)\n-{\n-\tfree(msg->parent_label);\n-\tfree(msg->reencoded_message);\n-}\n-\n-static char *get_encoding(const char *message)\n-{\n-\tconst char *p = message, *eol;\n-\n-\twhile (*p && *p != '\\n') {\n-\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n-\t\t\t; /* do nothing */\n-\t\tif (!prefixcmp(p, \"encoding \")) {\n-\t\t\tchar *result = xmalloc(eol - 8 - p);\n-\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n-\t\t\treturn result;\n-\t\t}\n-\t\tp = eol;\n-\t\tif (*p == '\\n')\n-\t\t\tp++;\n-\t}\n-\treturn NULL;\n-}\n-\n-static void write_cherry_pick_head(struct commit *commit)\n-{\n-\tint fd;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n-\n-\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"CHERRY_PICK_HEAD\"));\n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n-\tstrbuf_release(&buf);\n-}\n-\n-static void print_advice(int show_hint)\n-{\n-\tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n-\n-\tif (msg) {\n-\t\tfprintf(stderr, \"%s\\n\", msg);\n-\t\t/*\n-\t\t * A conflict has occured but the porcelain\n-\t\t * (typically rebase --interactive) wants to take care\n-\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n-\t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\t\treturn;\n-\t}\n-\n-\tif (show_hint) {\n-\t\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n-\t\tadvise(\"with 'git add ' or 'git rm '\");\n-\t\tadvise(\"and commit the result with 'git commit'\");\n-\t}\n-}\n-\n-static void write_message(struct strbuf *msgbuf, const char *filename)\n-{\n-\tstatic struct lock_file msg_file;\n-\n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n-\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s.\"), filename);\n-\tstrbuf_release(msgbuf);\n-\tif (commit_lock_file(&msg_file) < 0)\n-\t\tdie(_(\"Error wrapping up %s\"), filename);\n-}\n-\n-static struct tree *empty_tree(void)\n-{\n-\treturn lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n-}\n-\n-static int error_dirty_index(struct replay_opts *opts)\n-{\n-\tif (read_cache_unmerged())\n-\t\treturn error_resolve_conflict(action_name(opts));\n-\n-\t/* Different translation strings for cherry-pick and revert */\n-\tif (opts->action == REPLAY_PICK)\n-\t\terror(_(\"Your local changes would be overwritten by cherry-pick.\"));\n-\telse\n-\t\terror(_(\"Your local changes would be overwritten by revert.\"));\n-\n-\tif (advice_commit_before_merge)\n-\t\tadvise(_(\"Commit your changes or stash them to proceed.\"));\n-\treturn -1;\n-}\n-\n-static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n-{\n-\tstruct ref_lock *ref_lock;\n-\n-\tread_cache();\n-\tif (checkout_fast_forward(from, to))\n-\t\texit(1); /* the callee should have complained already */\n-\tref_lock = lock_any_ref_for_update(\"HEAD\", from, 0);\n-\treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n-}\n-\n-static int do_recursive_merge(struct commit *base, struct commit *next,\n-\t\t\t const char *base_label, const char *next_label,\n-\t\t\t unsigned char *head, struct strbuf *msgbuf,\n-\t\t\t struct replay_opts *opts)\n-{\n-\tstruct merge_options o;\n-\tstruct tree *result, *next_tree, *base_tree, *head_tree;\n-\tint clean, index_fd;\n-\tconst char **xopt;\n-\tstatic struct lock_file index_lock;\n-\n-\tindex_fd = hold_locked_index(&index_lock, 1);\n-\n-\tread_cache();\n-\n-\tinit_merge_options(&o);\n-\to.ancestor = base ? base_label : \"(empty tree)\";\n-\to.branch1 = \"HEAD\";\n-\to.branch2 = next ? next_label : \"(empty tree)\";\n-\n-\thead_tree = parse_tree_indirect(head);\n-\tnext_tree = next ? next->tree : empty_tree();\n-\tbase_tree = base ? base->tree : empty_tree();\n-\n-\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n-\t\tparse_merge_opt(&o, *xopt);\n-\n-\tclean = merge_trees(&o,\n-\t\t\t head_tree,\n-\t\t\t next_tree, base_tree, &result);\n-\n-\tif (active_cache_changed &&\n-\t (write_cache(index_fd, active_cache, active_nr) ||\n-\t commit_locked_index(&index_lock)))\n-\t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n-\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n-\trollback_lock_file(&index_lock);\n-\n-\tif (!clean) {\n-\t\tint i;\n-\t\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\\n\");\n-\t\tfor (i = 0; i < active_nr;) {\n-\t\t\tstruct cache_entry *ce = active_cache[i++];\n-\t\t\tif (ce_stage(ce)) {\n-\t\t\t\tstrbuf_addch(msgbuf, '\\t');\n-\t\t\t\tstrbuf_addstr(msgbuf, ce->name);\n-\t\t\t\tstrbuf_addch(msgbuf, '\\n');\n-\t\t\t\twhile (i < active_nr && !strcmp(ce->name,\n-\t\t\t\t\t\tactive_cache[i]->name))\n-\t\t\t\t\ti++;\n-\t\t\t}\n-\t\t}\n-\t}\n-\n-\treturn !clean;\n-}\n-\n-/*\n- * If we are cherry-pick, and if the merge did not result in\n- * hand-editing, we will hit this commit and inherit the original\n- * author date and name.\n- * If we are revert, or if our cherry-pick results in a hand merge,\n- * we had better say that the current user is responsible for that.\n- */\n-static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n-{\n-\t/* 6 is max possible length of our args array including NULL */\n-\tconst char *args[6];\n-\tint i = 0;\n-\n-\targs[i++] = \"commit\";\n-\targs[i++] = \"-n\";\n-\tif (opts->signoff)\n-\t\targs[i++] = \"-s\";\n-\tif (!opts->edit) {\n-\t\targs[i++] = \"-F\";\n-\t\targs[i++] = defmsg;\n-\t}\n-\targs[i] = NULL;\n-\n-\treturn run_command_v_opt(args, RUN_GIT_CMD);\n-}\n-\n-static int do_pick_commit(struct commit *commit, enum replay_action action,\n-\t\t\tstruct replay_opts *opts)\n-{\n-\tunsigned char head[20];\n-\tstruct commit *base, *next, *parent;\n-\tconst char *base_label, *next_label;\n-\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n-\tchar *defmsg = NULL;\n-\tstruct strbuf msgbuf = STRBUF_INIT;\n-\tint res;\n-\n-\tif (opts->no_commit) {\n-\t\t/*\n-\t\t * We do not intend to commit immediately. We just want to\n-\t\t * merge the differences in, so let's compute the tree\n-\t\t * that represents the \"current\" state for merge-recursive\n-\t\t * to work on.\n-\t\t */\n-\t\tif (write_cache_as_tree(head, 0, NULL))\n-\t\t\tdie (_(\"Your index file is unmerged.\"));\n-\t} else {\n-\t\tif (get_sha1(\"HEAD\", head))\n-\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n-\t\tif (index_differs_from(\"HEAD\", 0))\n-\t\t\treturn error_dirty_index(opts);\n-\t}\n-\tdiscard_cache();\n-\n-\tif (!commit->parents) {\n-\t\tparent = NULL;\n-\t}\n-\telse if (commit->parents->next) {\n-\t\t/* Reverting or cherry-picking a merge commit */\n-\t\tint cnt;\n-\t\tstruct commit_list *p;\n-\n-\t\tif (!opts->mainline)\n-\t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1));\n-\n-\t\tfor (cnt = 1, p = commit->parents;\n-\t\t cnt != opts->mainline && p;\n-\t\t cnt++)\n-\t\t\tp = p->next;\n-\t\tif (cnt != opts->mainline || !p)\n-\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n-\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n-\t\tparent = p->item;\n-\t} else if (0 < opts->mainline)\n-\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n-\t\t\tsha1_to_hex(commit->object.sha1));\n-\telse\n-\t\tparent = commit->parents->item;\n-\n-\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n-\t\treturn fast_forward_to(commit->object.sha1, head);\n-\n-\tif (parent && parse_commit(parent) < 0)\n-\t\t/* TRANSLATORS: The first %s will be \"revert\" or\n-\t\t \"cherry-pick\", the second %s a SHA1 */\n-\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n-\t\t\taction_name(opts), sha1_to_hex(parent->object.sha1));\n-\n-\tif (get_message(commit, &msg) != 0)\n-\t\treturn error(_(\"Cannot get commit message for %s\"),\n-\t\t\tsha1_to_hex(commit->object.sha1));\n-\n-\t/*\n-\t * \"commit\" is an existing commit. We would want to apply\n-\t * the difference it introduces since its first parent \"prev\"\n-\t * on top of the current HEAD if we are cherry-pick. Or the\n-\t * reverse of it if we are revert.\n-\t */\n-\n-\tdefmsg = git_pathdup(\"MERGE_MSG\");\n-\n-\tif (action == REPLAY_REVERT) {\n-\t\tbase = commit;\n-\t\tbase_label = msg.label;\n-\t\tnext = parent;\n-\t\tnext_label = msg.parent_label;\n-\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n-\t\tstrbuf_addstr(&msgbuf, msg.subject);\n-\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n-\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n-\n-\t\tif (commit->parents && commit->parents->next) {\n-\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n-\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));\n-\t\t}\n-\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n-\t} else {\n-\t\tconst char *p;\n-\n-\t\tbase = parent;\n-\t\tbase_label = msg.parent_label;\n-\t\tnext = commit;\n-\t\tnext_label = msg.label;\n-\n-\t\t/*\n-\t\t * Append the commit log message to msgbuf; it starts\n-\t\t * after the tree, parent, author, committer\n-\t\t * information followed by \"\\n\\n\".\n-\t\t */\n-\t\tp = strstr(msg.message, \"\\n\\n\");\n-\t\tif (p) {\n-\t\t\tp += 2;\n-\t\t\tstrbuf_addstr(&msgbuf, p);\n-\t\t}\n-\n-\t\tif (opts->record_origin) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n-\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n-\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n-\t\t}\n-\t}\n-\n-\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || action == REPLAY_REVERT) {\n-\t\tres = do_recursive_merge(base, next, base_label, next_label,\n-\t\t\t\t\t head, &msgbuf, opts);\n-\t\twrite_message(&msgbuf, defmsg);\n-\t} else {\n-\t\tstruct commit_list *common = NULL;\n-\t\tstruct commit_list *remotes = NULL;\n-\n-\t\twrite_message(&msgbuf, defmsg);\n-\n-\t\tcommit_list_insert(base, &common);\n-\t\tcommit_list_insert(next, &remotes);\n-\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n-\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n-\t\tfree_commit_list(common);\n-\t\tfree_commit_list(remotes);\n-\t}\n-\n-\t/*\n-\t * If the merge was clean or if it failed due to conflict, we write\n-\t * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.\n-\t * However, if the merge did not even start, then we don't want to\n-\t * write it at all.\n-\t */\n-\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n-\t\twrite_cherry_pick_head(commit);\n-\n-\tif (res) {\n-\t\terror(action == REPLAY_REVERT\n-\t\t ? _(\"could not revert %s... %s\")\n-\t\t : _(\"could not apply %s... %s\"),\n-\t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n-\t\t msg.subject);\n-\t\tprint_advice(res == 1);\n-\t\trerere(opts->allow_rerere_auto);\n-\t} else {\n-\t\tif (!opts->no_commit)\n-\t\t\tres = run_git_commit(defmsg, opts);\n-\t}\n-\n-\tfree_message(&msg);\n-\tfree(defmsg);\n-\n-\treturn res;\n-}\n-\n-static void prepare_revs(struct replay_opts *opts)\n-{\n-\tif (opts->action != REPLAY_REVERT)\n-\t\topts->revs->reverse ^= 1;\n-\n-\tif (prepare_revision_walk(opts->revs))\n-\t\tdie(_(\"revision walk setup failed\"));\n-\n-\tif (!opts->revs->commits)\n-\t\tdie(_(\"empty commit set passed\"));\n-}\n-\n-static void read_and_refresh_cache(struct replay_opts *opts)\n-{\n-\tstatic struct lock_file index_lock;\n-\tint index_fd = hold_locked_index(&index_lock, 0);\n-\tif (read_index_preload(&the_index, NULL) < 0)\n-\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n-\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n-\tif (the_index.cache_changed) {\n-\t\tif (write_index(&the_index, index_fd) ||\n-\t\t commit_locked_index(&index_lock))\n-\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n-\t}\n-\trollback_lock_file(&index_lock);\n-}\n-\n-/*\n- * Append a commit to the end of the commit_list.\n- *\n- * next starts by pointing to the variable that holds the head of an\n- * empty commit_list, and is updated to point to the \"next\" field of\n- * the last item on the list as new commits are appended.\n- *\n- * Usage example:\n- *\n- * struct commit_list *list;\n- * struct commit_list **next = &list;\n- *\n- * next = commit_list_append(c1, next);\n- * next = commit_list_append(c2, next);\n- * assert(commit_list_count(list) == 2);\n- * return list;\n- */\n-static struct replay_insn_list **replay_insn_list_append(enum replay_action action,\n-\t\t\t\t\t\tstruct commit *operand,\n-\t\t\t\t\t\tstruct replay_insn_list **next)\n-{\n-\tstruct replay_insn_list *new = xmalloc(sizeof(*new));\n-\tnew->action = action;\n-\tnew->operand = operand;\n-\t*next = new;\n-\tnew->next = NULL;\n-\treturn &new->next;\n-}\n-\n-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n-{\n-\tstruct replay_insn_list *cur;\n-\n-\tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tconst char *sha1_abbrev, *action_str, *subject;\n-\t\tint subject_len;\n-\n-\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n-\t\tsha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);\n-\t\tsubject_len = find_commit_subject(cur->operand->buffer, &subject);\n-\t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n-\t\t\tsubject_len, subject);\n-\t}\n-\treturn 0;\n-}\n-\n-static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n-{\n-\tunsigned char commit_sha1[20];\n-\tchar *end_of_object_name;\n-\tint saved, status;\n-\n-\tif (!prefixcmp(bol, \"pick \")) {\n-\t\titem->action = REPLAY_PICK;\n-\t\tbol += strlen(\"pick \");\n-\t} else if (!prefixcmp(bol, \"revert \")) {\n-\t\titem->action = REPLAY_REVERT;\n-\t\tbol += strlen(\"revert \");\n-\t} else {\n-\t\tsize_t len = strchrnul(bol, '\\n') - bol;\n-\t\tif (len > 255)\n-\t\t\tlen = 255;\n-\t\treturn error(_(\"Unrecognized action: %.*s\"), (int)len, bol);\n-\t}\n-\n-\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n-\tsaved = *end_of_object_name;\n-\t*end_of_object_name = '\\0';\n-\tstatus = get_sha1(bol, commit_sha1);\n-\t*end_of_object_name = saved;\n-\n-\tif (status < 0)\n-\t\treturn error(_(\"Malformed object name: %s\"), bol);\n-\n-\titem->operand = lookup_commit_reference(commit_sha1);\n-\tif (!item->operand)\n-\t\treturn error(_(\"Not a valid commit: %s\"), bol);\n-\n-\titem->next = NULL;\n-\treturn 0;\n-}\n-\n-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n-{\n-\tstruct replay_insn_list **next = todo_list;\n-\tstruct replay_insn_list item = {0, NULL, NULL};\n-\tchar *p = buf;\n-\tint i;\n-\n-\tfor (i = 1; *p; i++) {\n-\t\tchar *eol = strchrnul(p, '\\n');\n-\t\tif (parse_insn_line(p, eol, &item) < 0)\n-\t\t\treturn error(_(\"on line %d.\"), i);\n-\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n-\t\tp = *eol ? eol + 1 : eol;\n-\t}\n-\tif (!*todo_list)\n-\t\treturn error(_(\"No commits parsed.\"));\n-\treturn 0;\n-}\n-\n-static void read_populate_todo(struct replay_insn_list **todo_list)\n-{\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tint fd, res;\n-\n-\tfd = open(todo_file, O_RDONLY);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open %s.\"), todo_file);\n-\tif (strbuf_read(&buf, fd, 0) < 0) {\n-\t\tclose(fd);\n-\t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Could not read %s.\"), todo_file);\n-\t}\n-\tclose(fd);\n-\n-\tres = parse_insn_buffer(buf.buf, todo_list);\n-\tstrbuf_release(&buf);\n-\tif (res)\n-\t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n-}\n-\n-static int populate_opts_cb(const char *key, const char *value, void *data)\n-{\n-\tstruct replay_opts *opts = data;\n-\tint error_flag = 1;\n-\n-\tif (!value)\n-\t\terror_flag = 0;\n-\telse if (!strcmp(key, \"options.no-commit\"))\n-\t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.edit\"))\n-\t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.signoff\"))\n-\t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.record-origin\"))\n-\t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.allow-ff\"))\n-\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n-\telse if (!strcmp(key, \"options.mainline\"))\n-\t\topts->mainline = git_config_int(key, value);\n-\telse if (!strcmp(key, \"options.strategy\"))\n-\t\tgit_config_string(&opts->strategy, key, value);\n-\telse if (!strcmp(key, \"options.strategy-option\")) {\n-\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n-\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n-\t} else\n-\t\treturn error(_(\"Invalid key: %s\"), key);\n-\n-\tif (!error_flag)\n-\t\treturn error(_(\"Invalid value for %s: %s\"), key, value);\n-\n-\treturn 0;\n-}\n-\n-static void read_populate_opts(struct replay_opts **opts_ptr)\n-{\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n-\n-\tif (!file_exists(opts_file))\n-\t\treturn;\n-\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n-\t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n-}\n-\n-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n-\t\t\t\tstruct replay_opts *opts)\n-{\n-\tstruct commit *commit;\n-\tstruct replay_insn_list **next;\n-\n-\tprepare_revs(opts);\n-\n-\tnext = todo_list;\n-\twhile ((commit = get_revision(opts->revs)))\n-\t\tnext = replay_insn_list_append(opts->action, commit, next);\n-}\n-\n-static int create_seq_dir(void)\n-{\n-\tconst char *seq_dir = git_path(SEQ_DIR);\n-\n-\tif (file_exists(seq_dir))\n-\t\treturn error(_(\"%s already exists.\"), seq_dir);\n-\telse if (mkdir(seq_dir, 0777) < 0)\n-\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n-\treturn 0;\n-}\n-\n-static void save_head(const char *head)\n-{\n-\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n-\tstatic struct lock_file head_lock;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tint fd;\n-\n-\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n-\tstrbuf_addf(&buf, \"%s\\n\", head);\n-\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n-\t\tdie_errno(_(\"Could not write to %s.\"), head_file);\n-\tif (commit_lock_file(&head_lock) < 0)\n-\t\tdie(_(\"Error wrapping up %s.\"), head_file);\n-}\n-\n-static void save_todo(struct replay_insn_list *todo_list)\n-{\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n-\tstatic struct lock_file todo_lock;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tint fd;\n-\n-\tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n-\tif (format_todo(&buf, todo_list) < 0)\n-\t\tdie(_(\"Could not format %s.\"), todo_file);\n-\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n-\t\tstrbuf_release(&buf);\n-\t\tdie_errno(_(\"Could not write to %s.\"), todo_file);\n-\t}\n-\tif (commit_lock_file(&todo_lock) < 0) {\n-\t\tstrbuf_release(&buf);\n-\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n-\t}\n-\tstrbuf_release(&buf);\n-}\n-\n-static void save_opts(struct replay_opts *opts)\n-{\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n-\n-\tif (opts->no_commit)\n-\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n-\tif (opts->edit)\n-\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n-\tif (opts->signoff)\n-\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n-\tif (opts->record_origin)\n-\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n-\tif (opts->allow_ff)\n-\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n-\tif (opts->mainline) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n-\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n-\t\tstrbuf_release(&buf);\n-\t}\n-\tif (opts->strategy)\n-\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n-\tif (opts->xopts) {\n-\t\tint i;\n-\t\tfor (i = 0; i < opts->xopts_nr; i++)\n-\t\t\tgit_config_set_multivar_in_file(opts_file,\n-\t\t\t\t\t\t\t\"options.strategy-option\",\n-\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n-\t}\n-}\n-\n-static int pick_commits(struct replay_insn_list *todo_list,\n-\t\t\tstruct replay_opts *opts)\n-{\n-\tstruct replay_insn_list *cur;\n-\tint res;\n-\n-\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n-\tif (opts->allow_ff)\n-\t\tassert(!(opts->signoff || opts->no_commit ||\n-\t\t\t\topts->record_origin || opts->edit));\n-\tread_and_refresh_cache(opts);\n-\n-\tfor (cur = todo_list; cur; cur = cur->next) {\n-\t\tsave_todo(cur);\n-\t\tres = do_pick_commit(cur->operand, cur->action, opts);\n-\t\tif (res) {\n-\t\t\tif (!cur->next)\n-\t\t\t\t/*\n-\t\t\t\t * An error was encountered while\n-\t\t\t\t * picking the last commit; the\n-\t\t\t\t * sequencer state is useless now --\n-\t\t\t\t * the user simply needs to resolve\n-\t\t\t\t * the conflict and commit\n-\t\t\t\t */\n-\t\t\t\tremove_sequencer_state(0);\n-\t\t\treturn res;\n-\t\t}\n-\t}\n-\n-\t/*\n-\t * Sequence of picks finished successfully; cleanup by\n-\t * removing the .git/sequencer directory\n-\t */\n-\tremove_sequencer_state(1);\n-\treturn 0;\n-}\n-\n-static int pick_revisions(struct replay_opts *opts)\n-{\n-\tstruct replay_insn_list *todo_list = NULL;\n-\tunsigned char sha1[20];\n-\n-\tif (opts->subcommand == REPLAY_NONE)\n-\t\tassert(opts->revs);\n-\n-\tread_and_refresh_cache(opts);\n-\n-\t/*\n-\t * Decide what to do depending on the arguments; a fresh\n-\t * cherry-pick should be handled differently from an existing\n-\t * one that is being continued\n-\t */\n-\tif (opts->subcommand == REPLAY_RESET) {\n-\t\tremove_sequencer_state(1);\n-\t\treturn 0;\n-\t} else if (opts->subcommand == REPLAY_CONTINUE) {\n-\t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n-\t\t\tgoto error;\n-\t\tread_populate_opts(&opts);\n-\t\tread_populate_todo(&todo_list);\n-\n-\t\t/* Verify that the conflict has been resolved */\n-\t\tif (!index_differs_from(\"HEAD\", 0))\n-\t\t\ttodo_list = todo_list->next;\n-\t} else {\n-\t\t/*\n-\t\t * Start a new cherry-pick/ revert sequence; but\n-\t\t * first, make sure that an existing one isn't in\n-\t\t * progress\n-\t\t */\n-\n-\t\twalk_revs_populate_todo(&todo_list, opts);\n-\t\tif (create_seq_dir() < 0) {\n-\t\t\terror(_(\"A cherry-pick or revert is in progress.\"));\n-\t\t\tadvise(_(\"Use --continue to continue the operation\"));\n-\t\t\tadvise(_(\"or --reset to forget about it\"));\n-\t\t\treturn -1;\n-\t\t}\n-\t\tif (get_sha1(\"HEAD\", sha1)) {\n-\t\t\tif (opts->action == REPLAY_REVERT)\n-\t\t\t\treturn error(_(\"Can't revert as initial commit\"));\n-\t\t\treturn error(_(\"Can't cherry-pick into empty head\"));\n-\t\t}\n-\t\tsave_head(sha1_to_hex(sha1));\n-\t\tsave_opts(opts);\n-\t}\n-\treturn pick_commits(todo_list, opts);\n-error:\n-\treturn error(_(\"No %s in progress\"), action_name(opts));\n-}\n-\n int cmd_revert(int argc, const char **argv, const char *prefix)\n {\n \tstruct replay_opts opts;\n@@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n \topts.action = REPLAY_REVERT;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n-\tres = pick_revisions(&opts);\n+\tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\n \t\tdie(_(\"revert failed\"));\n \treturn res;\n@@ -1026,7 +209,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)\n \topts.action = REPLAY_PICK;\n \tgit_config(git_default_config, NULL);\n \tparse_args(argc, argv, &opts);\n-\tres = pick_revisions(&opts);\n+\tres = sequencer_pick_revisions(&opts);\n \tif (res < 0)\n \t\tdie(_(\"cherry-pick failed\"));\n \treturn res;\ndiff --git a/sequencer.c b/sequencer.c\nindex bc2c046..87f146b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1,7 +1,27 @@\n #include \"cache.h\"\n-#include \"sequencer.h\"\n-#include \"strbuf.h\"\n+#include \"object.h\"\n+#include \"commit.h\"\n+#include \"tag.h\"\n+#include \"run-command.h\"\n+#include \"exec_cmd.h\"\n+#include \"utf8.h\"\n+#include \"cache-tree.h\"\n+#include \"diff.h\"\n+#include \"revision.h\"\n+#include \"rerere.h\"\n+#include \"merge-recursive.h\"\n+#include \"refs.h\"\n #include \"dir.h\"\n+#include \"sequencer.h\"\n+\n+#define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n+\n+static const char *action_name(const struct replay_opts *opts)\n+{\n+\treturn opts->action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n+}\n+\n+static char *get_encoding(const char *message);\n \n void remove_sequencer_state(int aggressive)\n {\n@@ -17,3 +37,781 @@ void remove_sequencer_state(int aggressive)\n \tstrbuf_release(&seq_dir);\n \tstrbuf_release(&seq_old_dir);\n }\n+\n+struct commit_message {\n+\tchar *parent_label;\n+\tconst char *label;\n+\tconst char *subject;\n+\tchar *reencoded_message;\n+\tconst char *message;\n+};\n+\n+static int get_message(struct commit *commit, struct commit_message *out)\n+{\n+\tconst char *encoding;\n+\tconst char *abbrev, *subject;\n+\tint abbrev_len, subject_len;\n+\tchar *q;\n+\n+\tif (!commit->buffer)\n+\t\treturn -1;\n+\tencoding = get_encoding(commit->buffer);\n+\tif (!encoding)\n+\t\tencoding = \"UTF-8\";\n+\tif (!git_commit_encoding)\n+\t\tgit_commit_encoding = \"UTF-8\";\n+\n+\tout->reencoded_message = NULL;\n+\tout->message = commit->buffer;\n+\tif (strcmp(encoding, git_commit_encoding))\n+\t\tout->reencoded_message = reencode_string(commit->buffer,\n+\t\t\t\t\tgit_commit_encoding, encoding);\n+\tif (out->reencoded_message)\n+\t\tout->message = out->reencoded_message;\n+\n+\tabbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);\n+\tabbrev_len = strlen(abbrev);\n+\n+\tsubject_len = find_commit_subject(out->message, &subject);\n+\n+\tout->parent_label = xmalloc(strlen(\"parent of \") + abbrev_len +\n+\t\t\t strlen(\"... \") + subject_len + 1);\n+\tq = out->parent_label;\n+\tq = mempcpy(q, \"parent of \", strlen(\"parent of \"));\n+\tout->label = q;\n+\tq = mempcpy(q, abbrev, abbrev_len);\n+\tq = mempcpy(q, \"... \", strlen(\"... \"));\n+\tout->subject = q;\n+\tq = mempcpy(q, subject, subject_len);\n+\t*q = '\\0';\n+\treturn 0;\n+}\n+\n+static void free_message(struct commit_message *msg)\n+{\n+\tfree(msg->parent_label);\n+\tfree(msg->reencoded_message);\n+}\n+\n+static char *get_encoding(const char *message)\n+{\n+\tconst char *p = message, *eol;\n+\n+\twhile (*p && *p != '\\n') {\n+\t\tfor (eol = p + 1; *eol && *eol != '\\n'; eol++)\n+\t\t\t; /* do nothing */\n+\t\tif (!prefixcmp(p, \"encoding \")) {\n+\t\t\tchar *result = xmalloc(eol - 8 - p);\n+\t\t\tstrlcpy(result, p + 9, eol - 8 - p);\n+\t\t\treturn result;\n+\t\t}\n+\t\tp = eol;\n+\t\tif (*p == '\\n')\n+\t\t\tp++;\n+\t}\n+\treturn NULL;\n+}\n+\n+static void write_cherry_pick_head(struct commit *commit)\n+{\n+\tint fd;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n+\n+\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n+\t\t\t git_path(\"CHERRY_PICK_HEAD\"));\n+\tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n+\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n+\tstrbuf_release(&buf);\n+}\n+\n+static void print_advice(int show_hint)\n+{\n+\tchar *msg = getenv(\"GIT_CHERRY_PICK_HELP\");\n+\n+\tif (msg) {\n+\t\tfprintf(stderr, \"%s\\n\", msg);\n+\t\t/*\n+\t\t * A conflict has occured but the porcelain\n+\t\t * (typically rebase --interactive) wants to take care\n+\t\t * of the commit itself so remove CHERRY_PICK_HEAD\n+\t\t */\n+\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\treturn;\n+\t}\n+\n+\tif (show_hint) {\n+\t\tadvise(\"after resolving the conflicts, mark the corrected paths\");\n+\t\tadvise(\"with 'git add ' or 'git rm '\");\n+\t\tadvise(\"and commit the result with 'git commit'\");\n+\t}\n+}\n+\n+static void write_message(struct strbuf *msgbuf, const char *filename)\n+{\n+\tstatic struct lock_file msg_file;\n+\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n+\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), filename);\n+\tstrbuf_release(msgbuf);\n+\tif (commit_lock_file(&msg_file) < 0)\n+\t\tdie(_(\"Error wrapping up %s\"), filename);\n+}\n+\n+static struct tree *empty_tree(void)\n+{\n+\treturn lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);\n+}\n+\n+static int error_dirty_index(struct replay_opts *opts)\n+{\n+\tif (read_cache_unmerged())\n+\t\treturn error_resolve_conflict(action_name(opts));\n+\n+\t/* Different translation strings for cherry-pick and revert */\n+\tif (opts->action == REPLAY_PICK)\n+\t\terror(_(\"Your local changes would be overwritten by cherry-pick.\"));\n+\telse\n+\t\terror(_(\"Your local changes would be overwritten by revert.\"));\n+\n+\tif (advice_commit_before_merge)\n+\t\tadvise(_(\"Commit your changes or stash them to proceed.\"));\n+\treturn -1;\n+}\n+\n+static int fast_forward_to(const unsigned char *to, const unsigned char *from)\n+{\n+\tstruct ref_lock *ref_lock;\n+\n+\tread_cache();\n+\tif (checkout_fast_forward(from, to))\n+\t\texit(1); /* the callee should have complained already */\n+\tref_lock = lock_any_ref_for_update(\"HEAD\", from, 0);\n+\treturn write_ref_sha1(ref_lock, to, \"cherry-pick\");\n+}\n+\n+static int do_recursive_merge(struct commit *base, struct commit *next,\n+\t\t\t const char *base_label, const char *next_label,\n+\t\t\t unsigned char *head, struct strbuf *msgbuf,\n+\t\t\t struct replay_opts *opts)\n+{\n+\tstruct merge_options o;\n+\tstruct tree *result, *next_tree, *base_tree, *head_tree;\n+\tint clean, index_fd;\n+\tconst char **xopt;\n+\tstatic struct lock_file index_lock;\n+\n+\tindex_fd = hold_locked_index(&index_lock, 1);\n+\n+\tread_cache();\n+\n+\tinit_merge_options(&o);\n+\to.ancestor = base ? base_label : \"(empty tree)\";\n+\to.branch1 = \"HEAD\";\n+\to.branch2 = next ? next_label : \"(empty tree)\";\n+\n+\thead_tree = parse_tree_indirect(head);\n+\tnext_tree = next ? next->tree : empty_tree();\n+\tbase_tree = base ? base->tree : empty_tree();\n+\n+\tfor (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)\n+\t\tparse_merge_opt(&o, *xopt);\n+\n+\tclean = merge_trees(&o,\n+\t\t\t head_tree,\n+\t\t\t next_tree, base_tree, &result);\n+\n+\tif (active_cache_changed &&\n+\t (write_cache(index_fd, active_cache, active_nr) ||\n+\t commit_locked_index(&index_lock)))\n+\t\t/* TRANSLATORS: %s will be \"revert\" or \"cherry-pick\" */\n+\t\tdie(_(\"%s: Unable to write new index file\"), action_name(opts));\n+\trollback_lock_file(&index_lock);\n+\n+\tif (!clean) {\n+\t\tint i;\n+\t\tstrbuf_addstr(msgbuf, \"\\nConflicts:\\n\\n\");\n+\t\tfor (i = 0; i < active_nr;) {\n+\t\t\tstruct cache_entry *ce = active_cache[i++];\n+\t\t\tif (ce_stage(ce)) {\n+\t\t\t\tstrbuf_addch(msgbuf, '\\t');\n+\t\t\t\tstrbuf_addstr(msgbuf, ce->name);\n+\t\t\t\tstrbuf_addch(msgbuf, '\\n');\n+\t\t\t\twhile (i < active_nr && !strcmp(ce->name,\n+\t\t\t\t\t\tactive_cache[i]->name))\n+\t\t\t\t\ti++;\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\treturn !clean;\n+}\n+\n+/*\n+ * If we are cherry-pick, and if the merge did not result in\n+ * hand-editing, we will hit this commit and inherit the original\n+ * author date and name.\n+ * If we are revert, or if our cherry-pick results in a hand merge,\n+ * we had better say that the current user is responsible for that.\n+ */\n+static int run_git_commit(const char *defmsg, struct replay_opts *opts)\n+{\n+\t/* 6 is max possible length of our args array including NULL */\n+\tconst char *args[6];\n+\tint i = 0;\n+\n+\targs[i++] = \"commit\";\n+\targs[i++] = \"-n\";\n+\tif (opts->signoff)\n+\t\targs[i++] = \"-s\";\n+\tif (!opts->edit) {\n+\t\targs[i++] = \"-F\";\n+\t\targs[i++] = defmsg;\n+\t}\n+\targs[i] = NULL;\n+\n+\treturn run_command_v_opt(args, RUN_GIT_CMD);\n+}\n+\n+static int do_pick_commit(struct commit *commit, enum replay_action action,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tunsigned char head[20];\n+\tstruct commit *base, *next, *parent;\n+\tconst char *base_label, *next_label;\n+\tstruct commit_message msg = { NULL, NULL, NULL, NULL, NULL };\n+\tchar *defmsg = NULL;\n+\tstruct strbuf msgbuf = STRBUF_INIT;\n+\tint res;\n+\n+\tif (opts->no_commit) {\n+\t\t/*\n+\t\t * We do not intend to commit immediately. We just want to\n+\t\t * merge the differences in, so let's compute the tree\n+\t\t * that represents the \"current\" state for merge-recursive\n+\t\t * to work on.\n+\t\t */\n+\t\tif (write_cache_as_tree(head, 0, NULL))\n+\t\t\tdie (_(\"Your index file is unmerged.\"));\n+\t} else {\n+\t\tif (get_sha1(\"HEAD\", head))\n+\t\t\treturn error(_(\"You do not have a valid HEAD\"));\n+\t\tif (index_differs_from(\"HEAD\", 0))\n+\t\t\treturn error_dirty_index(opts);\n+\t}\n+\tdiscard_cache();\n+\n+\tif (!commit->parents) {\n+\t\tparent = NULL;\n+\t}\n+\telse if (commit->parents->next) {\n+\t\t/* Reverting or cherry-picking a merge commit */\n+\t\tint cnt;\n+\t\tstruct commit_list *p;\n+\n+\t\tif (!opts->mainline)\n+\t\t\treturn error(_(\"Commit %s is a merge but no -m option was given.\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1));\n+\n+\t\tfor (cnt = 1, p = commit->parents;\n+\t\t cnt != opts->mainline && p;\n+\t\t cnt++)\n+\t\t\tp = p->next;\n+\t\tif (cnt != opts->mainline || !p)\n+\t\t\treturn error(_(\"Commit %s does not have parent %d\"),\n+\t\t\t\tsha1_to_hex(commit->object.sha1), opts->mainline);\n+\t\tparent = p->item;\n+\t} else if (0 < opts->mainline)\n+\t\treturn error(_(\"Mainline was specified but commit %s is not a merge.\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n+\telse\n+\t\tparent = commit->parents->item;\n+\n+\tif (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))\n+\t\treturn fast_forward_to(commit->object.sha1, head);\n+\n+\tif (parent && parse_commit(parent) < 0)\n+\t\t/* TRANSLATORS: The first %s will be \"revert\" or\n+\t\t \"cherry-pick\", the second %s a SHA1 */\n+\t\treturn error(_(\"%s: cannot parse parent commit %s\"),\n+\t\t\taction_name(opts), sha1_to_hex(parent->object.sha1));\n+\n+\tif (get_message(commit, &msg) != 0)\n+\t\treturn error(_(\"Cannot get commit message for %s\"),\n+\t\t\tsha1_to_hex(commit->object.sha1));\n+\n+\t/*\n+\t * \"commit\" is an existing commit. We would want to apply\n+\t * the difference it introduces since its first parent \"prev\"\n+\t * on top of the current HEAD if we are cherry-pick. Or the\n+\t * reverse of it if we are revert.\n+\t */\n+\n+\tdefmsg = git_pathdup(\"MERGE_MSG\");\n+\n+\tif (action == REPLAY_REVERT) {\n+\t\tbase = commit;\n+\t\tbase_label = msg.label;\n+\t\tnext = parent;\n+\t\tnext_label = msg.parent_label;\n+\t\tstrbuf_addstr(&msgbuf, \"Revert \\\"\");\n+\t\tstrbuf_addstr(&msgbuf, msg.subject);\n+\t\tstrbuf_addstr(&msgbuf, \"\\\"\\n\\nThis reverts commit \");\n+\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n+\n+\t\tif (commit->parents && commit->parents->next) {\n+\t\t\tstrbuf_addstr(&msgbuf, \", reversing\\nchanges made to \");\n+\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));\n+\t\t}\n+\t\tstrbuf_addstr(&msgbuf, \".\\n\");\n+\t} else {\n+\t\tconst char *p;\n+\n+\t\tbase = parent;\n+\t\tbase_label = msg.parent_label;\n+\t\tnext = commit;\n+\t\tnext_label = msg.label;\n+\n+\t\t/*\n+\t\t * Append the commit log message to msgbuf; it starts\n+\t\t * after the tree, parent, author, committer\n+\t\t * information followed by \"\\n\\n\".\n+\t\t */\n+\t\tp = strstr(msg.message, \"\\n\\n\");\n+\t\tif (p) {\n+\t\t\tp += 2;\n+\t\t\tstrbuf_addstr(&msgbuf, p);\n+\t\t}\n+\n+\t\tif (opts->record_origin) {\n+\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n+\t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n+\t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n+\t\t}\n+\t}\n+\n+\tif (!opts->strategy || !strcmp(opts->strategy, \"recursive\") || action == REPLAY_REVERT) {\n+\t\tres = do_recursive_merge(base, next, base_label, next_label,\n+\t\t\t\t\t head, &msgbuf, opts);\n+\t\twrite_message(&msgbuf, defmsg);\n+\t} else {\n+\t\tstruct commit_list *common = NULL;\n+\t\tstruct commit_list *remotes = NULL;\n+\n+\t\twrite_message(&msgbuf, defmsg);\n+\n+\t\tcommit_list_insert(base, &common);\n+\t\tcommit_list_insert(next, &remotes);\n+\t\tres = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,\n+\t\t\t\t\tcommon, sha1_to_hex(head), remotes);\n+\t\tfree_commit_list(common);\n+\t\tfree_commit_list(remotes);\n+\t}\n+\n+\t/*\n+\t * If the merge was clean or if it failed due to conflict, we write\n+\t * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.\n+\t * However, if the merge did not even start, then we don't want to\n+\t * write it at all.\n+\t */\n+\tif (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))\n+\t\twrite_cherry_pick_head(commit);\n+\n+\tif (res) {\n+\t\terror(action == REPLAY_REVERT\n+\t\t ? _(\"could not revert %s... %s\")\n+\t\t : _(\"could not apply %s... %s\"),\n+\t\t find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),\n+\t\t msg.subject);\n+\t\tprint_advice(res == 1);\n+\t\trerere(opts->allow_rerere_auto);\n+\t} else {\n+\t\tif (!opts->no_commit)\n+\t\t\tres = run_git_commit(defmsg, opts);\n+\t}\n+\n+\tfree_message(&msg);\n+\tfree(defmsg);\n+\n+\treturn res;\n+}\n+\n+static void prepare_revs(struct replay_opts *opts)\n+{\n+\tif (opts->action != REPLAY_REVERT)\n+\t\topts->revs->reverse ^= 1;\n+\n+\tif (prepare_revision_walk(opts->revs))\n+\t\tdie(_(\"revision walk setup failed\"));\n+\n+\tif (!opts->revs->commits)\n+\t\tdie(_(\"empty commit set passed\"));\n+}\n+\n+static void read_and_refresh_cache(struct replay_opts *opts)\n+{\n+\tstatic struct lock_file index_lock;\n+\tint index_fd = hold_locked_index(&index_lock, 0);\n+\tif (read_index_preload(&the_index, NULL) < 0)\n+\t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n+\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n+\tif (the_index.cache_changed) {\n+\t\tif (write_index(&the_index, index_fd) ||\n+\t\t commit_locked_index(&index_lock))\n+\t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n+\t}\n+\trollback_lock_file(&index_lock);\n+}\n+\n+/*\n+ * Append a commit to the end of the commit_list.\n+ *\n+ * next starts by pointing to the variable that holds the head of an\n+ * empty commit_list, and is updated to point to the \"next\" field of\n+ * the last item on the list as new commits are appended.\n+ *\n+ * Usage example:\n+ *\n+ * struct commit_list *list;\n+ * struct commit_list **next = &list;\n+ *\n+ * next = commit_list_append(c1, next);\n+ * next = commit_list_append(c2, next);\n+ * assert(commit_list_count(list) == 2);\n+ * return list;\n+ */\n+static struct replay_insn_list **replay_insn_list_append(enum replay_action action,\n+\t\t\t\t\t\tstruct commit *operand,\n+\t\t\t\t\t\tstruct replay_insn_list **next)\n+{\n+\tstruct replay_insn_list *new = xmalloc(sizeof(*new));\n+\tnew->action = action;\n+\tnew->operand = operand;\n+\t*next = new;\n+\tnew->next = NULL;\n+\treturn &new->next;\n+}\n+\n+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)\n+{\n+\tstruct replay_insn_list *cur;\n+\n+\tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tconst char *sha1_abbrev, *action_str, *subject;\n+\t\tint subject_len;\n+\n+\t\taction_str = cur->action == REPLAY_REVERT ? \"revert\" : \"pick\";\n+\t\tsha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);\n+\t\tsubject_len = find_commit_subject(cur->operand->buffer, &subject);\n+\t\tstrbuf_addf(buf, \"%s %s %.*s\\n\", action_str, sha1_abbrev,\n+\t\t\tsubject_len, subject);\n+\t}\n+\treturn 0;\n+}\n+\n+static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)\n+{\n+\tunsigned char commit_sha1[20];\n+\tchar *end_of_object_name;\n+\tint saved, status;\n+\n+\tif (!prefixcmp(bol, \"pick \")) {\n+\t\titem->action = REPLAY_PICK;\n+\t\tbol += strlen(\"pick \");\n+\t} else if (!prefixcmp(bol, \"revert \")) {\n+\t\titem->action = REPLAY_REVERT;\n+\t\tbol += strlen(\"revert \");\n+\t} else {\n+\t\tsize_t len = strchrnul(bol, '\\n') - bol;\n+\t\tif (len > 255)\n+\t\t\tlen = 255;\n+\t\treturn error(_(\"Unrecognized action: %.*s\"), (int)len, bol);\n+\t}\n+\n+\tend_of_object_name = bol + strcspn(bol, \" \\n\");\n+\tsaved = *end_of_object_name;\n+\t*end_of_object_name = '\\0';\n+\tstatus = get_sha1(bol, commit_sha1);\n+\t*end_of_object_name = saved;\n+\n+\tif (status < 0)\n+\t\treturn error(_(\"Malformed object name: %s\"), bol);\n+\n+\titem->operand = lookup_commit_reference(commit_sha1);\n+\tif (!item->operand)\n+\t\treturn error(_(\"Not a valid commit: %s\"), bol);\n+\n+\titem->next = NULL;\n+\treturn 0;\n+}\n+\n+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)\n+{\n+\tstruct replay_insn_list **next = todo_list;\n+\tstruct replay_insn_list item = {0, NULL, NULL};\n+\tchar *p = buf;\n+\tint i;\n+\n+\tfor (i = 1; *p; i++) {\n+\t\tchar *eol = strchrnul(p, '\\n');\n+\t\tif (parse_insn_line(p, eol, &item) < 0)\n+\t\t\treturn error(_(\"on line %d.\"), i);\n+\t\tnext = replay_insn_list_append(item.action, item.operand, next);\n+\t\tp = *eol ? eol + 1 : eol;\n+\t}\n+\tif (!*todo_list)\n+\t\treturn error(_(\"No commits parsed.\"));\n+\treturn 0;\n+}\n+\n+static void read_populate_todo(struct replay_insn_list **todo_list)\n+{\n+\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd, res;\n+\n+\tfd = open(todo_file, O_RDONLY);\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"Could not open %s.\"), todo_file);\n+\tif (strbuf_read(&buf, fd, 0) < 0) {\n+\t\tclose(fd);\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Could not read %s.\"), todo_file);\n+\t}\n+\tclose(fd);\n+\n+\tres = parse_insn_buffer(buf.buf, todo_list);\n+\tstrbuf_release(&buf);\n+\tif (res)\n+\t\tdie(_(\"Unusable instruction sheet: %s\"), todo_file);\n+}\n+\n+static int populate_opts_cb(const char *key, const char *value, void *data)\n+{\n+\tstruct replay_opts *opts = data;\n+\tint error_flag = 1;\n+\n+\tif (!value)\n+\t\terror_flag = 0;\n+\telse if (!strcmp(key, \"options.no-commit\"))\n+\t\topts->no_commit = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.edit\"))\n+\t\topts->edit = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.signoff\"))\n+\t\topts->signoff = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.record-origin\"))\n+\t\topts->record_origin = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.allow-ff\"))\n+\t\topts->allow_ff = git_config_bool_or_int(key, value, &error_flag);\n+\telse if (!strcmp(key, \"options.mainline\"))\n+\t\topts->mainline = git_config_int(key, value);\n+\telse if (!strcmp(key, \"options.strategy\"))\n+\t\tgit_config_string(&opts->strategy, key, value);\n+\telse if (!strcmp(key, \"options.strategy-option\")) {\n+\t\tALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);\n+\t\topts->xopts[opts->xopts_nr++] = xstrdup(value);\n+\t} else\n+\t\treturn error(_(\"Invalid key: %s\"), key);\n+\n+\tif (!error_flag)\n+\t\treturn error(_(\"Invalid value for %s: %s\"), key, value);\n+\n+\treturn 0;\n+}\n+\n+static void read_populate_opts(struct replay_opts **opts_ptr)\n+{\n+\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\n+\tif (!file_exists(opts_file))\n+\t\treturn;\n+\tif (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)\n+\t\tdie(_(\"Malformed options sheet: %s\"), opts_file);\n+}\n+\n+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n+\t\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct commit *commit;\n+\tstruct replay_insn_list **next;\n+\n+\tprepare_revs(opts);\n+\n+\tnext = todo_list;\n+\twhile ((commit = get_revision(opts->revs)))\n+\t\tnext = replay_insn_list_append(opts->action, commit, next);\n+}\n+\n+static int create_seq_dir(void)\n+{\n+\tconst char *seq_dir = git_path(SEQ_DIR);\n+\n+\tif (file_exists(seq_dir))\n+\t\treturn error(_(\"%s already exists.\"), seq_dir);\n+\telse if (mkdir(seq_dir, 0777) < 0)\n+\t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n+\treturn 0;\n+}\n+\n+static void save_head(const char *head)\n+{\n+\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n+\tstatic struct lock_file head_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n+\tstrbuf_addf(&buf, \"%s\\n\", head);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0)\n+\t\tdie_errno(_(\"Could not write to %s.\"), head_file);\n+\tif (commit_lock_file(&head_lock) < 0)\n+\t\tdie(_(\"Error wrapping up %s.\"), head_file);\n+}\n+\n+static void save_todo(struct replay_insn_list *todo_list)\n+{\n+\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n+\tstatic struct lock_file todo_lock;\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tint fd;\n+\n+\tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n+\tif (format_todo(&buf, todo_list) < 0)\n+\t\tdie(_(\"Could not format %s.\"), todo_file);\n+\tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie_errno(_(\"Could not write to %s.\"), todo_file);\n+\t}\n+\tif (commit_lock_file(&todo_lock) < 0) {\n+\t\tstrbuf_release(&buf);\n+\t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n+\t}\n+\tstrbuf_release(&buf);\n+}\n+\n+static void save_opts(struct replay_opts *opts)\n+{\n+\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\n+\tif (opts->no_commit)\n+\t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n+\tif (opts->edit)\n+\t\tgit_config_set_in_file(opts_file, \"options.edit\", \"true\");\n+\tif (opts->signoff)\n+\t\tgit_config_set_in_file(opts_file, \"options.signoff\", \"true\");\n+\tif (opts->record_origin)\n+\t\tgit_config_set_in_file(opts_file, \"options.record-origin\", \"true\");\n+\tif (opts->allow_ff)\n+\t\tgit_config_set_in_file(opts_file, \"options.allow-ff\", \"true\");\n+\tif (opts->mainline) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tstrbuf_addf(&buf, \"%d\", opts->mainline);\n+\t\tgit_config_set_in_file(opts_file, \"options.mainline\", buf.buf);\n+\t\tstrbuf_release(&buf);\n+\t}\n+\tif (opts->strategy)\n+\t\tgit_config_set_in_file(opts_file, \"options.strategy\", opts->strategy);\n+\tif (opts->xopts) {\n+\t\tint i;\n+\t\tfor (i = 0; i < opts->xopts_nr; i++)\n+\t\t\tgit_config_set_multivar_in_file(opts_file,\n+\t\t\t\t\t\t\t\"options.strategy-option\",\n+\t\t\t\t\t\t\topts->xopts[i], \"^$\", 0);\n+\t}\n+}\n+\n+static int pick_commits(struct replay_insn_list *todo_list,\n+\t\t\tstruct replay_opts *opts)\n+{\n+\tstruct replay_insn_list *cur;\n+\tint res;\n+\n+\tsetenv(GIT_REFLOG_ACTION, action_name(opts), 0);\n+\tif (opts->allow_ff)\n+\t\tassert(!(opts->signoff || opts->no_commit ||\n+\t\t\t\topts->record_origin || opts->edit));\n+\tread_and_refresh_cache(opts);\n+\n+\tfor (cur = todo_list; cur; cur = cur->next) {\n+\t\tsave_todo(cur);\n+\t\tres = do_pick_commit(cur->operand, cur->action, opts);\n+\t\tif (res) {\n+\t\t\tif (!cur->next)\n+\t\t\t\t/*\n+\t\t\t\t * An error was encountered while\n+\t\t\t\t * picking the last commit; the\n+\t\t\t\t * sequencer state is useless now --\n+\t\t\t\t * the user simply needs to resolve\n+\t\t\t\t * the conflict and commit\n+\t\t\t\t */\n+\t\t\t\tremove_sequencer_state(0);\n+\t\t\treturn res;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Sequence of picks finished successfully; cleanup by\n+\t * removing the .git/sequencer directory\n+\t */\n+\tremove_sequencer_state(1);\n+\treturn 0;\n+}\n+\n+int sequencer_pick_revisions(struct replay_opts *opts)\n+{\n+\tstruct replay_insn_list *todo_list = NULL;\n+\tunsigned char sha1[20];\n+\n+\tif (opts->subcommand == REPLAY_NONE)\n+\t\tassert(opts->revs);\n+\n+\tread_and_refresh_cache(opts);\n+\n+\t/*\n+\t * Decide what to do depending on the arguments; a fresh\n+\t * cherry-pick should be handled differently from an existing\n+\t * one that is being continued\n+\t */\n+\tif (opts->subcommand == REPLAY_RESET) {\n+\t\tremove_sequencer_state(1);\n+\t\treturn 0;\n+\t} else if (opts->subcommand == REPLAY_CONTINUE) {\n+\t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\t\t\tgoto error;\n+\t\tread_populate_opts(&opts);\n+\t\tread_populate_todo(&todo_list);\n+\n+\t\t/* Verify that the conflict has been resolved */\n+\t\tif (!index_differs_from(\"HEAD\", 0))\n+\t\t\ttodo_list = todo_list->next;\n+\t} else {\n+\t\t/*\n+\t\t * Start a new cherry-pick/ revert sequence; but\n+\t\t * first, make sure that an existing one isn't in\n+\t\t * progress\n+\t\t */\n+\n+\t\twalk_revs_populate_todo(&todo_list, opts);\n+\t\tif (create_seq_dir() < 0) {\n+\t\t\terror(_(\"A cherry-pick or revert is in progress.\"));\n+\t\t\tadvise(_(\"Use --continue to continue the operation\"));\n+\t\t\tadvise(_(\"or --reset to forget about it\"));\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (get_sha1(\"HEAD\", sha1)) {\n+\t\t\tif (opts->action == REPLAY_REVERT)\n+\t\t\t\treturn error(_(\"Can't revert as initial commit\"));\n+\t\t\treturn error(_(\"Can't cherry-pick into empty head\"));\n+\t\t}\n+\t\tsave_head(sha1_to_hex(sha1));\n+\t\tsave_opts(opts);\n+\t}\n+\treturn pick_commits(todo_list, opts);\n+error:\n+\treturn error(_(\"No %s in progress\"), action_name(opts));\n+}\ndiff --git a/sequencer.h b/sequencer.h\nindex f4db257..92b2d63 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -8,6 +8,30 @@\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n \n enum replay_action { REPLAY_REVERT, REPLAY_PICK };\n+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n+\n+struct replay_opts {\n+\tenum replay_action action;\n+\tenum replay_subcommand subcommand;\n+\n+\t/* Boolean options */\n+\tint edit;\n+\tint record_origin;\n+\tint no_commit;\n+\tint signoff;\n+\tint allow_ff;\n+\tint allow_rerere_auto;\n+\n+\tint mainline;\n+\n+\t/* Merge strategy */\n+\tconst char *strategy;\n+\tconst char **xopts;\n+\tsize_t xopts_nr, xopts_alloc;\n+\n+\t/* Only used by REPLAY_NONE */\n+\tstruct rev_info *revs;\n+};\n \n struct replay_insn_list {\n \tenum replay_action action;\n@@ -25,4 +49,6 @@ struct replay_insn_list {\n */\n void remove_sequencer_state(int aggressive);\n \n+int sequencer_pick_revisions(struct replay_opts *opts);\n+\n #endif\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178911","messageId":"1320510586-3940-3-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:43Z","receivedAt":"2011-11-05T16:29:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when\ninvoked aggressively, since we want to treat it as part of the\nsequencer state now. While at it, make some minor improvements to the\nfunction.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 27 ++++++++++++++++-----------\n 1 files changed, 16 insertions(+), 11 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 87f146b..e566043 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -25,17 +25,22 @@ static char *get_encoding(const char *message);\n \n void remove_sequencer_state(int aggressive)\n {\n-\tstruct strbuf seq_dir = STRBUF_INIT;\n-\tstruct strbuf seq_old_dir = STRBUF_INIT;\n-\n-\tstrbuf_addf(&seq_dir, \"%s\", git_path(SEQ_DIR));\n-\tstrbuf_addf(&seq_old_dir, \"%s\", git_path(SEQ_OLD_DIR));\n-\tremove_dir_recursively(&seq_old_dir, 0);\n-\trename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));\n-\tif (aggressive)\n-\t\tremove_dir_recursively(&seq_old_dir, 0);\n-\tstrbuf_release(&seq_dir);\n-\tstrbuf_release(&seq_old_dir);\n+\tconst char *seq_dir = git_path(SEQ_DIR);\n+\tconst char *seq_old_dir = git_path(SEQ_OLD_DIR);\n+\tconst char *cherry_pick_head = git_path(\"CHERRY_PICK_HEAD\");\n+\tstruct strbuf seq_dir_buf = STRBUF_INIT;\n+\tstruct strbuf seq_old_dir_buf = STRBUF_INIT;\n+\n+\tstrbuf_addf(&seq_dir_buf, \"%s\", seq_dir);\n+\tstrbuf_addf(&seq_old_dir_buf, \"%s\", seq_old_dir);\n+\tremove_dir_recursively(&seq_old_dir_buf, 0);\n+\trename(seq_dir, seq_old_dir);\n+\tif (aggressive) {\n+\t\tremove_dir_recursively(&seq_old_dir_buf, 0);\n+\t\tunlink(cherry_pick_head);\n+\t}\n+\tstrbuf_release(&seq_dir_buf);\n+\tstrbuf_release(&seq_old_dir_buf);\n }\n \n struct commit_message {\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178913","messageId":"1320510586-3940-4-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:44Z","receivedAt":"2011-11-05T16:29:44Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Later in the series, we will not write '.git/sequencer/todo' for a\nsingle commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains\nthis information. So, stomp the sequencer state in create_seq_state()\nunless the todo file is present.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 10 +++++++---\n 1 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e566043..517eb23 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n \n static int create_seq_dir(void)\n {\n+\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tconst char *seq_dir = git_path(SEQ_DIR);\n \n-\tif (file_exists(seq_dir))\n-\t\treturn error(_(\"%s already exists.\"), seq_dir);\n-\telse if (mkdir(seq_dir, 0777) < 0)\n+\tif (file_exists(todo_file))\n+\t\treturn error(_(\"%s already exists.\"), todo_file);\n+\n+\t/* If todo_file doesn't exist, discard sequencer state */\n+\tremove_sequencer_state(1);\n+\tif (mkdir(seq_dir, 0777) < 0)\n \t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n \treturn 0;\n }\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178915","messageId":"1320510586-3940-5-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/5] sequencer: handle single commit pick separately","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:45Z","receivedAt":"2011-11-05T16:29:45Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already\ncontains this information. However, '.git/sequencer/opts' and\n'.git/sequencer/head' are required to support '--reset' and\n'--continue' operations.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 517eb23..6762ceb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,\n \t\t\t\topts->record_origin || opts->edit));\n \tread_and_refresh_cache(opts);\n \n+\t/*\n+\t * Backward compatibility hack: when only a single commit is\n+\t * picked, don't save_todo(), because CHERRY_PICK_HEAD will\n+\t * contain this information anyway.\n+\t */\n+\tif (opts->subcommand == REPLAY_NONE &&\n+\t\ttodo_list->next == NULL && todo_list->action == REPLAY_PICK)\n+\t\treturn do_pick_commit(todo_list->operand, REPLAY_PICK, opts);\n+\n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur);\n \t\tres = do_pick_commit(cur->operand, cur->action, opts);\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178914","messageId":"1320510586-3940-6-git-send-email-artagnon@gmail.com","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 5/5] sequencer: revert d3f4628e","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-05T16:29:46Z","receivedAt":"2011-11-05T16:29:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Revert d3f4628e (revert: Remove sequencer state when no commits are\npending, 2011-06-06), because this is not the right approach. Instead\nof increasing coupling between the sequencer and 'git commit', a\nunified '--continue' that invokes 'git commit' on behalf of the\nend-user is preferred.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 12 +-----------\n t/t3510-cherry-pick-sequence.sh | 24 ------------------------\n 2 files changed, 1 insertions(+), 35 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 6762ceb..7caa550 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -758,18 +758,8 @@ static int pick_commits(struct replay_insn_list *todo_list,\n \tfor (cur = todo_list; cur; cur = cur->next) {\n \t\tsave_todo(cur);\n \t\tres = do_pick_commit(cur->operand, cur->action, opts);\n-\t\tif (res) {\n-\t\t\tif (!cur->next)\n-\t\t\t\t/*\n-\t\t\t\t * An error was encountered while\n-\t\t\t\t * picking the last commit; the\n-\t\t\t\t * sequencer state is useless now --\n-\t\t\t\t * the user simply needs to resolve\n-\t\t\t\t * the conflict and commit\n-\t\t\t\t */\n-\t\t\t\tremove_sequencer_state(0);\n+\t\tif (res)\n \t\t\treturn res;\n-\t\t}\n \t}\n \n \t/*\ndiff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh\nindex 4b12244..b30f13a 100755\n--- a/t/t3510-cherry-pick-sequence.sh\n+++ b/t/t3510-cherry-pick-sequence.sh\n@@ -85,30 +85,6 @@ test_expect_success '--reset cleans up sequencer state' '\n \ttest_path_is_missing .git/sequencer\n '\n \n-test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '\n-\tpristine_detach initial &&\n-\ttest_must_fail git cherry-pick base..picked &&\n-\ttest_path_is_missing .git/sequencer &&\n-\techo \"resolved\" >foo &&\n-\tgit add foo &&\n-\tgit commit &&\n-\t{\n-\t\tgit rev-list HEAD |\n-\t\tgit diff-tree --root --stdin |\n-\t\tsed \"s/$_x40/OBJID/g\"\n-\t} >actual &&\n-\tcat >expect <<-\\EOF &&\n-\tOBJID\n-\t:100644 100644 OBJID OBJID M\tfoo\n-\tOBJID\n-\t:100644 100644 OBJID OBJID M\tunrelated\n-\tOBJID\n-\t:000000 100644 OBJID OBJID A\tfoo\n-\t:000000 100644 OBJID OBJID A\tunrelated\n-\tEOF\n-\ttest_cmp expect actual\n-'\n-\n test_expect_success 'cherry-pick does not implicitly stomp an existing operation' '\n \tpristine_detach initial &&\n \ttest_must_fail git cherry-pick base..anotherpick &&\n-- \n1.7.6.351.gb35ac.dirty\n"},{"id":"178929","messageId":"20111105234312.GB27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/5] Sequencer: working around historical mistakes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-05T23:43:12Z","receivedAt":"2011-11-05T23:43:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hey,\n\nRamkumar Ramachandra wrote:\n\n> As described in the discussion following $gmane/179304/focus=179383,\n> we have decided to handle historical hacks in the sequencer itself.\n> This series that follows is one step in the right direction.\n\nI'm not sure what the above means. But let's see what the patches\nsay. :)\n\n[...]\n> 2. This series depends on rr/revert-cherry-pick, but doesn't apply to\n> the current 'next'- sorry, rebasing is a massive pita due to 1/5.\n\nShouldn't it be based against rr/revert-cherry-pick, rather than\n\"next\" which is more of a moving target?\n\nThanks,\nJonathan\n"},{"id":"178934","messageId":"20111106001232.GC27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T00:12:32Z","receivedAt":"2011-11-06T00:12:32Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Start building the generalized sequencer by moving code from revert.c\n> into sequencer.c and sequencer.h. Make the builtin responsible only\n> for command-line parsing, and expose a new sequencer_pick_revisions()\n> to do the actual work of sequencing commits.\n>\n> This is intended to be almost a pure code movement patch with no\n> functional changes. Check with:\n\nDo I understand correctly that the purpose of this patch is to expose\nsome functions through the \"sequencer.h\" API, which patches later in\nthe series will use? Which functions? What is this generalized\nsequencer which we are starting to build? Why should I be happy about\n(or care about, for that matter) code having moved from one source\nfile to another?\n\nRule of thumb for commit messages: after reading a commit message, I\nshould be able to predict what the patch will do, without reading the\npatch.\n\nI am guessing the above description started sane and then went through\na few revisions without a person reading it all the way through again.\nPlease consider just rewriting it.\n\n[...]\n> --- a/builtin/revert.c\n> +++ b/builtin/revert.c\n> @@ -1,19 +1,9 @@\n> #include \"cache.h\"\n> #include \"builtin.h\"\n> -#include \"object.h\"\n> -#include \"commit.h\"\n> -#include \"tag.h\"\n> -#include \"run-command.h\"\n> -#include \"exec_cmd.h\"\n> -#include \"utf8.h\"\n> #include \"parse-options.h\"\n> -#include \"cache-tree.h\"\n> #include \"diff.h\"\n> #include \"revision.h\"\n> #include \"rerere.h\"\n> -#include \"merge-recursive.h\"\n> -#include \"refs.h\"\n> -#include \"dir.h\"\n> #include \"sequencer.h\"\n\nHoorah!\n\n[snipping lots of deletion of code from builtin/revert.c]\n> @@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)\n> \topts.action = REPLAY_REVERT;\n> \tgit_config(git_default_config, NULL);\n> \tparse_args(argc, argv, &opts);\n> -\tres = pick_revisions(&opts);\n> +\tres = sequencer_pick_revisions(&opts);\n\nThe new sequencer_pick_revisions is just a new name for the old\npick_revisions. Sane, but probably worth mentioning in the log\nmessage.\n\n[...]\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1,7 +1,27 @@\n> #include \"cache.h\"\n> +#include \"object.h\"\n> +#include \"commit.h\"\n> +#include \"tag.h\"\n> +#include \"run-command.h\"\n> +#include \"exec_cmd.h\"\n> +#include \"utf8.h\"\n> +#include \"cache-tree.h\"\n> +#include \"diff.h\"\n> +#include \"revision.h\"\n> +#include \"rerere.h\"\n> +#include \"merge-recursive.h\"\n> +#include \"refs.h\"\n> -#include \"sequencer.h\"\n> -#include \"strbuf.h\"\n> #include \"dir.h\"\n> +#include \"sequencer.h\"\n\nWhy did sequencer.h move to after dir.h? Wow, we use a lot of headers\nhere --- I wonder if there are some pieces that could be split out\n(that's not due to your patch, though).\n\n[...]\n> --- a/sequencer.h\n> +++ b/sequencer.h\n> @@ -8,6 +8,30 @@\n> #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n> \n> enum replay_action { REPLAY_REVERT, REPLAY_PICK };\n> +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };\n> +\n> +struct replay_opts {\n[...]\n> @@ -25,4 +49,6 @@ struct replay_insn_list {\n> */\n> void remove_sequencer_state(int aggressive);\n> \n> +int sequencer_pick_revisions(struct replay_opts *opts);\n\nAh, so this moves most of the logic of \"git cherry-pick\" to the sequencer\nbut the only new API that needs to be exposed is pick_revisions(). The\ncalling sequence looks like this:\n\n\tmemset(&opts, o, sizeof(opts));\n\topts.action = REPLAY_PICK;\n\topts.revs = xmalloc(sizeof(*opts.revs));\n\n\tinit_revisions(opts.revs);\n\tadd_pending_object / setup_revisions / etc\n\n\tsequencer_pick_revisions(&opts);\n\nThe small exposed interface makes this a relatively uninvasive patch,\nand the immediate advantage is that we plan to reuse some of the\nfunctionality used in pick_revisions() in other, new APIs to be used\nby commands other than cherry-pick. No functional change yet intended.\n\nExcept for the commit message, looks reasonable (though I haven't\ntried the \"git blame\" magic to check the code movement part). Thanks.\n"},{"id":"178935","messageId":"20111106001538.GD27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/5] sequencer: remove CHERRY_PICK_HEAD with sequencer state","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T00:15:38Z","receivedAt":"2011-11-06T00:15:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Make remove_sequencer_state() remove '.git/CHERRY_PICK_HEAD' when\n> invoked aggressively, since we want to treat it as part of the\n> sequencer state now. While at it, make some minor improvements to the\n> function.\n\nWhat does it mean to invoke a function aggressively? What is the\nnature of these minor improvements (are they behavior changes or just\ncleanups)? (Remember, the reader hasn't seen the patch yet.)\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -25,17 +25,22 @@ static char *get_encoding(const char *message);\n> \n> void remove_sequencer_state(int aggressive)\n> {\n> +\tconst char *seq_dir = git_path(SEQ_DIR);\n> +\tconst char *seq_old_dir = git_path(SEQ_OLD_DIR);\n> +\tconst char *cherry_pick_head = git_path(\"CHERRY_PICK_HEAD\");\n\nIf there were just two more like this, the behavior would change\ncompletely. Scary. Are these temporary variables needed?\n"},{"id":"178937","messageId":"20111106002645.GE27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T00:26:45Z","receivedAt":"2011-11-06T00:26:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Later in the series, we will not write '.git/sequencer/todo' for a\n> single commit cherry-pick, because 'CHERRY_PICK_HEAD' already contains\n> this information. So, stomp the sequencer state in create_seq_state()\n> unless the todo file is present.\n\nWhat problem does this solve? How does it solve it? What does it\nmean to stomp?\n\nThe usual commit-message debugging strategy applies here: imagine you\nare a BIOS clone manufacturer, and for legal reasons you are not\nallowed to read this part of the git implementation embedded in the\nstandard BIOS. However, you are allowed to read the commit message,\nand if that message is clear enough, it will explain the purpose and\nbehavior of that code and you will be able to implement a compatible\nimplementation addressing the same problem without scratching your\nhead too much.\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n> \n> static int create_seq_dir(void)\n> {\n> +\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n> \tconst char *seq_dir = git_path(SEQ_DIR);\n\nScary idiom.\n\n> -\tif (file_exists(seq_dir))\n> -\t\treturn error(_(\"%s already exists.\"), seq_dir);\n> -\telse if (mkdir(seq_dir, 0777) < 0)\n> +\tif (file_exists(todo_file))\n> +\t\treturn error(_(\"%s already exists.\"), todo_file);\n> +\n> +\t/* If todo_file doesn't exist, discard sequencer state */\n> +\tremove_sequencer_state(1);\n> +\tif (mkdir(seq_dir, 0777) < 0)\n> \t\tdie_errno(_(\"Could not create sequencer directory '%s'.\"), seq_dir);\n\nI guess this patch would make more sense after patch 4.\n"},{"id":"178939","messageId":"20111106003519.GF27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-5-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 4/5] sequencer: handle single commit pick separately","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T00:35:19Z","receivedAt":"2011-11-06T00:35:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Don't write a '.git/sequencer/todo', as CHERRY_PICK_HEAD already\n> contains this information. However, '.git/sequencer/opts' and\n> '.git/sequencer/head' are required to support '--reset' and\n> '--continue' operations.\n\nThis is meant as a signal to later \"git cherry-pick\" commands that it\nis okay to forget about the cherry-pick, right? How is the reader\nsupposed to know that? Say so!\n\nBy the way, it's not clear to me yet whether the resulting UI would be\nmore pleasant or not. What is the expected calling sequence? Any odd\ncorners of behavior changing? What happens if I do\n\n\tgit cherry-pick foo; # conflicts!\n\tgit cherry-pick bar; # just ignore them\n\nor\n\n\tgit cherry-pick foo; # conflicts! but resolved in index by rerere\n\tgit checkout something-else\n\nIs there any potential downside to the change?\n\n[...]\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -746,6 +746,15 @@ static int pick_commits(struct replay_insn_list *todo_list,\n> \t\t\t\topts->record_origin || opts->edit));\n> \tread_and_refresh_cache(opts);\n> \n> +\t/*\n> +\t * Backward compatibility hack: when only a single commit is\n> +\t * picked, don't save_todo(), because CHERRY_PICK_HEAD will\n> +\t * contain this information anyway.\n> +\t */\n\nHow does saving disk space by avoiding saving redundant information\naffect backward compatibility? I'm not sure what this comment is\ntrying to say.\n\n> +\tif (opts->subcommand == REPLAY_NONE &&\n> +\t\ttodo_list->next == NULL && todo_list->action == REPLAY_PICK)\n> +\t\treturn do_pick_commit(todo_list->operand, REPLAY_PICK, opts);\n> +\n> \tfor (cur = todo_list; cur; cur = cur->next) {\n"},{"id":"178941","messageId":"20111106004257.GG27272@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"1320510586-3940-6-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 5/5] sequencer: revert d3f4628e","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-06T00:42:57Z","receivedAt":"2011-11-06T00:42:57Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Revert d3f4628e (revert: Remove sequencer state when no commits are\n> pending, 2011-06-06), because this is not the right approach. Instead\n> of increasing coupling between the sequencer and 'git commit', a\n> unified '--continue' that invokes 'git commit' on behalf of the\n> end-user is preferred.\n\nForgive me for forgetting: what is the problem that d3f4628e was going\nto resolve (i.e., right approach to what)? What is this increased\ncoupling, and why do we want to avoid it? Is \"to prefer\" another word\nfor \"to implement\"? Who is being united by this new --continue\nswitch?\n\nIs this patch just reverting a previous patch? If so, why doesn't the\ncommit message use the usual format\n\n\tRevert \"\"\n\n\tThis reverts commit .\n\n\t\n\n?\n\n> sequencer.c | 12 +-----------\n> t/t3510-cherry-pick-sequence.sh | 24 ------------------------\n> 2 files changed, 1 insertions(+), 35 deletions(-)\n\nWhen changing behavior, it's more comforting to modify tests to describe\nthe new behavior than to just get rid of them. :)\n\nTo sum matters up: with a new commit message, patch 1 seems likely to\nbe ready. Patches 2-5 seem to need more work --- it's not clear to me\nyet what they are supposed to do.\n\nHope that helps,\nJonathan\n"},{"id":"178975","messageId":"7vlirt5aod.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"20111106004257.GG27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 5/5] sequencer: revert d3f4628e","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-06T19:10:42Z","receivedAt":"2011-11-06T19:10:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks for detailed reviews, Jonathan; looking forward for a re-roll, as I\nthink the general direction the series seems to be aiming to go is good.\n"},{"id":"179006","messageId":"CALkWK0k1AQt7Qr=rfpB7hRi-CNoHX2oUo4HR8eh09SwpnrXaCQ@mail.gmail.com","threadId":"28854","inReplyTo":"7vlirt5aod.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 5/5] sequencer: revert d3f4628e","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-07T06:06:20Z","receivedAt":"2011-11-07T06:06:20Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan and Junio,\n\nJonathan Nieder writes:\n> [...]\n\nJunio C Hamano writes:\n> Thanks for detailed reviews, Jonathan; looking forward for a re-roll, as I\n> think the general direction the series seems to be aiming to go is good.\n\nThanks for the early feedback! I'll polish the series this week.\n\n-- Ram\n"},{"id":"179353","messageId":"CALkWK0=QHUeKH6ccVLYJVW_RxXbEaLfwafTVzJ94+s49j=8QjA@mail.gmail.com","threadId":"28854","inReplyTo":"20111106004257.GG27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 5/5] sequencer: revert d3f4628e","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-12T16:13:13Z","receivedAt":"2011-11-12T16:13:13Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder writes:\n> Is this patch just reverting a previous patch?  If so, why doesn't the\n> commit message use the usual format\n>\n>        Revert \"\"\n>\n>        This reverts commit .\n>\n>        \n> [...]\n\nI'd have loved to use 'git revert d3f4628e', but that ends up creating\na lot more work: recall the big move made by 1/5? I'm trying to\n\"effectively port the inverse of the changes made by d3f4628e in\nrevert.c to sequencer.c\" -- would you still like to see a git-revert\nstyle commit message? Don't you think it'll be misleading?\n\nSorry about the shoddy commit messages though: I'm polishing the\nseries now that I'm convinced that it's heading in the right\ndirection. Hopefully, I'll have more to show soon.\n\nThanks.\n\n-- Ram\n"},{"id":"179364","messageId":"20111112224012.GA31766@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"CALkWK0=QHUeKH6ccVLYJVW_RxXbEaLfwafTVzJ94+s49j=8QjA@mail.gmail.com","subject":"Re: [PATCH 5/5] sequencer: revert d3f4628e","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-12T22:40:12Z","receivedAt":"2011-11-12T22:40:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> I'm trying to\n> \"effectively port the inverse of the changes made by d3f4628e in\n> revert.c to sequencer.c\" -- would you still like to see a git-revert\n> style commit message? Don't you think it'll be misleading?\n\nMy main complaint is that the subject line (and then the body) didn't\ntell me what effect the patch would have in a self-contained way.\n\nI don't think a git-revert style commit message would be misleading.\nCouldn't you avoid confusing people by providing the relevant\ninformation directly? \"This commit was not made with 'git revert',\nsince there has been too much code reorganization in the meantime;\ninstead, I applied the inverse of the changes made by d3f4628e by\nhand. This patch also tweaks the test added in that commit instead of\nremoving it.\"\n\n> Sorry about the shoddy commit messages though: I'm polishing the\n> series now that I'm convinced that it's heading in the right\n> direction. Hopefully, I'll have more to show soon.\n\nThanks. I'll try not to be distracted and to just focus on the code\nfor the next round.\n"},{"id":"179372","messageId":"CALkWK0n7v15n_s3CNq1Qu3LHjYkV-ENAkv2b+oB+VBkyV+Sphw@mail.gmail.com","threadId":"28854","inReplyTo":"20111106001232.GC27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-13T10:40:12Z","receivedAt":"2011-11-13T10:40:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nSmall note.\n\nJonathan Nieder writes:\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -1,7 +1,27 @@\n>>  #include \"cache.h\"\n>> +#include \"object.h\"\n>> +#include \"commit.h\"\n>> +#include \"tag.h\"\n>> +#include \"run-command.h\"\n>> +#include \"exec_cmd.h\"\n>> +#include \"utf8.h\"\n>> +#include \"cache-tree.h\"\n>> +#include \"diff.h\"\n>> +#include \"revision.h\"\n>> +#include \"rerere.h\"\n>> +#include \"merge-recursive.h\"\n>> +#include \"refs.h\"\n>> -#include \"sequencer.h\"\n>> -#include \"strbuf.h\"\n>>  #include \"dir.h\"\n>> +#include \"sequencer.h\"\n>\n> Why did sequencer.h move to after dir.h?\n\n1. I like the convention of including the \"foo.h\" as the last header\nin \"foo.c\". I suppose it has to do with the way I include standard\nheaders in my own code (for pet projects).\n2. I didn't want to include many of these headers in sequencer.h again\n-- it uses a lot of these data types. I've noticed that ordering of\nheader inclusion is important in many parts of Git, so the convention\njust stuck.\n\nThanks.\n\n-- Ram\n"},{"id":"179373","messageId":"CALkWK0kMvHj6ZfMqZwZ91PV88d0wmSRZim6jQ12Mm3xqsVoWDg@mail.gmail.com","threadId":"28854","inReplyTo":"20111105234312.GB27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/5] Sequencer: working around historical mistakes","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-13T10:42:01Z","receivedAt":"2011-11-13T10:42:01Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder writes:\n> Shouldn't it be based against rr/revert-cherry-pick, rather than\n> \"next\" which is more of a moving target?\n\nWhen I read this, I went: \"Now why didn't I think of that?\"\n\nThanks.\n\n-- Ram\n"},{"id":"179374","messageId":"CALkWK0nGhUshwJM1vmAUhBG9foH+=6+_KFhfTTF6+kNS0Hm2JA@mail.gmail.com","threadId":"28854","inReplyTo":"20111106002645.GE27272@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-13T10:44:12Z","receivedAt":"2011-11-13T10:44:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n> [...]\n> The usual commit-message debugging strategy applies here: imagine you\n> are a BIOS clone manufacturer, and for legal reasons you are not\n> allowed to read this part of the git implementation embedded in the\n> standard BIOS.  However, you are allowed to read the commit message,\n> and if that message is clear enough, it will explain the purpose and\n> behavior of that code and you will be able to implement a compatible\n> implementation addressing the same problem without scratching your\n> head too much.\n\nAh, it helps to think about commit messages like this. Thanks.\n\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -654,11 +654,15 @@ static void walk_revs_populate_todo(struct replay_insn_list **todo_list,\n>>\n>>  static int create_seq_dir(void)\n>>  {\n>> +     const char *todo_file = git_path(SEQ_TODO_FILE);\n>>       const char *seq_dir = git_path(SEQ_DIR);\n>\n> Scary idiom.\n\nWhat's scary about it?\n\n-- Ram\n"},{"id":"179393","messageId":"7v7h33oifq.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"CALkWK0nGhUshwJM1vmAUhBG9foH+=6+_KFhfTTF6+kNS0Hm2JA@mail.gmail.com","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-13T20:50:49Z","receivedAt":"2011-11-13T20:50:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n>>>  static int create_seq_dir(void)\n>>>  {\n>>> +     const char *todo_file = git_path(SEQ_TODO_FILE);\n>>>       const char *seq_dir = git_path(SEQ_DIR);\n>>\n>> Scary idiom.\n>\n> What's scary about it?\n\nThe next person who copies and pastes this code to other codepaths without\nthinking that the return value of git_path() is ephemeral and may need to\nbe saved away depending on what goes between its assignment and its use.\n"},{"id":"179398","messageId":"7vvcqnmxeu.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"CALkWK0n7v15n_s3CNq1Qu3LHjYkV-ENAkv2b+oB+VBkyV+Sphw@mail.gmail.com","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-13T23:10:17Z","receivedAt":"2011-11-13T23:10:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n>> Why did sequencer.h move to after dir.h?\n>\n> 1. I like the convention of including the \"foo.h\" as the last header\n> in \"foo.c\".\n\nI do not think it is a good convention. The implementation of \"foo.c\" may\nneed to include many other headers for its own use of other APIs, and the\ndeclarations in \"foo.h\" may depend on some types declared in some of them,\nbut by definition the latter is a subset of the former. Having \"foo.h\" at\nthe end of \"foo.c\" makes it difficult for others to tell between the two.\n\nA user of foo.h API should need to include only git-compat-util.h and\nfoo.h to be able to use foo.h API in the ideal world, even though it may\nneed to include other headers to use other APIs defined in them.\n\nA workable alternative in a world that is not so perfect for a user of\n\"foo.h\" API is to include git-compat-util.h and what \"foo.h\" needs before\nincluding \"foo.h\" and then other headers it needs. I think the current\nsource code takes this approach.\n\nWith that observation, it would probably make more sense if \"foo.c\"\nincluded the headers in the following order:\n\n - git-compat-util.h (or the prominent ones like \"cache.h\" that is known\n to include it at the beginning);\n - Anything the declarations in \"foo.h\" depends on;\n - \"foo.h\" itself; and finally\n - Other headers that \"foo.c\" implementation needs.\n\nThat way, people who want to use \"foo.h\" can guess what needs to be\nincluded before using \"foo.h\" a lot more easily.\n"},{"id":"179488","messageId":"CALkWK0mtmRYyFosQNJixhheUmHpRjWc4A5zPQ6AaBfmw4H4eLQ@mail.gmail.com","threadId":"28854","inReplyTo":"7vvcqnmxeu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-15T09:00:56Z","receivedAt":"2011-11-15T09:00:56Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano writes:\n> [...]\n> With that observation, it would probably make more sense if \"foo.c\"\n> included the headers in the following order:\n>\n>  - git-compat-util.h (or the prominent ones like \"cache.h\" that is known\n>   to include it at the beginning);\n>  - Anything the declarations in \"foo.h\" depends on;\n>  - \"foo.h\" itself; and finally\n>  - Other headers that \"foo.c\" implementation needs.\n>\n> That way, people who want to use \"foo.h\" can guess what needs to be\n> included before using \"foo.h\" a lot more easily.\n\nThat's a good rule-of-thumb. Thanks :)\n\n-- Ram\n"},{"id":"179489","messageId":"CALkWK0nUuzn2_itdACHLQBpUaVv97tFAjNGdVBEhWC7a6Rp75w@mail.gmail.com","threadId":"28854","inReplyTo":"7v7h33oifq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-15T09:13:33Z","receivedAt":"2011-11-15T09:13:33Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Ramkumar Ramachandra writes:\n>\n>>>>  static int create_seq_dir(void)\n>>>>  {\n>>>> +     const char *todo_file = git_path(SEQ_TODO_FILE);\n>>>>       const char *seq_dir = git_path(SEQ_DIR);\n>>>\n>>> Scary idiom.\n>>\n>> What's scary about it?\n>\n> The next person who copies and pastes this code to other codepaths without\n> thinking that the return value of git_path() is ephemeral and may need to\n> be saved away depending on what goes between its assignment and its use.\n\nYeah, git_path() writes to one of the four static buffers in\npath.c:get_pathname(). Which brings me to: what should (can) we do\nabout it? Explicitly xmalloc()'ing and free()'ing a tiny path buffer\nis an overkill, so I'm thinking more on the lines of good\ndocumentation. I've been guilty of misusing git_path() blindly in the\npast myself.\n\nThanks.\n\n-- Ram\n"},{"id":"179490","messageId":"buo4ny5u4k6.fsf@dhlpc061.dev.necel.com","threadId":"28854","inReplyTo":"CALkWK0mtmRYyFosQNJixhheUmHpRjWc4A5zPQ6AaBfmw4H4eLQ@mail.gmail.com","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Miles Bader","fromEmail":"miles@gnu.org","sentAt":"2011-11-15T09:18:33Z","receivedAt":"2011-11-15T09:18:33Z","isPatch":true,"sender":{"key":"miles@gnu.org","avatar":"https://gravatar.com/avatar/01069b69593af7bff28e2f97afeb3644ae6fe2f5f56cb3a8cf34c5fb8c36efe5?d=mp&s=160"},"body":"Ramkumar Ramachandra writes:\n> Junio C Hamano writes:\n>> [...]\n>> With that observation, it would probably make more sense if \"foo.c\"\n>> included the headers in the following order:\n>>\n>>  - Anything the declarations in \"foo.h\" depends on;\n>>  - \"foo.h\" itself; and finally\n>>  - Other headers that \"foo.c\" implementation needs.\n>>\n>> That way, people who want to use \"foo.h\" can guess what needs to be\n>> included before using \"foo.h\" a lot more easily.\n>\n> That's a good rule-of-thumb. Thanks :)\n\nDoes git not use the common practice of self-contained headers?\n\n-miles\n\n-- \nFast, small, soon; pick any 2.\n"},{"id":"179492","messageId":"20111115094739.GA23139@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"buo4ny5u4k6.fsf@dhlpc061.dev.necel.com","subject":"Re: [PATCH 1/5] sequencer: factor code out of revert builtin","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-15T09:47:40Z","receivedAt":"2011-11-15T09:47:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Miles Bader wrote:\n\n> Does git not use the common practice of self-contained headers?\n\nIt usually does, with two exceptions.\n\nHeaders do not usually include git-compat-util.h directly, which is a\ngood thing, since it reminds callers to include git-compat-util.h\nbefore anything else.\n\nHeaders might sometimes forget to declare types defined in cache.h,\nwhich would be a mistake. For example, in branch.h we see:\n\n int validate_new_branchname(const char *name, struct strbuf *ref, int force, int attr_only);\n\nWhich means the following code does not type-check:\n\n #include \"git-compat-util.h\"\n #include \"branch.h\"\n #include \"strbuf.h\"\n\n int demo(const char *name, struct strbuf *ref)\n {\n\treturn validate_new_branchname(name, ref, 0, 0);\n }\n\nReordering the #includes to put strbuf.h before branch.h is a possible\nworkaround. Adding the missing forward declaration is better:\n\ndiff --git i/branch.h w/branch.h\nindex 1285158d..d5240a20 100644\n--- i/branch.h\n+++ w/branch.h\n@@ -1,6 +1,9 @@\n #ifndef BRANCH_H\n #define BRANCH_H\n \n+struct strbuf;\n+enum branch_track;\n+\n /* Functions for acting on the information about branches. */\n \n /*\n-- \n"},{"id":"179493","messageId":"20111115095225.GB23139@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"CALkWK0nUuzn2_itdACHLQBpUaVv97tFAjNGdVBEhWC7a6Rp75w@mail.gmail.com","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-15T09:52:25Z","receivedAt":"2011-11-15T09:52:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Yeah, git_path() writes to one of the four static buffers in\n> path.c:get_pathname(). Which brings me to: what should (can) we do\n> about it?\n\nJust use a sane idiom. Which means: as few git_path() values in\nflight at a time as possible.\n\nIn other words, do not save the git_path() result in a variable, but\npass it directly to whatever computation needs to use it.\n"},{"id":"179509","messageId":"7v7h31wduv.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"20111115095225.GB23139@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-15T16:27:04Z","receivedAt":"2011-11-15T16:27:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Ramkumar Ramachandra wrote:\n>\n>> Yeah, git_path() writes to one of the four static buffers in\n>> path.c:get_pathname(). Which brings me to: what should (can) we do\n>> about it?\n>\n> Just use a sane idiom. Which means: as few git_path() values in\n> flight at a time as possible.\n>\n> In other words, do not save the git_path() result in a variable, but\n> pass it directly to whatever computation needs to use it.\n\nOr perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n"},{"id":"179552","messageId":"CALkWK0kOrGzjcGNcf2qPahJSgkvCsQwSrEfAA3wj6PqnMzDBVQ@mail.gmail.com","threadId":"28854","inReplyTo":"7v7h31wduv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-16T06:17:43Z","receivedAt":"2011-11-16T06:17:43Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nJonathan Nieder wrote:\n> Just use a sane idiom.  Which means: as few git_path() values in\n> flight at a time as possible.\n\nMakes sense, thanks.\n\nJunio C Hamano wrote:\n> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n\nI noticed that sha1_to_hex() also operates like this. The\nresolve_ref() function is really important, but using the same\ntechnique for these tiny functions is probably an overkill; something\nin `Documentation/technical` perhaps?\n\nThanks.\n\n-- Ram\n"},{"id":"179562","messageId":"7vhb24qzxy.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"CALkWK0kOrGzjcGNcf2qPahJSgkvCsQwSrEfAA3wj6PqnMzDBVQ@mail.gmail.com","subject":"Re: [PATCH 3/5] sequencer: sequencer state is useless without todo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-16T07:38:49Z","receivedAt":"2011-11-16T07:38:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> I noticed that sha1_to_hex() also operates like this.\n\nA function to externalize our internal representation like sha1_to_hex()\nis not such a big problem in practice, as the lifetime of its result is\ninherently much shorter.\n\nAnybody sane with a datum that eventually needs to be externalized will\nkeep it in its internal representation as long as possible, and then call\nsuch an internal-to-external function just before it becomes absolutely\nnecessary to externalize it (e.g. calling printf(), packet_write(), etc).\nThis is because the whole point of having an internal representation\n(e.g. when our code talks about an object name, we always use \"unsigned\nchar[20]\") is so that all of our functions can use that representation to\npass it around. It would be insane to call such a function earlier than\nnecessary, having to pass external representation around.\n\nOn the other hand, resolve_ref() is an interface to canonicalize external\nrepresentation into a form suitable to be kept and passed around as its\ninternal representation. The lifetime of its result fundamentally has to\nbe a lot longer than that of functions that work in the opposite\ndirection, e.g. sha1_to_hex().\n"},{"id":"179564","messageId":"20111116075955.GB13706@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"CALkWK0kOrGzjcGNcf2qPahJSgkvCsQwSrEfAA3wj6PqnMzDBVQ@mail.gmail.com","subject":"[PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T07:59:55Z","receivedAt":"2011-11-16T07:59:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Junio C Hamano wrote:\n\n>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n>\n> I noticed that sha1_to_hex() also operates like this. The\n> resolve_ref() function is really important, but using the same\n> technique for these tiny functions is probably an overkill\n\nI don't follow. Do you mean that not being confusing is overkill,\nbecause the function is small that no one will bother to look up the\nright semantics? Wait, that sentence didn't come out the way I\nwanted. ;-)\n\nJokes aside, here's a rough series to do the git_path ->\ngit_path_unsafe renaming. While writing it, I noticed a couple of\nbugs, hence the two patches before the last one. Patch 2 is the more\ninteresting one.\n\nPatches are against \"master\", but patch 2 probably should be thought\nof as being against maint-1.7.6. Improvements welcome, as always.\n\nThanks,\n\nJonathan Nieder (3):\n do not let git_path clobber errno when reporting errors\n Bigfile: dynamically allocate buffer for marks file name\n rename git_path() to git_path_unsafe()\n\n Documentation/technical/api-string-list.txt | 5 +-\n attr.c | 2 +-\n bisect.c | 8 ++--\n branch.c | 12 ++--\n builtin/add.c | 2 +-\n builtin/commit.c | 57 ++++++++++++-----------\n builtin/config.c | 4 +-\n builtin/fetch-pack.c | 4 +-\n builtin/fetch.c | 5 +-\n builtin/fsck.c | 2 +-\n builtin/init-db.c | 12 ++--\n builtin/merge.c | 67 +++++++++++++++------------\n builtin/notes.c | 2 +-\n builtin/remote.c | 6 +-\n builtin/reset.c | 2 +-\n builtin/revert.c | 25 +++++-----\n cache.h | 3 +-\n contrib/examples/builtin-fetch--tool.c | 4 +-\n dir.c | 2 +-\n fast-import.c | 2 +-\n http-backend.c | 2 +-\n notes-merge.c | 22 +++++----\n pack-refs.c | 6 +-\n path.c | 2 +-\n refs.c | 51 +++++++++++---------\n remote.c | 4 +-\n rerere.c | 12 ++--\n run-command.c | 4 +-\n sequencer.c | 6 +-\n server-info.c | 2 +-\n sha1_file.c | 22 ++++++--\n shallow.c | 2 +-\n transport.c | 4 +-\n unpack-trees.c | 2 +-\n 34 files changed, 200 insertions(+), 167 deletions(-)\n\n-- \n1.7.8.rc0\n"},{"id":"179565","messageId":"20111116080336.GC13706@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"[PATCH 1/3] do not let git_path clobber errno when reporting errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T08:03:36Z","receivedAt":"2011-11-16T08:03:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Because git_path() calls vsnprintf(), code like\n\n\tfd = open(git_path(\"SQUASH_MSG\"), O_WRONLY | O_CREAT, 0666);\n\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"SQUASH_MSG\"));\n\ncan end up printing an error indicator from vsnprintf() instead of\nopen() by mistake. Store the path we are trying to write to in a\ntemporary variable and pass _that_ to die_errno(), so the messages\nwritten by git cherry-pick/revert and git merge can avoid this source\nof confusion.\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/merge.c | 49 +++++++++++++++++++++++++++++--------------------\n builtin/revert.c | 9 +++++----\n 2 files changed, 34 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex dffd5ec1..2870a6af 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -316,13 +316,15 @@ static void squash_message(struct commit *commit)\n \tstruct rev_info rev;\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct commit_list *j;\n+\tconst char *filename;\n \tint fd;\n \tstruct pretty_print_context ctx = {0};\n \n \tprintf(_(\"Squash commit -- not updating HEAD\\n\"));\n-\tfd = open(git_path(\"SQUASH_MSG\"), O_WRONLY | O_CREAT, 0666);\n+\tfilename = git_path(\"SQUASH_MSG\");\n+\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"SQUASH_MSG\"));\n+\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \n \tinit_revisions(&rev, NULL);\n \trev.ignore_merges = 1;\n@@ -492,14 +494,16 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \n \tif (!strcmp(remote, \"FETCH_HEAD\") &&\n \t\t\t!access(git_path(\"FETCH_HEAD\"), R_OK)) {\n+\t\tconst char *filename;\n \t\tFILE *fp;\n \t\tstruct strbuf line = STRBUF_INIT;\n \t\tchar *ptr;\n \n-\t\tfp = fopen(git_path(\"FETCH_HEAD\"), \"r\");\n+\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfp = fopen(filename, \"r\");\n \t\tif (!fp)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n-\t\t\t\t git_path(\"FETCH_HEAD\"));\n+\t\t\t\t filename);\n \t\tstrbuf_getline(&line, fp, '\\n');\n \t\tfclose(fp);\n \t\tptr = strstr(line.buf, \"\\tnot-for-merge\\t\");\n@@ -847,20 +851,22 @@ static void add_strategies(const char *string, unsigned attr)\n \n static void write_merge_msg(struct strbuf *msg)\n {\n-\tint fd = open(git_path(\"MERGE_MSG\"), O_WRONLY | O_CREAT, 0666);\n+\tconst char *filename = git_path(\"MERGE_MSG\");\n+\tint fd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"MERGE_MSG\"));\n+\t\t\t filename);\n \tif (write_in_full(fd, msg->buf, msg->len) != msg->len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MSG\"));\n+\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \tclose(fd);\n }\n \n static void read_merge_msg(struct strbuf *msg)\n {\n+\tconst char *filename = git_path(\"MERGE_MSG\");\n \tstrbuf_reset(msg);\n-\tif (strbuf_read_file(msg, git_path(\"MERGE_MSG\"), 0) < 0)\n-\t\tdie_errno(_(\"Could not read from '%s'\"), git_path(\"MERGE_MSG\"));\n+\tif (strbuf_read_file(msg, filename, 0) < 0)\n+\t\tdie_errno(_(\"Could not read from '%s'\"), filename);\n }\n \n static void write_merge_state(void);\n@@ -948,13 +954,14 @@ static int finish_automerge(struct commit *head,\n \n static int suggest_conflicts(int renormalizing)\n {\n+\tconst char *filename;\n \tFILE *fp;\n \tint pos;\n \n-\tfp = fopen(git_path(\"MERGE_MSG\"), \"a\");\n+\tfilename = git_path(\"MERGE_MSG\");\n+\tfp = fopen(filename, \"a\");\n \tif (!fp)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"MERGE_MSG\"));\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tfprintf(fp, \"\\nConflicts:\\n\");\n \tfor (pos = 0; pos < active_nr; pos++) {\n \t\tstruct cache_entry *ce = active_cache[pos];\n@@ -1046,6 +1053,7 @@ static int setup_with_upstream(const char ***argv)\n \n static void write_merge_state(void)\n {\n+\tconst char *filename;\n \tint fd;\n \tstruct commit_list *j;\n \tstruct strbuf buf = STRBUF_INIT;\n@@ -1053,24 +1061,25 @@ static void write_merge_state(void)\n \tfor (j = remoteheads; j; j = j->next)\n \t\tstrbuf_addf(&buf, \"%s\\n\",\n \t\t\tsha1_to_hex(j->item->object.sha1));\n-\tfd = open(git_path(\"MERGE_HEAD\"), O_WRONLY | O_CREAT, 0666);\n+\tfilename = git_path(\"MERGE_HEAD\");\n+\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"MERGE_HEAD\"));\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_HEAD\"));\n+\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \tclose(fd);\n \tstrbuf_addch(&merge_msg, '\\n');\n \twrite_merge_msg(&merge_msg);\n-\tfd = open(git_path(\"MERGE_MODE\"), O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\n+\tfilename = git_path(\"MERGE_MODE\");\n+\tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"MERGE_MODE\"));\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tstrbuf_reset(&buf);\n \tif (!allow_fast_forward)\n \t\tstrbuf_addf(&buf, \"no-ff\");\n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"MERGE_MODE\"));\n+\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \tclose(fd);\n }\n \ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 87df70ed..985f95b0 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -288,17 +288,18 @@ static char *get_encoding(const char *message)\n \n static void write_cherry_pick_head(struct commit *commit)\n {\n+\tconst char *filename;\n \tint fd;\n \tstruct strbuf buf = STRBUF_INIT;\n \n \tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n \n-\tfd = open(git_path(\"CHERRY_PICK_HEAD\"), O_WRONLY | O_CREAT, 0666);\n+\tfilename = git_path(\"CHERRY_PICK_HEAD\");\n+\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t git_path(\"CHERRY_PICK_HEAD\"));\n+\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tif (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))\n-\t\tdie_errno(_(\"Could not write to '%s'\"), git_path(\"CHERRY_PICK_HEAD\"));\n+\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \tstrbuf_release(&buf);\n }\n \n-- \n1.7.8.rc0\n"},{"id":"179566","messageId":"20111116080420.GD13706@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"[PATCH 2/3] Bigfile: dynamically allocate buffer for marks file name","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T08:04:20Z","receivedAt":"2011-11-16T08:04:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This prevents a buffer overrun that could otherwise be triggered by\ncreating a .git file with a long destination path and trying to \"git\nadd\" a file larger than the big-file threshold (which defaults to 512\nMiB), ever since v1.7.6-rc0~31^2 (Bigfile: teach \"git add\" to send a\nlarge file straight to a pack, 2011-05-08).\n\nSigned-off-by: Jonathan Nieder \n---\n sha1_file.c | 20 +++++++++++++++-----\n 1 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 27f3b9b2..86705bc9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2697,20 +2697,28 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \t\t\tunsigned flags)\n {\n \tstruct child_process fast_import;\n-\tchar export_marks[512];\n-\tconst char *argv[] = { \"fast-import\", \"--quiet\", export_marks, NULL };\n-\tchar tmpfile[512];\n+\tconst char *argv[4];\t/* command, two args, NULL */\n+\tconst char **arg;\n+\tstruct strbuf export_marks = STRBUF_INIT;\n+\tchar *tmpfile;\n \tchar fast_import_cmd[512];\n \tchar buf[512];\n \tint len, tmpfd;\n \n-\tstrcpy(tmpfile, git_path(\"hashstream_XXXXXX\"));\n+\tstrbuf_addstr(&export_marks, \"--export-marks=\");\n+\tstrbuf_addstr(&export_marks, git_path(\"hashstream_XXXXXX\"));\n+\ttmpfile = export_marks.buf + strlen(\"--export-marks=\");\n \ttmpfd = git_mkstemp_mode(tmpfile, 0600);\n \tif (tmpfd < 0)\n \t\tdie_errno(\"cannot create tempfile: %s\", tmpfile);\n \tif (close(tmpfd))\n \t\tdie_errno(\"cannot close tempfile: %s\", tmpfile);\n-\tsprintf(export_marks, \"--export-marks=%s\", tmpfile);\n+\n+\targ = argv;\n+\t*arg++ = \"fast-import\";\n+\t*arg++ = \"--quiet\";\n+\t*arg++ = export_marks.buf;\n+\t*arg++ = NULL;\n \n \tmemset(&fast_import, 0, sizeof(fast_import));\n \tfast_import.in = -1;\n@@ -2754,6 +2762,8 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \t memcmp(\":1 \", buf, 3) ||\n \t get_sha1_hex(buf + 3, sha1))\n \t\tdie_errno(\"index-stream: unexpected fast-import mark: <%s>\", buf);\n+\n+\tstrbuf_release(&export_marks);\n \treturn 0;\n }\n \n-- \n1.7.8.rc0\n"},{"id":"179567","messageId":"20111116080716.GE13706@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"[PATCH 3/3] rename git_path() to git_path_unsafe()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T08:07:16Z","receivedAt":"2011-11-16T08:07:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"git_path() stores its result to one of a rotating collection of four\nstatic buffers. If more than 4 git_path() results are in play, the\nresult can be a little unpleasant, as each call clobbers the return\nvalue from previous calls.\n\nTherefore callers should be careful not to assign the return value\nfrom git_path() to a long-lived variable. Rename the function to\ngit_path_unsafe() as a reminder.\n\nMechanics: This patch only makes three kinds of changes:\n\n 1) changing git_path(foo) to git_path_unsafe(foo)\n 2) changing xstrdup(git_path(foo)) to git_pathdup(foo)\n 3) rewrapping lines that were made longer by (1)\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jonathan Nieder \n---\n Documentation/technical/api-string-list.txt | 5 +-\n attr.c | 2 +-\n bisect.c | 8 ++--\n branch.c | 12 +++---\n builtin/add.c | 2 +-\n builtin/commit.c | 57 ++++++++++++++-------------\n builtin/config.c | 4 +-\n builtin/fetch-pack.c | 4 +-\n builtin/fetch.c | 5 +-\n builtin/fsck.c | 2 +-\n builtin/init-db.c | 12 +++---\n builtin/merge.c | 32 +++++++-------\n builtin/notes.c | 2 +-\n builtin/remote.c | 6 +-\n builtin/reset.c | 2 +-\n builtin/revert.c | 18 ++++----\n cache.h | 3 +-\n contrib/examples/builtin-fetch--tool.c | 4 +-\n dir.c | 2 +-\n fast-import.c | 2 +-\n http-backend.c | 2 +-\n notes-merge.c | 22 ++++++-----\n pack-refs.c | 6 +-\n path.c | 2 +-\n refs.c | 51 +++++++++++++-----------\n remote.c | 4 +-\n rerere.c | 12 +++---\n run-command.c | 4 +-\n sequencer.c | 6 +-\n server-info.c | 2 +-\n sha1_file.c | 4 +-\n shallow.c | 2 +-\n transport.c | 4 +-\n unpack-trees.c | 2 +-\n 34 files changed, 160 insertions(+), 147 deletions(-)\n\ndiff --git a/Documentation/technical/api-string-list.txt b/Documentation/technical/api-string-list.txt\nindex ce24eb96..446a51ab 100644\n--- a/Documentation/technical/api-string-list.txt\n+++ b/Documentation/technical/api-string-list.txt\n@@ -13,8 +13,9 @@ The caller:\n \n . Initializes the members. You might want to set the flag `strdup_strings`\n if the strings should be strdup()ed. For example, this is necessary\n- when you add something like git_path(\"...\"), since that function returns\n- a static buffer that will change with the next call to git_path().\n+ when you add something like git_path_unsafe(\"...\"), since that function\n+ returns a static buffer that will change with the next call to\n+ git_path_unsafe().\n +\n If you need something advanced, you can manually malloc() the `items`\n member (you need this if you add things later) and you should set the\ndiff --git a/attr.c b/attr.c\nindex 76b079f0..afdd6d24 100644\n--- a/attr.c\n+++ b/attr.c\n@@ -529,7 +529,7 @@ static void bootstrap_attr_stack(void)\n \t\t\tdebug_push(elem);\n \t\t}\n \n-\t\telem = read_attr_from_file(git_path(INFOATTRIBUTES_FILE), 1);\n+\t\telem = read_attr_from_file(git_path_unsafe(INFOATTRIBUTES_FILE), 1);\n \t\tif (!elem)\n \t\t\telem = xcalloc(1, sizeof(*elem));\n \t\telem->origin = NULL;\ndiff --git a/bisect.c b/bisect.c\nindex 6e186e29..315d22e6 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -422,7 +422,7 @@ static int read_bisect_refs(void)\n static void read_bisect_paths(struct argv_array *array)\n {\n \tstruct strbuf str = STRBUF_INIT;\n-\tconst char *filename = git_path(\"BISECT_NAMES\");\n+\tconst char *filename = git_path_unsafe(\"BISECT_NAMES\");\n \tFILE *fp = fopen(filename, \"r\");\n \n \tif (!fp)\n@@ -643,7 +643,7 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \n static int is_expected_rev(const unsigned char *sha1)\n {\n-\tconst char *filename = git_path(\"BISECT_EXPECTED_REV\");\n+\tconst char *filename = git_path_unsafe(\"BISECT_EXPECTED_REV\");\n \tstruct stat st;\n \tstruct strbuf str = STRBUF_INIT;\n \tFILE *fp;\n@@ -668,7 +668,7 @@ static int is_expected_rev(const unsigned char *sha1)\n static void mark_expected_rev(char *bisect_rev_hex)\n {\n \tint len = strlen(bisect_rev_hex);\n-\tconst char *filename = git_path(\"BISECT_EXPECTED_REV\");\n+\tconst char *filename = git_path_unsafe(\"BISECT_EXPECTED_REV\");\n \tint fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);\n \n \tif (fd < 0)\n@@ -833,7 +833,7 @@ static int check_ancestors(const char *prefix)\n */\n static void check_good_are_ancestors_of_bad(const char *prefix, int no_checkout)\n {\n-\tconst char *filename = git_path(\"BISECT_ANCESTORS_OK\");\n+\tconst char *filename = git_path_unsafe(\"BISECT_ANCESTORS_OK\");\n \tstruct stat st;\n \tint fd;\n \ndiff --git a/branch.c b/branch.c\nindex d8098762..5a3faa10 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -240,11 +240,11 @@ void create_branch(const char *head,\n \n void remove_branch_state(void)\n {\n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_RR\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n-\tunlink(git_path(\"SQUASH_MSG\"));\n+\tunlink(git_path_unsafe(\"CHERRY_PICK_HEAD\"));\n+\tunlink(git_path_unsafe(\"MERGE_HEAD\"));\n+\tunlink(git_path_unsafe(\"MERGE_RR\"));\n+\tunlink(git_path_unsafe(\"MERGE_MSG\"));\n+\tunlink(git_path_unsafe(\"MERGE_MODE\"));\n+\tunlink(git_path_unsafe(\"SQUASH_MSG\"));\n \tremove_sequencer_state(0);\n }\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c59b0c98..0aabb61d 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -259,7 +259,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)\n \n static int edit_patch(int argc, const char **argv, const char *prefix)\n {\n-\tchar *file = xstrdup(git_path(\"ADD_EDIT.patch\"));\n+\tchar *file = git_pathdup(\"ADD_EDIT.patch\");\n \tconst char *apply_argv[] = { \"apply\", \"--recount\", \"--cached\",\n \t\tNULL, NULL };\n \tstruct child_process child;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c46f2d18..e9aa5e75 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -178,9 +178,9 @@ static struct option builtin_commit_options[] = {\n \n static void determine_whence(struct wt_status *s)\n {\n-\tif (file_exists(git_path(\"MERGE_HEAD\")))\n+\tif (file_exists(git_path_unsafe(\"MERGE_HEAD\")))\n \t\twhence = FROM_MERGE;\n-\telse if (file_exists(git_path(\"CHERRY_PICK_HEAD\")))\n+\telse if (file_exists(git_path_unsafe(\"CHERRY_PICK_HEAD\")))\n \t\twhence = FROM_CHERRY_PICK;\n \telse\n \t\twhence = FROM_COMMIT;\n@@ -465,8 +465,8 @@ static char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\tdie(_(\"unable to write new_index file\"));\n \n \tfd = hold_lock_file_for_update(&false_lock,\n-\t\t\t\t git_path(\"next-index-%\"PRIuMAX,\n-\t\t\t\t\t\t(uintmax_t) getpid()),\n+\t\t\t\t git_path_unsafe(\"next-index-%\"PRIuMAX,\n+\t\t\t\t\t\t\t(uintmax_t) getpid()),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \n \tcreate_base_index(current_head);\n@@ -691,12 +691,12 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tformat_commit_message(commit, \"fixup! %s\\n\\n\",\n \t\t\t\t &sb, &ctx);\n \t\thook_arg1 = \"message\";\n-\t} else if (!stat(git_path(\"MERGE_MSG\"), &statbuf)) {\n-\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MSG\"), 0) < 0)\n+\t} else if (!stat(git_path_unsafe(\"MERGE_MSG\"), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path_unsafe(\"MERGE_MSG\"), 0) < 0)\n \t\t\tdie_errno(_(\"could not read MERGE_MSG\"));\n \t\thook_arg1 = \"merge\";\n-\t} else if (!stat(git_path(\"SQUASH_MSG\"), &statbuf)) {\n-\t\tif (strbuf_read_file(&sb, git_path(\"SQUASH_MSG\"), 0) < 0)\n+\t} else if (!stat(git_path_unsafe(\"SQUASH_MSG\"), &statbuf)) {\n+\t\tif (strbuf_read_file(&sb, git_path_unsafe(\"SQUASH_MSG\"), 0) < 0)\n \t\t\tdie_errno(_(\"could not read SQUASH_MSG\"));\n \t\thook_arg1 = \"squash\";\n \t} else if (template_file) {\n@@ -727,9 +727,10 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\thook_arg2 = \"\";\n \t}\n \n-\ts->fp = fopen(git_path(commit_editmsg), \"w\");\n+\ts->fp = fopen(git_path_unsafe(commit_editmsg), \"w\");\n \tif (s->fp == NULL)\n-\t\tdie_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n+\t\tdie_errno(_(\"could not open '%s'\"),\n+\t\t\t git_path_unsafe(commit_editmsg));\n \n \tif (clean_message_contents)\n \t\tstripspace(&sb, 0);\n@@ -773,7 +774,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\t\"and try again.\\n\"\n \t\t\t\t\"\"),\n \t\t\t\twhence_s(),\n-\t\t\t\tgit_path(whence == FROM_MERGE\n+\t\t\t\tgit_path_unsafe(whence == FROM_MERGE\n \t\t\t\t\t ? \"MERGE_HEAD\"\n \t\t\t\t\t : \"CHERRY_PICK_HEAD\"));\n \n@@ -870,8 +871,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\treturn 0;\n \t}\n \n-\tif (run_hook(index_file, \"prepare-commit-msg\",\n-\t\t git_path(commit_editmsg), hook_arg1, hook_arg2, NULL))\n+\tif (run_hook(index_file,\n+\t\t \"prepare-commit-msg\", git_path_unsafe(commit_editmsg),\n+\t\t hook_arg1, hook_arg2, NULL))\n \t\treturn 0;\n \n \tif (use_editor) {\n@@ -879,7 +881,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\tconst char *env[2] = { NULL };\n \t\tenv[0] = index;\n \t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n-\t\tif (launch_editor(git_path(commit_editmsg), NULL, env)) {\n+\t\tif (launch_editor(git_path_unsafe(commit_editmsg), NULL, env)) {\n \t\t\tfprintf(stderr,\n \t\t\t_(\"Please supply the message using either -m or -F option.\\n\"));\n \t\t\texit(1);\n@@ -887,7 +889,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t}\n \n \tif (!no_verify &&\n-\t run_hook(index_file, \"commit-msg\", git_path(commit_editmsg), NULL)) {\n+\t run_hook(index_file,\n+\t\t \"commit-msg\", git_path_unsafe(commit_editmsg), NULL)) {\n \t\treturn 0;\n \t}\n \n@@ -1347,10 +1350,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,\n \tint code;\n \tsize_t n;\n \n-\tif (access(git_path(post_rewrite_hook), X_OK) < 0)\n+\tif (access(git_path_unsafe(post_rewrite_hook), X_OK) < 0)\n \t\treturn 0;\n \n-\targv[0] = git_path(post_rewrite_hook);\n+\targv[0] = git_path_unsafe(post_rewrite_hook);\n \targv[1] = \"amend\";\n \targv[2] = NULL;\n \n@@ -1431,10 +1434,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = \"commit (merge)\";\n \t\tpptr = &commit_list_insert(current_head, pptr)->next;\n-\t\tfp = fopen(git_path(\"MERGE_HEAD\"), \"r\");\n+\t\tfp = fopen(git_path_unsafe(\"MERGE_HEAD\"), \"r\");\n \t\tif (fp == NULL)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n-\t\t\t\t git_path(\"MERGE_HEAD\"));\n+\t\t\t\t git_path_unsafe(\"MERGE_HEAD\"));\n \t\twhile (strbuf_getline(&m, fp, '\\n') != EOF) {\n \t\t\tunsigned char sha1[20];\n \t\t\tif (get_sha1_hex(m.buf, sha1) < 0)\n@@ -1444,8 +1447,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tfclose(fp);\n \t\tstrbuf_release(&m);\n-\t\tif (!stat(git_path(\"MERGE_MODE\"), &statbuf)) {\n-\t\t\tif (strbuf_read_file(&sb, git_path(\"MERGE_MODE\"), 0) < 0)\n+\t\tif (!stat(git_path_unsafe(\"MERGE_MODE\"), &statbuf)) {\n+\t\t\tif (strbuf_read_file(&sb, git_path_unsafe(\"MERGE_MODE\"), 0) < 0)\n \t\t\t\tdie_errno(_(\"could not read MERGE_MODE\"));\n \t\t\tif (!strcmp(sb.buf, \"no-ff\"))\n \t\t\t\tallow_fast_forward = 0;\n@@ -1462,7 +1465,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \n \t/* Finally, get the commit message */\n \tstrbuf_reset(&sb);\n-\tif (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0) {\n+\tif (strbuf_read_file(&sb, git_path_unsafe(commit_editmsg), 0) < 0) {\n \t\tint saved_errno = errno;\n \t\trollback_index_files();\n \t\tdie(_(\"could not read commit message: %s\"), strerror(saved_errno));\n@@ -1513,11 +1516,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"cannot update HEAD ref\"));\n \t}\n \n-\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n-\tunlink(git_path(\"SQUASH_MSG\"));\n+\tunlink(git_path_unsafe(\"CHERRY_PICK_HEAD\"));\n+\tunlink(git_path_unsafe(\"MERGE_HEAD\"));\n+\tunlink(git_path_unsafe(\"MERGE_MSG\"));\n+\tunlink(git_path_unsafe(\"MERGE_MODE\"));\n+\tunlink(git_path_unsafe(\"SQUASH_MSG\"));\n \n \tif (commit_index_files())\n \t\tdie (_(\"Repository has been updated, but unable to write\\n\"\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 0315ad76..407d7ca8 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -434,8 +434,8 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\tdie(\"not in a git directory\");\n \t\tgit_config(git_default_config, NULL);\n \t\tlaunch_editor(config_exclusive_filename ?\n-\t\t\t config_exclusive_filename : git_path(\"config\"),\n-\t\t\t NULL, NULL);\n+\t\t\t config_exclusive_filename :\n+\t\t\t git_path_unsafe(\"config\"), NULL, NULL);\n \t}\n \telse if (actions == ACTION_SET) {\n \t\tint ret;\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c6bc8eb0..f8d0954c 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1026,7 +1026,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \tif (&args != my_args)\n \t\tmemcpy(&args, my_args, sizeof(args));\n \tif (args.depth > 0) {\n-\t\tif (stat(git_path(\"shallow\"), &st))\n+\t\tif (stat(git_path_unsafe(\"shallow\"), &st))\n \t\t\tst.st_mtime = 0;\n \t}\n \n@@ -1041,7 +1041,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\n \t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tchar *shallow = git_path(\"shallow\");\n+\t\tchar *shallow = git_path_unsafe(\"shallow\");\n \t\tint fd;\n \n \t\tmtime.sec = st.st_mtime;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 91731b90..ccbe63c7 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -367,7 +367,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tchar note[1024];\n \tconst char *what, *kind;\n \tstruct ref *rm;\n-\tchar *url, *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n+\tchar *url;\n+\tchar *filename = dry_run ? \"/dev/null\" : git_path_unsafe(\"FETCH_HEAD\");\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -647,7 +648,7 @@ static void check_not_current_branch(struct ref *ref_map)\n \n static int truncate_fetch_head(void)\n {\n-\tchar *filename = git_path(\"FETCH_HEAD\");\n+\tchar *filename = git_path_unsafe(\"FETCH_HEAD\");\n \tFILE *fp = fopen(filename, \"w\");\n \n \tif (!fp)\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex df1a88b5..f8429f55 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -215,7 +215,7 @@ static void check_unreachable_object(struct object *obj)\n \t\tprintf(\"dangling %s %s\\n\", typename(obj->type),\n \t\t sha1_to_hex(obj->sha1));\n \t\tif (write_lost_and_found) {\n-\t\t\tchar *filename = git_path(\"lost-found/%s/%s\",\n+\t\t\tchar *filename = git_path_unsafe(\"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n \t\t\t\tsha1_to_hex(obj->sha1));\n \t\t\tFILE *f;\ndiff --git a/builtin/init-db.c b/builtin/init-db.c\nindex d07554c8..7ae89f85 100644\n--- a/builtin/init-db.c\n+++ b/builtin/init-db.c\n@@ -198,9 +198,9 @@ static int create_default_files(const char *template_path)\n \t/*\n \t * Create .git/refs/{heads,tags}\n \t */\n-\tsafe_create_dir(git_path(\"refs\"), 1);\n-\tsafe_create_dir(git_path(\"refs/heads\"), 1);\n-\tsafe_create_dir(git_path(\"refs/tags\"), 1);\n+\tsafe_create_dir(git_path_unsafe(\"refs\"), 1);\n+\tsafe_create_dir(git_path_unsafe(\"refs/heads\"), 1);\n+\tsafe_create_dir(git_path_unsafe(\"refs/tags\"), 1);\n \n \t/* Just look for `init.templatedir` */\n \tgit_config(git_init_db_config, NULL);\n@@ -224,9 +224,9 @@ static int create_default_files(const char *template_path)\n \t */\n \tif (shared_repository) {\n \t\tadjust_shared_perm(get_git_dir());\n-\t\tadjust_shared_perm(git_path(\"refs\"));\n-\t\tadjust_shared_perm(git_path(\"refs/heads\"));\n-\t\tadjust_shared_perm(git_path(\"refs/tags\"));\n+\t\tadjust_shared_perm(git_path_unsafe(\"refs\"));\n+\t\tadjust_shared_perm(git_path_unsafe(\"refs/heads\"));\n+\t\tadjust_shared_perm(git_path_unsafe(\"refs/tags\"));\n \t}\n \n \t/*\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 2870a6af..3e75c30b 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -213,9 +213,9 @@ static struct option builtin_merge_options[] = {\n /* Cleans up metadata that is uninteresting after a succeeded merge. */\n static void drop_save(void)\n {\n-\tunlink(git_path(\"MERGE_HEAD\"));\n-\tunlink(git_path(\"MERGE_MSG\"));\n-\tunlink(git_path(\"MERGE_MODE\"));\n+\tunlink(git_path_unsafe(\"MERGE_HEAD\"));\n+\tunlink(git_path_unsafe(\"MERGE_MSG\"));\n+\tunlink(git_path_unsafe(\"MERGE_MODE\"));\n }\n \n static int save_state(unsigned char *stash)\n@@ -321,7 +321,7 @@ static void squash_message(struct commit *commit)\n \tstruct pretty_print_context ctx = {0};\n \n \tprintf(_(\"Squash commit -- not updating HEAD\\n\"));\n-\tfilename = git_path(\"SQUASH_MSG\");\n+\tfilename = git_path_unsafe(\"SQUASH_MSG\");\n \tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n@@ -493,13 +493,13 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t}\n \n \tif (!strcmp(remote, \"FETCH_HEAD\") &&\n-\t\t\t!access(git_path(\"FETCH_HEAD\"), R_OK)) {\n+\t\t\t!access(git_path_unsafe(\"FETCH_HEAD\"), R_OK)) {\n \t\tconst char *filename;\n \t\tFILE *fp;\n \t\tstruct strbuf line = STRBUF_INIT;\n \t\tchar *ptr;\n \n-\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfilename = git_path_unsafe(\"FETCH_HEAD\");\n \t\tfp = fopen(filename, \"r\");\n \t\tif (!fp)\n \t\t\tdie_errno(_(\"could not open '%s' for reading\"),\n@@ -851,7 +851,7 @@ static void add_strategies(const char *string, unsigned attr)\n \n static void write_merge_msg(struct strbuf *msg)\n {\n-\tconst char *filename = git_path(\"MERGE_MSG\");\n+\tconst char *filename = git_path_unsafe(\"MERGE_MSG\");\n \tint fd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"),\n@@ -863,7 +863,7 @@ static void write_merge_msg(struct strbuf *msg)\n \n static void read_merge_msg(struct strbuf *msg)\n {\n-\tconst char *filename = git_path(\"MERGE_MSG\");\n+\tconst char *filename = git_path_unsafe(\"MERGE_MSG\");\n \tstrbuf_reset(msg);\n \tif (strbuf_read_file(msg, filename, 0) < 0)\n \t\tdie_errno(_(\"Could not read from '%s'\"), filename);\n@@ -887,9 +887,9 @@ static void prepare_to_commit(void)\n \tstrbuf_addch(&msg, '\\n');\n \twrite_merge_msg(&msg);\n \trun_hook(get_index_file(), \"prepare-commit-msg\",\n-\t\t git_path(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n+\t\t git_path_unsafe(\"MERGE_MSG\"), \"merge\", NULL, NULL);\n \tif (option_edit) {\n-\t\tif (launch_editor(git_path(\"MERGE_MSG\"), NULL, NULL))\n+\t\tif (launch_editor(git_path_unsafe(\"MERGE_MSG\"), NULL, NULL))\n \t\t\tabort_commit(NULL);\n \t}\n \tread_merge_msg(&msg);\n@@ -958,7 +958,7 @@ static int suggest_conflicts(int renormalizing)\n \tFILE *fp;\n \tint pos;\n \n-\tfilename = git_path(\"MERGE_MSG\");\n+\tfilename = git_path_unsafe(\"MERGE_MSG\");\n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -1061,7 +1061,7 @@ static void write_merge_state(void)\n \tfor (j = remoteheads; j; j = j->next)\n \t\tstrbuf_addf(&buf, \"%s\\n\",\n \t\t\tsha1_to_hex(j->item->object.sha1));\n-\tfilename = git_path(\"MERGE_HEAD\");\n+\tfilename = git_path_unsafe(\"MERGE_HEAD\");\n \tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -1071,7 +1071,7 @@ static void write_merge_state(void)\n \tstrbuf_addch(&merge_msg, '\\n');\n \twrite_merge_msg(&merge_msg);\n \n-\tfilename = git_path(\"MERGE_MODE\");\n+\tfilename = git_path_unsafe(\"MERGE_MODE\");\n \tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -1126,7 +1126,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tint nargc = 2;\n \t\tconst char *nargv[] = {\"reset\", \"--merge\", NULL};\n \n-\t\tif (!file_exists(git_path(\"MERGE_HEAD\")))\n+\t\tif (!file_exists(git_path_unsafe(\"MERGE_HEAD\")))\n \t\t\tdie(_(\"There is no merge to abort (MERGE_HEAD missing).\"));\n \n \t\t/* Invoke 'git reset --merge' */\n@@ -1136,7 +1136,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (read_cache_unmerged())\n \t\tdie_resolve_conflict(\"merge\");\n \n-\tif (file_exists(git_path(\"MERGE_HEAD\"))) {\n+\tif (file_exists(git_path_unsafe(\"MERGE_HEAD\"))) {\n \t\t/*\n \t\t * There is no unmerged entry, don't advise 'git\n \t\t * add/rm ', just 'git commit'.\n@@ -1147,7 +1147,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tdie(_(\"You have not concluded your merge (MERGE_HEAD exists).\"));\n \t}\n-\tif (file_exists(git_path(\"CHERRY_PICK_HEAD\"))) {\n+\tif (file_exists(git_path_unsafe(\"CHERRY_PICK_HEAD\"))) {\n \t\tif (advice_resolve_conflict)\n \t\t\tdie(_(\"You have not concluded your cherry-pick (CHERRY_PICK_HEAD exists).\\n\"\n \t\t\t \"Please, commit your changes before you can merge.\"));\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437db..c037afe6 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -944,7 +944,7 @@ static int merge(int argc, const char **argv, const char *prefix)\n \t\tprintf(\"Automatic notes merge failed. Fix conflicts in %s and \"\n \t\t \"commit the result with 'git notes merge --commit', or \"\n \t\t \"abort the merge with 'git notes merge --abort'.\\n\",\n-\t\t git_path(NOTES_MERGE_WORKTREE));\n+\t\t git_path_unsafe(NOTES_MERGE_WORKTREE));\n \t}\n \n \tfree_notes(t);\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c8106438..c7032125 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -546,7 +546,7 @@ static int add_branch_for_removal(const char *refname,\n \n \t/* make sure that symrefs are deleted */\n \tif (flags & REF_ISSYMREF)\n-\t\treturn unlink(git_path(\"%s\", refname));\n+\t\treturn unlink(git_path_unsafe(\"%s\", refname));\n \n \titem = string_list_append(branches->branches, refname);\n \titem->util = xmalloc(20);\n@@ -608,9 +608,9 @@ static int migrate_file(struct remote *remote)\n \t\t\treturn error(\"Could not append '%s' to '%s'\",\n \t\t\t\t\tremote->fetch_refspec[i], buf.buf);\n \tif (remote->origin == REMOTE_REMOTES)\n-\t\tpath = git_path(\"remotes/%s\", remote->name);\n+\t\tpath = git_path_unsafe(\"remotes/%s\", remote->name);\n \telse if (remote->origin == REMOTE_BRANCHES)\n-\t\tpath = git_path(\"branches/%s\", remote->name);\n+\t\tpath = git_path_unsafe(\"branches/%s\", remote->name);\n \tif (path)\n \t\tunlink_or_warn(path);\n \treturn 0;\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 811e8e25..18bacdac 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -35,7 +35,7 @@ static const char *reset_type_names[] = {\n \n static inline int is_merge(void)\n {\n-\treturn !access(git_path(\"MERGE_HEAD\"), F_OK);\n+\treturn !access(git_path_unsafe(\"MERGE_HEAD\"), F_OK);\n }\n \n static int reset_index_file(const unsigned char *sha1, int reset_type, int quiet)\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 985f95b0..09a062c6 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -294,7 +294,7 @@ static void write_cherry_pick_head(struct commit *commit)\n \n \tstrbuf_addf(&buf, \"%s\\n\", sha1_to_hex(commit->object.sha1));\n \n-\tfilename = git_path(\"CHERRY_PICK_HEAD\");\n+\tfilename = git_path_unsafe(\"CHERRY_PICK_HEAD\");\n \tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n \tif (fd < 0)\n \t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n@@ -314,7 +314,7 @@ static void print_advice(int show_hint)\n \t\t * (typically rebase --interactive) wants to take care\n \t\t * of the commit itself so remove CHERRY_PICK_HEAD\n \t\t */\n-\t\tunlink(git_path(\"CHERRY_PICK_HEAD\"));\n+\t\tunlink(git_path_unsafe(\"CHERRY_PICK_HEAD\"));\n \t\treturn;\n \t}\n \n@@ -762,7 +762,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n static void read_populate_todo(struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n+\tconst char *todo_file = git_path_unsafe(SEQ_TODO_FILE);\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd, res;\n \n@@ -817,7 +817,7 @@ static int populate_opts_cb(const char *key, const char *value, void *data)\n \n static void read_populate_opts(struct replay_opts **opts_ptr)\n {\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\tconst char *opts_file = git_path_unsafe(SEQ_OPTS_FILE);\n \n \tif (!file_exists(opts_file))\n \t\treturn;\n@@ -841,7 +841,7 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,\n \n static int create_seq_dir(void)\n {\n-\tconst char *seq_dir = git_path(SEQ_DIR);\n+\tconst char *seq_dir = git_path_unsafe(SEQ_DIR);\n \n \tif (file_exists(seq_dir))\n \t\treturn error(_(\"%s already exists.\"), seq_dir);\n@@ -852,7 +852,7 @@ static int create_seq_dir(void)\n \n static void save_head(const char *head)\n {\n-\tconst char *head_file = git_path(SEQ_HEAD_FILE);\n+\tconst char *head_file = git_path_unsafe(SEQ_HEAD_FILE);\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n@@ -867,7 +867,7 @@ static void save_head(const char *head)\n \n static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n {\n-\tconst char *todo_file = git_path(SEQ_TODO_FILE);\n+\tconst char *todo_file = git_path_unsafe(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint fd;\n@@ -888,7 +888,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \n static void save_opts(struct replay_opts *opts)\n {\n-\tconst char *opts_file = git_path(SEQ_OPTS_FILE);\n+\tconst char *opts_file = git_path_unsafe(SEQ_OPTS_FILE);\n \n \tif (opts->no_commit)\n \t\tgit_config_set_in_file(opts_file, \"options.no-commit\", \"true\");\n@@ -969,7 +969,7 @@ static int pick_revisions(struct replay_opts *opts)\n \t\tremove_sequencer_state(1);\n \t\treturn 0;\n \t} else if (opts->subcommand == REPLAY_CONTINUE) {\n-\t\tif (!file_exists(git_path(SEQ_TODO_FILE)))\n+\t\tif (!file_exists(git_path_unsafe(SEQ_TODO_FILE)))\n \t\t\tgoto error;\n \t\tread_populate_opts(&opts);\n \t\tread_populate_todo(&todo_list, opts);\ndiff --git a/cache.h b/cache.h\nindex 2e6ad360..7fb85445 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -662,7 +662,8 @@ extern char *git_pathdup(const char *fmt, ...)\n \n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n+extern char *git_path_unsafe(const char *fmt, ...)\n+\t__attribute__((format (printf, 1, 2)));\n extern char *git_path_submodule(const char *path, const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n \ndiff --git a/contrib/examples/builtin-fetch--tool.c b/contrib/examples/builtin-fetch--tool.c\nindex 3140e405..ea18fdac 100644\n--- a/contrib/examples/builtin-fetch--tool.c\n+++ b/contrib/examples/builtin-fetch--tool.c\n@@ -515,7 +515,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \n \t\tif (argc != 8)\n \t\t\treturn error(\"append-fetch-head takes 6 args\");\n-\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfilename = git_path_unsafe(\"FETCH_HEAD\");\n \t\tfp = fopen(filename, \"a\");\n \t\tif (!fp)\n \t\t\treturn error(\"cannot open %s: %s\\n\", filename, strerror(errno));\n@@ -533,7 +533,7 @@ int cmd_fetch__tool(int argc, const char **argv, const char *prefix)\n \n \t\tif (argc != 5)\n \t\t\treturn error(\"fetch-native-store takes 3 args\");\n-\t\tfilename = git_path(\"FETCH_HEAD\");\n+\t\tfilename = git_path_unsafe(\"FETCH_HEAD\");\n \t\tfp = fopen(filename, \"a\");\n \t\tif (!fp)\n \t\t\treturn error(\"cannot open %s: %s\\n\", filename, strerror(errno));\ndiff --git a/dir.c b/dir.c\nindex 6c0d7825..94662509 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -1224,7 +1224,7 @@ void setup_standard_excludes(struct dir_struct *dir)\n \tconst char *path;\n \n \tdir->exclude_per_dir = \".gitignore\";\n-\tpath = git_path(\"info/exclude\");\n+\tpath = git_path_unsafe(\"info/exclude\");\n \tif (!access(path, R_OK))\n \t\tadd_excludes_from_file(dir, path);\n \tif (excludes_file && !access(excludes_file, R_OK))\ndiff --git a/fast-import.c b/fast-import.c\nindex 8d8ea3c4..04bcd353 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -403,7 +403,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);\n \n static void write_crash_report(const char *err)\n {\n-\tchar *loc = git_path(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n+\tchar *loc = git_path_unsafe(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n \tFILE *rpt = fopen(loc, \"w\");\n \tstruct branch *b;\n \tunsigned long lu;\ndiff --git a/http-backend.c b/http-backend.c\nindex 59ad7da6..7169e040 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -161,7 +161,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \n static void send_local_file(const char *the_type, const char *name)\n {\n-\tconst char *p = git_path(\"%s\", name);\n+\tconst char *p = git_path_unsafe(\"%s\", name);\n \tsize_t buf_alloc = 8192;\n \tchar *buf = xmalloc(buf_alloc);\n \tint fd;\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e9e41993..0b49e8ad 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -275,35 +275,37 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)\n \t\t * Must establish NOTES_MERGE_WORKTREE.\n \t\t * Abort if NOTES_MERGE_WORKTREE already exists\n \t\t */\n-\t\tif (file_exists(git_path(NOTES_MERGE_WORKTREE))) {\n+\t\tif (file_exists(git_path_unsafe(NOTES_MERGE_WORKTREE))) {\n \t\t\tif (advice_resolve_conflict)\n \t\t\t\tdie(\"You have not concluded your previous \"\n \t\t\t\t \"notes merge (%s exists).\\nPlease, use \"\n \t\t\t\t \"'git notes merge --commit' or 'git notes \"\n \t\t\t\t \"merge --abort' to commit/abort the \"\n \t\t\t\t \"previous merge before you start a new \"\n-\t\t\t\t \"notes merge.\", git_path(\"NOTES_MERGE_*\"));\n+\t\t\t\t \"notes merge.\",\n+\t\t\t\t git_path_unsafe(\"NOTES_MERGE_*\"));\n \t\t\telse\n \t\t\t\tdie(\"You have not concluded your notes merge \"\n-\t\t\t\t \"(%s exists).\", git_path(\"NOTES_MERGE_*\"));\n+\t\t\t\t \"(%s exists).\",\n+\t\t\t\t git_path_unsafe(\"NOTES_MERGE_*\"));\n \t\t}\n \n-\t\tif (safe_create_leading_directories(git_path(\n+\t\tif (safe_create_leading_directories(git_path_unsafe(\n \t\t\t\tNOTES_MERGE_WORKTREE \"/.test\")))\n \t\t\tdie_errno(\"unable to create directory %s\",\n-\t\t\t\t git_path(NOTES_MERGE_WORKTREE));\n+\t\t\t\t git_path_unsafe(NOTES_MERGE_WORKTREE));\n \t\to->has_worktree = 1;\n-\t} else if (!file_exists(git_path(NOTES_MERGE_WORKTREE)))\n+\t} else if (!file_exists(git_path_unsafe(NOTES_MERGE_WORKTREE)))\n \t\t/* NOTES_MERGE_WORKTREE should already be established */\n \t\tdie(\"missing '%s'. This should not happen\",\n-\t\t git_path(NOTES_MERGE_WORKTREE));\n+\t\t git_path_unsafe(NOTES_MERGE_WORKTREE));\n }\n \n static void write_buf_to_worktree(const unsigned char *obj,\n \t\t\t\t const char *buf, unsigned long size)\n {\n \tint fd;\n-\tchar *path = git_path(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n+\tchar *path = git_path_unsafe(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n \tif (safe_create_leading_directories(path))\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n \tif (file_exists(path))\n@@ -681,7 +683,7 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t * Finally store the new commit object SHA1 into 'result_sha1'.\n \t */\n \tstruct dir_struct dir;\n-\tchar *path = xstrdup(git_path(NOTES_MERGE_WORKTREE \"/\"));\n+\tchar *path = git_pathdup(NOTES_MERGE_WORKTREE \"/\");\n \tint path_len = strlen(path), i;\n \tconst char *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n \n@@ -731,7 +733,7 @@ int notes_merge_abort(struct notes_merge_options *o)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \n-\tstrbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));\n+\tstrbuf_addstr(&buf, git_path_unsafe(NOTES_MERGE_WORKTREE));\n \tOUTPUT(o, 3, \"Removing notes merge worktree at %s\", buf.buf);\n \tret = remove_dir_recursively(&buf, 0);\n \tstrbuf_release(&buf);\ndiff --git a/pack-refs.c b/pack-refs.c\nindex 23bbd00e..9557b063 100644\n--- a/pack-refs.c\n+++ b/pack-refs.c\n@@ -86,7 +86,7 @@ static void try_remove_empty_parents(char *name)\n \t\tif (q == p)\n \t\t\tbreak;\n \t\t*q = '\\0';\n-\t\tif (rmdir(git_path(\"%s\", name)))\n+\t\tif (rmdir(git_path_unsafe(\"%s\", name)))\n \t\t\tbreak;\n \t}\n }\n@@ -97,7 +97,7 @@ static void prune_ref(struct ref_to_prune *r)\n \tstruct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);\n \n \tif (lock) {\n-\t\tunlink_or_warn(git_path(\"%s\", r->name));\n+\t\tunlink_or_warn(git_path_unsafe(\"%s\", r->name));\n \t\tunlock_ref(lock);\n \t\ttry_remove_empty_parents(r->name);\n \t}\n@@ -121,7 +121,7 @@ int pack_refs(unsigned int flags)\n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"),\n+\tfd = hold_lock_file_for_update(&packed, git_path_unsafe(\"packed-refs\"),\n \t\t\t\t LOCK_DIE_ON_ERROR);\n \tcbdata.refs_file = fdopen(fd, \"w\");\n \tif (!cbdata.refs_file)\ndiff --git a/path.c b/path.c\nindex b6f71d10..0611b7be 100644\n--- a/path.c\n+++ b/path.c\n@@ -101,7 +101,7 @@ char *mkpath(const char *fmt, ...)\n \treturn cleanup_path(pathname);\n }\n \n-char *git_path(const char *fmt, ...)\n+char *git_path_unsafe(const char *fmt, ...)\n {\n \tconst char *git_dir = get_git_dir();\n \tchar *pathname = get_pathname();\ndiff --git a/refs.c b/refs.c\nindex e69ba26b..e527c7b7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -268,7 +268,7 @@ static struct ref_array *get_packed_refs(const char *submodule)\n \t\tif (submodule)\n \t\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n \t\telse\n-\t\t\tpacked_refs_file = git_path(\"packed-refs\");\n+\t\t\tpacked_refs_file = git_path_unsafe(\"packed-refs\");\n \t\tf = fopen(packed_refs_file, \"r\");\n \t\tif (f) {\n \t\t\tread_packed_refs(f, &refs->packed);\n@@ -288,7 +288,7 @@ static void get_ref_dir(const char *submodule, const char *base,\n \tif (submodule)\n \t\tpath = git_path_submodule(submodule, \"%s\", base);\n \telse\n-\t\tpath = git_path(\"%s\", base);\n+\t\tpath = git_path_unsafe(\"%s\", base);\n \n \n \tdir = opendir(path);\n@@ -319,7 +319,7 @@ static void get_ref_dir(const char *submodule, const char *base,\n \t\t\tmemcpy(ref + baselen, de->d_name, namelen+1);\n \t\t\trefdir = submodule\n \t\t\t\t? git_path_submodule(submodule, \"%s\", ref)\n-\t\t\t\t: git_path(\"%s\", ref);\n+\t\t\t\t: git_path_unsafe(\"%s\", ref);\n \t\t\tif (stat(refdir, &st) < 0)\n \t\t\t\tcontinue;\n \t\t\tif (S_ISDIR(st.st_mode)) {\n@@ -1146,11 +1146,11 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tref = resolve_ref(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n-\t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n+\t\tif (!stat(git_path_unsafe(\"logs/%s\", path), &st) &&\n \t\t S_ISREG(st.st_mode))\n \t\t\tit = path;\n \t\telse if (strcmp(ref, path) &&\n-\t\t\t !stat(git_path(\"logs/%s\", ref), &st) &&\n+\t\t\t !stat(git_path_unsafe(\"logs/%s\", ref), &st) &&\n \t\t\t S_ISREG(st.st_mode))\n \t\t\tit = ref;\n \t\telse\n@@ -1186,7 +1186,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t * it is normal for the empty directory 'foo'\n \t\t * to remain.\n \t\t */\n-\t\tref_file = git_path(\"%s\", orig_ref);\n+\t\tref_file = git_path_unsafe(\"%s\", orig_ref);\n \t\tif (remove_empty_directories(ref_file)) {\n \t\t\tlast_errno = errno;\n \t\t\terror(\"there are still refs under '%s'\", orig_ref);\n@@ -1223,7 +1223,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t}\n \tlock->ref_name = xstrdup(ref);\n \tlock->orig_ref_name = xstrdup(orig_ref);\n-\tref_file = git_path(\"%s\", ref);\n+\tref_file = git_path_unsafe(\"%s\", ref);\n \tif (missing)\n \t\tlock->force_write = 1;\n \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n@@ -1272,9 +1272,10 @@ static int repack_without_ref(const char *refname)\n \tref = search_ref_array(packed, refname);\n \tif (ref == NULL)\n \t\treturn 0;\n-\tfd = hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0);\n+\tfd = hold_lock_file_for_update(&packlock,\n+\t\t\t\t git_path_unsafe(\"packed-refs\"), 0);\n \tif (fd < 0) {\n-\t\tunable_to_lock_error(git_path(\"packed-refs\"), errno);\n+\t\tunable_to_lock_error(git_path_unsafe(\"packed-refs\"), errno);\n \t\treturn error(\"cannot delete '%s' from packed refs\", refname);\n \t}\n \n@@ -1313,7 +1314,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \t\t\tlock->lk->filename[i] = 0;\n \t\t\tpath = lock->lk->filename;\n \t\t} else {\n-\t\t\tpath = git_path(\"%s\", refname);\n+\t\t\tpath = git_path_unsafe(\"%s\", refname);\n \t\t}\n \t\terr = unlink_or_warn(path);\n \t\tif (err && errno != ENOENT)\n@@ -1328,7 +1329,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \t */\n \tret |= repack_without_ref(refname);\n \n-\tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n+\tunlink_or_warn(git_path_unsafe(\"logs/%s\", lock->ref_name));\n \tinvalidate_ref_cache(NULL);\n \tunlock_ref(lock);\n \treturn ret;\n@@ -1349,7 +1350,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tint flag = 0, logmoved = 0;\n \tstruct ref_lock *lock;\n \tstruct stat loginfo;\n-\tint log = !lstat(git_path(\"logs/%s\", oldref), &loginfo);\n+\tint log = !lstat(git_path_unsafe(\"logs/%s\", oldref), &loginfo);\n \tconst char *symref = NULL;\n \n \tif (log && S_ISLNK(loginfo.st_mode))\n@@ -1368,7 +1369,8 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tif (!is_refname_available(newref, oldref, get_loose_refs(NULL), 0))\n \t\treturn 1;\n \n-\tif (log && rename(git_path(\"logs/%s\", oldref), git_path(TMP_RENAMED_LOG)))\n+\tif (log && rename(git_path_unsafe(\"logs/%s\", oldref),\n+\t\t\t git_path_unsafe(TMP_RENAMED_LOG)))\n \t\treturn error(\"unable to move logfile logs/%s to \"TMP_RENAMED_LOG\": %s\",\n \t\t\toldref, strerror(errno));\n \n@@ -1379,7 +1381,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \n \tif (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n-\t\t\tif (remove_empty_directories(git_path(\"%s\", newref))) {\n+\t\t\tif (remove_empty_directories(git_path_unsafe(\"%s\", newref))) {\n \t\t\t\terror(\"Directory not empty: %s\", newref);\n \t\t\t\tgoto rollback;\n \t\t\t}\n@@ -1389,20 +1391,21 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \t\t}\n \t}\n \n-\tif (log && safe_create_leading_directories(git_path(\"logs/%s\", newref))) {\n+\tif (log && safe_create_leading_directories(git_path_unsafe(\"logs/%s\", newref))) {\n \t\terror(\"unable to create directory for %s\", newref);\n \t\tgoto rollback;\n \t}\n \n retry:\n-\tif (log && rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", newref))) {\n+\tif (log && rename(git_path_unsafe(TMP_RENAMED_LOG),\n+\t\t\t git_path_unsafe(\"logs/%s\", newref))) {\n \t\tif (errno==EISDIR || errno==ENOTDIR) {\n \t\t\t/*\n \t\t\t * rename(a, b) when b is an existing\n \t\t\t * directory ought to result in ISDIR, but\n \t\t\t * Solaris 5.8 gives ENOTDIR. Sheesh.\n \t\t\t */\n-\t\t\tif (remove_empty_directories(git_path(\"logs/%s\", newref))) {\n+\t\t\tif (remove_empty_directories(git_path_unsafe(\"logs/%s\", newref))) {\n \t\t\t\terror(\"Directory not empty: logs/%s\", newref);\n \t\t\t\tgoto rollback;\n \t\t\t}\n@@ -1444,11 +1447,13 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tlog_all_ref_updates = flag;\n \n rollbacklog:\n-\tif (logmoved && rename(git_path(\"logs/%s\", newref), git_path(\"logs/%s\", oldref)))\n+\tif (logmoved && rename(git_path_unsafe(\"logs/%s\", newref),\n+\t\t\t git_path_unsafe(\"logs/%s\", oldref)))\n \t\terror(\"unable to restore logfile %s from %s: %s\",\n \t\t\toldref, newref, strerror(errno));\n \tif (!logmoved && log &&\n-\t rename(git_path(TMP_RENAMED_LOG), git_path(\"logs/%s\", oldref)))\n+\t rename(git_path_unsafe(TMP_RENAMED_LOG),\n+\t\t git_path_unsafe(\"logs/%s\", oldref)))\n \t\terror(\"unable to restore logfile %s from \"TMP_RENAMED_LOG\": %s\",\n \t\t\toldref, strerror(errno));\n \n@@ -1741,7 +1746,7 @@ int read_ref_at(const char *ref, unsigned long at_time, int cnt, unsigned char *\n \tvoid *log_mapped;\n \tsize_t mapsz;\n \n-\tlogfile = git_path(\"logs/%s\", ref);\n+\tlogfile = git_path_unsafe(\"logs/%s\", ref);\n \tlogfd = open(logfile, O_RDONLY, 0);\n \tif (logfd < 0)\n \t\tdie_errno(\"Unable to read log '%s'\", logfile);\n@@ -1841,7 +1846,7 @@ int for_each_recent_reflog_ent(const char *ref, each_reflog_ent_fn fn, long ofs,\n \tstruct strbuf sb = STRBUF_INIT;\n \tint ret = 0;\n \n-\tlogfile = git_path(\"logs/%s\", ref);\n+\tlogfile = git_path_unsafe(\"logs/%s\", ref);\n \tlogfp = fopen(logfile, \"r\");\n \tif (!logfp)\n \t\treturn -1;\n@@ -1899,7 +1904,7 @@ int for_each_reflog_ent(const char *ref, each_reflog_ent_fn fn, void *cb_data)\n \n static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)\n {\n-\tDIR *dir = opendir(git_path(\"logs/%s\", base));\n+\tDIR *dir = opendir(git_path_unsafe(\"logs/%s\", base));\n \tint retval = 0;\n \n \tif (dir) {\n@@ -1923,7 +1928,7 @@ static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)\n \t\t\tif (has_extension(de->d_name, \".lock\"))\n \t\t\t\tcontinue;\n \t\t\tmemcpy(log + baselen, de->d_name, namelen+1);\n-\t\t\tif (stat(git_path(\"logs/%s\", log), &st) < 0)\n+\t\t\tif (stat(git_path_unsafe(\"logs/%s\", log), &st) < 0)\n \t\t\t\tcontinue;\n \t\t\tif (S_ISDIR(st.st_mode)) {\n \t\t\t\tretval = do_for_each_reflog(log, fn, cb_data);\ndiff --git a/remote.c b/remote.c\nindex e2ef9911..bb85b326 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -224,7 +224,7 @@ static void add_instead_of(struct rewrite *rewrite, const char *instead_of)\n \n static void read_remotes_file(struct remote *remote)\n {\n-\tFILE *f = fopen(git_path(\"remotes/%s\", remote->name), \"r\");\n+\tFILE *f = fopen(git_path_unsafe(\"remotes/%s\", remote->name), \"r\");\n \n \tif (!f)\n \t\treturn;\n@@ -275,7 +275,7 @@ static void read_branches_file(struct remote *remote)\n \tchar *frag;\n \tstruct strbuf branch = STRBUF_INIT;\n \tint n = slash ? slash - remote->name : 1000;\n-\tFILE *f = fopen(git_path(\"branches/%.*s\", n, remote->name), \"r\");\n+\tFILE *f = fopen(git_path_unsafe(\"branches/%.*s\", n, remote->name), \"r\");\n \tchar *s, *p;\n \tint len;\n \ndiff --git a/rerere.c b/rerere.c\nindex dcb525a4..61f60701 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -22,7 +22,7 @@ static char *merge_rr_path;\n \n const char *rerere_path(const char *hex, const char *file)\n {\n-\treturn git_path(\"rr-cache/%s/%s\", hex, file);\n+\treturn git_path_unsafe(\"rr-cache/%s/%s\", hex, file);\n }\n \n int has_rerere_resolution(const char *hex)\n@@ -524,7 +524,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)\n \t\t\t\tcontinue;\n \t\t\thex = xstrdup(sha1_to_hex(sha1));\n \t\t\tstring_list_insert(rr, path)->util = hex;\n-\t\t\tif (mkdir(git_path(\"rr-cache/%s\", hex), 0755))\n+\t\t\tif (mkdir(git_path_unsafe(\"rr-cache/%s\", hex), 0755))\n \t\t\t\tcontinue;\n \t\t\thandle_file(path, NULL, rerere_path(hex, \"preimage\"));\n \t\t\tfprintf(stderr, \"Recorded preimage for '%s'\\n\", path);\n@@ -591,7 +591,7 @@ static int is_rerere_enabled(void)\n \tif (!rerere_enabled)\n \t\treturn 0;\n \n-\trr_cache = git_path(\"rr-cache\");\n+\trr_cache = git_path_unsafe(\"rr-cache\");\n \trr_cache_exists = is_directory(rr_cache);\n \tif (rerere_enabled < 0)\n \t\treturn rr_cache_exists;\n@@ -695,7 +695,7 @@ static void unlink_rr_item(const char *name)\n \tunlink(rerere_path(name, \"thisimage\"));\n \tunlink(rerere_path(name, \"preimage\"));\n \tunlink(rerere_path(name, \"postimage\"));\n-\trmdir(git_path(\"rr-cache/%s\", name));\n+\trmdir(git_path_unsafe(\"rr-cache/%s\", name));\n }\n \n struct rerere_gc_config_cb {\n@@ -726,7 +726,7 @@ void rerere_gc(struct string_list *rr)\n \tstruct rerere_gc_config_cb cf = { 15, 60 };\n \n \tgit_config(git_rerere_gc_config, &cf);\n-\tdir = opendir(git_path(\"rr-cache\"));\n+\tdir = opendir(git_path_unsafe(\"rr-cache\"));\n \tif (!dir)\n \t\tdie_errno(\"unable to open rr-cache directory\");\n \twhile ((e = readdir(dir))) {\n@@ -760,5 +760,5 @@ void rerere_clear(struct string_list *merge_rr)\n \t\tif (!has_rerere_resolution(name))\n \t\t\tunlink_rr_item(name);\n \t}\n-\tunlink_or_warn(git_path(\"MERGE_RR\"));\n+\tunlink_or_warn(git_path_unsafe(\"MERGE_RR\"));\n }\ndiff --git a/run-command.c b/run-command.c\nindex 1c510438..598e41dd 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -612,11 +612,11 @@ int run_hook(const char *index_file, const char *name, ...)\n \tva_list args;\n \tint ret;\n \n-\tif (access(git_path(\"hooks/%s\", name), X_OK) < 0)\n+\tif (access(git_path_unsafe(\"hooks/%s\", name), X_OK) < 0)\n \t\treturn 0;\n \n \tva_start(args, name);\n-\targv_array_push(&argv, git_path(\"hooks/%s\", name));\n+\targv_array_push(&argv, git_path_unsafe(\"hooks/%s\", name));\n \twhile ((p = va_arg(args, const char *)))\n \t\targv_array_push(&argv, p);\n \tva_end(args);\ndiff --git a/sequencer.c b/sequencer.c\nindex bc2c046a..2e29152c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -8,10 +8,10 @@ void remove_sequencer_state(int aggressive)\n \tstruct strbuf seq_dir = STRBUF_INIT;\n \tstruct strbuf seq_old_dir = STRBUF_INIT;\n \n-\tstrbuf_addf(&seq_dir, \"%s\", git_path(SEQ_DIR));\n-\tstrbuf_addf(&seq_old_dir, \"%s\", git_path(SEQ_OLD_DIR));\n+\tstrbuf_addf(&seq_dir, \"%s\", git_path_unsafe(SEQ_DIR));\n+\tstrbuf_addf(&seq_old_dir, \"%s\", git_path_unsafe(SEQ_OLD_DIR));\n \tremove_dir_recursively(&seq_old_dir, 0);\n-\trename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));\n+\trename(git_path_unsafe(SEQ_DIR), git_path_unsafe(SEQ_OLD_DIR));\n \tif (aggressive)\n \t\tremove_dir_recursively(&seq_old_dir, 0);\n \tstrbuf_release(&seq_dir);\ndiff --git a/server-info.c b/server-info.c\nindex 9ec744e9..348a3447 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -243,7 +243,7 @@ int update_server_info(int force)\n \terrs = errs | update_info_packs(force);\n \n \t/* remove leftover rev-cache file if there is any */\n-\tunlink_or_warn(git_path(\"info/rev-cache\"));\n+\tunlink_or_warn(git_path_unsafe(\"info/rev-cache\"));\n \n \treturn errs;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 86705bc9..ba7eca89 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -382,7 +382,7 @@ static void read_info_alternates(const char * relative_base, int depth)\n void add_to_alternates_file(const char *reference)\n {\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n+\tint fd = hold_lock_file_for_append(lock, git_path_unsafe(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n \tchar *alt = mkpath(\"%s\\n\", reference);\n \twrite_or_die(fd, alt, strlen(alt));\n \tif (commit_lock_file(lock))\n@@ -2706,7 +2706,7 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \tint len, tmpfd;\n \n \tstrbuf_addstr(&export_marks, \"--export-marks=\");\n-\tstrbuf_addstr(&export_marks, git_path(\"hashstream_XXXXXX\"));\n+\tstrbuf_addstr(&export_marks, git_path_unsafe(\"hashstream_XXXXXX\"));\n \ttmpfile = export_marks.buf + strlen(\"--export-marks=\");\n \ttmpfd = git_mkstemp_mode(tmpfile, 0600);\n \tif (tmpfd < 0)\ndiff --git a/shallow.c b/shallow.c\nindex a0363dea..d721397a 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -25,7 +25,7 @@ int is_repository_shallow(void)\n \tif (is_shallow >= 0)\n \t\treturn is_shallow;\n \n-\tfp = fopen(git_path(\"shallow\"), \"r\");\n+\tfp = fopen(git_path_unsafe(\"shallow\"), \"r\");\n \tif (!fp) {\n \t\tis_shallow = 0;\n \t\treturn is_shallow;\ndiff --git a/transport.c b/transport.c\nindex 51814b5d..cc0ca04c 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -203,7 +203,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)\n \n \t/* copy the refs to the temporary directory */\n \n-\tstrbuf_addstr(&temp_dir, git_path(\"rsync-refs-XXXXXX\"));\n+\tstrbuf_addstr(&temp_dir, git_path_unsafe(\"rsync-refs-XXXXXX\"));\n \tif (!mkdtemp(temp_dir.buf))\n \t\tdie_errno (\"Could not make temporary directory\");\n \ttemp_dir_len = temp_dir.len;\n@@ -366,7 +366,7 @@ static int rsync_transport_push(struct transport *transport,\n \n \t/* copy the refs to the temporary directory; they could be packed. */\n \n-\tstrbuf_addstr(&temp_dir, git_path(\"rsync-refs-XXXXXX\"));\n+\tstrbuf_addstr(&temp_dir, git_path_unsafe(\"rsync-refs-XXXXXX\"));\n \tif (!mkdtemp(temp_dir.buf))\n \t\tdie_errno (\"Could not make temporary directory\");\n \tstrbuf_addch(&temp_dir, '/');\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8282f5e5..44f408b8 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1010,7 +1010,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \tif (!core_apply_sparse_checkout || !o->update)\n \t\to->skip_sparse_checkout = 1;\n \tif (!o->skip_sparse_checkout) {\n-\t\tif (add_excludes_from_file_to_list(git_path(\"info/sparse-checkout\"), \"\", 0, NULL, &el, 0) < 0)\n+\t\tif (add_excludes_from_file_to_list(git_path_unsafe(\"info/sparse-checkout\"), \"\", 0, NULL, &el, 0) < 0)\n \t\t\to->skip_sparse_checkout = 1;\n \t\telse\n \t\t\to->el = ⪙\n-- \n1.7.8.rc0\n"},{"id":"179569","messageId":"CACsJy8Di3ZrPdXh1Jf=PbLYRWwx-TEV78NzUukwaxA0xW=rSNg@mail.gmail.com","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T08:37:34Z","receivedAt":"2011-11-16T08:37:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/11/16 Jonathan Nieder :\n> Ramkumar Ramachandra wrote:\n>> Junio C Hamano wrote:\n>\n>>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n>>\n>> I noticed that sha1_to_hex() also operates like this.  The\n>> resolve_ref() function is really important, but using the same\n>> technique for these tiny functions is probably an overkill\n>\n> I don't follow.  Do you mean that not being confusing is overkill,\n> because the function is small that no one will bother to look up the\n> right semantics?  Wait, that sentence didn't come out the way I\n> wanted. ;-)\n>\n> Jokes aside, here's a rough series to do the git_path ->\n> git_path_unsafe renaming.  While writing it, I noticed a couple of\n> bugs, hence the two patches before the last one.  Patch 2 is the more\n> interesting one.\n\nOr perhaps\n - kill git_path(const char *fmt, ...) in favor of git_pathdup() companion\n - git_path(const char *path) maintains a small hash table to keep\ntrack of all returned strings based with \"path\" as key.\n\nOut of 142 git_path() calls in my tree, 97 of them are in form\ngit_path(\"some static string\"). git_path() could learn to keep track\nof all generated strings while keep it convenient to use. I suspect\nwith some macro magic, we can keep track of generated strings without\na hash table.\n-- \nDuy\n"},{"id":"179570","messageId":"CACsJy8A8U130gzsd5ZdcW2gt8FvE=zFk34mRbp+T-R+mvn0TRw@mail.gmail.com","threadId":"28854","inReplyTo":"CACsJy8Di3ZrPdXh1Jf=PbLYRWwx-TEV78NzUukwaxA0xW=rSNg@mail.gmail.com","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T08:42:37Z","receivedAt":"2011-11-16T08:42:37Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 16, 2011 at 3:37 PM, Nguyen Thai Ngoc Duy wrote:\n> I suspect\n> with some macro magic, we can keep track of generated strings without\n> a hash table.\n\nI misremembered. There is an operator to convert symbol to string, not\nthe other way around.\n-- \nDuy\n"},{"id":"179572","messageId":"CALkWK0kPM1S9ELTWL1SiGjFN1e3NK2V-q4ixqM77aUMuDPxdqg@mail.gmail.com","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-16T08:51:46Z","receivedAt":"2011-11-16T08:51:46Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n>> Junio C Hamano wrote:\n>\n>>> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n>>\n>> I noticed that sha1_to_hex() also operates like this.  The\n>> resolve_ref() function is really important, but using the same\n>> technique for these tiny functions is probably an overkill\n>\n> I don't follow.  Do you mean that not being confusing is overkill,\n> because the function is small that no one will bother to look up the\n> right semantics?  Wait, that sentence didn't come out the way I\n> wanted. ;-)\n\nI meant overkill in terms of the work required and code churn.\nOfcourse, I'd have been more than happy to see it being implemented-\nand you've actually done it now! :) Nguyễn has a more fancy solution,\nthough I'm quite happy with this as it is.\n\nFinally, for all the times I've fumbled in git_path() usage:\nLiked-by: Ramkumar Ramachandra \n\nThanks for working on this.\n\n-- Ram\n"},{"id":"179573","messageId":"20111116085944.GA18781@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"CACsJy8Di3ZrPdXh1Jf=PbLYRWwx-TEV78NzUukwaxA0xW=rSNg@mail.gmail.com","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T08:59:44Z","receivedAt":"2011-11-16T08:59:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> Or perhaps\n[...]\n> - git_path(const char *path) maintains a small hash table to keep\n> track of all returned strings based with \"path\" as key.\n>\n> Out of 142 git_path() calls in my tree, 97 of them are in form\n> git_path(\"some static string\").\n\nThe main bit I dislike about patch 3/3 is that constructs like\n'unlink(git_path(\"MERGE_HEAD\"));' are not actually unsafe, unless they\nhappen to sit in the middle of an unsafe\n\n\tconst char *filename = git_path(foo);\n\tint fd;\n\n\tcall_a_function_i_dont_control();\n\tfd = open(filename, O_CREAT|O_WRONLY|O_TRUNC, 0600);\n\nsequence. Lacks that feeling of truth in advertising. And on the\nother hand that this doesn't help with thread-safety at all.\n\nI think if I ran the world, the fundamental operation would be\nstrbuf_addpath(). Unlike git_pathdup(), this lets callers avoid some\nallocation churn if they are in the middle of a loop.\n"},{"id":"179575","messageId":"CACsJy8CYj_s92zG-LnBKtHxV2uaG8-rq-VNJiQYwNJXGKbFeDw@mail.gmail.com","threadId":"28854","inReplyTo":"20111116085944.GA18781@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T09:31:02Z","receivedAt":"2011-11-16T09:31:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 16, 2011 at 3:59 PM, Jonathan Nieder wrote:\n> Nguyen Thai Ngoc Duy wrote:\n>\n>> Or perhaps\n> [...]\n>>  - git_path(const char *path) maintains a small hash table to keep\n>> track of all returned strings based with \"path\" as key.\n>>\n>> Out of 142 git_path() calls in my tree, 97 of them are in form\n>> git_path(\"some static string\").\n>\n> The main bit I dislike about patch 3/3 is that constructs like\n> 'unlink(git_path(\"MERGE_HEAD\"));' are not actually unsafe\n\nWell, we can create wrappers (e.g. repo_unlink(const char *) that\ncalls git_path internally). According to grep/sed these functions are\nused in form xxx(git_path(xxx))\n\n 16 unlink\n 8 file_exists\n 7 stat\n 6 fopen\n 5 rename\n 5 open\n 4 unlink_or_warn\n 3 safe_create_dir\n 3 adjust_shared_perm\n 3 access\n 2 xstrdup\n 2 safe_create_leading_directories\n 2 rmdir\n 2 remove_empty_directories\n 2 opendir\n 1 unable_to_lock_error\n 1 read_attr_from_file\n 1 mkdir\n 1 lstat\n 1 launch_editor\n 1 add_excludes_from_file_to_list\n\nBy creating wrappers for unlink, file_exists, stat, fopen, rename and\nopen we can safely avoid git_pathdup()/free() in 42 places.\n-- \nDuy\n"},{"id":"179587","messageId":"CACsJy8A2=qBiyY3SD-PZo+E=U+Dfjm1UQidgq6khQARZ3d41WQ@mail.gmail.com","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-16T13:33:26Z","receivedAt":"2011-11-16T13:33:26Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/11/16 Jonathan Nieder :\n> Jokes aside, here's a rough series to do the git_path ->\n> git_path_unsafe renaming.  While writing it, I noticed a couple of\n> bugs, hence the two patches before the last one.  Patch 2 is the more\n> interesting one.\n\nAnother approach is do nothing and leave it for a static analysis tool\nto detect potential problems. I'm looking at sparse at the moment,\nalthough I know nothing about it to say if it can or cannot detect\nsuch problems. We can at least make sparse detect return value from\ngit_path() being passed to an unsafe function, I think.\n-- \nDuy\n"},{"id":"179589","messageId":"4EC3BE53.3020705@alum.mit.edu","threadId":"28854","inReplyTo":"CACsJy8A2=qBiyY3SD-PZo+E=U+Dfjm1UQidgq6khQARZ3d41WQ@mail.gmail.com","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-11-16T13:44:51Z","receivedAt":"2011-11-16T13:44:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/16/2011 02:33 PM, Nguyen Thai Ngoc Duy wrote:\n> 2011/11/16 Jonathan Nieder :\n>> Jokes aside, here's a rough series to do the git_path ->\n>> git_path_unsafe renaming. While writing it, I noticed a couple of\n>> bugs, hence the two patches before the last one. Patch 2 is the more\n>> interesting one.\n> \n> Another approach is do nothing and leave it for a static analysis tool\n> to detect potential problems. I'm looking at sparse at the moment,\n> although I know nothing about it to say if it can or cannot detect\n> such problems. We can at least make sparse detect return value from\n> git_path() being passed to an unsafe function, I think.\n\nFor the cases when static analysis doesn't suffice, recently I posted\nsome patches that make it possible for debug a problem that results from\nthe use of a \"stale\" buffer [1]. But having myself also been bitten by\nthis problem, I'd also be in favor of a more systematic solution, even\nif it has a small runtime cost. After all, most of the time the\nfilename created by git_path() is going to be passed to the kernel a\nmoment later, which will usually be vastly slower than an extra malloc/free.\n\nMichael\n\n[1] http://comments.gmane.org/gmane.comp.version-control.git/182209\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"179596","messageId":"20111116215004.GA29872@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"20111116085944.GA18781@elie.hsd1.il.comcast.net","subject":"[PATCH/RFC] introduce strbuf_addpath()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-16T21:50:04Z","receivedAt":"2011-11-16T21:50:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"strbuf_addpath() is like git_path_unsafe(), except instead of\nreturning its own buffer, it appends its result to a buffer provided\nby the caller.\n\nBenefits:\n\n - Since it uses a caller-supplied buffer, unlike git_path_unsafe(),\n there is no risk that one call will clobber the result from\n another.\n\n - Unlike git_pathdup(), it does not need to waste time allocating\n memory in the middle of your tight loop over refs.\n\n - The size of the result is not limited to PATH_MAX.\n\nCaveat: the size of its result is not limited to PATH_MAX. Existing\ncode might be relying on git_path*() to produce a result that is safe\nto copy to a PATH_MAX-sized buffer. Be careful.\n\nThis patch introduces the strbuf_addpath() function and converts a few\nexisting users of the strbuf_addstr(git_path(...)) idiom to\ndemonstrate the API.\n\nSigned-off-by: Jonathan Nieder \n---\nJonathan Nieder wrote:\n\n> I think if I ran the world, the fundamental operation would be\n> strbuf_addpath().\n\nLike this, maybe.\n\nIn these v1.7.8-rc2 days, you should probably spend your time\nreviewing patch 2/3 of the previous series, though. ;-)\n\n cache.h | 3 +++\n notes-merge.c | 2 +-\n path.c | 34 ++++++++++++++++++++++++++++++++++\n sha1_file.c | 2 +-\n transport.c | 4 ++--\n 5 files changed, 41 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 7fb85445..33d7d147 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -659,6 +659,8 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n \t__attribute__((format (printf, 3, 4)));\n extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n+extern void strbuf_addpath(struct strbuf *sb, const char *fmt, ...)\n+\t__attribute__((format (printf, 2, 3)));\n \n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\ndiff --git a/notes-merge.c b/notes-merge.c\nindex 0b49e8ad..738442de 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -733,7 +733,7 @@ int notes_merge_abort(struct notes_merge_options *o)\n \tstruct strbuf buf = STRBUF_INIT;\n \tint ret;\n \n-\tstrbuf_addstr(&buf, git_path_unsafe(NOTES_MERGE_WORKTREE));\n+\tstrbuf_addpath(&buf, NOTES_MERGE_WORKTREE);\n \tOUTPUT(o, 3, \"Removing notes merge worktree at %s\", buf.buf);\n \tret = remove_dir_recursively(&buf, 0);\n \tstrbuf_release(&buf);\ndiff --git a/path.c b/path.c\nindex 0611b7be..28d6326d 100644\n--- a/path.c\n+++ b/path.c\n@@ -33,6 +33,20 @@ static char *cleanup_path(char *path)\n \treturn path;\n }\n \n+static void strbuf_cleanup_path(struct strbuf *sb, size_t pos)\n+{\n+\tchar *newstart;\n+\n+\t/*\n+\t * cleanup_path expects to be acting on a static buffer,\n+\t * so it modifies its argument in place and returns\n+\t * a pointer to the new start of the path.\n+\t */\n+\tnewstart = cleanup_path(sb->buf + pos);\n+\tstrbuf_remove(sb, pos, newstart - sb->buf - pos);\n+\tstrbuf_setlen(sb, strlen(sb->buf));\n+}\n+\n char *mksnpath(char *buf, size_t n, const char *fmt, ...)\n {\n \tva_list args;\n@@ -68,6 +82,18 @@ bad:\n \treturn buf;\n }\n \n+static void strbuf_vaddpath(struct strbuf *sb, const char *fmt, va_list args)\n+{\n+\tconst char *git_dir = get_git_dir();\n+\tsize_t pos = sb->len;\n+\n+\tstrbuf_addstr(sb, git_dir);\n+\tif (pos < sb->len && !is_dir_sep(sb->buf[sb->len - 1]))\n+\t\tstrbuf_addch(sb, '/');\n+\tstrbuf_vaddf(sb, fmt, args);\n+\tstrbuf_cleanup_path(sb, pos);\n+}\n+\n char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n {\n \tva_list args;\n@@ -87,6 +113,14 @@ char *git_pathdup(const char *fmt, ...)\n \treturn xstrdup(path);\n }\n \n+void strbuf_addpath(struct strbuf *sb, const char *fmt, ...)\n+{\n+\tva_list args;\n+\tva_start(args, fmt);\n+\tstrbuf_vaddpath(sb, fmt, args);\n+\tva_end(args);\n+}\n+\n char *mkpath(const char *fmt, ...)\n {\n \tva_list args;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex ba7eca89..315d1004 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2706,7 +2706,7 @@ static int index_stream(unsigned char *sha1, int fd, size_t size,\n \tint len, tmpfd;\n \n \tstrbuf_addstr(&export_marks, \"--export-marks=\");\n-\tstrbuf_addstr(&export_marks, git_path_unsafe(\"hashstream_XXXXXX\"));\n+\tstrbuf_addpath(&export_marks, \"hashstream_XXXXXX\");\n \ttmpfile = export_marks.buf + strlen(\"--export-marks=\");\n \ttmpfd = git_mkstemp_mode(tmpfile, 0600);\n \tif (tmpfd < 0)\ndiff --git a/transport.c b/transport.c\nindex cc0ca04c..f5c95b40 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -203,7 +203,7 @@ static struct ref *get_refs_via_rsync(struct transport *transport, int for_push)\n \n \t/* copy the refs to the temporary directory */\n \n-\tstrbuf_addstr(&temp_dir, git_path_unsafe(\"rsync-refs-XXXXXX\"));\n+\tstrbuf_addpath(&temp_dir, \"rsync-refs-XXXXXX\");\n \tif (!mkdtemp(temp_dir.buf))\n \t\tdie_errno (\"Could not make temporary directory\");\n \ttemp_dir_len = temp_dir.len;\n@@ -366,7 +366,7 @@ static int rsync_transport_push(struct transport *transport,\n \n \t/* copy the refs to the temporary directory; they could be packed. */\n \n-\tstrbuf_addstr(&temp_dir, git_path_unsafe(\"rsync-refs-XXXXXX\"));\n+\tstrbuf_addpath(&temp_dir, \"rsync-refs-XXXXXX\");\n \tif (!mkdtemp(temp_dir.buf))\n \t\tdie_errno (\"Could not make temporary directory\");\n \tstrbuf_addch(&temp_dir, '/');\n-- \n1.7.8.rc2\n"},{"id":"179597","messageId":"7v1ut7rag9.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"CACsJy8Di3ZrPdXh1Jf=PbLYRWwx-TEV78NzUukwaxA0xW=rSNg@mail.gmail.com","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-16T22:04:06Z","receivedAt":"2011-11-16T22:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> ... git_path() could learn to keep track\n> of all generated strings while keep it convenient to use.\n\nThat certainly sounds an interesting approach.\n"},{"id":"179608","messageId":"7vzkfvo88i.fsf@alter.siamese.dyndns.org","threadId":"28854","inReplyTo":"20111116080716.GE13706@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 3/3] rename git_path() to git_path_unsafe()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-17T01:20:13Z","receivedAt":"2011-11-17T01:20:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Given that other functions like real_path() and mkpath() share the same\n\"perishable, use it immediately\" property, and also git_path() is such a\nshort and sweet name, I am beginning to think that we probably should\nleave these alone but document that *path() are \"unsafe\" somewhere and\njust add *path_cpy() or your strbuf_addpath() function.\n\nIn any case, I do not like seeing many list regulars throwing too many\nnon-regression-fix patches during prerelease freeze period on the\nlist. Continuing development for the next cycle is encouraged and trying\nto do so using workflows that you do not usually use is even more\nencouraged, though. You would make more use of the release candidate Git\nfor such activities, and may uncover regressions before the final.\n\nThanks.\n"},{"id":"179618","messageId":"20111117070307.GA18853@elie.hsd1.il.comcast.net","threadId":"28854","inReplyTo":"7vzkfvo88i.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] rename git_path() to git_path_unsafe()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-17T07:03:07Z","receivedAt":"2011-11-17T07:03:07Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> In any case, I do not like seeing many list regulars throwing too many\n> non-regression-fix patches during prerelease freeze period on the\n> list.\n\nFine, but what about the buffer overflow (not an incredibly recent\nregression, but certainly a fix) addressed in patch 2 of the series?\n"},{"id":"179659","messageId":"20111118014235.GA10917@tre","threadId":"28854","inReplyTo":"20111116215004.GA29872@elie.hsd1.il.comcast.net","subject":"Re: [PATCH/RFC] introduce strbuf_addpath()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-18T01:42:35Z","receivedAt":"2011-11-18T01:42:35Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 16, 2011 at 03:50:04PM -0600, Jonathan Nieder wrote:\n> strbuf_addpath() is like git_path_unsafe(), except instead of\n> returning its own buffer, it appends its result to a buffer provided\n> by the caller.\n> \n> Benefits:\n> \n> - Since it uses a caller-supplied buffer, unlike git_path_unsafe(),\n> there is no risk that one call will clobber the result from\n> another.\n> \n> - Unlike git_pathdup(), it does not need to waste time allocating\n> memory in the middle of your tight loop over refs.\n> \n> - The size of the result is not limited to PATH_MAX.\n> \n> Caveat: the size of its result is not limited to PATH_MAX. Existing\n> code might be relying on git_path*() to produce a result that is safe\n> to copy to a PATH_MAX-sized buffer. Be careful.\n> \n> This patch introduces the strbuf_addpath() function and converts a few\n> existing users of the strbuf_addstr(git_path(...)) idiom to\n> demonstrate the API.\n> \n> Signed-off-by: Jonathan Nieder \n> ---\n> Jonathan Nieder wrote:\n> \n> > I think if I ran the world, the fundamental operation would be\n> > strbuf_addpath().\n> \n> Like this, maybe.\n> \n\n\nIf I ran the world, I would change get_pathname() to return \"struct\nstrbuf *\" instead and change \"char *git_path(..)\" to \"const char *git_path(...)\".\nCode paths that want to modify git_path() return value could\njust use strbuf_addpath()\n\nI did try to turn git_path to return const char *, the following patch\nseems to make it build without any warnings\n\n-- 8< --\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex c6bc8eb..fd7c682 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -1041,7 +1041,7 @@ struct ref *fetch_pack(struct fetch_pack_args *my_args,\n \tif (args.depth > 0) {\n \t\tstruct cache_time mtime;\n \t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tchar *shallow = git_path(\"shallow\");\n+\t\tconst char *shallow = git_path(\"shallow\");\n \t\tint fd;\n \n \t\tmtime.sec = st.st_mtime;\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 91731b9..261f1a0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -367,7 +367,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tchar note[1024];\n \tconst char *what, *kind;\n \tstruct ref *rm;\n-\tchar *url, *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n+\tconst char *filename = dry_run ? \"/dev/null\" : git_path(\"FETCH_HEAD\");\n+\tchar *url;\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -647,7 +648,7 @@ static void check_not_current_branch(struct ref *ref_map)\n \n static int truncate_fetch_head(void)\n {\n-\tchar *filename = git_path(\"FETCH_HEAD\");\n+\tconst char *filename = git_path(\"FETCH_HEAD\");\n \tFILE *fp = fopen(filename, \"w\");\n \n \tif (!fp)\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 0e0e17a..f9624d7 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -215,12 +215,12 @@ static void check_unreachable_object(struct object *obj)\n \t\tprintf(\"dangling %s %s\\n\", typename(obj->type),\n \t\t sha1_to_hex(obj->sha1));\n \t\tif (write_lost_and_found) {\n-\t\t\tchar *filename = git_path(\"lost-found/%s/%s\",\n+\t\t\tconst char *filename = git_path(\"lost-found/%s/%s\",\n \t\t\t\tobj->type == OBJ_COMMIT ? \"commit\" : \"other\",\n \t\t\t\tsha1_to_hex(obj->sha1));\n \t\t\tFILE *f;\n \n-\t\t\tif (safe_create_leading_directories(filename)) {\n+\t\t\tif (safe_create_leading_directories_const(filename)) {\n \t\t\t\terror(\"Could not create lost-found\");\n \t\t\t\treturn;\n \t\t\t}\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 583eec9..0662d37 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -587,7 +587,7 @@ static int migrate_file(struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \tint i;\n-\tchar *path = NULL;\n+\tconst char *path = NULL;\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n \tfor (i = 0; i < remote->url_nr; i++)\ndiff --git a/fast-import.c b/fast-import.c\nindex 8d8ea3c..4293a9f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -403,7 +403,7 @@ static void dump_marks_helper(FILE *, uintmax_t, struct mark_set *);\n \n static void write_crash_report(const char *err)\n {\n-\tchar *loc = git_path(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n+\tconst char *loc = git_path(\"fast_import_crash_%\"PRIuMAX, (uintmax_t) getpid());\n \tFILE *rpt = fopen(loc, \"w\");\n \tstruct branch *b;\n \tunsigned long lu;\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e33c2c9..8da2e0a 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -288,7 +288,7 @@ static void check_notes_merge_worktree(struct notes_merge_options *o)\n \t\t\t\t \"(%s exists).\", git_path(\"NOTES_MERGE_*\"));\n \t\t}\n \n-\t\tif (safe_create_leading_directories(git_path(\n+\t\tif (safe_create_leading_directories_const(git_path(\n \t\t\t\tNOTES_MERGE_WORKTREE \"/.test\")))\n \t\t\tdie_errno(\"unable to create directory %s\",\n \t\t\t\t git_path(NOTES_MERGE_WORKTREE));\n@@ -303,8 +303,8 @@ static void write_buf_to_worktree(const unsigned char *obj,\n \t\t\t\t const char *buf, unsigned long size)\n {\n \tint fd;\n-\tchar *path = git_path(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n-\tif (safe_create_leading_directories(path))\n+\tconst char *path = git_path(NOTES_MERGE_WORKTREE \"/%s\", sha1_to_hex(obj));\n+\tif (safe_create_leading_directories_const(path))\n \t\tdie_errno(\"unable to create directory for '%s'\", path);\n \tif (file_exists(path))\n \t\tdie(\"found existing file at '%s'\", path);\ndiff --git a/refs.c b/refs.c\nindex 62d8a37..7469cf1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1189,7 +1189,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \n static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char *old_sha1, int flags, int *type_p)\n {\n-\tchar *ref_file;\n+\tconst char *ref_file;\n \tconst char *orig_ref = ref;\n \tstruct ref_lock *lock;\n \tint last_errno = 0;\n@@ -1250,7 +1250,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tif ((flags & REF_NODEREF) && (type & REF_ISSYMREF))\n \t\tlock->force_write = 1;\n \n-\tif (safe_create_leading_directories(ref_file)) {\n+\tif (safe_create_leading_directories_const(ref_file)) {\n \t\tlast_errno = errno;\n \t\terror(\"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n@@ -1411,7 +1411,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \t\t}\n \t}\n \n-\tif (log && safe_create_leading_directories(git_path(\"logs/%s\", newref))) {\n+\tif (log && safe_create_leading_directories_const(git_path(\"logs/%s\", newref))) {\n \t\terror(\"unable to create directory for %s\", newref);\n \t\tgoto rollback;\n \t}\n-- 8< --\n"},{"id":"179666","messageId":"20111118033349.GA20827@tre","threadId":"28854","inReplyTo":"20111116075955.GB13706@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-18T03:33:49Z","receivedAt":"2011-11-18T03:33:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Nov 16, 2011 at 01:59:55AM -0600, Jonathan Nieder wrote:\n> Ramkumar Ramachandra wrote:\n> > Junio C Hamano wrote:\n> \n> >> Or perhaps http://thread.gmane.org/gmane.comp.version-control.git/184963/focus=185436\n> >\n> > I noticed that sha1_to_hex() also operates like this. The\n> > resolve_ref() function is really important, but using the same\n> > technique for these tiny functions is probably an overkill\n> \n> I don't follow. Do you mean that not being confusing is overkill,\n> because the function is small that no one will bother to look up the\n> right semantics? Wait, that sentence didn't come out the way I\n> wanted. ;-)\n> \n> Jokes aside, here's a rough series to do the git_path ->\n> git_path_unsafe renaming. While writing it, I noticed a couple of\n> bugs, hence the two patches before the last one. Patch 2 is the more\n> interesting one.\n\nOr perhaps we can use per-file buffer rings instead of a global one.\nThis means git_path() can only interfere another one in the same file,\nmaking the interaction simpler and hopefully simple enough for reviewers\nto catch 90% bugs, therefore safe enough to avoid the _unsafe suffix.\n\nAdding static variable declaration in cache.h is ugly, but that could be\nmoved to a separate header file.\n\ndiff --git a/cache.h b/cache.h\nindex 2e6ad36..437bc3a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -660,9 +660,13 @@ extern char *git_snpath(char *buf, size_t n, const char *fmt, ...)\n extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n \n+#define git_path(...) git_path_1(pathname_array[3 & ++pathname_index], __VA_ARGS__)\n+static char pathname_array[4][PATH_MAX];\n+static int pathname_index;\n+\n /* Return a statically allocated filename matching the sha1 signature */\n extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n+extern char *git_path_1(char *pathname, const char *fmt, ...) __attribute__((format (printf, 2, 3)));\n extern char *git_path_submodule(const char *path, const char *fmt, ...)\n \t__attribute__((format (printf, 2, 3)));\n \ndiff --git a/path.c b/path.c\nindex b6f71d1..3c95db1 100644\n--- a/path.c\n+++ b/path.c\n@@ -101,10 +101,9 @@ char *mkpath(const char *fmt, ...)\n \treturn cleanup_path(pathname);\n }\n \n-char *git_path(const char *fmt, ...)\n+char *git_path_1(char *pathname, const char *fmt, ...)\n {\n \tconst char *git_dir = get_git_dir();\n-\tchar *pathname = get_pathname();\n \tva_list args;\n \tunsigned len;\n \n"},{"id":"179739","messageId":"4EC802AD.1060405@ramsay1.demon.co.uk","threadId":"28854","inReplyTo":"CACsJy8CYj_s92zG-LnBKtHxV2uaG8-rq-VNJiQYwNJXGKbFeDw@mail.gmail.com","subject":"Re: [PATCH 0/3] avoiding unintended consequences of git_path() usage","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-11-19T19:25:33Z","receivedAt":"2011-11-19T19:25:33Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n> On Wed, Nov 16, 2011 at 3:59 PM, Jonathan Nieder wrote:\n>> Nguyen Thai Ngoc Duy wrote:\n>>\n>>> Or perhaps\n>> [...]\n>>> - git_path(const char *path) maintains a small hash table to keep\n>>> track of all returned strings based with \"path\" as key.\n>>>\n>>> Out of 142 git_path() calls in my tree, 97 of them are in form\n>>> git_path(\"some static string\").\n>> The main bit I dislike about patch 3/3 is that constructs like\n>> 'unlink(git_path(\"MERGE_HEAD\"));' are not actually unsafe\n> \n> Well, we can create wrappers (e.g. repo_unlink(const char *) that\n> calls git_path internally). According to grep/sed these functions are\n> used in form xxx(git_path(xxx))\n> \n> 16 unlink\n> 8 file_exists\n> 7 stat\n> 6 fopen\n> 5 rename\n> 5 open\n> 4 unlink_or_warn\n> 3 safe_create_dir\n> 3 adjust_shared_perm\n> 3 access\n> 2 xstrdup\n> 2 safe_create_leading_directories\n\nThis one at least, maybe others, is unsafe on cygwin. Indeed it causes\na test failure in t3200-branch.sh; patch is on it's way ...\n\nATB,\nRamsay Jones\n"}]}