This patch series depends on pt/pull-builtin for OPT_PASSTHRU_ARGV() and
argv_array_pushv().
This is a re-roll of [WIP v3]. Thanks Junio and Stefan for the reviews last
round.
The biggest addition this round would be the support for git-rebase. Here's
a small unscientific benchmark that rebases 50 patches:
git init &&
echo initial >file &&
git add file &&
git commit -m initial &&
git tag initial &&
for x in $(seq 50)
do
echo $x >>file &&
git commit -a -m $x
done &&
git checkout -b onto-rebase initial &&
git commit --allow-empty -mempty &&
time git rebase -q --onto onto-rebase initial master
With master:
1.53s, 1.55s, 1.17s, 1.52s, 1.22s. Avg: ~1.40s
With master + this patch series:
0.22s, 0.22s, 0.18s, 0.21s, 0.18s. Avg: ~0.20s
So this is around a 6-7x speedup.
Previous versions:
[WIP v1] http://thread.gmane.org/gmane.comp.version-control.git/270048
[WIP v2] http://thread.gmane.org/gmane.comp.version-control.git/271381
git-am is a commonly used command for applying a series of patches from a
mailbox to the current branch. Currently, it is implemented by the shell script
git-am.sh. However, compared to C, shell scripts have certain deficiencies:
they need to spawn a lot of processes, introduce a lot of dependencies and
cannot take advantage of git's internal caches.
This WIP patch series rewrites git-am.sh into optimized C builtin/am.c, and is
part of my GSoC project to rewrite git-pull and git-am into C builtins[1].
[1] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1
Paul Tan (31):
wrapper: implement xopen()
wrapper: implement xfopen()
am: implement skeletal builtin am
am: implement patch queue mechanism
am: split out mbox/maildir patches with git-mailsplit
am: detect mbox patches
am: extract patch, message and authorship with git-mailinfo
am: apply patch with git-apply
am: commit applied patch
am: refresh the index at start
am: refuse to apply patches if index is dirty
am: implement --resolved/--continue
am: implement --skip
am: implement --abort
am: don't accept patches when there's a session in progress
am: implement quiet option
am: exit with user friendly message on patch failure
am: implement am --signoff
cache-tree: introduce write_index_as_tree()
am: implement 3-way merge
am: --rebasing
am: don't use git-mailinfo if --rebasing
am: handle stray state directory
am: implement -k/--keep, --keep-non-patch
am: implement --[no-]message-id, am.messageid
am: support --keep-cr, am.keepcr
am: implement --[no-]scissors
am: pass git-apply's options to git-apply
am: implement --ignore-date
am: implement --committer-date-is-author-date
am: implement -S/--gpg-sign, commit.gpgsign
Makefile | 1 +
builtin.h | 1 +
builtin/am.c | 1650 +++++++++++++++++++++++++++++++++++++++++++++++++++++
cache-tree.c | 29 +-
cache-tree.h | 1 +
git-compat-util.h | 2 +
git.c | 1 +
wrapper.c | 43 ++
8 files changed, 1716 insertions(+), 12 deletions(-)
create mode 100644 builtin/am.c
--
2.1.4
A common usage pattern of open() is to check if it was successful, and
die() if it was not:
int fd = open(path, O_WRONLY | O_CREAT, 0777);
if (fd < 0)
die_errno(_("Could not open '%s' for writing."), path);
Implement a wrapper function xopen() that does the above so that we can
save a few lines of code, and make the die() messages consistent.
Helped-by: Torsten Bögershausen [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
git-compat-util.h | 1 +
wrapper.c | 25 +++++++++++++++++++++++++
2 files changed, 26 insertions(+)
A common usage pattern of fopen() is to check if it succeeded, and die()
if it failed:
FILE *fp = fopen(path, "w");
if (!fp)
die_errno(_("could not open '%s' for writing"), path);
Implement a wrapper function xfopen() for the above, so that we can save
a few lines of code and make the die() messages consistent.
Signed-off-by: Paul Tan <redacted>
---
git-compat-util.h | 1 +
wrapper.c | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)
@@ -336,6 +336,24 @@ int xdup(int fd)returnret;}+/**+*xfopen()isthesameasfopen(),butitdie()sifthefopen()fails.+*/+FILE*xfopen(constchar*path,constchar*mode)+{+assert(path);+assert(mode);++for(;;){+FILE*fp=fopen(path,mode);+if(fp)+returnfp;+if(errno==EINTR)+continue;+die_errno(_("could not open '%s'"),path);+}+}+FILE*xfdopen(intfd,constchar*mode){FILE*stream=fdopen(fd,mode);
git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.
Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.
Signed-off-by: Paul Tan <redacted>
---
Notes:
v3
* Moved the TODO comment to the previous patch
builtin/am.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 101 insertions(+), 3 deletions(-)
@@ -8,6 +8,12 @@#include"exec_cmd.h"#include"parse-options.h"#include"dir.h"+#include"run-command.h"++enumpatch_format{+PATCH_FORMAT_UNKNOWN=0,+PATCH_FORMAT_MBOX+};structam_state{/* state directory path */
@@ -16,6 +22,9 @@ struct am_state {/* current and last patch numbers, 1-indexed */intcur;intlast;++/* number of digits in patch filename */+intprec;};/**
For the purpose of rewriting git-am.sh into a C builtin, implement a
skeletal builtin/am.c that redirects to $GIT_EXEC_PATH/git-am if the
environment variable _GIT_USE_BUILTIN_AM is not defined. Since in the
Makefile git-am.sh takes precedence over builtin/am.c,
$GIT_EXEC_PATH/git-am will contain the shell script git-am.sh, and thus
this allows us to fall back on the functional git-am.sh when running the
test suite for tests that depend on a working git-am implementation.
Since git-am.sh cannot handle any environment modifications by
setup_git_directory(), "am" has to be declared as NO_SETUP in git.c. On
the other hand, to re-implement git-am.sh in builtin/am.c, we do need to
run all the git dir and work tree setup logic that git.c does for us. As
such, we work around this temporarily by copying the logic in git.c's
run_builtin(), which amounts to:
prefix = setup_git_directory();
trace_repo_setup(prefix);
setup_work_tree();
This redirection should be removed when all the features of git-am.sh
have been re-implemented in builtin/am.c.
Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
Notes:
v3
* Style fixes
* git-am.sh cannot handle the chdir() and GIT_DIR envionment variable
that setup_git_directory() sets, so we work around it by copying the
logic of git.c's run_builtin(), and running it only when we are using
the builtin am.
Makefile | 1 +
builtin.h | 1 +
builtin/am.c | 28 ++++++++++++++++++++++++++++
git.c | 1 +
4 files changed, 31 insertions(+)
create mode 100644 builtin/am.c
Since 15ced75 (git-am foreign patch support: autodetect some patch
formats, 2009-05-27), git-am.sh is able to autodetect mbox, stgit and
mercurial patches through heuristics.
Re-implement support for autodetecting mbox/maildir files.
Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 98 insertions(+)
Implement applying the patch to the index using git-apply.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 56 insertions(+), 1 deletion(-)
@@ -27,6 +27,18 @@ static int is_empty_file(const char *filename)return!st.st_size;}+/**+*Returnsthefirstlineofmsg+*/+staticconstchar*firstline(constchar*msg)+{+staticstructstrbufsb=STRBUF_INIT;++strbuf_reset(&sb);+strbuf_add(&sb,msg,strchrnul(msg,'\n')-msg);+returnsb.buf;+}+enumpatch_format{PATCH_FORMAT_UNKNOWN=0,PATCH_FORMAT_MBOX
@@ -519,6 +531,31 @@ static int parse_patch(struct am_state *state, const char *patch)return0;}+/*+*Appliescurrentpatchwithgit-apply.Returns0onsuccess,-1otherwise.+*/+staticintrun_apply(conststructam_state*state)+{+structchild_processcp=CHILD_PROCESS_INIT;++cp.git_cmd=1;++argv_array_push(&cp.args,"apply");++argv_array_push(&cp.args,"--index");++argv_array_push(&cp.args,am_path(state,"patch"));++if(run_command(&cp))+return-1;++/* Reload index as git-apply will have modified it. */+discard_cache();+read_cache();++return0;+}+/***Appliesallqueuedpatches.*/
@@ -536,7 +573,25 @@ static void am_run(struct am_state *state)write_author_script(state);write_file(am_path(state,"final-commit"),1,"%s",state->msg.buf);-/* TODO: Patch application not implemented yet */+printf_ln(_("Applying: %s"),firstline(state->msg.buf));++if(run_apply(state)<0){+intvalue;++printf_ln(_("Patch failed at %s %s"),msgnum(state),+firstline(state->msg.buf));++if(!git_config_get_bool("advice.amworkdir",&value)&&!value)+printf_ln(_("The copy of the patch that failed is found in: %s"),+am_path(state,"patch"));++exit(128);+}++/*+*TODO:Afterthepatchhasbeenappliedtotheindexwith+*git-apply,weneedtomakecommitaswell.+*/next:am_next(state);
git-am applies a series of patches. If the process terminates
abnormally, we want to be able to resume applying the series of patches.
This requires the session state to be saved in a persistent location.
Implement the mechanism of a "patch queue", represented by 2 integers --
the index of the current patch we are applying and the index of the last
patch, as well as its lifecycle through the following functions:
* am_setup(), which will set up the state directory
$GIT_DIR/rebase-apply. As such, even if the process exits abnormally,
the last-known state will still persist.
* am_load(), which is called if there is an am session in
progress, to load the last known state from the state directory so we
can resume applying patches.
* am_run(), which will do the actual patch application. After applying a
patch, it calls am_next() to increment the current patch index. The
logic for applying and committing a patch is not implemented yet.
* am_destroy(), which is finally called when we successfully applied all
the patches in the queue, to clean up by removing the state directory
and its contents.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 168 insertions(+)
@@ -6,6 +6,158 @@#include"cache.h"#include"builtin.h"#include"exec_cmd.h"+#include"parse-options.h"+#include"dir.h"++structam_state{+/* state directory path */+structstrbufdir;++/* current and last patch numbers, 1-indexed */+intcur;+intlast;+};++/**+*Initializesam_statewiththedefaultvalues.+*/+staticvoidam_state_init(structam_state*state)+{+memset(state,0,sizeof(*state));++strbuf_init(&state->dir,0);+}++/**+*Releasememoryallocatedbyanam_state.+*/+staticvoidam_state_release(structam_state*state)+{+strbuf_release(&state->dir);+}++/**+*Returnspathrelativetotheam_statedirectory.+*/+staticinlineconstchar*am_path(conststructam_state*state,constchar*path)+{+returnmkpath("%s/%s",state->dir.buf,path);+}++/**+*Returns1ifthereisanamsessioninprogress,0otherwise.+*/+staticintam_in_progress(conststructam_state*state)+{+structstatst;++if(lstat(state->dir.buf,&st)<0||!S_ISDIR(st.st_mode))+return0;+if(lstat(am_path(state,"last"),&st)||!S_ISREG(st.st_mode))+return0;+if(lstat(am_path(state,"next"),&st)||!S_ISREG(st.st_mode))+return0;+return1;+}++/**+*Readsthecontentsof`file`.Thethirdargumentcanbeusedtogiveahint+*aboutthefilesize,toavoidreallocs.Returnsnumberofbytesreadon+*success,-1ifthefiledoesnotexist.Iftrimisset,trailingwhitespace+*willberemovedfromthefilecontents.+*/+staticintread_state_file(structstrbuf*sb,constchar*file,size_thint,inttrim)+{+strbuf_reset(sb);+if(strbuf_read_file(sb,file,hint)>=0){+if(trim)+strbuf_trim(sb);++returnsb->len;+}++if(errno==ENOENT)+return-1;++die_errno(_("could not read '%s'"),file);+}++/**+*Loadsstatefromdisk.+*/+staticvoidam_load(structam_state*state)+{+structstrbufsb=STRBUF_INIT;++read_state_file(&sb,am_path(state,"next"),8,1);+state->cur=strtol(sb.buf,NULL,10);++read_state_file(&sb,am_path(state,"last"),8,1);+state->last=strtol(sb.buf,NULL,10);++strbuf_release(&sb);+}++/**+*Removetheam_statedirectory.+*/+staticvoidam_destroy(conststructam_state*state)+{+structstrbufsb=STRBUF_INIT;++strbuf_addstr(&sb,state->dir.buf);+remove_dir_recursively(&sb,0);+strbuf_release(&sb);+}++/**+*Setupanewamsessionforapplyingpatches+*/+staticvoidam_setup(structam_state*state)+{+if(mkdir(state->dir.buf,0777)<0&&errno!=EEXIST)+die_errno(_("failed to create directory '%s'"),state->dir.buf);++write_file(am_path(state,"next"),1,"%d",state->cur);++write_file(am_path(state,"last"),1,"%d",state->last);+}++/**+*Incrementsthepatchpointer,andcleansam_statefortheapplicationofthe+*nextpatch.+*/+staticvoidam_next(structam_state*state)+{+state->cur++;+write_file(am_path(state,"next"),1,"%d",state->cur);+}++/**+*Appliesallqueuedpatches.+*/+staticvoidam_run(structam_state*state)+{+while(state->cur<=state->last){++/* TODO: Patch application not implemented yet */++am_next(state);+}++am_destroy(state);+}++staticstructam_statestate;++staticconstchar*constam_usage[]={+N_("git am [options] [(<mbox>|<Maildir>)...]"),+NULL+};++staticstructoptionam_options[]={+OPT_END()+};intcmd_am(intargc,constchar**argv,constchar*prefix){
For the purpose of applying the patch and committing the results,
implement extracting the patch data, commit message and authorship from
an e-mail message using git-mailinfo.
git-mailinfo is run as a separate process, but ideally in the future,
we should be be able to access its functionality directly without
spawning a new process.
Helped-by: Junio C Hamano [off-list ref]
Helped-by: Jeff King [off-list ref]
Signed-off-by: Paul Tan <redacted>
---
Notes:
v3
* Style fixes
builtin/am.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 232 insertions(+)
@@ -9,6 +9,23 @@#include"parse-options.h"#include"dir.h"#include"run-command.h"+#include"quote.h"++/**+*Returns1ifthefileisemptyordoesnotexist,0otherwise.+*/+staticintis_empty_file(constchar*filename)+{+structstatst;++if(stat(filename,&st)<0){+if(errno==ENOENT)+return1;+die_errno(_("could not stat %s"),filename);+}++return!st.st_size;+}enumpatch_format{PATCH_FORMAT_UNKNOWN=0,
@@ -23,6 +40,12 @@ struct am_state {intcur;intlast;+/* commit message and metadata */+structstrbufauthor_name;+structstrbufauthor_email;+structstrbufauthor_date;+structstrbufmsg;+/* number of digits in patch filename */intprec;};
@@ -296,6 +432,91 @@ static void am_next(struct am_state *state){state->cur++;write_file(am_path(state,"next"),1,"%d",state->cur);++strbuf_reset(&state->author_name);+strbuf_reset(&state->author_email);+strbuf_reset(&state->author_date);+unlink(am_path(state,"author-script"));++strbuf_reset(&state->msg);+unlink(am_path(state,"final-commit"));+}++/**+*Returnsthefilenameofthecurrentpatch.+*/+staticconstchar*msgnum(conststructam_state*state)+{+staticstructstrbufsb=STRBUF_INIT;++strbuf_reset(&sb);+strbuf_addf(&sb,"%0*d",state->prec,state->cur);++returnsb.buf;+}++/**+*Parses`patch`usinggit-mailinfo.state->msgwillbesettothepatch+*message.state->author_name,state->author_email,state->author_datewillbe+*settothepatchauthor'sname,emailanddaterespectively.Thepatch's+*bodywillbewrittento"$state_dir/patch",where$state_diristhestate+*directory.+*+*Returns1ifthepatchshouldbeskipped,0otherwise.+*/+staticintparse_patch(structam_state*state,constchar*patch)+{+FILE*fp;+structchild_processcp=CHILD_PROCESS_INIT;+structstrbufsb=STRBUF_INIT;++cp.git_cmd=1;+cp.in=xopen(patch,O_RDONLY,0);+cp.out=xopen(am_path(state,"info"),O_WRONLY|O_CREAT,0777);++argv_array_push(&cp.args,"mailinfo");+argv_array_push(&cp.args,am_path(state,"msg"));+argv_array_push(&cp.args,am_path(state,"patch"));++if(run_command(&cp)<0)+die("could not parse patch");++close(cp.in);+close(cp.out);++/* Extract message and author information */+fp=xfopen(am_path(state,"info"),"r");+while(!strbuf_getline(&sb,fp,'\n')){+constchar*x;++if(skip_prefix(sb.buf,"Subject: ",&x)){+if(state->msg.len)+strbuf_addch(&state->msg,'\n');+strbuf_addstr(&state->msg,x);+}elseif(skip_prefix(sb.buf,"Author: ",&x))+strbuf_addstr(&state->author_name,x);+elseif(skip_prefix(sb.buf,"Email: ",&x))+strbuf_addstr(&state->author_email,x);+elseif(skip_prefix(sb.buf,"Date: ",&x))+strbuf_addstr(&state->author_date,x);+}+fclose(fp);++/* Skip pine's internal folder data */+if(!strcmp(state->author_name.buf,"Mail System Internal Data"))+return1;++if(is_empty_file(am_path(state,"patch")))+die(_("Patch is empty. Was it split wrong?\n"+"If you would prefer to skip this patch, instead run \"git am --skip\".\n"+"To restore the original branch and stop patching run \"git am --abort\"."));++strbuf_addstr(&state->msg,"\n\n");+if(strbuf_read_file(&state->msg,am_path(state,"msg"),0)<0)+die_errno(_("could not read '%s'"),am_path(state,"msg"));+stripspace(&state->msg,0);++return0;}/**
@@ -304,9 +525,20 @@ static void am_next(struct am_state *state)staticvoidam_run(structam_state*state){while(state->cur<=state->last){+constchar*patch=am_path(state,msgnum(state));++if(!file_exists(patch))+gotonext;++if(parse_patch(state,patch))+gotonext;/* patch should be skipped */++write_author_script(state);+write_file(am_path(state,"final-commit"),1,"%s",state->msg.buf);/* TODO: Patch application not implemented yet */+next:am_next(state);}
Implement do_commit(), which commits the index which contains the
results of applying the patch, along with the extracted commit message
and authorship information.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 47 insertions(+), 4 deletions(-)
@@ -557,6 +560,49 @@ static int run_apply(const struct am_state *state)}/**+*Commitsthecurrentindexwithstate->msgasthecommitmessageand+*state->author_name,state->author_emailandstate->author_dateastheauthor+*information.+*/+staticvoiddo_commit(conststructam_state*state)+{+unsignedchartree[GIT_SHA1_RAWSZ],parent[GIT_SHA1_RAWSZ],+commit[GIT_SHA1_RAWSZ];+unsignedchar*ptr;+structcommit_list*parents=NULL;+constchar*reflog_msg,*author;+structstrbufsb=STRBUF_INIT;++if(write_cache_as_tree(tree,0,NULL))+die(_("git write-tree failed to write a tree"));++if(!get_sha1_commit("HEAD",parent)){+ptr=parent;+commit_list_insert(lookup_commit(parent),&parents);+}else{+ptr=NULL;+fprintf_ln(stderr,_("applying to an empty history"));+}++author=fmt_ident(state->author_name.buf,state->author_email.buf,+state->author_date.buf,IDENT_STRICT);++if(commit_tree(state->msg.buf,state->msg.len,tree,parents,commit,+author,NULL))+die(_("failed to write commit object"));++reflog_msg=getenv("GIT_REFLOG_ACTION");+if(!reflog_msg)+reflog_msg="am";++strbuf_addf(&sb,"%s: %s",reflog_msg,firstline(state->msg.buf));++update_ref(sb.buf,"HEAD",commit,ptr,0,UPDATE_REFS_DIE_ON_ERR);++strbuf_release(&sb);+}++/***Appliesallqueuedpatches.*/staticvoidam_run(structam_state*state)
If a file is unchanged but stat-dirty, git-apply may erroneously fail to
apply patches, thinking that they conflict with a dirty working tree.
As such, since 2a6f08a (am: refresh the index at start and --resolved,
2011-08-15), git-am will refresh the index before applying patches.
Re-implement this behavior.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
supported resuming from a failed patch application by skipping the
current patch. Re-implement this feature by introducing am_skip().
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 119 insertions(+), 2 deletions(-)
@@ -735,6 +737,114 @@ static void am_resolve(struct am_state *state)}/**+*Performsacheckoutfast-forwardfrom`head`to`remote`.If`reset`is+*true,anyunmergedentrieswillbediscarded.Returns0onsuccess,-1on+*failure.+*/+staticintfast_forward_to(structtree*head,structtree*remote,intreset)+{+structlock_file*lock_file=xcalloc(1,sizeof(structlock_file));+structunpack_trees_optionsopts;+structtree_desct[2];++if(parse_tree(head)||parse_tree(remote))+return-1;++hold_locked_index(lock_file,1);++refresh_cache(REFRESH_QUIET);++memset(&opts,0,sizeof(opts));+opts.head_idx=1;+opts.src_index=&the_index;+opts.dst_index=&the_index;+opts.update=1;+opts.merge=1;+opts.reset=reset;+opts.fn=twoway_merge;+init_tree_desc(&t[0],head->buffer,head->size);+init_tree_desc(&t[1],remote->buffer,remote->size);++if(unpack_trees(2,t,&opts)){+rollback_lock_file(lock_file);+return-1;+}++if(write_locked_index(&the_index,lock_file,COMMIT_LOCK))+die(_("unable to write new index file"));++return0;+}++/**+*Cleantheindexwithouttouchingentriesthatarenotmodifiedbetween+*`head`and`remote`.+*/+staticintclean_index(constunsignedchar*head,constunsignedchar*remote)+{+structlock_file*lock_file=xcalloc(1,sizeof(structlock_file));+structtree*head_tree,*remote_tree,*index_tree;+unsignedcharindex[GIT_SHA1_RAWSZ];+structpathspecpathspec;++head_tree=parse_tree_indirect(head);+if(!head_tree)+returnerror(_("Could not parse object '%s'."),sha1_to_hex(head));++remote_tree=parse_tree_indirect(remote);+if(!remote_tree)+returnerror(_("Could not parse object '%s'."),sha1_to_hex(remote));++read_cache_unmerged();++if(fast_forward_to(head_tree,head_tree,1))+return-1;++if(write_cache_as_tree(index,0,NULL))+return-1;++index_tree=parse_tree_indirect(index);+if(!index_tree)+returnerror(_("Could not parse object '%s'."),sha1_to_hex(index));++if(fast_forward_to(index_tree,remote_tree,0))+return-1;++memset(&pathspec,0,sizeof(pathspec));++hold_locked_index(lock_file,1);++if(read_tree(remote_tree,0,&pathspec)){+rollback_lock_file(lock_file);+return-1;+}++if(write_locked_index(&the_index,lock_file,COMMIT_LOCK))+die(_("unable to write new index file"));++remove_branch_state();++return0;+}++/**+*Resumethecurrentamsessionbyskippingthecurrentpatch.+*/+staticvoidam_skip(structam_state*state)+{+unsignedcharhead[GIT_SHA1_RAWSZ];++if(get_sha1("HEAD",head))+hashcpy(head,EMPTY_TREE_SHA1_BIN);++if(clean_index(head,head))+die(_("failed to clean index"));++am_next(state);+am_run(state);+}++/***parse_options()callbackthatvalidatesandsetsopt->valuetothe*PATCH_FORMAT_*enumvaluecorrespondingto`arg`.*/
@@ -760,7 +871,7 @@ static enum resume_mode opt_resume;staticconstchar*constam_usage[]={N_("git am [options] [(<mbox>|<Maildir>)...]"),-N_("git am [options] --continue"),+N_("git am [options] (--continue | --skip)"),NULL};
@@ -773,6 +884,9 @@ static struct option am_options[] = {OPT_CMDMODE('r',"resolved",&opt_resume,N_("synonyms for --continue"),RESUME_RESOLVED),+OPT_CMDMODE(0,"skip",&opt_resume,+N_("skip the current patch"),+RESUME_SKIP),OPT_END()};
Since 0c15cc9 (git-am: --resolved., 2005-11-16), git-am supported
resuming from a failed patch application. The user will manually apply
the patch, and the run git am --resolved which will then commit the
resulting index. Re-implement this feature by introducing am_resolve().
Since it makes no sense for the user to run am --resolved when there is
no session in progress, we error out in this case.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
@@ -707,6 +707,34 @@ next:}/**+*Resumethecurrentamsessionafterpatchapplicationfailure.Theuserdid+*allthehardwork,andwedonothavetodoanypatchapplication.Just+*trustandcommitwhattheuserhasintheindexandworkingtree.+*/+staticvoidam_resolve(structam_state*state)+{+printf_ln(_("Applying: %s"),firstline(state->msg.buf));++if(!index_has_changes(NULL)){+printf_ln(_("No changes - did you forget to use 'git add'?\n"+"If there is nothing left to stage, chances are that something else\n"+"already introduced the same changes; you might want to skip this patch."));+exit(128);+}++if(unmerged_cache()){+printf_ln(_("You still have unmerged paths in your index.\n"+"Did you forget to use 'git add'?"));+exit(128);+}++do_commit(state);++am_next(state);+am_run(state);+}++/***parse_options()callbackthatvalidatesandsetsopt->valuetothe*PATCH_FORMAT_*enumvaluecorrespondingto`arg`.*/
@@ -721,17 +749,30 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, intreturn0;}+enumresume_mode{+RESUME_FALSE=0,+RESUME_RESOLVED+};+staticstructam_statestate;staticintopt_patch_format;+staticenumresume_modeopt_resume;staticconstchar*constam_usage[]={N_("git am [options] [(<mbox>|<Maildir>)...]"),+N_("git am [options] --continue"),NULL};staticstructoptionam_options[]={OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),+OPT_CMDMODE(0,"continue",&opt_resume,+N_("continue applying patches after resolving a conflict"),+RESUME_RESOLVED),+OPT_CMDMODE('r',"resolved",&opt_resume,+N_("synonyms for --continue"),+RESUME_RESOLVED),OPT_END()};
@@ -768,6 +809,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)structstring_listpaths=STRING_LIST_INIT_DUP;inti;+if(opt_resume)+die(_("Resolve operation not in progress, we are not resuming."));+for(i=0;i<argc;i++){if(is_absolute_path(argv[i])||!prefix)string_list_append(&paths,argv[i]);
Since 3e5057a (git am --abort, 2008-07-16), git-am supported the --abort
option that will rewind HEAD back to the original commit. Re-implement
this feature through am_abort().
Since 7b3b7e3 (am --abort: keep unrelated commits since the last failure
and warn, 2010-12-21), to prevent commits made since the last failure
from being lost, git-am will not rewind HEAD back to the original
commit if HEAD moved since the last failure. Re-implement this through
safe_to_abort().
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 92 insertions(+), 3 deletions(-)
@@ -844,6 +865,67 @@ static void am_skip(struct am_state *state)am_run(state);}+staticintsafe_to_abort(conststructam_state*state)+{+structstrbufsb=STRBUF_INIT;+unsignedcharabort_safety[GIT_SHA1_RAWSZ],head[GIT_SHA1_RAWSZ];++if(file_exists(am_path(state,"dirtyindex")))+return0;++if(read_state_file(&sb,am_path(state,"abort-safety"),40,1)>0){+if(get_sha1_hex(sb.buf,abort_safety))+die(_("could not parse %s"),am_path(state,"abort_safety"));+}else+hashclr(abort_safety);++if(get_sha1("HEAD",head))+hashclr(head);++if(!hashcmp(head,abort_safety))+return1;++error(_("You seem to have moved HEAD since the last 'am' failure.\n"+"Not rewinding to ORIG_HEAD"));++return0;+}++/**+*Abortsthecurrentamsessionifitissafetodoso.+*/+staticvoidam_abort(structam_state*state)+{+unsignedcharcurr_head[GIT_SHA1_RAWSZ],orig_head[GIT_SHA1_RAWSZ];+inthas_curr_head,has_orig_head;+constchar*curr_branch;++if(!safe_to_abort(state)){+am_destroy(state);+return;+}++curr_branch=resolve_refdup("HEAD",0,curr_head,NULL);+has_curr_head=!is_null_sha1(curr_head);+if(!has_curr_head)+hashcpy(curr_head,EMPTY_TREE_SHA1_BIN);++has_orig_head=!get_sha1("ORIG_HEAD",orig_head);+if(!has_orig_head)+hashcpy(orig_head,EMPTY_TREE_SHA1_BIN);++clean_index(curr_head,orig_head);++if(has_orig_head)+update_ref("am --abort","HEAD",orig_head,+has_curr_head?curr_head:NULL,0,+UPDATE_REFS_DIE_ON_ERR);+elseif(curr_branch)+delete_ref(curr_branch,NULL,REF_NODEREF);++am_destroy(state);+}+/***parse_options()callbackthatvalidatesandsetsopt->valuetothe*PATCH_FORMAT_*enumvaluecorrespondingto`arg`.
@@ -871,7 +954,7 @@ static enum resume_mode opt_resume;staticconstchar*constam_usage[]={N_("git am [options] [(<mbox>|<Maildir>)...]"),-N_("git am [options] (--continue | --skip)"),+N_("git am [options] (--continue | --skip | --abort)"),NULL};
@@ -887,6 +970,9 @@ static struct option am_options[] = {OPT_CMDMODE(0,"skip",&opt_resume,N_("skip the current patch"),RESUME_SKIP),+OPT_CMDMODE(0,"abort",&opt_resume,+N_("restore the original branch and abort the patching operation."),+RESUME_ABORT),OPT_END()};
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
would error out if the user gave it mbox(s) on the command-line, but
there was a session in progress.
Since c95b138 (Fix git-am safety checks, 2006-09-15), git-am would
detect if the user attempted to feed it a mbox via stdin, by checking if
stdin is not a tty and there is no resume command given.
Re-implement the above two safety checks.
Signed-off-by: Paul Tan <redacted>
---
Notes:
NOTE: there's no test for this
builtin/am.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
@@ -1003,9 +1003,24 @@ int cmd_am(int argc, const char **argv, const char *prefix)if(read_index_preload(&the_index,NULL)<0)die(_("failed to read the index"));-if(am_in_progress(&state))+if(am_in_progress(&state)){+/*+*Catchusererrortofeeduspatcheswhenthereisasession+*inprogress:+*+*1.mboxpath(s)areprovidedonthecommand-line.+*2.stdinisnotatty:theuseristryingtofeedusapatch+*fromstandardinput.Thisissomewhatunreliable--stdin+*couldbe/dev/nullforexampleandthecallerdidnot+*intendtofeedusapatchbutwantedtocontinue+*unattended.+*/+if(argc||(!opt_resume&&!isatty(0)))+die(_("previous rebase directory %s still exists but mbox given."),+state.dir.buf);+am_load(&state);-else{+}else{structstring_listpaths=STRING_LIST_INIT_DUP;inti;
Since ced9456 (Give the user a hint for how to continue in the case that
git-am fails because it requires user intervention, 2006-05-02), git-am
prints additional information on how the user can re-invoke git-am to
resume patch application after resolving the failure. Re-implement this
through the die_user_resolve() function.
Since cc12005 (Make git rebase interactive help match documentation.,
2006-05-13), git-am supports the --resolvemsg option which is used by
git-rebase to override the message printed out when git-am fails.
Re-implement this option.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 26 +++++++++++++++++++++++---
1 file changed, 23 insertions(+), 3 deletions(-)
@@ -636,6 +639,21 @@ static int parse_patch(struct am_state *state, const char *patch)return0;}+/**+*Dieswithauser-friendlymessageonhowtoproceedafterresolvingthe+*problem.Thismessagecanbeoverriddenwithstate->resolvemsg.+*/+staticvoidNORETURNdie_user_resolve(conststructam_state*state)+{+if(state->resolvemsg)+printf_ln("%s",state->resolvemsg);+else+printf_ln(_("When you have resolved this problem, run \"git am --continue\".\n"+"If you prefer to skip this patch, run \"git am --skip\" instead.\n"+"To restore the original branch and stop patching, run \"git am --abort\"."));+exit(128);+}+/**Appliescurrentpatchwithgit-apply.Returns0onsuccess,-1otherwise.*/
@@ -746,7 +764,7 @@ static void am_run(struct am_state *state)printf_ln(_("The copy of the patch that failed is found in: %s"),am_path(state,"patch"));-exit(128);+die_user_resolve(state);}do_commit(state);
@@ -771,13 +789,13 @@ static void am_resolve(struct am_state *state)printf_ln(_("No changes - did you forget to use 'git add'?\n""If there is nothing left to stage, chances are that something else\n""already introduced the same changes; you might want to skip this patch."));-exit(128);+die_user_resolve(state);}if(unmerged_cache()){printf_ln(_("You still have unmerged paths in your index.\n""Did you forget to use 'git add'?"));-exit(128);+die_user_resolve(state);}do_commit(state);
@@ -991,6 +1009,8 @@ static struct option am_options[] = {OPT__QUIET(&state.quiet,N_("be quiet")),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),+OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,+N_("override error message when patch failure occurs")),OPT_CMDMODE(0,"continue",&opt_resume,N_("continue applying patches after resolving a conflict"),RESUME_RESOLVED),
Since 0e987a1 (am, rebase: teach quiet option, 2009-06-16), git-am
supported the --quiet option and GIT_QUIET environment variable, and
when told to be quiet, would only speak on failure. Re-implement this by
introducing the say() function, which works like fprintf_ln(), but would
only write to the stream when state->quiet is false.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 36 +++++++++++++++++++++++++++++++++---
1 file changed, 33 insertions(+), 3 deletions(-)
@@ -654,7 +683,7 @@ static void do_commit(const struct am_state *state)commit_list_insert(lookup_commit(parent),&parents);}else{ptr=NULL;-fprintf_ln(stderr,_("applying to an empty history"));+say(state,stderr,_("applying to an empty history"));}author=fmt_ident(state->author_name.buf,state->author_email.buf,
@@ -736,7 +765,7 @@ next:*/staticvoidam_resolve(structam_state*state){-printf_ln(_("Applying: %s"),firstline(state->msg.buf));+say(state,stdout,_("Applying: %s"),firstline(state->msg.buf));if(!index_has_changes(NULL)){printf_ln(_("No changes - did you forget to use 'git add'?\n"
@@ -959,6 +988,7 @@ static const char * const am_usage[] = {};staticstructoptionam_options[]={+OPT__QUIET(&state.quiet,N_("be quiet")),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_CMDMODE(0,"continue",&opt_resume,
A caller may wish to write a temporary index as a tree. However,
write_cache_as_tree() assumes that the index was read from, and will
write to, the default index file path. Introduce write_index_as_tree()
which removes this limitation by allowing the caller to specify its own
index_state and index file path.
Signed-off-by: Paul Tan <redacted>
---
cache-tree.c | 29 +++++++++++++++++------------
cache-tree.h | 1 +
2 files changed, 18 insertions(+), 12 deletions(-)
@@ -603,23 +603,23 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)*/lock_file=xcalloc(1,sizeof(structlock_file));-newfd=hold_locked_index(lock_file,1);+newfd=hold_lock_file_for_update(lock_file,index_path,LOCK_DIE_ON_ERROR);-entries=read_cache();+entries=read_index_from(index_state,index_path);if(entries<0)returnWRITE_TREE_UNREADABLE_INDEX;if(flags&WRITE_TREE_IGNORE_CACHE_TREE)-cache_tree_free(&(active_cache_tree));+cache_tree_free(&index_state->cache_tree);-if(!active_cache_tree)-active_cache_tree=cache_tree();+if(!index_state->cache_tree)+index_state->cache_tree=cache_tree();-was_valid=cache_tree_fully_valid(active_cache_tree);+was_valid=cache_tree_fully_valid(index_state->cache_tree);if(!was_valid){-if(cache_tree_update(&the_index,flags)<0)+if(cache_tree_update(index_state,flags)<0)returnWRITE_TREE_UNMERGED_INDEX;if(0<=newfd){-if(!write_locked_index(&the_index,lock_file,COMMIT_LOCK))+if(!write_locked_index(index_state,lock_file,COMMIT_LOCK))newfd=-1;}/* Not being able to write is fine -- we are only interested
@@ -631,14 +631,14 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)}if(prefix){-structcache_tree*subtree=-cache_tree_find(active_cache_tree,prefix);+structcache_tree*subtree;+subtree=cache_tree_find(index_state->cache_tree,prefix);if(!subtree)returnWRITE_TREE_PREFIX_ERROR;hashcpy(sha1,subtree->sha1);}else-hashcpy(sha1,active_cache_tree->sha1);+hashcpy(sha1,index_state->cache_tree->sha1);if(0<=newfd)rollback_lock_file(lock_file);
@@ -646,6 +646,11 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)return0;}+intwrite_cache_as_tree(unsignedchar*sha1,intflags,constchar*prefix)+{+returnwrite_index_as_tree(sha1,&the_index,get_index_file(),flags,prefix);+}+staticvoidprime_cache_tree_rec(structcache_tree*it,structtree*tree){structtree_descdesc;
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
will refuse to apply patches if the index is dirty. Re-implement this
behavior.
Signed-off-by: Paul Tan <redacted>
---
Notes:
Note: no tests for this
builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
supported the --signoff option which will append a signoff at the end of
the commit messsage. Re-implement this feature by calling
append_signoff() if the option is set.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
@@ -636,6 +644,9 @@ static int parse_patch(struct am_state *state, const char *patch)die_errno(_("could not read '%s'"),am_path(state,"msg"));stripspace(&state->msg,0);+if(state->sign)+append_signoff(&state->msg,0,0);+return0;}
@@ -1007,6 +1018,8 @@ static const char * const am_usage[] = {staticstructoptionam_options[]={OPT__QUIET(&state.quiet,N_("be quiet")),+OPT_BOOL('s',"signoff",&state.sign,+N_("add a Signed-off-by line to the commit message")),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07),
git-am.sh supported the --3way option, and if set, would attempt to do a
3-way merge if the initial patch application fails.
Since 5d86861 (am -3: list the paths that needed 3-way fallback,
2012-03-28), in a 3-way merge git-am.sh would list the paths that needed
3-way fallback, so that the user can review them more carefully to spot
mismerges.
Re-implement the above in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 143 insertions(+), 4 deletions(-)
@@ -685,8 +710,100 @@ static int run_apply(const struct am_state *state)/* Reload index as git-apply will have modified it. */discard_cache();+read_cache_from(index_file?index_file:get_index_file());++return0;+}++/**+*Buildsaindexthatcontainsjusttheblobsneededfora3waymerge.+*/+staticintbuild_fake_ancestor(conststructam_state*state,constchar*index_file)+{+structchild_processcp=CHILD_PROCESS_INIT;++cp.git_cmd=1;+argv_array_push(&cp.args,"apply");+argv_array_pushf(&cp.args,"--build-fake-ancestor=%s",index_file);+argv_array_push(&cp.args,am_path(state,"patch"));++if(run_command(&cp))+return-1;++return0;+}++/**+*Attemptathreewaymerge,usingindex_pathasthetemporaryindex.+*/+staticintfall_back_threeway(conststructam_state*state,constchar*index_path)+{+unsignedcharorig_tree[GIT_SHA1_RAWSZ],his_tree[GIT_SHA1_RAWSZ],+our_tree[GIT_SHA1_RAWSZ];+constunsignedchar*bases[1]={orig_tree};+structmerge_optionso;+structcommit*result;++if(get_sha1("HEAD",our_tree)<0)+hashcpy(our_tree,EMPTY_TREE_SHA1_BIN);++if(build_fake_ancestor(state,index_path))+returnerror("could not build fake ancestor");++discard_cache();+read_cache_from(index_path);++if(write_index_as_tree(orig_tree,&the_index,index_path,0,NULL))+returnerror(_("Repository lacks necessary blobs to fall back on 3-way merge."));++say(state,stdout,_("Using index info to reconstruct a base tree..."));++if(!state->quiet){+/*+*Listpathsthatneeded3-wayfallback,sothattheusercan+*reviewthemwithextracaretospotmismerges.+*/+structrev_inforev_info;+constchar*diff_filter_str="--diff-filter=AM";++init_revisions(&rev_info,NULL);+rev_info.diffopt.output_format=DIFF_FORMAT_NAME_STATUS;+diff_opt_parse(&rev_info.diffopt,&diff_filter_str,1);+add_pending_sha1(&rev_info,"HEAD",our_tree,0);+diff_setup_done(&rev_info.diffopt);+run_diff_index(&rev_info,1);+}++if(run_apply(state,index_path))+returnerror(_("Did you hand edit your patch?\n"+"It does not apply to blobs recorded in its index."));++if(write_index_as_tree(his_tree,&the_index,index_path,0,NULL))+returnerror("could not write tree");++say(state,stdout,_("Falling back to patching base and 3-way merge..."));++discard_cache();read_cache();+/*+*Thisisnotsowrong.Dependingonwhichbasewepicked,orig_tree+*maybewildlydifferentfromours,buthis_treehasthesamesetof+*wildlydifferentchangesinpartsthepatchdidnottouch,so+*recursiveendsupcancelingthem,sayingthatwerevertedallthose+*changes.+*/++init_merge_options(&o);++o.branch1="HEAD";+o.branch2=firstline(state->msg.buf);+if(state->quiet)+o.verbosity=0;++if(merge_recursive_generic(&o,our_tree,his_tree,1,bases,&result))+returnerror(_("Failed to merge in the changes."));+return0;}
@@ -1017,6 +1154,8 @@ static const char * const am_usage[] = {};staticstructoptionam_options[]={+OPT_BOOL('3',"3way",&state.threeway,+N_("allow fall back on 3way merging if needed")),OPT__QUIET(&state.quiet,N_("be quiet")),OPT_BOOL('s',"signoff",&state.sign,N_("add a Signed-off-by line to the commit message")),
Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07),
git-am.sh supported the -k/--keep option to pass the -k option to
git-mailsplit.
Since f7e5ea1 (am: learn passing -b to mailinfo, 2012-01-16), git-am.sh
supported the --keep-non-patch option to pass the -b option to
git-mailsplit.
Re-implement these two options in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 51 insertions(+)
@@ -81,6 +81,12 @@ enum patch_format {PATCH_FORMAT_MBOX};+enumkeep_type{+KEEP_FALSE=0,+KEEP_TRUE,/* pass -k flag to git-mailinfo */+KEEP_NON_PATCH/* pass -b flag to git-mailinfo */+};+structam_state{/* state directory path */structstrbufdir;
@@ -104,6 +110,9 @@ struct am_state {intsign;+/* one of the enum keep_type values */+intkeep;+/* override error message when patch failure occurs */constchar*resolvemsg;
@@ -527,6 +545,21 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,write_file(am_path(state,"sign"),1,state->sign?"t":"f");+switch(state->keep){+caseKEEP_FALSE:+str="f";+break;+caseKEEP_TRUE:+str="t";+break;+caseKEEP_NON_PATCH:+str="b";+break;+default:+die("BUG: invalid value for state->keep");+}+write_file(am_path(state,"keep"),1,"%s",str);+if(state->rebasing)write_file(am_path(state,"rebasing"),1,"%s","");else
@@ -653,6 +686,20 @@ static int parse_patch(struct am_state *state, const char *patch)cp.out=xopen(am_path(state,"info"),O_WRONLY|O_CREAT,0777);argv_array_push(&cp.args,"mailinfo");++switch(state->keep){+caseKEEP_FALSE:+break;+caseKEEP_TRUE:+argv_array_push(&cp.args,"-k");+break;+caseKEEP_NON_PATCH:+argv_array_push(&cp.args,"-b");+break;+default:+die("BUG: invalid value for state->keep");+}+argv_array_push(&cp.args,am_path(state,"msg"));argv_array_push(&cp.args,am_path(state,"patch"));
@@ -1326,6 +1373,10 @@ static struct option am_options[] = {OPT__QUIET(&state.quiet,N_("be quiet")),OPT_BOOL('s',"signoff",&state.sign,N_("add a Signed-off-by line to the commit message")),+OPT_SET_INT('k',"keep",&state.keep,+N_("pass -k flag to git-mailinfo"),KEEP_TRUE),+OPT_SET_INT(0,"keep-non-patch",&state.keep,+N_("pass -b flag to git-mailinfo"),KEEP_NON_PATCH),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,
Should git-am terminate unexpectedly between the point where the state
directory is created, but the "next" and "last" files are not written
yet, a stray state directory will be left behind.
As such, since b141f3c (am: handle stray $dotest directory, 2013-06-15),
git-am.sh explicitly recognizes such a stray directory, and allows the
user to remove it with am --abort.
Re-implement this feature in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
@@ -1395,6 +1395,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)structstring_listpaths=STRING_LIST_INIT_DUP;inti;+/*+*Possiblestraydotestdirectoryintheindependent-runcase.+*/+if(file_exists(state.dir.buf)&&!state.rebasing){+if(opt_resume==RESUME_ABORT){+am_destroy(&state);+am_state_release(&state);+return0;+}++die(_("Stray %s directory found.\n"+"Use \"git am --abort\" to remove it."),+state.dir.buf);+}+if(opt_resume)die(_("Resolve operation not in progress, we are not resuming."));
Since 3041c32 (am: --rebasing, 2008-03-04), git-am.sh supported the
--rebasing option, which is used internally by git-rebase to tell git-am
that it is being used for its purpose. It would create the empty file
$state_dir/rebasing to help "completion" scripts tell if the ongoing
operation is am or rebase.
As of 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26),
--rebasing also implies --3way as well.
Since a1549e1 (am: return control to caller, for housekeeping,
2013-05-12), git-am.sh would only clean up the state directory when it
is not --rebasing, instead deferring cleanup to git-rebase.sh.
Re-implement the above in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
@@ -1175,6 +1191,8 @@ static struct option am_options[] = {OPT_CMDMODE(0,"abort",&opt_resume,N_("restore the original branch and abort the patching operation."),RESUME_ABORT),+OPT_HIDDEN_BOOL(0,"rebasing",&state.rebasing,+N_("(internal use for git-rebase)")),OPT_END()};
Since ad2c928 (git-am: Add command line parameter `--keep-cr` passing it
to git-mailsplit, 2010-02-27), git-am.sh supported the --keep-cr option
and would pass it to git-mailsplit.
Since e80d4cb (git-am: Add am.keepcr and --no-keep-cr to override it,
2010-02-27), git-am.sh supported the am.keepcr config setting, which
controls whether --keep-cr is on by default.
Re-implement the above in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
@@ -1392,6 +1404,12 @@ static struct option am_options[] = {N_("pass -b flag to git-mailinfo"),KEEP_NON_PATCH),OPT_BOOL('m',"message-id",&state.message_id,N_("pass -m flag to git-mailinfo")),+{OPTION_SET_INT,0,"keep-cr",&opt_keep_cr,NULL,+N_("pass --keep-cr flag to git-mailsplit for mbox format"),+PARSE_OPT_NOARG|PARSE_OPT_NONEG,NULL,1},+{OPTION_SET_INT,0,"no-keep-cr",&opt_keep_cr,NULL,+N_("do not pass --keep-cr flag to git-mailsplit independent of am.keepcr"),+PARSE_OPT_NOARG|PARSE_OPT_NONEG,NULL,0},OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,
Since 5e835ca (rebase: do not munge commit log message, 2008-04-16),
git am --rebasing no longer gets the commit log message from the patch,
but reads it directly from the commit identified by the "From " header
line.
Since 43c2325 (am: use get_author_ident_from_commit instead of mailinfo
when rebasing, 2010-06-16), git am --rebasing also gets the author name,
email and date directly from the commit.
Since 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26), git am
--rebasing does not use git-mailinfo to get the patch body, but rather
generates it directly from the commit itself.
The above 3 commits introduced a separate parse_patch() code path in
git-am.sh's --rebasing mode that bypasses git-mailinfo. Re-implement
this code path in builtin/am.c as parse_patch_rebase().
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 153 insertions(+), 2 deletions(-)
Since a078f73 (git-am: add --message-id/--no-message-id, 2014-11-25),
git-am.sh supported the --[no-]message-id options, and the
"am.messageid" setting which specifies the default option.
--[no-]message-id tells git-am whether or not the -m option should be
passed to git-mailinfo.
Re-implement this option in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
Notes:
No test for am.messageid
builtin/am.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
@@ -113,6 +113,9 @@ struct am_state {/* one of the enum keep_type values */intkeep;+/* pass -m flag to git-mailinfo */+intmessage_id;+/* override error message when patch failure occurs */constchar*resolvemsg;
@@ -700,6 +710,9 @@ static int parse_patch(struct am_state *state, const char *patch)die("BUG: invalid value for state->keep");}+if(state->message_id)+argv_array_push(&cp.args,"-m");+argv_array_push(&cp.args,am_path(state,"msg"));argv_array_push(&cp.args,am_path(state,"patch"));
@@ -1377,6 +1390,8 @@ static struct option am_options[] = {N_("pass -k flag to git-mailinfo"),KEEP_TRUE),OPT_SET_INT(0,"keep-non-patch",&state.keep,N_("pass -b flag to git-mailinfo"),KEEP_NON_PATCH),+OPT_BOOL('m',"message-id",&state.message_id,+N_("pass -m flag to git-mailinfo")),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,
Since 017678b (am/mailinfo: Disable scissors processing by default,
2009-08-26), git-am supported the --[no-]scissors option, passing it to
git-mailinfo.
Re-implement support for this option.
Signed-off-by: Paul Tan <redacted>
---
Notes:
There are tests for mailinfo --scissors, but not am --scissors, or
mailinfo.scissors.
builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
@@ -87,6 +87,12 @@ enum keep_type {KEEP_NON_PATCH/* pass -b flag to git-mailinfo */};+enumscissors_type{+SCISSORS_UNSET=-1,+SCISSORS_TRUE,/* pass --scissors to git-mailinfo */+SCISSORS_FALSE/* pass --no-scissors to git-mailinfo */+};+structam_state{/* state directory path */structstrbufdir;
@@ -116,6 +122,9 @@ struct am_state {/* pass -m flag to git-mailinfo */intmessage_id;+/* one of the enum scissors_type values */+intscissors;+/* override error message when patch failure occurs */constchar*resolvemsg;
@@ -581,6 +600,21 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,write_file(am_path(state,"messageid"),1,state->message_id?"t":"f");+switch(state->scissors){+caseSCISSORS_UNSET:+str="";+break;+caseSCISSORS_FALSE:+str="f";+break;+caseSCISSORS_TRUE:+str="t";+break;+default:+die("BUG: invalid value for state->scissors");+}+write_file(am_path(state,"scissors"),1,"%s",str);+if(state->rebasing)write_file(am_path(state,"rebasing"),1,"%s","");else
@@ -724,6 +758,19 @@ static int parse_patch(struct am_state *state, const char *patch)if(state->message_id)argv_array_push(&cp.args,"-m");+switch(state->scissors){+caseSCISSORS_UNSET:+break;+caseSCISSORS_FALSE:+argv_array_push(&cp.args,"--no-scissors");+break;+caseSCISSORS_TRUE:+argv_array_push(&cp.args,"--scissors");+break;+default:+die("BUG: invalid value for state->scissors");+}+argv_array_push(&cp.args,am_path(state,"msg"));argv_array_push(&cp.args,am_path(state,"patch"));
@@ -1410,6 +1457,8 @@ static struct option am_options[] = {{OPTION_SET_INT,0,"no-keep-cr",&opt_keep_cr,NULL,N_("do not pass --keep-cr flag to git-mailsplit independent of am.keepcr"),PARSE_OPT_NOARG|PARSE_OPT_NONEG,NULL,0},+OPT_BOOL('c',"scissors",&state.scissors,+N_("strip everything before a scissors line")),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,
Since a79ec62 (git-am: Add --ignore-date option, 2009-01-24), git-am.sh
supported the --ignore-date option, and would use the current timestamp
instead of the one provided in the patch if the option was set.
Re-implement this option in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
@@ -1521,6 +1524,8 @@ static struct option am_options[] = {OPT_CMDMODE(0,"abort",&opt_resume,N_("restore the original branch and abort the patching operation."),RESUME_ABORT),+OPT_BOOL(0,"ignore-date",&state.ignore_date,+N_("use current timestamp for author date")),OPT_HIDDEN_BOOL(0,"rebasing",&state.rebasing,N_("(internal use for git-rebase)")),OPT_END()
Since 3b4e395 (am: add the --gpg-sign option, 2014-02-01), git-am.sh
supported the --gpg-sign option, and would pass it to git-commit-tree,
thus GPG-signing the commit object.
Re-implement this option in builtin/am.c.
git-commit-tree would also sign the commit by default if the
commit.gpgsign setting is true. Since we do not run commit-tree, we
re-implement this behavior by handling the commit.gpgsign setting
ourselves.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
@@ -1535,6 +1541,9 @@ static struct option am_options[] = {N_("lie about committer date")),OPT_BOOL(0,"ignore-date",&state.ignore_date,N_("use current timestamp for author date")),+{OPTION_STRING,'S',"gpg-sign",&state.sign_commit,N_("key-id"),+N_("GPG-sign commits"),+PARSE_OPT_OPTARG,NULL,(intptr_t)""},OPT_HIDDEN_BOOL(0,"rebasing",&state.rebasing,N_("(internal use for git-rebase)")),OPT_END()
git-am.sh recognizes some of git-apply's options, and would pass them to
git-apply:
* --whitespace, since 8c31cb8 (git-am: --whitespace=x option.,
2006-02-28)
* -C, since 67dad68 (add -C[NUM] to git-am, 2007-02-08)
* -p, since 2092a1f (Teach git-am to pass -p option down to git-apply,
2007-02-11)
* --directory, since b47dfe9 (git-am: add --directory=<dir> option,
2009-01-11)
* --reject, since b80da42 (git-am: implement --reject option passed to
git-apply, 2009-01-23)
* --ignore-space-change, --ignore-whitespace, since 86c91f9 (git apply:
option to ignore whitespace differences, 2009-08-04)
* --exclude, since 77e9e49 (am: pass exclude down to apply, 2011-08-03)
* --include, since 58725ef (am: support --include option, 2012-03-28)
Re-implement support for these options in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
@@ -1459,8 +1478,35 @@ static struct option am_options[] = {PARSE_OPT_NOARG|PARSE_OPT_NONEG,NULL,0},OPT_BOOL('c',"scissors",&state.scissors,N_("strip everything before a scissors line")),+OPT_PASSTHRU_ARGV(0,"whitespace",&state.git_apply_opts,N_("action"),+N_("pass it through git-apply"),+0),+OPT_PASSTHRU_ARGV(0,"ignore-space-change",&state.git_apply_opts,NULL,+N_("pass it through git-apply"),+PARSE_OPT_NOARG),+OPT_PASSTHRU_ARGV(0,"ignore-whitespace",&state.git_apply_opts,NULL,+N_("pass it through git-apply"),+PARSE_OPT_NOARG),+OPT_PASSTHRU_ARGV(0,"directory",&state.git_apply_opts,N_("root"),+N_("pass it through git-apply"),+0),+OPT_PASSTHRU_ARGV(0,"exclude",&state.git_apply_opts,N_("path"),+N_("pass it through git-apply"),+0),+OPT_PASSTHRU_ARGV(0,"include",&state.git_apply_opts,N_("path"),+N_("pass it through git-apply"),+0),+OPT_PASSTHRU_ARGV('C',NULL,&state.git_apply_opts,N_("n"),+N_("pass it through git-apply"),+0),+OPT_PASSTHRU_ARGV('p',NULL,&state.git_apply_opts,N_("num"),+N_("pass it through git-apply"),+0),OPT_CALLBACK(0,"patch-format",&opt_patch_format,N_("format"),N_("format the patch(es) are in"),parse_opt_patchformat),+OPT_PASSTHRU_ARGV(0,"reject",&state.git_apply_opts,NULL,+N_("pass it through git-apply"),+PARSE_OPT_NOARG),OPT_STRING(0,"resolvemsg",&state.resolvemsg,NULL,N_("override error message when patch failure occurs")),OPT_CMDMODE(0,"continue",&opt_resume,
Since 3f01ad6 (am: Add --committer-date-is-author-date option,
2009-01-22), git-am.sh implemented the --committer-date-is-author-date
option, which tells git-am to use the timestamp recorded in the email
message as both author and committer date.
Re-implement this option in builtin/am.c.
Signed-off-by: Paul Tan <redacted>
---
builtin/am.c | 9 +++++++++
1 file changed, 9 insertions(+)
@@ -1524,6 +1530,9 @@ static struct option am_options[] = {OPT_CMDMODE(0,"abort",&opt_resume,N_("restore the original branch and abort the patching operation."),RESUME_ABORT),+OPT_BOOL(0,"committer-date-is-author-date",+&state.committer_date_is_author_date,+N_("lie about committer date")),OPT_BOOL(0,"ignore-date",&state.ignore_date,N_("use current timestamp for author date")),OPT_HIDDEN_BOOL(0,"rebasing",&state.rebasing,
From: Stefan Beller <hidden> Date: 2016-06-15 23:05:22
On Thu, Jun 18, 2015 at 4:25 AM, Paul Tan [off-list ref] wrote:
+/**
+ * Reads the contents of `file`. The third argument can be used to give a hint
I would avoid `third` here. (I needed to count twice to be sure which
argument you
were referring to, as I was confused.) Also how do you abstain from
giving a hint?
(0 or negative or MAX_INT?)
So maybe
/**
* Reads the contents of `file`. Returns number of bytes read on success,
* -1 if the file does not exist. If trim is set, trailing
whitespace will be removed
* from the file contents. If `hint` is non-zero, it is used as a
hint for initial
* allocation to avoid reallocs.
*/
quoted hunk
+ * about the file size, to avoid reallocs. Returns number of bytes read on
+ * success, -1 if the file does not exist. If trim is set, trailing whitespace
+ * will be removed from the file contents.
+ */
+static int read_state_file(struct strbuf *sb, const char *file, size_t hint, int trim)
+{
+ strbuf_reset(sb);
+ if (strbuf_read_file(sb, file, hint) >= 0) {
+ if (trim)
+ strbuf_trim(sb);
+
+ return sb->len;
+ }
+
+ if (errno == ENOENT)
+ return -1;
+
+ die_errno(_("could not read '%s'"), file);
+}
+
+/**
+ * Loads state from disk.
+ */
+static void am_load(struct am_state *state)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ read_state_file(&sb, am_path(state, "next"), 8, 1);
+ state->cur = strtol(sb.buf, NULL, 10);
+
+ read_state_file(&sb, am_path(state, "last"), 8, 1);
+ state->last = strtol(sb.buf, NULL, 10);
+
+ strbuf_release(&sb);
+}
+
+/**
+ * Remove the am_state directory.
+ */
+static void am_destroy(const struct am_state *state)
+{
+ struct strbuf sb = STRBUF_INIT;
+
+ strbuf_addstr(&sb, state->dir.buf);
+ remove_dir_recursively(&sb, 0);
+ strbuf_release(&sb);
+}
+
+/**
+ * Setup a new am session for applying patches
+ */
+static void am_setup(struct am_state *state)
+{
+ if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
+ die_errno(_("failed to create directory '%s'"), state->dir.buf);
+
+ write_file(am_path(state, "next"), 1, "%d", state->cur);
+
+ write_file(am_path(state, "last"), 1, "%d", state->last);
+}
+
+/**
+ * Increments the patch pointer, and cleans am_state for the application of the
+ * next patch.
+ */
+static void am_next(struct am_state *state)
+{
+ state->cur++;
+ write_file(am_path(state, "next"), 1, "%d", state->cur);
+}
+
+/**
+ * Applies all queued patches.
+ */
+static void am_run(struct am_state *state)
+{
+ while (state->cur <= state->last) {
+
+ /* TODO: Patch application not implemented yet */
+
+ am_next(state);
+ }
+
+ am_destroy(state);
+}
+
+static struct am_state state;
+
+static const char * const am_usage[] = {
+ N_("git am [options] [(<mbox>|<Maildir>)...]"),
+ NULL
+};
+
+static struct option am_options[] = {
+ OPT_END()
+};
int cmd_am(int argc, const char **argv, const char *prefix)
{
@@ -6,6 +6,158 @@#include"cache.h"#include"builtin.h"#include"exec_cmd.h"+#include"parse-options.h"+#include"dir.h"++structam_state{+/* state directory path */+structstrbufdir;++/* current and last patch numbers, 1-indexed */+intcur;+intlast;+};++/**+*Initializesam_statewiththedefaultvalues.+*/+staticvoidam_state_init(structam_state*state)+{+memset(state,0,sizeof(*state));++strbuf_init(&state->dir,0);+}
With strbufs, we use the initializer STRBUF_INIT. How about using
#define AM_STATE_INIT { STRBUF_INIT, 0, 0 }
here?
+/**
+ * Reads the contents of `file`. The third argument can be used to give a hint
+ * about the file size, to avoid reallocs. Returns number of bytes read on
+ * success, -1 if the file does not exist. If trim is set, trailing whitespace
+ * will be removed from the file contents.
+ */
+static int read_state_file(struct strbuf *sb, const char *file,
size_t hint, int trim)
+{
+ strbuf_reset(sb);
+ if (strbuf_read_file(sb, file, hint) >= 0) {
+ if (trim)
+ strbuf_trim(sb);
+
+ return sb->len;
+ }
+
+ if (errno == ENOENT)
+ return -1;
+
+ die_errno(_("could not read '%s'"), file);
+}
A couple of thoughts:
- why not reuse the strbuf by making it a part of the am_state()? That way, you can allocate, say, 1024 bytes (should be plenty enough for most of our operations) and just reuse them in all of the functions. We will not make any of this multi-threaded anyway, I don't think.
- Given that we only read short files all the time, why not skip the hint parameter? Especially if we reuse the strbuf, it should be good enough to allocate a reasonable buffer first and then just assume that we do not have to reallocate it all that often anyway.
- Since we only read files from the state directory, why not pass the basename as parameter? That way we can avoid calling `am_path()` explicitly over and over again (and yours truly cannot forget to call `am_path()` in future patches).
- If you agree with these suggestions, the signature would become something like
static void read_state_file(struct am_state *state, const char *basename, int trim);
Given that `remove_dir_recursively()` has to reset the strbuf with the directory's path to the original value before it returns (because it recurses into itself, therefore the value *has* to be reset when returning), we can just call
remove_dir_recursively(&state->dir, 0);
and do not need another temporary strbuf.
+/**
+ * Increments the patch pointer, and cleans am_state for the application of the
+ * next patch.
+ */
+static void am_next(struct am_state *state)
+{
+ state->cur++;
+ write_file(am_path(state, "next"), 1, "%d", state->cur);
+}
Locking and re-checking the contents of "next" before writing the incremented value would probably be a little too paranoid...
(Just saying it out loud, the current code is fine by me.)
Ciao,
Dscho
@@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)strbuf_release(&sb);}+/*+*Returns1ifthefilelookslikeapieceofemaila-laRFC2822,0otherwise.+*Wecheckthisbygrabbingallthenon-indentedlinesandseeingiftheylook+*liketheybeginwithvalidheaderfieldnames.+*/+staticintis_email(constchar*filename)+{+structstrbufsb=STRBUF_INIT;+FILE*fp=xfopen(filename,"r");+intret=1;++while(!strbuf_getline(&sb,fp,'\n')){+constchar*x;++strbuf_rtrim(&sb);++if(!sb.len)+break;/* End of header */++/* Ignore indented folded lines */+if(*sb.buf=='\t'||*sb.buf==' ')+continue;++/* It's a header if it matches the regexp "^[!-9;-~]+:" */
Why not just compile a regex and use it here? We use regexes elsewhere anyway...
+/**
+ * Attempts to detect the patch_format of the patches contained in `paths`,
+ * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if
+ * detection fails.
+ */
+static int detect_patch_format(struct string_list *paths)
+{
+ enum patch_format ret = PATCH_FORMAT_UNKNOWN;
+ struct strbuf l1 = STRBUF_INIT;
+ struct strbuf l2 = STRBUF_INIT;
+ struct strbuf l3 = STRBUF_INIT;
+ FILE *fp;
+
+ /*
+ * We default to mbox format if input is from stdin and for directories
+ */
+ if (!paths->nr || !strcmp(paths->items->string, "-") ||
+ is_directory(paths->items->string)) {
+ ret = PATCH_FORMAT_MBOX;
+ goto done;
+ }
+
+ /*
+ * Otherwise, check the first 3 lines of the first patch, starting
+ * from the first non-blank line, to try to detect its format.
+ */
+ fp = xfopen(paths->items->string, "r");
+ while (!strbuf_getline(&l1, fp, '\n')) {
+ strbuf_trim(&l1);
+ if (l1.len)
+ break;
+ }
+ strbuf_getline(&l2, fp, '\n');
We should test the return value of `strbuf_getline()`; if EOF was reached already, `strbuf_getwholeline()` does not touch the strbuf. I know, the strbuf is still initialized empty here, but it is too easy to forget when changing this code.
Hmm. We can test that earlier and return without reading from the file any further, I think.
+ else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
+ ret = PATCH_FORMAT_MBOX;
Maybe we can do better than this by folding the `is_email() function into this here function, reusing the same strbuf to read the lines and keeping track of the email header lines we saw... I would really like to avoid opening the same file twice just to figure out whether it is in email format.
The rest looks very nice!
Dscho
int fd, off_t offset);
extern void *xmmap_gently(void *start, size_t length, int prot, int
flags, int fd, off_t offset);
+extern int xopen(const char *path, int flags, ...);
I wonder whether it is worth it to make this a varargs function. It is not too much to ask callers to specify a specific mode everytime they call `xopen()`, no?
@@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)# endif#endif+/**+*xopen()isthesameasopen(),butitdie()siftheopen()fails.+*/+intxopen(constchar*path,intoflag,...)+{+mode_tmode=0;+va_listap;++va_start(ap,oflag);+if(oflag&O_CREAT)+mode=va_arg(ap,mode_t);+va_end(ap);++assert(path);++for(;;){+intfd=open(path,oflag,mode);+if(fd>=0)+returnfd;+if(errno==EINTR)+continue;+die_errno(_("could not open '%s'"),path);
It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like
if (oflag & O_WRITE)
die_errno(_("could not open '%s' for writing"), path);
else if (oflag & O_READ)
die_errno(_("could not open '%s' for reading"), path);
else
die_errno(_("could not open '%s'"), path);
? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.
Ciao,
Dscho
@@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb,
const char *file, size_t hint, int
}
/**
+ * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE
+ * in `value`. VALUE must be a quoted string, and the KEY must match `key`.
+ * Returns 0 on success, -1 on failure.
+ *
+ * This is used by read_author_script() to read the GIT_AUTHOR_* variables from
+ * the author-script.
+ */
+static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)
+{
+ struct strbuf sb = STRBUF_INIT;
+ char *str;
+
+ if (strbuf_getline(&sb, fp, '\n'))
+ return -1;
+
+ if (!skip_prefix(sb.buf, key, (const char **)&str))
+ return -1;
+
+ if (!skip_prefix(str, "=", (const char **)&str))
+ return -1;
+
+ str = sq_dequote(str);
+ if (!str)
+ return -1;
+
+ strbuf_reset(value);
+ strbuf_addstr(value, str);
+
+ strbuf_release(&sb);
+
+ return 0;
+}
How about using `strbuf_remove()` and keeping `str` as `const char *`? I also think we can fold it into the `read_author_script()` function and make it more resilient with regards to the order of the variables. Something like this:
static int read_author_script(struct am_state *state)
{
struct strbuf sb = STRBUF_INIT;
const char *filename = am_path(state, "author-script");
FILE *fp = fopen(filename, "r");
if (!fp) {
if (errno == ENOENT)
return 0;
die_errno(_("could not open '%s' for reading"), filename);
}
while (!strbuf_getline(&sb, fp, '\n')) {
char *equal = strchr(sb.buf, '='), **var;
if (!equal) {
error:
fclose(fp);
return -1;
}
*equal = '\0';
if (!strcmp(sb.buf, "GIT_AUTHOR_NAME"))
var = &state->author_name;
else if (!strcmp(sb.buf, "GIT_AUTHOR_EMAIL"))
var = &state->author_email;
else if (!strcmp(sb.buf, "GIT_AUTHOR_DATE"))
var = &state->author_date;
else
goto error;
*var = xstrdup(sq_dequote(equal + 1));
}
fclose(fp);
return -1;
}
If you follow my earlier suggestion to keep a strbuf inside the am_state, you could reuse that here, too.
+/**
+ * Saves state->author_name, state->author_email and state->author_date in
+ * `filename` as an "author script", which is the format used by git-am.sh.
+ */
+static void write_author_script(const struct am_state *state)
+{
+ static const char fmt[] = "GIT_AUTHOR_NAME=%s\n"
+ "GIT_AUTHOR_EMAIL=%s\n"
+ "GIT_AUTHOR_DATE=%s\n";
+ struct strbuf author_name = STRBUF_INIT;
+ struct strbuf author_email = STRBUF_INIT;
+ struct strbuf author_date = STRBUF_INIT;
+
+ sq_quote_buf(&author_name, state->author_name.buf);
+ sq_quote_buf(&author_email, state->author_email.buf);
+ sq_quote_buf(&author_date, state->author_date.buf);
The `sq_quote_buf()` function does not call `strbuf_reset()`. Therefore you could just use a single strbuf to construct the entire three lines and then write that out. Again, if you follow my suggestion to keep a "scratch pad" strbuf in am_state, you could reuse that.
That scratch pad could come in handy in a couple of other places in the rest of this patch.
Ciao,
Dscho
int fd, off_t offset);
extern void *xmmap_gently(void *start, size_t length, int prot, int
flags, int fd, off_t offset);
+extern int xopen(const char *path, int flags, ...);
I wonder whether it is worth it to make this a varargs function. It is not too much to ask callers to specify a specific mode everytime they call `xopen()`, no?
@@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)# endif#endif+/**+*xopen()isthesameasopen(),butitdie()siftheopen()fails.+*/+intxopen(constchar*path,intoflag,...)+{+mode_tmode=0;+va_listap;++va_start(ap,oflag);+if(oflag&O_CREAT)+mode=va_arg(ap,mode_t);+va_end(ap);++assert(path);++for(;;){+intfd=open(path,oflag,mode);+if(fd>=0)+returnfd;+if(errno==EINTR)+continue;+die_errno(_("could not open '%s'"),path);
It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like
if (oflag & O_WRITE)
die_errno(_("could not open '%s' for writing"), path);
else if (oflag & O_READ)
die_errno(_("could not open '%s' for reading"), path);
else
die_errno(_("could not open '%s'"), path);
? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.
From: Johannes Schindelin <hidden> Date: 2016-06-15 23:05:29
Hi Stefan,
On 2015-06-24 18:59, Stefan Beller wrote:
On Wed, Jun 24, 2015 at 9:28 AM, Johannes Schindelin
[off-list ref] wrote:
quoted
On 2015-06-18 13:25, Paul Tan wrote:
quoted
+ int fd = open(path, oflag, mode);
+ if (fd >= 0)
+ return fd;
+ if (errno == EINTR)
+ continue;
+ die_errno(_("could not open '%s'"), path);
It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like
if (oflag & O_WRITE)
die_errno(_("could not open '%s' for writing"), path);
else if (oflag & O_READ)
die_errno(_("could not open '%s' for reading"), path);
else
die_errno(_("could not open '%s'"), path);
? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.
So sorry that I missed that (it is still somewhere in my ever-growing inbox). I would have politely disagreed with Torsten if I had not missed it, though.
IMO the varargs make the code more cumbersome to read (and even fragile, because you can easily call `xopen(path, O_WRITE | O_CREATE)` and would not even get so much as a compiler warning!) and the error message does carry value: it helps you resolve the issue (it is completely unnecessary to check write permissions of the directory when a file could not be opened for reading, for example, yet if the error message does not say that and you suspect that the file could not be opened for *writing* that is exactly what you would waste your time checking).
Ciao,
Dscho
@@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)strbuf_release(&sb);}+/*+*Returns1ifthefilelookslikeapieceofemaila-laRFC2822,0otherwise.+*Wecheckthisbygrabbingallthenon-indentedlinesandseeingiftheylook+*liketheybeginwithvalidheaderfieldnames.+*/+staticintis_email(constchar*filename)+{+structstrbufsb=STRBUF_INIT;+FILE*fp=xfopen(filename,"r");+intret=1;++while(!strbuf_getline(&sb,fp,'\n')){+constchar*x;++strbuf_rtrim(&sb);++if(!sb.len)+break;/* End of header */++/* Ignore indented folded lines */+if(*sb.buf=='\t'||*sb.buf==' ')+continue;++/* It's a header if it matches the regexp "^[!-9;-~]+:" */
Why not just compile a regex and use it here? We use regexes elsewhere anyway...
Ah, you're right. A regular expression would definitely be clearer.
I've fixed it on my end.
quoted
+/**
+ * Attempts to detect the patch_format of the patches contained in `paths`,
+ * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if
+ * detection fails.
+ */
+static int detect_patch_format(struct string_list *paths)
+{
+ enum patch_format ret = PATCH_FORMAT_UNKNOWN;
+ struct strbuf l1 = STRBUF_INIT;
+ struct strbuf l2 = STRBUF_INIT;
+ struct strbuf l3 = STRBUF_INIT;
+ FILE *fp;
+
+ /*
+ * We default to mbox format if input is from stdin and for directories
+ */
+ if (!paths->nr || !strcmp(paths->items->string, "-") ||
+ is_directory(paths->items->string)) {
+ ret = PATCH_FORMAT_MBOX;
+ goto done;
+ }
+
+ /*
+ * Otherwise, check the first 3 lines of the first patch, starting
+ * from the first non-blank line, to try to detect its format.
+ */
+ fp = xfopen(paths->items->string, "r");
+ while (!strbuf_getline(&l1, fp, '\n')) {
+ strbuf_trim(&l1);
+ if (l1.len)
+ break;
+ }
+ strbuf_getline(&l2, fp, '\n');
We should test the return value of `strbuf_getline()`; if EOF was reached already, `strbuf_getwholeline()` does not touch the strbuf. I know, the strbuf is still initialized empty here, but it is too easy to forget when changing this code.
Ah OK. I'll vote for doing a strbuf_reset() just before the
strbuf_getline() though.
Hmm. We can test that earlier and return without reading from the file any further, I think.
The "reading 3 lines at the beginning" logic is meant to support a
later patch where support for StGit and mercurial patches is added.
That said, it's true that we don't need to read 3 lines in this patch,
so I think I will remove it in this patch.
quoted
+ else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
+ ret = PATCH_FORMAT_MBOX;
Maybe we can do better than this by folding the `is_email() function into this here function, reusing the same strbuf to read the lines and keeping track of the email header lines we saw... I would really like to avoid opening the same file twice just to figure out whether it is in email format.
Okay, how about every time we call a strbuf_getline(), we save the
line to a string_list as well? Like string_list_getline_crlf() below:
/**
* Like strbuf_getline(), but supports both '\n' and "\r\n" as line
* terminators.
*/
static int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)
{
if (strbuf_getwholeline(sb, fp, '\n'))
return EOF;
if (sb->buf[sb->len - 1] == '\n') {
strbuf_setlen(sb, sb->len - 1);
if (sb->len > 0 && sb->buf[sb->len - 1] == '\r')
strbuf_setlen(sb, sb->len - 1);
}
return 0;
}
/**
* Like strbuf_getline_crlf(), but appends the line to a string_list and
* returns it as a string. Returns NULL on EOF.
*/
static const char *string_list_getline_crlf(struct string_list *list, FILE *fp)
{
struct strbuf sb = STRBUF_INIT;
struct string_list_item *item;
if (strbuf_getline_crlf(&sb, fp))
return NULL;
item = string_list_append_nodup(list, strbuf_detach(&sb, NULL));
return item->string;
}
So now, is_email() can have access to previously-read lines, and if it
needs some more, it can read more as well:
static int is_email(struct string_list *lines, FILE *fp)
{
const char *header_regex = "^[!-9;-~]+:";
regex_t regex;
int ret = 1;
size_t i;
if (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED))
die("Invalid search pattern: %s", header_regex);
for (i = 0; i < lines->nr || string_list_getline_crlf(lines, fp); i++) {
const char *line = lines->items[i].string;
if (!*line)
break; /* End of header */
/* Ignore indented folded lines */
if (*line == '\t' || *line == ' ')
continue;
/* It's a header if it matches header_regex */
if (regexec(®ex, line, 0, NULL, 0)) {
ret = 0;
goto done;
}
}
done:
regfree(®ex);
return ret;
}
Which solves the problem of opening the file 2 times. What do you think?
Regards,
Paul
@@ -6,6 +6,158 @@#include"cache.h"#include"builtin.h"#include"exec_cmd.h"+#include"parse-options.h"+#include"dir.h"++structam_state{+/* state directory path */+structstrbufdir;++/* current and last patch numbers, 1-indexed */+intcur;+intlast;+};++/**+*Initializesam_statewiththedefaultvalues.+*/+staticvoidam_state_init(structam_state*state)+{+memset(state,0,sizeof(*state));++strbuf_init(&state->dir,0);+}
With strbufs, we use the initializer STRBUF_INIT. How about using
#define AM_STATE_INIT { STRBUF_INIT, 0, 0 }
here?
Later in the patch series am_state_init() will also take into account
config settings when filling up the default values. e.g. see patches
25/31[1] or 31/31[2]. I think that is reasonable: the purpose of
am_state_init() is to initialize the am_state struct with the default
values, and the default values can be set by the user through the
config settings.
This means, though, that we can't use initializers without introducing
global variables.
[1] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972
[2] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972
quoted
+/**
+ * Reads the contents of `file`. The third argument can be used to give a hint
+ * about the file size, to avoid reallocs. Returns number of bytes read on
+ * success, -1 if the file does not exist. If trim is set, trailing whitespace
+ * will be removed from the file contents.
+ */
+static int read_state_file(struct strbuf *sb, const char *file,
size_t hint, int trim)
+{
+ strbuf_reset(sb);
+ if (strbuf_read_file(sb, file, hint) >= 0) {
+ if (trim)
+ strbuf_trim(sb);
+
+ return sb->len;
+ }
+
+ if (errno == ENOENT)
+ return -1;
+
+ die_errno(_("could not read '%s'"), file);
+}
A couple of thoughts:
- why not reuse the strbuf by making it a part of the am_state()? That way, you can allocate, say, 1024 bytes (should be plenty enough for most of our operations) and just reuse them in all of the functions. We will not make any of this multi-threaded anyway, I don't think.
But too much usage of this temporary strbuf may lead to a situation
where one function calls another, and both functions write to the
strbuf and clobber its contents.
Besides, if we are talking about read_state_file(), it takes an
external strbuf, so it gives the caller the freedom to choose which
strbuf it uses (e.g. if it is stack allocated or in the am_state
struct). I think it's more flexible this way.
- Given that we only read short files all the time, why not skip the hint parameter? Especially if we reuse the strbuf, it should be good enough to allocate a reasonable buffer first and then just assume that we do not have to reallocate it all that often anyway.
Doh! Right, the hint parameter is quite useless, since in am_load() we
use the same strbuf anyway. (And strbuf_init() can set a hint as well)
I've removed it on my side.
- Since we only read files from the state directory, why not pass the basename as parameter? That way we can avoid calling `am_path()` explicitly over and over again (and yours truly cannot forget to call `am_path()` in future patches).
Makes sense. After all, this function is called read_STATE_file() ;-)
- If you agree with these suggestions, the signature would become something like
static void read_state_file(struct am_state *state, const char *basename, int trim);
So for now, my function signature is
static void read_state_file(struct strbuf *sb, const struct am_state
*state, const char *basename, int trim);
Given that `remove_dir_recursively()` has to reset the strbuf with the directory's path to the original value before it returns (because it recurses into itself, therefore the value *has* to be reset when returning), we can just call
remove_dir_recursively(&state->dir, 0);
and do not need another temporary strbuf.
Ah right. Although, state->dir is not an strbuf anymore on my side. As
Junio[3] rightfully noted, state->dir is not modified by the am_*()
API, so it's been changed to a char*. Which means an strbuf is
required to be passed to remove_dir_recursively();
quoted
+/**
+ * Increments the patch pointer, and cleans am_state for the application of the
+ * next patch.
+ */
+static void am_next(struct am_state *state)
+{
+ state->cur++;
+ write_file(am_path(state, "next"), 1, "%d", state->cur);
+}
Locking and re-checking the contents of "next" before writing the incremented value would probably be a little too paranoid...
Yeah, Junio did bring something like that[3]. I'm still thinking about
it, though I don't think I would like this issue to block the patch
series since it's a delicate issue, and git-am.sh does not do anything
special either.
For now though, I've moved all the write_file()s into a central
am_save() function, so if we want to do any locking or syncing it
would be easy to modify am_save(), and then all the callers will
benefit.
[3] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972
On Thu, Jun 25, 2015 at 9:40 PM, Paul Tan [off-list ref] wrote:
On Wed, Jun 24, 2015 at 11:10 PM, Johannes Schindelin
[off-list ref] wrote:
quoted
quoted
+ else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
+ ret = PATCH_FORMAT_MBOX;
Maybe we can do better than this by folding the `is_email() function into this here function, reusing the same strbuf to read the lines and keeping track of the email header lines we saw... I would really like to avoid opening the same file twice just to figure out whether it is in email format.
Okay, how about every time we call a strbuf_getline(), we save the
line to a string_list as well? Like string_list_getline_crlf() below:
[...]
Hmm, on second thought, I don't think it's worth the code complexity.
While I agree it's desirable to not open the file twice, I don't think
detecting the patch format is so IO intensive that it needs to be
optimized to that extent.
Instead, we should probably just modify is_email() to take a FILE*,
and then fseek(fp, 0L, SEEK_SET) to the beginning.
I think the logic of is_email() is complex and so it should not be
folded into the detect_patch_format() function, especially since we
may add detection of other patch formats in the future, and may need
more complex heuristics.
Regards,
Paul
@@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb,
const char *file, size_t hint, int
}
/**
+ * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE
+ * in `value`. VALUE must be a quoted string, and the KEY must match `key`.
+ * Returns 0 on success, -1 on failure.
+ *
+ * This is used by read_author_script() to read the GIT_AUTHOR_* variables from
+ * the author-script.
+ */
+static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)
+{
+ struct strbuf sb = STRBUF_INIT;
+ char *str;
+
+ if (strbuf_getline(&sb, fp, '\n'))
+ return -1;
+
+ if (!skip_prefix(sb.buf, key, (const char **)&str))
+ return -1;
+
+ if (!skip_prefix(str, "=", (const char **)&str))
+ return -1;
+
+ str = sq_dequote(str);
+ if (!str)
+ return -1;
+
+ strbuf_reset(value);
+ strbuf_addstr(value, str);
+
+ strbuf_release(&sb);
+
+ return 0;
+}
How about using `strbuf_remove()` and keeping `str` as `const char *`?
OK, I'll try that out. Looks like this now:
static char *read_shell_var(FILE *fp, const char *key)
{
struct strbuf sb = STRBUF_INIT;
const char *str;
if (strbuf_getline(&sb, fp, '\n'))
return NULL;
if (!skip_prefix(sb.buf, key, &str))
return NULL;
if (!skip_prefix(str, "=", &str))
return NULL;
strbuf_remove(&sb, 0, str - sb.buf);
str = sq_dequote(sb.buf);
if (!str)
return NULL;
return strbuf_detach(&sb, NULL);
}
I also think we can fold it into the `read_author_script()` function and make it more resilient with regards to the order of the variables. Something like this:
[...]
Hmm, I think we should be very strict about parsing the author-script
file though. As explained in read_author_script(), git-am.sh evals the
author-script, which we can't in C. I would much rather we barf at the
first sign that the author-script is not what we expect, rather than
attempt to parse it as much as possible, but end up with the wrong
results as compared to git-am.sh.
Besides, currently git-am.sh will always write the author-script with
the order of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE. If
the order is wrong, I would think it means that something is messing
with the author-script, and it would be better if we bail out
immediately, instead of potentially doing something wrong.
quoted
+/**
+ * Saves state->author_name, state->author_email and state->author_date in
+ * `filename` as an "author script", which is the format used by git-am.sh.
+ */
+static void write_author_script(const struct am_state *state)
+{
+ static const char fmt[] = "GIT_AUTHOR_NAME=%s\n"
+ "GIT_AUTHOR_EMAIL=%s\n"
+ "GIT_AUTHOR_DATE=%s\n";
+ struct strbuf author_name = STRBUF_INIT;
+ struct strbuf author_email = STRBUF_INIT;
+ struct strbuf author_date = STRBUF_INIT;
+
+ sq_quote_buf(&author_name, state->author_name.buf);
+ sq_quote_buf(&author_email, state->author_email.buf);
+ sq_quote_buf(&author_date, state->author_date.buf);
The `sq_quote_buf()` function does not call `strbuf_reset()`. Therefore you could just use a single strbuf to construct the entire three lines and then write that out. Again, if you follow my suggestion to keep a "scratch pad" strbuf in am_state, you could reuse that.
Right, makes sense. I've implemented it on my end.
Thanks,
Paul
On Thu, Jun 25, 2015 at 2:39 AM, Johannes Schindelin
[off-list ref] wrote:
IMO the varargs make the code more cumbersome to read (and even fragile, because you can easily call `xopen(path, O_WRITE | O_CREATE)` and would not even get so much as a compiler warning!)
I think that since xopen() is a wrapper around open(), it is best to
follow its interface (as defined by the POSIX spec) as much as
possible. It's important to note that the POSIX spec explicitly
defines that open() takes a variable number of arguments, and that the
`mode` argument is only used if O_CREAT is set. This means that if we
cement xopen() to take 3 arguments, and the third is a mode_t (or an
int), then we may not be able to keep up with changes in the POSIX
spec which e.g. in the future may specify that open() takes 4
arguments if certain flags are set.
and the error message does carry value: it helps you resolve the issue (it is completely unnecessary to check write permissions of the directory when a file could not be opened for reading, for example, yet if the error message does not say that and you suspect that the file could not be opened for *writing* that is exactly what you would waste your time checking).
Good point, I agree with this. I'll look into putting the error messages back.
Thanks,
Paul
On Wed, Jul 1, 2015 at 5:41 PM, Paul Tan [off-list ref] wrote:
Good point, I agree with this. I'll look into putting the error messages back.
This should work I think. It should take into account that O_RDONLY,
O_WRONLY, O_RDWR is defines as 0, 1, 2 on glibc, while the POSIX spec
also defines that O_RDONLY | O_WRONLY == O_RDWR.
@@ -214,7 +214,13 @@ int xopen(const char *path, int oflag, ...)returnfd;if(errno==EINTR)continue;-die_errno(_("could not open '%s'"),path);++if((oflag&O_RDWR)==O_RDWR)+die_errno(_("could not open '%s' for reading and writing"),path);+elseif((oflag&O_WRONLY)==O_WRONLY)+die_errno(_("could not open '%s' for writing"),path);+else+die_errno(_("could not open '%s' for reading"),path);}}
@@ -351,7 +357,13 @@ FILE *xfopen(const char *path, const char *mode)returnfp;if(errno==EINTR)continue;-die_errno(_("could not open '%s'"),path);++if(*mode&&mode[1]=='+')+die_errno(_("could not open '%s' for reading and writing"),path);+elseif(*mode=='w'||*mode=='a')+die_errno(_("could not open '%s' for writing"),path);+else+die_errno(_("could not open '%s' for reading"),path);}}