From b9e6e429378a7763025d1e2f5a1e19d4166ffa05 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 23 Oct 2011 00:43:42 +0530 Subject: [PATCH 1/5] revert: free msg in format_todo() Memory allocated to the fields of msg by get_message() isn't freed. This is potentially a big leak, because fresh memory is allocated to store the commit message for each commit. Fix this using free_message(). Reported-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/revert.c b/builtin/revert.c index 87df70edc3..a6f2ea7e27 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -689,6 +689,7 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list, 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); + free_message(&msg); } return 0; } From e6010ee77afa781cdb910c7b15762cca27c4bec9 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 23 Oct 2011 00:43:43 +0530 Subject: [PATCH 2/5] revert: simplify getting commit subject in format_todo() format_todo() calls get_message(), but uses only the subject line of the commit message. Save work and unnecessary memory allocations by using find_commit_subject() instead. Suggested-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index a6f2ea7e27..efa8d009a1 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -680,16 +680,16 @@ 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"; + const char *subject; + int subject_len; 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); - free_message(&msg); + subject_len = find_commit_subject(cur->item->buffer, &subject); + strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, + subject_len, subject); } return 0; } From 300f5e81b25161bc7da99bf60171a1c9d65eb3d8 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 23 Oct 2011 00:43:44 +0530 Subject: [PATCH 3/5] revert: make commit subjects in insn sheet optional Change the instruction sheet format subtly so that the subject of the commit message that follows the object name is optional. As a result, an instruction sheet like this is now perfectly valid: pick 35b0426 pick fbd5bbcbc2e pick 7362160f While at it, also fix a bug: currently, we use a commit-id-shaped buffer to store the word after "pick" in '.git/sequencer/todo'. This is both wasteful and wrong because it places an artificial limit on the line length. Eliminate the need for the buffer altogether, and add a test demonstrating this. [jc: simplify parsing] Suggested-by: Jonathan Nieder Helped-by: Junio C Hamano Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 37 ++++++++++++++------------------- t/t3510-cherry-pick-sequence.sh | 28 +++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index efa8d009a1..14462ca526 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -694,31 +694,27 @@ 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) +static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts) { unsigned char commit_sha1[20]; - char sha1_abbrev[40]; enum replay_action action; - int insn_len = 0; - char *p, *q; + char *end_of_object_name; + int saved, status; - if (!prefixcmp(start, "pick ")) { + if (!prefixcmp(bol, "pick ")) { action = CHERRY_PICK; - insn_len = strlen("pick"); - p = start + insn_len + 1; - } else if (!prefixcmp(start, "revert ")) { + bol += strlen("pick "); + } else if (!prefixcmp(bol, "revert ")) { action = REVERT; - insn_len = strlen("revert"); - p = start + insn_len + 1; + bol += strlen("revert "); } else return NULL; - q = strchr(p, ' '); - if (!q) - return NULL; - q++; - - strlcpy(sha1_abbrev, p, q - p); + end_of_object_name = bol + strcspn(bol, " \n"); + saved = *end_of_object_name; + *end_of_object_name = '\0'; + status = get_sha1(bol, commit_sha1); + *end_of_object_name = saved; /* * Verify that the action matches up with the one in @@ -731,7 +727,7 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts) return NULL; } - if (get_sha1(sha1_abbrev, commit_sha1) < 0) + if (status < 0) return NULL; return lookup_commit_reference(commit_sha1); @@ -746,13 +742,12 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, int i; for (i = 1; *p; i++) { - commit = parse_insn_line(p, opts); + char *eol = strchrnul(p, '\n'); + commit = parse_insn_line(p, eol, opts); if (!commit) return error(_("Could not parse line %d."), i); next = commit_list_append(commit, next); - p = strchrnul(p, '\n'); - if (*p) - p++; + p = *eol ? eol + 1 : eol; } if (!*todo_list) return error(_("No commits parsed.")); diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 3bca2b3dd5..39b55c13fa 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -12,6 +12,9 @@ test_description='Test cherry-pick continuation features . ./test-lib.sh +# Repeat first match 10 times +_r10='\1\1\1\1\1\1\1\1\1\1' + pristine_detach () { git cherry-pick --reset && git checkout -f "$1^0" && @@ -211,4 +214,29 @@ test_expect_success 'malformed instruction sheet 2' ' test_must_fail git cherry-pick --continue ' +test_expect_success 'malformed instruction sheet 3' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "resolved" >foo && + git add foo && + git commit && + sed "s/pick \([0-9a-f]*\)/pick $_r10/" .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + test_must_fail git cherry-pick --continue +' + +test_expect_success 'commit descriptions in insn sheet are optional' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + cut -d" " -f1,2 .git/sequencer/todo >new_sheet && + cp new_sheet .git/sequencer/todo && + git cherry-pick --continue && + test_path_is_missing .git/sequencer && + git rev-list HEAD >commits + test_line_count = 4 commits +' + test_done From 755b65dac0355edca3004bad2103a60033ab1986 Mon Sep 17 00:00:00 2001 From: Ramkumar Ramachandra Date: Sun, 23 Oct 2011 00:43:45 +0530 Subject: [PATCH 4/5] revert: allow mixed pick and revert instructions Parse the instruction sheet in '.git/sequencer/todo' as a list of (action, operand) pairs, instead of assuming that all instructions use the same action. Now you can do: pick fdc0b12 picked revert 965fed4 anotherpick For cherry-pick and revert, this means that a 'git cherry-pick --continue' can continue an ongoing revert operation and viceversa. This patch lays the foundation for extending the parser to support more actions so 'git rebase -i' can reuse this machinery in the future. While at it, also improve the error messages reported by the insn sheet parser. Helped-by: Jonathan Nieder Acked-by: Jonathan Nieder Signed-off-by: Ramkumar Ramachandra Signed-off-by: Junio C Hamano --- builtin/revert.c | 132 ++++++++++++++++---------------- sequencer.h | 8 ++ t/t3510-cherry-pick-sequence.sh | 58 ++++++++++++++ 3 files changed, 130 insertions(+), 68 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 14462ca526..8c3e4fc6f6 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -39,7 +39,6 @@ static const char * const cherry_pick_usage[] = { NULL }; -enum replay_action { REVERT, CHERRY_PICK }; enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE }; struct replay_opts { @@ -68,14 +67,14 @@ struct replay_opts { static const char *action_name(const struct replay_opts *opts) { - return opts->action == REVERT ? "revert" : "cherry-pick"; + return opts->action == REPLAY_REVERT ? "revert" : "cherry-pick"; } static char *get_encoding(const char *message); static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts) { - return opts->action == REVERT ? revert_usage : cherry_pick_usage; + return opts->action == REPLAY_REVERT ? revert_usage : cherry_pick_usage; } static int option_parse_x(const struct option *opt, @@ -152,7 +151,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) OPT_END(), }; - if (opts->action == CHERRY_PICK) { + if (opts->action == REPLAY_PICK) { struct option cp_extra[] = { OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"), OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"), @@ -348,7 +347,7 @@ static int error_dirty_index(struct replay_opts *opts) return error_resolve_conflict(action_name(opts)); /* Different translation strings for cherry-pick and revert */ - if (opts->action == CHERRY_PICK) + if (opts->action == REPLAY_PICK) error(_("Your local changes would be overwritten by cherry-pick.")); else error(_("Your local changes would be overwritten by revert.")); @@ -452,7 +451,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts) return run_command_v_opt(args, RUN_GIT_CMD); } -static int do_pick_commit(struct commit *commit, struct replay_opts *opts) +static int do_pick_commit(struct commit *commit, enum replay_action action, + struct replay_opts *opts) { unsigned char head[20]; struct commit *base, *next, *parent; @@ -527,7 +527,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) defmsg = git_pathdup("MERGE_MSG"); - if (opts->action == REVERT) { + if (action == REPLAY_REVERT) { base = commit; base_label = msg.label; next = parent; @@ -568,7 +568,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) } } - if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) { + if (!opts->strategy || !strcmp(opts->strategy, "recursive") || action == REPLAY_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); write_message(&msgbuf, defmsg); @@ -592,11 +592,11 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * However, if the merge did not even start, then we don't want to * write it at all. */ - if (opts->action == CHERRY_PICK && !opts->no_commit && (res == 0 || res == 1)) + if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1)) write_cherry_pick_head(commit); if (res) { - error(opts->action == REVERT + error(action == REPLAY_REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV), @@ -620,7 +620,7 @@ static void prepare_revs(struct rev_info *revs, struct replay_opts *opts) init_revisions(revs, NULL); revs->no_walk = 1; - if (opts->action != REVERT) + if (opts->action != REPLAY_REVERT) revs->reverse = 1; argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL); @@ -666,49 +666,53 @@ static void read_and_refresh_cache(struct replay_opts *opts) * assert(commit_list_count(list) == 2); * return list; */ -static struct commit_list **commit_list_append(struct commit *commit, - struct commit_list **next) +static struct replay_insn_list **replay_insn_list_append(enum replay_action action, + struct commit *operand, + struct replay_insn_list **next) { - struct commit_list *new = xmalloc(sizeof(struct commit_list)); - new->item = commit; + struct replay_insn_list *new = xmalloc(sizeof(*new)); + new->action = action; + new->operand = operand; *next = new; new->next = NULL; return &new->next; } -static int format_todo(struct strbuf *buf, struct commit_list *todo_list, - struct replay_opts *opts) +static int format_todo(struct strbuf *buf, struct replay_insn_list *todo_list) { - struct commit_list *cur = NULL; - const char *sha1_abbrev = NULL; - const char *action_str = opts->action == REVERT ? "revert" : "pick"; - const char *subject; - int subject_len; + struct replay_insn_list *cur; for (cur = todo_list; cur; cur = cur->next) { - sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV); - subject_len = find_commit_subject(cur->item->buffer, &subject); + const char *sha1_abbrev, *action_str, *subject; + int subject_len; + + action_str = cur->action == REPLAY_REVERT ? "revert" : "pick"; + sha1_abbrev = find_unique_abbrev(cur->operand->object.sha1, DEFAULT_ABBREV); + subject_len = find_commit_subject(cur->operand->buffer, &subject); strbuf_addf(buf, "%s %s %.*s\n", action_str, sha1_abbrev, subject_len, subject); } return 0; } -static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts) +static int parse_insn_line(char *bol, char *eol, struct replay_insn_list *item) { unsigned char commit_sha1[20]; - enum replay_action action; char *end_of_object_name; int saved, status; if (!prefixcmp(bol, "pick ")) { - action = CHERRY_PICK; + item->action = REPLAY_PICK; bol += strlen("pick "); } else if (!prefixcmp(bol, "revert ")) { - action = REVERT; + item->action = REPLAY_REVERT; bol += strlen("revert "); - } else - return NULL; + } else { + size_t len = strchrnul(bol, '\n') - bol; + if (len > 255) + len = 255; + return error(_("Unrecognized action: %.*s"), (int)len, bol); + } end_of_object_name = bol + strcspn(bol, " \n"); saved = *end_of_object_name; @@ -716,37 +720,29 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts * status = get_sha1(bol, commit_sha1); *end_of_object_name = saved; - /* - * 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 (status < 0) - return NULL; + return error(_("Malformed object name: %s"), bol); - return lookup_commit_reference(commit_sha1); + item->operand = lookup_commit_reference(commit_sha1); + if (!item->operand) + return error(_("Not a valid commit: %s"), bol); + + item->next = NULL; + return 0; } -static int parse_insn_buffer(char *buf, struct commit_list **todo_list, - struct replay_opts *opts) +static int parse_insn_buffer(char *buf, struct replay_insn_list **todo_list) { - struct commit_list **next = todo_list; - struct commit *commit; + struct replay_insn_list **next = todo_list; + struct replay_insn_list item = {0, NULL, NULL}; char *p = buf; int i; for (i = 1; *p; i++) { char *eol = strchrnul(p, '\n'); - commit = parse_insn_line(p, eol, opts); - if (!commit) - return error(_("Could not parse line %d."), i); - next = commit_list_append(commit, next); + if (parse_insn_line(p, eol, &item) < 0) + return error(_("on line %d."), i); + next = replay_insn_list_append(item.action, item.operand, next); p = *eol ? eol + 1 : eol; } if (!*todo_list) @@ -754,8 +750,7 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list, return 0; } -static void read_populate_todo(struct commit_list **todo_list, - struct replay_opts *opts) +static void read_populate_todo(struct replay_insn_list **todo_list) { const char *todo_file = git_path(SEQ_TODO_FILE); struct strbuf buf = STRBUF_INIT; @@ -771,7 +766,7 @@ static void read_populate_todo(struct commit_list **todo_list, } close(fd); - res = parse_insn_buffer(buf.buf, todo_list, opts); + res = parse_insn_buffer(buf.buf, todo_list); strbuf_release(&buf); if (res) die(_("Unusable instruction sheet: %s"), todo_file); @@ -820,18 +815,18 @@ static void read_populate_opts(struct replay_opts **opts_ptr) die(_("Malformed options sheet: %s"), opts_file); } -static void walk_revs_populate_todo(struct commit_list **todo_list, +static void walk_revs_populate_todo(struct replay_insn_list **todo_list, struct replay_opts *opts) { struct rev_info revs; struct commit *commit; - struct commit_list **next; + struct replay_insn_list **next; prepare_revs(&revs, opts); next = todo_list; while ((commit = get_revision(&revs))) - next = commit_list_append(commit, next); + next = replay_insn_list_append(opts->action, commit, next); } static int create_seq_dir(void) @@ -860,7 +855,7 @@ static void save_head(const char *head) die(_("Error wrapping up %s."), head_file); } -static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) +static void save_todo(struct replay_insn_list *todo_list) { const char *todo_file = git_path(SEQ_TODO_FILE); static struct lock_file todo_lock; @@ -868,7 +863,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts) int fd; fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR); - if (format_todo(&buf, todo_list, opts) < 0) + if (format_todo(&buf, todo_list) < 0) die(_("Could not format %s."), todo_file); if (write_in_full(fd, buf.buf, buf.len) < 0) { strbuf_release(&buf); @@ -912,9 +907,10 @@ static void save_opts(struct replay_opts *opts) } } -static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) +static int pick_commits(struct replay_insn_list *todo_list, + struct replay_opts *opts) { - struct commit_list *cur; + struct replay_insn_list *cur; int res; setenv(GIT_REFLOG_ACTION, action_name(opts), 0); @@ -924,8 +920,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) read_and_refresh_cache(opts); for (cur = todo_list; cur; cur = cur->next) { - save_todo(cur, opts); - res = do_pick_commit(cur->item, opts); + save_todo(cur); + res = do_pick_commit(cur->operand, cur->action, opts); if (res) { if (!cur->next) /* @@ -950,7 +946,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) static int pick_revisions(struct replay_opts *opts) { - struct commit_list *todo_list = NULL; + struct replay_insn_list *todo_list = NULL; unsigned char sha1[20]; read_and_refresh_cache(opts); @@ -967,7 +963,7 @@ static int pick_revisions(struct replay_opts *opts) if (!file_exists(git_path(SEQ_TODO_FILE))) goto error; read_populate_opts(&opts); - read_populate_todo(&todo_list, opts); + read_populate_todo(&todo_list); /* Verify that the conflict has been resolved */ if (!index_differs_from("HEAD", 0)) @@ -987,7 +983,7 @@ static int pick_revisions(struct replay_opts *opts) return -1; } if (get_sha1("HEAD", sha1)) { - if (opts->action == REVERT) + if (opts->action == REPLAY_REVERT) return error(_("Can't revert as initial commit")); return error(_("Can't cherry-pick into empty head")); } @@ -1007,7 +1003,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix) memset(&opts, 0, sizeof(opts)); if (isatty(0)) opts.edit = 1; - opts.action = REVERT; + opts.action = REPLAY_REVERT; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = pick_revisions(&opts); @@ -1022,7 +1018,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) int res; memset(&opts, 0, sizeof(opts)); - opts.action = CHERRY_PICK; + opts.action = REPLAY_PICK; git_config(git_default_config, NULL); parse_args(argc, argv, &opts); res = pick_revisions(&opts); diff --git a/sequencer.h b/sequencer.h index 905d295012..f4db257a5b 100644 --- a/sequencer.h +++ b/sequencer.h @@ -7,6 +7,14 @@ #define SEQ_TODO_FILE "sequencer/todo" #define SEQ_OPTS_FILE "sequencer/opts" +enum replay_action { REPLAY_REVERT, REPLAY_PICK }; + +struct replay_insn_list { + enum replay_action action; + struct commit *operand; + struct replay_insn_list *next; +}; + /* * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring * any errors. Intended to be used by 'git reset'. diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh index 39b55c13fa..4b122441d0 100755 --- a/t/t3510-cherry-pick-sequence.sh +++ b/t/t3510-cherry-pick-sequence.sh @@ -239,4 +239,62 @@ test_expect_success 'commit descriptions in insn sheet are optional' ' test_line_count = 4 commits ' +test_expect_success 'revert --continue continues after cherry-pick' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + git revert --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 'mixed pick and revert instructions' ' + pristine_detach initial && + test_must_fail git cherry-pick base..anotherpick && + echo "c" >foo && + git add foo && + git commit && + oldsha=`git rev-parse --short HEAD~1` && + echo "revert $oldsha unrelatedpick" >>.git/sequencer/todo && + 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 unrelated + 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_done From 6a156fdea266d26f4de083ae920a569179d2c893 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Sun, 23 Oct 2011 00:43:46 +0530 Subject: [PATCH 5/5] revert: simplify communicating command-line arguments Currently, command-line arguments are communicated using (argc, argv) until a prepare_revs() turns it into a terse structure. However, since we plan to expose the cherry-picking machinery through a public API in the future, we want callers to be able to call in with a filled-in structure. For the revert builtin, this means that the chief argument parser, parse_args(), should parse into such a structure. Make this change. [rr: minor improvements, commit message] Signed-off-by: Ramkumar Ramachandra Signed-off-by: Jonathan Nieder Signed-off-by: Junio C Hamano --- builtin/revert.c | 53 ++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/builtin/revert.c b/builtin/revert.c index 8c3e4fc6f6..df9459b6be 100644 --- a/builtin/revert.c +++ b/builtin/revert.c @@ -54,13 +54,14 @@ struct replay_opts { 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; + + /* Only used by REPLAY_NONE */ + struct rev_info *revs; }; #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -161,9 +162,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) die(_("program error")); } - opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str, - PARSE_OPT_KEEP_ARGV0 | - PARSE_OPT_KEEP_UNKNOWN); + argc = parse_options(argc, argv, NULL, options, usage_str, + PARSE_OPT_KEEP_ARGV0 | + PARSE_OPT_KEEP_UNKNOWN); /* Check for incompatible subcommands */ verify_opt_mutually_compatible(me, @@ -198,9 +199,6 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) NULL); } - else if (opts->commit_argc < 2) - usage_with_options(usage_str, options); - if (opts->allow_ff) verify_opt_compatible(me, "--ff", "--signoff", opts->signoff, @@ -208,7 +206,20 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts) "-x", opts->record_origin, "--edit", opts->edit, NULL); - opts->commit_argv = argv; + + if (opts->subcommand == REPLAY_NONE) { + opts->revs = xmalloc(sizeof(*opts->revs)); + init_revisions(opts->revs, NULL); + opts->revs->no_walk = 1; + if (argc < 2) + usage_with_options(usage_str, options); + argc = setup_revisions(argc, argv, opts->revs, NULL); + } else + opts->revs = NULL; + + /* Forbid stray command-line arguments */ + if (argc > 1) + usage_with_options(usage_str, options); } struct commit_message { @@ -614,23 +625,15 @@ static int do_pick_commit(struct commit *commit, enum replay_action action, return res; } -static void prepare_revs(struct rev_info *revs, struct replay_opts *opts) +static void prepare_revs(struct replay_opts *opts) { - int argc; - - init_revisions(revs, NULL); - revs->no_walk = 1; if (opts->action != REPLAY_REVERT) - revs->reverse = 1; + opts->revs->reverse ^= 1; - argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL); - if (argc > 1) - usage(*revert_or_cherry_pick_usage(opts)); - - if (prepare_revision_walk(revs)) + if (prepare_revision_walk(opts->revs)) die(_("revision walk setup failed")); - if (!revs->commits) + if (!opts->revs->commits) die(_("empty commit set passed")); } @@ -818,14 +821,13 @@ static void read_populate_opts(struct replay_opts **opts_ptr) static void walk_revs_populate_todo(struct replay_insn_list **todo_list, struct replay_opts *opts) { - struct rev_info revs; struct commit *commit; struct replay_insn_list **next; - prepare_revs(&revs, opts); + prepare_revs(opts); next = todo_list; - while ((commit = get_revision(&revs))) + while ((commit = get_revision(opts->revs))) next = replay_insn_list_append(opts->action, commit, next); } @@ -949,6 +951,9 @@ static int pick_revisions(struct replay_opts *opts) struct replay_insn_list *todo_list = NULL; unsigned char sha1[20]; + if (opts->subcommand == REPLAY_NONE) + assert(opts->revs); + read_and_refresh_cache(opts); /*