From 38ef61cfde8aea0864e898d37ce3213a5771c59e Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:08:59 +0530 Subject: [PATCH 01/19] advice: Introduce error_resolve_conflict Enable future callers to report a conflict and not die immediately by introducing a new function called error_resolve_conflict. Re-implement die_resolve_conflict as a call to error_resolve_conflict followed by a call to die. Consequently, the message printed by die_resolve_conflict changes from fatal: 'commit' is not possible because you have unmerged files. Please, fix them up in the work tree ... ... to error: 'commit' is not possible because you have unmerged files. hint: Fix them up in the work tree ... hint: ... fatal: Exiting because of an unresolved conflict. Hints are printed using the same advise function introduced in v1.7.3-rc0~26^2~3 (Introduce advise() to print hints, 2010-08-11). Inspired-by: Christian Couder Helped-by: Jonathan Nieder Reviewed-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- advice.c | 31 ++++++++++++++++++++++++------- advice.h | 3 ++- builtin/revert.c | 9 --------- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/advice.c b/advice.c index 0be4b5f008..e02e632df3 100644 --- a/advice.c +++ b/advice.c @@ -19,6 +19,15 @@ static struct { { "detachedhead", &advice_detached_head }, }; +void advise(const char *advice, ...) +{ + va_list params; + + va_start(params, advice); + vreportf("hint: ", advice, params); + va_end(params); +} + int git_default_advice_config(const char *var, const char *value) { const char *k = skip_prefix(var, "advice."); @@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value) return 0; } -void NORETURN die_resolve_conflict(const char *me) +int error_resolve_conflict(const char *me) { - if (advice_resolve_conflict) + error("'%s' is not possible because you have unmerged files.", me); + if (advice_resolve_conflict) { /* * Message used both when 'git commit' fails and when * other commands doing a merge do. */ - die("'%s' is not possible because you have unmerged files.\n" - "Please, fix them up in the work tree, and then use 'git add/rm ' as\n" - "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me); - else - die("'%s' is not possible because you have unmerged files.", me); + advise("Fix them up in the work tree,"); + advise("and then use 'git add/rm ' as"); + advise("appropriate to mark resolution and make a commit,"); + advise("or use 'git commit -a'."); + } + return -1; +} + +void NORETURN die_resolve_conflict(const char *me) +{ + error_resolve_conflict(me); + die("Exiting because of an unresolved conflict."); } diff --git a/advice.h b/advice.h index 3244ebb5c1..e5d0af782b 100644 --- a/advice.h +++ b/advice.h @@ -11,7 +11,8 @@ extern int advice_implicit_identity; extern int advice_detached_head; int git_default_advice_config(const char *var, const char *value); - +void advise(const char *advice, ...); +int error_resolve_conflict(const char *me); extern void NORETURN die_resolve_conflict(const char *me); #endif /* ADVICE_H */ diff --git a/builtin/revert.c b/builtin/revert.c index 1f27c63343..2df3f3be4f 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -214,15 +214,6 @@ static void write_cherry_pick_head(void) strbuf_release(&buf); } -static void advise(const char *advice, ...) -{ - va_list params; - - va_start(params, advice); - vreportf("hint: ", advice, params); - va_end(params); -} - static void print_advice(void) { char *msg = getenv("GIT_CHERRY_PICK_HELP"); From 5ec3118293931cf1fe8677f16449999cfbfc8b92 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:00 +0530 Subject: [PATCH 02/19] config: Introduce functions to write non-standard file Introduce two new functions corresponding to "git_config_set" and "git_config_set_multivar" to write a non-standard configuration file. Expose these new functions in cache.h for other git programs to use. Helped-by: Jeff King Helped-by: Jonathan Nieder Reviewed-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- cache.h | 2 ++ config.c | 36 +++++++++++++++++++++++++++--------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/cache.h b/cache.h index e11cf6ab1c..58a277d7a5 100644 --- a/cache.h +++ b/cache.h @@ -1051,9 +1051,11 @@ extern int git_config_bool(const char *, const char *); extern int git_config_maybe_bool(const char *, const char *); extern int git_config_string(const char **, const char *, const char *); extern int git_config_pathname(const char **, const char *, const char *); +extern int git_config_set_in_file(const char *, const char *, const char *); extern int git_config_set(const char *, const char *); extern int git_config_parse_key(const char *, char **, int *); extern int git_config_set_multivar(const char *, const char *, const char *, int); +extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int); extern int git_config_rename_section(const char *, const char *); extern const char *git_etc_gitconfig(void); extern int check_repository_format_version(const char *var, const char *value, void *cb); diff --git a/config.c b/config.c index e0b3b80d92..275427b53a 100644 --- a/config.c +++ b/config.c @@ -1073,6 +1073,12 @@ contline: return offset; } +int git_config_set_in_file(const char *config_filename, + const char *key, const char *value) +{ + return git_config_set_multivar_in_file(config_filename, key, value, NULL, 0); +} + int git_config_set(const char *key, const char *value) { return git_config_set_multivar(key, value, NULL, 0); @@ -1170,19 +1176,14 @@ out_free_ret_1: * - the config file is removed and the lock file rename()d to it. * */ -int git_config_set_multivar(const char *key, const char *value, - const char *value_regex, int multi_replace) +int git_config_set_multivar_in_file(const char *config_filename, + const char *key, const char *value, + const char *value_regex, int multi_replace) { int fd = -1, in_fd; int ret; - char *config_filename; struct lock_file *lock = NULL; - if (config_exclusive_filename) - config_filename = xstrdup(config_exclusive_filename); - else - config_filename = git_pathdup("config"); - /* parse-key returns negative; flip the sign to feed exit(3) */ ret = 0 - git_config_parse_key(key, &store.key, &store.baselen); if (ret) @@ -1359,7 +1360,6 @@ int git_config_set_multivar(const char *key, const char *value, out_free: if (lock) rollback_lock_file(lock); - free(config_filename); return ret; write_err_out: @@ -1368,6 +1368,24 @@ write_err_out: } +int git_config_set_multivar(const char *key, const char *value, + const char *value_regex, int multi_replace) +{ + const char *config_filename; + char *buf = NULL; + int ret; + + if (config_exclusive_filename) + config_filename = config_exclusive_filename; + else + config_filename = buf = git_pathdup("config"); + + ret = git_config_set_multivar_in_file(config_filename, key, value, + value_regex, multi_replace); + free(buf); + return ret; +} + static int section_name_match (const char *buf, const char *name) { int i = 0, j = 0, dot = 0; From be33c46cda2cd74344d338150019cbabd88df12f Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:01 +0530 Subject: [PATCH 03/19] revert: Simplify and inline add_message_to_msg The add_message_to_msg function has some dead code, an unclear API, only one callsite. While it originally intended fill up an empty commit message with the commit object name while picking, it really doesn't do this -- a bug introduced in v1.5.1-rc1~65^2~2 (Make git-revert & git-cherry-pick a builtin, 2007-03-01). Today, tests in t3505-cherry-pick-empty.sh indicate that not filling up an empty commit message is the desired behavior. Re-implement and inline the function accordingly, with a beneficial side-effect: don't dereference a NULL pointer when the commit doesn't have a delimeter after the header. Helped-by: Junio C Hamano Mentored-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 2df3f3be4f..7dfe2951de 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -185,19 +185,6 @@ static char *get_encoding(const char *message) return NULL; } -static void add_message_to_msg(struct strbuf *msgbuf, const char *message) -{ - const char *p = message; - while (*p && (*p != '\n' || p[1] != '\n')) - p++; - - if (!*p) - strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1)); - - p += 2; - strbuf_addstr(msgbuf, p); -} - static void write_cherry_pick_head(void) { int fd; @@ -462,11 +449,24 @@ static int do_pick_commit(void) } strbuf_addstr(&msgbuf, ".\n"); } else { + const char *p; + base = parent; base_label = msg.parent_label; next = commit; next_label = msg.label; - add_message_to_msg(&msgbuf, msg.message); + + /* + * 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 (no_replay) { strbuf_addstr(&msgbuf, "(cherry picked from commit "); strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1)); From a2ec3ad28f4608bc848feaa3d087632e44842dbf Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:02 +0530 Subject: [PATCH 04/19] revert: Don't check lone argument in get_encoding The only place get_encoding uses the global "commit" variable is when writing an error message explaining that its lone argument was NULL. Since the function's only caller ensures that a NULL argument isn't passed, we can remove this check with two beneficial consequences: 1. Since the function doesn't use the global "commit" variable any more, it won't need to change when we eliminate the global variable later in the series. 2. Translators no longer need to localize an error message that will never be shown. Suggested-by: Junio C Hamano Mentored-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 7dfe2951de..30b39c040c 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -167,9 +167,6 @@ static char *get_encoding(const char *message) { const char *p = message, *eol; - if (!p) - die (_("Could not read commit message of %s"), - sha1_to_hex(commit->object.sha1)); while (*p && *p != '\n') { for (eol = p + 1; *eol && *eol != '\n'; eol++) ; /* do nothing */ From 54decbd4d858f21e2c31601f91675700c7093b1f Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:03 +0530 Subject: [PATCH 05/19] revert: Rename no_replay to record_origin The "-x" command-line option is used to record the name of the original commits being picked in the commit message. The variable corresponding to this option is named "no_replay" for historical reasons; the name is especially confusing because the term "replay" is used to describe what cherry-pick does (for example, in the documentation of the "--mainline" option). So, give the variable corresponding to the "-x" command-line option a better name: "record_origin". Mentored-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 30b39c040c..794c050c30 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = { NULL }; -static int edit, no_replay, no_commit, mainline, signoff, allow_ff; +static int edit, record_origin, no_commit, mainline, signoff, allow_ff; static enum { REVERT, CHERRY_PICK } action; static struct commit *commit; static int commit_argc; @@ -91,7 +91,7 @@ static void parse_args(int argc, const char **argv) if (action == CHERRY_PICK) { struct option cp_extra[] = { - OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"), + OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"), OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"), OPT_END(), }; @@ -464,7 +464,7 @@ static int do_pick_commit(void) strbuf_addstr(&msgbuf, p); } - if (no_replay) { + if (record_origin) { strbuf_addstr(&msgbuf, "(cherry picked from commit "); strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1)); strbuf_addstr(&msgbuf, ")\n"); @@ -559,7 +559,7 @@ static int revert_or_cherry_pick(int argc, const char **argv) die(_("cherry-pick --ff cannot be used with --signoff")); if (no_commit) die(_("cherry-pick --ff cannot be used with --no-commit")); - if (no_replay) + if (record_origin) die(_("cherry-pick --ff cannot be used with -x")); if (edit) die(_("cherry-pick --ff cannot be used with --edit")); From 708f9d96d95be7e9912eea8350fdf3f04484695f Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:04 +0530 Subject: [PATCH 06/19] revert: Eliminate global "commit" variable Functions which act on commits currently rely on a file-scope static variable to be set before they're called. Consequently, the API and corresponding callsites are ugly and unclear. Remove this variable and change their API to accept the commit to act on as additional argument so that the callsites change from looking like commit = prepare_a_commit(); act_on_commit(); to looking like commit = prepare_a_commit(); act_on_commit(commit); This change is also in line with our long-term goal of exposing some of these functions through a public API. Inspired-by: Christian Couder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 794c050c30..d6c9f1dadb 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = { static int edit, record_origin, no_commit, mainline, signoff, allow_ff; static enum { REVERT, CHERRY_PICK } action; -static struct commit *commit; static int commit_argc; static const char **commit_argv; static int allow_rerere_auto; @@ -116,25 +115,25 @@ struct commit_message { const char *message; }; -static int get_message(const char *raw_message, struct commit_message *out) +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 (!raw_message) + if (!commit->buffer) return -1; - encoding = get_encoding(raw_message); + 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 = raw_message; + out->message = commit->buffer; if (strcmp(encoding, git_commit_encoding)) - out->reencoded_message = reencode_string(raw_message, + out->reencoded_message = reencode_string(commit->buffer, git_commit_encoding, encoding); if (out->reencoded_message) out->message = out->reencoded_message; @@ -182,7 +181,7 @@ static char *get_encoding(const char *message) return NULL; } -static void write_cherry_pick_head(void) +static void write_cherry_pick_head(struct commit *commit) { int fd; struct strbuf buf = STRBUF_INIT; @@ -355,7 +354,7 @@ static int run_git_commit(const char *defmsg) return run_command_v_opt(args, RUN_GIT_CMD); } -static int do_pick_commit(void) +static int do_pick_commit(struct commit *commit) { unsigned char head[20]; struct commit *base, *next, *parent; @@ -417,7 +416,7 @@ static int do_pick_commit(void) die(_("%s: cannot parse parent commit %s"), me, sha1_to_hex(parent->object.sha1)); - if (get_message(commit->buffer, &msg) != 0) + if (get_message(commit, &msg) != 0) die(_("Cannot get commit message for %s"), sha1_to_hex(commit->object.sha1)); @@ -470,7 +469,7 @@ static int do_pick_commit(void) strbuf_addstr(&msgbuf, ")\n"); } if (!no_commit) - write_cherry_pick_head(); + write_cherry_pick_head(commit); } if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) { @@ -548,6 +547,7 @@ static void read_and_refresh_cache(const char *me) static int revert_or_cherry_pick(int argc, const char **argv) { struct rev_info revs; + struct commit *commit; git_config(git_default_config, NULL); me = action == REVERT ? "revert" : "cherry-pick"; @@ -570,7 +570,7 @@ static int revert_or_cherry_pick(int argc, const char **argv) prepare_revs(&revs); while ((commit = get_revision(&revs))) { - int res = do_pick_commit(); + int res = do_pick_commit(commit); if (res) return res; } From 80e1f791883a9b6104a2a1f0149118ade1890f25 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:05 +0530 Subject: [PATCH 07/19] revert: Introduce struct to keep command-line options The current code uses a set of file-scope static variables to tell the cherry-pick/ revert machinery how to replay the changes, and initializes them by parsing the command-line arguments. In later steps in this series, we would like to introduce an API function that calls into this machinery directly and have a way to tell it what to do. Hence, introduce a structure to group these variables, so that the API can take them as a single replay_options parameter. The only exception is the variable "me" -- remove it since it not an independent option, and can be inferred from the action. Unfortunately, this patch introduces a minor regression. Parsing strategy-option violates a C89 rule: Initializers cannot refer to variables whose address is not known at compile time. Currently, this rule is violated by some other parts of Git as well, and it is possible to get GCC to report these instances using the "-std=c89 -pedantic" option. Inspired-by: Christian Couder Mentored-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 197 ++++++++++++++++++++++++++--------------------- 1 file changed, 111 insertions(+), 86 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index d6c9f1dadb..50f72debe0 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -35,76 +35,94 @@ static const char * const cherry_pick_usage[] = { NULL }; -static int edit, record_origin, no_commit, mainline, signoff, allow_ff; -static enum { REVERT, CHERRY_PICK } action; -static int commit_argc; -static const char **commit_argv; -static int allow_rerere_auto; +enum replay_action { REVERT, CHERRY_PICK }; -static const char *me; +struct replay_opts { + enum replay_action action; -/* Merge strategy. */ -static const char *strategy; -static const char **xopts; -static size_t xopts_nr, xopts_alloc; + /* Boolean options */ + int edit; + int record_origin; + int no_commit; + int signoff; + int allow_ff; + int allow_rerere_auto; + + int mainline; + int commit_argc; + const char **commit_argv; + + /* Merge strategy */ + const char *strategy; + const char **xopts; + size_t xopts_nr, xopts_alloc; +}; #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" +static const char *action_name(const struct replay_opts *opts) +{ + return opts->action == REVERT ? "revert" : "cherry-pick"; +} + static char *get_encoding(const char *message); -static const char * const *revert_or_cherry_pick_usage(void) +static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) { - return action == REVERT ? revert_usage : cherry_pick_usage; + return opts->action == REVERT ? revert_usage : cherry_pick_usage; } static int option_parse_x(const struct option *opt, const char *arg, int unset) { + struct replay_opts **opts_ptr = opt->value; + struct replay_opts *opts = *opts_ptr; + if (unset) return 0; - ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc); - xopts[xopts_nr++] = xstrdup(arg); + ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc); + opts->xopts[opts->xopts_nr++] = xstrdup(arg); return 0; } -static void parse_args(int argc, const char **argv) +static void parse_args(int argc, const char **argv, struct replay_opts *opts) { - const char * const * usage_str = revert_or_cherry_pick_usage(); + const char * const * usage_str = revert_or_cherry_pick_usage(opts); int noop; struct option options[] = { - OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"), - OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"), + OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"), + OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"), { OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)", PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 }, - OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"), - OPT_INTEGER('m', "mainline", &mainline, "parent number"), - OPT_RERERE_AUTOUPDATE(&allow_rerere_auto), - OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"), - OPT_CALLBACK('X', "strategy-option", &xopts, "option", + OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"), + OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"), + OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto), + OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"), + OPT_CALLBACK('X', "strategy-option", &opts, "option", "option for merge strategy", option_parse_x), OPT_END(), OPT_END(), OPT_END(), }; - if (action == CHERRY_PICK) { + if (opts->action == CHERRY_PICK) { struct option cp_extra[] = { - OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"), - OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"), + OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), + OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), OPT_END(), }; if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra)) die(_("program error")); } - commit_argc = parse_options(argc, argv, NULL, options, usage_str, - PARSE_OPT_KEEP_ARGV0 | - PARSE_OPT_KEEP_UNKNOWN); - if (commit_argc < 2) + opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str, + PARSE_OPT_KEEP_ARGV0 | + PARSE_OPT_KEEP_UNKNOWN); + if (opts->commit_argc < 2) usage_with_options(usage_str, options); - commit_argv = argv; + opts->commit_argv = argv; } struct commit_message { @@ -240,20 +258,20 @@ static struct tree *empty_tree(void) return tree; } -static NORETURN void die_dirty_index(const char *me) +static NORETURN void die_dirty_index(struct replay_opts *opts) { if (read_cache_unmerged()) { - die_resolve_conflict(me); + die_resolve_conflict(action_name(opts)); } else { if (advice_commit_before_merge) { - if (action == REVERT) + if (opts->action == REVERT) die(_("Your local changes would be overwritten by revert.\n" "Please, commit your changes or stash them to proceed.")); else die(_("Your local changes would be overwritten by cherry-pick.\n" "Please, commit your changes or stash them to proceed.")); } else { - if (action == REVERT) + if (opts->action == REVERT) die(_("Your local changes would be overwritten by revert.\n")); else die(_("Your local changes would be overwritten by cherry-pick.\n")); @@ -274,7 +292,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from) 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) + unsigned char *head, struct strbuf *msgbuf, + struct replay_opts *opts) { struct merge_options o; struct tree *result, *next_tree, *base_tree, *head_tree; @@ -295,7 +314,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, next_tree = next ? next->tree : empty_tree(); base_tree = base ? base->tree : empty_tree(); - for (xopt = xopts; xopt != xopts + xopts_nr; xopt++) + for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++) parse_merge_opt(&o, *xopt); clean = merge_trees(&o, @@ -306,7 +325,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, (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"), me); + die(_("%s: Unable to write new index file"), action_name(opts)); rollback_lock_file(&index_lock); if (!clean) { @@ -335,7 +354,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next, * 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) +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]; @@ -343,9 +362,9 @@ static int run_git_commit(const char *defmsg) args[i++] = "commit"; args[i++] = "-n"; - if (signoff) + if (opts->signoff) args[i++] = "-s"; - if (!edit) { + if (!opts->edit) { args[i++] = "-F"; args[i++] = defmsg; } @@ -354,7 +373,7 @@ static int run_git_commit(const char *defmsg) return run_command_v_opt(args, RUN_GIT_CMD); } -static int do_pick_commit(struct commit *commit) +static int do_pick_commit(struct commit *commit, struct replay_opts *opts) { unsigned char head[20]; struct commit *base, *next, *parent; @@ -364,7 +383,7 @@ static int do_pick_commit(struct commit *commit) struct strbuf msgbuf = STRBUF_INIT; int res; - if (no_commit) { + 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 @@ -377,7 +396,7 @@ static int do_pick_commit(struct commit *commit) if (get_sha1("HEAD", head)) die (_("You do not have a valid HEAD")); if (index_differs_from("HEAD", 0)) - die_dirty_index(me); + die_dirty_index(opts); } discard_cache(); @@ -389,32 +408,32 @@ static int do_pick_commit(struct commit *commit) int cnt; struct commit_list *p; - if (!mainline) + if (!opts->mainline) die(_("Commit %s is a merge but no -m option was given."), sha1_to_hex(commit->object.sha1)); for (cnt = 1, p = commit->parents; - cnt != mainline && p; + cnt != opts->mainline && p; cnt++) p = p->next; - if (cnt != mainline || !p) + if (cnt != opts->mainline || !p) die(_("Commit %s does not have parent %d"), - sha1_to_hex(commit->object.sha1), mainline); + sha1_to_hex(commit->object.sha1), opts->mainline); parent = p->item; - } else if (0 < mainline) + } else if (0 < opts->mainline) die(_("Mainline was specified but commit %s is not a merge."), sha1_to_hex(commit->object.sha1)); else parent = commit->parents->item; - if (allow_ff && parent && !hashcmp(parent->object.sha1, head)) + 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 */ die(_("%s: cannot parse parent commit %s"), - me, sha1_to_hex(parent->object.sha1)); + action_name(opts), sha1_to_hex(parent->object.sha1)); if (get_message(commit, &msg) != 0) die(_("Cannot get commit message for %s"), @@ -429,7 +448,7 @@ static int do_pick_commit(struct commit *commit) defmsg = git_pathdup("MERGE_MSG"); - if (action == REVERT) { + if (opts->action == REVERT) { base = commit; base_label = msg.label; next = parent; @@ -463,18 +482,18 @@ static int do_pick_commit(struct commit *commit) strbuf_addstr(&msgbuf, p); } - if (record_origin) { + 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 (!no_commit) + if (!opts->no_commit) write_cherry_pick_head(commit); } - if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) { + if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) { res = do_recursive_merge(base, next, base_label, next_label, - head, &msgbuf); + head, &msgbuf, opts); write_message(&msgbuf, defmsg); } else { struct commit_list *common = NULL; @@ -484,23 +503,23 @@ static int do_pick_commit(struct commit *commit) commit_list_insert(base, &common); commit_list_insert(next, &remotes); - res = try_merge_command(strategy, xopts_nr, xopts, common, - sha1_to_hex(head), 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 (res) { - error(action == REVERT + error(opts->action == REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV), msg.subject); print_advice(); - rerere(allow_rerere_auto); + rerere(opts->allow_rerere_auto); } else { - if (!no_commit) - res = run_git_commit(defmsg); + if (!opts->no_commit) + res = run_git_commit(defmsg, opts); } free_message(&msg); @@ -509,18 +528,18 @@ static int do_pick_commit(struct commit *commit) return res; } -static void prepare_revs(struct rev_info *revs) +static void prepare_revs(struct rev_info *revs, struct replay_opts *opts) { int argc; init_revisions(revs, NULL); revs->no_walk = 1; - if (action != REVERT) + if (opts->action != REVERT) revs->reverse = 1; - argc = setup_revisions(commit_argc, commit_argv, revs, NULL); + argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL); if (argc > 1) - usage(*revert_or_cherry_pick_usage()); + usage(*revert_or_cherry_pick_usage(opts)); if (prepare_revision_walk(revs)) die(_("revision walk setup failed")); @@ -529,48 +548,48 @@ static void prepare_revs(struct rev_info *revs) die(_("empty commit set passed")); } -static void read_and_refresh_cache(const char *me) +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"), me); + die(_("git %s: failed to read the index"), action_name(opts)); refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL); if (the_index.cache_changed) { if (write_index(&the_index, index_fd) || commit_locked_index(&index_lock)) - die(_("git %s: failed to refresh the index"), me); + die(_("git %s: failed to refresh the index"), action_name(opts)); } rollback_lock_file(&index_lock); } -static int revert_or_cherry_pick(int argc, const char **argv) +static int revert_or_cherry_pick(int argc, const char **argv, + struct replay_opts *opts) { struct rev_info revs; struct commit *commit; git_config(git_default_config, NULL); - me = action == REVERT ? "revert" : "cherry-pick"; - setenv(GIT_REFLOG_ACTION, me, 0); - parse_args(argc, argv); + setenv(GIT_REFLOG_ACTION, action_name(opts), 0); + parse_args(argc, argv, opts); - if (allow_ff) { - if (signoff) + if (opts->allow_ff) { + if (opts->signoff) die(_("cherry-pick --ff cannot be used with --signoff")); - if (no_commit) + if (opts->no_commit) die(_("cherry-pick --ff cannot be used with --no-commit")); - if (record_origin) + if (opts->record_origin) die(_("cherry-pick --ff cannot be used with -x")); - if (edit) + if (opts->edit) die(_("cherry-pick --ff cannot be used with --edit")); } - read_and_refresh_cache(me); + read_and_refresh_cache(opts); - prepare_revs(&revs); + prepare_revs(&revs, opts); while ((commit = get_revision(&revs))) { - int res = do_pick_commit(commit); + int res = do_pick_commit(commit, opts); if (res) return res; } @@ -580,14 +599,20 @@ static int revert_or_cherry_pick(int argc, const char **argv) int cmd_revert(int argc, const char **argv, const char *prefix) { + struct replay_opts opts; + + memset(&opts, 0, sizeof(opts)); if (isatty(0)) - edit = 1; - action = REVERT; - return revert_or_cherry_pick(argc, argv); + opts.edit = 1; + opts.action = REVERT; + return revert_or_cherry_pick(argc, argv, &opts); } int cmd_cherry_pick(int argc, const char **argv, const char *prefix) { - action = CHERRY_PICK; - return revert_or_cherry_pick(argc, argv); + struct replay_opts opts; + + memset(&opts, 0, sizeof(opts)); + opts.action = CHERRY_PICK; + return revert_or_cherry_pick(argc, argv, &opts); } From 89641472aafc28469448688a4ba610cf082921fa Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:06 +0530 Subject: [PATCH 08/19] revert: Separate cmdline parsing from functional code Currently, revert_or_cherry_pick sets up a default git config, parses command-line arguments, before preparing to pick commits. This makes for a bad API as the central worry of callers is to assert whether or not a conflict occured while cherry picking. The current API is like: if (revert_or_cherry_pick(argc, argv, opts) < 0) print "Something failed, we're not sure what" Simplify and rename revert_or_cherry_pick to pick_commits so that it only has the responsibility of setting up the revision walker and picking commits in a loop. Transfer the remaining work to its callers. Now, the API is simplified as: if (parse_args(argc, argv, opts) < 0) print "Can't parse arguments" if (pick_commits(opts) < 0) print "Error encountered in picking machinery" Later in the series, pick_commits will also serve as the starting point for continuing a cherry-pick or revert. Inspired-by: Christian Couder Helped-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 50f72debe0..90fe2efe94 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -563,16 +563,12 @@ static void read_and_refresh_cache(struct replay_opts *opts) rollback_lock_file(&index_lock); } -static int revert_or_cherry_pick(int argc, const char **argv, - struct replay_opts *opts) +static int pick_commits(struct replay_opts *opts) { struct rev_info revs; struct commit *commit; - git_config(git_default_config, NULL); setenv(GIT_REFLOG_ACTION, action_name(opts), 0); - parse_args(argc, argv, opts); - if (opts->allow_ff) { if (opts->signoff) die(_("cherry-pick --ff cannot be used with --signoff")); @@ -605,7 +601,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix) if (isatty(0)) opts.edit = 1; opts.action = REVERT; - return revert_or_cherry_pick(argc, argv, &opts); + git_config(git_default_config, NULL); + parse_args(argc, argv, &opts); + return pick_commits(&opts); } int cmd_cherry_pick(int argc, const char **argv, const char *prefix) @@ -614,5 +612,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) memset(&opts, 0, sizeof(opts)); opts.action = CHERRY_PICK; - return revert_or_cherry_pick(argc, argv, &opts); + git_config(git_default_config, NULL); + parse_args(argc, argv, &opts); + return pick_commits(&opts); } From 9044143ff150a4ccc834523ae2f9ee11c2c961c3 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:07 +0530 Subject: [PATCH 09/19] revert: Don't create invalid replay_opts in parse_args The "--ff" command-line option cannot be used with some other command-line options. However, parse_args still parses these incompatible options into a replay_opts structure for use by the rest of the program. Although pick_commits, the current gatekeeper to the cherry-pick machinery, checks the validity of the replay_opts structure before before starting its operation, there will be multiple entry points to the cherry-pick machinery in future. To futureproof the code and catch these errors in one place, make sure that an invalid replay_opts structure is not created by parse_args in the first place. We still check the replay_opts structure for validity in pick_commits, but this is an assert() now to emphasize that it's the caller's responsibility to get it right. Inspired-by: Christian Couder Mentored-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 90fe2efe94..f75c9cb6c8 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -86,9 +86,26 @@ static int option_parse_x(const struct option *opt, return 0; } +static void verify_opt_compatible(const char *me, const char *base_opt, ...) +{ + const char *this_opt; + va_list ap; + + va_start(ap, base_opt); + while ((this_opt = va_arg(ap, const char *))) { + if (va_arg(ap, int)) + break; + } + va_end(ap); + + if (this_opt) + die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt); +} + static void parse_args(int argc, const char **argv, struct replay_opts *opts) { const char * const * usage_str = revert_or_cherry_pick_usage(opts); + const char *me = action_name(opts); int noop; struct option options[] = { OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"), @@ -122,6 +139,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) if (opts->commit_argc < 2) usage_with_options(usage_str, options); + if (opts->allow_ff) + verify_opt_compatible(me, "--ff", + "--signoff", opts->signoff, + "--no-commit", opts->no_commit, + "-x", opts->record_origin, + "--edit", opts->edit, + NULL); opts->commit_argv = argv; } @@ -569,17 +593,9 @@ static int pick_commits(struct replay_opts *opts) struct commit *commit; setenv(GIT_REFLOG_ACTION, action_name(opts), 0); - if (opts->allow_ff) { - if (opts->signoff) - die(_("cherry-pick --ff cannot be used with --signoff")); - if (opts->no_commit) - die(_("cherry-pick --ff cannot be used with --no-commit")); - if (opts->record_origin) - die(_("cherry-pick --ff cannot be used with -x")); - if (opts->edit) - die(_("cherry-pick --ff cannot be used with --edit")); - } - + if (opts->allow_ff) + assert(!(opts->signoff || opts->no_commit || + opts->record_origin || opts->edit)); read_and_refresh_cache(opts); prepare_revs(&revs, opts); From 04d3d3cfc4a26f003c5ae2b5598cc975a31e4395 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:08 +0530 Subject: [PATCH 10/19] revert: Save data for continuing after conflict resolution Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than one commit, 2010-06-02), a single invocation of "git cherry-pick" or "git revert" can perform picks of several individual commits. To implement features like "--continue" to continue the whole operation, we will need to store some information about the state and the plan at the beginning. Introduce a ".git/sequencer/head" file to store this state, and ".git/sequencer/todo" file to store the plan. The head file contains the SHA-1 of the HEAD before the start of the operation, and the todo file contains an instruction sheet whose format is inspired by the format of the "rebase -i" instruction sheet. As a result, a typical todo file looks like: pick 8537f0e submodule add: test failure when url is not configured pick 4d68932 submodule add: allow relative repository path pick f22a17e submodule add: clean up duplicated code pick 59a5775 make copy_ref globally available Since SHA-1 hex is abbreviated using an find_unique_abbrev(), it is unambiguous. This does not guarantee that there will be no ambiguity when more objects are added to the repository. These two files alone are not enough to implement a "--continue" that remembers the command-line options specified; later patches in the series save them too. These new files are unrelated to the existing .git/CHERRY_PICK_HEAD, which will still be useful while committing after a conflict resolution. Inspired-by: Christian Couder Helped-by: Junio C Hamano Helped-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 134 +++++++++++++++++++++++++++++++- t/t3510-cherry-pick-sequence.sh | 48 ++++++++++++ 2 files changed, 178 insertions(+), 4 deletions(-) create mode 100755 t/t3510-cherry-pick-sequence.sh diff --git a/builtin/revert.c b/builtin/revert.c index f75c9cb6c8..54df8a23b8 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -13,6 +13,7 @@ #include "rerere.h" #include "merge-recursive.h" #include "refs.h" +#include "dir.h" /* * This implements the builtins revert and cherry-pick. @@ -60,6 +61,10 @@ struct replay_opts { #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" +#define SEQ_DIR "sequencer" +#define SEQ_HEAD_FILE "sequencer/head" +#define SEQ_TODO_FILE "sequencer/todo" + static const char *action_name(const struct replay_opts *opts) { return opts->action == REVERT ? "revert" : "cherry-pick"; @@ -587,10 +592,116 @@ static void read_and_refresh_cache(struct replay_opts *opts) rollback_lock_file(&index_lock); } -static int pick_commits(struct replay_opts *opts) +/* + * Append a commit to the end of the commit_list. + * + * next starts by pointing to the variable that holds the head of an + * empty commit_list, and is updated to point to the "next" field of + * the last item on the list as new commits are appended. + * + * Usage example: + * + * struct commit_list *list; + * struct commit_list **next = &list; + * + * next = commit_list_append(c1, next); + * next = commit_list_append(c2, next); + * assert(commit_list_count(list) == 2); + * return list; + */ +struct commit_list **commit_list_append(struct commit *commit, + struct commit_list **next) +{ + struct commit_list *new = xmalloc(sizeof(struct commit_list)); + new->item = commit; + *next = new; + new->next = NULL; + return &new->next; +} + +static int format_todo(struct strbuf *buf, struct commit_list *todo_list, + struct replay_opts *opts) +{ + struct commit_list *cur = NULL; + struct commit_message msg = { NULL, NULL, NULL, NULL, NULL }; + const char *sha1_abbrev = NULL; + const char *action_str = opts->action == REVERT ? "revert" : "pick"; + + for (cur = todo_list; cur; cur = cur->next) { + sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV); + if (get_message(cur->item, &msg)) + return error(_("Cannot get commit message for %s"), sha1_abbrev); + strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject); + } + return 0; +} + +static void walk_revs_populate_todo(struct commit_list **todo_list, + struct replay_opts *opts) { struct rev_info revs; struct commit *commit; + struct commit_list **next; + + prepare_revs(&revs, opts); + + next = todo_list; + while ((commit = get_revision(&revs))) + next = commit_list_append(commit, next); +} + +static void create_seq_dir(void) +{ + const char *seq_dir = git_path(SEQ_DIR); + + if (!(file_exists(seq_dir) && is_directory(seq_dir)) + && mkdir(seq_dir, 0777) < 0) + die_errno(_("Could not create sequencer directory '%s'."), seq_dir); +} + +static void save_head(const char *head) +{ + const char *head_file = git_path(SEQ_HEAD_FILE); + static struct lock_file head_lock; + struct strbuf buf = STRBUF_INIT; + int fd; + + fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR); + strbuf_addf(&buf, "%s\n", head); + if (write_in_full(fd, buf.buf, buf.len) < 0) + die_errno(_("Could not write to %s."), head_file); + if (commit_lock_file(&head_lock) < 0) + die(_("Error wrapping up %s."), head_file); +} + +static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) +{ + const char *todo_file = git_path(SEQ_TODO_FILE); + static struct lock_file todo_lock; + struct strbuf buf = STRBUF_INIT; + int fd; + + fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR); + if (format_todo(&buf, todo_list, opts) < 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 int pick_commits(struct replay_opts *opts) +{ + struct commit_list *todo_list = NULL; + struct strbuf buf = STRBUF_INIT; + unsigned char sha1[20]; + struct commit_list *cur; + int res; setenv(GIT_REFLOG_ACTION, action_name(opts), 0); if (opts->allow_ff) @@ -598,14 +709,29 @@ static int pick_commits(struct replay_opts *opts) opts->record_origin || opts->edit)); read_and_refresh_cache(opts); - prepare_revs(&revs, opts); + walk_revs_populate_todo(&todo_list, opts); + create_seq_dir(); + if (get_sha1("HEAD", sha1)) { + if (opts->action == REVERT) + die(_("Can't revert as initial commit")); + die(_("Can't cherry-pick into empty head")); + } + save_head(sha1_to_hex(sha1)); - while ((commit = get_revision(&revs))) { - int res = do_pick_commit(commit, opts); + for (cur = todo_list; cur; cur = cur->next) { + save_todo(cur, opts); + res = do_pick_commit(cur->item, opts); if (res) return res; } + /* + * Sequence of picks finished successfully; cleanup by + * removing the .git/sequencer directory + */ + strbuf_addf(&buf, "%s", git_path(SEQ_DIR)); + remove_dir_recursively(&buf, 0); + strbuf_release(&buf); return 0; } diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh new file mode 100755 index 0000000000..a2c70ad4b8 --- /dev/null +++ b/t/t3510-cherry-pick-sequence.sh @@ -0,0 +1,48 @@ +#!/bin/sh + +test_description='Test cherry-pick continuation features + + + anotherpick: rewrites foo to d + + picked: rewrites foo to c + + unrelatedpick: rewrites unrelated to reallyunrelated + + base: rewrites foo to b + + initial: writes foo as a, unrelated as unrelated + +' + +. ./test-lib.sh + +pristine_detach () { + rm -rf .git/sequencer && + git checkout -f "$1^0" && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x +} + +test_expect_success setup ' + echo unrelated >unrelated && + git add unrelated && + test_commit initial foo a && + test_commit base foo b && + test_commit unrelatedpick unrelated reallyunrelated && + test_commit picked foo c && + test_commit anotherpick foo d && + git config advice.detachedhead false + +' + +test_expect_success 'cherry-pick persists data on failure' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + test_path_is_dir .git/sequencer && + test_path_is_file .git/sequencer/head && + test_path_is_file .git/sequencer/todo +' + +test_expect_success 'cherry-pick cleans up sequencer state upon success' ' + pristine_detach initial && + git cherry-pick initial..picked && + test_path_is_missing .git/sequencer +' + +test_done From 6f0322633b2659d26d1c7b20d4af1fba33978690 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:09 +0530 Subject: [PATCH 11/19] revert: Save command-line options for continuing operation In the same spirit as ".git/sequencer/head" and ".git/sequencer/todo", introduce ".git/sequencer/opts" to persist the replay_opts structure for continuing after a conflict resolution. Use the gitconfig format for this file so that it looks like: [options] signoff = true record-origin = true mainline = 1 strategy = recursive strategy-option = patience strategy-option = ours Helped-by: Jonathan Nieder Helped-by: Christian Couder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 33 +++++++++++++++++++++++++++++++++ t/t3510-cherry-pick-sequence.sh | 29 +++++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 54df8a23b8..308c1ce2ed 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -64,6 +64,7 @@ struct replay_opts { #define SEQ_DIR "sequencer" #define SEQ_HEAD_FILE "sequencer/head" #define SEQ_TODO_FILE "sequencer/todo" +#define SEQ_OPTS_FILE "sequencer/opts" static const char *action_name(const struct replay_opts *opts) { @@ -695,6 +696,37 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) 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_opts *opts) { struct commit_list *todo_list = NULL; @@ -717,6 +749,7 @@ static int pick_commits(struct replay_opts *opts) die(_("Can't cherry-pick into empty head")); } save_head(sha1_to_hex(sha1)); + save_opts(opts); for (cur = todo_list; cur; cur = cur->next) { save_todo(cur, opts); diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index a2c70ad4b8..8ee000180a 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -33,10 +33,35 @@ test_expect_success setup ' test_expect_success 'cherry-pick persists data on failure' ' pristine_detach initial && - test_must_fail git cherry-pick base..anotherpick && + test_must_fail git cherry-pick -s base..anotherpick && test_path_is_dir .git/sequencer && test_path_is_file .git/sequencer/head && - test_path_is_file .git/sequencer/todo + test_path_is_file .git/sequencer/todo && + test_path_is_file .git/sequencer/opts +' + +test_expect_success 'cherry-pick persists opts correctly' ' + pristine_detach initial && + test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick && + test_path_is_dir .git/sequencer && + test_path_is_file .git/sequencer/head && + test_path_is_file .git/sequencer/todo && + test_path_is_file .git/sequencer/opts && + echo "true" >expect && + git config --file=.git/sequencer/opts --get-all options.signoff >actual && + test_cmp expect actual && + echo "1" >expect && + git config --file=.git/sequencer/opts --get-all options.mainline >actual && + test_cmp expect actual && + echo "recursive" >expect && + git config --file=.git/sequencer/opts --get-all options.strategy >actual && + test_cmp expect actual && + cat >expect <<-\EOF && + patience + ours + EOF + git config --file=.git/sequencer/opts --get-all options.strategy-option >actual && + test_cmp expect actual ' test_expect_success 'cherry-pick cleans up sequencer state upon success' ' From 21b14778a9b033d80b3e5757d9576da13ba446cd Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:10 +0530 Subject: [PATCH 12/19] revert: Make pick_commits functionally act on a commit list Apart from its central objective of calling into the picking mechanism, pick_commits creates a sequencer directory, prepares a todo list, and even acts upon the "--reset" subcommand. This makes for a bad API since the central worry of callers is to figure out whether or not any conflicts were encountered during the cherry picking. The current API is like: if (pick_commits(opts) < 0) print "Something failed, we're not sure what" So, change pick_commits so that it's only responsible for picking commits in a loop and reporting any errors, leaving the rest to a new function called pick_revisions. Consequently, the API of pick_commits becomes much clearer: act_on_subcommand(opts->subcommand); todo_list = prepare_todo_list(); if (pick_commits(todo_list, opts) < 0) print "Error encountered while picking commits" Now, callers can easily call-in to the cherry-picking machinery by constructing an arbitrary todo list along with some options. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 43 ++++++++++++++++++++++++++++--------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 308c1ce2ed..f088e1cc71 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -727,11 +727,9 @@ static void save_opts(struct replay_opts *opts) } } -static int pick_commits(struct replay_opts *opts) +static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) { - struct commit_list *todo_list = NULL; struct strbuf buf = STRBUF_INIT; - unsigned char sha1[20]; struct commit_list *cur; int res; @@ -741,16 +739,6 @@ static int pick_commits(struct replay_opts *opts) opts->record_origin || opts->edit)); read_and_refresh_cache(opts); - walk_revs_populate_todo(&todo_list, opts); - create_seq_dir(); - if (get_sha1("HEAD", sha1)) { - if (opts->action == REVERT) - die(_("Can't revert as initial commit")); - die(_("Can't cherry-pick into empty head")); - } - save_head(sha1_to_hex(sha1)); - save_opts(opts); - for (cur = todo_list; cur; cur = cur->next) { save_todo(cur, opts); res = do_pick_commit(cur->item, opts); @@ -768,6 +756,31 @@ static int pick_commits(struct replay_opts *opts) return 0; } +static int pick_revisions(struct replay_opts *opts) +{ + struct commit_list *todo_list = NULL; + unsigned char sha1[20]; + + 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 + */ + walk_revs_populate_todo(&todo_list, opts); + create_seq_dir(); + if (get_sha1("HEAD", sha1)) { + if (opts->action == REVERT) + die(_("Can't revert as initial commit")); + die(_("Can't cherry-pick into empty head")); + } + 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; @@ -778,7 +791,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) opts.action = REVERT; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); - return pick_commits(&opts); + return pick_revisions(&opts); } int cmd_cherry_pick(int argc, const char **argv, const char *prefix) @@ -789,5 +802,5 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) opts.action = CHERRY_PICK; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); - return pick_commits(&opts); + return pick_revisions(&opts); } From 26ae337be11e440420d8ec7ce415425daaabe573 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:11 +0530 Subject: [PATCH 13/19] revert: Introduce --reset to remove sequencer state To explicitly remove the sequencer state for a fresh cherry-pick or revert invocation, introduce a new subcommand called "--reset" to remove the sequencer state. Take the opportunity to publicly expose the sequencer paths, and a generic function called "remove_sequencer_state" that various git programs can use to remove the sequencer state in a uniform manner; "git reset" uses it later in this series. Introducing this public API is also in line with our long-term goal of eventually factoring out functions from revert.c into a generic commit sequencer. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Documentation/git-cherry-pick.txt | 5 +++ Documentation/git-revert.txt | 5 +++ Documentation/sequencer.txt | 4 ++ Makefile | 2 + builtin/revert.c | 62 +++++++++++++++++++++---------- sequencer.c | 19 ++++++++++ sequencer.h | 20 ++++++++++ t/t3510-cherry-pick-sequence.sh | 14 ++++++- 8 files changed, 111 insertions(+), 20 deletions(-) create mode 100644 Documentation/sequencer.txt create mode 100644 sequencer.c create mode 100644 sequencer.h diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 9d8fe0d261..138a292849 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -8,6 +8,7 @@ git-cherry-pick - Apply the changes introduced by some existing commits SYNOPSIS -------- 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] ... +'git cherry-pick' --reset DESCRIPTION ----------- @@ -109,6 +110,10 @@ effect to your index in a row. Pass the merge strategy-specific option through to the merge strategy. See linkgit:git-merge[1] for details. +SEQUENCER SUBCOMMANDS +--------------------- +include::sequencer.txt[] + EXAMPLES -------- git cherry-pick master:: diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index ac10cfbb14..783ffb4a68 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -8,6 +8,7 @@ git-revert - Revert some existing commits SYNOPSIS -------- 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] ... +'git revert' --reset DESCRIPTION ----------- @@ -90,6 +91,10 @@ effect to your index in a row. Pass the merge strategy-specific option through to the merge strategy. See linkgit:git-merge[1] for details. +SEQUENCER SUBCOMMANDS +--------------------- +include::sequencer.txt[] + EXAMPLES -------- git revert HEAD~3:: diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt new file mode 100644 index 0000000000..16ce88ce6b --- /dev/null +++ b/Documentation/sequencer.txt @@ -0,0 +1,4 @@ +--reset:: + Forget about the current operation in progress. Can be used + to clear the sequencer state after a failed cherry-pick or + revert. diff --git a/Makefile b/Makefile index e40ac0c7f5..f69eef254c 100644 --- a/Makefile +++ b/Makefile @@ -551,6 +551,7 @@ LIB_H += rerere.h LIB_H += resolve-undo.h LIB_H += revision.h LIB_H += run-command.h +LIB_H += sequencer.h LIB_H += sha1-array.h LIB_H += sha1-lookup.h LIB_H += sideband.h @@ -654,6 +655,7 @@ LIB_OBJS += revision.o LIB_OBJS += run-command.o LIB_OBJS += server-info.o LIB_OBJS += setup.o +LIB_OBJS += sequencer.o LIB_OBJS += sha1-array.o LIB_OBJS += sha1-lookup.o LIB_OBJS += sha1_file.o diff --git a/builtin/revert.c b/builtin/revert.c index f088e1cc71..a8accd6c7a 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -14,6 +14,7 @@ #include "merge-recursive.h" #include "refs.h" #include "dir.h" +#include "sequencer.h" /* * This implements the builtins revert and cherry-pick. @@ -28,18 +29,22 @@ static const char * const revert_usage[] = { "git revert [options] ", + "git revert ", NULL }; static const char * const cherry_pick_usage[] = { "git cherry-pick [options] ", + "git cherry-pick ", NULL }; enum replay_action { REVERT, CHERRY_PICK }; +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET }; struct replay_opts { enum replay_action action; + enum replay_subcommand subcommand; /* Boolean options */ int edit; @@ -61,11 +66,6 @@ struct replay_opts { #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" -#define SEQ_DIR "sequencer" -#define SEQ_HEAD_FILE "sequencer/head" -#define SEQ_TODO_FILE "sequencer/todo" -#define SEQ_OPTS_FILE "sequencer/opts" - static const char *action_name(const struct replay_opts *opts) { return opts->action == REVERT ? "revert" : "cherry-pick"; @@ -113,7 +113,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); int noop; + int reset = 0; struct option options[] = { + OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"), OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"), OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"), { OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)", @@ -142,7 +144,27 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str, PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN); - if (opts->commit_argc < 2) + + /* Set the subcommand */ + if (reset) + opts->subcommand = REPLAY_RESET; + else + opts->subcommand = REPLAY_NONE; + + /* Check for incompatible command line arguments */ + if (opts->subcommand == REPLAY_RESET) { + verify_opt_compatible(me, "--reset", + "--no-commit", opts->no_commit, + "--signoff", opts->signoff, + "--mainline", opts->mainline, + "--strategy", opts->strategy ? 1 : 0, + "--strategy-option", opts->xopts ? 1 : 0, + "-x", opts->record_origin, + "--ff", opts->allow_ff, + NULL); + } + + else if (opts->commit_argc < 2) usage_with_options(usage_str, options); if (opts->allow_ff) @@ -729,7 +751,6 @@ static void save_opts(struct replay_opts *opts) static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) { - struct strbuf buf = STRBUF_INIT; struct commit_list *cur; int res; @@ -750,9 +771,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) * Sequence of picks finished successfully; cleanup by * removing the .git/sequencer directory */ - strbuf_addf(&buf, "%s", git_path(SEQ_DIR)); - remove_dir_recursively(&buf, 0); - strbuf_release(&buf); + remove_sequencer_state(1); return 0; } @@ -768,16 +787,21 @@ static int pick_revisions(struct replay_opts *opts) * cherry-pick should be handled differently from an existing * one that is being continued */ - walk_revs_populate_todo(&todo_list, opts); - create_seq_dir(); - if (get_sha1("HEAD", sha1)) { - if (opts->action == REVERT) - die(_("Can't revert as initial commit")); - die(_("Can't cherry-pick into empty head")); + if (opts->subcommand == REPLAY_RESET) { + remove_sequencer_state(1); + return 0; + } else { + /* Start a new cherry-pick/ revert sequence */ + walk_revs_populate_todo(&todo_list, opts); + create_seq_dir(); + if (get_sha1("HEAD", sha1)) { + if (opts->action == REVERT) + die(_("Can't revert as initial commit")); + die(_("Can't cherry-pick into empty head")); + } + save_head(sha1_to_hex(sha1)); + save_opts(opts); } - save_head(sha1_to_hex(sha1)); - save_opts(opts); - return pick_commits(todo_list, opts); } diff --git a/sequencer.c b/sequencer.c new file mode 100644 index 0000000000..bc2c046aab --- /dev/null +++ b/sequencer.c @@ -0,0 +1,19 @@ +#include "cache.h" +#include "sequencer.h" +#include "strbuf.h" +#include "dir.h" + +void remove_sequencer_state(int aggressive) +{ + struct strbuf seq_dir = STRBUF_INIT; + struct strbuf seq_old_dir = STRBUF_INIT; + + strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR)); + strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR)); + remove_dir_recursively(&seq_old_dir, 0); + rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR)); + if (aggressive) + remove_dir_recursively(&seq_old_dir, 0); + strbuf_release(&seq_dir); + strbuf_release(&seq_old_dir); +} diff --git a/sequencer.h b/sequencer.h new file mode 100644 index 0000000000..905d295012 --- /dev/null +++ b/sequencer.h @@ -0,0 +1,20 @@ +#ifndef SEQUENCER_H +#define SEQUENCER_H + +#define SEQ_DIR "sequencer" +#define SEQ_OLD_DIR "sequencer-old" +#define SEQ_HEAD_FILE "sequencer/head" +#define SEQ_TODO_FILE "sequencer/todo" +#define SEQ_OPTS_FILE "sequencer/opts" + +/* + * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring + * any errors. Intended to be used by 'git reset'. + * + * With the aggressive flag, it additionally removes SEQ_OLD_DIR, + * ignoring any errors. Inteded to be used by the sequencer's + * '--reset' subcommand. + */ +void remove_sequencer_state(int aggressive); + +#endif diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 8ee000180a..fd6986541a 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -13,7 +13,7 @@ test_description='Test cherry-pick continuation features . ./test-lib.sh pristine_detach () { - rm -rf .git/sequencer && + git cherry-pick --reset && git checkout -f "$1^0" && git read-tree -u --reset HEAD && git clean -d -f -f -q -x @@ -70,4 +70,16 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' ' test_path_is_missing .git/sequencer ' +test_expect_success '--reset does not complain when no cherry-pick is in progress' ' + pristine_detach initial && + git cherry-pick --reset +' + +test_expect_success '--reset cleans up sequencer state' ' + pristine_detach initial && + test_must_fail git cherry-pick base..picked && + git cherry-pick --reset && + test_path_is_missing .git/sequencer +' + test_done From 95eb88d8ee588d89b4f06d2753ed4d16ab13b39f Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:12 +0530 Subject: [PATCH 14/19] reset: Make reset remove the sequencer state Years of muscle memory have trained users to use "git reset --hard" to remove the branch state after any sort operation. Make it also remove the sequencer state to facilitate this established workflow: $ git cherry-pick foo..bar ... conflict encountered ... $ git reset --hard # Oops, I didn't mean that $ git cherry-pick quux..bar ... cherry-pick succeeded ... Guard against accidental removal of the sequencer state by providing one level of "undo". In the first "reset" invocation, ".git/sequencer" is moved to ".git/sequencer-old"; it is completely removed only in the second invocation. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- branch.c | 2 ++ t/t7106-reset-sequence.sh | 44 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100755 t/t7106-reset-sequence.sh diff --git a/branch.c b/branch.c index c0c865a4b1..d06aec4637 100644 --- a/branch.c +++ b/branch.c @@ -3,6 +3,7 @@ #include "refs.h" #include "remote.h" #include "commit.h" +#include "sequencer.h" struct tracking { struct refspec spec; @@ -228,4 +229,5 @@ void remove_branch_state(void) unlink(git_path("MERGE_MSG")); unlink(git_path("MERGE_MODE")); unlink(git_path("SQUASH_MSG")); + remove_sequencer_state(0); } diff --git a/t/t7106-reset-sequence.sh b/t/t7106-reset-sequence.sh new file mode 100755 index 0000000000..4956caaf82 --- /dev/null +++ b/t/t7106-reset-sequence.sh @@ -0,0 +1,44 @@ +#!/bin/sh + +test_description='Test interaction of reset --hard with sequencer + + + anotherpick: rewrites foo to d + + picked: rewrites foo to c + + unrelatedpick: rewrites unrelated to reallyunrelated + + base: rewrites foo to b + + initial: writes foo as a, unrelated as unrelated +' + +. ./test-lib.sh + +pristine_detach () { + git cherry-pick --reset && + git checkout -f "$1^0" && + git read-tree -u --reset HEAD && + git clean -d -f -f -q -x +} + +test_expect_success setup ' + echo unrelated >unrelated && + git add unrelated && + test_commit initial foo a && + test_commit base foo b && + test_commit unrelatedpick unrelated reallyunrelated && + test_commit picked foo c && + test_commit anotherpick foo d && + git config advice.detachedhead false + +' + +test_expect_success 'reset --hard cleans up sequencer state, providing one-level undo' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + test_path_is_dir .git/sequencer && + git reset --hard && + test_path_is_missing .git/sequencer && + test_path_is_dir .git/sequencer-old && + git reset --hard && + test_path_is_missing .git/sequencer-old +' + +test_done From 2d27daa91da1d6376916533de2ec4b50e6acc925 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:13 +0530 Subject: [PATCH 15/19] revert: Remove sequencer state when no commits are pending When cherry-pick or revert is called on a list of commits, and a conflict encountered somewhere in the middle, the data in ".git/sequencer" is required to continue the operation. However, when a conflict is encountered in the very last commit, the user will have to "continue" after resolving the conflict and committing just so that the sequencer state is removed. This is how the current "rebase -i" script works as well. $ git cherry-pick foo..bar ... conflict encountered while picking "bar" ... $ echo "resolved" >problematicfile $ git add problematicfile $ git commit $ git cherry-pick --continue # This would be a no-op Change this so that the sequencer state is cleared when a conflict is encountered in the last commit. Incidentally, this patch makes sure that some existing tests don't break when features like "--reset" and "--continue" are implemented later in the series. A better way to implement this feature is to get the last "git commit" to remove the sequencer state. However, that requires tighter coupling between "git commit" and the sequencer, a goal that can be pursued once the sequencer is made more general. Signed-off-by: Ramkumar Ramachandra Acked-by: Jonathan Nieder Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 12 +++++++++++- t/t3510-cherry-pick-sequence.sh | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/builtin/revert.c b/builtin/revert.c index a8accd6c7a..000806c7c4 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -763,8 +763,18 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) for (cur = todo_list; cur; cur = cur->next) { save_todo(cur, opts); res = do_pick_commit(cur->item, opts); - if (res) + if (res) { + if (!cur->next) + /* + * An error was encountered while + * picking the last commit; the + * sequencer state is useless now -- + * the user simply needs to resolve + * the conflict and commit + */ + remove_sequencer_state(0); return res; + } } /* diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index fd6986541a..a414086578 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -82,4 +82,28 @@ test_expect_success '--reset cleans up sequencer state' ' test_path_is_missing .git/sequencer ' +test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' ' + pristine_detach initial && + test_must_fail git cherry-pick base..picked && + test_path_is_missing .git/sequencer && + echo "resolved" >foo && + git add foo && + git commit && + { + git rev-list HEAD | + git diff-tree --root --stdin | + sed "s/$_x40/OBJID/g" + } >actual && + cat >expect <<-\EOF && + OBJID + :100644 100644 OBJID OBJID M foo + OBJID + :100644 100644 OBJID OBJID M unrelated + OBJID + :000000 100644 OBJID OBJID A foo + :000000 100644 OBJID OBJID A unrelated + EOF + test_cmp expect actual +' + test_done From 21afd0806205d4a41c7c6076bd049842779ec080 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:14 +0530 Subject: [PATCH 16/19] revert: Don't implicitly stomp pending sequencer operation Protect the user from forgetting about a pending sequencer operation by immediately erroring out when an existing cherry-pick or revert operation is in progress like: $ git cherry-pick foo ... conflict ... $ git cherry-pick moo error: .git/sequencer already exists hint: A cherry-pick or revert is in progress hint: Use --reset to forget about it fatal: cherry-pick failed A naive version of this would break the following established ways of working: $ git cherry-pick foo ... conflict ... $ git reset --hard # I actually meant "moo" when I said "foo" $ git cherry-pick moo $ git cherry-pick foo ... conflict ... $ git commit # commit the resolution $ git cherry-pick moo # New operation However, the previous patches "reset: Make reset remove the sequencer state" and "revert: Remove sequencer state when no commits are pending" make sure that this does not happen. Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 30 +++++++++++++++++++++++++----- t/t3510-cherry-pick-sequence.sh | 9 +++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 000806c7c4..19b6739104 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -66,6 +66,15 @@ struct replay_opts { #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" +static void fatal(const char *advice, ...) +{ + va_list params; + + va_start(params, advice); + vreportf("fatal: ", advice, params); + va_end(params); +} + static const char *action_name(const struct replay_opts *opts) { return opts->action == REVERT ? "revert" : "cherry-pick"; @@ -673,13 +682,15 @@ static void walk_revs_populate_todo(struct commit_list **todo_list, next = commit_list_append(commit, next); } -static void create_seq_dir(void) +static int create_seq_dir(void) { const char *seq_dir = git_path(SEQ_DIR); - if (!(file_exists(seq_dir) && is_directory(seq_dir)) - && mkdir(seq_dir, 0777) < 0) + if (file_exists(seq_dir)) + return error(_("%s already exists."), seq_dir); + else if (mkdir(seq_dir, 0777) < 0) die_errno(_("Could not create sequencer directory '%s'."), seq_dir); + return 0; } static void save_head(const char *head) @@ -801,9 +812,18 @@ static int pick_revisions(struct replay_opts *opts) remove_sequencer_state(1); return 0; } else { - /* Start a new cherry-pick/ revert sequence */ + /* + * 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); - create_seq_dir(); + if (create_seq_dir() < 0) { + fatal(_("A cherry-pick or revert is in progress.")); + advise(_("Use --reset to forget about it")); + exit(128); + } if (get_sha1("HEAD", sha1)) { if (opts->action == REVERT) die(_("Can't revert as initial commit")); diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index a414086578..566a15ed8a 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -106,4 +106,13 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is le test_cmp expect actual ' +test_expect_success 'cherry-pick does not implicitly stomp an existing operation' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + test-chmtime -v +0 .git/sequencer >expect && + test_must_fail git cherry-pick unrelatedpick && + test-chmtime -v +0 .git/sequencer >actual && + test_cmp expect actual +' + test_done From 5a5d80f4ca16fdb1f2577474ad94135839853a3e Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:15 +0530 Subject: [PATCH 17/19] revert: Introduce --continue to continue the operation Introduce a new "git cherry-pick --continue" command which uses the information in ".git/sequencer" to continue a cherry-pick that stopped because of a conflict or other error. It works by dropping the first instruction from .git/sequencer/todo and performing the remaining cherry-picks listed there, with options (think "-s" and "-X") from the initial command listed in ".git/sequencer/opts". So now you can do: $ git cherry-pick -Xpatience foo..bar ... description conflict in commit moo ... $ git cherry-pick --continue error: 'cherry-pick' is not possible because you have unmerged files. fatal: failed to resume cherry-pick $ echo resolved >conflictingfile $ git add conflictingfile && git commit $ git cherry-pick --continue; # resumes with the commit after "moo" During the "git commit" stage, CHERRY_PICK_HEAD will aid by providing the commit message from the conflicting "moo" commit. Note that the cherry-pick mechanism has no control at this stage, so the user is free to violate anything that was specified during the first cherry-pick invocation. For example, if "-x" was specified during the first cherry-pick invocation, the user is free to edit out the message during commit time. Note that the "--signoff" option specified at cherry-pick invocation time is not reflected in the commit message provided by CHERRY_PICK_HEAD; the user must take care to add "--signoff" during the "git commit" invocation. Helped-by: Christian Couder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- Documentation/git-cherry-pick.txt | 1 + Documentation/git-revert.txt | 1 + Documentation/sequencer.txt | 5 + builtin/revert.c | 188 +++++++++++++++++++++++++++++- t/t3510-cherry-pick-sequence.sh | 96 +++++++++++++++ 5 files changed, 287 insertions(+), 4 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 138a292849..663186bda7 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -9,6 +9,7 @@ SYNOPSIS -------- 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] ... 'git cherry-pick' --reset +'git cherry-pick' --continue DESCRIPTION ----------- diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt index 783ffb4a68..9be2fe2b2f 100644 --- a/Documentation/git-revert.txt +++ b/Documentation/git-revert.txt @@ -9,6 +9,7 @@ SYNOPSIS -------- 'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] ... 'git revert' --reset +'git revert' --continue DESCRIPTION ----------- diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt index 16ce88ce6b..3e6df338be 100644 --- a/Documentation/sequencer.txt +++ b/Documentation/sequencer.txt @@ -2,3 +2,8 @@ Forget about the current operation in progress. Can be used to clear the sequencer state after a failed cherry-pick or revert. + +--continue:: + Continue the operation in progress using the information in + '.git/sequencer'. Can be used to continue after resolving + conflicts in a failed cherry-pick or revert. diff --git a/builtin/revert.c b/builtin/revert.c index 19b6739104..4f0d8f1733 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -40,7 +40,7 @@ static const char * const cherry_pick_usage[] = { }; enum replay_action { REVERT, CHERRY_PICK }; -enum replay_subcommand { REPLAY_NONE, REPLAY_RESET }; +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE }; struct replay_opts { enum replay_action action; @@ -117,14 +117,37 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...) die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt); } +static void verify_opt_mutually_compatible(const char *me, ...) +{ + const char *opt1, *opt2; + va_list ap; + + va_start(ap, me); + while ((opt1 = va_arg(ap, const char *))) { + if (va_arg(ap, int)) + break; + } + if (opt1) { + while ((opt2 = va_arg(ap, const char *))) { + if (va_arg(ap, int)) + break; + } + } + + if (opt1 && opt2) + die(_("%s: %s cannot be used with %s"), me, opt1, opt2); +} + static void parse_args(int argc, const char **argv, struct replay_opts *opts) { const char * const * usage_str = revert_or_cherry_pick_usage(opts); const char *me = action_name(opts); int noop; int reset = 0; + int contin = 0; struct option options[] = { OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"), + OPT_BOOLEAN(0, "continue", &contin, "continue the current operation"), OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"), OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"), { OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)", @@ -154,15 +177,29 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN); + /* Check for incompatible subcommands */ + verify_opt_mutually_compatible(me, + "--reset", reset, + "--continue", contin, + NULL); + /* Set the subcommand */ if (reset) opts->subcommand = REPLAY_RESET; + else if (contin) + opts->subcommand = REPLAY_CONTINUE; else opts->subcommand = REPLAY_NONE; /* Check for incompatible command line arguments */ - if (opts->subcommand == REPLAY_RESET) { - verify_opt_compatible(me, "--reset", + if (opts->subcommand != REPLAY_NONE) { + char *this_operation; + if (opts->subcommand == REPLAY_RESET) + this_operation = "--reset"; + else + this_operation = "--continue"; + + verify_opt_compatible(me, this_operation, "--no-commit", opts->no_commit, "--signoff", opts->signoff, "--mainline", opts->mainline, @@ -668,6 +705,137 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, return 0; } +static struct commit *parse_insn_line(char *start, struct replay_opts *opts) +{ + unsigned char commit_sha1[20]; + char sha1_abbrev[40]; + enum replay_action action; + int insn_len = 0; + char *p, *q; + + if (!prefixcmp(start, "pick ")) { + action = CHERRY_PICK; + insn_len = strlen("pick"); + p = start + insn_len + 1; + } else if (!prefixcmp(start, "revert ")) { + action = REVERT; + insn_len = strlen("revert"); + p = start + insn_len + 1; + } else + return NULL; + + q = strchr(p, ' '); + if (!q) + return NULL; + q++; + + strlcpy(sha1_abbrev, p, q - p); + + /* + * Verify that the action matches up with the one in + * opts; we don't support arbitrary instructions + */ + if (action != opts->action) { + const char *action_str; + action_str = action == REVERT ? "revert" : "cherry-pick"; + error(_("Cannot %s during a %s"), action_str, action_name(opts)); + return NULL; + } + + if (get_sha1(sha1_abbrev, commit_sha1) < 0) + return NULL; + + return lookup_commit_reference(commit_sha1); +} + +static int parse_insn_buffer(char *buf, struct commit_list **todo_list, + struct replay_opts *opts) +{ + struct commit_list **next = todo_list; + struct commit *commit; + char *p = buf; + int i; + + for (i = 1; *p; i++) { + commit = parse_insn_line(p, opts); + if (!commit) + return error(_("Could not parse line %d."), i); + next = commit_list_append(commit, next); + p = strchrnul(p, '\n'); + if (*p) + p++; + } + if (!*todo_list) + return error(_("No commits parsed.")); + return 0; +} + +static void read_populate_todo(struct commit_list **todo_list, + struct replay_opts *opts) +{ + const char *todo_file = git_path(SEQ_TODO_FILE); + struct strbuf buf = STRBUF_INIT; + int fd, res; + + fd = open(todo_file, O_RDONLY); + 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, opts); + 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 commit_list **todo_list, struct replay_opts *opts) { @@ -811,6 +979,15 @@ static int pick_revisions(struct replay_opts *opts) if (opts->subcommand == REPLAY_RESET) { remove_sequencer_state(1); return 0; + } else if (opts->subcommand == REPLAY_CONTINUE) { + if (!file_exists(git_path(SEQ_TODO_FILE))) + goto error; + read_populate_opts(&opts); + read_populate_todo(&todo_list, opts); + + /* Verify that the conflict has been resolved */ + if (!index_differs_from("HEAD", 0)) + todo_list = todo_list->next; } else { /* * Start a new cherry-pick/ revert sequence; but @@ -821,7 +998,8 @@ static int pick_revisions(struct replay_opts *opts) walk_revs_populate_todo(&todo_list, opts); if (create_seq_dir() < 0) { fatal(_("A cherry-pick or revert is in progress.")); - advise(_("Use --reset to forget about it")); + advise(_("Use --continue to continue the operation")); + advise(_("or --reset to forget about it")); exit(128); } if (get_sha1("HEAD", sha1)) { @@ -833,6 +1011,8 @@ static int pick_revisions(struct replay_opts *opts) save_opts(opts); } return pick_commits(todo_list, opts); +error: + die(_("No %s in progress"), action_name(opts)); } int cmd_revert(int argc, const char **argv, const char *prefix) diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 566a15ed8a..3bca2b3dd5 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -115,4 +115,100 @@ test_expect_success 'cherry-pick does not implicitly stomp an existing operation test_cmp expect actual ' +test_expect_success '--continue complains when no cherry-pick is in progress' ' + pristine_detach initial && + test_must_fail git cherry-pick --continue +' + +test_expect_success '--continue complains when there are unresolved conflicts' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + test_must_fail git cherry-pick --continue +' + +test_expect_success '--continue continues after conflicts are resolved' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + git cherry-pick --continue && + test_path_is_missing .git/sequencer && + { + git rev-list HEAD | + git diff-tree --root --stdin | + sed "s/$_x40/OBJID/g" + } >actual && + cat >expect <<-\EOF && + OBJID + :100644 100644 OBJID OBJID M foo + OBJID + :100644 100644 OBJID OBJID M foo + OBJID + :100644 100644 OBJID OBJID M unrelated + OBJID + :000000 100644 OBJID OBJID A foo + :000000 100644 OBJID OBJID A unrelated + EOF + test_cmp expect actual +' + +test_expect_success '--continue respects opts' ' + pristine_detach initial && + test_must_fail git cherry-pick -x base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + git cherry-pick --continue && + test_path_is_missing .git/sequencer && + git cat-file commit HEAD >anotherpick_msg && + git cat-file commit HEAD~1 >picked_msg && + git cat-file commit HEAD~2 >unrelatedpick_msg && + git cat-file commit HEAD~3 >initial_msg && + test_must_fail grep "cherry picked from" initial_msg && + grep "cherry picked from" unrelatedpick_msg && + grep "cherry picked from" picked_msg && + grep "cherry picked from" anotherpick_msg +' + +test_expect_success '--signoff is not automatically propagated to resolved conflict' ' + pristine_detach initial && + test_must_fail git cherry-pick --signoff base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + git cherry-pick --continue && + test_path_is_missing .git/sequencer && + git cat-file commit HEAD >anotherpick_msg && + git cat-file commit HEAD~1 >picked_msg && + git cat-file commit HEAD~2 >unrelatedpick_msg && + git cat-file commit HEAD~3 >initial_msg && + test_must_fail grep "Signed-off-by:" initial_msg && + grep "Signed-off-by:" unrelatedpick_msg && + test_must_fail grep "Signed-off-by:" picked_msg && + grep "Signed-off-by:" anotherpick_msg +' + +test_expect_success 'malformed instruction sheet 1' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "resolved" >foo && + git add foo && + git commit && + sed "s/pick /pick/" .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + test_must_fail git cherry-pick --continue +' + +test_expect_success 'malformed instruction sheet 2' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "resolved" >foo && + git add foo && + git commit && + sed "s/pick/revert/" .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + test_must_fail git cherry-pick --continue +' + test_done From cf3e2486d698356a130ebcda9d728c53a0569813 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Thu, 4 Aug 2011 16:09:16 +0530 Subject: [PATCH 18/19] revert: Propagate errors upwards from do_pick_commit Currently, revert_or_cherry_pick can fail in two ways. If it encounters a conflict, it returns a positive number indicating the intended exit status for the git wrapper to pass on; for all other errors, it calls die(). The latter behavior is inconsiderate towards callers, as it denies them the opportunity to recover from errors and do other things. After this patch, revert_or_cherry_pick will still return a positive return value to indicate an exit status for conflicts as before, while for some other errors, it will print an error message and return -1 instead of die()-ing. The cmd_revert and cmd_cherry_pick are adjusted to handle the fatal errors by die()-ing themselves. While the full benefits of this patch will only be seen once all the "die" calls are replaced with calls to "error", its immediate impact is to change some "fatal:" messages to say "error:" and to add a new "fatal: cherry-pick failed" message at the end when the operation fails. Inspired-by: Christian Couder Mentored-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 86 ++++++++++++++++++++++-------------------------- 1 file changed, 40 insertions(+), 46 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 4f0d8f1733..8b452e810e 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -66,15 +66,6 @@ struct replay_opts { #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" -static void fatal(const char *advice, ...) -{ - va_list params; - - va_start(params, advice); - vreportf("fatal: ", advice, params); - va_end(params); -} - static const char *action_name(const struct replay_opts *opts) { return opts->action == REVERT ? "revert" : "cherry-pick"; @@ -356,25 +347,20 @@ static struct tree *empty_tree(void) return tree; } -static NORETURN void die_dirty_index(struct replay_opts *opts) +static int error_dirty_index(struct replay_opts *opts) { - if (read_cache_unmerged()) { - die_resolve_conflict(action_name(opts)); - } else { - if (advice_commit_before_merge) { - if (opts->action == REVERT) - die(_("Your local changes would be overwritten by revert.\n" - "Please, commit your changes or stash them to proceed.")); - else - die(_("Your local changes would be overwritten by cherry-pick.\n" - "Please, commit your changes or stash them to proceed.")); - } else { - if (opts->action == REVERT) - die(_("Your local changes would be overwritten by revert.\n")); - else - die(_("Your local changes would be overwritten by cherry-pick.\n")); - } - } + if (read_cache_unmerged()) + return error_resolve_conflict(action_name(opts)); + + /* Different translation strings for cherry-pick and revert */ + if (opts->action == 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) @@ -492,9 +478,9 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) die (_("Your index file is unmerged.")); } else { if (get_sha1("HEAD", head)) - die (_("You do not have a valid HEAD")); + return error(_("You do not have a valid HEAD")); if (index_differs_from("HEAD", 0)) - die_dirty_index(opts); + return error_dirty_index(opts); } discard_cache(); @@ -507,20 +493,20 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) struct commit_list *p; if (!opts->mainline) - die(_("Commit %s is a merge but no -m option was given."), - sha1_to_hex(commit->object.sha1)); + 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) - die(_("Commit %s does not have parent %d"), - sha1_to_hex(commit->object.sha1), opts->mainline); + 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) - die(_("Mainline was specified but commit %s is not a merge."), - sha1_to_hex(commit->object.sha1)); + return error(_("Mainline was specified but commit %s is not a merge."), + sha1_to_hex(commit->object.sha1)); else parent = commit->parents->item; @@ -530,12 +516,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) if (parent && parse_commit(parent) < 0) /* TRANSLATORS: The first %s will be "revert" or "cherry-pick", the second %s a SHA1 */ - die(_("%s: cannot parse parent commit %s"), - action_name(opts), sha1_to_hex(parent->object.sha1)); + return error(_("%s: cannot parse parent commit %s"), + action_name(opts), sha1_to_hex(parent->object.sha1)); if (get_message(commit, &msg) != 0) - die(_("Cannot get commit message for %s"), - sha1_to_hex(commit->object.sha1)); + return error(_("Cannot get commit message for %s"), + sha1_to_hex(commit->object.sha1)); /* * "commit" is an existing commit. We would want to apply @@ -997,27 +983,28 @@ static int pick_revisions(struct replay_opts *opts) walk_revs_populate_todo(&todo_list, opts); if (create_seq_dir() < 0) { - fatal(_("A cherry-pick or revert is in progress.")); + error(_("A cherry-pick or revert is in progress.")); advise(_("Use --continue to continue the operation")); advise(_("or --reset to forget about it")); - exit(128); + return -1; } if (get_sha1("HEAD", sha1)) { if (opts->action == REVERT) - die(_("Can't revert as initial commit")); - die(_("Can't cherry-pick into empty head")); + return error(_("Can't revert as initial commit")); + return error(_("Can't cherry-pick into empty head")); } save_head(sha1_to_hex(sha1)); save_opts(opts); } return pick_commits(todo_list, opts); error: - die(_("No %s in progress"), action_name(opts)); + return error(_("No %s in progress"), action_name(opts)); } int cmd_revert(int argc, const char **argv, const char *prefix) { struct replay_opts opts; + int res; memset(&opts, 0, sizeof(opts)); if (isatty(0)) @@ -1025,16 +1012,23 @@ int cmd_revert(int argc, const char **argv, const char *prefix) opts.action = REVERT; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); - return pick_revisions(&opts); + res = pick_revisions(&opts); + if (res < 0) + die(_("revert failed")); + return res; } int cmd_cherry_pick(int argc, const char **argv, const char *prefix) { struct replay_opts opts; + int res; memset(&opts, 0, sizeof(opts)); opts.action = CHERRY_PICK; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); - return pick_revisions(&opts); + res = pick_revisions(&opts); + if (res < 0) + die(_("cherry-pick failed")); + return res; } From fb3198c57f4dfa462e29844d47fa9eececc3bb8f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sun, 11 Sep 2011 15:29:21 -0700 Subject: [PATCH 19/19] builtin/revert.c: make commit_list_append() static There is nobody outside that calls into this helper function. Signed-off-by: Junio C Hamano --- builtin/revert.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 8b452e810e..8409f4c886 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -664,8 +664,8 @@ static void read_and_refresh_cache(struct replay_opts *opts) * assert(commit_list_count(list) == 2); * return list; */ -struct commit_list **commit_list_append(struct commit *commit, - struct commit_list **next) +static struct commit_list **commit_list_append(struct commit *commit, + struct commit_list **next) { struct commit_list *new = xmalloc(sizeof(struct commit_list)); new->item = commit;