Hi,
I've started working on building a sequencer for Git. While the
outline is described in [1], I'd like some additional clarifications.
A big thanks to Christian's series [2] for the valuable roadmap.
Please note that 10/11 is not related to this series, but seems to be
a minor nit that's required to make all existing tests pass.
0. Is the general flow alright?
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly
inelegant, mainly because of the command-line argument parsing
overhead. Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate. Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
4. I have a feeling that I've broken translation strings. Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?
Thanks for reading.
-- Ram
[1]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170908
[2]: http://thread.gmane.org/gmane.comp.version-control.git/162183
[3]: http://thread.gmane.org/gmane.comp.version-control.git/170758/focus=170834
Ramkumar Ramachandra (11):
revert: Avoid calling die; return error instead
revert: Lose global variables "commit" and "me"
revert: Introduce a struct to parse command-line options into
revert: Separate cmdline argument handling from the functional code
revert: Catch incompatible command-line options early
revert: Implement parsing --continue, --abort and --skip
revert: Handle conflict resolutions more elegantly
usage: Introduce error_errno correspoding to die_errno
revert: Write head, todo, done files
revert: Give noop a default value while argument parsing
revert: Implement --abort processing
advice.c | 14 ++
advice.h | 1 +
builtin/revert.c | 454 ++++++++++++++++++++++++++++++++++++++---------------
git-compat-util.h | 2 +
usage.c | 34 ++++
5 files changed, 381 insertions(+), 124 deletions(-)
--
1.7.4.rc1.7.g2cf08.dirty
@@ -265,23 +265,23 @@ 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"+returnerror(_("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"+returnerror(_("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"));+returnerror(_("Your local changes would be overwritten by revert.\n"));else-die(_("Your local changes would be overwritten by cherry-pick.\n"));+returnerror(_("Your local changes would be overwritten by cherry-pick.\n"));}}}
@@ -331,7 +331,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);+returnerror(_("%s: Unable to write new index file"),me);rollback_lock_file(&index_lock);if(!clean){
@@ -397,18 +397,18 @@ 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();if(!commit->parents){if(action==REVERT)-die(_("Cannot revert a root commit"));+returnerror(_("Cannot revert a root commit"));parent=NULL;}elseif(commit->parents->next){
@@ -417,19 +417,19 @@ 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"),+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."),+}elseif(mainline>0)+returnerror(_("Mainline was specified but commit %s is not a merge."),sha1_to_hex(commit->object.sha1));elseparent=commit->parents->item;
@@ -440,11 +440,11 @@ 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"),+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"),+returnerror(_("Cannot get commit message for %s"),sha1_to_hex(commit->object.sha1));/*
@@ -523,7 +523,7 @@ static int do_pick_commit(void)returnres;}-staticvoidprepare_revs(structrev_info*revs)+staticintprepare_revs(structrev_info*revs){intargc;
@@ -533,34 +533,39 @@ static void prepare_revs(struct rev_info *revs)revs->reverse=1;argc=setup_revisions(commit_argc,commit_argv,revs,NULL);-if(argc>1)-usage(*revert_or_cherry_pick_usage());+if(argc>1){+fprintf(stderr,"usage: %s",_(*revert_or_cherry_pick_usage()));+return129;+}if(prepare_revision_walk(revs))-die(_("revision walk setup failed"));+returnerror(_("revision walk setup failed"));if(!revs->commits)-die(_("empty commit set passed"));+returnerror(_("empty commit set passed"));+return0;}-staticvoidread_and_refresh_cache(constchar*me)+staticintread_and_refresh_cache(constchar*me){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);+returnerror(_("%s: failed to read the index"),me);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);+returnerror(_("%s: failed to refresh the index"),me);}rollback_lock_file(&index_lock);+return0;}staticintrevert_or_cherry_pick(intargc,constchar**argv){structrev_inforevs;+intres;git_config(git_default_config,NULL);me=action==REVERT?"revert":"cherry-pick";
@@ -578,17 +583,15 @@ static int revert_or_cherry_pick(int argc, const char **argv)die(_("cherry-pick --ff cannot be used with --edit"));}-read_and_refresh_cache(me);+if((res=read_and_refresh_cache(me))||+(res=prepare_revs(&revs)))+returnres;-prepare_revs(&revs);+while((commit=get_revision(&revs))&&+!(res=do_pick_commit(commit)))+;-while((commit=get_revision(&revs))){-intres=do_pick_commit();-if(res)-returnres;-}--return0;+returnres;}intcmd_revert(intargc,constchar**argv,constchar*prefix)
@@ -268,17 +278,17 @@ static struct tree *empty_tree(void)staticinterror_dirty_index(){if(read_cache_unmerged()){-die_resolve_conflict(action==REVERT?"revert":"cherry-pick");+die_resolve_conflict(cmd_opts.action==REVERT?"revert":"cherry-pick");}else{if(advice_commit_before_merge){-if(action==REVERT)+if(cmd_opts.action==REVERT)returnerror(_("Your local changes would be overwritten by revert.\n""Please, commit your changes or stash them to proceed."));elsereturnerror(_("Your local changes would be overwritten by cherry-pick.\n""Please, commit your changes or stash them to proceed."));}else{-if(action==REVERT)+if(cmd_opts.action==REVERT)returnerror(_("Your local changes would be overwritten by revert.\n"));elsereturnerror(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -331,7 +341,8 @@ 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" */-returnerror(_("%s: Unable to write new index file"),me);+returnerror(_("%s: Unable to write new index file"),+cmd_opts.action==REVERT?"revert":"cherry-pick");rollback_lock_file(&index_lock);if(!clean){
@@ -368,9 +379,9 @@ static int run_git_commit(const char *defmsg)args[i++]="commit";args[i++]="-n";-if(signoff)+if(cmd_opts.signoff)args[i++]="-s";-if(!edit){+if(!cmd_opts.edit){args[i++]="-F";args[i++]=defmsg;}
@@ -389,7 +400,7 @@ static int do_pick_commit(struct commit *commit)structstrbufmsgbuf=STRBUF_INIT;intres;-if(no_commit){+if(cmd_opts.no_commit){/**Wedonotintendtocommitimmediately.Wejustwantto*mergethedifferencesin,solet'scomputethetree
@@ -407,7 +418,7 @@ static int do_pick_commit(struct commit *commit)discard_cache();if(!commit->parents){-if(action==REVERT)+if(cmd_opts.action==REVERT)returnerror(_("Cannot revert a root commit"));parent=NULL;}
@@ -416,32 +427,32 @@ static int do_pick_commit(struct commit *commit)intcnt;structcommit_list*p;-if(!mainline)+if(!cmd_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!=cmd_opts.mainline&&p;cnt++)p=p->next;-if(cnt!=mainline||!p)+if(cnt!=cmd_opts.mainline||!p)returnerror(_("Commit %s does not have parent %d"),-sha1_to_hex(commit->object.sha1),mainline);+sha1_to_hex(commit->object.sha1),cmd_opts.mainline);parent=p->item;-}elseif(mainline>0)+}elseif(cmd_opts.mainline>0)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(cmd_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*/returnerror(_("%s: cannot parse parent commit %s"),-action==REVERT?"revert":"cherry-pick",+cmd_opts.action==REVERT?"revert":"cherry-pick",sha1_to_hex(parent->object.sha1));if(get_message(commit,commit->buffer,&msg)!=0)
@@ -457,7 +468,7 @@ static int do_pick_commit(struct commit *commit)defmsg=git_pathdup("MERGE_MSG");-if(action==REVERT){+if(cmd_opts.action==REVERT){base=commit;base_label=msg.label;next=parent;
@@ -478,16 +489,16 @@ static int do_pick_commit(struct commit *commit)next=commit;next_label=msg.label;add_message_to_msg(commit,&msgbuf,msg.message);-if(no_replay){+if(cmd_opts.no_replay){strbuf_addstr(&msgbuf,"(cherry picked from commit ");strbuf_addstr(&msgbuf,sha1_to_hex(commit->object.sha1));strbuf_addstr(&msgbuf,")\n");}-if(!no_commit)+if(!cmd_opts.no_commit)write_cherry_pick_head(commit);}-if(!strategy||!strcmp(strategy,"recursive")||action==REVERT){+if(!cmd_opts.strategy||!strcmp(cmd_opts.strategy,"recursive")||cmd_opts.action==REVERT){res=do_recursive_merge(base,next,base_label,next_label,head,&msgbuf);write_message(&msgbuf,defmsg);
@@ -499,22 +510,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,+res=try_merge_command(cmd_opts.strategy,cmd_opts.xopts_nr,+cmd_opts.xopts,common,sha1_to_hex(head),remotes);free_commit_list(common);free_commit_list(remotes);}if(res){-error(action==REVERT+error(cmd_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(cmd_opts.allow_rerere_auto);}else{-if(!no_commit)+if(!cmd_opts.no_commit)res=run_git_commit(defmsg);}
@@ -530,10 +542,10 @@ static int prepare_revs(struct rev_info *revs)init_revisions(revs,NULL);revs->no_walk=1;-if(action!=REVERT)+if(cmd_opts.action!=REVERT)revs->reverse=1;-argc=setup_revisions(commit_argc,commit_argv,revs,NULL);+argc=setup_revisions(cmd_opts.commit_argc,cmd_opts.commit_argv,revs,NULL);if(argc>1){fprintf(stderr,"usage: %s",_(*revert_or_cherry_pick_usage()));return129;
@@ -574,18 +586,18 @@ static int revert_or_cherry_pick(int argc, const char **argv)intres;git_config(git_default_config,NULL);-me=(action==REVERT?"revert":"cherry-pick");+me=(cmd_opts.action==REVERT?"revert":"cherry-pick");setenv(GIT_REFLOG_ACTION,me,0);parse_args(argc,argv);-if(allow_ff){-if(signoff)+if(cmd_opts.allow_ff){+if(cmd_opts.signoff)die(_("cherry-pick --ff cannot be used with --signoff"));-if(no_commit)+if(cmd_opts.no_commit)die(_("cherry-pick --ff cannot be used with --no-commit"));-if(no_replay)+if(cmd_opts.no_replay)die(_("cherry-pick --ff cannot be used with -x"));-if(edit)+if(cmd_opts.edit)die(_("cherry-pick --ff cannot be used with --edit"));}
@@ -578,18 +578,12 @@ static int read_and_refresh_cache(void)return0;}-staticintrevert_or_cherry_pick(intargc,constchar**argv)+staticintpick_commits(void){structrev_inforevs;structcommit*commit;-constchar*me;intres;-git_config(git_default_config,NULL);-me=(cmd_opts.action==REVERT?"revert":"cherry-pick");-setenv(GIT_REFLOG_ACTION,me,0);-parse_args(argc,argv);-if(cmd_opts.allow_ff){if(cmd_opts.signoff)die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -77,6 +77,24 @@ static int option_parse_x(const struct option *opt,return0;}+staticvoiddie_opt_incompatible(constchar*me,constchar*base_opt,+intnargs,intopt_bitarray[],...)+{+constchar*this_opt=NULL;+va_listap;+inti;++va_start(ap,opt_bitarray);+for(i=0;i<nargs;i++){+this_opt=va_arg(ap,constchar*);+if(opt_bitarray[i]){+die(_("%s: %s cannot be used with %s"),+me,this_opt,base_opt);+}+}+va_end(ap);+}+staticvoidparse_args(intargc,constchar**argv){constchar*const*usage_str=revert_or_cherry_pick_usage();
@@ -584,17 +609,6 @@ static int pick_commits(void)structcommit*commit;intres;-if(cmd_opts.allow_ff){-if(cmd_opts.signoff)-die(_("cherry-pick --ff cannot be used with --signoff"));-if(cmd_opts.no_commit)-die(_("cherry-pick --ff cannot be used with --no-commit"));-if(cmd_opts.no_replay)-die(_("cherry-pick --ff cannot be used with -x"));-if(cmd_opts.edit)-die(_("cherry-pick --ff cannot be used with --edit"));-}-if((res=read_and_refresh_cache())||(res=prepare_revs(&revs)))returnres;
@@ -98,8 +103,12 @@ static void die_opt_incompatible(const char *me, const char *base_opt,staticvoidparse_args(intargc,constchar**argv){constchar*const*usage_str=revert_or_cherry_pick_usage();+constchar*me;intnoop;structoptionoptions[]={+OPT_BOOLEAN(0,"abort",&cmd_opts.abort_oper,"abort the current operation"),+OPT_BOOLEAN(0,"skip",&cmd_opts.skip_oper,"skip the current commit"),+OPT_BOOLEAN(0,"continue",&cmd_opts.continue_oper,"continue the current operation"),OPT_BOOLEAN('n',"no-commit",&cmd_opts.no_commit,"don't automatically commit"),OPT_BOOLEAN('e',"edit",&cmd_opts.edit,"edit the commit message"),OPT_BOOLEAN('r',NULL,&noop,"no-op (backward compatibility)"),
@@ -127,7 +136,42 @@ static void parse_args(int argc, const char **argv)cmd_opts.commit_argc=parse_options(argc,argv,NULL,options,usage_str,PARSE_OPT_KEEP_ARGV0|PARSE_OPT_KEEP_UNKNOWN);-if(cmd_opts.commit_argc<2)++me=(cmd_opts.action==REVERT?"revert":"cherry-pick");++/* Check for incompatible command line arguments */+if(cmd_opts.abort_oper||cmd_opts.skip_oper||cmd_opts.continue_oper){+char*this_oper;+intopt_bitarray[]={cmd_opts.no_commit,cmd_opts.edit,noop,+cmd_opts.signoff,cmd_opts.mainline,+(cmd_opts.strategy?1:0),+(cmd_opts.xopts?1:0),+cmd_opts.no_replay,cmd_opts.allow_ff};+if(cmd_opts.abort_oper){+this_oper="--abort";+die_opt_incompatible(me,this_oper,1,+&cmd_opts.skip_oper,"--skip");+die_opt_incompatible(me,this_oper,1,+&cmd_opts.continue_oper,"--continue");+}elseif(cmd_opts.skip_oper){+this_oper="--skip";+die_opt_incompatible(me,this_oper,1,+&cmd_opts.abort_oper,"--abort");+die_opt_incompatible(me,this_oper,1,+&cmd_opts.continue_oper,"--continue");+}else{+this_oper="--continue";+die_opt_incompatible(me,this_oper,1,+&cmd_opts.abort_oper,"--abort");+die_opt_incompatible(me,this_oper,1,+&cmd_opts.skip_oper,"--skip");+}+die_opt_incompatible(me,this_oper,9,opt_bitarray,"--no-commit",+"--edit","-r","--signoff","--mainline",+"--strategy","--strategy-option","-x","--ff");+}++elseif(cmd_opts.commit_argc<2)usage_with_options(usage_str,options);if(cmd_opts.allow_ff){
@@ -47,3 +47,17 @@ void NORETURN die_resolve_conflict(const char *me)elsedie("'%s' is not possible because you have unmerged files.",me);}++interror_resolve_conflict(constchar*me)+{+if(advice_resolve_conflict)+/*+*Messageusedbothwhen'gitcommit'failsandwhen+*othercommandsdoingamergedo.+*/+returnerror("'%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+returnerror("'%s' is not possible because you have unmerged files.",me);+}
@@ -344,25 +344,20 @@ static struct tree *empty_tree(void)returntree;}-staticinterror_dirty_index()+staticintverify_resolution(constchar*me){-if(read_cache_unmerged()){-die_resolve_conflict(cmd_opts.action==REVERT?"revert":"cherry-pick");-}else{-if(advice_commit_before_merge){-if(cmd_opts.action==REVERT)-returnerror(_("Your local changes would be overwritten by revert.\n"-"Please, commit your changes or stash them to proceed."));-else-returnerror(_("Your local changes would be overwritten by cherry-pick.\n"-"Please, commit your changes or stash them to proceed."));-}else{-if(cmd_opts.action==REVERT)-returnerror(_("Your local changes would be overwritten by revert.\n"));-else-returnerror(_("Your local changes would be overwritten by cherry-pick.\n"));-}-}+if(!read_cache_unmerged())+return0;++returnerror_resolve_conflict(me);+}++staticinterror_dirty_worktree(constchar*me)+{+if(advice_commit_before_merge)+returnerror(_("Your local changes would be overwritten by %s.\n"+"Please, commit your changes or stash them to proceed."),me);+returnerror(_("Your local changes would be overwritten by %s.\n"),me);}staticintfast_forward_to(constunsignedchar*to,constunsignedchar*from)
@@ -467,8 +462,10 @@ static int do_pick_commit(struct commit *commit)structcommit_messagemsg={NULL,NULL,NULL,NULL,NULL};char*defmsg=NULL;structstrbufmsgbuf=STRBUF_INIT;+constchar*me;intres;+me=(cmd_opts.action==REVERT?"revert":"cherry-pick");if(cmd_opts.no_commit){/**Wedonotintendtocommitimmediately.Wejustwantto
@@ -481,8 +478,8 @@ static int do_pick_commit(struct commit *commit)}else{if(get_sha1("HEAD",head))returnerror(_("You do not have a valid HEAD"));-if(index_differs_from("HEAD",0))-returnerror_dirty_index();+if(index_differs_from("HEAD",0)&&!verify_resolution(me))+returnerror_dirty_worktree(me);}discard_cache();
@@ -521,8 +518,7 @@ static int do_pick_commit(struct commit *commit)/* TRANSLATORS: The first %s will be "revert" or"cherry-pick",thesecond%saSHA1*/returnerror(_("%s: cannot parse parent commit %s"),-cmd_opts.action==REVERT?"revert":"cherry-pick",-sha1_to_hex(parent->object.sha1));+me,sha1_to_hex(parent->object.sha1));if(get_message(commit,commit->buffer,&msg)!=0)returnerror(_("Cannot get commit message for %s"),
@@ -97,6 +102,35 @@ void NORETURN die_errno(const char *fmt, ...)va_end(params);}+interror_errno(constchar*fmt,...)+{+va_listparams;+charfmt_with_err[1024];+charstr_error[256],*err;+inti,j;++err=strerror(errno);+for(i=j=0;err[i]&&j<sizeof(str_error)-1;){+if((str_error[j++]=err[i++])!='%')+continue;+if(j<sizeof(str_error)-1){+str_error[j++]='%';+}else{+/* No room to double the '%', so we overwrite it with+*'\0'below*/+j--;+break;+}+}+str_error[j]=0;+snprintf(fmt_with_err,sizeof(fmt_with_err),"%s: %s",fmt,str_error);++va_start(params,fmt);+error_routine(fmt_with_err,params);+va_end(params);+return-1;+}+interror(constchar*err,...){va_listparams;
@@ -643,21 +651,95 @@ static int read_and_refresh_cache(void)return0;}+staticvoidformat_todo(structstrbuf*buf,structcommit_list*list)+{+structcommit_list*cur=NULL;+structcommit_messagemsg={NULL,NULL,NULL,NULL,NULL};+constchar*sha1=NULL;+constchar*action;++action=(cmd_opts.action==REVERT?"revert":"pick");+for(cur=list;cur;cur=cur->next){+sha1=find_unique_abbrev(cur->item->object.sha1,DEFAULT_ABBREV);+if(get_message(cur->item,cur->item->buffer,&msg))+die("Cannot get commit message for %s",sha1);++strbuf_addf(buf,"%s %s %s\n",action,sha1,msg.subject);+}+}++staticintpersist_initialize(unsignedchar*head)+{+structstrbufbuf=STRBUF_INIT;+intfd;++if(!file_exists(SEQ_PATH)&&mkdir(SEQ_PATH,0777))+returnerror_errno(_("Could not create sequencer directory '%s'"),SEQ_PATH);++if((fd=open(HEAD_FILE,O_WRONLY|O_CREAT|O_TRUNC,0666))<0)+returnerror_errno(_("Could not open '%s' for writing"),HEAD_FILE);++strbuf_addf(&buf,"%s",find_unique_abbrev(head,DEFAULT_ABBREV));+write_or_whine(fd,buf.buf,buf.len,HEAD_FILE);+close(fd);+strbuf_release(&buf);+return0;+}++staticintpersist_todo_done(intres,structcommit_list*todo_list,+structcommit_list*done_list)+{+structstrbufbuf=STRBUF_INIT;+intfd;++if(!res)+return0;++/* TODO file */+if((fd=open(TODO_FILE,O_WRONLY|O_CREAT|O_TRUNC,0666))<0)+returnerror_errno(_("Could not open '%s' for writing"),TODO_FILE);++format_todo(&buf,todo_list);+write_or_whine(fd,buf.buf,buf.len,TODO_FILE);+close(fd);++/* DONE file */+strbuf_reset(&buf);+if((fd=open(DONE_FILE,O_WRONLY|O_CREAT|O_TRUNC,0666))<0)+returnerror_errno(_("Could not open '%s' for writing"),DONE_FILE);++format_todo(&buf,done_list);+write_or_whine(fd,buf.buf,buf.len,DONE_FILE);+close(fd);+strbuf_release(&buf);+returnres;+}+staticintpick_commits(void){+structcommit_list*done_list=NULL;structrev_inforevs;structcommit*commit;+unsignedcharhead[20];intres;+if(get_sha1("HEAD",head))+returnerror(_("You do not have a valid HEAD"));+if((res=read_and_refresh_cache())||-(res=prepare_revs(&revs)))+(res=prepare_revs(&revs))||+(res=persist_initialize(head)))returnres;-while((commit=get_revision(&revs))&&-!(res=do_pick_commit(commit)))-;--returnres;+while((commit=get_revision(&revs))){+if(!(res=do_pick_commit(commit)))+commit_list_insert(commit,&done_list);+else{+commit_list_insert(commit,&revs.commits);+break;+}+}+returnpersist_todo_done(res,revs.commits,done_list);}intcmd_revert(intargc,constchar**argv,constchar*prefix)
@@ -112,7 +112,8 @@ static void parse_args(int argc, const char **argv){constchar*const*usage_str=revert_or_cherry_pick_usage();constchar*me;-intnoop;+intnoop=0;+structoptionoptions[]={OPT_BOOLEAN(0,"abort",&cmd_opts.abort_oper,"abort the current operation"),OPT_BOOLEAN(0,"skip",&cmd_opts.skip_oper,"skip the current commit"),
@@ -743,6 +743,51 @@ static int pick_commits(void)returnpersist_todo_done(res,revs.commits,done_list);}+staticintprocess_args(intargc,constchar**argv)+{+constchar*me;+intfd;++parse_args(argc,argv);+me=(cmd_opts.action==REVERT?"revert":"cherry-pick");+if(cmd_opts.abort_oper){+constchar*args[3]={NULL,NULL,NULL};+charhead[DEFAULT_ABBREV];++args[0]="rerere";+args[1]="clear";+cmd_rerere(2,args,NULL);++if(!file_exists(HEAD_FILE))+gotoerror;++if((fd=open(HEAD_FILE,O_RDONLY,0666))<0)+returnerror_errno(_("Could not open '%s' for reading"),HEAD_FILE);+if(xread(fd,head,DEFAULT_ABBREV)<DEFAULT_ABBREV)+returnerror_errno(_("Corrupt '%s'"),HEAD_FILE);+close(fd);++args[0]="reset";+args[1]="--hard";+args[2]=head;+returncmd_reset(3,args,NULL);+}+elseif(cmd_opts.skip_oper){+if(!file_exists(TODO_FILE))+gotoerror;+return0;+}+elseif(cmd_opts.continue_oper){+if(!file_exists(TODO_FILE))+gotoerror;+return0;+}++returnpick_commits();+error:+returnerror(_("No %s in progress"),me);+}+intcmd_revert(intargc,constchar**argv,constchar*prefix){if(isatty(0))
@@ -265,10 +265,10 @@ static struct tree *empty_tree(void)returntree;}-staticinterror_dirty_index(constchar*me)+staticinterror_dirty_index(){if(read_cache_unmerged()){-die_resolve_conflict(me);+die_resolve_conflict(action==REVERT?"revert":"cherry-pick");}else{if(advice_commit_before_merge){if(action==REVERT)
@@ -379,7 +379,7 @@ static int run_git_commit(const char *defmsg)returnrun_command_v_opt(args,RUN_GIT_CMD);}-staticintdo_pick_commit(void)+staticintdo_pick_commit(structcommit*commit){unsignedcharhead[20];structcommit*base,*next,*parent;
@@ -402,7 +402,7 @@ static int do_pick_commit(void)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();}discard_cache();
@@ -441,9 +441,10 @@ static int do_pick_commit(void)/* TRANSLATORS: The first %s will be "revert" or"cherry-pick",thesecond%saSHA1*/returnerror(_("%s: cannot parse parent commit %s"),-me,sha1_to_hex(parent->object.sha1));+action==REVERT?"revert":"cherry-pick",+sha1_to_hex(parent->object.sha1));-if(get_message(commit->buffer,&msg)!=0)+if(get_message(commit,commit->buffer,&msg)!=0)returnerror(_("Cannot get commit message for %s"),sha1_to_hex(commit->object.sha1));
@@ -476,14 +477,14 @@ static int do_pick_commit(void)base_label=msg.parent_label;next=commit;next_label=msg.label;-add_message_to_msg(&msgbuf,msg.message);+add_message_to_msg(commit,&msgbuf,msg.message);if(no_replay){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();+write_cherry_pick_head(commit);}if(!strategy||!strcmp(strategy,"recursive")||action==REVERT){
@@ -546,10 +547,13 @@ static int prepare_revs(struct rev_info *revs)return0;}-staticintread_and_refresh_cache(constchar*me)+staticintread_and_refresh_cache(void){staticstructlock_fileindex_lock;intindex_fd=hold_locked_index(&index_lock,0);+constchar*me;++me=(cmd_opts.action==REVERT?"revert":"cherry-pick");if(read_index_preload(&the_index,NULL)<0)returnerror(_("%s: failed to read the index"),me);refresh_index(&the_index,REFRESH_QUIET|REFRESH_UNMERGED,NULL,NULL,NULL);
@@ -565,10 +569,12 @@ static int read_and_refresh_cache(const char *me)staticintrevert_or_cherry_pick(intargc,constchar**argv){structrev_inforevs;+structcommit*commit;+constchar*me;intres;git_config(git_default_config,NULL);-me=action==REVERT?"revert":"cherry-pick";+me=(action==REVERT?"revert":"cherry-pick");setenv(GIT_REFLOG_ACTION,me,0);parse_args(argc,argv);
@@ -583,7 +589,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)die(_("cherry-pick --ff cannot be used with --edit"));}-if((res=read_and_refresh_cache(me))||+if((res=read_and_refresh_cache())||(res=prepare_revs(&revs)))returnres;
Presumably this is because the sequencer is going to pick up after the
error and clean up a little (why doesn't the change description say
so?). Will it be resuming after that or just performing a little
cleanup before the exit?
Might make sense, but:
quoted hunk
--- a/builtin/revert.c+++ b/builtin/revert.c
@@ -265,23 +265,23 @@ 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);
Won't that exit?
} else {
This "else" could be removed (decreasing the indent of the rest by
one tab stop) since the "if" case has already returned or exited.
Not the subject of this patch, just an idea for earlier or later. ;-)
[...]
quoted hunk
@@ -331,7 +331,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);+ return error(_("%s: Unable to write new index file"), me); rollback_lock_file(&index_lock);
What happens to index_lock in the error case?
[...]
Yuck. Maybe the error can be returned to the caller somehow, but
that seems somehow ambitious given that setup_revisions has all sorts
of ways to die anyway.
So you are bending the assumptions of many existing git functions (in
a good way).
I can think of at least three ways to go:
1) Come up with a convention to give more information about the nature
of returned errors in the functions you are touching. For
example, make sure errno is valid after the relevant functions, or
use multiple negative values to express the nature of the error.
So a caller could do:
if (prepare_revs(...)) {
if (errno == EINVAL)
usage(*revert_or_cherry_pick_usage());
die("BUG: unexpected error from prepare_revs");
}
Or:
2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
has to happen before exiting. Keep using die().
3) Provide a new facility to register cleanup handlers that will free
resources and otherwise return to a consistent state before
unwinding the stack. This way, you'd still have to audit die()
calls to look for missing cleanup handlers, but they could stay as
die() rather than changing to "return error" and the worried
caller could use
set_die_routine(longjmp_to_here);
to keep git alive. I don't suggest doing this. It is a pain to
get right and not obviously cleaner than "return error", and some
errors really are unrecoverable (rather than just being a symptom
of programmers to lazy to write error recovery code :)).
Okay, I'm stopping here. Hope that helps.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:01
Ramkumar Ramachandra wrote:
[Subject: revert: Introduce a struct to parse command-line options into]
Signed-off-by: Ramkumar Ramachandra <redacted>
This gives a hint of another way to avoid the usage() trouble
mentioned in patch 1: it might be possible to let the caller take care
of parsing arguments.
That would require giving this struct a name, so it can be passed
around. Not a bad idea anyway imho since then a person reading
top-to-bottom is not left in suspense:
struct cherry_pick_opts {
enum { REVERT, CHERRY_PICK } action;
unsigned edit:1;
unsigned no_replay:1;
...
};
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:51:01
On Sun, 10 Apr 2011, Ramkumar Ramachandra wrote:
Hi,
I've started working on building a sequencer for Git. While the
outline is described in [1], I'd like some additional clarifications.
A big thanks to Christian's series [2] for the valuable roadmap.
Please note that 10/11 is not related to this series, but seems to be
a minor nit that's required to make all existing tests pass.
That looks like an actual bug that only doesn't matter currently because
the function is never called with enough junk on the stack.
0. Is the general flow alright?
I suspect it would be easier to review some of this with certain things
squashed together; one patch that changes all of the variable references
to what you want them to be is easier to understand than one that moves
statics to function arguments, one that moves statics to struct fields,
etc. Likewise, when you're converting some of the die() calls to error(),
it's easier to review the patch if all of the die() calls that aren't
changed in that patch don't get changed later in the series.
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
I think it should be systematic but not necessarily uniform. You should be
able to give a guideline as to how to decide which to use (and you should
probably actually give the guideline, so future developers make consistent
choices). I think of "die" as being ideally for situations where the
program can't understand what has happened well enough to know what to do
about it.
2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly
inelegant, mainly because of the command-line argument parsing
overhead. Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate. Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
(ITYM cmd_reset here)
I think rerere.c should get a rerere_clear(). I think it would make sense
to implement the reset locally; the abort ought to be undoing exactly
those things that you did, and I'm not actually sure the ORIG_HEAD is
entirely appropriate. You ought to be able to use cleanup functions that
correspond to the functions you used to make the mess in the first place.
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I certainly like it. :)
Thanks for reading.
You're welcome. :)
-Daniel
*This .sig left intentionally blank*
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:01
Hi again,
Ramkumar Ramachandra wrote:
I've started working on building a sequencer for Git.
Happy to see.
0. Is the general flow alright?
Not sure --- I don't have the big picture. Could you give a quick
summary of the flow in the cover letter ("patch 1 does such-and-such,
so patch 2 can do such-and-such, leading to...") to the next revision,
and quick explanations of the purpose (i.e., why we should want each
change) in the individual change descriptions?
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
'die' gets used in two ways (well, one way really):
- to say "there is no sane way to recover from this failure". For
example, xmalloc dies if even after trying to free up memory,
malloc still could not satisfy the request.
- to say "so far we've been too lazy to implement recovery from
this failure". Or "while we *could* recover from this failure, no
one has needed it, so let's not --- that code would just bitrot."
Therefore a mixture of 'die' and 'return error' seems inevitable. The
dangerous mixtures to avoid are those likely to trip up callers (e.g.,
if all code paths 'return error' except one edge case).
2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly
inelegant, mainly because of the command-line argument parsing
overhead. Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate. Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
See "git log --grep=libify" for examples. Isn't rerere already
libified? Ah, you need "rerere clear" --- I think a rerere_clear
function alongside rerere_forget et al would make sense.
More generally, it should be feasible to expose a nice, simple API for
any functionality you need (with params struct if necessary, etc).
That's how many of the current APIs (revision walking, diffcore, ...)
came about.
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
This is still bouncing in my head. I think I like it --- is the idea
that some day you could put commands like
am topic.mbox
in your insn sheet, or do nested rebases with a --force-nested option?
That does sound useful. How would one request "skip to the next
operation in the outer rebase" on the command line? This is starting
to feel like a debugger.
4. I have a feeling that I've broken translation strings. Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?
If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
translations will be translated to gibberish when the GIT_GETTEXT_POISON
envvar is set, so you can use the test suite as a sanity check.
"make pot" can be used to get the translation template that
translators will see.
As for a readme explaining how to use _, N_, and Q_, yes, I think that
would be useful. I think Ævar's series has something like that (but
targetted at translators) later on; it might make sense to prod him or
me for a simpler explanation can be merged immediately. Until then,
there is "git log gettext.h".
Regards,
Jonathan
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:51:01
On Sun, 10 Apr 2011, Jonathan Nieder wrote:
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
This is still bouncing in my head. I think I like it --- is the idea
that some day you could put commands like
am topic.mbox
in your insn sheet, or do nested rebases with a --force-nested option?
That does sound useful. How would one request "skip to the next
operation in the outer rebase" on the command line? This is starting
to feel like a debugger.
The most basic idea is that, in the case of a rebase where a patch fails
to apply, there are actually two levels of operation that have to be
deferred until after the human intervention: making the commit where the
pick failed, and applying further patches. If there are going to be
multiple higher-level operations (e.g., rebase and rebase -i), they should
be able to share the completion of the pick operation, and a basic "git
cherry-pick <sha1>" ought to be able to share it, too, so you can finish
that up without having to remember what the command is. People should be
able to implement custom higher-level operations (e.g., "cherry-pick all
the commits attached to bugs in bugzilla that block the release tracker"
or something), they should be able to rely on library code that knows how
to finish the conflicted cherry-pick. (Another interesting example might
be "reconstruct the linux-next tree", which is a series of merges, and
which should have --continue share the post-intervention code from the
single merge process before going on to do the custom higher-level thing.)
My feeling is that "--skip" is actually "abort the pick, but continue the
rebase". I suppose there could be more than two levels, and people could
want to skip a higher-level chunk, but that's something to get to when
someone actually wants it; we've obviously already got the two-level
situation now, so we can implement that.
-Daniel
*This .sig left intentionally blank*
From: Christian Couder <hidden> Date: 2016-06-15 22:51:01
On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
Hi,
I've started working on building a sequencer for Git.
So you are starting the GSoC early! Great!
When (or before) it really starts, just make sure you put your work on a
public Git repository and you send status updates regularly (weekly if
possible).
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I think that the TODO and DONE file format will need at one point to include
options and it is simpler if this change is done early. Using a cherry-pick
specific file to keep the options is not very generic for a sequencer that could
be used for many things.
For example, as we have rebase --interactive, we will probably want to have
cherry-pick --interactive, and when editing the TODO file we might want to use
different cherry-pick options when picking different commits.
This would also make the different cherry-pick options available when using
rebase --interactive once it uses the sequencer.
On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
quoted
Hi,
I've started working on building a sequencer for Git.
So you are starting the GSoC early! Great!
When (or before) it really starts, just make sure you put your work on a
public Git repository and you send status updates regularly (weekly if
possible).
Ofcourse. I've already discussed many of these issues last year [1].
The work corresponding to this particular series can be found in the
'sequencer' branch on my GitHub fork [2]. Since the results haven't
been announced, and the coding period hasn't begun, this work should
be treated like "normal work" -- I just wrote it this weekend.
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I think that the TODO and DONE file format will need at one point to include
options and it is simpler if this change is done early. Using a cherry-pick
specific file to keep the options is not very generic for a sequencer that could
be used for many things.
For example, as we have rebase --interactive, we will probably want to have
cherry-pick --interactive, and when editing the TODO file we might want to use
different cherry-pick options when picking different commits.
Point noted -- I shouldn't narrow down the various things I can do
with a single commit early on and lock us into a more restrictive
design. However, I'm not in favor of making it too generic; I
certainly wouldn't like to edit an instruction sheet that looks like
this:
cherry-pick -m 1 -s -r 83a4fe9
revert -n 3a6fe42
cherry-pick -x --ff dacfe41
cherry-pick -s recursive -Xpatience b31d4e2
It'll become impossible to tell which options are disallowed over what
else, and it'll become a nightmare to debug when something goes wrong.
My idea is that we add commit-specific options in an optional
backward-compatible manner later:
pick 83a4fe9
revert 3a6fe42 # -n
pick dacfe41 # -s
pick b31d4e2
That way, there'll be two sets of options:
1. One "global" set of command-line switches that applies
to all the commits, which will be written to a command-specific
location. The sequencer itself knows nothing about this.
2. Optional commit-specific stuff that's passed in the
form of a (modified) commit_list to the sequencer API to write to the
todo/ done files.
Do you like this idea?
This would also make the different cherry-pick options available when using
rebase --interactive once it uses the sequencer.
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:51:01
On Mon, 11 Apr 2011, Christian Couder wrote:
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I think that the TODO and DONE file format will need at one point to include
options and it is simpler if this change is done early. Using a cherry-pick
specific file to keep the options is not very generic for a sequencer that could
be used for many things.
My idea is that cherry-pick (or merge, or whatever) would have its own
state to store what it was trying to do when it got a conflict. This is to
enable the cherry-pick (etc) to complete without needing to know anything
about what called it. The sequencer's information would keep track of
what was left to do, what was already complete, and how to abort the whole
sequence.
It would be nice to get the following commands to work:
$ git cherry-pick <sha1>
Automatic cherry-pick failed. After resolving the conflicts,
mark the corrected paths with 'git add <paths>' or 'git rm <paths>' and
run 'git continue'
$ (resolve conflicts)
$ git continue
Finished one cherry-pick.
[master sha1b] Subject
2 files changes, 2 insertions(+)
And, of course, if this happened as a step in a rebase, the rebase would
continue after the cherry-pick finished. But it's annoying that, in order
to finish a conflicted "git cherry-pick <branch>", you currently need to
go back and find the instruction that says to commit it yourself, with the
option "-c <sha1>" to retain authorship and message. And if you want to
abort it, you need to remember "git reset --hard HEAD" (and maybe you also
want "git rerere clear"). That's what cherry-pick should be keeping its
own state for. This is also parallel to how "merge" works, with its
information kept in private state files, including some things like the
list of files that had conflicts which isn't trivial to reproduce after
the user has resolved them.
-Daniel
*This .sig left intentionally blank*
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:01
Hi,
Daniel Barkalow wrote:
But it's annoying that, in order
to finish a conflicted "git cherry-pick <branch>", you currently need to
go back and find the instruction that says to commit it yourself, with the
option "-c <sha1>" to retain authorship and message.
You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,
2011-02-19).
And if you want to
abort it, you need to remember "git reset --hard HEAD" (and maybe you also
want "git rerere clear").
Hm, I had assumed reset --hard (or "git reset --merge HEAD") would
take care of the "rerere clear" but it seems that indeed it doesn't.
From: Christian Couder <hidden> Date: 2016-06-15 22:51:01
Hi Ram,
On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
Hi Christian,
Christian Couder writes:
quoted
On Sunday 10 April 2011 17:11:46 Ramkumar Ramachandra wrote:
quoted
Hi,
I've started working on building a sequencer for Git.
So you are starting the GSoC early! Great!
When (or before) it really starts, just make sure you put your work on a
public Git repository and you send status updates regularly (weekly if
possible).
Ofcourse. I've already discussed many of these issues last year [1].
The work corresponding to this particular series can be found in the
'sequencer' branch on my GitHub fork [2].
Great! Thanks!
Since the results haven't
been announced, and the coding period hasn't begun, this work should
be treated like "normal work" -- I just wrote it this weekend.
Ok.
quoted
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I think that the TODO and DONE file format will need at one point to
include options and it is simpler if this change is done early. Using a
cherry-pick specific file to keep the options is not very generic for a
sequencer that could be used for many things.
For example, as we have rebase --interactive, we will probably want to
have cherry-pick --interactive, and when editing the TODO file we might
want to use different cherry-pick options when picking different
commits.
Point noted -- I shouldn't narrow down the various things I can do
with a single commit early on and lock us into a more restrictive
design. However, I'm not in favor of making it too generic; I
certainly wouldn't like to edit an instruction sheet that looks like
this:
cherry-pick -m 1 -s -r 83a4fe9
revert -n 3a6fe42
cherry-pick -x --ff dacfe41
cherry-pick -s recursive -Xpatience b31d4e2
I wouldn't like either, but I would like it even less if it was like this:
pick 83a4fe9 # -m 1 -s -r
revert 3a6fe42 # -n
pick dacfe41 # -x --ff
pick b31d4e2 # -s recursive -Xpatience
I mean that of course people should not use too many options for no good
reason, but if they do need to use some options, it's better if they are shown
like in a shell, as they will be more familiar with them this way.
There is no point of making options look different to prevent people from
abusing them.
It'll become impossible to tell which options are disallowed over what
else, and it'll become a nightmare to debug when something goes wrong.
My idea is that we add commit-specific options in an optional
backward-compatible manner later:
pick 83a4fe9
revert 3a6fe42 # -n
pick dacfe41 # -s
pick b31d4e2
That way, there'll be two sets of options:
1. One "global" set of command-line switches that applies
to all the commits, which will be written to a command-specific
location. The sequencer itself knows nothing about this.
I don't see the point of this global set. And if the sequencer knows nothing
about it, the user may not know about it too and so may not understand how
things work.
2. Optional commit-specific stuff that's passed in the
form of a (modified) commit_list to the sequencer API to write to the
todo/ done files.
Do you like this idea?
quoted
This would also make the different cherry-pick options available when
using rebase --interactive once it uses the sequencer.
From: Daniel Barkalow <hidden> Date: 2016-06-15 22:51:01
On Mon, 11 Apr 2011, Jonathan Nieder wrote:
Hi,
Daniel Barkalow wrote:
quoted
But it's annoying that, in order
to finish a conflicted "git cherry-pick <branch>", you currently need to
go back and find the instruction that says to commit it yourself, with the
option "-c <sha1>" to retain authorship and message.
You might like v1.7.5-rc0~88^2 (Teach commit about CHERRY_PICK_HEAD,
2011-02-19).
Ah, so I'm actually proposing that we not revert that series, as far as
having cherry-pick-specific state is concerned. :) On the other hand, I'd
like to have the logic for using that state be in the cherry-pick
implementation, rather than having commit.c need to understand merge and
cherry-pick (and revert, and applying a patch from email, and...). Which
is to say, there should be a core state file that ends up with
"cherry-pick" in it, and revert.c has registered that it handles that, and
commit.c should call the registered function to use the saved state.
Also, git-rebase should be able to take advantage of the fact that other
code knows how to start a cherry-pick, find that it has a conflict, tell
the user, and use saved information to finish it.
quoted
And if you want to
abort it, you need to remember "git reset --hard HEAD" (and maybe you also
want "git rerere clear").
Hm, I had assumed reset --hard (or "git reset --merge HEAD") would
take care of the "rerere clear" but it seems that indeed it doesn't.
In fact, Ram's series makes sure to call both, which is how I knew.
-Daniel
*This .sig left intentionally blank*
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:51:01
Daniel Barkalow wrote:
My feeling is that "--skip" is actually "abort the pick, but continue the
rebase". I suppose there could be more than two levels, and people could
want to skip a higher-level chunk, but that's something to get to when
someone actually wants it
Ah, I think I see better now. What was confusing was
git cherry-pick foo..bar
for which --skip would mean "abort the pick, but continue the pick".
It fits very well into your story. There are two levels: "atomic"
(for lack of a better word) operations, like a single cherry-pick, and
sequences of operations.
(As for the more-than-two-levels story, I want it, but I agree with
you that that is tangential to this project. I find myself attempting
to go back and rearrange earlier commits during an interactive rebase
very often.)
Please note that 10/11 is not related to this series, but seems to be
a minor nit that's required to make all existing tests pass.
That looks like an actual bug that only doesn't matter currently because
the function is never called with enough junk on the stack.
Fixing this bug was one is one of the hidden motives of the patch I
send out just afterward [1].
quoted
0. Is the general flow alright?
I suspect it would be easier to review some of this with certain things
squashed together; one patch that changes all of the variable references
to what you want them to be is easier to understand than one that moves
statics to function arguments, one that moves statics to struct fields,
etc. Likewise, when you're converting some of the die() calls to error(),
it's easier to review the patch if all of the die() calls that aren't
changed in that patch don't get changed later in the series.
Agreed -- I'll post a reworked series shortly. Jonathan was finding
it hard to follow as well.
quoted
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
I think it should be systematic but not necessarily uniform. You should be
able to give a guideline as to how to decide which to use (and you should
probably actually give the guideline, so future developers make consistent
choices). I think of "die" as being ideally for situations where the
program can't understand what has happened well enough to know what to do
about it.
Jonathan concurs. Where should I document this?
quoted
2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly
inelegant, mainly because of the command-line argument parsing
overhead. Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate. Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
(ITYM cmd_reset here)
Yeah, sorry.
I think rerere.c should get a rerere_clear(). I think it would make sense
to implement the reset locally; the abort ought to be undoing exactly
those things that you did, and I'm not actually sure the ORIG_HEAD is
entirely appropriate. You ought to be able to use cleanup functions that
correspond to the functions you used to make the mess in the first place.
Not sure --- I don't have the big picture. Could you give a quick
summary of the flow in the cover letter ("patch 1 does such-and-such,
so patch 2 can do such-and-such, leading to...") to the next revision,
and quick explanations of the purpose (i.e., why we should want each
change) in the individual change descriptions?
Agreed. I'll ask again after the next re-roll.
quoted
1. Is it okay to use this kind of adaptive error handling (calling
'die' in some places and returning error in other places), or should
it be more uniform?
'die' gets used in two ways (well, one way really):
- to say "there is no sane way to recover from this failure". For
example, xmalloc dies if even after trying to free up memory,
malloc still could not satisfy the request.
- to say "so far we've been too lazy to implement recovery from
this failure". Or "while we *could* recover from this failure, no
one has needed it, so let's not --- that code would just bitrot."
Therefore a mixture of 'die' and 'return error' seems inevitable. The
dangerous mixtures to avoid are those likely to trip up callers (e.g.,
if all code paths 'return error' except one edge case).
My thoughts precisely. I was worried about what would happen in
future when Git is completely lib'ified, but I suppose it makes little
sense to think about that now.
quoted
2. In 11/11, I've used cmd_revert and cmd_rerere. This is highly
inelegant, mainly because of the command-line argument parsing
overhead. Re-implementing it using more low-level functions doesn't
seem to be the way to go either: for example, 'reset --hard' has some
additional logic of writing HEAD and ORIG_HEAD, which I don't want to
duplicate. Should I work on reworking parts of 'rerere.c' and
'revert.c', or is there some other way?
See "git log --grep=libify" for examples. Isn't rerere already
libified? Ah, you need "rerere clear" --- I think a rerere_clear
function alongside rerere_forget et al would make sense.
Done in [1].
More generally, it should be feasible to expose a nice, simple API for
any functionality you need (with params struct if necessary, etc).
That's how many of the current APIs (revision walking, diffcore, ...)
came about.
I see. Okay, I'll see if it makes sense to craft an API for rebase.
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
This is still bouncing in my head. I think I like it --- is the idea
that some day you could put commands like
am topic.mbox
in your insn sheet, or do nested rebases with a --force-nested option?
That does sound useful. How would one request "skip to the next
operation in the outer rebase" on the command line? This is starting
to feel like a debugger.
I'll respond to this later in the thread, since Daniel has already
said something. I just need help with crafting a nice instruction
sheet now -- please join the discussion at [2].
quoted
4. I have a feeling that I've broken translation strings. Is there a
README, plus a bunch of tests I can run to make sure that I've not
broken anything?
If you put "GETTEXT_POISON = YesPlease" in your config.mak, the
translations will be translated to gibberish when the GIT_GETTEXT_POISON
envvar is set, so you can use the test suite as a sanity check.
"make pot" can be used to get the translation template that
translators will see.
As for a readme explaining how to use _, N_, and Q_, yes, I think that
would be useful. I think Ævar's series has something like that (but
targetted at translators) later on; it might make sense to prod him or
me for a simpler explanation can be merged immediately. Until then,
there is "git log gettext.h".
On Monday 11 April 2011 06:49:05 Ramkumar Ramachandra wrote:
quoted
quoted
quoted
3. From the format of the TODO and DONE files, one more thing should
be clear- I'm trying to stick to a slight variation of the 'rebase -i'
format. This part will go into the sequencer. Then I'll use a
cherry-pick specific file to keep the command-line options. Yes, I'm
trying to work on Daniel's idea [3] from the very start. Is this a
good idea?
I think that the TODO and DONE file format will need at one point to
include options and it is simpler if this change is done early. Using a
cherry-pick specific file to keep the options is not very generic for a
sequencer that could be used for many things.
For example, as we have rebase --interactive, we will probably want to
have cherry-pick --interactive, and when editing the TODO file we might
want to use different cherry-pick options when picking different
commits.
Point noted -- I shouldn't narrow down the various things I can do
with a single commit early on and lock us into a more restrictive
design. However, I'm not in favor of making it too generic; I
certainly wouldn't like to edit an instruction sheet that looks like
this:
cherry-pick -m 1 -s -r 83a4fe9
revert -n 3a6fe42
cherry-pick -x --ff dacfe41
cherry-pick -s recursive -Xpatience b31d4e2
I wouldn't like either, but I would like it even less if it was like this:
pick 83a4fe9 # -m 1 -s -r
revert 3a6fe42 # -n
pick dacfe41 # -x --ff
pick b31d4e2 # -s recursive -Xpatience
I mean that of course people should not use too many options for no good
reason, but if they do need to use some options, it's better if they are shown
like in a shell, as they will be more familiar with them this way.
There is no point of making options look different to prevent people from
abusing them.
You're right -- no point obscuring things. I can't help thinking that
there must be a more elegant representation. I'll think about it for
a while, and try to come up with something.
quoted
It'll become impossible to tell which options are disallowed over what
else, and it'll become a nightmare to debug when something goes wrong.
My idea is that we add commit-specific options in an optional
backward-compatible manner later:
pick 83a4fe9
revert 3a6fe42 # -n
pick dacfe41 # -s
pick b31d4e2
That way, there'll be two sets of options:
1. One "global" set of command-line switches that applies
to all the commits, which will be written to a command-specific
location. The sequencer itself knows nothing about this.
I don't see the point of this global set. And if the sequencer knows nothing
about it, the user may not know about it too and so may not understand how
things work.
Hm. I originally wanted this so that each commit in the instruction
sheet isn't polluted with the same command-line options, but this
doesn't seem to be a good solution.
-- Ram
From: Junio C Hamano <hidden> Date: 2016-06-15 22:51:01
Ramkumar Ramachandra [off-list ref] writes:
Signed-off-by: Ramkumar Ramachandra <redacted>
Again, "In later steps, a new API that takes a single commit and replays
it in forward (cherry-pick) or backward (revert) direction will be
introduced, and will take this structure as a parameter to tell it what to
do" is missing from the above description.
More importantly, the primary purpose of these variables is _not_ "to
parse command line options into". It is to actively affect what happens
in the code, and "parse command line" is merely a way to assign the
initial values to them. So I'd rather see this patch described perhaps
like this:
cherry-pick/revert: introduce "struct replay_options"
The current code uses a set of file-scope static variables to instruct
the cherry-pick/revert machinery how to replay the changes, and
initialises 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.
Introduce a structure to group these variables, so that the API can
take them as a single "replay_options" parameter.
I strongly prefer to see this patch also update the callchain to pass a
pointer to the options struct as parameter. I can guess without reading
the rest the series that at some later step you would do that, but I think
it makes more sense to do the conversion at this step, as you will be
touching lines that use the global variables in this patch anyway, like
this:
- return action == REVERT ? revert_usage : cherry_pick_usage;
+ return cmd_opts.action == REVERT ? revert_usage : cherry_pick_usage;
which should eventually become something like:
return opts->usage_message;
at the very last step when this codepath is _fully_ libified (I don't know
if the final result of your series still is a "choose only from these two
canned messages" API, or gives the ability to the caller to specify the
usage message---I am just showing how it should look like at the end of a
full libification). So it would make sense to see:
return opts->action == REVERT ? revert_usage : cherry_pick_usage;
in this patch.
quoted hunk
@@ -268,17 +278,17 @@ static struct tree *empty_tree(void) static int error_dirty_index()
It is probably a remnant of the earlier patches in this series, but this
should start with:
static int error_dirty_index(void)
Of course, you will actually be passing the options structure, so it would
become:
static int error_dirty_index(struct replay_options *opts)
{
...
if (opts->action == REVERT)
...
}
From: Junio C Hamano <hidden> Date: 2016-06-15 22:51:01
Ramkumar Ramachandra [off-list ref] writes:
quoted hunk
+static void die_opt_incompatible(const char *me, const char *base_opt,+ int nargs, int opt_bitarray[], ...)+{
This feels way overkill to use vararg. This may be _one_ way to do what
you want to do, and while I don't think it is the best way, it may be Ok.
"opt_bitarray[]" is a horrible name, though. It does not convey what
the variable is about (its "purpose"). It doesn't even correctly describe
the way that unspecified purpose happens to be implemented (claims to be
bitarray but it is just an array of ints and what matters is if any bit is
set in the array).
Why not do it like this instead?
struct incompatible {
unsigned option_bit;
const char *option_name;
} incompatible[] = {
{ opts->signoff, "--signoff" },
{ opts->no_commit, "--no-commit" },
...
};
verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));
Or if you are shooting for ease-of-use, it might make sense to do it like
this:
verify_compatible("me", "--ff",
"--signoff", opts->signoff,
"--no-commit", opts->no_commit,
...
NULL);
and make verify_compatible() a varargs function that takes two optional
arguments at a time, i.e. const char *, followed by an int. Then there is
no need for extra "int opt_bitarray[]" or "struct incompatible".
That would justify use of varargs, I think.
Why not do it like this instead?
struct incompatible {
unsigned option_bit;
const char *option_name;
} incompatible[] = {
{ opts->signoff, "--signoff" },
{ opts->no_commit, "--no-commit" },
...
};
verify_compatible("me", "--ff", incompatible, ARRAY_SIZE(incompatible));
Or if you are shooting for ease-of-use, it might make sense to do it like
this:
verify_compatible("me", "--ff",
"--signoff", opts->signoff,
"--no-commit", opts->no_commit,
...
NULL);
and make verify_compatible() a varargs function that takes two optional
arguments at a time, i.e. const char *, followed by an int. Then there is
no need for extra "int opt_bitarray[]" or "struct incompatible".
That would justify use of varargs, I think.
Now that you point it out, my original approach was unnecessarily
cryptic and convoluted. I've followed this approach in my new series,
and kept varargs -- the code looks much prettier now. Thanks :)
I can't justify changing the name from "die_opt_incompatible" to
"verify_compatible" though; the name I've chosen seems to be more
appropriate/ descriptive. Further, I think command-line parsing
should always be the toplevel caller, and "die" is therefore more
appropriate than "return error" here.
-- Ram
Presumably this is because the sequencer is going to pick up after the
error and clean up a little (why doesn't the change description say
so?). Will it be resuming after that or just performing a little
cleanup before the exit?
I didn't write commit messages for any of the patches in the previous
round -- I just wanted to show the idea quickly. Anyway, it's fixed
in the next round (coming soon).
quoted
--- a/builtin/revert.c+++ b/builtin/revert.c
@@ -265,23 +265,23 @@ 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);
Won't that exit?
Fixed.
quoted
} else {
This "else" could be removed (decreasing the indent of the rest by
one tab stop) since the "if" case has already returned or exited.
Not the subject of this patch, just an idea for earlier or later. ;-)
Fixed.
[...]
quoted
@@ -331,7 +331,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);+ return error(_("%s: Unable to write new index file"), me); rollback_lock_file(&index_lock);
Yuck. Maybe the error can be returned to the caller somehow, but
that seems somehow ambitious given that setup_revisions has all sorts
of ways to die anyway.
So you are bending the assumptions of many existing git functions (in
a good way).
I can think of at least three ways to go:
1) Come up with a convention to give more information about the nature
of returned errors in the functions you are touching. For
example, make sure errno is valid after the relevant functions, or
use multiple negative values to express the nature of the error.
So a caller could do:
if (prepare_revs(...)) {
if (errno == EINVAL)
usage(*revert_or_cherry_pick_usage());
die("BUG: unexpected error from prepare_revs");
}
Or:
2) Use set_die_routine or sigchain_push + atexit to declare what cleanup
has to happen before exiting. Keep using die().
3) Provide a new facility to register cleanup handlers that will free
resources and otherwise return to a consistent state before
unwinding the stack. This way, you'd still have to audit die()
calls to look for missing cleanup handlers, but they could stay as
die() rather than changing to "return error" and the worried
caller could use
set_die_routine(longjmp_to_here);
to keep git alive. I don't suggest doing this. It is a pain to
get right and not obviously cleaner than "return error", and some
errors really are unrecoverable (rather than just being a symptom
of programmers to lazy to write error recovery code :)).
Hm, I'm a little confused about error handling now. I'll defer this
part until my patches naturally establish some convention for error
handling -- designing one in advance isn't as easy as I thought.
Finally, thanks for the review! I'll look forward to more reviews as
I post more iterations of the series.
-- Ram
Again, "In later steps, a new API that takes a single commit and replays
it in forward (cherry-pick) or backward (revert) direction will be
introduced, and will take this structure as a parameter to tell it what to
do" is missing from the above description.
More importantly, the primary purpose of these variables is _not_ "to
parse command line options into". It is to actively affect what happens
in the code, and "parse command line" is merely a way to assign the
initial values to them. So I'd rather see this patch described perhaps
like this:
cherry-pick/revert: introduce "struct replay_options"
The current code uses a set of file-scope static variables to instruct
the cherry-pick/revert machinery how to replay the changes, and
initialises 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.
Introduce a structure to group these variables, so that the API can
take them as a single "replay_options" parameter.
Right. Thanks for the nice commit message :)
I strongly prefer to see this patch also update the callchain to pass a
pointer to the options struct as parameter. I can guess without reading
the rest the series that at some later step you would do that, but I think
it makes more sense to do the conversion at this step, as you will be
touching lines that use the global variables in this patch anyway, like
this:
Good idea. I've passed the opts pointer around in the new series.
quoted
@@ -268,17 +278,17 @@ static struct tree *empty_tree(void) static int error_dirty_index()
It is probably a remnant of the earlier patches in this series, but this
should start with:
static int error_dirty_index(void)
Of course, you will actually be passing the options structure, so it would
become:
static int error_dirty_index(struct replay_options *opts)
{
...
if (opts->action == REVERT)
...
}
The very first patch removes this function (and puts the functionality
elsewhere) in the new series, so this comment doesn't apply.
Thanks for the review!
-- Ram
That would require giving this struct a name, so it can be passed
around. Not a bad idea anyway imho since then a person reading
top-to-bottom is not left in suspense:
struct cherry_pick_opts {
enum { REVERT, CHERRY_PICK } action;
unsigned edit:1;
unsigned no_replay:1;
...
};
Thanks. A couple of nits:
1. You can't take the address of a bitfield, so 'edit' and 'replay'
can't be passed to the command line argument parsing framework
directly.
2. GCC throws a "warning: useless storage class specifier in empty
declaration" for this kind of declaration.
-- Ram