* [PATCH 6/6] sequencer: factor code out of revert builtin
From: Ramkumar Ramachandra @ 2012-01-08 12:27 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jonathan Nieder
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>
Expose the cherry-picking machinery through a public
sequencer_pick_revisions() (renamed from pick_revisions() in
builtin/revert.c), so that cherry-picking and reverting are special
cases of a general sequencer operation. The cherry-pick builtin is
now a thin wrapper that does command-line argument parsing before
calling into sequencer_pick_revisions(). In the future, we can write
a new "foo" builtin that calls into the sequencer like:
memset(&opts, 0, sizeof(opts));
opts.command = REPLAY_CMD_FOO;
opts.revisions = xmalloc(sizeof(*opts.revs));
parse_args_populate_opts(argc, argv, &opts);
init_revisions(opts.revs);
sequencer_pick_revisions(&opts);
This patch does not intend to make any functional changes. Check
with:
$ git blame -s -C HEAD^..HEAD -- sequencer.c | grep -C3 '^[^^]'
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 1006 +-----------------------------------------------------
sequencer.c | 987 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
sequencer.h | 25 ++
3 files changed, 1013 insertions(+), 1005 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 35553e7..47f71f3 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -1,19 +1,9 @@
#include "cache.h"
#include "builtin.h"
-#include "object.h"
-#include "commit.h"
-#include "tag.h"
-#include "run-command.h"
-#include "exec_cmd.h"
-#include "utf8.h"
#include "parse-options.h"
-#include "cache-tree.h"
#include "diff.h"
#include "revision.h"
#include "rerere.h"
-#include "merge-recursive.h"
-#include "refs.h"
-#include "dir.h"
#include "sequencer.h"
/*
@@ -39,43 +29,11 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-struct replay_opts {
- enum replay_command command;
- enum replay_subcommand subcommand;
-
- /* Boolean options */
- int edit;
- int record_origin;
- int no_commit;
- int signoff;
- int allow_ff;
- int allow_rerere_auto;
-
- int mainline;
-
- /* Merge strategy */
- const char *strategy;
- const char **xopts;
- size_t xopts_nr, xopts_alloc;
-
- /* Only used by REPLAY_NONE */
- struct rev_info *revs;
-};
-
-#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-
static const char *command_name(struct replay_opts *opts)
{
return opts->command == REPLAY_CMD_REVERT ? "revert" : "cherry-pick";
}
-static const char *action_name(enum replay_action action)
-{
- return action == REPLAY_REVERT ? "revert" : "pick";
-}
-
-static char *get_encoding(const char *message);
-
static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
{
return opts->command == REPLAY_CMD_REVERT ? revert_usage : cherry_pick_usage;
@@ -234,966 +192,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
usage_with_options(usage_str, options);
}
-struct commit_message {
- char *parent_label;
- const char *label;
- const char *subject;
- char *reencoded_message;
- const char *message;
-};
-
-static int get_message(struct commit *commit, struct commit_message *out)
-{
- const char *encoding;
- const char *abbrev, *subject;
- int abbrev_len, subject_len;
- char *q;
-
- if (!commit->buffer)
- return -1;
- encoding = get_encoding(commit->buffer);
- if (!encoding)
- encoding = "UTF-8";
- if (!git_commit_encoding)
- git_commit_encoding = "UTF-8";
-
- out->reencoded_message = NULL;
- out->message = commit->buffer;
- if (strcmp(encoding, git_commit_encoding))
- out->reencoded_message = reencode_string(commit->buffer,
- git_commit_encoding, encoding);
- if (out->reencoded_message)
- out->message = out->reencoded_message;
-
- abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
- abbrev_len = strlen(abbrev);
-
- subject_len = find_commit_subject(out->message, &subject);
-
- out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
- strlen("... ") + subject_len + 1);
- q = out->parent_label;
- q = mempcpy(q, "parent of ", strlen("parent of "));
- out->label = q;
- q = mempcpy(q, abbrev, abbrev_len);
- q = mempcpy(q, "... ", strlen("... "));
- out->subject = q;
- q = mempcpy(q, subject, subject_len);
- *q = '\0';
- return 0;
-}
-
-static void free_message(struct commit_message *msg)
-{
- free(msg->parent_label);
- free(msg->reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
- const char *p = message, *eol;
-
- while (*p && *p != '\n') {
- for (eol = p + 1; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
- if (!prefixcmp(p, "encoding ")) {
- char *result = xmalloc(eol - 8 - p);
- strlcpy(result, p + 9, eol - 8 - p);
- return result;
- }
- p = eol;
- if (*p == '\n')
- p++;
- }
- return NULL;
-}
-
-static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
-{
- const char *filename;
- int fd;
- struct strbuf buf = STRBUF_INIT;
-
- strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
-
- filename = git_path("%s", pseudoref);
- fd = open(filename, O_WRONLY | O_CREAT, 0666);
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"), filename);
- if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
- die_errno(_("Could not write to '%s'"), filename);
- strbuf_release(&buf);
-}
-
-static void print_advice(int show_hint)
-{
- char *msg = getenv("GIT_CHERRY_PICK_HELP");
-
- if (msg) {
- fprintf(stderr, "%s\n", msg);
- /*
- * A conflict has occured but the porcelain
- * (typically rebase --interactive) wants to take care
- * of the commit itself so remove CHERRY_PICK_HEAD
- */
- unlink(git_path("CHERRY_PICK_HEAD"));
- return;
- }
-
- if (show_hint) {
- advise("after resolving the conflicts, mark the corrected paths");
- advise("with 'git add <paths>' or 'git rm <paths>'");
- advise("and commit the result with 'git commit'");
- }
-}
-
-static void write_message(struct strbuf *msgbuf, const char *filename)
-{
- static struct lock_file msg_file;
-
- int msg_fd = hold_lock_file_for_update(&msg_file, filename,
- LOCK_DIE_ON_ERROR);
- if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
- die_errno(_("Could not write to %s"), filename);
- strbuf_release(msgbuf);
- if (commit_lock_file(&msg_file) < 0)
- die(_("Error wrapping up %s"), filename);
-}
-
-static struct tree *empty_tree(void)
-{
- return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
-}
-
-static int error_dirty_index(struct replay_opts *opts)
-{
- if (read_cache_unmerged())
- return error_resolve_conflict(command_name(opts));
-
- /* Different translation strings for cherry-pick and revert */
- if (opts->command == REPLAY_CMD_CHERRY_PICK)
- error(_("Your local changes would be overwritten by cherry-pick."));
- else
- error(_("Your local changes would be overwritten by revert."));
-
- if (advice_commit_before_merge)
- advise(_("Commit your changes or stash them to proceed."));
- return -1;
-}
-
-static int fast_forward_to(const unsigned char *to, const unsigned char *from)
-{
- struct ref_lock *ref_lock;
-
- read_cache();
- if (checkout_fast_forward(from, to))
- exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update("HEAD", from, 0);
- return write_ref_sha1(ref_lock, to, "cherry-pick");
-}
-
-static int do_recursive_merge(struct commit *base, struct commit *next,
- const char *base_label, const char *next_label,
- unsigned char *head, struct strbuf *msgbuf,
- struct replay_opts *opts)
-{
- struct merge_options o;
- struct tree *result, *next_tree, *base_tree, *head_tree;
- int clean, index_fd;
- const char **xopt;
- static struct lock_file index_lock;
-
- index_fd = hold_locked_index(&index_lock, 1);
-
- read_cache();
-
- init_merge_options(&o);
- o.ancestor = base ? base_label : "(empty tree)";
- o.branch1 = "HEAD";
- o.branch2 = next ? next_label : "(empty tree)";
-
- head_tree = parse_tree_indirect(head);
- next_tree = next ? next->tree : empty_tree();
- base_tree = base ? base->tree : empty_tree();
-
- for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
- parse_merge_opt(&o, *xopt);
-
- clean = merge_trees(&o,
- head_tree,
- next_tree, base_tree, &result);
-
- if (active_cache_changed &&
- (write_cache(index_fd, active_cache, active_nr) ||
- commit_locked_index(&index_lock)))
- /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
- die(_("%s: Unable to write new index file"), command_name(opts));
- rollback_lock_file(&index_lock);
-
- if (!clean) {
- int i;
- strbuf_addstr(msgbuf, "\nConflicts:\n\n");
- for (i = 0; i < active_nr;) {
- struct cache_entry *ce = active_cache[i++];
- if (ce_stage(ce)) {
- strbuf_addch(msgbuf, '\t');
- strbuf_addstr(msgbuf, ce->name);
- strbuf_addch(msgbuf, '\n');
- while (i < active_nr && !strcmp(ce->name,
- active_cache[i]->name))
- i++;
- }
- }
- }
-
- return !clean;
-}
-
-/*
- * If we are cherry-pick, and if the merge did not result in
- * hand-editing, we will hit this commit and inherit the original
- * author date and name.
- * If we are revert, or if our cherry-pick results in a hand merge,
- * we had better say that the current user is responsible for that.
- */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
-{
- /* 6 is max possible length of our args array including NULL */
- const char *args[6];
- int i = 0;
-
- args[i++] = "commit";
- args[i++] = "-n";
- if (opts->signoff)
- args[i++] = "-s";
- if (!opts->edit) {
- args[i++] = "-F";
- args[i++] = defmsg;
- }
- args[i] = NULL;
-
- return run_command_v_opt(args, RUN_GIT_CMD);
-}
-
-static int do_pick_commit(struct commit *commit, enum replay_action action,
- struct replay_opts *opts)
-{
- unsigned char head[20];
- struct commit *base, *next, *parent;
- const char *base_label, *next_label;
- struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
- char *defmsg = NULL;
- struct strbuf msgbuf = STRBUF_INIT;
- int res;
-
- if (opts->no_commit) {
- /*
- * We do not intend to commit immediately. We just want to
- * merge the differences in, so let's compute the tree
- * that represents the "current" state for merge-recursive
- * to work on.
- */
- if (write_cache_as_tree(head, 0, NULL))
- die (_("Your index file is unmerged."));
- } else {
- if (get_sha1("HEAD", head))
- return error(_("You do not have a valid HEAD"));
- if (index_differs_from("HEAD", 0))
- return error_dirty_index(opts);
- }
- discard_cache();
-
- if (!commit->parents) {
- parent = NULL;
- }
- else if (commit->parents->next) {
- /* Reverting or cherry-picking a merge commit */
- int cnt;
- struct commit_list *p;
-
- if (!opts->mainline)
- return error(_("Commit %s is a merge but no -m option was given."),
- sha1_to_hex(commit->object.sha1));
-
- for (cnt = 1, p = commit->parents;
- cnt != opts->mainline && p;
- cnt++)
- p = p->next;
- if (cnt != opts->mainline || !p)
- return error(_("Commit %s does not have parent %d"),
- sha1_to_hex(commit->object.sha1), opts->mainline);
- parent = p->item;
- } else if (0 < opts->mainline)
- return error(_("Mainline was specified but commit %s is not a merge."),
- sha1_to_hex(commit->object.sha1));
- else
- parent = commit->parents->item;
-
- if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
- return fast_forward_to(commit->object.sha1, head);
-
- if (parent && parse_commit(parent) < 0)
- /* TRANSLATORS: The first %s will be "revert" or
- "cherry-pick", the second %s a SHA1 */
- return error(_("%s: cannot parse parent commit %s"),
- command_name(opts), sha1_to_hex(parent->object.sha1));
-
- if (get_message(commit, &msg) != 0)
- return error(_("Cannot get commit message for %s"),
- sha1_to_hex(commit->object.sha1));
-
- /*
- * "commit" is an existing commit. We would want to apply
- * the difference it introduces since its first parent "prev"
- * on top of the current HEAD if we are cherry-pick. Or the
- * reverse of it if we are revert.
- */
-
- defmsg = git_pathdup("MERGE_MSG");
-
- if (action == REPLAY_REVERT) {
- base = commit;
- base_label = msg.label;
- next = parent;
- next_label = msg.parent_label;
- strbuf_addstr(&msgbuf, "Revert \"");
- strbuf_addstr(&msgbuf, msg.subject);
- strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
- strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-
- if (commit->parents && commit->parents->next) {
- strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
- strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
- }
- strbuf_addstr(&msgbuf, ".\n");
- } else {
- const char *p;
-
- base = parent;
- base_label = msg.parent_label;
- next = commit;
- next_label = msg.label;
-
- /*
- * Append the commit log message to msgbuf; it starts
- * after the tree, parent, author, committer
- * information followed by "\n\n".
- */
- p = strstr(msg.message, "\n\n");
- if (p) {
- p += 2;
- strbuf_addstr(&msgbuf, p);
- }
-
- if (opts->record_origin) {
- strbuf_addstr(&msgbuf, "(cherry picked from commit ");
- strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
- strbuf_addstr(&msgbuf, ")\n");
- }
- }
-
- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
- res = do_recursive_merge(base, next, base_label, next_label,
- head, &msgbuf, opts);
- write_message(&msgbuf, defmsg);
- } else {
- struct commit_list *common = NULL;
- struct commit_list *remotes = NULL;
-
- write_message(&msgbuf, defmsg);
-
- commit_list_insert(base, &common);
- commit_list_insert(next, &remotes);
- res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
- common, sha1_to_hex(head), remotes);
- free_commit_list(common);
- free_commit_list(remotes);
- }
-
- /*
- * If the merge was clean or if it failed due to conflict, we write
- * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
- * However, if the merge did not even start, then we don't want to
- * write it at all.
- */
- if (action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
- write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
- if (action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
- write_cherry_pick_head(commit, "REVERT_HEAD");
-
- if (res) {
- error(action == REPLAY_REVERT
- ? _("could not revert %s... %s")
- : _("could not apply %s... %s"),
- find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
- msg.subject);
- print_advice(res == 1);
- rerere(opts->allow_rerere_auto);
- } else {
- if (!opts->no_commit)
- res = run_git_commit(defmsg, opts);
- }
-
- free_message(&msg);
- free(defmsg);
-
- return res;
-}
-
-static void prepare_revs(struct replay_opts *opts)
-{
- if (opts->command != REPLAY_CMD_REVERT)
- opts->revs->reverse ^= 1;
-
- if (prepare_revision_walk(opts->revs))
- die(_("revision walk setup failed"));
-
- if (!opts->revs->commits)
- die(_("empty commit set passed"));
-}
-
-static void read_and_refresh_cache(struct replay_opts *opts)
-{
- static struct lock_file index_lock;
- int index_fd = hold_locked_index(&index_lock, 0);
- if (read_index_preload(&the_index, NULL) < 0)
- die(_("git %s: failed to read the index"), command_name(opts));
- refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
- if (the_index.cache_changed) {
- if (write_index(&the_index, index_fd) ||
- commit_locked_index(&index_lock))
- die(_("git %s: failed to refresh the index"),
- command_name(opts));
- }
- rollback_lock_file(&index_lock);
-}
-
-/*
- * Append a (commit, action) to the end of the replay_insn_list.
- *
- * next starts by pointing to the variable that holds the head of an
- * empty replay_insn_list, and is updated to point to the "next" field of
- * the last item on the list as new (commit, action) pairs are appended.
- *
- * Usage example:
- *
- * struct replay_insn_list *list;
- * struct replay_insn_list **next = &list;
- *
- * next = replay_insn_list_append(c1, a1, next);
- * next = replay_insn_list_append(c2, a2, next);
- * assert(len(list) == 2);
- * return list;
- */
-static struct replay_insn_list **replay_insn_list_append(struct commit *operand,
- enum replay_action action,
- struct replay_insn_list **next)
-{
- struct replay_insn_list *new = xmalloc(sizeof(*new));
- new->action = action;
- new->operand = operand;
- *next = new;
- new->next = NULL;
- return &new->next;
-}
-
-static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
-{
- struct replay_insn_list *cur;
-
- for (cur = todo_list; cur; cur = cur->next) {
- const char *sha1_abbrev, *action_str, *subject;
- int subject_len;
-
- action_str = action_name(cur->action);
- sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
- subject_len = find_commit_subject(cur->operand->buffer, &subject);
- strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
- subject_len, subject);
- }
- return 0;
-}
-
-static int parse_insn_line(char *bol, char *eol,
- struct replay_insn_list *item, int lineno)
-{
- const char *todo_file = git_path(SEQ_TODO_FILE);
- unsigned char commit_sha1[20];
- char *end_of_object_name;
- int saved, status;
- size_t error_len;
-
- if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
- item->action = REPLAY_PICK;
- bol += strlen("pick ");
- } else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
- item->action = REPLAY_REVERT;
- bol += strlen("revert ");
- } else {
- error_len = eol - bol > 255 ? 255 : eol - bol;
- return error(_("%s:%d: Unrecognized action: %.*s"),
- todo_file, lineno, (int)error_len, bol);
- }
-
- /* Eat up extra spaces/ tabs before object name */
- bol += strspn(bol, " \t");
-
- end_of_object_name = bol + strcspn(bol, " \t\n");
- saved = *end_of_object_name;
- *end_of_object_name = '\0';
- status = get_sha1(bol, commit_sha1);
- *end_of_object_name = saved;
-
- if (status < 0) {
- error_len = eol - bol > 255 ? 255 : eol - bol;
- return error(_("%s:%d: Malformed object name: %.*s"),
- todo_file, lineno, (int)error_len, bol);
- }
-
- item->operand = lookup_commit_reference(commit_sha1);
- if (!item->operand) {
- error_len = eol - bol > 255 ? 255 : eol - bol;
- return error(_("%s:%d: Not a valid commit: %.*s"),
- todo_file, lineno, (int)error_len, bol);
- }
-
- item->next = NULL;
- return 0;
-}
-
-static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
-{
- struct replay_insn_list **next = todo_list;
- struct replay_insn_list item = { NULL, 0, NULL };
- char *p = buf;
- int i;
-
- for (i = 1; *p; i++) {
- char *eol = strchrnul(p, '\n');
- if (parse_insn_line(p, eol, &item, i) < 0)
- return -1;
- next = replay_insn_list_append(item.operand, item.action, next);
- p = *eol ? eol + 1 : eol;
- }
- if (!*todo_list)
- return error(_("No commits parsed."));
- return 0;
-}
-
-static void read_populate_todo(struct replay_insn_list **todo_list)
-{
- const char *todo_file = git_path(SEQ_TODO_FILE);
- struct strbuf buf = STRBUF_INIT;
- int fd, res;
-
- fd = open(todo_file, O_RDONLY);
- if (fd < 0)
- die_errno(_("Could not open %s"), todo_file);
- if (strbuf_read(&buf, fd, 0) < 0) {
- close(fd);
- strbuf_release(&buf);
- die(_("Could not read %s."), todo_file);
- }
- close(fd);
-
- res = parse_insn_buffer(buf.buf, todo_list);
- strbuf_release(&buf);
- if (res)
- die(_("Unusable instruction sheet: %s"), todo_file);
-}
-
-static int populate_opts_cb(const char *key, const char *value, void *data)
-{
- struct replay_opts *opts = data;
- int error_flag = 1;
-
- if (!value)
- error_flag = 0;
- else if (!strcmp(key, "options.no-commit"))
- opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.edit"))
- opts->edit = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.signoff"))
- opts->signoff = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.record-origin"))
- opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.allow-ff"))
- opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
- else if (!strcmp(key, "options.mainline"))
- opts->mainline = git_config_int(key, value);
- else if (!strcmp(key, "options.strategy"))
- git_config_string(&opts->strategy, key, value);
- else if (!strcmp(key, "options.strategy-option")) {
- ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
- opts->xopts[opts->xopts_nr++] = xstrdup(value);
- } else
- return error(_("Invalid key: %s"), key);
-
- if (!error_flag)
- return error(_("Invalid value for %s: %s"), key, value);
-
- return 0;
-}
-
-static void read_populate_opts(struct replay_opts **opts_ptr)
-{
- const char *opts_file = git_path(SEQ_OPTS_FILE);
-
- if (!file_exists(opts_file))
- return;
- if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
- die(_("Malformed options sheet: %s"), opts_file);
-}
-
-static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
- struct replay_opts *opts)
-{
- struct commit *commit;
- struct replay_insn_list **next;
- enum replay_action action;
-
- prepare_revs(opts);
-
- next = todo_list;
- action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
- while ((commit = get_revision(opts->revs)))
- next = replay_insn_list_append(commit, action, next);
-}
-
-static int create_seq_dir(void)
-{
- const char *seq_dir = git_path(SEQ_DIR);
-
- if (file_exists(seq_dir)) {
- error(_("a cherry-pick or revert is already in progress"));
- advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
- return -1;
- }
- else if (mkdir(seq_dir, 0777) < 0)
- die_errno(_("Could not create sequencer directory %s"), seq_dir);
- return 0;
-}
-
-static enum replay_command read_cmd(void)
-{
- const char *cmd_file = git_path(SEQ_CMD_FILE);
- struct strbuf buf = STRBUF_INIT;
- enum replay_command res;
- int fd;
-
- fd = open(cmd_file, O_RDONLY);
- if (fd < 0)
- die_errno(_("Could not open %s"), cmd_file);
- if (strbuf_read(&buf, fd, 0) < 0) {
- close(fd);
- strbuf_release(&buf);
- die(_("Could not read %s."), cmd_file);
- }
- close(fd);
-
- if (!strcmp(buf.buf, "revert\n"))
- res = REPLAY_CMD_REVERT;
- else if (!strcmp(buf.buf, "cherry-pick\n"))
- res = REPLAY_CMD_CHERRY_PICK;
- else {
- strbuf_release(&buf);
- die(_("Malformed command file: %s"), cmd_file);
- }
- strbuf_release(&buf);
- return res;
-}
-
-static void save_cmd(struct replay_opts *opts)
-{
- const char *cmd_file = git_path(SEQ_CMD_FILE);
- static struct lock_file cmd_lock;
- struct strbuf buf = STRBUF_INIT;
- int fd;
-
- fd = hold_lock_file_for_update(&cmd_lock, cmd_file, LOCK_DIE_ON_ERROR);
- strbuf_addf(&buf, "%s\n", command_name(opts));
- if (write_in_full(fd, buf.buf, buf.len) < 0)
- die_errno(_("Could not write to %s"), cmd_file);
- if (commit_lock_file(&cmd_lock) < 0)
- die(_("Error wrapping up %s."), cmd_file);
-}
-
-static void save_head(const char *head)
-{
- const char *head_file = git_path(SEQ_HEAD_FILE);
- static struct lock_file head_lock;
- struct strbuf buf = STRBUF_INIT;
- int fd;
-
- fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
- strbuf_addf(&buf, "%s\n", head);
- if (write_in_full(fd, buf.buf, buf.len) < 0)
- die_errno(_("Could not write to %s"), head_file);
- if (commit_lock_file(&head_lock) < 0)
- die(_("Error wrapping up %s."), head_file);
-}
-
-static int reset_for_rollback(const unsigned char *sha1)
-{
- const char *argv[4]; /* reset --merge <arg> + NULL */
- argv[0] = "reset";
- argv[1] = "--merge";
- argv[2] = sha1_to_hex(sha1);
- argv[3] = NULL;
- return run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-static int rollback_single_pick(void)
-{
- unsigned char head_sha1[20];
-
- if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
- !file_exists(git_path("REVERT_HEAD")))
- return error(_("no cherry-pick or revert in progress"));
- if (read_ref_full("HEAD", head_sha1, 0, NULL))
- return error(_("cannot resolve HEAD"));
- if (is_null_sha1(head_sha1))
- return error(_("cannot abort from a branch yet to be born"));
- return reset_for_rollback(head_sha1);
-}
-
-static int sequencer_rollback(struct replay_opts *opts)
-{
- const char *filename;
- FILE *f;
- unsigned char sha1[20];
- struct strbuf buf = STRBUF_INIT;
-
- filename = git_path(SEQ_HEAD_FILE);
- f = fopen(filename, "r");
- if (!f && errno == ENOENT) {
- /*
- * There is no multiple-cherry-pick in progress.
- * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
- * a single-cherry-pick in progress, abort that.
- */
- return rollback_single_pick();
- }
- if (!f)
- return error(_("cannot open %s: %s"), filename,
- strerror(errno));
- if (strbuf_getline(&buf, f, '\n')) {
- error(_("cannot read %s: %s"), filename, ferror(f) ?
- strerror(errno) : _("unexpected end of file"));
- fclose(f);
- goto fail;
- }
- fclose(f);
- if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
- error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
- filename);
- goto fail;
- }
- if (reset_for_rollback(sha1))
- goto fail;
- remove_sequencer_state();
- strbuf_release(&buf);
- return 0;
-fail:
- strbuf_release(&buf);
- return -1;
-}
-
-static void save_todo(struct replay_insn_list *todo_list)
-{
- const char *todo_file = git_path(SEQ_TODO_FILE);
- static struct lock_file todo_lock;
- struct strbuf buf = STRBUF_INIT;
- int fd;
-
- fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
- if (format_todo(&buf, todo_list) < 0)
- die(_("Could not format %s."), todo_file);
- if (write_in_full(fd, buf.buf, buf.len) < 0) {
- strbuf_release(&buf);
- die_errno(_("Could not write to %s"), todo_file);
- }
- if (commit_lock_file(&todo_lock) < 0) {
- strbuf_release(&buf);
- die(_("Error wrapping up %s."), todo_file);
- }
- strbuf_release(&buf);
-}
-
-static void save_opts(struct replay_opts *opts)
-{
- const char *opts_file = git_path(SEQ_OPTS_FILE);
-
- if (opts->no_commit)
- git_config_set_in_file(opts_file, "options.no-commit", "true");
- if (opts->edit)
- git_config_set_in_file(opts_file, "options.edit", "true");
- if (opts->signoff)
- git_config_set_in_file(opts_file, "options.signoff", "true");
- if (opts->record_origin)
- git_config_set_in_file(opts_file, "options.record-origin", "true");
- if (opts->allow_ff)
- git_config_set_in_file(opts_file, "options.allow-ff", "true");
- if (opts->mainline) {
- struct strbuf buf = STRBUF_INIT;
- strbuf_addf(&buf, "%d", opts->mainline);
- git_config_set_in_file(opts_file, "options.mainline", buf.buf);
- strbuf_release(&buf);
- }
- if (opts->strategy)
- git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
- if (opts->xopts) {
- int i;
- for (i = 0; i < opts->xopts_nr; i++)
- git_config_set_multivar_in_file(opts_file,
- "options.strategy-option",
- opts->xopts[i], "^$", 0);
- }
-}
-
-static int pick_commits(struct replay_insn_list *todo_list, struct replay_opts *opts)
-{
- struct replay_insn_list *cur;
- int res;
-
- setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
- if (opts->allow_ff)
- assert(!(opts->signoff || opts->no_commit ||
- opts->record_origin || opts->edit));
- read_and_refresh_cache(opts);
-
- for (cur = todo_list; cur; cur = cur->next) {
- save_todo(cur);
- res = do_pick_commit(cur->operand, cur->action, opts);
- if (res)
- return res;
- }
-
- /*
- * Sequence of picks finished successfully; cleanup by
- * removing the .git/sequencer directory
- */
- remove_sequencer_state();
- return 0;
-}
-
-static int continue_single_pick(void)
-{
- const char *argv[] = { "commit", NULL };
-
- if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
- !file_exists(git_path("REVERT_HEAD")))
- return error(_("no cherry-pick or revert in progress"));
- return run_command_v_opt(argv, RUN_GIT_CMD);
-}
-
-static int sequencer_continue(struct replay_opts *opts)
-{
- struct replay_insn_list *todo_list = NULL;
- enum replay_command cmd;
-
- if (!file_exists(git_path(SEQ_TODO_FILE)))
- return continue_single_pick();
-
- /*
- * Disallow continuing a cherry-pick with 'git revert
- * --continue' and viceversa
- */
- cmd = read_cmd();
- if (cmd != opts->command)
- return error(_("cannot %s: a %s is in progress."),
- command_name(opts),
- cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
-
- read_populate_opts(&opts);
- read_populate_todo(&todo_list);
-
- /* Verify that the conflict has been resolved */
- if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
- file_exists(git_path("REVERT_HEAD"))) {
- int ret = continue_single_pick();
- if (ret)
- return ret;
- }
- if (index_differs_from("HEAD", 0))
- return error_dirty_index(opts);
- todo_list = todo_list->next;
- return pick_commits(todo_list, opts);
-}
-
-static int single_pick(struct commit *cmit, struct replay_opts *opts)
-{
- enum replay_action action;
- action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
-
- setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
- return do_pick_commit(cmit, action, opts);
-}
-
-static int pick_revisions(struct replay_opts *opts)
-{
- struct replay_insn_list *todo_list = NULL;
- unsigned char sha1[20];
-
- if (opts->subcommand == REPLAY_NONE)
- assert(opts->revs);
-
- read_and_refresh_cache(opts);
-
- /*
- * Decide what to do depending on the arguments; a fresh
- * cherry-pick should be handled differently from an existing
- * one that is being continued
- */
- if (opts->subcommand == REPLAY_REMOVE_STATE) {
- remove_sequencer_state();
- return 0;
- }
- if (opts->subcommand == REPLAY_ROLLBACK)
- return sequencer_rollback(opts);
- if (opts->subcommand == REPLAY_CONTINUE)
- return sequencer_continue(opts);
-
- /*
- * If we were called as "git cherry-pick <commit>", just
- * cherry-pick/revert it, set CHERRY_PICK_HEAD /
- * REVERT_HEAD, and don't touch the sequencer state.
- * This means it is possible to cherry-pick in the middle
- * of a cherry-pick sequence.
- */
- if (opts->revs->cmdline.nr == 1 &&
- opts->revs->cmdline.rev->whence == REV_CMD_REV &&
- opts->revs->no_walk &&
- !opts->revs->cmdline.rev->flags) {
- struct commit *cmit;
- if (prepare_revision_walk(opts->revs))
- die(_("revision walk setup failed"));
- cmit = get_revision(opts->revs);
- if (!cmit || get_revision(opts->revs))
- die("BUG: expected exactly one commit from walk");
- return single_pick(cmit, opts);
- }
-
- /*
- * Start a new cherry-pick/ revert sequence; but
- * first, make sure that an existing one isn't in
- * progress
- */
-
- walk_revs_populate_todo(&todo_list, opts);
- if (create_seq_dir() < 0)
- return -1;
- if (get_sha1("HEAD", sha1)) {
- if (opts->command == REPLAY_CMD_REVERT)
- return error(_("Can't revert as initial commit"));
- return error(_("Can't cherry-pick into empty head"));
- }
- save_cmd(opts);
- save_head(sha1_to_hex(sha1));
- save_opts(opts);
- return pick_commits(todo_list, opts);
-}
-
int cmd_revert(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts;
@@ -1205,7 +203,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
opts.command = REPLAY_CMD_REVERT;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
- res = pick_revisions(&opts);
+ res = sequencer_pick_revisions(&opts);
if (res < 0)
die(_("revert failed"));
return res;
@@ -1220,7 +218,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
opts.command = REPLAY_CMD_CHERRY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
- res = pick_revisions(&opts);
+ res = sequencer_pick_revisions(&opts);
if (res < 0)
die(_("cherry-pick failed"));
return res;
diff --git a/sequencer.c b/sequencer.c
index d1f28a6..a7e494c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1,7 +1,20 @@
#include "cache.h"
#include "sequencer.h"
-#include "strbuf.h"
#include "dir.h"
+#include "object.h"
+#include "commit.h"
+#include "tag.h"
+#include "run-command.h"
+#include "exec_cmd.h"
+#include "utf8.h"
+#include "cache-tree.h"
+#include "diff.h"
+#include "revision.h"
+#include "rerere.h"
+#include "merge-recursive.h"
+#include "refs.h"
+
+#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
void remove_sequencer_state(void)
{
@@ -11,3 +24,975 @@ void remove_sequencer_state(void)
remove_dir_recursively(&seq_dir, 0);
strbuf_release(&seq_dir);
}
+
+static const char *command_name(struct replay_opts *opts)
+{
+ return opts->command == REPLAY_CMD_REVERT ? "revert" : "cherry-pick";
+}
+
+static const char *action_name(enum replay_action action)
+{
+ return action == REPLAY_REVERT ? "revert" : "pick";
+}
+
+struct commit_message {
+ char *parent_label;
+ const char *label;
+ const char *subject;
+ char *reencoded_message;
+ const char *message;
+};
+
+static char *get_encoding(const char *message);
+
+static int get_message(struct commit *commit, struct commit_message *out)
+{
+ const char *encoding;
+ const char *abbrev, *subject;
+ int abbrev_len, subject_len;
+ char *q;
+
+ if (!commit->buffer)
+ return -1;
+ encoding = get_encoding(commit->buffer);
+ if (!encoding)
+ encoding = "UTF-8";
+ if (!git_commit_encoding)
+ git_commit_encoding = "UTF-8";
+
+ out->reencoded_message = NULL;
+ out->message = commit->buffer;
+ if (strcmp(encoding, git_commit_encoding))
+ out->reencoded_message = reencode_string(commit->buffer,
+ git_commit_encoding, encoding);
+ if (out->reencoded_message)
+ out->message = out->reencoded_message;
+
+ abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+ abbrev_len = strlen(abbrev);
+
+ subject_len = find_commit_subject(out->message, &subject);
+
+ out->parent_label = xmalloc(strlen("parent of ") + abbrev_len +
+ strlen("... ") + subject_len + 1);
+ q = out->parent_label;
+ q = mempcpy(q, "parent of ", strlen("parent of "));
+ out->label = q;
+ q = mempcpy(q, abbrev, abbrev_len);
+ q = mempcpy(q, "... ", strlen("... "));
+ out->subject = q;
+ q = mempcpy(q, subject, subject_len);
+ *q = '\0';
+ return 0;
+}
+
+static void free_message(struct commit_message *msg)
+{
+ free(msg->parent_label);
+ free(msg->reencoded_message);
+}
+
+static char *get_encoding(const char *message)
+{
+ const char *p = message, *eol;
+
+ while (*p && *p != '\n') {
+ for (eol = p + 1; *eol && *eol != '\n'; eol++)
+ ; /* do nothing */
+ if (!prefixcmp(p, "encoding ")) {
+ char *result = xmalloc(eol - 8 - p);
+ strlcpy(result, p + 9, eol - 8 - p);
+ return result;
+ }
+ p = eol;
+ if (*p == '\n')
+ p++;
+ }
+ return NULL;
+}
+
+static void write_cherry_pick_head(struct commit *commit, const char *pseudoref)
+{
+ const char *filename;
+ int fd;
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addf(&buf, "%s\n", sha1_to_hex(commit->object.sha1));
+
+ filename = git_path("%s", pseudoref);
+ fd = open(filename, O_WRONLY | O_CREAT, 0666);
+ if (fd < 0)
+ die_errno(_("Could not open '%s' for writing"), filename);
+ if (write_in_full(fd, buf.buf, buf.len) != buf.len || close(fd))
+ die_errno(_("Could not write to '%s'"), filename);
+ strbuf_release(&buf);
+}
+
+static void print_advice(int show_hint)
+{
+ char *msg = getenv("GIT_CHERRY_PICK_HELP");
+
+ if (msg) {
+ fprintf(stderr, "%s\n", msg);
+ /*
+ * A conflict has occured but the porcelain
+ * (typically rebase --interactive) wants to take care
+ * of the commit itself so remove CHERRY_PICK_HEAD
+ */
+ unlink(git_path("CHERRY_PICK_HEAD"));
+ return;
+ }
+
+ if (show_hint) {
+ advise("after resolving the conflicts, mark the corrected paths");
+ advise("with 'git add <paths>' or 'git rm <paths>'");
+ advise("and commit the result with 'git commit'");
+ }
+}
+
+static void write_message(struct strbuf *msgbuf, const char *filename)
+{
+ static struct lock_file msg_file;
+
+ int msg_fd = hold_lock_file_for_update(&msg_file, filename,
+ LOCK_DIE_ON_ERROR);
+ if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+ die_errno(_("Could not write to %s"), filename);
+ strbuf_release(msgbuf);
+ if (commit_lock_file(&msg_file) < 0)
+ die(_("Error wrapping up %s"), filename);
+}
+
+static struct tree *empty_tree(void)
+{
+ return lookup_tree((const unsigned char *)EMPTY_TREE_SHA1_BIN);
+}
+
+static int error_dirty_index(struct replay_opts *opts)
+{
+ if (read_cache_unmerged())
+ return error_resolve_conflict(command_name(opts));
+
+ /* Different translation strings for cherry-pick and revert */
+ if (opts->command == REPLAY_CMD_CHERRY_PICK)
+ error(_("Your local changes would be overwritten by cherry-pick."));
+ else
+ error(_("Your local changes would be overwritten by revert."));
+
+ if (advice_commit_before_merge)
+ advise(_("Commit your changes or stash them to proceed."));
+ return -1;
+}
+
+static int fast_forward_to(const unsigned char *to, const unsigned char *from)
+{
+ struct ref_lock *ref_lock;
+
+ read_cache();
+ if (checkout_fast_forward(from, to))
+ exit(1); /* the callee should have complained already */
+ ref_lock = lock_any_ref_for_update("HEAD", from, 0);
+ return write_ref_sha1(ref_lock, to, "cherry-pick");
+}
+
+static int do_recursive_merge(struct commit *base, struct commit *next,
+ const char *base_label, const char *next_label,
+ unsigned char *head, struct strbuf *msgbuf,
+ struct replay_opts *opts)
+{
+ struct merge_options o;
+ struct tree *result, *next_tree, *base_tree, *head_tree;
+ int clean, index_fd;
+ const char **xopt;
+ static struct lock_file index_lock;
+
+ index_fd = hold_locked_index(&index_lock, 1);
+
+ read_cache();
+
+ init_merge_options(&o);
+ o.ancestor = base ? base_label : "(empty tree)";
+ o.branch1 = "HEAD";
+ o.branch2 = next ? next_label : "(empty tree)";
+
+ head_tree = parse_tree_indirect(head);
+ next_tree = next ? next->tree : empty_tree();
+ base_tree = base ? base->tree : empty_tree();
+
+ for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
+ parse_merge_opt(&o, *xopt);
+
+ clean = merge_trees(&o,
+ head_tree,
+ next_tree, base_tree, &result);
+
+ if (active_cache_changed &&
+ (write_cache(index_fd, active_cache, active_nr) ||
+ commit_locked_index(&index_lock)))
+ /* TRANSLATORS: %s will be "revert" or "cherry-pick" */
+ die(_("%s: Unable to write new index file"), command_name(opts));
+ rollback_lock_file(&index_lock);
+
+ if (!clean) {
+ int i;
+ strbuf_addstr(msgbuf, "\nConflicts:\n\n");
+ for (i = 0; i < active_nr;) {
+ struct cache_entry *ce = active_cache[i++];
+ if (ce_stage(ce)) {
+ strbuf_addch(msgbuf, '\t');
+ strbuf_addstr(msgbuf, ce->name);
+ strbuf_addch(msgbuf, '\n');
+ while (i < active_nr && !strcmp(ce->name,
+ active_cache[i]->name))
+ i++;
+ }
+ }
+ }
+
+ return !clean;
+}
+
+/*
+ * If we are cherry-pick, and if the merge did not result in
+ * hand-editing, we will hit this commit and inherit the original
+ * author date and name.
+ * If we are revert, or if our cherry-pick results in a hand merge,
+ * we had better say that the current user is responsible for that.
+ */
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+{
+ /* 6 is max possible length of our args array including NULL */
+ const char *args[6];
+ int i = 0;
+
+ args[i++] = "commit";
+ args[i++] = "-n";
+ if (opts->signoff)
+ args[i++] = "-s";
+ if (!opts->edit) {
+ args[i++] = "-F";
+ args[i++] = defmsg;
+ }
+ args[i] = NULL;
+
+ return run_command_v_opt(args, RUN_GIT_CMD);
+}
+
+static int do_pick_commit(struct commit *commit, enum replay_action action,
+ struct replay_opts *opts)
+{
+ unsigned char head[20];
+ struct commit *base, *next, *parent;
+ const char *base_label, *next_label;
+ struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+ char *defmsg = NULL;
+ struct strbuf msgbuf = STRBUF_INIT;
+ int res;
+
+ if (opts->no_commit) {
+ /*
+ * We do not intend to commit immediately. We just want to
+ * merge the differences in, so let's compute the tree
+ * that represents the "current" state for merge-recursive
+ * to work on.
+ */
+ if (write_cache_as_tree(head, 0, NULL))
+ die (_("Your index file is unmerged."));
+ } else {
+ if (get_sha1("HEAD", head))
+ return error(_("You do not have a valid HEAD"));
+ if (index_differs_from("HEAD", 0))
+ return error_dirty_index(opts);
+ }
+ discard_cache();
+
+ if (!commit->parents) {
+ parent = NULL;
+ }
+ else if (commit->parents->next) {
+ /* Reverting or cherry-picking a merge commit */
+ int cnt;
+ struct commit_list *p;
+
+ if (!opts->mainline)
+ return error(_("Commit %s is a merge but no -m option was given."),
+ sha1_to_hex(commit->object.sha1));
+
+ for (cnt = 1, p = commit->parents;
+ cnt != opts->mainline && p;
+ cnt++)
+ p = p->next;
+ if (cnt != opts->mainline || !p)
+ return error(_("Commit %s does not have parent %d"),
+ sha1_to_hex(commit->object.sha1), opts->mainline);
+ parent = p->item;
+ } else if (0 < opts->mainline)
+ return error(_("Mainline was specified but commit %s is not a merge."),
+ sha1_to_hex(commit->object.sha1));
+ else
+ parent = commit->parents->item;
+
+ if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
+ return fast_forward_to(commit->object.sha1, head);
+
+ if (parent && parse_commit(parent) < 0)
+ /* TRANSLATORS: The first %s will be "revert" or
+ "cherry-pick", the second %s a SHA1 */
+ return error(_("%s: cannot parse parent commit %s"),
+ command_name(opts), sha1_to_hex(parent->object.sha1));
+
+ if (get_message(commit, &msg) != 0)
+ return error(_("Cannot get commit message for %s"),
+ sha1_to_hex(commit->object.sha1));
+
+ /*
+ * "commit" is an existing commit. We would want to apply
+ * the difference it introduces since its first parent "prev"
+ * on top of the current HEAD if we are cherry-pick. Or the
+ * reverse of it if we are revert.
+ */
+
+ defmsg = git_pathdup("MERGE_MSG");
+
+ if (action == REPLAY_REVERT) {
+ base = commit;
+ base_label = msg.label;
+ next = parent;
+ next_label = msg.parent_label;
+ strbuf_addstr(&msgbuf, "Revert \"");
+ strbuf_addstr(&msgbuf, msg.subject);
+ strbuf_addstr(&msgbuf, "\"\n\nThis reverts commit ");
+ strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+
+ if (commit->parents && commit->parents->next) {
+ strbuf_addstr(&msgbuf, ", reversing\nchanges made to ");
+ strbuf_addstr(&msgbuf, sha1_to_hex(parent->object.sha1));
+ }
+ strbuf_addstr(&msgbuf, ".\n");
+ } else {
+ const char *p;
+
+ base = parent;
+ base_label = msg.parent_label;
+ next = commit;
+ next_label = msg.label;
+
+ /*
+ * Append the commit log message to msgbuf; it starts
+ * after the tree, parent, author, committer
+ * information followed by "\n\n".
+ */
+ p = strstr(msg.message, "\n\n");
+ if (p) {
+ p += 2;
+ strbuf_addstr(&msgbuf, p);
+ }
+
+ if (opts->record_origin) {
+ strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+ strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
+ strbuf_addstr(&msgbuf, ")\n");
+ }
+ }
+
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) {
+ res = do_recursive_merge(base, next, base_label, next_label,
+ head, &msgbuf, opts);
+ write_message(&msgbuf, defmsg);
+ } else {
+ struct commit_list *common = NULL;
+ struct commit_list *remotes = NULL;
+
+ write_message(&msgbuf, defmsg);
+
+ commit_list_insert(base, &common);
+ commit_list_insert(next, &remotes);
+ res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+ common, sha1_to_hex(head), remotes);
+ free_commit_list(common);
+ free_commit_list(remotes);
+ }
+
+ /*
+ * If the merge was clean or if it failed due to conflict, we write
+ * CHERRY_PICK_HEAD for the subsequent invocation of commit to use.
+ * However, if the merge did not even start, then we don't want to
+ * write it at all.
+ */
+ if (action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1))
+ write_cherry_pick_head(commit, "CHERRY_PICK_HEAD");
+ if (action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1))
+ write_cherry_pick_head(commit, "REVERT_HEAD");
+
+ if (res) {
+ error(action == REPLAY_REVERT
+ ? _("could not revert %s... %s")
+ : _("could not apply %s... %s"),
+ find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+ msg.subject);
+ print_advice(res == 1);
+ rerere(opts->allow_rerere_auto);
+ } else {
+ if (!opts->no_commit)
+ res = run_git_commit(defmsg, opts);
+ }
+
+ free_message(&msg);
+ free(defmsg);
+
+ return res;
+}
+
+static void prepare_revs(struct replay_opts *opts)
+{
+ if (opts->command != REPLAY_CMD_REVERT)
+ opts->revs->reverse ^= 1;
+
+ if (prepare_revision_walk(opts->revs))
+ die(_("revision walk setup failed"));
+
+ if (!opts->revs->commits)
+ die(_("empty commit set passed"));
+}
+
+static void read_and_refresh_cache(struct replay_opts *opts)
+{
+ static struct lock_file index_lock;
+ int index_fd = hold_locked_index(&index_lock, 0);
+ if (read_index_preload(&the_index, NULL) < 0)
+ die(_("git %s: failed to read the index"), command_name(opts));
+ refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
+ if (the_index.cache_changed) {
+ if (write_index(&the_index, index_fd) ||
+ commit_locked_index(&index_lock))
+ die(_("git %s: failed to refresh the index"),
+ command_name(opts));
+ }
+ rollback_lock_file(&index_lock);
+}
+
+/*
+ * Append a (commit, action) to the end of the replay_insn_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty replay_insn_list, and is updated to point to the "next" field of
+ * the last item on the list as new (commit, action) pairs are appended.
+ *
+ * Usage example:
+ *
+ * struct replay_insn_list *list;
+ * struct replay_insn_list **next = &list;
+ *
+ * next = replay_insn_list_append(c1, a1, next);
+ * next = replay_insn_list_append(c2, a2, next);
+ * assert(len(list) == 2);
+ * return list;
+ */
+static struct replay_insn_list **replay_insn_list_append(struct commit *operand,
+ enum replay_action action,
+ struct replay_insn_list **next)
+{
+ struct replay_insn_list *new = xmalloc(sizeof(*new));
+ new->action = action;
+ new->operand = operand;
+ *next = new;
+ new->next = NULL;
+ return &new->next;
+}
+
+static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
+{
+ struct replay_insn_list *cur;
+
+ for (cur = todo_list; cur; cur = cur->next) {
+ const char *sha1_abbrev, *action_str, *subject;
+ int subject_len;
+
+ action_str = action_name(cur->action);
+ sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV);
+ subject_len = find_commit_subject(cur->operand->buffer, &subject);
+ strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev,
+ subject_len, subject);
+ }
+ return 0;
+}
+
+static int parse_insn_line(char *bol, char *eol,
+ struct replay_insn_list *item, int lineno)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ unsigned char commit_sha1[20];
+ char *end_of_object_name;
+ int saved, status;
+ size_t error_len;
+
+ if (!prefixcmp(bol, "pick ") || !prefixcmp(bol, "pick\t")) {
+ item->action = REPLAY_PICK;
+ bol += strlen("pick ");
+ } else if (!prefixcmp(bol, "revert ") || !prefixcmp(bol, "revert\t")) {
+ item->action = REPLAY_REVERT;
+ bol += strlen("revert ");
+ } else {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Unrecognized action: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
+
+ /* Eat up extra spaces/ tabs before object name */
+ bol += strspn(bol, " \t");
+
+ end_of_object_name = bol + strcspn(bol, " \t\n");
+ saved = *end_of_object_name;
+ *end_of_object_name = '\0';
+ status = get_sha1(bol, commit_sha1);
+ *end_of_object_name = saved;
+
+ if (status < 0) {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Malformed object name: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
+
+ item->operand = lookup_commit_reference(commit_sha1);
+ if (!item->operand) {
+ error_len = eol - bol > 255 ? 255 : eol - bol;
+ return error(_("%s:%d: Not a valid commit: %.*s"),
+ todo_file, lineno, (int)error_len, bol);
+ }
+
+ item->next = NULL;
+ return 0;
+}
+
+static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
+{
+ struct replay_insn_list **next = todo_list;
+ struct replay_insn_list item = { NULL, 0, NULL };
+ char *p = buf;
+ int i;
+
+ for (i = 1; *p; i++) {
+ char *eol = strchrnul(p, '\n');
+ if (parse_insn_line(p, eol, &item, i) < 0)
+ return -1;
+ next = replay_insn_list_append(item.operand, item.action, next);
+ p = *eol ? eol + 1 : eol;
+ }
+ if (!*todo_list)
+ return error(_("No commits parsed."));
+ return 0;
+}
+
+static void read_populate_todo(struct replay_insn_list **todo_list)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ struct strbuf buf = STRBUF_INIT;
+ int fd, res;
+
+ fd = open(todo_file, O_RDONLY);
+ if (fd < 0)
+ die_errno(_("Could not open %s"), todo_file);
+ if (strbuf_read(&buf, fd, 0) < 0) {
+ close(fd);
+ strbuf_release(&buf);
+ die(_("Could not read %s."), todo_file);
+ }
+ close(fd);
+
+ res = parse_insn_buffer(buf.buf, todo_list);
+ strbuf_release(&buf);
+ if (res)
+ die(_("Unusable instruction sheet: %s"), todo_file);
+}
+
+static int populate_opts_cb(const char *key, const char *value, void *data)
+{
+ struct replay_opts *opts = data;
+ int error_flag = 1;
+
+ if (!value)
+ error_flag = 0;
+ else if (!strcmp(key, "options.no-commit"))
+ opts->no_commit = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.edit"))
+ opts->edit = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.signoff"))
+ opts->signoff = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.record-origin"))
+ opts->record_origin = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.allow-ff"))
+ opts->allow_ff = git_config_bool_or_int(key, value, &error_flag);
+ else if (!strcmp(key, "options.mainline"))
+ opts->mainline = git_config_int(key, value);
+ else if (!strcmp(key, "options.strategy"))
+ git_config_string(&opts->strategy, key, value);
+ else if (!strcmp(key, "options.strategy-option")) {
+ ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+ opts->xopts[opts->xopts_nr++] = xstrdup(value);
+ } else
+ return error(_("Invalid key: %s"), key);
+
+ if (!error_flag)
+ return error(_("Invalid value for %s: %s"), key, value);
+
+ return 0;
+}
+
+static void read_populate_opts(struct replay_opts **opts_ptr)
+{
+ const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+ if (!file_exists(opts_file))
+ return;
+ if (git_config_from_file(populate_opts_cb, opts_file, *opts_ptr) < 0)
+ die(_("Malformed options sheet: %s"), opts_file);
+}
+
+static void walk_revs_populate_todo(struct replay_insn_list **todo_list,
+ struct replay_opts *opts)
+{
+ struct commit *commit;
+ struct replay_insn_list **next;
+ enum replay_action action;
+
+ prepare_revs(opts);
+
+ next = todo_list;
+ action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
+ while ((commit = get_revision(opts->revs)))
+ next = replay_insn_list_append(commit, action, next);
+}
+
+static int create_seq_dir(void)
+{
+ const char *seq_dir = git_path(SEQ_DIR);
+
+ if (file_exists(seq_dir)) {
+ error(_("a cherry-pick or revert is already in progress"));
+ advise(_("try \"git cherry-pick (--continue | --quit | --abort)\""));
+ return -1;
+ }
+ else if (mkdir(seq_dir, 0777) < 0)
+ die_errno(_("Could not create sequencer directory %s"), seq_dir);
+ return 0;
+}
+
+static enum replay_command read_cmd(void)
+{
+ const char *cmd_file = git_path(SEQ_CMD_FILE);
+ struct strbuf buf = STRBUF_INIT;
+ enum replay_command res;
+ int fd;
+
+ fd = open(cmd_file, O_RDONLY);
+ if (fd < 0)
+ die_errno(_("Could not open %s"), cmd_file);
+ if (strbuf_read(&buf, fd, 0) < 0) {
+ close(fd);
+ strbuf_release(&buf);
+ die(_("Could not read %s."), cmd_file);
+ }
+ close(fd);
+
+ if (!strcmp(buf.buf, "revert\n"))
+ res = REPLAY_CMD_REVERT;
+ else if (!strcmp(buf.buf, "cherry-pick\n"))
+ res = REPLAY_CMD_CHERRY_PICK;
+ else {
+ strbuf_release(&buf);
+ die(_("Malformed command file: %s"), cmd_file);
+ }
+ strbuf_release(&buf);
+ return res;
+}
+
+static void save_cmd(struct replay_opts *opts)
+{
+ const char *cmd_file = git_path(SEQ_CMD_FILE);
+ static struct lock_file cmd_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&cmd_lock, cmd_file, LOCK_DIE_ON_ERROR);
+ strbuf_addf(&buf, "%s\n", command_name(opts));
+ if (write_in_full(fd, buf.buf, buf.len) < 0)
+ die_errno(_("Could not write to %s"), cmd_file);
+ if (commit_lock_file(&cmd_lock) < 0)
+ die(_("Error wrapping up %s."), cmd_file);
+}
+
+static void save_head(const char *head)
+{
+ const char *head_file = git_path(SEQ_HEAD_FILE);
+ static struct lock_file head_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+ strbuf_addf(&buf, "%s\n", head);
+ if (write_in_full(fd, buf.buf, buf.len) < 0)
+ die_errno(_("Could not write to %s"), head_file);
+ if (commit_lock_file(&head_lock) < 0)
+ die(_("Error wrapping up %s."), head_file);
+}
+
+static int reset_for_rollback(const unsigned char *sha1)
+{
+ const char *argv[4]; /* reset --merge <arg> + NULL */
+ argv[0] = "reset";
+ argv[1] = "--merge";
+ argv[2] = sha1_to_hex(sha1);
+ argv[3] = NULL;
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int rollback_single_pick(void)
+{
+ unsigned char head_sha1[20];
+
+ if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+ !file_exists(git_path("REVERT_HEAD")))
+ return error(_("no cherry-pick or revert in progress"));
+ if (read_ref_full("HEAD", head_sha1, 0, NULL))
+ return error(_("cannot resolve HEAD"));
+ if (is_null_sha1(head_sha1))
+ return error(_("cannot abort from a branch yet to be born"));
+ return reset_for_rollback(head_sha1);
+}
+
+static int sequencer_rollback(struct replay_opts *opts)
+{
+ const char *filename;
+ FILE *f;
+ unsigned char sha1[20];
+ struct strbuf buf = STRBUF_INIT;
+
+ filename = git_path(SEQ_HEAD_FILE);
+ f = fopen(filename, "r");
+ if (!f && errno == ENOENT) {
+ /*
+ * There is no multiple-cherry-pick in progress.
+ * If CHERRY_PICK_HEAD or REVERT_HEAD indicates
+ * a single-cherry-pick in progress, abort that.
+ */
+ return rollback_single_pick();
+ }
+ if (!f)
+ return error(_("cannot open %s: %s"), filename,
+ strerror(errno));
+ if (strbuf_getline(&buf, f, '\n')) {
+ error(_("cannot read %s: %s"), filename, ferror(f) ?
+ strerror(errno) : _("unexpected end of file"));
+ fclose(f);
+ goto fail;
+ }
+ fclose(f);
+ if (get_sha1_hex(buf.buf, sha1) || buf.buf[40] != '\0') {
+ error(_("stored pre-cherry-pick HEAD file '%s' is corrupt"),
+ filename);
+ goto fail;
+ }
+ if (reset_for_rollback(sha1))
+ goto fail;
+ remove_sequencer_state();
+ strbuf_release(&buf);
+ return 0;
+fail:
+ strbuf_release(&buf);
+ return -1;
+}
+
+static void save_todo(struct replay_insn_list *todo_list)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ static struct lock_file todo_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+ if (format_todo(&buf, todo_list) < 0)
+ die(_("Could not format %s."), todo_file);
+ if (write_in_full(fd, buf.buf, buf.len) < 0) {
+ strbuf_release(&buf);
+ die_errno(_("Could not write to %s"), todo_file);
+ }
+ if (commit_lock_file(&todo_lock) < 0) {
+ strbuf_release(&buf);
+ die(_("Error wrapping up %s."), todo_file);
+ }
+ strbuf_release(&buf);
+}
+
+static void save_opts(struct replay_opts *opts)
+{
+ const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+ if (opts->no_commit)
+ git_config_set_in_file(opts_file, "options.no-commit", "true");
+ if (opts->edit)
+ git_config_set_in_file(opts_file, "options.edit", "true");
+ if (opts->signoff)
+ git_config_set_in_file(opts_file, "options.signoff", "true");
+ if (opts->record_origin)
+ git_config_set_in_file(opts_file, "options.record-origin", "true");
+ if (opts->allow_ff)
+ git_config_set_in_file(opts_file, "options.allow-ff", "true");
+ if (opts->mainline) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "%d", opts->mainline);
+ git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+ strbuf_release(&buf);
+ }
+ if (opts->strategy)
+ git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+ if (opts->xopts) {
+ int i;
+ for (i = 0; i < opts->xopts_nr; i++)
+ git_config_set_multivar_in_file(opts_file,
+ "options.strategy-option",
+ opts->xopts[i], "^$", 0);
+ }
+}
+
+static int pick_commits(struct replay_insn_list *todo_list, struct replay_opts *opts)
+{
+ struct replay_insn_list *cur;
+ int res;
+
+ setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
+ if (opts->allow_ff)
+ assert(!(opts->signoff || opts->no_commit ||
+ opts->record_origin || opts->edit));
+ read_and_refresh_cache(opts);
+
+ for (cur = todo_list; cur; cur = cur->next) {
+ save_todo(cur);
+ res = do_pick_commit(cur->operand, cur->action, opts);
+ if (res)
+ return res;
+ }
+
+ /*
+ * Sequence of picks finished successfully; cleanup by
+ * removing the .git/sequencer directory
+ */
+ remove_sequencer_state();
+ return 0;
+}
+
+static int continue_single_pick(void)
+{
+ const char *argv[] = { "commit", NULL };
+
+ if (!file_exists(git_path("CHERRY_PICK_HEAD")) &&
+ !file_exists(git_path("REVERT_HEAD")))
+ return error(_("no cherry-pick or revert in progress"));
+ return run_command_v_opt(argv, RUN_GIT_CMD);
+}
+
+static int sequencer_continue(struct replay_opts *opts)
+{
+ struct replay_insn_list *todo_list = NULL;
+ enum replay_command cmd;
+
+ if (!file_exists(git_path(SEQ_TODO_FILE)))
+ return continue_single_pick();
+
+ /*
+ * Disallow continuing a cherry-pick with 'git revert
+ * --continue' and viceversa
+ */
+ cmd = read_cmd();
+ if (cmd != opts->command)
+ return error(_("cannot %s: a %s is in progress."),
+ command_name(opts),
+ cmd == REPLAY_CMD_REVERT ? "revert" : "cherry-pick");
+
+ read_populate_opts(&opts);
+ read_populate_todo(&todo_list);
+
+ /* Verify that the conflict has been resolved */
+ if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
+ file_exists(git_path("REVERT_HEAD"))) {
+ int ret = continue_single_pick();
+ if (ret)
+ return ret;
+ }
+ if (index_differs_from("HEAD", 0))
+ return error_dirty_index(opts);
+ todo_list = todo_list->next;
+ return pick_commits(todo_list, opts);
+}
+
+static int single_pick(struct commit *cmit, struct replay_opts *opts)
+{
+ enum replay_action action;
+ action = opts->command == REPLAY_CMD_REVERT ? REPLAY_REVERT : REPLAY_PICK;
+
+ setenv(GIT_REFLOG_ACTION, command_name(opts), 0);
+ return do_pick_commit(cmit, action, opts);
+}
+
+int sequencer_pick_revisions(struct replay_opts *opts)
+{
+ struct replay_insn_list *todo_list = NULL;
+ unsigned char sha1[20];
+
+ if (opts->subcommand == REPLAY_NONE)
+ assert(opts->revs);
+
+ read_and_refresh_cache(opts);
+
+ /*
+ * Decide what to do depending on the arguments; a fresh
+ * cherry-pick should be handled differently from an existing
+ * one that is being continued
+ */
+ if (opts->subcommand == REPLAY_REMOVE_STATE) {
+ remove_sequencer_state();
+ return 0;
+ }
+ if (opts->subcommand == REPLAY_ROLLBACK)
+ return sequencer_rollback(opts);
+ if (opts->subcommand == REPLAY_CONTINUE)
+ return sequencer_continue(opts);
+
+ /*
+ * If we were called as "git cherry-pick <commit>", just
+ * cherry-pick/revert it, set CHERRY_PICK_HEAD /
+ * REVERT_HEAD, and don't touch the sequencer state.
+ * This means it is possible to cherry-pick in the middle
+ * of a cherry-pick sequence.
+ */
+ if (opts->revs->cmdline.nr == 1 &&
+ opts->revs->cmdline.rev->whence == REV_CMD_REV &&
+ opts->revs->no_walk &&
+ !opts->revs->cmdline.rev->flags) {
+ struct commit *cmit;
+ if (prepare_revision_walk(opts->revs))
+ die(_("revision walk setup failed"));
+ cmit = get_revision(opts->revs);
+ if (!cmit || get_revision(opts->revs))
+ die("BUG: expected exactly one commit from walk");
+ return single_pick(cmit, opts);
+ }
+
+ /*
+ * Start a new cherry-pick/ revert sequence; but
+ * first, make sure that an existing one isn't in
+ * progress
+ */
+
+ walk_revs_populate_todo(&todo_list, opts);
+ if (create_seq_dir() < 0)
+ return -1;
+ if (get_sha1("HEAD", sha1)) {
+ if (opts->command == REPLAY_CMD_REVERT)
+ return error(_("Can't revert as initial commit"));
+ return error(_("Can't cherry-pick into empty head"));
+ }
+ save_cmd(opts);
+ save_head(sha1_to_hex(sha1));
+ save_opts(opts);
+ return pick_commits(todo_list, opts);
+}
diff --git a/sequencer.h b/sequencer.h
index d1cb5c2..58482f1 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -24,6 +24,29 @@ enum replay_subcommand {
REPLAY_ROLLBACK
};
+struct replay_opts {
+ enum replay_command command;
+ enum replay_subcommand subcommand;
+
+ /* Boolean options */
+ int edit;
+ int record_origin;
+ int no_commit;
+ int signoff;
+ int allow_ff;
+ int allow_rerere_auto;
+
+ int mainline;
+
+ /* Merge strategy */
+ const char *strategy;
+ const char **xopts;
+ size_t xopts_nr, xopts_alloc;
+
+ /* Only used by REPLAY_NONE */
+ struct rev_info *revs;
+};
+
struct replay_insn_list {
struct commit *operand;
enum replay_action action;
@@ -33,4 +56,6 @@ struct replay_insn_list {
/* Removes SEQ_DIR. */
extern void remove_sequencer_state(void);
+int sequencer_pick_revisions(struct replay_opts *opts);
+
#endif
--
1.7.8.2
^ permalink raw reply related
* Re: SVN -> Git *but* with special changes
From: Thomas Hochstein @ 2012-01-08 11:20 UTC (permalink / raw)
To: git
In-Reply-To: <1325999865995-7163737.post@n2.nabble.com>
Abscissa schrieb:
> Ok, I see. It's reporting 1.7.0, so that would explain it. One "sudo apt-get
> upgrade git" and...erm...well, it seems to be upgrading my whole damn
> computer, but I'll see how it all works out. Thanks!
"apt-get upgrade" will upgrade _all_ packages and doesn't take a
parameter:
| upgrade
| upgrade is used to install the newest versions of all packages
| currently installed on the system from the sources enumerated in
| /etc/apt/sources.list.
-thh
^ permalink raw reply
* Re: SVN -> Git *but* with special changes
From: Thomas Hochstein @ 2012-01-08 11:24 UTC (permalink / raw)
To: git
In-Reply-To: <1326000327637-7163752.post@n2.nabble.com>
Abscissa wrote:
> Well that's strange, it finished "upgrading", but now git is still just
> reporting 1.7.0.4, which is *exactly* the same version it said before.
What kind of distribution do you use?
"apt-get" sounds like Debian or Ubuntu, but those all have at least
git 1.7.1:
| Debian versions:
| stable 1:1.7.2.5-3
| stable-bpo 1:1.7.7.3-1~bpo60+2
| testing 1:1.7.7.3-1
| unstable 1:1.7.8.2-1
| exp 1:1.7.8~rc3-1
| Ubuntu versions:
| Precise Pangolin 1:1.7.7.3-1
| Oneiric Ocelot 1:1.7.5.4-1
| Natty Narwhal 1:1.7.4.1-3
| Maverick Meerkat 1:1.7.1-1.1ubuntu0.1
-thh
^ permalink raw reply
* Re: [PATCH 0/6] The move to sequencer.c
From: Jonathan Nieder @ 2012-01-08 19:28 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1326025653-11922-1-git-send-email-artagnon@gmail.com>
(-cc: Junio)
Ramkumar Ramachandra wrote:
> I've tried a slightly different approach: the objective of the patches
> seem to be much clearer this time.
For the sake of new and forgetful readers: what is that objective?
^ permalink raw reply
* Re: [PATCH 1/6] revert: move replay_action, replay_subcommand to header
From: Jonathan Nieder @ 2012-01-08 19:31 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Our plan to build a sequencer involves leaving the revert builtin with
> just argument parsing work. Since the enums replay_action and
> replay_subcommand have nothing to do with this argument parsing, move
> them to sequencer.h in advance.
>
> "REVERT" and "CHERRY_PICK" are unsuitable variable names for exposing
> publicly, as their purpose is unclear in the global context: rename
> them to "REPLAY_REVERT" and "REPLAY_PICK" respectively.
My first reaction: this probably would be more self-explanatory if
squashed with the patch the moves the rest of the code to sequencer.[ch].
Second reaction: ah, but the s/REVERT/REPLAY_REVERT/ and
s/CHERRY_PICK/REPLAY_PICK/ changes would be lost in the noise of the move.
Maybe it would be possible to make those changes but keep the enum in
builtin/revert.c, explaining that this is a preparatory step and they
will be moving in a few moments?
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 19:34 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> So, while a future
> instruction sheet would look like:
>
> pick next~4
> action3 b74fea
> revert rr/moo^2~34
>
> the actions "pick", "action3" and "revert" need not necessarily
> correspond to the specific builtins.
So what change does the patch actually make? Is this a renaming?
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -64,16 +64,21 @@ struct replay_opts {
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>
> -static const char *action_name(const struct replay_opts *opts)
> +static const char *command_name(struct replay_opts *opts)
Why is the const being dropped? I'm lost, so not reading further.
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 19:37 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-4-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> When we allow mixing "revert" and "pick" instructions in the same
> sheet in the next patch, the following workflow would be perfectly
> valid:
>
> $ git cherry-pick base..latercommit
> [conflict occurs]
> $ edit problematicfile
> $ git add problematicfile
> $ git revert --continue
> [finishes successfully]
Does "workflow" mean "sequence of commands"?
> This is confusing to the operator, because the sequencer is an
> implementation detail hidden behind the 'git cherry-pick' and 'git
> revert' builtins.
I don't know --- it's not confusing to me. Could you explain further
what harm the current behavior does? E.g., could it cause me to
misunderstand some basic concepts, or could it lead me to run commands
that cause me to scratch my head or lose data?
> builtin/revert.c | 57 +++++++++++++++++++++++++++++++++++++++
> sequencer.h | 1 +
> t/t3510-cherry-pick-sequence.sh | 11 +++++++
> 3 files changed, 69 insertions(+), 0 deletions(-)
I haven't read the patch, but based on the above rationale and this
diffstat, it doesn't look good.
^ permalink raw reply
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Jonathan Nieder @ 2012-01-08 19:40 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Parse the instruction sheet in '.git/sequencer/todo' as a list of
> (action, operand) pairs, instead of assuming that all lines have the
> same action.
Yes, I've always liked this one.
Haven't re-read the patch to avoid wasted effort if there are changes
when the previous patches in the series change. Maybe it would be
possible to send as a standalone?
^ permalink raw reply
* Re: [PATCH 0/6] The move to sequencer.c
From: Ramkumar Ramachandra @ 2012-01-08 19:51 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List
In-Reply-To: <20120108192853.GE1942@burratino>
Jonathan Nieder wrote:
> For the sake of new and forgetful readers: what is that objective?
The objective is to build a generalized sequencer, of which "git
revert" and "git cherry-pick" are two special cases. We can later
extend it to encompass more builtins and other commands. Why? In
general, we want uniform semantics to continue, skip, and abort any
operation that requires user-intervention (example: a conflict that
needs to be resolved before proceeding). A lot of this work is
inspired by the way "rebase -i" works: it presents the user an
instruction sheet with a sequence of actions to perform. Example
future use cases:
$ git cherry-pick moo..foo
[conflict]
$ edit problematicfile
$ git add problematicfile
$ git continue
[finishes successfully]
$ git revert moo..foo
[conflict]
$ edit problematicfile
$ git add problematicfile
$ git continue
[finishes successfully]
In these examples, the instruction sheet is uniformly filled with
"pick" or "revert" actions, which is not very interesting. When we
get an interface to allow easy editing of the instruction sheet and
encompass more builtins, more interesting sequences of operations will
be possible like:
pick rr/revert-cherry-pick^2~34
revert master@{12}
merge next
am /tmp/jrn.patch
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 19:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108193454.GG1942@burratino>
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> So, while a future
>> instruction sheet would look like:
>>
>> pick next~4
>> action3 b74fea
>> revert rr/moo^2~34
>>
>> the actions "pick", "action3" and "revert" need not necessarily
>> correspond to the specific builtins.
>
> So what change does the patch actually make? Is this a renaming?
Yes, it renames "action" to "command" where appropriate.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -64,16 +64,21 @@ struct replay_opts {
>>
>> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>>
>> -static const char *action_name(const struct replay_opts *opts)
>> +static const char *command_name(struct replay_opts *opts)
>
> Why is the const being dropped? I'm lost, so not reading further.
Minor error. The rest of the patch should be fine.
-- Ram
^ permalink raw reply
* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Jonathan Nieder @ 2012-01-08 20:07 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-6-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Three kinds of errors can arise from parsing '.git/sequencer/todo':
> 1. Unrecognized action
> 2. Malformed object name
> 3. Object name does not refer to a valid commit
>
> Since we would like to make the instruction sheet user-editable in the
> future (much like the 'rebase -i' sheet), report more fine-grained
> parse errors prefixed with the filename and line number.
Seems like a sensible idea. In other words, the infrastructure that
parses .git/sequencer/todo is meant to handle arbitrary user input
some day, so it can be used as the implementation of "git rebase
--interactive" and "git sequence --edit", and currently it is
suboptimal for that purpose because the parse error messages just say
error: Could not parse line 5.
This patch shifts responsibility to parse_insn_line(), which can come
up with a more detailed message like
error: .git/sequencer/todo:5: unrecognized action "frobnicate"
Once the operator is allowed to edit the sequence, the message might
be adjusted to something meaning
error: <sequence you just gave me>:5: unrecognized action "frobnicate"
instead of exposing an implementation detail, or maybe some day "git
sequence --edit" could even re-launch the editor with an error message
in a comment before the problematic line and the cursor pointing
there. And for now, pointing to the explicit filename is useful since
this should only happen if there was filesystem corruption, tampering,
or a git bug.
By the way, an example of the output before and after the patch would
have been useful in saving some trouble of figuring this out.
Ok, onward to the patch.
[...]
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
> return 0;
> }
>
> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> +static int parse_insn_line(char *bol, char *eol,
> + struct replay_insn_list *item, int lineno)
> {
> + const char *todo_file = git_path(SEQ_TODO_FILE);
This idiom is _still_ scary. I don't want to have to shout about
this, but why didn't the commit message at least acknowledge it to put
the reader's mind at ease when this has come up again and again?
[...]
> + bol += strlen("revert ");
> + } else {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Unrecognized action: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
Ah, so my example above was wrong: the actual message is
error: .git/sequencer/todo:5: Unrecognized action: the quick bro
wn fox jumps over the lazy dog
I guess that's fine. Is it intended?
> + }
>
> /* Eat up extra spaces/ tabs before object name */
> - padding = strspn(bol, " \t");
> - if (!padding)
> - return -1;
> - bol += padding;
> + bol += strspn(bol, " \t");
What does this have to do with the stated goal of the patch? It seems
to me that an unrelated and unadvertised bugfix snuck in.
[...]
> @@ -741,12 +744,18 @@ static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
> status = get_sha1(bol, commit_sha1);
> *end_of_object_name = saved;
>
> - if (status < 0)
> - return -1;
> + if (status < 0) {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Malformed object name: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
> + }
This seems a little repetitive --- maybe it would make sense to
factor out a function to emit errors of the form
error: file:lineno: message: line
By the way, this is gross. Probably get_sha1 should grow a variant
that takes a buffer and a length.
[...]
> item->operand = lookup_commit_reference(commit_sha1);
> - if (!item->operand)
> - return -1;
> + if (!item->operand) {
> + error_len = eol - bol > 255 ? 255 : eol - bol;
> + return error(_("%s:%d: Not a valid commit: %.*s"),
> + todo_file, lineno, (int)error_len, bol);
> + }
Hmm, this one can be emitted even when there was no corruption or
internal error, because the user removed a commit she was
cherry-picking and the gc-ed before a "git cherry-pick --continue".
Alternatively, it can happen because the repository has grown very
crowded and what used to be an unambiguous commit name no longer is
one (not enough digits). Will the error message be intuitive in these
situations?
[...]
> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>
> for (i = 1; *p; i++) {
> char *eol = strchrnul(p, '\n');
> - if (parse_insn_line(p, eol, &item) < 0)
> - return error(_("Could not parse line %d."), i);
> + if (parse_insn_line(p, eol, &item, i) < 0)
> + return -1;
Not related to this patch, but why the "< 0" test? It makes this
reader worry that there is some unusual return value convention that
he should be taking into account.
To sum up: the idea looks promising, but this patch doesn't seem to be
ready yet.
Hope that helps,
Jonathan
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 20:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0kHn+SDaeRHa8bUHWvOEVkr01sVDzvnw9E+-Nne2cii1Q@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> So what change does the patch actually make? Is this a renaming?
>
> Yes, it renames "action" to "command" where appropriate.
Wouldn't a simple renaming have a diffstat with the same number of added
and removed lines?
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:03 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108193749.GH1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> When we allow mixing "revert" and "pick" instructions in the same
>> sheet in the next patch, the following workflow would be perfectly
>> valid:
>>
>> $ git cherry-pick base..latercommit
>> [conflict occurs]
>> $ edit problematicfile
>> $ git add problematicfile
>> $ git revert --continue
>> [finishes successfully]
>
> Does "workflow" mean "sequence of commands"?
Yes. Clarified wording.
>> This is confusing to the operator, because the sequencer is an
>> implementation detail hidden behind the 'git cherry-pick' and 'git
>> revert' builtins.
>
> I don't know --- it's not confusing to me. Could you explain further
> what harm the current behavior does? E.g., could it cause me to
> misunderstand some basic concepts, or could it lead me to run commands
> that cause me to scratch my head or lose data?
Junio explained this to me in [1]. It's very unnatural for a user to
want to execute "git cherry-pick --continue" when the previous command
was a "git revert": it probably means that she forgot about the
in-progress "git revert". The problem becomes more serious when the
sequencer grows more capabilities: a "git merge --continue" to
continue a "git am" sounds much more absurd. Ofcourse, we will
provide a way to continue any sequencer operation in the future: "git
continue" seems to be a good candidate.
[1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Ramkumar Ramachandra @ 2012-01-08 20:07 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200910.GK1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
>
>>> So what change does the patch actually make? Is this a renaming?
>>
>> Yes, it renames "action" to "command" where appropriate.
>
> Wouldn't a simple renaming have a diffstat with the same number of added
> and removed lines?
Yes, almost. A few extra lines added because I needed a new data enum
for the "command"; also added a convenience function: action_name().
-- Ram
^ permalink raw reply
* Re: [PATCH 5/6] revert: report fine-grained error messages from insn parser
From: Ramkumar Ramachandra @ 2012-01-08 20:16 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108200748.GJ1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Three kinds of errors can arise from parsing '.git/sequencer/todo':
>> 1. Unrecognized action
>> 2. Malformed object name
>> 3. Object name does not refer to a valid commit
>>
>> Since we would like to make the instruction sheet user-editable in the
>> future (much like the 'rebase -i' sheet), report more fine-grained
>> parse errors prefixed with the filename and line number.
>
> Seems like a sensible idea. In other words,
> [...]
Thanks for the new wording.
> [...]
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
>> @@ -714,26 +714,29 @@ static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list)
>> return 0;
>> }
>>
>> -static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item)
>> +static int parse_insn_line(char *bol, char *eol,
>> + struct replay_insn_list *item, int lineno)
>> {
>> + const char *todo_file = git_path(SEQ_TODO_FILE);
>
> This idiom is _still_ scary. I don't want to have to shout about
> this, but why didn't the commit message at least acknowledge it to put
> the reader's mind at ease when this has come up again and again?
Carried over from the previous re-roll; sorry I didn't pay enough attention.
> [...]
>> + bol += strlen("revert ");
>> + } else {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + return error(_("%s:%d: Unrecognized action: %.*s"),
>> + todo_file, lineno, (int)error_len, bol);
>
> Ah, so my example above was wrong: the actual message is
>
> error: .git/sequencer/todo:5: Unrecognized action: the quick bro
> wn fox jumps over the lazy dog
>
> I guess that's fine. Is it intended?
>
>> + }
>>
>> /* Eat up extra spaces/ tabs before object name */
>> - padding = strspn(bol, " \t");
>> - if (!padding)
>> - return -1;
>> - bol += padding;
>> + bol += strspn(bol, " \t");
>
> What does this have to do with the stated goal of the patch? It seems
> to me that an unrelated and unadvertised bugfix snuck in.
Not a bugfix: since I have to report sensible error messages now, I
changed the "pick" and "revert" checks to "pick " || "pick\t" and
"revert " || "revert\t" -- then, I can report "invalid action" if it
doesn't match instead of a useless "missing space after action".
> [...]
> By the way, this is gross. Probably get_sha1 should grow a variant
> that takes a buffer and a length.
Yes; will do.
> [...]
>> item->operand = lookup_commit_reference(commit_sha1);
>> - if (!item->operand)
>> - return -1;
>> + if (!item->operand) {
>> + error_len = eol - bol > 255 ? 255 : eol - bol;
>> + return error(_("%s:%d: Not a valid commit: %.*s"),
>> + todo_file, lineno, (int)error_len, bol);
>> + }
>
> Hmm, this one can be emitted even when there was no corruption or
> internal error, because the user removed a commit she was
> cherry-picking and the gc-ed before a "git cherry-pick --continue".
> Alternatively, it can happen because the repository has grown very
> crowded and what used to be an unambiguous commit name no longer is
> one (not enough digits). Will the error message be intuitive in these
> situations?
Something like "Unable to look up commit: %s" perhaps?
> [...]
>> @@ -761,8 +770,8 @@ static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list)
>>
>> for (i = 1; *p; i++) {
>> char *eol = strchrnul(p, '\n');
>> - if (parse_insn_line(p, eol, &item) < 0)
>> - return error(_("Could not parse line %d."), i);
>> + if (parse_insn_line(p, eol, &item, i) < 0)
>> + return -1;
>
> Not related to this patch, but why the "< 0" test? It makes this
> reader worry that there is some unusual return value convention that
> he should be taking into account.
Right. Will fix.
Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 20:22 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=-AWy7HnVASB1rt8njavTYOhV7Zxsdq4TE+VShVZmEzQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> I don't know --- it's not confusing to me. Could you explain further
>> what harm the current behavior does? E.g., could it cause me to
>> misunderstand some basic concepts, or could it lead me to run commands
>> that cause me to scratch my head or lose data?
>
> Junio explained this to me in [1]. It's very unnatural for a user to
> want to execute "git cherry-pick --continue" when the previous command
> was a "git revert": it probably means that she forgot about the
> in-progress "git revert".
[...]
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
I don't think that's what Junio said.
Did this actually happen, or is it a theoretical worry? I think I would
be more likely to run "git cherry-pick <foo>..<bar>" than "git
cherry-pick --continue" if I had forgotten about an in-progress
revert. The former already errors out with a sensible message.
Or is the problem that I might run:
git revert foo..bar
git reset --merge; # conflict --- let's clean this up
# ah, I remember reverting the patch that conflicted before;
# let's reuse the resolution.
git cherry-pick baz
edit file.c; # another conflict, sigh
git add file.c
git cherry-pick --continue; # oops!
? That seems like a real worry, but the same problem could happen
with cherry-pick used both for the multipick and single-pick, so I
don't think your patch fundamentally addresses it.
In other words, this is a problem caused by the overloading of the
same cherry-pick command for single-pick and multi-pick. I think it
should be preventable by remembering which action failed when stopping
a sequence and doing only a single-pick resume if
CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.
The "oops" is bad since the operator might have been intending to run
some more tests and amend as necessary before continuing the
multi-pick. It is not _that_ bad, since more typically one would have
already run some tests before running cherry-pick --continue to commit
the resolution. Still probably worth fixing.
> The problem becomes more serious when the
> sequencer grows more capabilities: a "git merge --continue" to
> continue a "git am" sounds much more absurd. Ofcourse, we will
> provide a way to continue any sequencer operation in the future: "git
> continue" seems to be a good candidate.
I don't understand why "cherry-pick --continue" resuming a revert
sequence implies that "merge --continue" would have to as well.
All that said, forbidding cherry-pick --continue from resuming a
revert sequence would be fine with me, _as long as the semantics are
clearly spelled out in the commit message and documentation_. What
happens when there is a mixture of picks and reverts?
Thanks.
Jonathan
^ permalink raw reply
* Re: [PATCH 4/6] revert: allow mixing "pick" and "revert" actions
From: Ramkumar Ramachandra @ 2012-01-08 20:17 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108194014.GI1942@burratino>
Hi,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>
>> Parse the instruction sheet in '.git/sequencer/todo' as a list of
>> (action, operand) pairs, instead of assuming that all lines have the
>> same action.
>
> Yes, I've always liked this one.
>
> Haven't re-read the patch to avoid wasted effort if there are changes
> when the previous patches in the series change. Maybe it would be
> possible to send as a standalone?
If I don't get manage to get the series right in a couple of re-rolls,
I'll do that.
-- Ram
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Ramkumar Ramachandra @ 2012-01-08 20:28 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Git List, Junio C Hamano
In-Reply-To: <20120108202216.GL1942@burratino>
Hi again,
Jonathan Nieder wrote:
> Ramkumar Ramachandra wrote:
>> Jonathan Nieder wrote:
> [...]
>> Junio explained this to me in [1]. It's very unnatural for a user to
>> want to execute "git cherry-pick --continue" when the previous command
>> was a "git revert": it probably means that she forgot about the
>> in-progress "git revert".
> [...]
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/185355
>
> I don't think that's what Junio said.
>
> Did this actually happen, or is it a theoretical worry? I think I would
> be more likely to run "git cherry-pick <foo>..<bar>" than "git
> cherry-pick --continue" if I had forgotten about an in-progress
> revert. The former already errors out with a sensible message.
>
> Or is the problem that I might run:
>
> git revert foo..bar
> git reset --merge; # conflict --- let's clean this up
>
> # ah, I remember reverting the patch that conflicted before;
> # let's reuse the resolution.
> git cherry-pick baz
> edit file.c; # another conflict, sigh
> git add file.c
> git cherry-pick --continue; # oops!
>
> ? That seems like a real worry, but the same problem could happen
> with cherry-pick used both for the multipick and single-pick, so I
> don't think your patch fundamentally addresses it.
Good catch. I didn't replay this scenario in my head earlier.
> In other words, this is a problem caused by the overloading of the
> same cherry-pick command for single-pick and multi-pick. I think it
> should be preventable by remembering which action failed when stopping
> a sequence and doing only a single-pick resume if
> CHERRY_PICK_HEAD/REVERT_HEAD/whatever doesn't match that.
I was attempting to fix this to simplify the life of the user, not
complicate it further- the user might have no idea what the next
command in the sequence is, and I don't see the point in
inconveniencing her. In retrospect, I think we should simply drop
this patch.
> What
> happens when there is a mixture of picks and reverts?
Permitted.
-- Ram
^ permalink raw reply
* Re: [PATCH 6/6] sequencer: factor code out of revert builtin
From: Jonathan Nieder @ 2012-01-08 20:38 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326025653-11922-7-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Expose the cherry-picking machinery through a public
> sequencer_pick_revisions() (renamed from pick_revisions() in
> builtin/revert.c), so that cherry-picking and reverting are special
> cases of a general sequencer operation. The cherry-pick builtin is
> now a thin wrapper that does command-line argument parsing before
> calling into sequencer_pick_revisions(). In the future, we can write
> a new "foo" builtin that calls into the sequencer like:
Nice.
I don't think the plan is actually a "foo" builtin.
I guess I would say "So now your 'foo' builtin can use the sequencer
machinery by implementing a 'parse_args_populate_opts' function and
then running the following:" so as to leave the reader more excited
and not to imply a promise we are not going to keep. :)
> memset(&opts, 0, sizeof(opts));
> opts.command = REPLAY_CMD_FOO;
> opts.revisions = xmalloc(sizeof(*opts.revs));
> parse_args_populate_opts(argc, argv, &opts);
> init_revisions(opts.revs);
> sequencer_pick_revisions(&opts);
Hm, I wonder if opts.command should be a string so each new caller
does not have to add to the enum and switch statements...
> This patch does not intend to make any functional changes. Check
> with:
>
> $ git blame -s -C HEAD^..HEAD -- sequencer.c
Neat.
[...]
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -24,6 +24,29 @@ enum replay_subcommand {
> REPLAY_ROLLBACK
> };
>
> +struct replay_opts {
[...]
> +};
[...]
> @@ -33,4 +56,6 @@ struct replay_insn_list {
[...]
> +int sequencer_pick_revisions(struct replay_opts *opts);
Looks sensible. Maybe it would be useful to include a
Documentation/technical/api-sequencer.txt explaining how this is meant
to be used. (Not much detail needed, just an overview. See the
surrounding files for some examples.)
Haven't checked the code movement yet, since it is sitting on top of a
slushy base. Next time, hopefully.
Thanks,
Jonathan
^ permalink raw reply
* Re: [PATCH 0/6] The move to sequencer.c
From: Jonathan Nieder @ 2012-01-08 20:43 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <CALkWK0=xhnXq4_uEAri74Kk9zbAgiS+z-nG7Etm3MCo2cXaNPw@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> For the sake of new and forgetful readers: what is that objective?
>
> The objective is to build a generalized sequencer
[...]
> In these examples, the instruction sheet is uniformly filled with
> "pick" or "revert" actions, which is not very interesting. When we
> get an interface to allow easy editing of the instruction sheet and
> encompass more builtins, more interesting sequences of operations will
> be possible like:
>
> pick rr/revert-cherry-pick^2~34
> revert master@{12}
> merge next
> am /tmp/jrn.patch
Ah, okay. So the tasks at hand are (1) allowing heterogeneous
instruction sheets (pick + revert for now) and (2) exposing the
sequencer interface in sequencer.h. (2) does not strictly
depend on (1) but (1) has more short-term benefit that is easy
to test so it comes first. No UI aside from editing the instruction
sheet manually for now, but that can come later. Thanks for
explaining.
Jonathan
^ permalink raw reply
* Re: [PATCH 3/6] revert: don't let revert continue a cherry-pick
From: Jonathan Nieder @ 2012-01-08 20:45 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0kwbzXRyFf=JjfAW9yD7M_FTB80+q1UPOCv-Em4qO2RKQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> In retrospect, I think we should simply drop
> this patch.
Great --- I don't think it had much to do with this series.
I'll keep this mail in my inbox. Who knows --- maybe I'll get a
moment to teach conflicted single-picks to write a
.git/sequencer/when-continuing-just-continue-the-single-pick file.
^ permalink raw reply
* Re: [PATCH 2/6] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-08 20:48 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <CALkWK0=kw=EEXDfnrFkeNV678r4u3v72-=hKh9w8ujRN125NPQ@mail.gmail.com>
Ramkumar Ramachandra wrote:
> Jonathan Nieder wrote:
>> Ramkumar Ramachandra wrote:
>>> Jonathan Nieder wrote:
>>>> So what change does the patch actually make? Is this a renaming?
>>>
>>> Yes, it renames "action" to "command" where appropriate.
>>
>> Wouldn't a simple renaming have a diffstat with the same number of added
>> and removed lines?
>
> Yes, almost. A few extra lines added because I needed a new data enum
> for the "command"; also added a convenience function: action_name().
It's not a simple renaming, then.
What user-visible effect will this have, if any? What programmer-visible
effect will it have, if any? I _really_ should not have to read the patch
to learn the impact of a patch.
^ permalink raw reply
* [PATCH 2/5 v2] dashed externals: kill children on exit
From: Clemens Buchacher @ 2012-01-08 20:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Jonathan Nieder, Erik Faye-Lund, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <7v4nw6hfpy.fsf@alter.siamese.dyndns.org>
Several git commands are so-called dashed externals, that is commands
executed as a child process of the git wrapper command. If the git
wrapper is killed by a signal, the child process will continue to run.
This is different from internal commands, which always die with the git
wrapper command.
Enable the recently introduced cleanup mechanism for child processes in
order to make dashed externals act more in line with internal commands.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sat, Jan 07, 2012 at 10:26:49PM -0800, Junio C Hamano wrote:
>
> Yeah, I agree 100% with that reasoning. I seem to recall that was how this
> commit was done in what I privately reviewed after Clemens announced his
> github branch?
What I had previously enabled child cleanup for all callers of
run_command_v_opt. There is quite a few of those as well, most of them
not related to dashed externals at all.
So I reworked that patch a bit to enable cleanup only for dashed
externals. This is a replacement for "[PATCH 2/5] run-command: kill
children on exit by default".
I also re-added Jeff's "send-pack: kill pack-objects helper on signal or
exit" and I dropped the extraneous newline that Erik spotted in
"[PATCH 1/5] run-command: optionally kill children on exit".
I have pushed the reworked series to cb/git-daemon-tests on my github
repo.
git.c | 2 +-
run-command.c | 1 +
run-command.h | 1 +
3 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/git.c b/git.c
index fb9029c..3805616 100644
--- a/git.c
+++ b/git.c
@@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv)
* if we fail because the command is not found, it is
* OK to return. Otherwise, we just pass along the status code.
*/
- status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+ status = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);
if (status >= 0 || errno != ENOENT)
exit(status);
diff --git a/run-command.c b/run-command.c
index fff9073..90bfd8c 100644
--- a/run-command.c
+++ b/run-command.c
@@ -496,6 +496,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,
cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+ cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
}
int run_command_v_opt(const char **argv, int opt)
diff --git a/run-command.h b/run-command.h
index 2a69466..44f7d2b 100644
--- a/run-command.h
+++ b/run-command.h
@@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...);
#define RUN_COMMAND_STDOUT_TO_STDERR 4
#define RUN_SILENT_EXEC_FAILURE 8
#define RUN_USING_SHELL 16
+#define RUN_CLEAN_ON_EXIT 32
int run_command_v_opt(const char **argv, int opt);
/*
--
1.7.8
^ permalink raw reply related
* Re: [PATCH 1/5] run-command: optionally kill children on exit
From: Clemens Buchacher @ 2012-01-08 20:56 UTC (permalink / raw)
To: Erik Faye-Lund
Cc: Junio C Hamano, git, Jeff King, Jonathan Nieder, Ilari Liusvaara,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CABPQNSb57LA6dYJvT7xF_vFfBFqKhCMbrQYp49_Ko1WmbUnYPw@mail.gmail.com>
On Sat, Jan 07, 2012 at 01:45:03PM +0100, Erik Faye-Lund wrote:
>
> Our Windows implementation of kill (mingw_kill in compat/mingw.c) only
> supports SIGKILL, so propagating other signals to child-processes will
> fail with EINVAL. That being said, Windows' support for signals is
> severely limited, but I'm not entirely sure which ones can be
> generated in this case.
On Linux at least, SIGKILL is not a viable alternative for SIGTERM,
since it does not give the child process to do any cleanup of its own
(such as signaling its own children, for example).
In any case, due this whole experience, and recently another one with
overzealous virus scanners, I have added a "get rid of dashed externals"
work item to my TODO list.
^ permalink raw reply
* Re: [PATCH 2/5 v2] dashed externals: kill children on exit
From: Jeff King @ 2012-01-08 21:07 UTC (permalink / raw)
To: Clemens Buchacher
Cc: Junio C Hamano, git, Jonathan Nieder, Erik Faye-Lund,
Ilari Liusvaara, Nguyễn Thái Ngọc Duy
In-Reply-To: <20120108204109.GA3394@ecki.lan>
On Sun, Jan 08, 2012 at 09:41:09PM +0100, Clemens Buchacher wrote:
> What I had previously enabled child cleanup for all callers of
> run_command_v_opt. There is quite a few of those as well, most of them
> not related to dashed externals at all.
>
> So I reworked that patch a bit to enable cleanup only for dashed
> externals. This is a replacement for "[PATCH 2/5] run-command: kill
> children on exit by default".
Thanks, this looks much closer to what I was expecting.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox