From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:36
This patch series marks the '4' in the countdown to speed up rebase -i
by implementing large parts in C. It is based on the `libify-sequencer`
patch series that I submitted last week.
The patches in this series merely prepare the sequencer code for the
next patch series that actually teaches the sequencer to run an
interactive rebase.
The reason to split these two patch series is simple: to keep them at a
sensible size.
The two patch series after that are much smaller: a two-patch "series"
that switches rebase -i to use the sequencer (except with --root or
--preserve-merges), and a couple of patches to move several pretty
expensive script processing steps to C (think: autosquash).
The end game of this patch series is a git-rebase--helper that makes
rebase -i 5x faster on Windows (according to t/perf/p3404). Travis says
that even MacOSX and Linux benefit (4x and 3x, respectively).
I have been working on this since early February, whenever time allowed,
and it is time to put it into the users' hands. To that end, I will most
likely submit the remaining three patch series in the next two days, and
integrate the whole shebang into Git for Windows 2.10.0.
Therefore I would be most grateful for every in-depth review.
Johannes Schindelin (22):
sequencer: use static initializers for replay_opts
sequencer: use memoized sequencer directory path
sequencer: avoid unnecessary indirection
sequencer: future-proof remove_sequencer_state()
sequencer: allow the sequencer to take custody of malloc()ed data
sequencer: release memory that was allocated when reading options
sequencer: future-proof read_populate_todo()
sequencer: remove overzealous assumption
sequencer: completely revamp the "todo" script parsing
sequencer: avoid completely different messages for different actions
sequencer: get rid of the subcommand field
sequencer: refactor the code to obtain a short commit name
sequencer: remember the onelines when parsing the todo file
sequencer: prepare for rebase -i's commit functionality
sequencer: introduce a helper to read files written by scripts
sequencer: prepare for rebase -i's GPG settings
sequencer: allow editing the commit message on a case-by-case basis
sequencer: support amending commits
sequencer: support cleaning up commit messages
sequencer: remember do_recursive_merge()'s return value
sequencer: left-trim the lines read from the script
sequencer: refactor write_message()
builtin/commit.c | 2 +-
builtin/revert.c | 42 ++-
sequencer.c | 573 +++++++++++++++++++++++++++-------------
sequencer.h | 27 +-
t/t3510-cherry-pick-sequence.sh | 11 -
5 files changed, 428 insertions(+), 227 deletions(-)
Based-On: libify-sequencer at https://github.com/dscho/git
Fetch-Base-Via: git fetch https://github.com/dscho/git libify-sequencer
Published-As: https://github.com/dscho/git/releases/tag/prepare-sequencer-v1
Fetch-It-Via: git fetch https://github.com/dscho/git prepare-sequencer-v1
--
2.10.0.rc1.114.g2bd6b38
base-commit: 2d6d71e2a2d410b12d783f0a8edd22791f303c12
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:39
We really do not need the *pointer to a* pointer to the options in
the read_populate_opts() function.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:43
In a couple of commits, we will teach the sequencer to handle the
nitty gritty of the interactive rebase, which keeps its state in a
different directory.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:48
The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.
This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was
done to allow using the functionality as a library function, though,
including proper clean-up after use.
This patch introduces an API to pass the responsibility of releasing
certain memory to the sequencer.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 13 +++++++++++++
sequencer.h | 8 +++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:51
The sequencer reads options from disk and stores them in its struct
for use during sequencer's operations.
With this patch, the memory is released afterwards, plugging a
memory leak.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:54
Over the next commits, we will work on improving the sequencer to the
point where it can process the edit script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
@@ -772,25 +777,24 @@ static int parse_insn_buffer(char *buf, struct commit_list **todo_list,staticintread_populate_todo(structcommit_list**todo_list,structreplay_opts*opts){+constchar*todo_file=get_todo_path(opts);structstrbufbuf=STRBUF_INIT;intfd,res;-fd=open(git_path_todo_file(),O_RDONLY);+fd=open(todo_file,O_RDONLY);if(fd<0)-returnerror_errno(_("Could not open %s"),-git_path_todo_file());+returnerror_errno(_("Could not open %s"),todo_file);if(strbuf_read(&buf,fd,0)<0){close(fd);strbuf_release(&buf);-returnerror(_("Could not read %s."),git_path_todo_file());+returnerror(_("Could not read %s."),todo_file);}close(fd);res=parse_insn_buffer(buf.buf,todo_list,opts);strbuf_release(&buf);if(res)-returnerror(_("Unusable instruction sheet: %s"),-git_path_todo_file());+returnerror(_("Unusable instruction sheet: %s"),todo_file);return0;}
@@ -1064,7 +1068,7 @@ static int sequencer_continue(struct replay_opts *opts){structcommit_list*todo_list=NULL;-if(!file_exists(git_path_todo_file()))+if(!file_exists(get_todo_path(opts)))returncontinue_single_pick();if(read_populate_opts(opts)||read_populate_todo(&todo_list,opts))
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:04:58
The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.
The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.
Therefore let's just get rid of the test that wants to verify that this
limitation is still in place, in preparation for the upcoming work to
teach the sequencer to do rebase -i's work.
Signed-off-by: Johannes Schindelin <redacted>
---
t/t3510-cherry-pick-sequence.sh | 11 -----------
1 file changed, 11 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:05:51
When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.
However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.
Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).
Finally, the sequencer code chose to deviate from the interactive rebase
code insofar that it *reformats* the "todo" script instead of just
writing the part of the parsed script that were not yet processed. This
is not only unnecessary churn, but might well lose information that is
valuable to the user (i.e. comments after the commands).
Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.
While at it, do not stop at the first problem, but list *all* of the
problems. This helps the user by allowing to address all issues in
one go rather than going back and forth until the todo list is valid.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 241 +++++++++++++++++++++++++++++++++---------------------------
1 file changed, 134 insertions(+), 107 deletions(-)
@@ -535,7 +554,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)/* TRANSLATORS: The first %s will be "revert" or"cherry-pick",thesecond%saSHA1*/returnerror(_("%s: cannot parse parent commit %s"),-action_name(opts),oid_to_hex(&parent->object.oid));+command_to_string(command),+oid_to_hex(&parent->object.oid));if(get_message(commit,&msg)!=0)returnerror(_("Cannot get commit message for %s"),
@@ -615,17 +635,17 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)*However,ifthemergedidnotevenstart,thenwedon'twantto*writeitatall.*/-if(opts->action==REPLAY_PICK&&!opts->no_commit&&(res==0||res==1)&&+if(command==TODO_PICK&&!opts->no_commit&&(res==0||res==1)&&update_ref(NULL,"CHERRY_PICK_HEAD",commit->object.oid.hash,NULL,REF_NODEREF,UPDATE_REFS_MSG_ON_ERR))res=-1;-if(opts->action==REPLAY_REVERT&&((opts->no_commit&&res==0)||res==1)&&+if(command==TODO_REVERT&&((opts->no_commit&&res==0)||res==1)&&update_ref(NULL,"REVERT_HEAD",commit->object.oid.hash,NULL,REF_NODEREF,UPDATE_REFS_MSG_ON_ERR))res=-1;if(res){-error(opts->action==REPLAY_REVERT+error(command==TODO_REVERT?_("could not revert %s... %s"):_("could not apply %s... %s"),find_unique_abbrev(commit->object.oid.hash,DEFAULT_ABBREV),
@@ -683,116 +703,107 @@ static int read_and_refresh_cache(struct replay_opts *opts)return0;}-staticintformat_todo(structstrbuf*buf,structcommit_list*todo_list,-structreplay_opts*opts)+structtodo_item{+enumtodo_commandcommand;+structcommit*commit;+size_toffset_in_buf;+};++structtodo_list{+structstrbufbuf;+structtodo_item*items;+intnr,alloc,current;+};++#define TODO_LIST_INIT { STRBUF_INIT, NULL, 0, 0, 0 }++staticvoidtodo_list_release(structtodo_list*todo_list){-structcommit_list*cur=NULL;-constchar*sha1_abbrev=NULL;-constchar*action_str=opts->action==REPLAY_REVERT?"revert":"pick";-constchar*subject;-intsubject_len;+strbuf_release(&todo_list->buf);+free(todo_list->items);+todo_list->items=NULL;+todo_list->nr=todo_list->alloc=0;+}-for(cur=todo_list;cur;cur=cur->next){-constchar*commit_buffer=get_commit_buffer(cur->item,NULL);-sha1_abbrev=find_unique_abbrev(cur->item->object.oid.hash,DEFAULT_ABBREV);-subject_len=find_commit_subject(commit_buffer,&subject);-strbuf_addf(buf,"%s %s %.*s\n",action_str,sha1_abbrev,-subject_len,subject);-unuse_commit_buffer(cur->item,commit_buffer);-}-return0;+structtodo_item*append_todo(structtodo_list*todo_list)+{+ALLOC_GROW(todo_list->items,todo_list->nr+1,todo_list->alloc);+returntodo_list->items+todo_list->nr++;}-staticstructcommit*parse_insn_line(char*bol,char*eol,structreplay_opts*opts)+staticintparse_insn_line(structtodo_item*item,constchar*bol,char*eol){unsignedcharcommit_sha1[20];-enumreplay_actionaction;char*end_of_object_name;-intsaved,status,padding;--if(starts_with(bol,"pick")){-action=REPLAY_PICK;-bol+=strlen("pick");-}elseif(starts_with(bol,"revert")){-action=REPLAY_REVERT;-bol+=strlen("revert");-}else-returnNULL;+inti,saved,status,padding;++for(i=0;i<ARRAY_SIZE(todo_command_strings);i++)+if(skip_prefix(bol,todo_command_strings[i],&bol)){+item->command=i;+break;+}+if(i>=ARRAY_SIZE(todo_command_strings))+return-1;/* Eat up extra spaces/ tabs before object name */padding=strspn(bol," \t");if(!padding)-returnNULL;+return-1;bol+=padding;-end_of_object_name=bol+strcspn(bol," \t\n");+end_of_object_name=(char*)bol+strcspn(bol," \t\n");saved=*end_of_object_name;*end_of_object_name='\0';status=get_sha1(bol,commit_sha1);*end_of_object_name=saved;-/*-*Verifythattheactionmatchesupwiththeonein-*opts;wedon'tsupportarbitraryinstructions-*/-if(action!=opts->action){-if(action==REPLAY_REVERT)-error((opts->action==REPLAY_REVERT)-?_("Cannot revert during another revert.")-:_("Cannot revert during a cherry-pick."));-else-error((opts->action==REPLAY_REVERT)-?_("Cannot cherry-pick during a revert.")-:_("Cannot cherry-pick during another cherry-pick."));-returnNULL;-}-if(status<0)-returnNULL;+return-1;-returnlookup_commit_reference(commit_sha1);+item->commit=lookup_commit_reference(commit_sha1);+return!item->commit;}-staticintparse_insn_buffer(char*buf,structcommit_list**todo_list,-structreplay_opts*opts)+staticintparse_insn_buffer(char*buf,structtodo_list*todo_list){-structcommit_list**next=todo_list;-structcommit*commit;+structtodo_item*item;char*p=buf;-inti;+inti,res=0;for(i=1;*p;i++){char*eol=strchrnul(p,'\n');-commit=parse_insn_line(p,eol,opts);-if(!commit)-returnerror(_("Could not parse line %d."),i);-next=commit_list_append(commit,next);++item=append_todo(todo_list);+item->offset_in_buf=p-todo_list->buf.buf;+if(parse_insn_line(item,p,eol)){+error("Invalid line: %.*s",(int)(eol-p),p);+res|=error(_("Could not parse line %d."),i);+item->command=-1;+}p=*eol?eol+1:eol;}-if(!*todo_list)+if(!todo_list->nr)returnerror(_("No commits parsed."));-return0;+returnres;}-staticintread_populate_todo(structcommit_list**todo_list,+staticintread_populate_todo(structtodo_list*todo_list,structreplay_opts*opts){constchar*todo_file=get_todo_path(opts);-structstrbufbuf=STRBUF_INIT;intfd,res;+strbuf_reset(&todo_list->buf);fd=open(todo_file,O_RDONLY);if(fd<0)returnerror_errno(_("Could not open %s"),todo_file);-if(strbuf_read(&buf,fd,0)<0){+if(strbuf_read(&todo_list->buf,fd,0)<0){close(fd);-strbuf_release(&buf);returnerror(_("Could not read %s."),todo_file);}close(fd);-res=parse_insn_buffer(buf.buf,todo_list,opts);-strbuf_release(&buf);+res=parse_insn_buffer(todo_list->buf.buf,todo_list);if(res)returnerror(_("Unusable instruction sheet: %s"),todo_file);return0;
@@ -964,30 +990,24 @@ static int sequencer_rollback(struct replay_opts *opts)return-1;}-staticintsave_todo(structcommit_list*todo_list,structreplay_opts*opts)+staticintsave_todo(structtodo_list*todo_list,structreplay_opts*opts){staticstructlock_filetodo_lock;-structstrbufbuf=STRBUF_INIT;-intfd;+constchar*todo_path=get_todo_path(opts);+intnext=todo_list->current,offset,fd;-fd=hold_lock_file_for_update(&todo_lock,git_path_todo_file(),0);+fd=hold_lock_file_for_update(&todo_lock,todo_path,0);if(fd<0)returnerror_errno(_("Could not lock '%s'"),git_path_todo_file());-if(format_todo(&buf,todo_list,opts)<0){-strbuf_release(&buf);-returnerror(_("Could not format %s."),git_path_todo_file());-}-if(write_in_full(fd,buf.buf,buf.len)<0){-strbuf_release(&buf);-returnerror_errno(_("Could not write to %s"),-git_path_todo_file());-}-if(commit_lock_file(&todo_lock)<0){-strbuf_release(&buf);-returnerror(_("Error wrapping up %s."),git_path_todo_file());-}-strbuf_release(&buf);+offset=next<todo_list->nr?+todo_list->items[next].offset_in_buf:todo_list->buf.len;+if(write_in_full(fd,todo_list->buf.buf+offset,+todo_list->buf.len-offset)<0)+returnerror(_("Could not write to %s (%s)"),+todo_path,strerror(errno));+if(commit_lock_file(&todo_lock)<0)+returnerror(_("Error wrapping up %s."),todo_path);return0;}
@@ -1026,9 +1046,8 @@ static int save_opts(struct replay_opts *opts)returnres;}-staticintpick_commits(structcommit_list*todo_list,structreplay_opts*opts)+staticintpick_commits(structtodo_list*todo_list,structreplay_opts*opts){-structcommit_list*cur;intres;setenv(GIT_REFLOG_ACTION,action_name(opts),0);
@@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)if(read_cache_unmerged())returnerror_resolve_conflict(action_name(opts));-/* Different translation strings for cherry-pick and revert */-if(opts->action==REPLAY_PICK)-error(_("Your local changes would be overwritten by cherry-pick."));-else-error(_("Your local changes would be overwritten by revert."));+error(_("Your local changes would be overwritten by %s."),+action_name(opts));if(advice_commit_before_merge)advise(_("Commit your changes or stash them to proceed."));
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:24
The `git-rebase-todo` file contains a list of commands. Most of those
commands have the form
<verb> <sha1> <oneline>
The <oneline> is displayed primarily for the user's convenience, as
rebase -i really interprets only the <verb> <sha1> part. However, there
are *some* places in interactive rebase where the <oneline> is used to
display messages, e.g. for reporting at which commit we stopped.
So let's just remember it when parsing the todo file; we keep a copy of
the entire todo file anyway (to write out the new `done` and
`git-rebase-todo` file just before processing each command), so all we
need to do is remember the begin and end offsets.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 7 +++++++
1 file changed, 7 insertions(+)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:27
In interactive rebases, we commit a little bit differently than the
sequencer did so far: we heed the "author-script", the "message" and
the "amend" files in the .git/rebase-merge/ subdirectory.
Likewise, we may want to edit the commit message *even* when providing
a file containing the suggested commit message. Therefore we change the
code to not even provide a default message when we do not want any, and
to call the editor explicitly.
As interactive rebase's GPG settings are configured differently from
how cherry-pick (and therefore sequencer) handles them, we will leave
support for that to the next commit.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++--------
sequencer.h | 3 ++
2 files changed, 83 insertions(+), 12 deletions(-)
@@ -27,6 +27,16 @@ static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")staticGIT_PATH_FUNC(git_path_opts_file,"sequencer/opts")staticGIT_PATH_FUNC(git_path_head_file,"sequencer/head")+/*+*AscripttosettheGIT_AUTHOR_NAME,GIT_AUTHOR_EMAIL,and+*GIT_AUTHOR_DATEthatwillbeusedforthecommitthatiscurrently+*beingrebased.+*/+staticGIT_PATH_FUNC(rebase_path_author_script,"rebase-merge/author-script")++/* We will introduce the 'interactive rebase' mode later */+#define IS_REBASE_I() 0+staticconstchar*get_dir(conststructreplay_opts*opts){returngit_path_seq_dir();
@@ -377,20 +387,72 @@ static int is_index_unchanged(void)return!hashcmp(active_cache_tree->sha1,head_commit->tree->object.oid.hash);}+staticchar**read_author_script(void)+{+structstrbufscript=STRBUF_INIT;+inti,count=0;+char*p,*p2,**env;+size_tenv_size;++if(strbuf_read_file(&script,rebase_path_author_script(),256)<=0)+returnNULL;++for(p=script.buf;*p;p++)+if(skip_prefix(p,"'\\\\''",(constchar**)&p2))+strbuf_splice(&script,p-script.buf,p2-p,"'",1);+elseif(*p=='\'')+strbuf_splice(&script,p---script.buf,1,"",0);+elseif(*p=='\n'){+*p='\0';+count++;+}++env_size=(count+1)*sizeof(*env);+strbuf_grow(&script,env_size);+memmove(script.buf+env_size,script.buf,script.len);+p=script.buf+env_size;+env=(char**)strbuf_detach(&script,NULL);++for(i=0;i<count;i++){+env[i]=p;+p+=strlen(p)+1;+}+env[count]=NULL;++returnenv;+}+/**Ifwearecherry-pick,andifthemergedidnotresultin*hand-editing,wewillhitthiscommitandinherittheoriginal*authordateandname.*Ifwearerevert,orifourcherry-pickresultsinahandmerge,-*wehadbettersaythatthecurrentuserisresponsibleforthat.+*wehadbettersaythatthecurrentuserisresponsibleforthat+*(except,ofcourse,whilerunninganinteractiverebase).*/-staticintrun_git_commit(constchar*defmsg,structreplay_opts*opts,+intsequencer_commit(constchar*defmsg,structreplay_opts*opts,intallow_empty){+char**env=NULL;structargv_arrayarray;intrc;constchar*value;+if(IS_REBASE_I()){+env=read_author_script();+if(!env)+returnerror("You have staged changes in your working "+"tree. If these changes are meant to be\n"+"squashed into the previous commit, run:\n\n"+" git commit --amend $gpg_sign_opt_quoted\n\n"+"If they are meant to go into a new commit, "+"run:\n\n"+" git commit $gpg_sign_opt_quoted\n\n"+"In both case, once you're done, continue "+"with:\n\n"+" git rebase --continue\n");+}+argv_array_init(&array);argv_array_push(&array,"commit");argv_array_push(&array,"-n");
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:28
The rebase command sports a `--gpg-sign` option that is heeded by the
interactive rebase.
This patch teaches the sequencer that trick, as part of the bigger
effort to make the sequencer the work horse of the interactive rebase.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 43 insertions(+), 5 deletions(-)
@@ -33,6 +34,11 @@ static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")*beingrebased.*/staticGIT_PATH_FUNC(rebase_path_author_script,"rebase-merge/author-script")+/*+*Thefollowingfilesarewrittenbygit-rebasejustafterparsingthe+*command-line(andareonlyconsumed,notmodified,bythesequencer).+*/+staticGIT_PATH_FUNC(rebase_path_gpg_sign_opt,"rebase-merge/gpg_sign_opt")/* We will introduce the 'interactive rebase' mode later */#define IS_REBASE_I() 0
@@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts,if(IS_REBASE_I()){env=read_author_script();-if(!env)+if(!env){+constchar*gpg_opt=gpg_sign_opt_quoted(opts);+returnerror("You have staged changes in your working ""tree. If these changes are meant to be\n""squashed into the previous commit, run:\n\n"-" git commit --amend $gpg_sign_opt_quoted\n\n"+" git commit --amend %s\n\n""If they are meant to go into a new commit, ""run:\n\n"-" git commit $gpg_sign_opt_quoted\n\n"+" git commit %s\n\n""In both case, once you're done, continue ""with:\n\n"-" git rebase --continue\n");+" git rebase --continue\n",gpg_opt,gpg_opt);+}}argv_array_init(&array);
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:29
As we are slowly teaching the sequencer to perform the hard work for
the interactive rebase, we need to read files that were written by
shell scripts.
These files typically contain a single line and are invariably ended
by a line feed (and possibly a carriage return before that). Let's use
a helper to read such files and to remove the line ending.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:31
In the upcoming commits, we will implement more and more of rebase
-i's functionality. One particular feature of the commands to come is
that some of them allow editing the commit message while others don't,
i.e. we cannot define in the replay_opts whether the commit message
should be edited or not.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 6 +++---
sequencer.h | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:34
This teaches the sequencer_commit() function to take an argument that
will allow us to implement "todo" commands that need to amend the commit
messages ("fixup", "squash" and "reword").
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 6 ++++--
sequencer.h | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:40
The sequencer_commit() function already knows how to amend commits, and
with this new option, it can also clean up commit messages (i.e. strip
out commented lines). This is needed to implement rebase -i's 'fixup'
and 'squash' commands as sequencer commands.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 10 +++++++---
sequencer.h | 3 ++-
2 files changed, 9 insertions(+), 4 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:46
The return value of do_recursive_merge() may be positive (indicating merge
conflicts), se let's OR later error conditions so as not to overwrite them
with 0.
This is not yet a problem, but preparing for the patches to come: we will
teach the sequencer to do rebase -i's job.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:06:57
The write_message() function safely writes an strbuf to a file.
Sometimes this is inconvenient, though: the text to be written may not
be stored in a strbuf, or the strbuf should not be released after
writing.
Let's allow for such use cases by refactoring write_message() to allow
for a convenience function write_file_gently(). As some of the upcoming
callers of that new function will want to append a newline character,
let's just add a flag for that, too.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 21 ++++++++++++++++++---
1 file changed, 18 insertions(+), 3 deletions(-)
@@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)}}-staticintwrite_message(structstrbuf*msgbuf,constchar*filename)+staticintwrite_with_lock_file(constchar*filename,+constvoid*buf,size_tlen,intappend_eol){staticstructlock_filemsg_file;intmsg_fd=hold_lock_file_for_update(&msg_file,filename,0);if(msg_fd<0)returnerror_errno(_("Could not lock '%s'"),filename);-if(write_in_full(msg_fd,msgbuf->buf,msgbuf->len)<0)+if(write_in_full(msg_fd,buf,len)<0)returnerror_errno(_("Could not write to %s"),filename);-strbuf_release(msgbuf);+if(append_eol&&write(msg_fd,"\n",1)<0)+returnerror_errno(_("Could not write eol to %s"),filename);if(commit_lock_file(&msg_file)<0)returnerror(_("Error wrapping up %s."),filename);return0;}+staticintwrite_message(structstrbuf*msgbuf,constchar*filename)+{+intres=write_with_lock_file(filename,msgbuf->buf,msgbuf->len,0);+strbuf_release(msgbuf);+returnres;+}++staticintwrite_file_gently(constchar*filename,+constchar*text,intappend_eol)+{+returnwrite_with_lock_file(filename,text,strlen(text),append_eol);+}+/**Readsafilethatwaspresumablywrittenbyashellscript,i.e.*withanend-of-linemarkerthatneedstobestripped.
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:07:00
Interactive rebase's scripts may be indented; We need to handle this
case, too, now that we prepare the sequencer to process interactive
rebases.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 3 +++
1 file changed, 3 insertions(+)
From: Johannes Schindelin <hidden> Date: 2016-08-29 08:07:13
The subcommands are used exactly once, at the very beginning of
sequencer_pick_revisions(), to determine what to do. This is an
unnecessary level of indirection: we can simply call the correct
function to begin with. So let's do that.
While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).
Signed-off-by: Johannes Schindelin <redacted>
---
builtin/revert.c | 36 ++++++++++++++++--------------------
sequencer.c | 35 +++++++++++------------------------
sequencer.h | 13 ++++---------
3 files changed, 31 insertions(+), 53 deletions(-)
This looked off to me, as it replaces memset(..., 0, ...) so is not
100% equivalent. But the changed functions both set opts.action and
call parse_args which sets opts.subcommand.
D.
From: Dennis Kaarsemaker <hidden> Date: 2016-08-29 09:40:34
On ma, 2016-08-29 at 10:05 +0200, Johannes Schindelin wrote:
<snip actual commit>
I fail to see the point of this patch, would you mind enlightening me?
D.
From: Dennis Kaarsemaker <hidden> Date: 2016-08-29 09:51:56
On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:
The return value of do_recursive_merge() may be positive (indicating merge
conflicts), se let's OR later error conditions so as not to overwrite them
with 0.
This looked off to me, as it replaces memset(..., 0, ...) so is not
100% equivalent. But the changed functions both set opts.action and
call parse_args which sets opts.subcommand.
Okay... Do you want me to change anything?
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-08-29 11:04:58
Hi Dennis,
On Mon, 29 Aug 2016, Dennis Kaarsemaker wrote:
On ma, 2016-08-29 at 10:05 +0200, Johannes Schindelin wrote:
<snip actual commit>
I fail to see the point of this patch, would you mind enlightening me?
Two reasons:
1) by refactoring it into a function, the code is more DRY (with all the
advantages that come with it, such as: only a single point to change if
changing the behavior)
2) it is easier to reuse the code in upcoming patches (that would be in
the next patch series)
Will amend the commit message.
Ciao,
Dscho
Why not use open + strbuf_getline instead of hand-rolling a newline
eradicator?
Because strbuf_getline() erases the strbuf instead of appending to it
(which is what we sometimes need when converting shell scripts to C).
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-08-29 11:10:10
Hi Dennis,
On Mon, 29 Aug 2016, Dennis Kaarsemaker wrote:
On ma, 2016-08-29 at 10:06 +0200, Johannes Schindelin wrote:
quoted
The return value of do_recursive_merge() may be positive (indicating merge
conflicts), se let's OR later error conditions so as not to overwrite them
with 0.
From: Johannes Schindelin <hidden> Date: 2016-08-29 11:20:10
Hi Dennis,
On Mon, 29 Aug 2016, Johannes Schindelin wrote:
On Mon, 29 Aug 2016, Dennis Kaarsemaker wrote:
quoted
On ma, 2016-08-29 at 10:04 +0200, Johannes Schindelin wrote:
quoted
+ if (read_and_refresh_cache(opts))
+ return -1;
+
This doesn't seem to be related to the get_dir changes?
Good eyes.
Let me investigate why I have it here...
Unfortunately my reflogs got corrupted by the git-worktree
implementations, so I cannot back that far.
Looking at the code, and after running the tests, I am convinced that it
is a leftover of some misguided attempt to implement "git rebase -i
--abort" in sequencer_rollback().
I removed this hunk from the patch.
Again, Thank you so much for your review!
Dscho
This looked off to me, as it replaces memset(..., 0, ...) so is not
100% equivalent. But the changed functions both set opts.action and
call parse_args which sets opts.subcommand.
This information would be nice to have in the commit message.
--
Jakub Narębski
So it is more "Use memoized sequencer directory path" rather than
"sequencer: use memoized sequencer directory path" - it replaces
all occurrences of SEQ_DIR,... that's why it can be removed from
'sequencer.h'.
Though perhaps I misunderstood "sequencer: " prefix there. Don't
mind me then.
Especially that other *_populate_*() use 'struct replay_opts *opts':
read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
walk_revs_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
Though they use **todo_list, because they modify this list;
maybe that was why read_populate_opts was using **opts instead
of *opts?
quoted hunk
{
if (!file_exists(git_path_opts_file()))
return 0;
- if (git_config_from_file(populate_opts_cb, git_path_opts_file(), *opts) < 0)
+ if (git_config_from_file(populate_opts_cb, git_path_opts_file(), opts) < 0)
return error(_("Malformed options sheet: %s"),
git_path_opts_file());
return 0;
@@ -1038,7 +1038,7 @@ static int sequencer_continue(struct replay_opts *opts) if (!file_exists(git_path_todo_file())) return continue_single_pick();- if (read_populate_opts(&opts) ||+ if (read_populate_opts(opts) || read_populate_todo(&todo_list, opts)) return -1;
Not that I am against this part of change, making initialization
explicit, but why we are initializing automatic variables with 0,
which would be the default value anyway? I thought our coding
guidelines discourage initializing with 0 or NULL...
Puzzled,
--
Jakub Narębski
From: Jakub Narębski <hidden> Date: 2016-08-29 21:59:56
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.
This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was
done to allow using the functionality as a library function, though,
including proper clean-up after use.
This patch introduces an API to pass the responsibility of releasing
certain memory to the sequencer.
So how this API would be / is meant to be used? From the following
patches (which I shouldn't have to read to understand this one)
it looks like it is about strdup'ed strings from option parsing.
Or would there be something more in the future?
Would sequencer as a library function be called multiple times,
or only once?
I'm trying to find out how this is solved in other places of Git
code, and I have stumbled upon free_util in string_list...
I was wondering what this 'set_me_free_after_use' parameter is about;
wouldn't it be more readable if this parameter was called 'owned_data'
or 'owned_ptr'?
+}
+
static void remove_sequencer_state(const struct replay_opts *opts)
{
struct strbuf dir = STRBUF_INIT;
+ int i;
+
+ for (i = 0; i < opts->owned_nr; i++)
+ free(opts->owned[i]);
I guess you can remove owned data in any order, regardless if you
store struct or its members first...
@@ -43,8 +43,14 @@ struct replay_opts {/* Only used by REPLAY_NONE */structrev_info*revs;++/* malloc()ed data entrusted to the sequencer */+void**owned;+intowned_nr,owned_alloc;
I'm not sure about naming conventions for those types of data, but
wouldn't 'owned_data' be a better name? I could be wrong here...
Nb. it is a pity that we cannot use named initializers for structs,
so called designated inits. It would make this macro more readable.
It is actually pointless to add the 0's and NULL's here. This should be
sufficient:
#define REPLAY_OPTS_INIT { -1, -1 }
because initialization with 0 (or NULL) is the default for any omitted
members.
-- Hannes
This looked off to me, as it replaces memset(..., 0, ...) so is not
100% equivalent. But the changed functions both set opts.action and
call parse_args which sets opts.subcommand.
This information would be nice to have in the commit message.
So it is more "Use memoized sequencer directory path" rather than
"sequencer: use memoized sequencer directory path" - it replaces
all occurrences of SEQ_DIR,... that's why it can be removed from
'sequencer.h'.
Though perhaps I misunderstood "sequencer: " prefix there. Don't
mind me then.
The idea is that this path is declared and defined in the sequencer. There
are other call sites, too, so they have to be changed at the same time...
I'd really like to keep the "sequencer:" prefix because it is semantically
correct: this change is about the sequencer, not about the other call
sites.
Ciao,
Johannes
Especially that other *_populate_*() use 'struct replay_opts *opts':
read_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
walk_revs_populate_todo(struct commit_list **todo_list, struct replay_opts *opts)
Though they use **todo_list, because they modify this list;
maybe that was why read_populate_opts was using **opts instead
of *opts?
I won't speculate about the reasons why it was made so.
About read_populate_todo(): it uses **todo_list, but still only *opts.
In any case, in a later patch, the todo_list parsing is completely
revamped anyway, so I did not want to "fix" anything that would get
reverted later on.
Ciao,
Johannes
From: Johannes Schindelin <hidden> Date: 2016-08-30 07:29:49
Hi Kuba,
On Mon, 29 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted
The sequencer is our attempt to lib-ify cherry-pick. Yet it behaves
like a one-shot command when it reads its configuration: memory is
allocated and released only when the command exits.
This is kind of okay for git-cherry-pick, which *is* a one-shot
command. All the work to make the sequencer its work horse was done to
allow using the functionality as a library function, though, including
proper clean-up after use.
This patch introduces an API to pass the responsibility of releasing
certain memory to the sequencer.
So how this API would be / is meant to be used?
I added an example to the commit message.
Would sequencer as a library function be called multiple times,
or only once?
The point of a library function is that it should not care.
I'm trying to find out how this is solved in other places of Git
code, and I have stumbled upon free_util in string_list...
I wanted this to be flexible enough to take care of any type of data, not
just strings.
And while the string_list has a void *util field, it would be rather silly
to add strings to a string list for the sole purpose of free()ing their
util fields in the end.
(That was the conclusion I came to after a search of my own.)
I was wondering what this 'set_me_free_after_use' parameter is about;
wouldn't it be more readable if this parameter was called 'owned_data'
or 'owned_ptr'?
If I read "owned_ptr" as a function's parameter, I would assume that the
associated memory is owned by the caller. So I would be puzzled reading
that name.
quoted
static void remove_sequencer_state(const struct replay_opts *opts)
{
struct strbuf dir = STRBUF_INIT;
+ int i;
+
+ for (i = 0; i < opts->owned_nr; i++)
+ free(opts->owned[i]);
I guess you can remove owned data in any order, regardless if you
store struct or its members first...
Indeed, this is not like a C++ destructor. It's free().
@@ -43,8 +43,14 @@ struct replay_opts {/* Only used by REPLAY_NONE */structrev_info*revs;++/* malloc()ed data entrusted to the sequencer */+void**owned;+intowned_nr,owned_alloc;
I'm not sure about naming conventions for those types of data, but
wouldn't 'owned_data' be a better name? I could be wrong here...
The convention seemed to be "void *X; int X_nr, X_alloc;", so I stuck with
it.
Thanks for your review!
Johannes
Nb. it is a pity that we cannot use named initializers for structs,
so called designated inits. It would make this macro more readable.
It is actually pointless to add the 0's and NULL's here. This should be
sufficient:
#define REPLAY_OPTS_INIT { -1, -1 }
because initialization with 0 (or NULL) is the default for any omitted
members.
D'oh. You're right. The same applies to TODO_LIST_INIT, of course.
Fixed,
Johannes
I was wondering what this 'set_me_free_after_use' parameter is about;
wouldn't it be more readable if this parameter was called 'owned_data'
or 'owned_ptr'?
If I read "owned_ptr" as a function's parameter, I would assume that the
associated memory is owned by the caller. So I would be puzzled reading
that name.
Right. Well, it is difficult to come up with a good name for this
parameter that would make sense both in a declaration as an information
for a caller, and in the function itself as information about what it
holds.
In my personal opinion 'set_me_free_after_use' is not the best name,
but I unfortunately do not have a better proposal. Maybe 'entrust_ptr',
or 'entrusted_data' / 'entrusted_ptr' / 'entrusted'?
There are two hard things in computer science: cache invalidation,
*naming things*, and off-by-one errors ;-)
P.S. It would be nice to have generic mechanism for taking custody
of data to help libification, either at this or at lower level (on
the level of xstrdup, etc.), but that can safely wait. It even should
wait, so that we can see that this approach is a good one, before
trying to generalize it. That should be not a blocker for this series,
IMVHO.
Best,
--
Jakub Narębski
From: Jakub Narębski <hidden> Date: 2016-08-30 14:55:02
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted hunk
The sequencer reads options from disk and stores them in its struct
for use during sequencer's operations.
With this patch, the memory is released afterwards, plugging a
memory leak.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
This looks like independent change, not related to using the
sequencer_entrust() to store options read from disk in replay_opts
struct to be able to free memory afterwards.
I guess you wanted to avoid one line changes...
Sidenote: this patch would be easier to read if lines were reordered
as below, but I don't think any slider heuristics could help achieve
that automatically. Also, the patch might be invalid...
I wonder if the ability to free strings dup-ed by git_config_string()
be something that is part of replay_opts, or rather remove_sequencer_state(),
that is a list of
free(opts->strategy);
free(opts->gpg_sign);
And of course
for (i = 0; i < opts->xopts_nr; i++)
free(opts->xopts[i]);
free(opts->xopts);
Though... free(NULL) is nop as per standard, but can we rely on it?
If it is a problem, we can create xfree(ptr) being if(ptr)free(ptr);
The *_entrust() mechanism is more generic, but do we use this general-ness?
Well, it could be xstrdup or git_config_string doing entrust'ing...
From: Jakub Narębski <hidden> Date: 2016-08-30 16:08:03
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted hunk
Over the next commits, we will work on improving the sequencer to the
point where it can process the edit script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
I guess that in the future commit the return value of get_todo_path()
would change depending on what sequencer is used for, cherry-pick or
interactive rebase, that is, contents of replay_opts...
quoted hunk
+
static int is_rfc2822_line(const char *buf, int len)
{
int i;
From: Johannes Schindelin <hidden> Date: 2016-08-30 17:37:59
Hi Kuba,
On Tue, 30 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted
Over the next commits, we will work on improving the sequencer to the
point where it can process the edit script of an interactive rebase. To
that end, we will need to teach the sequencer to read interactive
rebase's todo file. In preparation, we consolidate all places where
that todo file is needed to call a function that we will later extend.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
I guess that in the future commit the return value of get_todo_path()
would change depending on what sequencer is used for, cherry-pick or
interactive rebase, that is, contents of replay_opts...
Right.
quoted
static int is_rfc2822_line(const char *buf, int len)
{
int i;
From: Johannes Schindelin <hidden> Date: 2016-08-30 17:53:14
Hi Kuba,
On Tue, 30 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted
The sequencer reads options from disk and stores them in its struct
for use during sequencer's operations.
With this patch, the memory is released afterwards, plugging a
memory leak.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
This looks like independent change, not related to using the
sequencer_entrust() to store options read from disk in replay_opts
struct to be able to free memory afterwards.
I guess you wanted to avoid one line changes...
Actually, it is not an independent change, but it free()s memory that has
been allocated while reading the options, as the commit message says ;-)
Sidenote: this patch would be easier to read if lines were reordered
as below, but I don't think any slider heuristics could help achieve
that automatically. Also, the patch might be invalid...
I wonder if the ability to free strings dup-ed by git_config_string()
be something that is part of replay_opts, or rather remove_sequencer_state(),
that is a list of
free(opts->strategy);
free(opts->gpg_sign);
That is not necessarily possible because the way sequencer works, the
options may have not actually be read from the file, but may be populated
by the caller (in which case we do not necessarily want to require
strdup()ing the strings just so that the sequencer can clean stuff up
afterwards).
Though... free(NULL) is nop as per standard, but can we rely on it?
We can, and we do.
The *_entrust() mechanism is more generic, but do we use this general-ness?
Well, it could be xstrdup or git_config_string doing entrust'ing...
Right, but that is exactly what I wanted to avoid, because it is rather
inelegant to strdup() strings just so that we do not have to record what
to free() and what not to free().
BTW I have no objection at all to generalize this sequencer_entrust()
mechanism further (read: to other, similar use cases), should it withstand
the test of time.
Ciao,
Johannes
From: Johannes Sixt <hidden> Date: 2016-08-30 20:47:06
Am 30.08.2016 um 19:52 schrieb Johannes Schindelin:
Right, but that is exactly what I wanted to avoid, because it is rather
inelegant to strdup() strings just so that we do not have to record what
to free() and what not to free().
Please, excuse, but when I have to choose what is more "elegant":
1. strdup() sometimes so that I can later free() always
2. use sequencer_entrust()
I would choose 1. at all times.
Particularly in this case: parsing options does not sound like a major
drain of resources, neither CPU- nor memory-wise.
-- Hannes
From: Jakub Narębski <hidden> Date: 2016-08-30 22:02:00
W dniu 30.08.2016 o 19:52, Johannes Schindelin pisze:
Hi Kuba,
On Tue, 30 Aug 2016, Jakub Narębski wrote:
quoted
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted
The sequencer reads options from disk and stores them in its struct
for use during sequencer's operations.
With this patch, the memory is released afterwards, plugging a
memory leak.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
This looks like independent change, not related to using the
sequencer_entrust() to store options read from disk in replay_opts
struct to be able to free memory afterwards.
I guess you wanted to avoid one line changes...
Actually, it is not an independent change, but it free()s memory that has
been allocated while reading the options, as the commit message says ;-)
Sidenote: this patch would be easier to read if lines were reordered
as below, but I don't think any slider heuristics could help achieve
that automatically. Also, the patch might be invalid...
I wonder if the ability to free strings dup-ed by git_config_string()
be something that is part of replay_opts, or rather remove_sequencer_state(),
that is a list of
free(opts->strategy);
free(opts->gpg_sign);
That is not necessarily possible because the way sequencer works, the
options may have not actually be read from the file, but may be populated
by the caller (in which case we do not necessarily want to require
strdup()ing the strings just so that the sequencer can clean stuff up
afterwards).
I guess from cursory browsing through the Git code that _currently_
they are only read from the config file, where git_config_string()
strdup's them, isn't it? And we want to prepare for the future, where
the caller would prepare replay_opts, and the caller would be responsible
for freeing data if necessary?
Would there be any sane situation where some of data should be owned
by caller (and freed by caller), and some of data should be owned by
sequencer library API (and freed in remove_sequencer_state())? If
not, perhaps *_entrust() mechanism is overthinking it, and we simply
need 'is_strdup' boolean flag or something like that...
quoted
The *_entrust() mechanism is more generic, but do we use this general-ness?
Well, it could be xstrdup or git_config_string doing entrust'ing...
Right, but that is exactly what I wanted to avoid, because it is rather
inelegant to strdup() strings just so that we do not have to record what
to free() and what not to free().
Maybe inelegant, but it might be easier than inventing and implementing
*_entrust() mechanism, like Hannes wrote.
BTW I have no objection at all to generalize this sequencer_entrust()
mechanism further (read: to other, similar use cases), should it withstand
the test of time.
From: Jakub Narębski <hidden> Date: 2016-08-31 13:41:25
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.
The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.
Actually the question is what does the test that got removed in this
commit actually check. Is it high-level sanity check that todo list
for git-cherry-pick contains only 'pick', and for git-revert contains
only 'revert'? Or does it check that at the low level sequencer
fails when instruction sheet includes only identical operations?
Only if it is the latter (we are testing too low level details of
how sequencer code works, tying too tightly test with implementation)
the test should be removed. I see that earlier test check that
sequencer handles correctly invalid instructions in todo.
Therefore let's just get rid of the test that wants to verify that this
limitation is still in place, in preparation for the upcoming work to
teach the sequencer to do rebase -i's work.
Is it "upcoming work" as in one of the patches in this series?
If so, which patch?
Hmmm... the description is somewhat lacking (especially compared to
the rest of test), anyway.
BTW. we should probably rename 'malformed instruction sheet 2'
to 'malformed instruction sheet' if there are no further such
tests after this removal, isn't it?
From: Jakub Narębski <hidden> Date: 2016-08-31 17:29:28
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.
However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.
Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).
Does this mean that after the change you would be able to continue
"git revert" with "git cherry-pick --continue", and vice versa? Or that
it would be possible for git-cherry-pick to do reverts (e.g. with ^<rev>)?
That's what we need to decide before becoming more lenient.
BTW. perhaps we would be able to continue with 'git continue', regardless
of what we have started with, I wonder...
Finally, the sequencer code chose to deviate from the interactive rebase
code insofar that it *reformats* the "todo" script instead of just
writing the part of the parsed script that were not yet processed. This
is not only unnecessary churn, but might well lose information that is
valuable to the user (i.e. comments after the commands).
That's a very good change.
Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.
Sidenote: this is not your fault, but Git doesn't do a good job on
changes which are mostly rewrites, trying to match stray '}' and the
like in generated diff. I wonder if existing diff heuristic options
could help here.
While at it, do not stop at the first problem, but list *all* of the
problems. This helps the user by allowing to address all issues in
one go rather than going back and forth until the todo list is valid.
That is also a good change, though I wonder how often users need
to worry about this outside interactive rebase case. If it is
preparation for rebase -i, where instruction list is written by
prone to errors human, it would be nice to have this information
in the commit message.
Do we have a naming convention for enums elements? Or are we explicitly
making enums and #defines interchangeable? I wonder...
...uh, I see we don't have naming convention, but all caps snake-case
names dominate:
$ git grep -A2 'enum .* {'
[...]
diff.h:enum color_diff {
diff.h- DIFF_RESET = 0,
diff.h- DIFF_CONTEXT = 1,
--
dir.c:enum path_treatment {
dir.c- path_none = 0,
dir.c- path_recurse,
--
Shouldn't we say 'TODO_PICK = 0' explicitly, though?
It's a bit pity that we cannot use designated inits, and hanging comma,
(from ISO C99 standard). That is:
+static const char *todo_command_strings[] = {
+ [TODO_PICK] = "pick",
+ [TODO_REVERT] = "revert",
+};
@@ -535,7 +554,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) /* TRANSLATORS: The first %s will be "revert" or "cherry-pick", the second %s a SHA1 */ return error(_("%s: cannot parse parent commit %s"),
I wonder if we should not change also the error message: it is no
longer about command, but about operation in todo list (from what
I understand). Though admittedly current message works for both...
quoted hunk
- action_name(opts), oid_to_hex(&parent->object.oid));
+ command_to_string(command),
+ oid_to_hex(&parent->object.oid));
if (get_message(commit, &msg) != 0)
return error(_("Cannot get commit message for %s"),
From here on changes are about
s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
Do we still need opts->action, or it is just needed less,
and it is 'todo' instruction that decides about command
(as it should)?
quoted hunk
* reverse of it if we are revert.
*/
- if (opts->action == REPLAY_REVERT) {
+ if (command == TODO_REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -589,7 +609,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) } }- if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || command == TODO_REVERT) { res = do_recursive_merge(base, next, base_label, next_label, head, &msgbuf, opts); if (res < 0)
@@ -615,17 +635,17 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts) * However, if the merge did not even start, then we don't want to * write it at all. */- if (opts->action == REPLAY_PICK && !opts->no_commit && (res == 0 || res == 1) &&+ if (command == TODO_PICK && !opts->no_commit && (res == 0 || res == 1) && update_ref(NULL, "CHERRY_PICK_HEAD", commit->object.oid.hash, NULL, REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) res = -1;- if (opts->action == REPLAY_REVERT && ((opts->no_commit && res == 0) || res == 1) &&+ if (command == TODO_REVERT && ((opts->no_commit && res == 0) || res == 1) && update_ref(NULL, "REVERT_HEAD", commit->object.oid.hash, NULL, REF_NODEREF, UPDATE_REFS_MSG_ON_ERR)) res = -1; if (res) {- error(opts->action == REPLAY_REVERT+ error(command == TODO_REVERT ? _("could not revert %s... %s") : _("could not apply %s... %s"), find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV),
And here those changes end.
s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
I wonder if Coccinelle / Undebt would help here; or would simple
sed or query-and-replace-regexp be enough...
So there should be s/commit_list [*]{1,2}todo_list/todo_list *todo_list/
from here on?
Hmmm... commit_list is, as defined in commit.h, a linked list.
Here todo_list uses growable array implementation of list. Which
is I guess better on current CPU architecture, with slow memory,
limited-size caches, and adjacency prefetching.
I guess using items_nr and items_alloc would be not necessary
(and a bit funny / overkill).
Errr... I don't quite understand the name of this function.
What are you appending here to the todo_list?
Compare string_list_append() and string_list_append_nodup(),
where the second parameter is item to append.
I'm not against what this function does (grow array if needed, and
return pointer to the new todo_item that is to be filled), but
I don't quite agree with the name. Naming is hard... :-(
[See later in reply for a proposal.]
Why the change of return type?
I guess the previous code used only opts->action out of whole replay_opts,
and now we use item->command instead; that is why replay_opts is replaced
by todo_item.
Why now struct todo_item is first when struct replay_opts was last?
Not that I say is was a bad change...
{
unsigned char commit_sha1[20];
- enum replay_action action;
char *end_of_object_name;
- int saved, status, padding;
-
- if (starts_with(bol, "pick")) {
- action = REPLAY_PICK;
- bol += strlen("pick");
- } else if (starts_with(bol, "revert")) {
- action = REPLAY_REVERT;
- bol += strlen("revert");
- } else
- return NULL;
+ int i, saved, status, padding;
int i or enum? Just kidding...
+
+ for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
+ if (skip_prefix(bol, todo_command_strings[i], &bol)) {
skip_prefix() is such a nice abstraction...
+ item->command = i;
+ break;
+ }
Nice. Replacing if-elsif chain with loop.
I guess any hashmap would be serious overkill, as there are and would be
only a few actions possible.
+ if (i >= ARRAY_SIZE(todo_command_strings))
+ return -1;
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
- return NULL;
+ return -1;
bol += padding;
- end_of_object_name = bol + strcspn(bol, " \t\n");
+ end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
Why is this cast needed?
And do we say '(char *) bol' or '(char *)bol'?
saved = *end_of_object_name;
*end_of_object_name = '\0';
status = get_sha1(bol, commit_sha1);
*end_of_object_name = saved;
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- if (action == REPLAY_REVERT)
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot revert during another revert.")
Errr... could the above ever happen? Namely
action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
Surely not.
- : _("Cannot revert during a cherry-pick."));
- else
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot cherry-pick during a revert.")
- : _("Cannot cherry-pick during another cherry-pick."));
- return NULL;
- }
Anyway, while it is / would be a good idea to prevent starting any
sequencer-based command (cherry-pick, revert, soon rebase -i) when
other command is in progress (cherry-pick, revert, soon rebase -i).
That is, if cherry-pick / revert waits for user action, you cannot
run another cherry-pick or revert.
Which I guess the above code was not about...
-
if (status < 0)
- return NULL;
+ return -1;
- return lookup_commit_reference(commit_sha1);
+ item->commit = lookup_commit_reference(commit_sha1);
+ return !item->commit;
}
-static int parse_insn_buffer(char *buf, struct commit_list **todo_list,
- struct replay_opts *opts)
+static int parse_insn_buffer(char *buf, struct todo_list *todo_list)
{
- struct commit_list **next = todo_list;
- struct commit *commit;
+ struct todo_item *item;
char *p = buf;
- int i;
+ int i, res = 0;
for (i = 1; *p; i++) {
char *eol = strchrnul(p, '\n');
- commit = parse_insn_line(p, eol, opts);
- if (!commit)
- return error(_("Could not parse line %d."), i);
- next = commit_list_append(commit, next);
+
+ item = append_todo(todo_list);
A better name, in my personal option, would be
+ item = todo_list_next(todo_list);
Or todo_next(todo_list).
+ item->offset_in_buf = p - todo_list->buf.buf;
+ if (parse_insn_line(item, p, eol)) {
+ error("Invalid line: %.*s", (int)(eol - p), p);
This error message should, I think, be also translatable:
+ error(_("Invalid line: %.*s"), (int)(eol - p), p);
+ res |= error(_("Could not parse line %d."), i);
Wouldn't it make more sense to reverse order of errors, that is
first tell which line, and then show it?
BTW. would be we able to show where exactly there was problem parsing,
that is at which character in line? Or is it something for the future?
Ah, so 'res' is "was there an error" in any of lines. Nice.
}
-static int read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
const char *todo_file = get_todo_path(opts);
If I understand it correctly, replay_opts is used only to find out
correct todo_file, isn't it?
- struct strbuf buf = STRBUF_INIT;
int fd, res;
+ strbuf_reset(&todo_list->buf);
fd = open(todo_file, O_RDONLY);
if (fd < 0)
return error_errno(_("Could not open %s"), todo_file);
- if (strbuf_read(&buf, fd, 0) < 0) {
+ if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
close(fd);
- strbuf_release(&buf);
A question: when is todo_list->buf released?
return error(_("Could not read %s."), todo_file);
}
close(fd);
- res = parse_insn_buffer(buf.buf, todo_list, opts);
+ res = parse_insn_buffer(todo_list->buf.buf, todo_list);
- strbuf_release(&buf);
if (res)
return error(_("Unusable instruction sheet: %s"), todo_file);
return 0;
Nice.
quoted hunk
@@ -848,18 +859,33 @@ static int read_populate_opts(struct replay_opts *opts) return 0; }-static int walk_revs_populate_todo(struct commit_list **todo_list,+static int walk_revs_populate_todo(struct todo_list *todo_list, struct replay_opts *opts) {+ enum todo_command command = opts->action == REPLAY_PICK ?+ TODO_PICK : TODO_REVERT; struct commit *commit;- struct commit_list **next; if (prepare_revs(opts)) return -1;- next = todo_list;- while ((commit = get_revision(opts->revs)))- next = commit_list_append(commit, next);+ while ((commit = get_revision(opts->revs))) {+ struct todo_item *item = append_todo(todo_list);+ const char *commit_buffer = get_commit_buffer(commit, NULL);
I see that you are creating todo file contents while walking revision list,
something that was left for later in current / previous implementation
of the sequencer...
[Added: I see it was done by format_todo() called from save_todo()]
Wouldn't it be simpler to use
+ todo_command_strings[command],
Also, this string does not change during the loop, though I guess
compiler should be able to optimize it.
...Did format of the 'todo' file changed? And if yes, was it in backward
compatible way, so that "git revert" or "git cherry-pick" started with
old version of Git can be continued with new version, and what is also
important (for somebody who sometimes uses system-installed Git, and
sometimes user-compiled one) the reverse: started with new, continued
with old?
@@ -964,30 +990,24 @@ static int sequencer_rollback(struct replay_opts *opts) return -1; }-static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) { static struct lock_file todo_lock;- struct strbuf buf = STRBUF_INIT;- int fd;+ const char *todo_path = get_todo_path(opts);+ int next = todo_list->current, offset, fd;
The "next = todo_list->current" looks a bit strange. Also, we do not
change todo_list->current, we use it in one place, so it can be used
directly without help of temporary / helper variable. But that is
just my personal opinion.
Also, from 'next', 'offset' and 'fd', all those are different
uses of int: the index (int, rarely size_t), the offset in string
(formally ptrdiff_t, or size_t, but usually int), and the file descriptor.
I think from those the file descriptor could be kept in separate line;
it would help diff to be more readable. But this is fairly marginal
nitpicking, and a matter of personal opinion.
We should use 'todo_path' here... and this should be done in
one of earlier patches, isn't it?
This means that
+ const char *todo_path = get_todo_path(opts);
should better be moved to earlier patch, too.
Or maybe not. But it looks like missed git_path_todo_file() -> get_todo_path(opts)
[-> todo_path ] change. If it was left because of planned whole rewrite,
it should be mentioned in the commit message of that earlier commit,
isn't it?
- if (format_todo(&buf, todo_list, opts) < 0) {
- strbuf_release(&buf);
- return error(_("Could not format %s."), git_path_todo_file());
Can we still get this error? Could we get this error anyway,
and under what conditions?
- }
- if (write_in_full(fd, buf.buf, buf.len) < 0) {
- strbuf_release(&buf);
- return error_errno(_("Could not write to %s"),
- git_path_todo_file());
- }
+ offset = next < todo_list->nr ?
+ todo_list->items[next].offset_in_buf : todo_list->buf.len;
+ if (write_in_full(fd, todo_list->buf.buf + offset,
+ todo_list->buf.len - offset) < 0)
+ return error(_("Could not write to %s (%s)"),
+ todo_path, strerror(errno));
Ah, so it saves the remaining todo_items on todo_list, not the
whole todo_list... the name does not fully show it.
- if (commit_lock_file(&todo_lock) < 0) {
- strbuf_release(&buf);
- return error(_("Error wrapping up %s."), git_path_todo_file());
- }
- strbuf_release(&buf);
+ if (commit_lock_file(&todo_lock) < 0)
+ return error(_("Error wrapping up %s."), todo_path);
Note: this is unrelated change, but we usually put paths in quotes, like this
+ return error(_("Error wrapping up '%s'."), todo_path);
(in this and earlier error message), so that paths containing spaces show
correctly and readably to the user. Though this possibly is not a problem
for this path.
Also, how user is to understand "wrapping up"?
quoted hunk
return 0;
}
@@ -1026,9 +1046,8 @@ static int save_opts(struct replay_opts *opts) return res; }-static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)+static int pick_commits(struct todo_list *todo_list, struct replay_opts *opts) {- struct commit_list *cur; int res; setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -1038,10 +1057,12 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts) if (read_and_refresh_cache(opts)) return -1;- for (cur = todo_list; cur; cur = cur->next) {+ while (todo_list->current < todo_list->nr) {
Why replace for loop with while loop? Especially that now it
looks more for-y ;-)
+ for ( ; todo_list->current < todo_list->nr; todo_list->current++) {
Oh... I now see why.
+ struct todo_item *item = todo_list->items + todo_list->current;
- if (save_todo(cur, opts))
+ if (save_todo(todo_list, opts))
return -1;
- res = do_pick_commit(cur->item, opts);
+ res = do_pick_commit(item->command, item->commit, opts);
I don't quite understand what sequencer tried to do here, but the
change looks all right.
quoted hunk
+ todo_list->current++;
if (res)
return res;
}
@@ -1066,7 +1087,8 @@ static int continue_single_pick(void) static int sequencer_continue(struct replay_opts *opts) {- struct commit_list *todo_list = NULL;+ struct todo_list todo_list = TODO_LIST_INIT;+ int res; if (!file_exists(get_todo_path(opts))) return continue_single_pick();
@@ -1083,21 +1105,24 @@ static int sequencer_continue(struct replay_opts *opts) } if (index_differs_from("HEAD", 0)) return error_dirty_index(opts);- todo_list = todo_list->next;- return pick_commits(todo_list, opts);+ todo_list.current++;+ res = pick_commits(&todo_list, opts);+ todo_list_release(&todo_list);+ return res;
@@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)if(read_cache_unmerged())returnerror_resolve_conflict(action_name(opts));-/* Different translation strings for cherry-pick and revert */-if(opts->action==REPLAY_PICK)-error(_("Your local changes would be overwritten by cherry-pick."));-else-error(_("Your local changes would be overwritten by revert."));+error(_("Your local changes would be overwritten by %s."),+action_name(opts));
If I understand it correctly, it would make "revert" or "cherry-pick"
untranslated part of error message. You would need to use translation
on the result with "_(action_name(opts))", you would have to mark
todo_command_strings elements for gettext lexicon with N_(...).
I am rather against this change (see also below).
From the first glance I though that there would be no problem with
translation legos / jigsaw it introduces, namely that the "revert"
and "cherry-pick" would require different rest of text:
po/bg.po:msgid "Your local changes would be overwritten by cherry-pick."
po/bg.po-msgstr "Локалните ви промени ще бъдат презаписани при отбирането на подавания."
--
po/bg.po:msgid "Your local changes would be overwritten by revert."
po/bg.po-msgstr "Локалните ви промени ще бъдат презаписани при отмяната на подавания."
po/de.po:msgid "Your local changes would be overwritten by cherry-pick."
po/de.po-msgstr "Ihre lokalen Änderungen würden durch den Cherry-Pick überschrieben werden."
--
po/de.po:msgid "Your local changes would be overwritten by revert."
po/de.po-msgstr "Ihre lokalen Änderungen würden durch den Revert überschrieben werden."
But it turns out that "revert" and "cherry-pick" can be of different
gender:
po/ca.po:msgid "Your local changes would be overwritten by cherry-pick."
po/ca.po-msgstr "Els vostres canvis locals se sobreescriurien pel recull de cireres."
--
po/ca.po:msgid "Your local changes would be overwritten by revert."
po/ca.po-msgstr "Els vostres canvis locals se sobreescriurien per la reversió."
In some cases "revert" and "cherry-pick" are not translated literally
(but compare translation for similar language: po/bg.po, without this):
po/ru.po:msgid "Your local changes would be overwritten by cherry-pick."
po/ru.po-msgstr "Ваши локальные изменение будут перезаписаны отбором лучшего."
--
po/ru.po:msgid "Your local changes would be overwritten by revert."
po/ru.po-msgstr "Ваши локальные изменение будут перезаписаны возвратом коммита."
Similar for (but here one side uses untranslated English term...):
po/vi.po:msgid "Your local changes would be overwritten by cherry-pick."
po/vi.po-msgstr "Các thay đổi nội bộ của bạn có thể bị ghi đè bởi lệnh cherry-pick."
--
po/vi.po:msgid "Your local changes would be overwritten by revert."
po/vi.po-msgstr "Các thay đổi nội bộ của bạn có thể bị ghi đè bởi lệnh hoàn nguyên."
For some I don't know which is the case:
po/zh_CN.po:msgid "Your local changes would be overwritten by cherry-pick."
po/zh_CN.po-msgstr "您的本地修改将被拣选操作覆盖。"
--
po/zh_CN.po:msgid "Your local changes would be overwritten by revert."
po/zh_CN.po-msgstr "您的本地修改将被还原操作覆盖。"
Unless we want to require to use English terms:
po/sv.po:msgid "Your local changes would be overwritten by cherry-pick."
po/sv.po-msgstr "Dina lokala ändringar skulle skrivas över av \"cherry-pick\"."
--
po/sv.po:msgid "Your local changes would be overwritten by revert."
po/sv.po-msgstr "Dina lokala ändringar skulle skrivas över av \"revert\"."
if (advice_commit_before_merge)
advise(_("Commit your changes or stash them to proceed."));
From: Jakub Narębski <hidden> Date: 2016-08-31 18:24:27
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
The subcommands are used exactly once, at the very beginning of
sequencer_pick_revisions(), to determine what to do. This is an
unnecessary level of indirection: we can simply call the correct
function to begin with. So let's do that.
Looks good. Parsing is moved from parse_args(), now unnecessary,
to the new run_sequencer(). Which also picked up dispatch from
sequencer_pick_revisions() - that sometimes didn't pick revisions :-o.
"All problems in computer science can be solved by another level
of indirection, except of course for the problem of too many
indirections." -- David John Wheeler
While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).
This perhaps should be moved to a separate patch, but I guess
there is a reason behind "while at it".
Also subcommand functions no longer are local to sequencer.c
From: Johannes Schindelin <hidden> Date: 2016-08-31 18:37:18
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
quoted
The sequencer was introduced to make the cherry-pick and revert
functionality available as library function, with the original idea
being to extend the sequencer to also implement the rebase -i
functionality.
The test to ensure that all of the commands in the script are identical
to the overall operation does not mesh well with that.
Actually the question is what does the test that got removed in this
commit actually check. Is it high-level sanity check that todo list
for git-cherry-pick contains only 'pick', and for git-revert contains
only 'revert'?
It might have been that at some stage.
But should we really check that? Or should we check the *effects*?
I am of the opinion that overzealous checking of certain implementation
details is something to be avoided.
quoted
Therefore let's just get rid of the test that wants to verify that this
limitation is still in place, in preparation for the upcoming work to
teach the sequencer to do rebase -i's work.
Is it "upcoming work" as in one of the patches in this series?
If so, which patch?
I left this a little vague, didn't I? ;-)
The problem is that the `git-rebase-todo` most definitely does *not* want
to be restricted to a single command.
So if you must have a patch that disagrees with this overzealous check,
the "revamp todo parsing" one is probably the first. But it is better to
think of this at a higher level than just patches: it is wrong to limit
the todo script to contain only identical commands.
Hmmm... the description is somewhat lacking (especially compared to
the rest of test), anyway.
BTW. we should probably rename 'malformed instruction sheet 2'
to 'malformed instruction sheet' if there are no further such
tests after this removal, isn't it?
No, we cannot rename it after this patch because the patch removes it ;-)
(It is not a file name but really a label for a test case.)
Thanks for the review,
Johannes
From: Jakub Narębski <hidden> Date: 2016-08-31 18:38:05
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
The `git-rebase-todo` file contains a list of commands. Most of those
commands have the form
<verb> <sha1> <oneline>
The <oneline> is displayed primarily for the user's convenience, as
rebase -i really interprets only the <verb> <sha1> part. However, there
are *some* places in interactive rebase where the <oneline> is used to
display messages, e.g. for reporting at which commit we stopped.
So let's just remember it when parsing the todo file; we keep a copy of
the entire todo file anyway (to write out the new `done` and
`git-rebase-todo` file just before processing each command), so all we
need to do is remember the begin and end offsets.
Actually what we remember is pointer and length, or begin offset and length,
not offset and offset.
Signed-off-by: Johannes Schindelin <redacted>
Nice, I'll see how it is used later (and in which commit in series).
From: Jakub Narębski <hidden> Date: 2016-08-31 18:47:20
Hello Johannes,
W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
On Wed, 31 Aug 2016, Jakub Narębski wrote:
quoted
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
I am of the opinion that overzealous checking of certain implementation
details is something to be avoided.
I agree.
quoted
quoted
Therefore let's just get rid of the test that wants to verify that this
limitation is still in place, in preparation for the upcoming work to
teach the sequencer to do rebase -i's work.
Is it "upcoming work" as in one of the patches in this series?
If so, which patch?
I left this a little vague, didn't I? ;-)
The problem is that the `git-rebase-todo` most definitely does *not* want
to be restricted to a single command.
So if you must have a patch that disagrees with this overzealous check,
the "revamp todo parsing" one is probably the first. But it is better to
think of this at a higher level than just patches: it is wrong to limit
the todo script to contain only identical commands.
I see. Right.
I wonder: would 'git cherry-pick --continue' be able to finish
'git revert', and vice versa, then? Or 'git sequencer --continue'?
Hmmm... the description is somewhat lacking (especially compared to
the rest of test), anyway.
BTW. we should probably rename 'malformed instruction sheet 2'
to 'malformed instruction sheet' if there are no further such
tests after this removal, isn't it?
No, we cannot rename it after this patch because the patch removes it ;-)
(It is not a file name but really a label for a test case.)
Ooops. What I wanted to say that after removing the test case named
'malformed instruction sheet 2' we should also rename *earlier* test
case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
as it is now the only 'malformed instruction sheet *' test case.
From: Jakub Narębski <hidden> Date: 2016-08-31 20:10:50
Hello Johannes,
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
The rebase command sports a `--gpg-sign` option that is heeded by the
interactive rebase.
Should it be "sports" or "supports"?
quoted hunk
This patch teaches the sequencer that trick, as part of the bigger
effort to make the sequencer the work horse of the interactive rebase.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 48 +++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 43 insertions(+), 5 deletions(-)
I know it is not your fault, but I wonder why this file uses
snake_case_name, while all other use kebab-case-names. That is,
why it is gpg_sign_opt and not gpg-sign-opt.
quoted hunk
/* We will introduce the 'interactive rebase' mode later */
#define IS_REBASE_I() 0
All right, this function is quite clear.
Sidenote: it's a pity api-quote.txt is just a placeholder for proper
documentation (including sq_quotef()). I also wonder why it is not
named sq_quotef_buf() or strbuf_addf_sq().
@@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts, if (IS_REBASE_I()) { env = read_author_script();- if (!env)+ if (!env) {+ const char *gpg_opt = gpg_sign_opt_quoted(opts);+ return error("You have staged changes in your working " "tree. If these changes are meant to be\n" "squashed into the previous commit, run:\n\n"- " git commit --amend $gpg_sign_opt_quoted\n\n"
How did this get expanded by error(), and why we want to replace
it if it works?
+ " git commit --amend %s\n\n"
"If they are meant to go into a new commit, "
"run:\n\n"
- " git commit $gpg_sign_opt_quoted\n\n"
+ " git commit %s\n\n"
"In both case, once you're done, continue "
"with:\n\n"
- " git rebase --continue\n");
+ " git rebase --continue\n", gpg_opt, gpg_opt);
Instead of passing option twice, why not make use of %1$s (arg reordering),
that is
+ " git commit --amend %1$s\n\n"
[...]
+ " git commit %1$s\n\n"
+ }
So shell quoting is required only for error output.
quoted hunk
}
argv_array_init(&array);
@@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char *value, void *data) static int read_populate_opts(struct replay_opts *opts) {- if (IS_REBASE_I())+ if (IS_REBASE_I()) {+ struct strbuf buf = STRBUF_INIT;++ if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {+ if (buf.len && buf.buf[buf.len - 1] == '\n') {+ if (--buf.len &&+ buf.buf[buf.len - 1] == '\r')+ buf.len--;+ buf.buf[buf.len] = '\0';+ }
Isn't there some strbuf_chomp() / strbuf_strip_eof() function?
Though as strbuf_getline() uses something similar...
+
+ if (!starts_with(buf.buf, "-S"))
+ strbuf_reset(&buf);
Should we signal that there was problem with a file contents?
From: Jakub Narębski <hidden> Date: 2016-08-31 20:56:29
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
In the upcoming commits, we will implement more and more of rebase
-i's functionality. One particular feature of the commands to come is
that some of them allow editing the commit message while others don't,
i.e. we cannot define in the replay_opts whether the commit message
should be edited or not.
It's a nice, pretty and self contained refactoring step. Small
enough that it is easy to review.
I would like to have in the commit message that it is sequencer_commit()
function that needs to rely on new parameter, instead of on a property
of command (of its replay_opts). And that currently it simply passes
the buck to caller, which uses opts->edit, but in the future the
caller that is rebase -i would use todo_item and replay_opts based
expression.
From: Jakub Narębski <hidden> Date: 2016-08-31 21:09:27
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
This teaches the sequencer_commit() function to take an argument that
will allow us to implement "todo" commands that need to amend the commit
messages ("fixup", "squash" and "reword").
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 6 ++++--
sequencer.h | 2 +-
2 files changed, 5 insertions(+), 3 deletions(-)
Nice and small addition of a new feature, a scaffolding for implementing
rebase -i using the sequencer.
From: Stefan Beller <hidden> Date: 2016-08-31 23:03:46
On Wed, Aug 31, 2016 at 10:29 AM, Jakub Narębski [off-list ref] wrote:
BTW. perhaps we would be able to continue with 'git continue', regardless
of what we have started with, I wonder...
git continue as a shorthand for `git <relevant-cmd> --continue` sounds great.
If we were to introduce that, I think we would need to unify the rest
as well then, e.g.
Both revert as well as cherry-pick have --quit as well as --abort,
but rebase doesn't have --quit documented, but instead an additional
--skip and --edit-todo
Would we pull these all up as a top level command? (That sounds not so
great to me)
From: Johannes Schindelin <hidden> Date: 2016-09-01 06:36:18
Hi Kuba and Stefan,
On Wed, 31 Aug 2016, Stefan Beller wrote:
On Wed, Aug 31, 2016 at 10:29 AM, Jakub Narębski [off-list ref] wrote:
quoted
BTW. perhaps we would be able to continue with 'git continue', regardless
of what we have started with, I wonder...
git continue as a shorthand for `git <relevant-cmd> --continue` sounds great.
Before we get ahead of ourselves:
1) this has nothing to do with the patch series at hand, and
2) if we were to introduce `git continue`, we would need to think long and
hard about the following issues:
I) are there potentially ambiguous <relevant-cmd>s that the user
may want to continue?
II) what about options? You can say `git rebase --continue
--no-ff`, for example, but not `git cherry-pick --continue
--no-ff`...
III) Would it not be confusing to have a subcommand `continue`
that does *not* serve a *single* purpose? It's kinda flying
into the face of the Unix philosophy.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-09-01 07:50:13
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
quoted
When we came up with the "sequencer" idea, we really wanted to have
kind of a plumbing equivalent of the interactive rebase. Hence the
choice of words: the "todo" script, a "pick", etc.
However, when it came time to implement the entire shebang, somehow this
idea got lost and the sequencer was used as working horse for
cherry-pick and revert instead. So as not to interfere with the
interactive rebase, it even uses a separate directory to store its
state.
Furthermore, it also is stupidly strict about the "todo" script it
accepts: while it parses commands in a way that was *designed* to be
similar to the interactive rebase, it then goes on to *error out* if the
commands disagree with the overall action (cherry-pick or revert).
Does this mean that after the change you would be able to continue
"git revert" with "git cherry-pick --continue", and vice versa? Or that
it would be possible for git-cherry-pick to do reverts (e.g. with ^<rev>)?
I guess that I allow that now. Is it harmful? I dunno.
quoted
Let's just bite the bullet and rewrite the entire parser; the code now
becomes not only more elegant: it allows us to go on and teach the
sequencer how to parse *true* "todo" scripts as used by the interactive
rebase itself. In a way, the sequencer is about to grow up to do its
older brother's job. Better.
Sidenote: this is not your fault, but Git doesn't do a good job on
changes which are mostly rewrites, trying to match stray '}' and the
like in generated diff. I wonder if existing diff heuristic options
could help here.
I guess --patience would have helped. Or Michael's upcoming
diff-heuristics.
quoted
While at it, do not stop at the first problem, but list *all* of the
problems. This helps the user by allowing to address all issues in
one go rather than going back and forth until the todo list is valid.
That is also a good change, though I wonder how often users need
to worry about this outside interactive rebase case. If it is
preparation for rebase -i, where instruction list is written by
prone to errors human, it would be nice to have this information
in the commit message.
Do we have a naming convention for enums elements? Or are we explicitly
making enums and #defines interchangeable? I wonder...
...uh, I see we don't have naming convention, but all caps snake-case
names dominate:
$ git grep -A2 'enum .* {'
[...]
diff.h:enum color_diff {
diff.h- DIFF_RESET = 0,
diff.h- DIFF_CONTEXT = 1,
--
dir.c:enum path_treatment {
dir.c- path_none = 0,
dir.c- path_recurse,
--
Shouldn't we say 'TODO_PICK = 0' explicitly, though?
It's a bit pity that we cannot use designated inits, and hanging comma,
(from ISO C99 standard). That is:
+static const char *todo_command_strings[] = {
+ [TODO_PICK] = "pick",
+ [TODO_REVERT] = "revert",
+};
I agree, it is a pity. I could do something like I did in fsck.c:
#define FOREACH_TODO_COMMAND(FUNC) \
FUNC(PICK, "pick") \
FUNC(REVERT, "revert")
#define COMMAND_ID(id, string) TODO_##id,
enum todo_command {
FOREACH_TODO_COMMAND(COMMAND_ID)
TODO_END
};
#undef COMMAND_ID
#define COMMAND_ID(id, string) string,
static const char *todo_command_string[] = {
FOREACH_TODO_COMMAND(COMMAND_ID)
NULL
};
#undef COMMAND_ID
However, this is not even readable, let alone any other type of an
improvement. So I won't.
From here on changes are about
s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
Do we still need opts->action, or it is just needed less,
and it is 'todo' instruction that decides about command
(as it should)?
We need opts->action. For example, the state directory changes depending
on it: REPLAY_INTERACTIVE_REBASE stores its stuff in
git_path("rebase-merge").
There is lots more behavior that also changes depending on opts->action.
quoted
[...]
if (res) {
- error(opts->action == REPLAY_REVERT
+ error(command == TODO_REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV),
And here those changes end.
s/opts->action == REPLAY_\(PICK\|REVERT\)/command == TODO_\1/
I wonder if Coccinelle / Undebt would help here; or would simple
sed or query-and-replace-regexp be enough...
I did this by hand, to verify that I did nothing idiotic.
So there should be s/commit_list [*]{1,2}todo_list/todo_list *todo_list/
from here on?
Almost, but not quite.
Hmmm... commit_list is, as defined in commit.h, a linked list.
That is the most prominent reason why the rest is not a mindless
conversion from commit_list to todo_list.
And we need todo_list as an array, because we need to be able to peek, or
even move, backwards from the current command.
Here todo_list uses growable array implementation of list. Which
is I guess better on current CPU architecture, with slow memory,
limited-size caches, and adjacency prefetching.
That is not the reason that an array is used here. The array allows us
much more flexibility.
One of the major performance improvements will come at the very end, for
example: the reordering of the fixup!/squash! lines. And that would be a
*major* pain to do if the todo_list were still a linked list.
Same as with other patches in this series, it would be enough to
+#define TODO_LIST_INIT { STRBUF_INIT }
As it happens, after Hannes' comment about REPLAY_OPTIONS_INIT, I already
had changed TODO_LIST_INIT as indicated. I just had no time to send out
another iteration (besides, I wanted to give the sequencer-i patch series
more visibility).
Errr... I don't quite understand the name of this function.
What are you appending here to the todo_list?
A new item.
Compare string_list_append() and string_list_append_nodup(),
where the second parameter is item to append.
Yes, that is correct. In the case of a todo_item, things are a lot more
complicated, though. Some of the values have to be determined tediously
(such as the offset and length of the oneline after the "pick <oid>"
command). I just put those values directly into the newly allocated item,
is all.
Because it makes no sense to return a commit here because not all commands
are about commits (think rebase -i's `exec`). It makes tons of sense to
return an error condition, though.
Why now struct todo_item is first when struct replay_opts was last?
Those play very, very different roles.
The opts parameter used to provide parse_insn_line() with enough
information to complain loudly when the overall command was not identical
to the parsed command.
The item parameter is a receptacle for the parsed data. It will contain
the pointer to the commit that was previously returned, if any. But it
will also contain much more information, such as the command, the oneline,
the offset in the buffer, etc etc
So "opts" was an "in" parameter while "item" is an "out" one. Apples and
oranges.
quoted
+ for (i = 0; i < ARRAY_SIZE(todo_command_strings); i++)
+ if (skip_prefix(bol, todo_command_strings[i], &bol)) {
skip_prefix() is such a nice abstraction...
quoted
+ item->command = i;
+ break;
+ }
Nice. Replacing if-elsif chain with loop.
I guess any hashmap would be serious overkill, as there are and would be
only a few actions possible.
If at all, we should use a trie here. But as you said: overkill to the
max.
quoted
+ if (i >= ARRAY_SIZE(todo_command_strings))
+ return -1;
/* Eat up extra spaces/ tabs before object name */
padding = strspn(bol, " \t");
if (!padding)
- return NULL;
+ return -1;
bol += padding;
- end_of_object_name = bol + strcspn(bol, " \t\n");
+ end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
Why is this cast needed?
Because bol is a "const char *" and we need to put "NUL" temporarily to
*end_of_object_name:
Technically, this would have made a fine excuse to teach get_sha1() a mode
where it expects a length parameter instead of relying on a NUL-terminated
string.
Practically, such fine excuses cost me months in this rebase--helper
project already, and I need to protect my time better.
quoted
- /*
- * Verify that the action matches up with the one in
- * opts; we don't support arbitrary instructions
- */
- if (action != opts->action) {
- if (action == REPLAY_REVERT)
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot revert during another revert.")
Errr... could the above ever happen? Namely
action != opts->action && action == REPLAY_REVERT && opts->action == REPLAY_REVERT
Surely not.
Your reply pointed to the very circumstance when this may happen: `git
cherry-pick --continue` after an interrupted `git revert`.
But then, I remove that code here, so I should not try to defend it.
quoted
- : _("Cannot revert during a cherry-pick."));
- else
- error((opts->action == REPLAY_REVERT)
- ? _("Cannot cherry-pick during a revert.")
- : _("Cannot cherry-pick during another cherry-pick."));
- return NULL;
- }
Anyway, while it is / would be a good idea to prevent starting any
sequencer-based command (cherry-pick, revert, soon rebase -i) when
other command is in progress (cherry-pick, revert, soon rebase -i).
That is, if cherry-pick / revert waits for user action, you cannot
run another cherry-pick or revert.
Which I guess the above code was not about...
It was about that, though.
It went about it in a pretty round-about way: opts->action comes from the
name of the command ("was I called as `git revert` or `git cherry-pick`?")
and action comes from the todo script, which was assumed to be written by
a previous run of the sequencer, using the then-current value of
opts->action.
So it wrote that command into *every single line* of the todo script, *for
the sole purpose* of verifying that it was the same action when running
via --continue.
As I said earlier, I would not complain at all if an interrupted `git
revert` could be continued via `git cherry-pick --continue`.
If that is not desirable, I can reintroduce that overzealous check, but
that will have to wait until after v2.10.0. And it would require an
argument that convinces me.
quoted
+ item = append_todo(todo_list);
A better name, in my personal option, would be
+ item = todo_list_next(todo_list);
Or todo_next(todo_list).
That sounds more like a function that performs the next command in the
todo_list.
While I agree that naming is hard, I still think that `append_todo()` with
the todo_list as single parameter and returning a todo_item is pretty much
self-explanatory: it appends a new item to the todo_list and returns a
pointer to it.
quoted
+ item->offset_in_buf = p - todo_list->buf.buf;
+ if (parse_insn_line(item, p, eol)) {
+ error("Invalid line: %.*s", (int)(eol - p), p);
This error message should, I think, be also translatable:
+ error(_("Invalid line: %.*s"), (int)(eol - p), p);
quoted
+ res |= error(_("Could not parse line %d."), i);
Sure. In the meantime, I consolidated those two error()s into one, and now
I also marked it translatable.
BTW. would be we able to show where exactly there was problem parsing,
that is at which character in line? Or is it something for the future?
Maybe for the future.
quoted
-static int read_populate_todo(struct commit_list **todo_list,
+static int read_populate_todo(struct todo_list *todo_list,
struct replay_opts *opts)
{
const char *todo_file = get_todo_path(opts);
If I understand it correctly, replay_opts is used only to find out
correct todo_file, isn't it?
Probably. Maybe also to make certain code paths conditional on rebase -i
mode. Maybe also to figure out whether we run in verbose mode in the
future. Or something.
Think of this `read_populate_todo()` function more as if it were a method
of the "replay class", and the "opts" parameter is kind of "self" or
"this" or whatever it is called in your favorite object-oriented language.
quoted
- if (strbuf_read(&buf, fd, 0) < 0) {
+ if (strbuf_read(&todo_list->buf, fd, 0) < 0) {
close(fd);
- strbuf_release(&buf);
A question: when is todo_list->buf released?
Why, I am glad you asked! It is released in todo_list_release(), called at
the end e.g. of sequencer_continue().
I see that you are creating todo file contents while walking revision list,
something that was left for later in current / previous implementation
of the sequencer...
Not really. This function was always about generating a todo_list. It just
did not format it yet.
With the change of keeping the original formatting of the todo script
instead of re-formatting it in save_todo(), this function now has to
format the todo_list itself.
Wouldn't it be simpler to use
+ todo_command_strings[command],
Also, this string does not change during the loop, though I guess
compiler should be able to optimize it.
...Did format of the 'todo' file changed? And if yes, was it in backward
compatible way, so that "git revert" or "git cherry-pick" started with
old version of Git can be continued with new version, and what is also
important (for somebody who sometimes uses system-installed Git, and
sometimes user-compiled one) the reverse: started with new, continued
with old?
The old format and the new format are compatible. In fact, sequencer's
format was based on rebase -i's format (which makes it all the more
surprising how much the processing deviated).
quoted
@@ -964,30 +990,24 @@ static int sequencer_rollback(struct replay_opts *opts) return -1; }-static int save_todo(struct commit_list *todo_list, struct replay_opts *opts)+static int save_todo(struct todo_list *todo_list, struct replay_opts *opts) { static struct lock_file todo_lock;- struct strbuf buf = STRBUF_INIT;- int fd;+ const char *todo_path = get_todo_path(opts);+ int next = todo_list->current, offset, fd;
The "next = todo_list->current" looks a bit strange.
Depending whether we need rebase -i processing or revert/cherry-pick's
slightly different one, the "current" position points to the next one
already...
Also, we do not change todo_list->current, we use it in one place, so it
can be used directly without help of temporary / helper variable. But
that is just my personal opinion.
No, it has nothing to do with opinion. It prepares the code to keep it
readable even when REPLAY_INTERACTIVE_REBASE is introduced.
Also, from 'next', 'offset' and 'fd', all those are different uses of
int: the index (int, rarely size_t), the offset in string (formally
ptrdiff_t, or size_t, but usually int), and the file descriptor. I
think from those the file descriptor could be kept in separate line; it
would help diff to be more readable. But this is fairly marginal
nitpicking, and a matter of personal opinion.
Right. At this point, I am really much more concerned about correctness of
code than discussing personal preferences.
and this should be done in one of earlier patches, isn't it?
No. I deliberately skipped save_todo() from "future-proofing" as I planned
to rewrite it anyway. There is no point in future-proofing something you
are going to toss in a minute.
quoted
- if (format_todo(&buf, todo_list, opts) < 0) {
- strbuf_release(&buf);
- return error(_("Could not format %s."), git_path_todo_file());
Can we still get this error? Could we get this error anyway,
and under what conditions?
No. We keep the original formatting. Keeping it cannot possibly result in
a formatting error.
quoted
- }
- if (write_in_full(fd, buf.buf, buf.len) < 0) {
- strbuf_release(&buf);
- return error_errno(_("Could not write to %s"),
- git_path_todo_file());
- }
+ offset = next < todo_list->nr ?
+ todo_list->items[next].offset_in_buf : todo_list->buf.len;
+ if (write_in_full(fd, todo_list->buf.buf + offset,
+ todo_list->buf.len - offset) < 0)
+ return error(_("Could not write to %s (%s)"),
+ todo_path, strerror(errno));
Ah, so it saves the remaining todo_items on todo_list, not the
whole todo_list... the name does not fully show it.
The name also does not fully show that it will write a "done" file after
the sequencer-i patch series.
quoted
- if (commit_lock_file(&todo_lock) < 0) {
- strbuf_release(&buf);
- return error(_("Error wrapping up %s."), git_path_todo_file());
- }
- strbuf_release(&buf);
+ if (commit_lock_file(&todo_lock) < 0)
+ return error(_("Error wrapping up %s."), todo_path);
Note: this is unrelated change, but we usually put paths in quotes, like this
+ return error(_("Error wrapping up '%s'."), todo_path);
(in this and earlier error message), so that paths containing spaces show
correctly and readably to the user. Though this possibly is not a problem
for this path.
Right.
Also, how user is to understand "wrapping up"?
The same as before: the removed lines already had the error message,
missing the quotes, too.
Don't get me wrong: I am a big fan of consistency, and I wish that Git's
source code had more of it. So I would love to see a patch series that
makes all error messages consistently reporting paths enclosed in single
quotes.
I am also a big fan of the separation of concerns, though. And this patch
series' concern is consistency *with the existing code*.
So I won't change the error message that I inherited at this point.
The ternary conditional operator here translates one enum to other enum,
isn't it?
Well, almost. Please note that the enum will receive a new value in the
sequencer-i patch series. And there is no equivalent todo_command for
REPLAY_INTERACTIVE_REBASE.
Thanks for the review!
Johannes
@@ -232,11 +232,8 @@ static int error_dirty_index(struct replay_opts *opts)if(read_cache_unmerged())returnerror_resolve_conflict(action_name(opts));-/* Different translation strings for cherry-pick and revert */-if(opts->action==REPLAY_PICK)-error(_("Your local changes would be overwritten by cherry-pick."));-else-error(_("Your local changes would be overwritten by revert."));+error(_("Your local changes would be overwritten by %s."),+action_name(opts));
If I understand it correctly, it would make "revert" or "cherry-pick"
untranslated part of error message. You would need to use translation
on the result with "_(action_name(opts))", you would have to mark
todo_command_strings elements for gettext lexicon with N_(...).
I am rather against this change (see also below).
Okay.
Unfortunately, I have to focus on the correctness of the code at the
moment (and Git for Windows does ship *without* translations for the time
being anyway, mostly to save on space, but also because users complained).
So I will take care of this after v2.10.0.
For the record, how is this supposed to be handled, in particular when I
introduce a new action whose action_name(opts) will be "rebase -i"? Do I
really need to repeat myself three times?
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-09-01 07:55:44
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
quoted
While at it, ensure that the subcommands return an error code so that
they do not have to die() all over the place (bad practice for library
functions...).
This perhaps should be moved to a separate patch, but I guess
there is a reason behind "while at it".
Yes. It seemed like the logical thing to do: I already introduce a new
function, why should I shlep over a paradigm I do not want in the end?
Also subcommand functions no longer are local to sequencer.c
They never were. All you had to do was to set a field and run the global
function.
The real problem there was that the different local functions needed
different parameters, and the round-about way to set those parameters as
fields in a struct and then call a global function with that struct just
makes it impossible to have compile-time safety.
Ciao,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-09-01 08:02:07
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 31.08.2016 o 20:36, Johannes Schindelin pisze:
I wonder: would 'git cherry-pick --continue' be able to finish
'git revert', and vice versa, then? Or 'git sequencer --continue'?
I just tested this, via
diff --git a/t/t3510-cherry-pick-sequence.sh
b/t/t3510-cherry-pick-sequence.sh
index 96c7640..085d8bc 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -55,7 +55,7 @@ test_expect_success 'cherry-pick
mid-cherry-pick-sequence' '
git checkout HEAD foo &&
git cherry-pick base &&
git cherry-pick picked &&
- git cherry-pick --continue &&
+ git revert --continue &&
git diff --exit-code anotherpick
(Danger! Whitespace corrupted!!!)
It appears that this passes now.
Probably `git sequencer --continue` would work, too, if there was a `git
sequencer`. :0)
quoted
On Wed, 31 Aug 2016, Jakub Narębski wrote:
quoted
W dniu 29.08.2016 o 10:04, Johannes Schindelin pisze:
Hmmm... the description is somewhat lacking (especially compared to
the rest of test), anyway.
BTW. we should probably rename 'malformed instruction sheet 2'
to 'malformed instruction sheet' if there are no further such
tests after this removal, isn't it?
No, we cannot rename it after this patch because the patch removes it ;-)
(It is not a file name but really a label for a test case.)
Ooops. What I wanted to say that after removing the test case named
'malformed instruction sheet 2' we should also rename *earlier* test
case from 'malformed instruction sheet 1' to 'malformed instruction sheet',
as it is now the only 'malformed instruction sheet *' test case.
Actually, you know, I completely missed the fact that there was a
"malformed instruction sheet 3". I renumbered it.
Thanks,
Dscho
From: Johannes Schindelin <hidden> Date: 2016-09-01 08:46:24
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:05, Johannes Schindelin pisze:
quoted
The `git-rebase-todo` file contains a list of commands. Most of those
commands have the form
<verb> <sha1> <oneline>
The <oneline> is displayed primarily for the user's convenience, as
rebase -i really interprets only the <verb> <sha1> part. However, there
are *some* places in interactive rebase where the <oneline> is used to
display messages, e.g. for reporting at which commit we stopped.
So let's just remember it when parsing the todo file; we keep a copy of
the entire todo file anyway (to write out the new `done` and
`git-rebase-todo` file just before processing each command), so all we
need to do is remember the begin and end offsets.
Actually what we remember is pointer and length, or begin offset and length,
not offset and offset.
Does it work correctly for line without <oneline>, that is
<verb> <sha1>
I think it does, but I not entirely sure.
It does work correctly: in the example, *end_of_object_name would be '\n',
and strspn(end_of_object_name, " \t") would return 0.
Thanks for the review!
Dscho
From: Jakub Narębski <hidden> Date: 2016-09-01 10:31:39
Hello Johannes,
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
The sequencer_commit() function already knows how to amend commits, and
with this new option, it can also clean up commit messages (i.e. strip
out commented lines). This is needed to implement rebase -i's 'fixup'
and 'squash' commands as sequencer commands.
Signed-off-by: Johannes Schindelin <redacted>
---
sequencer.c | 10 +++++++---
sequencer.h | 3 ++-
2 files changed, 9 insertions(+), 4 deletions(-)
This looks like nice little piece of enhancement, building scaffolding
for sequencer-izing interactive rebase bit by bit.
The calling convention begins to look unwieldy, but we have only
a single such callsite, and there are quite a bit callsites in
Git code that have similar API ("git grep ', 0, 0' -- '*.c'").
So we don't need to think about alternatives. Yet.
It's a pity that emulation of named parameters in C requires
relying on designated inits from C99
typedef struct {
double pressure, moles, temp;
} ideal_struct;
#define ideal_pressure(...) ideal_pressure_base((ideal_struct){.pressure=1, \
.moles=1, .temp=273.15, __VA_ARGS__})
double ideal_pressure_base(ideal_struct in)
{
return 8.314 * in.moles*in.temp/in.pressure;
}
... ideal_pressure(.moles=2, .temp=373.15) ...
From: Jakub Narębski <hidden> Date: 2016-09-01 10:50:43
Hello Johannes,
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
Subject: [PATCH 21/22] sequencer: left-trim the lines read from the script
In the subject, it should probably be without "the", as "lines"
are plural.
s/left-trim the lines/left-trim lines/
Interactive rebase's scripts may be indented; We need to handle this
case, too, now that we prepare the sequencer to process interactive
rebases.
s/; We need/; we need/
Nice little bit of scaffolding for sequencer-izing rebase -i.
From: Jakub Narębski <hidden> Date: 2016-09-01 11:10:43
Hello Johannes,
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
The write_message() function safely writes an strbuf to a file.
Sometimes this is inconvenient, though: the text to be written may not
be stored in a strbuf, or the strbuf should not be released after
writing.
By "this" you mean "using strbuf", isn't it? It is not very obvious,
and I think it would be better to say it explicitly.
Let's allow for such use cases by refactoring write_message() to allow
for a convenience function write_file_gently(). As some of the upcoming
callers of that new function will want to append a newline character,
let's just add a flag for that, too.
This paragraph feels a bit convoluted.
As I understand it, you refactor "safely writing string to a file"
into write_with_lock_file(), and make write_message() use it. The
new function makes it easy to create new convenience function
write_file_gently(); as some of the upcoming callers of this new
function would want to append a newline character, add a flag for
it in write_file_gently(), and thus in write_with_lock_file().
Isn't it better / easier to understand?
@@ -239,22 +239,37 @@ static void print_advice(int show_hint, struct replay_opts *opts)}}-staticintwrite_message(structstrbuf*msgbuf,constchar*filename)+staticintwrite_with_lock_file(constchar*filename,+constvoid*buf,size_tlen,intappend_eol){staticstructlock_filemsg_file;intmsg_fd=hold_lock_file_for_update(&msg_file,filename,0);if(msg_fd<0)returnerror_errno(_("Could not lock '%s'"),filename);-if(write_in_full(msg_fd,msgbuf->buf,msgbuf->len)<0)+if(write_in_full(msg_fd,buf,len)<0)returnerror_errno(_("Could not write to %s"),filename);
You could have, for consistency, add quotes around filename (see previous
error_errno callsite), *while at it*:
return error_errno(_("Could not write to '%s'"), filename);
- strbuf_release(msgbuf);
+ if (append_eol && write(msg_fd, "\n", 1) < 0)
+ return error_errno(_("Could not write eol to %s"), filename);
Same here, and it wouldn't even be 'while at it'
+ return error_errno(_("Could not write eol to '%s'"), filename);
if (commit_lock_file(&msg_file) < 0)
return error(_("Error wrapping up %s."), filename);
Another "while at it"... though the one that can be safely postponed
(well, the make message easier to understand part, not the quote
filename part):
return error(_("Error wrapping up writing to '%s'."), filename);
return 0;
}
+static int write_message(struct strbuf *msgbuf, const char *filename)
+{
+ int res = write_with_lock_file(filename, msgbuf->buf, msgbuf->len, 0);
+ strbuf_release(msgbuf);
+ return res;
+}
Nice.
+
+static int write_file_gently(const char *filename,
+ const char *text, int append_eol)
+{
+ return write_with_lock_file(filename, text, strlen(text), append_eol);
+}
Nice. And it is static function, so we don't need to come up
with a better function name (to describe its function better).
+
/*
* Reads a file that was presumably written by a shell script, i.e.
* with an end-of-line marker that needs to be stripped.
And thus we got to the last patch in this series. I have skipped
patches that already got reviewed; are there some that you would
like to have second review of? Is there patch series that needs
to be applied earlier that needs a review?
P.S. I'll try to respond to your comments later today.
Regards,
--
Jakub Narębski
From: Johannes Schindelin <hidden> Date: 2016-09-01 13:33:25
Hi Kuba,
On Wed, 31 Aug 2016, Jakub Narębski wrote:
W dniu 29.08.2016 o 10:06, Johannes Schindelin pisze:
quoted
The rebase command sports a `--gpg-sign` option that is heeded by the
interactive rebase.
Should it be "sports" or "supports"?
Funny. I got a PR last week that wanted to fix a similar expression.
I really meant "to sport", as in "To display; to have as a notable
feature.". See https://en.wiktionary.org/wiki/sport#Verb
I know it is not your fault, but I wonder why this file uses
snake_case_name, while all other use kebab-case-names. That is,
why it is gpg_sign_opt and not gpg-sign-opt.
Yes, you are correct: it is not my fault ;-)
Sidenote: it's a pity api-quote.txt is just a placeholder for proper
documentation (including sq_quotef()). I also wonder why it is not
named sq_quotef_buf() or strbuf_addf_sq().
Heh. I did not even bother to check the documentation, it is my long-time
habit to dive right into the code.
quoted
@@ -471,17 +487,20 @@ int sequencer_commit(const char *defmsg, struct replay_opts *opts, if (IS_REBASE_I()) { env = read_author_script();- if (!env)+ if (!env) {+ const char *gpg_opt = gpg_sign_opt_quoted(opts);+ return error("You have staged changes in your working " "tree. If these changes are meant to be\n" "squashed into the previous commit, run:\n\n"- " git commit --amend $gpg_sign_opt_quoted\n\n"
How did this get expanded by error(), and why we want to replace
it if it works?
It did not work. It was a place-holder waiting for this patch ;-)
quoted
+ " git commit --amend %s\n\n"
"If they are meant to go into a new commit, "
"run:\n\n"
- " git commit $gpg_sign_opt_quoted\n\n"
+ " git commit %s\n\n"
"In both case, once you're done, continue "
"with:\n\n"
- " git rebase --continue\n");
+ " git rebase --continue\n", gpg_opt, gpg_opt);
Instead of passing option twice, why not make use of %1$s (arg reordering),
that is
+ " git commit --amend %1$s\n\n"
[...]
+ " git commit %1$s\n\n"
Cute. But would this not drive the l10ners insane?
So shell quoting is required only for error output.
Indeed.
quoted
@@ -955,8 +974,27 @@ static int populate_opts_cb(const char *key, const char *value, void *data) static int read_populate_opts(struct replay_opts *opts) {- if (IS_REBASE_I())+ if (IS_REBASE_I()) {+ struct strbuf buf = STRBUF_INIT;++ if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) {+ if (buf.len && buf.buf[buf.len - 1] == '\n') {+ if (--buf.len &&+ buf.buf[buf.len - 1] == '\r')+ buf.len--;+ buf.buf[buf.len] = '\0';+ }
Isn't there some strbuf_chomp() / strbuf_strip_eof() function?
Though as strbuf_getline() uses something similar...
Even worse. read_oneliner() *already* does that. I just forgot to delete
this code when I introduced and used read_oneliner().
Thanks.
quoted
+ if (!starts_with(buf.buf, "-S"))
+ strbuf_reset(&buf);
Should we signal that there was problem with a file contents?
Maybe. But probably not: this file is written by git-rebase itself. I
merely safe-guarded against empty files here.
Wouldn't we leak 2 characters that got skipped? Maybe xstrdup would
be better (if it is leaked, and not reattached)?
We do not leak anything because I changed the code locally already to use
sequencer_entrust() (I guess in response to an earlier of your comments).
Ciao,
Dscho