Hi,
This is yet another iteration, with few improvements since last time.
Some minor nits pointed out by Jonathan and Christian are now fixed.
One additional detail: while playing with my generalized sequencer, I
noticed a couple of embarrassing bugs in the tests -- "malformed
instruction sheet 1" and "malformed instruction sheet 2" were picking
"base..picked" instead of "base..anotherpick". Fixed now.
Thanks.
-- Ram
Ramkumar Ramachandra (18):
advice: Introduce error_resolve_conflict
config: Introduce functions to write non-standard file
revert: Simplify and inline add_message_to_msg
revert: Don't check lone argument in get_encoding
revert: Rename no_replay to record_origin
revert: Eliminate global "commit" variable
revert: Introduce struct to keep command-line options
revert: Separate cmdline parsing from functional code
revert: Don't create invalid replay_opts in parse_args
revert: Save data for continuing after conflict resolution
revert: Save command-line options for continuing operation
revert: Make pick_commits functionally act on a commit list
revert: Introduce --reset to remove sequencer state
reset: Make reset remove the sequencer state
revert: Remove sequencer state when no commits are pending
revert: Don't implicitly stomp pending sequencer operation
revert: Introduce --continue to continue the operation
revert: Propagate errors upwards from do_pick_commit
Documentation/git-cherry-pick.txt | 6 +
Documentation/git-revert.txt | 6 +
Documentation/sequencer.txt | 9 +
Makefile | 2 +
advice.c | 31 ++-
advice.h | 3 +-
branch.c | 2 +
builtin/revert.c | 735 +++++++++++++++++++++++++++++--------
cache.h | 2 +
config.c | 36 ++-
sequencer.c | 19 +
sequencer.h | 20 +
t/7106-reset-sequence.sh | 44 +++
t/t3510-cherry-pick-sequence.sh | 214 +++++++++++
14 files changed, 958 insertions(+), 171 deletions(-)
create mode 100644 Documentation/sequencer.txt
create mode 100644 sequencer.c
create mode 100644 sequencer.h
create mode 100755 t/7106-reset-sequence.sh
create mode 100755 t/t3510-cherry-pick-sequence.sh
--
1.7.4.rc1.7.g2cf08.dirty
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 [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
cache.h | 2 ++
config.c | 36 +++++++++++++++++++++++++++---------
2 files changed, 29 insertions(+), 9 deletions(-)
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 [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
advice.c | 31 ++++++++++++++++++++++++-------
advice.h | 3 ++-
builtin/revert.c | 9 ---------
3 files changed, 26 insertions(+), 17 deletions(-)
@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)return0;}-voidNORETURNdie_resolve_conflict(constchar*me)+interror_resolve_conflict(constchar*me){-if(advice_resolve_conflict)+error("'%s' is not possible because you have unmerged files.",me);+if(advice_resolve_conflict){/**Messageusedbothwhen'gitcommit'failsandwhen*othercommandsdoingamergedo.*/-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 <file>' 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 <file>' as");+advise("appropriate to mark resolution and make a commit,");+advise("or use 'git commit -a'.");+}+return-1;+}++voidNORETURNdie_resolve_conflict(constchar*me)+{+error_resolve_conflict(me);+die("Exiting because of an unresolved conflict.");}
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 <redacted>
Mentored-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
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 [off-list ref]
Mentored-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
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 [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
@@ -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"));
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 [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
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 [off-list ref]
Mentored-by: Jonathan Nieder [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 201 ++++++++++++++++++++++++++++++------------------------
1 files changed, 113 insertions(+), 88 deletions(-)
@@ -240,20 +258,20 @@ static struct tree *empty_tree(void)returntree;}-staticNORETURNvoiddie_dirty_index(constchar*me)+staticNORETURNvoiddie_dirty_index(structreplay_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."));elsedie(_("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"));elsedie(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -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,*Ifwearerevert,orifourcherry-pickresultsinahandmerge,*wehadbettersaythatthecurrentuserisresponsibleforthat.*/-staticintrun_git_commit(constchar*defmsg)+staticintrun_git_commit(constchar*defmsg,structreplay_opts*opts){/* 6 is max possible length of our args array including NULL */constchar*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)returnrun_command_v_opt(args,RUN_GIT_CMD);}-staticintdo_pick_commit(structcommit*commit)+staticintdo_pick_commit(structcommit*commit,structreplay_opts*opts){unsignedcharhead[20];structcommit*base,*next,*parent;
@@ -364,7 +383,7 @@ static int do_pick_commit(struct commit *commit)structstrbufmsgbuf=STRBUF_INIT;intres;-if(no_commit){+if(opts->no_commit){/**Wedonotintendtocommitimmediately.Wejustwantto*mergethedifferencesin,solet'scomputethetree
@@ -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)intcnt;structcommit_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;-}elseif(0<mainline)+}elseif(0<opts->mainline)die(_("Mainline was specified but commit %s is not a merge."),sha1_to_hex(commit->object.sha1));elseparent=commit->parents->item;-if(allow_ff&&parent&&!hashcmp(parent->object.sha1,head))+if(opts->allow_ff&&parent&&!hashcmp(parent->object.sha1,head))returnfast_forward_to(commit->object.sha1,head);if(parent&&parse_commit(parent)<0)/* TRANSLATORS: The first %s will be "revert" or"cherry-pick",thesecond%saSHA1*/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{structcommit_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)returnres;}-staticvoidprepare_revs(structrev_info*revs)+staticvoidprepare_revs(structrev_info*revs,structreplay_opts*opts){intargc;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"));}-staticvoidread_and_refresh_cache(constchar*me)+staticvoidread_and_refresh_cache(structreplay_opts*opts){staticstructlock_fileindex_lock;intindex_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);}-staticintrevert_or_cherry_pick(intargc,constchar**argv)+staticintrevert_or_cherry_pick(intargc,constchar**argv,+structreplay_opts*opts){structrev_inforevs;structcommit*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))){-intres=do_pick_commit(commit);+intres=do_pick_commit(commit,opts);if(res)returnres;}
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 [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
@@ -563,16 +563,12 @@ static void read_and_refresh_cache(struct replay_opts *opts)rollback_lock_file(&index_lock);}-staticintrevert_or_cherry_pick(intargc,constchar**argv,-structreplay_opts*opts)+staticintpick_commits(structreplay_opts*opts){structrev_inforevs;structcommit*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"));
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 [off-list ref]
Mentored-by: Jonathan Nieder [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 40 +++++++++++++++++++++++++++++-----------
1 files changed, 29 insertions(+), 11 deletions(-)
@@ -86,9 +86,28 @@ static int option_parse_x(const struct option *opt,return0;}+staticvoidverify_opt_compatible(constchar*me,constchar*base_opt,...)+{+constchar*this_opt;+va_listap;+intset;++va_start(ap,base_opt);+while((this_opt=va_arg(ap,constchar*))){+set=va_arg(ap,int);+if(set){+va_end(ap);+die(_("%s: %s cannot be used with %s"),+me,this_opt,base_opt);+}+}+va_end(ap);+}+staticvoidparse_args(intargc,constchar**argv,structreplay_opts*opts){constchar*const*usage_str=revert_or_cherry_pick_usage(opts);+constchar*me=action_name(opts);intnoop;structoptionoptions[]={OPT_BOOLEAN('n',"no-commit",&opts->no_commit,"don't automatically commit"),
@@ -569,17 +595,9 @@ static int pick_commits(struct replay_opts *opts)structcommit*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);
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 [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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
@@ -589,10 +594,116 @@ static void read_and_refresh_cache(struct replay_opts *opts)rollback_lock_file(&index_lock);}-staticintpick_commits(structreplay_opts*opts)+/*+*Appendacommittotheendofthecommit_list.+*+*nextstartsbypointingtothevariablethatholdstheheadofan+*emptycommit_list,andisupdatedtopointtothe"next"fieldof+*thelastitemonthelistasnewcommitsareappended.+*+*Usageexample:+*+*structcommit_list*list;+*structcommit_list**next=&list;+*+*next=commit_list_append(c1,next);+*next=commit_list_append(c2,next);+*assert(commit_list_count(list)==2);+*returnlist;+*/+structcommit_list**commit_list_append(structcommit*commit,+structcommit_list**next)+{+structcommit_list*new=xmalloc(sizeof(structcommit_list));+new->item=commit;+*next=new;+new->next=NULL;+return&new->next;+}++staticintformat_todo(structstrbuf*buf,structcommit_list*todo_list,+structreplay_opts*opts)+{+structcommit_list*cur=NULL;+structcommit_messagemsg={NULL,NULL,NULL,NULL,NULL};+constchar*sha1_abbrev=NULL;+constchar*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))+returnerror(_("Cannot get commit message for %s"),sha1_abbrev);+strbuf_addf(buf,"%s %s %s\n",action_str,sha1_abbrev,msg.subject);+}+return0;+}++staticvoidwalk_revs_populate_todo(structcommit_list**todo_list,+structreplay_opts*opts){structrev_inforevs;structcommit*commit;+structcommit_list**next;++prepare_revs(&revs,opts);++next=todo_list;+while((commit=get_revision(&revs)))+next=commit_list_append(commit,next);+}++staticvoidcreate_seq_dir(void)+{+constchar*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);+}++staticvoidsave_head(constchar*head)+{+constchar*head_file=git_path(SEQ_HEAD_FILE);+staticstructlock_filehead_lock;+structstrbufbuf=STRBUF_INIT;+intfd;++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);+}++staticvoidsave_todo(structcommit_list*todo_list,structreplay_opts*opts)+{+constchar*todo_file=git_path(SEQ_TODO_FILE);+staticstructlock_filetodo_lock;+structstrbufbuf=STRBUF_INIT;+intfd;++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);+}++staticintpick_commits(structreplay_opts*opts)+{+structcommit_list*todo_list=NULL;+structstrbufbuf=STRBUF_INIT;+unsignedcharsha1[20];+structcommit_list*cur;+intres;setenv(GIT_REFLOG_ACTION,action_name(opts),0);if(opts->allow_ff)
@@ -600,14 +711,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))){-intres=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)returnres;}+/*+*Sequenceofpicksfinishedsuccessfully;cleanupby+*removingthe.git/sequencerdirectory+*/+strbuf_addf(&buf,"%s",git_path(SEQ_DIR));+remove_dir_recursively(&buf,0);+strbuf_release(&buf);return0;}
@@ -0,0 +1,48 @@+#!/bin/sh++test_description='Testcherry-pickcontinuationfeatures+++anotherpick:rewritesfootod++picked:rewritesfootoc++unrelatedpick:rewritesunrelatedtoreallyunrelated++base:rewritesfootob++initial:writesfooasa,unrelatedasunrelated++'++../test-lib.sh++pristine_detach(){+rm-rf.git/sequencer&&+gitcheckout-f"$1^0"&&+gitread-tree-u--resetHEAD&&+gitclean-d-f-f-q-x+}++test_expect_successsetup'+echounrelated>unrelated&&+gitaddunrelated&&+test_commitinitialfooa&&+test_commitbasefoob&&+test_commitunrelatedpickunrelatedreallyunrelated&&+test_commitpickedfooc&&+test_commitanotherpickfood&&+gitconfigadvice.detachedheadfalse++'++test_expect_success'cherry-pick persists data on failure''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..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_detachinitial&&+gitcherry-pickinitial..picked&&+test_path_is_missing.git/sequencer+'++test_done
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 [off-list ref]
Helped-by: Christian Couder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 33 +++++++++++++++++++++++++++++++++
t/t3510-cherry-pick-sequence.sh | 30 ++++++++++++++++++++++++++++--
2 files changed, 61 insertions(+), 2 deletions(-)
@@ -33,10 +33,36 @@ test_expect_success setup ' test_expect_success'cherry-pick persists data on failure''pristine_detachinitial&&-test_must_failgitcherry-pickbase..anotherpick&&+test_must_failgitcherry-pick-sbase..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''+rm-rf.git/sequencer&&+pristine_detachinitial&&+test_must_failgitcherry-pick-s-m1--strategy=recursive-Xpatience-Xoursbase..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+gitconfig--file=.git/sequencer/opts--get-alloptions.signoff>actual&&+test_cmpexpectactual&&+echo"1">expect+gitconfig--file=.git/sequencer/opts--get-alloptions.mainline>actual&&+test_cmpexpectactual&&+echo"recursive">expect+gitconfig--file=.git/sequencer/opts--get-alloptions.strategy>actual&&+test_cmpexpectactual&&+cat>expect<<-\EOF+patience+ours+EOF+gitconfig--file=.git/sequencer/opts--get-alloptions.strategy-option>actual&&+test_cmpexpectactual' test_expect_success'cherry-pick cleans up sequencer state upon success''
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 43 ++++++++++++++++++++++++++++---------------
1 files changed, 28 insertions(+), 15 deletions(-)
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
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 | 15 ++++++++-
8 files changed, 111 insertions(+), 21 deletions(-)
create mode 100644 Documentation/sequencer.txt
create mode 100644 sequencer.c
create mode 100644 sequencer.h
@@ -110,6 +111,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::
@@ -91,6 +92,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::
@@ -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.
@@ -770,16 +789,21 @@ static int pick_revisions(struct replay_opts *opts)*cherry-pickshouldbehandleddifferentlyfromanexisting*onethatisbeingcontinued*/-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);+return0;+}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);-returnpick_commits(todo_list,opts);}
@@ -13,7 +13,7 @@ test_description='Test cherry-pick continuation features ../test-lib.sh pristine_detach(){-rm-rf.git/sequencer&&+gitcherry-pick--reset&&gitcheckout-f"$1^0"&&gitread-tree-u--resetHEAD&&gitclean-d-f-f-q-x
@@ -41,7 +41,6 @@ test_expect_success 'cherry-pick persists data on failure' '' test_expect_success'cherry-pick persists opts correctly''-rm-rf.git/sequencer&&pristine_detachinitial&&test_must_failgitcherry-pick-s-m1--strategy=recursive-Xpatience-Xoursbase..anotherpick&&test_path_is_dir.git/sequencer&&
@@ -71,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_detachinitial&&+gitcherry-pick--reset+'++test_expect_success'--reset cleans up sequencer state''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..picked&&+gitcherry-pick--reset&&+test_path_is_missing.git/sequencer+'+ test_done
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
branch.c | 2 ++
t/7106-reset-sequence.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
create mode 100755 t/7106-reset-sequence.sh
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 <redacted>
Acked-by: Jonathan Nieder <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 12 +++++++++++-
t/t3510-cherry-pick-sequence.sh | 24 ++++++++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletions(-)
@@ -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_detachinitial&&+test_must_failgitcherry-pickbase..picked&&+test_path_is_missing.git/sequencer&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+{+gitrev-listHEAD|+gitdiff-tree--root--stdin|+sed"s/$_x40/OBJID/g"+}>actual&&+cat>expect<<-\EOF&&+OBJID+:100644100644OBJIDOBJIDMfoo+OBJID+:100644100644OBJIDOBJIDMunrelated+OBJID+:000000100644OBJIDOBJIDAfoo+:000000100644OBJIDOBJIDAunrelated+EOF+test_cmpexpectactual+'+ test_done
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 <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 30 +++++++++++++++++++++++++-----
t/t3510-cherry-pick-sequence.sh | 9 +++++++++
2 files changed, 34 insertions(+), 5 deletions(-)
@@ -803,9 +814,18 @@ static int pick_revisions(struct replay_opts *opts)remove_sequencer_state(1);return0;}else{-/* Start a new cherry-pick/ revert sequence */+/*+*Startanewcherry-pick/revertsequence;but+*first,makesurethatanexistingoneisn'tin+*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"));
@@ -106,4 +106,13 @@ test_expect_success 'cherry-pick cleans up sequencer state when one commit is letest_cmpexpectactual'+test_expect_success'cherry-pick does not implicitly stomp an existing operation''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..anotherpick&&+test-chmtime-v+0.git/sequencer>expect&&+test_must_failgitcherry-pickunrelatedpick&&+test-chmtime-v+0.git/sequencer>actual&&+test_cmpexpectactual+'+ test_done
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.
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
Documentation/git-cherry-pick.txt | 1 +
Documentation/git-revert.txt | 1 +
Documentation/sequencer.txt | 5 +
builtin/revert.c | 184 ++++++++++++++++++++++++++++++++++++-
t/t3510-cherry-pick-sequence.sh | 96 +++++++++++++++++++
5 files changed, 283 insertions(+), 4 deletions(-)
@@ -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.
@@ -119,14 +119,42 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)va_end(ap);}+staticvoidverify_opt_mutually_compatible(constchar*me,...)+{+constchar*opt1,*opt2;+va_listap;+intset;++va_start(ap,me);+while((opt1=va_arg(ap,constchar*))){+set=va_arg(ap,int);+if(set)+break;+}+if(!opt1)+gotook;+while((opt2=va_arg(ap,constchar*))){+set=va_arg(ap,int);+if(set){+va_end(ap);+die(_("%s: %s cannot be used with %s"),+me,opt1,opt2);+}+}+ok:+va_end(ap);+}+staticvoidparse_args(intargc,constchar**argv,structreplay_opts*opts){constchar*const*usage_str=revert_or_cherry_pick_usage(opts);constchar*me=action_name(opts);intnoop;intreset=0;+intcontin=0;structoptionoptions[]={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)",
@@ -156,15 +184,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;+elseif(contin)+opts->subcommand=REPLAY_CONTINUE;elseopts->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,
@@ -670,6 +712,128 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,return0;}+staticstructcommit*parse_insn_line(char*start,structreplay_opts*opts)+{+unsignedcharcommit_sha1[20];+charsha1_abbrev[40];+enumreplay_actionaction;+intinsn_len=0;+char*p,*q;++if(!prefixcmp(start,"pick ")){+action=CHERRY_PICK;+insn_len=strlen("pick");+p=start+insn_len+1;+}elseif(!prefixcmp(start,"revert ")){+action=REVERT;+insn_len=strlen("revert");+p=start+insn_len+1;+}else+returnNULL;++q=strchr(p,' ');+if(!q)+returnNULL;+q++;++strlcpy(sha1_abbrev,p,q-p);++/*+*Verifythattheactionmatchesupwiththeonein+*opts;wedon'tsupportarbitraryinstructions+*/+if(action!=opts->action){+constchar*action_str;+action_str=action==REVERT?"revert":"cherry-pick";+error(_("Cannot %s during a %s"),action_str,action_name(opts));+returnNULL;+}++if(get_sha1(sha1_abbrev,commit_sha1)<0)+returnNULL;++returnlookup_commit_reference(commit_sha1);+}++staticvoidread_populate_todo(structcommit_list**todo_list,+structreplay_opts*opts)+{+constchar*todo_file=git_path(SEQ_TODO_FILE);+structstrbufbuf=STRBUF_INIT;+structcommit_list**next;+structcommit*commit;+char*p;+intfd;++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);++next=todo_list;+for(p=buf.buf;*p;p=strchr(p,'\n')+1){+commit=parse_insn_line(p,opts);+if(!commit)+gotoerror;+next=commit_list_append(commit,next);+}+if(!*todo_list)+gotoerror;+strbuf_release(&buf);+return;+error:+strbuf_release(&buf);+die(_("Unusable instruction sheet: %s"),todo_file);+}++staticintpopulate_opts_cb(constchar*key,constchar*value,void*data)+{+structreplay_opts*opts=data;+interror_flag=1;++if(!value)+error_flag=0;+elseif(!strcmp(key,"options.no-commit"))+opts->no_commit=git_config_bool_or_int(key,value,&error_flag);+elseif(!strcmp(key,"options.edit"))+opts->edit=git_config_bool_or_int(key,value,&error_flag);+elseif(!strcmp(key,"options.signoff"))+opts->signoff=git_config_bool_or_int(key,value,&error_flag);+elseif(!strcmp(key,"options.record-origin"))+opts->record_origin=git_config_bool_or_int(key,value,&error_flag);+elseif(!strcmp(key,"options.allow-ff"))+opts->allow_ff=git_config_bool_or_int(key,value,&error_flag);+elseif(!strcmp(key,"options.mainline"))+opts->mainline=git_config_int(key,value);+elseif(!strcmp(key,"options.strategy"))+git_config_string(&opts->strategy,key,value);+elseif(!strcmp(key,"options.strategy-option")){+ALLOC_GROW(opts->xopts,opts->xopts_nr+1,opts->xopts_alloc);+opts->xopts[opts->xopts_nr++]=xstrdup(value);+}else+returnerror(_("Invalid key: %s"),key);++if(!error_flag)+returnerror(_("Invalid value for %s: %s"),key,value);++return0;+}++staticvoidread_populate_opts(structreplay_opts**opts_ptr)+{+constchar*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);+}+staticvoidwalk_revs_populate_todo(structcommit_list**todo_list,structreplay_opts*opts){
@@ -813,6 +977,15 @@ static int pick_revisions(struct replay_opts *opts)if(opts->subcommand==REPLAY_RESET){remove_sequencer_state(1);return0;+}elseif(opts->subcommand==REPLAY_CONTINUE){+if(!file_exists(git_path(SEQ_TODO_FILE)))+gotoerror;+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{/**Startanewcherry-pick/revertsequence;but
@@ -823,7 +996,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)){
@@ -835,6 +1009,8 @@ static int pick_revisions(struct replay_opts *opts)save_opts(opts);}returnpick_commits(todo_list,opts);+error:+die(_("No %s in progress"),action_name(opts));}intcmd_revert(intargc,constchar**argv,constchar*prefix)
@@ -115,4 +115,100 @@ test_expect_success 'cherry-pick does not implicitly stomp an existing operationtest_cmpexpectactual'+test_expect_success'--continue complains when no cherry-pick is in progress''+pristine_detachinitial&&+test_must_failgitcherry-pick--continue+'++test_expect_success'--continue complains when there are unresolved conflicts''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..picked&&+test_must_failgitcherry-pick--continue+'++test_expect_success'--continue continues after conflicts are resolved''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..anotherpick&&+echo"c">foo&&+gitaddfoo&&+gitcommit&&+gitcherry-pick--continue&&+test_path_is_missing.git/sequencer&&+{+gitrev-listHEAD|+gitdiff-tree--root--stdin|+sed"s/$_x40/OBJID/g"+}>actual&&+cat>expect<<-\EOF&&+OBJID+:100644100644OBJIDOBJIDMfoo+OBJID+:100644100644OBJIDOBJIDMfoo+OBJID+:100644100644OBJIDOBJIDMunrelated+OBJID+:000000100644OBJIDOBJIDAfoo+:000000100644OBJIDOBJIDAunrelated+EOF+test_cmpexpectactual+'++test_expect_success'--continue respects opts''+pristine_detachinitial&&+test_must_failgitcherry-pick-xbase..anotherpick&&+echo"c">foo&&+gitaddfoo&&+gitcommit&&+gitcherry-pick--continue&&+test_path_is_missing.git/sequencer&&+gitcat-filecommitHEAD>anotherpick_msg&&+gitcat-filecommitHEAD~1>picked_msg&&+gitcat-filecommitHEAD~2>unrelatedpick_msg&&+gitcat-filecommitHEAD~3>initial_msg&&+test_must_failgrep"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 propogated to resolved conflict''+pristine_detachinitial&&+test_must_failgitcherry-pick--signoffbase..anotherpick&&+echo"c">foo&&+gitaddfoo&&+gitcommit&&+gitcherry-pick--continue&&+test_path_is_missing.git/sequencer&&+gitcat-filecommitHEAD>anotherpick_msg&&+gitcat-filecommitHEAD~1>picked_msg&&+gitcat-filecommitHEAD~2>unrelatedpick_msg&&+gitcat-filecommitHEAD~3>initial_msg&&+test_must_failgrep"Signed-off-by:"initial_msg&&+grep"Signed-off-by:"unrelatedpick_msg&&+test_must_failgrep"Signed-off-by:"picked_msg&&+grep"Signed-off-by:"anotherpick_msg+'++test_expect_success'malformed instruction sheet 1''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..anotherpick&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+sed"s/pick /pick/".git/sequencer/todo>new_sheet+cpnew_sheet.git/sequencer/todo+test_must_failgitcherry-pick--continue+'++test_expect_success'malformed instruction sheet 2''+pristine_detachinitial&&+test_must_failgitcherry-pickbase..anotherpick&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+sed"s/pick/revert/".git/sequencer/todo>new_sheet+cpnew_sheet.git/sequencer/todo+test_must_failgitcherry-pick--continue+'+ test_done
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 [off-list ref]
Mentored-by: Jonathan Nieder [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
builtin/revert.c | 86 +++++++++++++++++++++++++-----------------------------
1 files changed, 40 insertions(+), 46 deletions(-)
@@ -363,25 +354,20 @@ static struct tree *empty_tree(void)returntree;}-staticNORETURNvoiddie_dirty_index(structreplay_opts*opts)+staticinterror_dirty_index(structreplay_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())+returnerror_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;}staticintfast_forward_to(constunsignedchar*to,constunsignedchar*from)
@@ -499,9 +485,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"));+returnerror(_("You do not have a valid HEAD"));if(index_differs_from("HEAD",0))-die_dirty_index(opts);+returnerror_dirty_index(opts);}discard_cache();
@@ -514,20 +500,20 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)structcommit_list*p;if(!opts->mainline)-die(_("Commit %s is a merge but no -m option was given."),-sha1_to_hex(commit->object.sha1));+returnerror(_("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);+returnerror(_("Commit %s does not have parent %d"),+sha1_to_hex(commit->object.sha1),opts->mainline);parent=p->item;}elseif(0<opts->mainline)-die(_("Mainline was specified but commit %s is not a merge."),-sha1_to_hex(commit->object.sha1));+returnerror(_("Mainline was specified but commit %s is not a merge."),+sha1_to_hex(commit->object.sha1));elseparent=commit->parents->item;
@@ -537,12 +523,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",thesecond%saSHA1*/-die(_("%s: cannot parse parent commit %s"),-action_name(opts),sha1_to_hex(parent->object.sha1));+returnerror(_("%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));+returnerror(_("Cannot get commit message for %s"),+sha1_to_hex(commit->object.sha1));/**"commit"isanexistingcommit.Wewouldwanttoapply
@@ -995,27 +981,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"));+returnerror(_("Can't revert as initial commit"));+returnerror(_("Can't cherry-pick into empty head"));}save_head(sha1_to_hex(sha1));save_opts(opts);}returnpick_commits(todo_list,opts);error:-die(_("No %s in progress"),action_name(opts));+returnerror(_("No %s in progress"),action_name(opts));}intcmd_revert(intargc,constchar**argv,constchar*prefix){structreplay_optsopts;+intres;memset(&opts,0,sizeof(opts));if(isatty(0))
From: Christian Couder <hidden> Date: 2016-06-15 22:51:43
On Tuesday 02 August 2011 04:36:41 Christian Couder wrote:
static int parse_insn_buffer(char *buffer,
struct commit_list **todo_list,
struct replay_opts *opts)
{
struct commit_list **next = todo_list;
char *p = buffer;
int i;
for (i = 1; p && *p; i++) {
struct commit *commit = parse_insn_line(p, opts);
if (!commit)
return error(_("Could not parse line %d."), i);
next = commit_list_append(commit, next);
p = strchr(p, '\n');
if (p)
p++;
}
We can simplify this loop using strchrnul() like this:
for (i = 1; *p; i++) {
struct commit *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');
}
Thanks,
Christian.
From: Christian Couder <hidden> Date: 2016-06-15 22:51:43
On Tuesday 02 August 2011 04:47:01 Christian Couder wrote:
We can simplify this loop using strchrnul() like this:
for (i = 1; *p; i++) {
struct commit *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');
}
Ooops, sorry I mean like this:
for (i = 1; *p; i++) {
struct commit *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++;
}
Thanks,
Christian.
On Tuesday 02 August 2011 04:47:01 Christian Couder wrote:
quoted
We can simplify this loop using strchrnul() like this:
[...]
for (i = 1; *p; i++) {
struct commit *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++;
}
Thanks for the valuable review. I would have preferred to receive it
earlier in the development cycle than later- although I've queued this
along with other similar changes (error reporting/ stylistic changes;
not urgent), I'm not in favor of holding up the series.
Thank you.
-- Ram
From: Christian Couder <hidden> Date: 2016-06-15 22:51:43
On Monday 01 August 2011 20:06:56 Ramkumar Ramachandra wrote:
+static void verify_opt_compatible(const char *me, const char *base_opt,
...) +{
+ const char *this_opt;
+ va_list ap;
+ int set;
+
+ va_start(ap, base_opt);
+ while ((this_opt = va_arg(ap, const char *))) {
+ set = va_arg(ap, int);
+ if (set) {
+ va_end(ap);
+ die(_("%s: %s cannot be used with %s"),
+ me, this_opt, base_opt);
+ }
+ }
+ va_end(ap);
+}
Here I'd suggest:
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 *))) {
int set = va_arg(ap, int);
if (set)
break;
}
va_end(ap);
if (this_opt)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
}
Thanks and sorry for the late suggestions,
Christian.
From: Christian Couder <hidden> Date: 2016-06-15 22:51:43
On Tuesday 02 August 2011 08:28:37 Christian Couder wrote:
On Monday 01 August 2011 20:06:56 Ramkumar Ramachandra wrote:
quoted
+static void verify_opt_compatible(const char *me, const char *base_opt,
...) +{
+ const char *this_opt;
+ va_list ap;
+ int set;
+
+ va_start(ap, base_opt);
+ while ((this_opt = va_arg(ap, const char *))) {
+ set = va_arg(ap, int);
+ if (set) {
+ va_end(ap);
+ die(_("%s: %s cannot be used with %s"),
+ me, this_opt, base_opt);
+ }
+ }
+ va_end(ap);
+}
Here I'd suggest:
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 *))) {
int set = va_arg(ap, int);
if (set)
break;
}
va_end(ap);
if (this_opt)
die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
}
... and we could remove the "set" variable like this:
while ((this_opt = va_arg(ap, const char *))) {
if (va_arg(ap, int))
break;
}
This could be done in verify_opt_mutually_compatible() too.
Thanks,
Christian.
From: Christian Couder <hidden> Date: 2016-06-15 22:51:44
On Monday 01 August 2011 20:07:04 Ramkumar Ramachandra wrote:
+test_expect_success '--continue complains when there are unresolved
conflicts' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..picked &&
+ test_must_fail git cherry-pick --continue
+'
When I try to manually run the above test I get:
-----------------------------------
$ pristine_detach initial
Warning: you are leaving 1 commit behind, not connected to
any of your branches:
30b20f1 unrelatedpick
HEAD is now at df2a63d... initial
$
$ git cherry-pick base..picked
[detached HEAD 30b20f1] unrelatedpick
Author: A U Thor [off-list ref]
1 files changed, 1 insertions(+), 1 deletions(-)
Auto-merging foo
CONFLICT (content): Merge conflict in foo
error: could not apply fdc0b12... picked
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
$
$ git cherry-pick --continue
fatal: No cherry-pick in progress
-----------------------------------
So it complains because there is no cherry-pick in progress, not because there
are unresolved conflicts.
Thanks,
Christian.
On Monday 01 August 2011 20:07:04 Ramkumar Ramachandra wrote:
quoted
+test_expect_success '--continue complains when there are unresolved
conflicts' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..picked &&
+ test_must_fail git cherry-pick --continue
+'
When I try to manually run the above test I get:
-----------------------------------
$ pristine_detach initial
Warning: you are leaving 1 commit behind, not connected to
any of your branches:
30b20f1 unrelatedpick
HEAD is now at df2a63d... initial
$
$ git cherry-pick base..picked
[detached HEAD 30b20f1] unrelatedpick
Author: A U Thor [off-list ref]
1 files changed, 1 insertions(+), 1 deletions(-)
Auto-merging foo
CONFLICT (content): Merge conflict in foo
error: could not apply fdc0b12... picked
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'
$
$ git cherry-pick --continue
fatal: No cherry-pick in progress
Good catch. It should be "base..anotherpick". Fixed now.
-- Ram
... and we could remove the "set" variable like this:
while ((this_opt = va_arg(ap, const char *))) {
if (va_arg(ap, int))
break;
}
Very elegant- I especially like this.
You've reduced this to simply checking if the while loop ended
properly, or broke (using a break statement) by checking the
conditional that drove the loop.
Thanks.
-- Ram
Ooops, sorry I mean like this:
for (i = 1; *p; i++) {
struct commit *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++;
}
Thanks for catching this bug, and showing me how to handle it correctly! :)
While fixing this, I took the opportunity to incorporate all your
other suggestions in my new series. Will post the new series shortly.
[It broke my new series, but this was definitely worth fixing]
-- Ram