Hi,
Sorry about the delay -- I was travelling again. I've chistled
everything until the 12th patch to near-perfection. One of the major
user-visible changes is renaming '--skip-all' to '--reset'. Initially
suggested by Christian, I really like this name; I think there will be
enough justification for it, especially after we get 'git reset
--hard' to clear away the sequencer state. Now, we're back to the
point where we need to finalize the instruction sheet format.
Thanks for reading.
-- Ram
Ramkumar Ramachandra (14):
advice: Introduce error_resolve_conflict
revert: Inline add_message_to_msg function
revert: Don't check lone argument in get_encoding
revert: Rename no_replay to record_origin
revert: Propogate errors upwards from do_pick_commit
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: Persist data for continuation
revert: Introduce a layer of indirection over pick_commits
revert: Introduce --reset to cleanup sequencer data
revert: Introduce --continue to continue the operation
revert: Change insn sheet format
advice.c | 31 ++-
advice.h | 1 +
builtin/revert.c | 657 ++++++++++++++++++++++++++++--------
git-rebase--interactive.sh | 25 ++-
t/t3032-merge-recursive-options.sh | 2 +
t/t3501-revert-cherry-pick.sh | 1 +
t/t3502-cherry-pick-merge.sh | 9 +-
t/t3504-cherry-pick-rerere.sh | 2 +
t/t3505-cherry-pick-empty.sh | 14 +-
t/t3506-cherry-pick-ff.sh | 3 +
t/t3507-cherry-pick-conflict.sh | 24 +-
t/t3510-cherry-pick-sequence.sh | 119 +++++++
t/t7502-commit.sh | 1 +
13 files changed, 720 insertions(+), 169 deletions(-)
create mode 100644 t/t3510-cherry-pick-sequence.sh
--
1.7.5.GIT
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: Please, 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]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
advice.c | 31 ++++++++++++++++++++++++-------
advice.h | 1 +
2 files changed, 25 insertions(+), 7 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("Please, 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 add_message_to_msg function is poorly implemented, has an unclear
API, and only one callsite. Replace the callsite with a cleaner
implementation. Additionally, fix a bug introduced in 9509af6 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01) -- a NULL pointer
was being incremented when "\n\n" was not found in "message".
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 20 ++++++--------------
1 files changed, 6 insertions(+), 14 deletions(-)
Rename the variable corresponding to the "-x" command-line option from
"no_replay" to a more apt "record_origin".
Suggested-by: Jonathan Nieder <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
@@ -465,7 +465,7 @@ static int do_pick_commit(void)p=p?p+2:sha1_to_hex(commit->object.sha1);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");
@@ -560,7 +560,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"));
Currently, the return value from revert_or_cherry_pick is a
non-negative number representing the intended exit status from `git
revert` or `git cherry-pick`. Change this by replacing some of the
calls to "die" with calls to "error", so that it can return negative
values too. Postive return values indicate conflicts, while negative
ones indicate other errors. This return status is propogated updwards
from do_pick_commit, to be finally handled in cmd_cherry_pick and
cmd_revert.
In the same spirit, also introduce a new function error_dirty_index,
based on die_dirty_index, which prints some hints and returns an error
to its caller do_pick_commit.
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 of the "die:" messages to "error:" messages and
print a new "fatal: cherry-pick failed" message 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>
---
builtin/revert.c | 69 ++++++++++++++++++++++++++++-------------------------
1 files changed, 36 insertions(+), 33 deletions(-)
@@ -250,25 +250,20 @@ static struct tree *empty_tree(void)returntree;}-staticNORETURNvoiddie_dirty_index(constchar*me)+staticinterror_dirty_index(constchar*me){-if(read_cache_unmerged()){-die_resolve_conflict(me);-}else{-if(advice_commit_before_merge){-if(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)-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(me);++/* Different translation strings for cherry-pick and revert */+if(action==CHERRY_PICK)+error(_("Your local changes would be overwritten by %s."),me);+else+error(_("Your local changes would be overwritten by %s."),me);++if(advice_commit_before_merge)+advise(_("Please, commit your changes or stash them to proceed."));+return-1;}staticintfast_forward_to(constunsignedchar*to,constunsignedchar*from)
@@ -382,12 +377,12 @@ static int do_pick_commit(void)*toworkon.*/if(write_cache_as_tree(head,0,NULL))-die(_("Your index file is unmerged."));+returnerror(_("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(me);+returnerror_dirty_index(me);}discard_cache();
@@ -400,20 +395,20 @@ static int do_pick_commit(void)structcommit_list*p;if(!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!=mainline&&p;cnt++)p=p->next;if(cnt!=mainline||!p)-die(_("Commit %s does not have parent %d"),-sha1_to_hex(commit->object.sha1),mainline);+returnerror(_("Commit %s does not have parent %d"),+sha1_to_hex(commit->object.sha1),mainline);parent=p->item;}elseif(0<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;
@@ -423,12 +418,12 @@ static int do_pick_commit(void)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));+returnerror(_("%s: cannot parse parent commit %s"),+me,sha1_to_hex(parent->object.sha1));if(get_message(commit->buffer,&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
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 variable "me" is left as a file-scope static variable because it
is not an independent option. "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.
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>
---
builtin/revert.c | 182 ++++++++++++++++++++++++++++++------------------------
1 files changed, 102 insertions(+), 80 deletions(-)
@@ -339,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];
@@ -347,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;}
@@ -358,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;
@@ -368,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
@@ -381,7 +396,7 @@ static int do_pick_commit(struct commit *commit)if(get_sha1("HEAD",head))returnerror(_("You do not have a valid HEAD"));if(index_differs_from("HEAD",0))-returnerror_dirty_index(me);+returnerror_dirty_index(me,opts->action);}discard_cache();
@@ -393,25 +408,25 @@ static int do_pick_commit(struct commit *commit)intcnt;structcommit_list*p;-if(!mainline)+if(!opts->mainline)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!=mainline&&p;+cnt!=opts->mainline&&p;cnt++)p=p->next;-if(cnt!=mainline||!p)+if(cnt!=opts->mainline||!p)returnerror(_("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)returnerror(_("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)
@@ -433,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;
@@ -459,18 +474,18 @@ static int do_pick_commit(struct commit *commit)p=p?p+2:sha1_to_hex(commit->object.sha1);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)-write_cherry_pick_head(sha1_to_hex(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;
@@ -480,23 +495,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);
@@ -505,18 +520,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"));
@@ -540,33 +555,34 @@ static void read_and_refresh_cache(const char *me)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";+me=opts->action==REVERT?"revert":"cherry-pick";setenv(GIT_REFLOG_ACTION,me,0);-parse_args(argc,argv);+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(me,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 does too many things including
argument parsing and setting up to pick the commits; this doesn't make
a good API. Simplify and rename the function to pick_commits, so that
it just has the responsibility of setting up the revision walker and
calling do_pick_commit in a loop. Transfer the remaining work to its
callers cmd_cherry_pick and cmd_revert. Later in the series,
pick_commits will serve as the starting point for continuing the
cherry-pick or revert.
Inspired-by: Christian Couder [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 17 +++++++++--------
1 files changed, 9 insertions(+), 8 deletions(-)
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. These new
files are unrelated to the existing CHERRY_PICK_HEAD, which will still
be useful when a conflict is encountered.
Inspired-by: Christian Couder [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 122 +++++++++++++++++++++++++++++++++++++--
t/t3510-cherry-pick-sequence.sh | 37 ++++++++++++
2 files changed, 153 insertions(+), 6 deletions(-)
create mode 100644 t/t3510-cherry-pick-sequence.sh
@@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)returnerror(_("Your index file is unmerged."));}else{if(get_sha1("HEAD",head))-returnerror(_("You do not have a valid HEAD"));+returnerror(_("Can't %s on an unborn branch"),me);if(index_differs_from("HEAD",0))returnerror_dirty_index(me,opts->action);}
@@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)rollback_lock_file(&index_lock);}-staticintpick_commits(structreplay_opts*opts)+staticvoidformat_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;++action=(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))+die(_("Cannot get commit message for %s"),sha1_abbrev);+strbuf_addf(buf,"%s %s %s\n",action,sha1_abbrev,msg.subject);+}+}++staticvoidwalk_revs_populate_todo(structcommit_list**todo_list,+structreplay_opts*opts){structrev_inforevs;structcommit*commit;+structcommit_list*new;+structcommit_list**next;++prepare_revs(&revs,opts);++/* Insert into todo_list in the same order */+/* NEEDSWORK: Expose this as commit_list_append */+next=todo_list;+while((commit=get_revision(&revs))){+new=xmalloc(sizeof(structcommit_list));+new->item=commit;+*next=new;+next=&new->next;+}+*next=NULL;+}++staticvoidcreate_seq_dir(void)+{+if(file_exists(git_path(SEQ_DIR))){+if(!is_directory(git_path(SEQ_DIR))&&remove_path(git_path(SEQ_DIR))<0)+die(_("Could not remove %s"),git_path(SEQ_DIR));+}elseif(mkdir(git_path(SEQ_DIR),0777)<0)+die_errno(_("Could not create sequencer directory '%s'."),git_path(SEQ_DIR));+}++staticvoidsave_head(constchar*head)+{+staticstructlock_filehead_lock;+structstrbufbuf=STRBUF_INIT;+intfd;++fd=hold_lock_file_for_update(&head_lock,git_path(SEQ_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."),git_path(SEQ_HEAD_FILE));+if(commit_lock_file(&head_lock)<0)+die(_("Error wrapping up %s"),git_path(SEQ_HEAD_FILE));+}++staticvoidsave_todo(structcommit_list*todo_list,structreplay_opts*opts)+{+staticstructlock_filetodo_lock;+structstrbufbuf=STRBUF_INIT;+intfd;++fd=hold_lock_file_for_update(&todo_lock,git_path(SEQ_TODO_FILE),LOCK_DIE_ON_ERROR);+format_todo(&buf,todo_list,opts);+if(write_in_full(fd,buf.buf,buf.len)<0){+strbuf_release(&buf);+die_errno(_("Could not write to %s."),git_path(SEQ_TODO_FILE));+}+if(commit_lock_file(&todo_lock)<0){+strbuf_release(&buf);+die(_("Error wrapping up %s"),git_path(SEQ_TODO_FILE));+}+strbuf_release(&buf);+}++staticintcleanup_sequencer_data(void)+{+staticstructstrbufseq_dir=STRBUF_INIT;++strbuf_addf(&seq_dir,"%s",git_path(SEQ_DIR));+if(remove_dir_recursively(&seq_dir,0)<0){+strbuf_release(&seq_dir);+returnerror(_("Unable to clean up after successful %s"),me);+}+strbuf_release(&seq_dir);+return0;+}++staticintpick_commits(structreplay_opts*opts)+{+structcommit_list*todo_list=NULL;+unsignedcharsha1[20];+structcommit_list*cur;+intres;setenv(GIT_REFLOG_ACTION,me,0);if(opts->allow_ff)
@@ -589,15 +690,24 @@ static int pick_commits(struct replay_opts *opts)opts->record_origin||opts->edit));read_and_refresh_cache(me,opts);-prepare_revs(&revs,opts);+walk_revs_populate_todo(&todo_list,opts);+create_seq_dir();+if(!get_sha1("HEAD",sha1))+save_head(sha1_to_hex(sha1));+save_todo(todo_list,opts);-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;}-return0;+/*+*Sequenceofpicksfinishedsuccessfully;cleanupby+*removingthe.git/sequencerdirectory+*/+returncleanup_sequencer_data();}intcmd_revert(intargc,constchar**argv,constchar*prefix)
@@ -0,0 +1,37 @@+#!/bin/sh++test_description='Testcherry-pickcontinuationfeatures+++picked:rewritesfootoc++unrelatedpick:rewritesunrelatedtoreallyunrelated++base:rewritesfootob++initial:writesfooasa,unrelatedasunrelated++'++../test-lib.sh++pristine_detach(){+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&&+gitconfigadvice.detachedheadfalse++'++test_expect_success'cherry-pick cleans up sequencer directory upon success''+pristine_detachinitial&&+gitcherry-pickinitial..picked&&+test_path_is_missing.git/sequencer+'++test_done
Write a new function called process_continuation to prepare a
todo_list to call pick_commits with; the job of pick_commits is
simplified into performing the tasks listed in todo_list. This will
be useful when continuation functionality like "--continue" is
introduced later in the series.
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 36 +++++++++++++++++++++++++-----------
1 files changed, 25 insertions(+), 11 deletions(-)
The "--ff" command-line option cannot be used with four other
command-line options. However, when these options are specified with
"--ff" on the command-line, parse_args will still parse these
incompatible options into a replay_opts structure for use by the rest
of the program. Although pick_commits checks the validity of the
replay_opts strucutre before before starting its operation, this is
inelegant design; pick_commits is currently the gatekeeper to the
cherry-pick machinery, but this will change 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. Also ensure that regressions in maintaining this
invariant are caught in the future by adding an assertion in
pick_commits.
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>
---
builtin/revert.c | 37 ++++++++++++++++++++++++++-----------
1 files changed, 26 insertions(+), 11 deletions(-)
@@ -82,6 +82,22 @@ 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)+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);
@@ -561,17 +584,9 @@ static int pick_commits(struct replay_opts *opts)structcommit*commit;setenv(GIT_REFLOG_ACTION,me,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(me,opts);prepare_revs(&revs,opts);
Since we want to develop the functionality to either pick or revert
individual commits atomically later in the series, make "commit" a
variable to be passed around explicitly as an argument for clarity.
This involves changing several functions to take an additional
argument, but no functional changes. Additionaly, this will permit
more than one commit to be cherry-picked at once, should we choose to
develop this functionality in future.
Inspired-by: Christian Couder [off-list ref]
Helped-by: Jonathan Nieder [off-list ref]
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
Add command-line options to the instruction sheet, but don't let the
existing parser break. Parsing out the command-line options back into
a replay_opts struct is unimplemented.
Signed-off-by: Ramkumar Ramachandra <redacted>
---
I've intentionally left parse_cmdline_args unimplemented -- Although
Stephen (and Christian subsequently) have written a parser [1], I'm
not happy with it. The point of this patch is to illustrate the
problem with this instruction sheet format, and provoke some
discussion about this. I don't want to parse command-line arguments
by hand handling all sorts of corner cases in quoting etc -- is there
any other way? Existing implementations in libraries like Glib are
much too heavyweight.
Note that I've used the '#' character as a separator to simplify
parsing. It's not the most elegant solution, but I think it should
work.
[1]: http://article.gmane.org/gmane.comp.version-control.git/162198
builtin/revert.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 66 insertions(+), 9 deletions(-)
After resolving a conflict, the user simply has to `git commit`
(CHERRY_PICK_HEAD will help), and then `git cherry-pick --continue` to
continue the operation. Command-line options are currently
unsupported. Note that a cherry-pick operation cannot be resumed with
a `git revert --continue` and vice-versa.
Signed-off-by: Ramkumar Ramachandra <redacted>
---
As suggested by Miles and Junio, I've obfuscated the variable name to
"contin". Would it make sense to persist the entire replay_opts
structure (so that it applies to all the commits, and no
commit-specific command-line options are supported) somewhere? Where
and how? My idea is that patch can be considered for inclusion then.
Patch 14 onwards can then start adding features like "commit-specific
command-line options", "mixed commands" etc.
builtin/revert.c | 130 ++++++++++++++++++++++++++++++++++++++-
t/t3510-cherry-pick-sequence.sh | 68 ++++++++++++++++++++
2 files changed, 195 insertions(+), 3 deletions(-)
@@ -105,12 +106,37 @@ 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)+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);intnoop;structoptionoptions[]={OPT_BOOLEAN(0,"reset",&opts->reset,"forget the current operation"),+OPT_BOOLEAN(0,"continue",&opts->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)",
@@ -140,9 +166,21 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN);+/* Check for mutually incompatible command line arguments */+verify_opt_mutually_compatible(me,+"--reset",opts->reset,+"--continue",opts->contin,+NULL);+/* Check for incompatible command line arguments */-if(opts->reset){-verify_opt_compatible(me,"--reset",+if(opts->reset||opts->contin){+char*this_operation;+if(opts->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,
@@ -617,6 +655,83 @@ static void format_todo(struct strbuf *buf, struct commit_list *todo_list,}}+staticstructcommit*parse_insn_line(char*start,structreplay_opts*opts)+{+unsignedcharcommit_sha1[20];+charsha1_abbrev[40];+structcommit*commit;+enumreplay_actionaction;+intinsn_len=0;+char*p;++p=start;+if(!(p=strchr(p,' ')))+returnNULL;+insn_len=p-start;+if(!(p=strchr(p+1,' ')))+returnNULL;+p+=1;+strlcpy(sha1_abbrev,start+insn_len+1,+p-(start+insn_len+1));++if(!strncmp(start,"pick",insn_len))+action=CHERRY_PICK;+elseif(!strncmp(start,"revert",insn_len))+action=REVERT;+else+returnNULL;++/*+*Verifythattheactionmatchesupwiththeonein+*opts;wedon'tsupportarbitraryinstructions+*/+if(action!=opts->action)+returnNULL;++if((get_sha1(sha1_abbrev,commit_sha1)<0)+||!(commit=lookup_commit_reference(commit_sha1)))+returnNULL;++returncommit;+}++staticvoidread_populate_todo(structcommit_list**todo_list,+structreplay_opts*opts)+{+structstrbufbuf=STRBUF_INIT;+structcommit_list*new;+structcommit_list**next;+structcommit*commit;+char*p;+intfd;++fd=open(git_path(SEQ_TODO_FILE),O_RDONLY);+if(fd<0){+strbuf_release(&buf);+die_errno(_("Could not open %s."),git_path(SEQ_TODO_FILE));+}+if(strbuf_read(&buf,fd,0)<buf.len){+close(fd);+strbuf_release(&buf);+die(_("Could not read %s."),git_path(SEQ_TODO_FILE));+}+close(fd);++next=todo_list;+for(p=buf.buf;*p;p=strchr(p,'\n')+1){+if(!(commit=parse_insn_line(p,opts))){+strbuf_release(&buf);+die(_("Malformed instruction sheet: %s"),git_path(SEQ_TODO_FILE));+}+new=xmalloc(sizeof(structcommit_list));+new->item=commit;+*next=new;+next=&new->next;+}+*next=NULL;+strbuf_release(&buf);+}+staticvoidwalk_revs_populate_todo(structcommit_list**todo_list,structreplay_opts*opts){
@@ -733,6 +848,14 @@ static int process_continuation(struct replay_opts *opts)if(!file_exists(git_path(SEQ_TODO_FILE)))gotoerror;returncleanup_sequencer_data();+}elseif(opts->contin){+if(!file_exists(git_path(SEQ_TODO_FILE)))+gotoerror;+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
@@ -741,7 +864,8 @@ static int process_continuation(struct replay_opts *opts)*/if(file_exists(git_path(SEQ_TODO_FILE))){error(_("A %s is already in progress"),me);-advise(_("Use %s --reset to forget about it"),me);+advise(_("Use %s --continue to continue the operation"),me);+advise(_("or use %s --reset to forget about it"),me);return-1;}
@@ -48,4 +48,72 @@ test_expect_success '--reset cleans up sequencer directory' 'test_path_is_missing.git/sequencer'+test_expect_success'--continue complains when no cherry-pick is in progress''+pristine_detachinitial&&+test_must_failgitcherry-pick--continue>actual2>&1&&+test_i18ngrep"error"actual+'++test_expect_success'--continue complains when there are unresolved conflicts''+pristine_detachinitial&&+head=$(gitrev-parseHEAD)&&+test_must_failgitcherry-pickbase..picked&&+test_must_failgitcherry-pick--continue&&+gitcherry-pick--reset+'++test_expect_success'--continue continues after conflicts are resolved''+pristine_detachinitial&&+head=$(gitrev-parseHEAD)&&+test_must_failgitcherry-pickbase..picked&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+gitcherry-pick--continue&&+test_path_is_missing.git/sequencer&&+{+gitrev-listHEAD|+gitdiff-tree--root--stdin|+sed"s/[0-9a-f]\{40\}/OBJID/g"+}>actual&&+cat>expect<<-\EOF&&+OBJID+:100644100644OBJIDOBJIDMfoo+OBJID+:100644100644OBJIDOBJIDMunrelated+OBJID+:000000100644OBJIDOBJIDAfoo+:000000100644OBJIDOBJIDAunrelated+EOF+test_cmpexpectactual+'++test_expect_success'malformed instruction sheet 1''+pristine_detachinitial&&+head=$(gitrev-parseHEAD)&&+test_must_failgitcherry-pickbase..picked&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+sed"s/pick /pick/".git/sequencer/todo>new_sheet+cpnew_sheet.git/sequencer/todo+test_must_failgitcherry-pick--continue>actual2>&1&&+gitcherry-pick--reset&&+test_i18ngrep"fatal"actual+'++test_expect_success'malformed instruction sheet 2''+pristine_detachinitial&&+head=$(gitrev-parseHEAD)&&+test_must_failgitcherry-pickbase..picked&&+echo"resolved">foo&&+gitaddfoo&&+gitcommit&&+sed"s/pick/revert/".git/sequencer/todo>new_sheet+cpnew_sheet.git/sequencer/todo+test_must_failgitcherry-pick--continue>actual2>&1&&+gitcherry-pick--reset&&+test_i18ngrep"fatal"actual+'+ test_done
When the sequencer data is persisted after a failed cherry-pick, don't
allow subsequent calls to cherry-pick to clobber this state: instead,
error out with the complaint that an existing cherry-pick is in
progress. To fix existing tests and the "rebase -i" script, introduce
a new "--reset" command-line option to call after every failed
cherry-pick; it essentially clears out the sequencer data, thereby
allowing subsequent calls.
Helped-by: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
This is perfect but for the fact that 'git reset --hard' doesn't blow
away the sequencer state. Why I haven't implemented that yet: should
ONLY a hard reset blow away the state?
builtin/revert.c | 54 ++++++++++++++++++++++++++++++-----
git-rebase--interactive.sh | 25 +++++++++++++---
t/t3032-merge-recursive-options.sh | 2 +
t/t3501-revert-cherry-pick.sh | 1 +
t/t3502-cherry-pick-merge.sh | 9 ++++-
t/t3504-cherry-pick-rerere.sh | 2 +
t/t3505-cherry-pick-empty.sh | 14 ++++-----
t/t3506-cherry-pick-ff.sh | 3 ++
t/t3507-cherry-pick-conflict.sh | 24 ++++++++++++---
t/t3510-cherry-pick-sequence.sh | 14 +++++++++
t/t7502-commit.sh | 1 +
11 files changed, 121 insertions(+), 28 deletions(-)
@@ -625,8 +642,11 @@ static void walk_revs_populate_todo(struct commit_list **todo_list,staticvoidcreate_seq_dir(void){if(file_exists(git_path(SEQ_DIR))){-if(!is_directory(git_path(SEQ_DIR))&&remove_path(git_path(SEQ_DIR))<0)-die(_("Could not remove %s"),git_path(SEQ_DIR));+error(_("%s already exists."),git_path(SEQ_DIR));+advise(_("This usually means that a %s operation is in progress."),me);+advise(_("Use %s --continue to continue the operation"),me);+advise(_("or use %s --reset to forget about it"),me);+die(_("%s failed"),me);}elseif(mkdir(git_path(SEQ_DIR),0777)<0)die_errno(_("Could not create sequencer directory '%s'."),git_path(SEQ_DIR));}
@@ -709,13 +729,31 @@ static int process_continuation(struct replay_opts *opts)read_and_refresh_cache(me,opts);-walk_revs_populate_todo(&todo_list,opts);-create_seq_dir();-if(!get_sha1("HEAD",sha1))-persist_head(sha1_to_hex(sha1));-persist_todo(todo_list,opts);+if(opts->reset){+if(!file_exists(git_path(SEQ_TODO_FILE)))+gotoerror;+returncleanup_sequencer_data();+}else{+/*+*Startanewcherry-pick/revertsequence;but+*first,makesurethatanexistingoneisn'tin+*progress+*/+if(file_exists(git_path(SEQ_TODO_FILE))){+error(_("A %s is already in progress"),me);+advise(_("Use %s --reset to forget about it"),me);+return-1;+}+walk_revs_populate_todo(&todo_list,opts);+create_seq_dir();+if(!get_sha1("HEAD",sha1))+save_head(sha1_to_hex(sha1));+save_todo(todo_list,opts);+}returnpick_commits(todo_list,opts);+error:+returnerror(_("No %s in progress"),me);}intcmd_revert(intargc,constchar**argv,constchar*prefix)
@@ -438,7 +450,10 @@ do_next () {echo"$author_script_content">"$author_script"eval"$author_script_content"outputgitreset--softHEAD^-pick_one-n$sha1||die_failed_squash$sha1"$rest"+pick_one-n$sha1||{+clear_cherry_pick_state+die_failed_squash$sha1"$rest"+}case"$(peek_next_command)"insquash|s|fixup|f)# This is an intermediate commit; its message will only be
@@ -96,6 +96,7 @@ test_expect_success 'revert forbidden on dirty working tree' 'echocontent>extra_file&&gitaddextra_file&&test_must_failgitrevertHEAD2>errors&&+gitrevert--reset&&test_i18ngrep"Your local changes would be overwritten by "errors'
@@ -36,6 +36,7 @@ test_expect_success 'cherry-pick a non-merge with -m should fail' 'gitreset--hard&&gitcheckouta^0&&test_must_failgitcherry-pick-m1b&&+gitcherry-pick--reset&&gitdiff--exit-codea--'
@@ -45,6 +46,7 @@ test_expect_success 'cherry pick a merge without -m should fail' 'gitreset--hard&&gitcheckouta^0&&test_must_failgitcherry-pickc&&+gitcherry-pick--reset&&gitdiff--exit-codea--'
@@ -71,8 +73,8 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should fgitreset--hard&&gitcheckoutb^0&&-test_must_failgitcherry-pick-m3c-+test_must_failgitcherry-pick-m3c&&+gitcherry-pick--reset' test_expect_success'revert a non-merge with -m should fail''
@@ -80,6 +82,7 @@ test_expect_success 'revert a non-merge with -m should fail' 'gitreset--hard&&gitcheckoutc^0&&test_must_failgitrevert-m1b&&+gitcherry-pick--reset&&gitdiff--exit-codec'
@@ -89,6 +92,7 @@ test_expect_success 'revert a merge without -m should fail' 'gitreset--hard&&gitcheckoutc^0&&test_must_failgitrevertc&&+gitcherry-pick--reset&&gitdiff--exit-codec'
@@ -116,6 +120,7 @@ test_expect_success 'revert a merge relative to nonexistent parent should fail'gitreset--hard&&gitcheckoutc^0&&test_must_failgitrevert-m3c&&+gitcherry-pick--reset&&gitdiff--exit-codec'
@@ -23,10 +23,9 @@ test_expect_success setup '' test_expect_success'cherry-pick an empty commit''-gitcheckoutmaster&&{-gitcherry-pickempty-branch^-test"$?"=1-}+gitcheckoutmaster&&+test_expect_code1gitcherry-pickempty-branch^+gitcherry-pick--reset' test_expect_success'index lockfile was removed''
@@ -36,10 +35,9 @@ test_expect_success 'index lockfile was removed' '' test_expect_success'cherry-pick a commit with an empty message''-gitcheckoutmaster&&{-gitcherry-pickempty-branch-test"$?"=1-}+gitcheckoutmaster&&+test_expect_code1gitcherry-pickempty-branch&&+gitcherry-pick--reset' test_expect_success'index lockfile was removed''
@@ -67,12 +67,14 @@ test_expect_success 'merge setup' ' test_expect_success'cherry-pick a non-merge with --ff and -m should fail''gitreset--hardA--&&test_must_failgitcherry-pick--ff-m1B&&+gitcherry-pick--reset&&gitdiff--exit-codeA--' test_expect_success'cherry pick a merge with --ff but without -m should fail''gitreset--hardA--&&test_must_failgitcherry-pick--ffC&&+gitcherry-pick--reset&&gitdiff--exit-codeA--'
@@ -93,6 +95,7 @@ test_expect_success 'cherry pick with --ff a merge (2)' ' test_expect_success'cherry pick a merge relative to nonexistent parent with --ff should fail''gitreset--hardB--&&test_must_failgitcherry-pick--ff-m3C+gitcherry-pick--reset' test_expect_success'cherry pick a root commit with --ff''
@@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' " test_expect_success'failed cherry-pick sets CHERRY_PICK_HEAD''pristine_detachinitial&&test_must_failgitcherry-pickpicked&&-test_cmp_revpickedCHERRY_PICK_HEAD+test_cmp_revpickedCHERRY_PICK_HEAD&&+gitcherry-pick--reset' test_expect_success'successful cherry-pick does not set CHERRY_PICK_HEAD''
@@ -102,7 +107,8 @@ test_expect_success 'failed commit does not clear CHERRY_PICK_HEAD' 'test_must_failgitcherry-pickpicked&&test_must_failgitcommit&&-test_cmp_revpickedCHERRY_PICK_HEAD+test_cmp_revpickedCHERRY_PICK_HEAD&&+gitcherry-pick--reset' test_expect_success'cancelled commit does not clear CHERRY_PICK_HEAD''
@@ -119,7 +125,8 @@ test_expect_success 'cancelled commit does not clear CHERRY_PICK_HEAD' 'test_must_failgitcommit)&&-test_cmp_revpickedCHERRY_PICK_HEAD+test_cmp_revpickedCHERRY_PICK_HEAD&&+gitcherry-pick--reset' test_expect_success'successful commit clears CHERRY_PICK_HEAD''
@@ -34,4 +34,18 @@ test_expect_success 'cherry-pick cleans up sequencer directory upon success' 'test_path_is_missing.git/sequencer'+test_expect_success'--reset complains when no cherry-pick is in progress''+pristine_detachinitial&&+test_must_failgitcherry-pick--reset>actual2>&1&&+test_i18ngrep"error"actual+'++test_expect_success'--reset cleans up sequencer directory''+pristine_detachinitial&&+head=$(gitrev-parseHEAD)&&+test_must_failgitcherry-pickbase..picked&&+gitcherry-pick--reset&&+test_path_is_missing.git/sequencer+'+ test_done
@@ -291,6 +291,7 @@ test_expect_success 'do not fire editor in the presence of conflicts' 'gitcommit-msecond&&# Must fail due to conflicttest_must_failgitcherry-pick-nmaster&&+gitcherry-pick--reset&&echo"editor not started">.git/result&&(GIT_EDITOR="$(pwd)/.git/FAKE_EDITOR"&&
The get_encoding function has only one callsite, and its caller makes
sure that a NULL argument isn't passed. Don't unnecessarily double
check the same argument in get_encoding.
Suggested-by: Jonathan Nieder <redacted>
Suggested-by: Junio C Hamano <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>
---
builtin/revert.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
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: Please, fix them up in the work tree ...
hint: ...
fatal: Exiting because of an unresolved conflict.
Thanks! Personally, I like it (since the tags on the left make it a
little clearer to the reader what is happening).
Rather than copy+pasting this code verbatim, wouldn't it make sense to
move it and expose it through advice.h so the old call site can use
the same code?
For what it's worth, with that change,
Reviewed-by: Jonathan Nieder <redacted>
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Hi,
Ramkumar Ramachandra wrote:
Currently, the return value from revert_or_cherry_pick is a
non-negative number representing the intended exit status from `git
revert` or `git cherry-pick`. Change this by replacing some of the
calls to "die" with calls to "error", so that it can return negative
values too. Postive return values indicate conflicts, while negative
The above seems to be suggesting that the current return value is a
_problem_, and that this change _fixes_ it.
But I had thought that the bulk of this patch's changes (die-to-error
conversions) were not meant as a means to that end but an end in
themselves. Wouldn't a clearer problem statement be "Currently,
revert_or_cherry_pick can fail in two ways. If it encounters
conflicts, it returns a positive number indicating the intended exit
status for the git wrapper to pass on; for all other errors, it
die()s. Some callers may not like the latter behavior because of
<reasons here>"?
Only after the reader understands that, she will be ready to
appreciate the value of the proposed alternate return value
convention. Similar comments apply to the commit messages of the few
patches before --- they are not terribly confusing, but they could
still easily be improved by mentioning what problem the patches are
supposed to solve.
quoted hunk
--- a/builtin/revert.c+++ b/builtin/revert.c
@@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
[...]
+ if (action == CHERRY_PICK)
+ error(_("Your local changes would be overwritten by %s."), me);
+ else
+ error(_("Your local changes would be overwritten by %s."), me);
gettext creates one msgid for these two strings, so translators have
no choice but to give them the same translation. Is that the intent?
[...]
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Since we want to develop the functionality to either pick or revert
individual commits atomically later in the series, make "commit" a
variable to be passed around explicitly as an argument for clarity.
This involves changing several functions to take an additional
argument, but no functional changes. Additionaly, this will permit
more than one commit to be cherry-picked at once, should we choose to
develop this functionality in future.
I don't understand the last sentence above --- doesn't "git cherry-pick
A B" work already?
The patch looks good, except for:
[...]
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
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 variable "me" is left as a file-scope static variable because it
is not an independent option. "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.
Hm, would it make sense for there to be a "private" section at the
end of the replay_opts struct for variables like this?
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.
I would be interested in fixing that (as a patch on top, maybe).
What do you suggest:
A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
manipulate a static "struct replay_opts" while pick_commits et al
pass around a pointer to it
B. Make parse_args work like this:
copy from argument to private static struct replay_opts
call parse_options()
copy private static struct replay_opts to argument
C. Use new option types:
OPT_BOOL_MEMBER('n', "no-commit",
offsetof(struct replay_opts, no_commit),
"don't automatically commit"),
and teach parse_options to take an additional parameter like it
takes "prefix" now, to be used as a base address for options that
write to an offset instead of a pointer
I'm leaning towards A but not sure if that would be wasted work in
light of your plans for these APIs in the long run (i.e., is
parse_args() going to be exposed and want to act on a caller-supplied
struct)?
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
quoted hunk
--- a/builtin/revert.c+++ b/builtin/revert.c
[...]
quoted hunk
@@ -612,7 +610,10 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix) memset(&opts, 0, sizeof(struct replay_opts)); opts.action = CHERRY_PICK;- res = revert_or_cherry_pick(argc, argv, &opts);+ git_config(git_default_config, NULL);+ me = "cherry-pick";+ parse_args(argc, argv, &opts);+ res = pick_commits(&opts); if (res < 0) die(_("%s failed"), me); return res;
I'd put the "me =" line right after "opts.action =" if doing it this
way. This means callers to pick_commits() are responsible for setting
the "me" variable and in particular it will not make sense to export
that function to callers outside of this file any more, right?
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
The "--ff" command-line option cannot be used with four other
command-line options. However, when these options are specified with
"--ff" on the command-line, parse_args will still parse these
incompatible options into a replay_opts structure for use by the rest
of the program. Although pick_commits checks the validity of the
replay_opts strucutre before before starting its operation, this is
inelegant design; pick_commits is currently the gatekeeper to the
cherry-pick machinery, but this will change 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. Also ensure that regressions in maintaining this
invariant are caught in the future by adding an assertion in
pick_commits.
Agh! The above seems totally convoluted, and worse, I can see some of
my own words in there so I feel I am to blame. Could you please
explain, simply, as though I am just an ordinary person, what the idea
of this patch is? I've heard you talk before. You are quite capable
of explaining things clearly.
The patch itself looks good.
Rather than copy+pasting this code verbatim, wouldn't it make sense to
move it and expose it through advice.h so the old call site can use
the same code?
Yes, but I was worried that I shouldn't expose it because your commit
message (2a41df) says:
It is local to revert.c for now because I am not sure this is
the right API (we may want to take an array of advice lines or a
boolean argument for easy suppression of unwanted advice).
So, is it still alright to expose it in advice.h?
For what it's worth, with that change,
Reviewed-by: Jonathan Nieder <redacted>
@@ -250,25 +250,20 @@ static struct tree *empty_tree(void)
[...]
quoted
+ if (action == CHERRY_PICK)
+ error(_("Your local changes would be overwritten by %s."), me);
+ else
+ error(_("Your local changes would be overwritten by %s."), me);
gettext creates one msgid for these two strings, so translators have
no choice but to give them the same translation. Is that the intent?
[...]
memset(&opts, 0, sizeof(struct replay_opts));
opts.action = CHERRY_PICK;
- res = revert_or_cherry_pick(argc, argv, &opts);
+ git_config(git_default_config, NULL);
+ me = "cherry-pick";
+ parse_args(argc, argv, &opts);
+ res = pick_commits(&opts);
if (res < 0)
die(_("%s failed"), me);
return res;
I'd put the "me =" line right after "opts.action =" if doing it this
way. This means callers to pick_commits() are responsible for setting
the "me" variable and in particular it will not make sense to export
that function to callers outside of this file any more, right?
Since we want to develop the functionality to either pick or revert
individual commits atomically later in the series, make "commit" a
variable to be passed around explicitly as an argument for clarity.
This involves changing several functions to take an additional
argument, but no functional changes. Additionaly, this will permit
more than one commit to be cherry-picked at once, should we choose to
develop this functionality in future.
I don't understand the last sentence above --- doesn't "git cherry-pick
A B" work already?
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
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.
I think I remember Junio being curious about which commit is stored in
"head"; this might be a good place to put a reminder so future readers
don't have to be confused.
quoted hunk
--- a/builtin/revert.c+++ b/builtin/revert.c
[...]
quoted hunk
@@ -25,6 +26,10 @@ * Copyright (c) 2005 Junio C Hamano */+#define SEQ_DIR "sequencer"+#define SEQ_HEAD_FILE "sequencer/head"+#define SEQ_TODO_FILE "sequencer/todo"
Yay. :)
quoted hunk
@@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) return error(_("Your index file is unmerged.")); } else { if (get_sha1("HEAD", head))- return error(_("You do not have a valid HEAD"));+ return error(_("Can't %s on an unborn branch"), me);
Remember that "me" is an untranslated command name, and see also
http://thread.gmane.org/gmane.comp.version-control.git/153026
Perhaps it would make sense to do something like
if (get_sha1("HEAD", head)) {
if (opts->action == REVERT)
return error(_("can't revert as initial commit"));
return error(_("cherry-pick into empty head not supported yet"));
}
In a way they feel like different operations, anyway. On the other
hand, there's no reason I can think of not to allow reverting a patch
that only removes files as the initial commit other than not having
implemented it.
Maybe some word like "command", "insn", or "keyword" would be more
suggestive than "action". It also might be worth mentioning somewhere
(in the commit message?) that this format is inspired by
rebase--interactive's insn sheet.
+ }
+}
+
+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 *new;
+ struct commit_list **next;
+
+ prepare_revs(&revs, opts);
+
+ /* Insert into todo_list in the same order */
+ /* NEEDSWORK: Expose this as commit_list_append */
+ next = todo_list;
+ while ((commit = get_revision(&revs))) {
+ new = xmalloc(sizeof(struct commit_list));
+ new->item = commit;
+ *next = new;
+ next = &new->next;
+ }
+ *next = NULL;
The operation that could be exposed does not include get_revision,
does it?
/*
* Example:
*
* struct commit_list *list;
* struct commit_list **next = &list;
*
* next = commit_list_append(c1, next);
* next = commit_list_append(c2, next);
* *next = NULL;
* assert(commit_list_count(list) == 2);
* return list;
*
* Don't forget to NULL-terminate!
*/
struct commit_list **commit_list_append(struct commit *commit,
struct commit_list **next)
{
struct commit_list *new = xmalloc(sizeof(*new_list));
new->item = commit;
*next = new;
return &new->next;
}
+static void create_seq_dir(void)
+{
+ if (file_exists(git_path(SEQ_DIR))) {
+ if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+ die(_("Could not remove %s"), git_path(SEQ_DIR));
+ } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+ die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}
A local variable to cache the git_path result would make this much
easier to read.
Thanks for thinking about these things. Maybe another test
demonstrating that the .git/sequencer directory is left behind on
failure would help put this in context.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Yes, but I was worried that I shouldn't expose it because your commit
message (2a41df) says:
It is local to revert.c for now because I am not sure this is
the right API (we may want to take an array of advice lines or a
boolean argument for easy suppression of unwanted advice).
So, is it still alright to expose it in advice.h?
Well, presumably this second caller is evidence that it is the right
API, no? :)
Of course the API can still easily be changed later.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
This is perfect but for the fact that 'git reset --hard' doesn't blow
away the sequencer state. Why I haven't implemented that yet: should
ONLY a hard reset blow away the state?
I don't know. What do you think?
The constraints I see are:
1. outside scripts that use "git cherry-pick" should continue to work
2. as a small indication that that's vaguely possible, unrelated parts
of the test suite should not need to be patched
3. when a person uses commands like "git reset --hard" without
_intending_ to blow away the sequencer state, it should be possible
to get the sequencer state back.
For dealing with "git rebase --interactive" and similar porcelain-ish
scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling
back to traditional behavior when the GIT_CHERRY_PICK_HELP environment
variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow
overriding the help text by the calling Porcelain, 2007-11-28).
Maybe:
int just_remove_state;
int resume;
int abort;
Or:
enum replay_subcommand {
REPLAY_RESET,
REPLAY_CONTINUE,
REPLAY_ABORT
};
enum replay_subcommand subcommand;
Or perhaps this does not need to be part of the replay_opts struct but
can be communicated by which API function gets called (e.g., via
parse_args returning an "enum replay_subcommand"). I dunno.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
I've intentionally left parse_cmdline_args unimplemented
[...]
Existing implementations in libraries like Glib are
much too heavyweight.
Wait, how did glib enter the picture? :) The implementation of
shell-style quoting in [1] is not very complicated; perhaps it could
complement git's existing parsers for shell-style single-quoted
expressions and C-style double-quoted expressions in quote.c.
Of course, a more basic question is whether we want to allow passing
arbitrary command-line arguments through the insn sheet at all (a
part of me wishes "no", at least at first).
Could you give an example to illustrate what this functionality would
be used for? I can understand wanting to pass "-s" and "-X" flags to
a merge insn and "-X" to pick, but that's as far as my imagination
goes.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Write a new function called process_continuation to prepare a
todo_list to call pick_commits with; the job of pick_commits is
simplified into performing the tasks listed in todo_list.
Why is it called process_continuation? What is its responsibility?
When would I call it?
+ /*
+ * Decide what to do depending on the arguments; a fresh
+ * cherry-pick should be handled differently from an existing
+ * one that is being continued
+ */
+ res = process_continuation(&opts);
Is this the new sole entry point to the cherry-pick/revert machinery?
In that case, I'd be mildly tempted to call it something crazy like
start_or_continue_replay(), and even more tempted to split it into
separate entry points for new_replay(), continue_replay(),
abort_replay(), and remove_replay_state() (but please don't trust me
about the names; this is just to get the idea across).
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Thanks for reading.
Thanks for the many improvements!
I think the hardest piece in the series so far is what to do about
the
git cherry-pick something-i-shouldnt-cherry-pick
git reset --hard; # or git reset --merge HEAD
git cherry-pick what-i-should-have-cherry-picked-instead
habit. Aside from that, it would be nice to have more tests and some
simple documentation, and most other things I found were nitpicks.
I've intentionally left parse_cmdline_args unimplemented
[...]
quoted
Existing implementations in libraries like Glib are
much too heavyweight.
Wait, how did glib enter the picture? :)
Completely unrelated -- I just incidentally saw "g_shell_parse_argv"
in glib which does what we want, but it's an overkill.
The implementation of
shell-style quoting in [1] is not very complicated; perhaps it could
complement git's existing parsers for shell-style single-quoted
expressions and C-style double-quoted expressions in quote.c.
Okay.
Of course, a more basic question is whether we want to allow passing
arbitrary command-line arguments through the insn sheet at all (a
part of me wishes "no", at least at first).
I have an overwhelming desire to say "no", but I can't think of an alternative.
Could you give an example to illustrate what this functionality would
be used for? I can understand wanting to pass "-s" and "-X" flags to
a merge insn and "-X" to pick, but that's as far as my imagination
goes.
I wasn't imagining anything else. That's just it -- I've just been
breaking my head trying to figure out how to do it :|
-- Ram
This is perfect but for the fact that 'git reset --hard' doesn't blow
away the sequencer state. Why I haven't implemented that yet: should
ONLY a hard reset blow away the state?
I don't know. What do you think?
I personally don't think any other kind of reset would warrant blowing
away the sequencer state.
The constraints I see are:
1. outside scripts that use "git cherry-pick" should continue to work
2. as a small indication that that's vaguely possible, unrelated parts
of the test suite should not need to be patched
I don't see how this is possible. I'm tempted to say that we should
call this new cherry-picking mechanism with the sequencing
functionality something else like "git sequencer"; then it's possible
to avoid breaking existing scripts.
3. when a person uses commands like "git reset --hard" without
_intending_ to blow away the sequencer state, it should be possible
to get the sequencer state back.
I like your suggestion here -- simply move .git/sequencer to
.git/sequencer-old or similar :)
For dealing with "git rebase --interactive" and similar porcelain-ish
scripts, the CHERRY_PICK_HEAD code-path has its own trick of falling
back to traditional behavior when the GIT_CHERRY_PICK_HELP environment
variable is set (see v1.5.4-rc0~106^2~1, revert/cherry-pick: Allow
overriding the help text by the calling Porcelain, 2007-11-28).
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 variable "me" is left as a file-scope static variable because it
is not an independent option. "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.
Hm, would it make sense for there to be a "private" section at the
end of the replay_opts struct for variables like this?
No. My justification: in later steps, we'd want to be able to mix
"pick" and "revert" instructions in the same instruction sheet. This
will essentially require the parser to return a commit + a replay_opts
struct (which will contain the action information). There's little
point in storing 100 "revert" strings for the 100 commits we want to
pick when that can easily be inferred from the action.
quoted
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.
I would be interested in fixing that (as a patch on top, maybe).
What do you suggest:
A. Apply patch 8 and make cmd_revert, cmd_cherry_pick, and parse_args
manipulate a static "struct replay_opts" while pick_commits et al
pass around a pointer to it
B. Make parse_args work like this:
copy from argument to private static struct replay_opts
call parse_options()
copy private static struct replay_opts to argument
This is something you suggested earlier, but I find it extremely inelegant.
C. Use new option types:
OPT_BOOL_MEMBER('n', "no-commit",
offsetof(struct replay_opts, no_commit),
"don't automatically commit"),
and teach parse_options to take an additional parameter like it
takes "prefix" now, to be used as a base address for options that
write to an offset instead of a pointer
I'm leaning towards A but not sure if that would be wasted work in
light of your plans for these APIs in the long run (i.e., is
parse_args() going to be exposed and want to act on a caller-supplied
struct)?
Yes, I'm definitely considering exposing parse_args in the future,
especially since I want to support command-line options in my
instruction sheet. Implementing (C) correctly will probably have
several long-term benefits as well -- what do you feel about it?
-- Ram
Write a new function called process_continuation to prepare a
todo_list to call pick_commits with; the job of pick_commits is
simplified into performing the tasks listed in todo_list.
Why is it called process_continuation? What is its responsibility?
When would I call it?
I wanted a general name for the features I'm writing (--reset,
--continue): I want to call these "continuation features". I
personally like the term, because it reminds me of the "call/cc" in
Scheme.
quoted
+ /*
+ * Decide what to do depending on the arguments; a fresh
+ * cherry-pick should be handled differently from an existing
+ * one that is being continued
+ */
+ res = process_continuation(&opts);
Is this the new sole entry point to the cherry-pick/revert machinery?
Yes.
In that case, I'd be mildly tempted to call it something crazy like
start_or_continue_replay(), and even more tempted to split it into
separate entry points for new_replay(), continue_replay(),
abort_replay(), and remove_replay_state() (but please don't trust me
about the names; this is just to get the idea across).
Why? Is introducing new terminology so bad? Should I explain what I
mean by "continuation" in the commit message/ a comment?
-- Ram
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Jonathan Nieder writes:
quoted
In that case, I'd be mildly tempted to call it something crazy like
start_or_continue_replay()
[...]
Why? Is introducing new terminology so bad? Should I explain what I
mean by "continuation" in the commit message/ a comment?
If "process_continuation" means "parse .git/sequencer state, which we
are pretending is a serialized continuation object, and either (a)
call it, (b) throw it away, or (c) modify it and then call it", then
yes, how do you expect anyone to know what you are talking about?
Less importantly, starting a cherry-pick (which is what pick_commits()
already does) doesn't seem to fit in that picture.
A simpler jargon-filled description of this model is checkpoint/
restart. But it is an incomplete analogy and still not a great name.
With a goal of making future writers' lives happier and more
productive in mind, I do not think it is often worth confusing them by
choosing a clever presentation of ideas instead of a clear one.
In that case, I'd be mildly tempted to call it something crazy like
start_or_continue_replay()
[...]
quoted
Why? Is introducing new terminology so bad? Should I explain what I
mean by "continuation" in the commit message/ a comment?
If "process_continuation" means "parse .git/sequencer state, which we
are pretending is a serialized continuation object, and either (a)
call it, (b) throw it away, or (c) modify it and then call it", then
yes, how do you expect anyone to know what you are talking about?
Less importantly, starting a cherry-pick (which is what pick_commits()
already does) doesn't seem to fit in that picture.
A simpler jargon-filled description of this model is checkpoint/
restart. But it is an incomplete analogy and still not a great name.
With a goal of making future writers' lives happier and more
productive in mind, I do not think it is often worth confusing them by
choosing a clever presentation of ideas instead of a clear one.
Thanks for the elaborate explanation; I can see what's wrong with it
now. However, I "start_or_continue_or_stop_or_[insert more options
here]_replay" isn't a good name. I want something future-proof,
because I intend to extend this with more nifty helpers like "skip
one". Your earlier "pick_revisions" suggestion doesn't sound like a
bad alternative now -- let me know if you have any other suggestions.
Thanks.
-- Ram
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
Thanks for the elaborate explanation; I can see what's wrong with it
now. However, I "start_or_continue_or_stop_or_[insert more options
here]_replay" isn't a good name. I want something future-proof,
because I intend to extend this with more nifty helpers like "skip
one". Your earlier "pick_revisions" suggestion doesn't sound like a
bad alternative now -- let me know if you have any other suggestions.
Sounds like sensible reasoning. You are free to choose a name; I just
wanted to make sure the effect of that name is clear.
If I were doing it, I would let the API include multiple entry points,
one for each operation ("start", "abort", "skip one", etc).
Side note: I believe a previous patch had a justification of allowing
for multiple entry points, which I had thought was preparation for
this. It is possible that that patch some other intended positive
effects, too, though.
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.
I think I remember Junio being curious about which commit is stored in
"head"; this might be a good place to put a reminder so future readers
don't have to be confused.
Oops, I totally forgot -- sorry Junio.
He suggested that we store the corresponding ref also somewhere. Have
to think about this some more before the next iteration.
quoted
--- a/builtin/revert.c+++ b/builtin/revert.c
[...]
quoted
@@ -25,6 +26,10 @@
* Copyright (c) 2005 Junio C Hamano
*/
+#define SEQ_DIR "sequencer"
+#define SEQ_HEAD_FILE "sequencer/head"
+#define SEQ_TODO_FILE "sequencer/todo"
Yay. :)
Sorry it took me so long to understand this. Your elaborate
explanation last time drove the point home.
return error(_("Your index file is unmerged."));
} else {
if (get_sha1("HEAD", head))
- return error(_("You do not have a valid HEAD"));
+ return error(_("Can't %s on an unborn branch"), me);
Remember that "me" is an untranslated command name, and see also
http://thread.gmane.org/gmane.comp.version-control.git/153026
Perhaps it would make sense to do something like
if (get_sha1("HEAD", head)) {
if (opts->action == REVERT)
return error(_("can't revert as initial commit"));
return error(_("cherry-pick into empty head not supported yet"));
}
In a way they feel like different operations, anyway. On the other
hand, there's no reason I can think of not to allow reverting a patch
that only removes files as the initial commit other than not having
implemented it.
Okay. That would be unrelated to this patch though -- I'll make it a
separate patch and move it to the beginning of the series.
Maybe some word like "command", "insn", or "keyword" would be more
suggestive than "action". It also might be worth mentioning somewhere
(in the commit message?) that this format is inspired by
rebase--interactive's insn sheet.
Okay, will do.
The operation that could be exposed does not include get_revision,
does it?
/*
* Example:
*
* struct commit_list *list;
* struct commit_list **next = &list;
*
* next = commit_list_append(c1, next);
* next = commit_list_append(c2, next);
* *next = NULL;
* assert(commit_list_count(list) == 2);
* return list;
*
* Don't forget to NULL-terminate!
*/
struct commit_list **commit_list_append(struct commit *commit,
struct commit_list **next)
{
struct commit_list *new = xmalloc(sizeof(*new_list));
new->item = commit;
*next = new;
return &new->next;
}
I would have done this, but I was worried about what the NULL
termination would mean API-wise. In retrospect, a lot of APIs
described in Documentation/technical are pretty non-trivial, and it's
not obvious how to use it without the documentation. Would it be okay
to expose this in commit.c and write some documentation? I already
have two callers.
quoted
+static void create_seq_dir(void)
+{
+ if (file_exists(git_path(SEQ_DIR))) {
+ if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
+ die(_("Could not remove %s"), git_path(SEQ_DIR));
+ } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
+ die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
+}
A local variable to cache the git_path result would make this much
easier to read.
Thanks for thinking about these things. Maybe another test
demonstrating that the .git/sequencer directory is left behind on
failure would help put this in context.
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra wrote:
My justification: in later steps, we'd want to be able to mix
"pick" and "revert" instructions in the same instruction sheet. This
will essentially require the parser to return a commit + a replay_opts
struct (which will contain the action information).
Side note: is it intended to support insns like
pick A..B
?
Anyway, the above explanation about the intended use for "struct
replay_opts" (it needs to be small, I guess?) would be a good thing
to add to the commit message, too. Basically, whatever information a
person needs in order to understand the design is a useful thing to
add.
[...]
Yes, I'm definitely considering exposing parse_args in the future,
especially since I want to support command-line options in my
instruction sheet.
Hm, I am not sure what to think about this direction (is the git
sequencer actually just a fast git shell? in that case, why is pick
spelled "pick" instead of "cherry-pick"?). Maybe it's a good thing.
Thanks for some useful clarifications.
By making `reset --hard` blow away the sequencer state, and trying
hard not to modify existing scripts, the diffstat still contains these
files:
builtin/revert.c
git-rebase--interactive.sh
t/t3032-merge-recursive-options.sh
r/t3505-cherry-pick-empty.sh
r/t3507-cherry-pick-conflict.sh
r/t3501-cherry-pick-sequence.sh
@@ -55,6 +56,7 @@ test_expect_success 'advice from failed cherry-pick' "
hint: and commit the result with 'git commit'
EOF
test_must_fail git cherry-pick picked 2>actual &&
+ git cherry-pick --reset &&
test_i18ncmp expected actual
"
@@ -62,7 +64,8 @@ test_expect_success 'advice from failed cherry-pick' "
EOF
test_must_fail git revert picked &&
+ git revert --reset &&
sed "s/[a-f0-9]*\.\.\./objid/" foo > actual &&
test_cmp expected actual
As you can see, there is no "reset --hard" in these, and I don't see
what other command I can piggy-bank on to blow away the sequencer
state. There is however, one other thing I can do: if there is
nothing left to cherry-pick after a successful conflict resolution +
git commit, I can modify commit.c to blow away the sequencer state
after checking appropriately. This will also have a nice end-user
experience side-effect:
$ git cherry-pick moo
fatal: Conflict in foo!
$ echo "Resolved" > foo
$ git add moo
$ git commit
$ git cherry-pick --continue # This no-op will be unnecessary
Then again, teaching commit about the sequencer is inelegant, and it's
possible to achieve this effect in another way: when a conflict is
encountered in the sequencer && length(todo_file) == 1, throw away the
sequencer state. When I say "throw away", I really mean "move
.git/sequencer to .git/sequencer-old". Does this seem reasonable?
-- Ram
This one is not a typical script, I think --- if you knew the
cherry-pick was going to be empty, why did you try it in the first
place? I think it would make sense to make it "git reset --hard" at
the beginning of each test as a separate, preparatory patch with
explanation.
[...]
There is however, one other thing I can do: if there is
nothing left to cherry-pick after a successful conflict resolution +
git commit, I can modify commit.c to blow away the sequencer state
after checking appropriately. This will also have a nice end-user
experience side-effect:
$ git cherry-pick moo
fatal: Conflict in foo!
$ echo "Resolved" > foo
$ git add moo
$ git commit
$ git cherry-pick --continue # This no-op will be unnecessary
Though it's not obvious to me how this would affect the scripts above,
it sounds like a nice enhancement to me independently, fwiw.
Then again, teaching commit about the sequencer is inelegant,
It's possible to add some hook-like thing to do this, or to structure
the code as if a hook was used.
and it's
possible to achieve this effect in another way: when a conflict is
encountered in the sequencer && length(todo_file) == 1, throw away the
sequencer state.
Yep, that seems like basically the same effect. Are there downsides?
(Maybe years from now when a "git cherry-pick --rewind" is introduced
we would regret this? But that can be figured out years from now.)
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:33
Jonathan Nieder wrote:
Ramkumar Ramachandra wrote:
quoted
and it's
possible to achieve this effect in another way: when a conflict is
encountered in the sequencer && length(todo_file) == 1, throw away the
sequencer state.
Yep, that seems like basically the same effect. Are there downsides?
(Maybe years from now when a "git cherry-pick --rewind" is introduced
we would regret this? But that can be figured out years from now.)
Doh, I'm not thinking straight. Would this break "git cherry-pick
--abort", or is there some hack layered on top to avoid that? Making
"git commit" remove the .git/sequencer for the last commit of the
sequence seems a little saner.
From: Junio C Hamano <hidden> Date: 2016-06-15 22:51:33
Ramkumar Ramachandra [off-list ref] writes:
The variable "me" is left as a file-scope static variable because it
is not an independent option. "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.
Would it make more sense to remove the variable, pass "action" around
where only "me" is passed around right now, and introduce a function
"static const char *action_name()" to help places that wants textual
command name for display purposes?
I wonder if the optimization to allow omitting unnecessary NULL
termination inside a tight loop by the caller is really worth the
potential trouble, though.
As a strict matter of language, no. And example we see far too often
@work is that MS Office 2010 is compatible with MS Office 2007, but the
same cannot be said the other way around. (Now if I can convince them
that not everyone in the world has MS Office...)
I.e. verify-option-compatibility perhaps?
+1 sensible, if that's the intention.
--
-Drew Northup
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
This one is not a typical script, I think --- if you knew the
cherry-pick was going to be empty, why did you try it in the first
place? I think it would make sense to make it "git reset --hard" at
the beginning of each test as a separate, preparatory patch with
explanation.
[...]
quoted
There is however, one other thing I can do: if there is
nothing left to cherry-pick after a successful conflict resolution +
git commit, I can modify commit.c to blow away the sequencer state
after checking appropriately. This will also have a nice end-user
experience side-effect:
$ git cherry-pick moo
fatal: Conflict in foo!
$ echo "Resolved" > foo
$ git add moo
$ git commit
$ git cherry-pick --continue # This no-op will be unnecessary
Though it's not obvious to me how this would affect the scripts above,
it sounds like a nice enhancement to me independently, fwiw.
Oh, it fixes everything :)
Just see my GitHub fork.
quoted
Then again, teaching commit about the sequencer is inelegant,
It's possible to add some hook-like thing to do this, or to structure
the code as if a hook was used.
quoted
and it's
possible to achieve this effect in another way: when a conflict is
encountered in the sequencer && length(todo_file) == 1, throw away the
sequencer state.
Yep, that seems like basically the same effect. Are there downsides?
(Maybe years from now when a "git cherry-pick --rewind" is introduced
we would regret this? But that can be figured out years from now.)
Doh, I'm not thinking straight. Would this break "git cherry-pick
--abort", or is there some hack layered on top to avoid that? Making
"git commit" remove the .git/sequencer for the last commit of the
sequence seems a little saner.
We could always inject a hack to avoid this. I'm not yet convinced
that we should teach commit about the sequencer, especially since the
patch to do it from the sequencer end is so simple. Please have a
look at the patch [1], and let me know what you think. I could submit
it as an RFC patch to the list for convenience, but I'm afraid it'll
be missing context.
[1]: https://github.com/artagnon/git/commit/0653bcccfa8d69687ed939f07f5b32dd14d302d3
-- Ram
Isn't "being compatible" by definition "mutual"?
I.e. verify-option-compatibility perhaps?
There's already a verify_opt_compatible which checks that the first
option supplied is compatible with all the other options. This one
checks that all the the options all the options are compatible with
each other. What names do you suggest?
-- Ram
Hi Jonathan,
Sorry about the delayed replay -- I intended to reply to this earlier;
I'm not sure why I didn't.
Jonathan Nieder writes:
quoted
My justification: in later steps, we'd want to be able to mix
"pick" and "revert" instructions in the same instruction sheet. This
will essentially require the parser to return a commit + a replay_opts
struct (which will contain the action information).
Side note: is it intended to support insns like
pick A..B
?
I wouldn't point to that instruction specifically, but yes- I plan to
support complex instructions in future.
Anyway, the above explanation about the intended use for "struct
replay_opts" (it needs to be small, I guess?) would be a good thing
to add to the commit message, too. Basically, whatever information a
person needs in order to understand the design is a useful thing to
add.
I'm not yet sure about this. On a related note, I'd like to ask: will
we encounter two commands like "am" and "revert" which have the same
command-line option name for two different functionality? If not, I'd
probably like to stick to the per-session opts for as long as possible
(ie. until someone finds a good usecase + implementation for
per-action command-line options).
quoted
Yes, I'm definitely considering exposing parse_args in the future,
especially since I want to support command-line options in my
instruction sheet.
Hm, I am not sure what to think about this direction (is the git
sequencer actually just a fast git shell? in that case, why is pick
spelled "pick" instead of "cherry-pick"?). Maybe it's a good thing.
This is a very important question. Yes, it's intentionally named
"pick" and not "cherry-pick" because I don't want the Sequencer to
merely be a fast git shell. That's part of the reason I don't want
arbitrary command-line options on the insn sheet. I want the
possibility of accommodating complex instructions in the insn sheet
(single instructions that might even involve talking to more than one
git command). For the same reason, I also felt that "action" is the
most appropriate name for the first word in each line in the insn
sheet. What I'd call an "instruction" is an action + the relevant
option(s) picked up from the opts sheet.
-- Ram
Hi Junio,
Again- intended to reply to this earlier; sorry.
Junio C Hamano writes:
Ramkumar Ramachandra [off-list ref] writes:
quoted
The variable "me" is left as a file-scope static variable because it
is not an independent option. "me" is simply a string that needs to
be inferred from the "action" option, and is kept global to save each
function the trouble of determining it independently.
Would it make more sense to remove the variable, pass "action" around
where only "me" is passed around right now, and introduce a function
"static const char *action_name()" to help places that wants textual
command name for display purposes?
Okay, let me put it like this: "me" exists because cherry-pick and
revert functionalities are mixed in the same file; builtin/revert.c.
In future, the sequencer in general will support many more actions --
and we will definitely require an "opts->action to instruction sheet
keyword" translation, and that'll probably be some sort of struct.
Since I'm not sure the function you propose will make it to
sequencer.c, I don't want to introduce it now. Let's wait and see how
it shapes up.
Thanks.
-- Ram
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:34
Ramkumar Ramachandra wrote:
Since I'm not sure the function you propose will make it to
sequencer.c, I don't want to introduce it now.
Wouldn't it be easy to remove such a function later?
Practically speaking, it is not obvious to me that making any of these
variables non-static is needed for "cherry-pick --continue" to work,
but given that most of the state is being made non-static anyway,
readers will be likely to wonder why "me" is left behind. So the
obvious choices would be to
a. make "me" a member of the replay_opts struct; or
b. compute "me" in each function that needs it by calling a helper
function; or
c. add some explanation to the commit message to clarify the status
of "me" as static-but-won't-be-in-the-long-term and a reason for
that
based on the needs of the current code. (b) sounds simplest to me,
though I haven't tried it.