{"thread":{"id":"39670","subject":"[PATCH/WIP v3 01/31] wrapper: implement xopen()","startedAt":"2015-06-18T11:25:12Z","lastAt":"2015-07-01T09:53:49Z","messageCount":64,"participants":["Paul Tan","Stefan Beller","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":3,"patchTotal":31},"messages":[{"id":"264140","messageId":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":null,"subject":"[PATCH/WIP v3 00/31] Make git-am a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:12Z","receivedAt":"2015-06-18T11:25:12Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"This patch series depends on pt/pull-builtin for OPT_PASSTHRU_ARGV() and\nargv_array_pushv().\n\nThis is a re-roll of [WIP v3]. Thanks Junio and Stefan for the reviews last\nround.\n\nThe biggest addition this round would be the support for git-rebase. Here's\na small unscientific benchmark that rebases 50 patches:\n\n\tgit init &&\n\techo initial >file &&\n\tgit add file &&\n\tgit commit -m initial &&\n\tgit tag initial &&\n\tfor x in $(seq 50)\n\tdo\n\t echo $x >>file &&\n\t git commit -a -m $x\n\tdone &&\n\tgit checkout -b onto-rebase initial &&\n\tgit commit --allow-empty -mempty &&\n\ttime git rebase -q --onto onto-rebase initial master\n\nWith master:\n\n1.53s, 1.55s, 1.17s, 1.52s, 1.22s. Avg: ~1.40s\n\nWith master + this patch series:\n\n0.22s, 0.22s, 0.18s, 0.21s, 0.18s. Avg: ~0.20s\n\nSo this is around a 6-7x speedup.\n\nPrevious versions:\n\n[WIP v1] http://thread.gmane.org/gmane.comp.version-control.git/270048\n[WIP v2] http://thread.gmane.org/gmane.comp.version-control.git/271381\n\ngit-am is a commonly used command for applying a series of patches from a\nmailbox to the current branch. Currently, it is implemented by the shell script\ngit-am.sh. However, compared to C, shell scripts have certain deficiencies:\nthey need to spawn a lot of processes, introduce a lot of dependencies and\ncannot take advantage of git's internal caches.\n\nThis WIP patch series rewrites git-am.sh into optimized C builtin/am.c, and is\npart of my GSoC project to rewrite git-pull and git-am into C builtins[1].\n\n[1] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1\n\n\nPaul Tan (31):\n wrapper: implement xopen()\n wrapper: implement xfopen()\n am: implement skeletal builtin am\n am: implement patch queue mechanism\n am: split out mbox/maildir patches with git-mailsplit\n am: detect mbox patches\n am: extract patch, message and authorship with git-mailinfo\n am: apply patch with git-apply\n am: commit applied patch\n am: refresh the index at start\n am: refuse to apply patches if index is dirty\n am: implement --resolved/--continue\n am: implement --skip\n am: implement --abort\n am: don't accept patches when there's a session in progress\n am: implement quiet option\n am: exit with user friendly message on patch failure\n am: implement am --signoff\n cache-tree: introduce write_index_as_tree()\n am: implement 3-way merge\n am: --rebasing\n am: don't use git-mailinfo if --rebasing\n am: handle stray state directory\n am: implement -k/--keep, --keep-non-patch\n am: implement --[no-]message-id, am.messageid\n am: support --keep-cr, am.keepcr\n am: implement --[no-]scissors\n am: pass git-apply's options to git-apply\n am: implement --ignore-date\n am: implement --committer-date-is-author-date\n am: implement -S/--gpg-sign, commit.gpgsign\n\n Makefile | 1 +\n builtin.h | 1 +\n builtin/am.c | 1650 +++++++++++++++++++++++++++++++++++++++++++++++++++++\n cache-tree.c | 29 +-\n cache-tree.h | 1 +\n git-compat-util.h | 2 +\n git.c | 1 +\n wrapper.c | 43 ++\n 8 files changed, 1716 insertions(+), 12 deletions(-)\n create mode 100644 builtin/am.c\n\n-- \n2.1.4\n"},{"id":"264138","messageId":"1434626743-8552-2-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:13Z","receivedAt":"2015-06-18T11:25:13Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"A common usage pattern of open() is to check if it was successful, and\ndie() if it was not:\n\n\tint fd = open(path, O_WRONLY | O_CREAT, 0777);\n\tif (fd < 0)\n\t\tdie_errno(_(\"Could not open '%s' for writing.\"), path);\n\nImplement a wrapper function xopen() that does the above so that we can\nsave a few lines of code, and make the die() messages consistent.\n\nHelped-by: Torsten Bögershausen \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\n git-compat-util.h | 1 +\n wrapper.c | 25 +++++++++++++++++++++++++\n 2 files changed, 26 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 0cc7ae8..bc77d77 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -719,6 +719,7 @@ extern void *xrealloc(void *ptr, size_t size);\n extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n extern void *xmmap_gently(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n+extern int xopen(const char *path, int flags, ...);\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\ndiff --git a/wrapper.c b/wrapper.c\nindex c1a663f..82658b3 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)\n # endif\n #endif\n \n+/**\n+ * xopen() is the same as open(), but it die()s if the open() fails.\n+ */\n+int xopen(const char *path, int oflag, ...)\n+{\n+\tmode_t mode = 0;\n+\tva_list ap;\n+\n+\tva_start(ap, oflag);\n+\tif (oflag & O_CREAT)\n+\t\tmode = va_arg(ap, mode_t);\n+\tva_end(ap);\n+\n+\tassert(path);\n+\n+\tfor (;;) {\n+\t\tint fd = open(path, oflag, mode);\n+\t\tif (fd >= 0)\n+\t\t\treturn fd;\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\tdie_errno(_(\"could not open '%s'\"), path);\n+\t}\n+}\n+\n /*\n * xread() is the same a read(), but it automatically restarts read()\n * operations with a recoverable error (EAGAIN and EINTR). xread()\n-- \n2.1.4\n"},{"id":"264139","messageId":"1434626743-8552-3-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 02/31] wrapper: implement xfopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:14Z","receivedAt":"2015-06-18T11:25:14Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"A common usage pattern of fopen() is to check if it succeeded, and die()\nif it failed:\n\n\tFILE *fp = fopen(path, \"w\");\n\tif (!fp)\n\t\tdie_errno(_(\"could not open '%s' for writing\"), path);\n\nImplement a wrapper function xfopen() for the above, so that we can save\na few lines of code and make the die() messages consistent.\n\nSigned-off-by: Paul Tan \n---\n git-compat-util.h | 1 +\n wrapper.c | 18 ++++++++++++++++++\n 2 files changed, 19 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex bc77d77..4e69110 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -724,6 +724,7 @@ extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);\n extern int xdup(int fd);\n+extern FILE *xfopen(const char *path, const char *mode);\n extern FILE *xfdopen(int fd, const char *mode);\n extern int xmkstemp(char *template);\n extern int xmkstemp_mode(char *template, int mode);\ndiff --git a/wrapper.c b/wrapper.c\nindex 82658b3..9692460 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -336,6 +336,24 @@ int xdup(int fd)\n \treturn ret;\n }\n \n+/**\n+ * xfopen() is the same as fopen(), but it die()s if the fopen() fails.\n+ */\n+FILE *xfopen(const char *path, const char *mode)\n+{\n+\tassert(path);\n+\tassert(mode);\n+\n+\tfor (;;) {\n+\t\tFILE *fp = fopen(path, mode);\n+\t\tif (fp)\n+\t\t\treturn fp;\n+\t\tif (errno == EINTR)\n+\t\t\tcontinue;\n+\t\tdie_errno(_(\"could not open '%s'\"), path);\n+\t}\n+}\n+\n FILE *xfdopen(int fd, const char *mode)\n {\n \tFILE *stream = fdopen(fd, mode);\n-- \n2.1.4\n"},{"id":"264142","messageId":"1434626743-8552-4-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 03/31] am: implement skeletal builtin am","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:15Z","receivedAt":"2015-06-18T11:25:15Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"For the purpose of rewriting git-am.sh into a C builtin, implement a\nskeletal builtin/am.c that redirects to $GIT_EXEC_PATH/git-am if the\nenvironment variable _GIT_USE_BUILTIN_AM is not defined. Since in the\nMakefile git-am.sh takes precedence over builtin/am.c,\n$GIT_EXEC_PATH/git-am will contain the shell script git-am.sh, and thus\nthis allows us to fall back on the functional git-am.sh when running the\ntest suite for tests that depend on a working git-am implementation.\n\nSince git-am.sh cannot handle any environment modifications by\nsetup_git_directory(), \"am\" has to be declared as NO_SETUP in git.c. On\nthe other hand, to re-implement git-am.sh in builtin/am.c, we do need to\nrun all the git dir and work tree setup logic that git.c does for us. As\nsuch, we work around this temporarily by copying the logic in git.c's\nrun_builtin(), which amounts to:\n\n\tprefix = setup_git_directory();\n\ttrace_repo_setup(prefix);\n\tsetup_work_tree();\n\nThis redirection should be removed when all the features of git-am.sh\nhave been re-implemented in builtin/am.c.\n\nHelped-by: Junio C Hamano \nSigned-off-by: Paul Tan \n---\n\nNotes:\n v3\n \n * Style fixes\n \n * git-am.sh cannot handle the chdir() and GIT_DIR envionment variable\n that setup_git_directory() sets, so we work around it by copying the\n logic of git.c's run_builtin(), and running it only when we are using\n the builtin am.\n\n Makefile | 1 +\n builtin.h | 1 +\n builtin/am.c | 28 ++++++++++++++++++++++++++++\n git.c | 1 +\n 4 files changed, 31 insertions(+)\n create mode 100644 builtin/am.c\n\ndiff --git a/Makefile b/Makefile\nindex 93e4fa2..ff9bdc0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -811,6 +811,7 @@ LIB_OBJS += xdiff-interface.o\n LIB_OBJS += zlib.o\n \n BUILTIN_OBJS += builtin/add.o\n+BUILTIN_OBJS += builtin/am.o\n BUILTIN_OBJS += builtin/annotate.o\n BUILTIN_OBJS += builtin/apply.o\n BUILTIN_OBJS += builtin/archive.o\ndiff --git a/builtin.h b/builtin.h\nindex ea3c834..f30cf00 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, const unsigned char\n extern int is_builtin(const char *s);\n \n extern int cmd_add(int argc, const char **argv, const char *prefix);\n+extern int cmd_am(int argc, const char **argv, const char *prefix);\n extern int cmd_annotate(int argc, const char **argv, const char *prefix);\n extern int cmd_apply(int argc, const char **argv, const char *prefix);\n extern int cmd_archive(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/am.c b/builtin/am.c\nnew file mode 100644\nindex 0000000..dbc8836\n--- /dev/null\n+++ b/builtin/am.c\n@@ -0,0 +1,28 @@\n+/*\n+ * Builtin \"git am\"\n+ *\n+ * Based on git-am.sh by Junio C Hamano.\n+ */\n+#include \"cache.h\"\n+#include \"builtin.h\"\n+#include \"exec_cmd.h\"\n+\n+int cmd_am(int argc, const char **argv, const char *prefix)\n+{\n+\t/*\n+\t * FIXME: Once all the features of git-am.sh have been re-implemented\n+\t * in builtin/am.c, this preamble can be removed.\n+\t */\n+\tif (!getenv(\"_GIT_USE_BUILTIN_AM\")) {\n+\t\tconst char *path = mkpath(\"%s/git-am\", git_exec_path());\n+\n+\t\tif (sane_execvp(path, (char **)argv) < 0)\n+\t\t\tdie_errno(\"could not exec %s\", path);\n+\t} else {\n+\t\tprefix = setup_git_directory();\n+\t\ttrace_repo_setup(prefix);\n+\t\tsetup_work_tree();\n+\t}\n+\n+\treturn 0;\n+}\ndiff --git a/git.c b/git.c\nindex e7a7713..a671535 100644\n--- a/git.c\n+++ b/git.c\n@@ -370,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \n static struct cmd_struct commands[] = {\n \t{ \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n+\t{ \"am\", cmd_am, NO_SETUP },\n \t{ \"annotate\", cmd_annotate, RUN_SETUP },\n \t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n \t{ \"archive\", cmd_archive },\n-- \n2.1.4\n"},{"id":"264145","messageId":"1434626743-8552-5-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:16Z","receivedAt":"2015-06-18T11:25:16Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"git-am applies a series of patches. If the process terminates\nabnormally, we want to be able to resume applying the series of patches.\nThis requires the session state to be saved in a persistent location.\n\nImplement the mechanism of a \"patch queue\", represented by 2 integers --\nthe index of the current patch we are applying and the index of the last\npatch, as well as its lifecycle through the following functions:\n\n* am_setup(), which will set up the state directory\n $GIT_DIR/rebase-apply. As such, even if the process exits abnormally,\n the last-known state will still persist.\n\n* am_load(), which is called if there is an am session in\n progress, to load the last known state from the state directory so we\n can resume applying patches.\n\n* am_run(), which will do the actual patch application. After applying a\n patch, it calls am_next() to increment the current patch index. The\n logic for applying and committing a patch is not implemented yet.\n\n* am_destroy(), which is finally called when we successfully applied all\n the patches in the queue, to clean up by removing the state directory\n and its contents.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 168 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex dbc8836..af68c51 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -6,6 +6,158 @@\n #include \"cache.h\"\n #include \"builtin.h\"\n #include \"exec_cmd.h\"\n+#include \"parse-options.h\"\n+#include \"dir.h\"\n+\n+struct am_state {\n+\t/* state directory path */\n+\tstruct strbuf dir;\n+\n+\t/* current and last patch numbers, 1-indexed */\n+\tint cur;\n+\tint last;\n+};\n+\n+/**\n+ * Initializes am_state with the default values.\n+ */\n+static void am_state_init(struct am_state *state)\n+{\n+\tmemset(state, 0, sizeof(*state));\n+\n+\tstrbuf_init(&state->dir, 0);\n+}\n+\n+/**\n+ * Release memory allocated by an am_state.\n+ */\n+static void am_state_release(struct am_state *state)\n+{\n+\tstrbuf_release(&state->dir);\n+}\n+\n+/**\n+ * Returns path relative to the am_state directory.\n+ */\n+static inline const char *am_path(const struct am_state *state, const char *path)\n+{\n+\treturn mkpath(\"%s/%s\", state->dir.buf, path);\n+}\n+\n+/**\n+ * Returns 1 if there is an am session in progress, 0 otherwise.\n+ */\n+static int am_in_progress(const struct am_state *state)\n+{\n+\tstruct stat st;\n+\n+\tif (lstat(state->dir.buf, &st) < 0 || !S_ISDIR(st.st_mode))\n+\t\treturn 0;\n+\tif (lstat(am_path(state, \"last\"), &st) || !S_ISREG(st.st_mode))\n+\t\treturn 0;\n+\tif (lstat(am_path(state, \"next\"), &st) || !S_ISREG(st.st_mode))\n+\t\treturn 0;\n+\treturn 1;\n+}\n+\n+/**\n+ * Reads the contents of `file`. The third argument can be used to give a hint\n+ * about the file size, to avoid reallocs. Returns number of bytes read on\n+ * success, -1 if the file does not exist. If trim is set, trailing whitespace\n+ * will be removed from the file contents.\n+ */\n+static int read_state_file(struct strbuf *sb, const char *file, size_t hint, int trim)\n+{\n+\tstrbuf_reset(sb);\n+\tif (strbuf_read_file(sb, file, hint) >= 0) {\n+\t\tif (trim)\n+\t\t\tstrbuf_trim(sb);\n+\n+\t\treturn sb->len;\n+\t}\n+\n+\tif (errno == ENOENT)\n+\t\treturn -1;\n+\n+\tdie_errno(_(\"could not read '%s'\"), file);\n+}\n+\n+/**\n+ * Loads state from disk.\n+ */\n+static void am_load(struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tread_state_file(&sb, am_path(state, \"next\"), 8, 1);\n+\tstate->cur = strtol(sb.buf, NULL, 10);\n+\n+\tread_state_file(&sb, am_path(state, \"last\"), 8, 1);\n+\tstate->last = strtol(sb.buf, NULL, 10);\n+\n+\tstrbuf_release(&sb);\n+}\n+\n+/**\n+ * Remove the am_state directory.\n+ */\n+static void am_destroy(const struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_addstr(&sb, state->dir.buf);\n+\tremove_dir_recursively(&sb, 0);\n+\tstrbuf_release(&sb);\n+}\n+\n+/**\n+ * Setup a new am session for applying patches\n+ */\n+static void am_setup(struct am_state *state)\n+{\n+\tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n+\t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n+\n+\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\n+\twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n+}\n+\n+/**\n+ * Increments the patch pointer, and cleans am_state for the application of the\n+ * next patch.\n+ */\n+static void am_next(struct am_state *state)\n+{\n+\tstate->cur++;\n+\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+}\n+\n+/**\n+ * Applies all queued patches.\n+ */\n+static void am_run(struct am_state *state)\n+{\n+\twhile (state->cur <= state->last) {\n+\n+\t\t/* TODO: Patch application not implemented yet */\n+\n+\t\tam_next(state);\n+\t}\n+\n+\tam_destroy(state);\n+}\n+\n+static struct am_state state;\n+\n+static const char * const am_usage[] = {\n+\tN_(\"git am [options] [(|)...]\"),\n+\tNULL\n+};\n+\n+static struct option am_options[] = {\n+\tOPT_END()\n+};\n \n int cmd_am(int argc, const char **argv, const char *prefix)\n {\n@@ -24,5 +176,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tsetup_work_tree();\n \t}\n \n+\tgit_config(git_default_config, NULL);\n+\n+\tam_state_init(&state);\n+\tstrbuf_addstr(&state.dir, git_path(\"rebase-apply\"));\n+\n+\targc = parse_options(argc, argv, prefix, am_options, am_usage, 0);\n+\n+\tif (am_in_progress(&state))\n+\t\tam_load(&state);\n+\telse\n+\t\tam_setup(&state);\n+\n+\tam_run(&state);\n+\n+\tam_state_release(&state);\n+\n \treturn 0;\n }\n-- \n2.1.4\n"},{"id":"264141","messageId":"1434626743-8552-6-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 05/31] am: split out mbox/maildir patches with git-mailsplit","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:17Z","receivedAt":"2015-06-18T11:25:17Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"git-am.sh supports mbox, stgit and mercurial patches. Re-implement\nsupport for splitting out mbox/maildirs using git-mailsplit, while also\nimplementing the framework required to support other patch formats in\nthe future.\n\nRe-implement support for the --patch-format option (since a5a6755\n(git-am foreign patch support: introduce patch_format, 2009-05-27)) to\nallow the user to choose between the different patch formats.\n\nSigned-off-by: Paul Tan \n---\n\nNotes:\n v3\n \n * Moved the TODO comment to the previous patch\n\n builtin/am.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 101 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex af68c51..e9a3687 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -8,6 +8,12 @@\n #include \"exec_cmd.h\"\n #include \"parse-options.h\"\n #include \"dir.h\"\n+#include \"run-command.h\"\n+\n+enum patch_format {\n+\tPATCH_FORMAT_UNKNOWN = 0,\n+\tPATCH_FORMAT_MBOX\n+};\n \n struct am_state {\n \t/* state directory path */\n@@ -16,6 +22,9 @@ struct am_state {\n \t/* current and last patch numbers, 1-indexed */\n \tint cur;\n \tint last;\n+\n+\t/* number of digits in patch filename */\n+\tint prec;\n };\n \n /**\n@@ -26,6 +35,8 @@ static void am_state_init(struct am_state *state)\n \tmemset(state, 0, sizeof(*state));\n \n \tstrbuf_init(&state->dir, 0);\n+\n+\tstate->prec = 4;\n }\n \n /**\n@@ -111,13 +122,69 @@ static void am_destroy(const struct am_state *state)\n }\n \n /**\n+ * Splits out individual patches from `paths`, where each path is either a mbox\n+ * file or a Maildir. Return 0 on success, -1 on failure.\n+ */\n+static int split_patches_mbox(struct am_state *state, struct string_list *paths)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct string_list_item *item;\n+\tstruct strbuf last = STRBUF_INIT;\n+\n+\tcp.git_cmd = 1;\n+\targv_array_push(&cp.args, \"mailsplit\");\n+\targv_array_pushf(&cp.args, \"-d%d\", state->prec);\n+\targv_array_pushf(&cp.args, \"-o%s\", state->dir.buf);\n+\targv_array_push(&cp.args, \"-b\");\n+\targv_array_push(&cp.args, \"--\");\n+\n+\tfor_each_string_list_item(item, paths)\n+\t\targv_array_push(&cp.args, item->string);\n+\n+\tif (capture_command(&cp, &last, 8))\n+\t\treturn -1;\n+\n+\tstate->cur = 1;\n+\tstate->last = strtol(last.buf, NULL, 10);\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Splits out individual patches, of patch_format, contained within paths.\n+ * These patches will be stored in the state directory, with each patch's\n+ * filename being its index, padded to state->prec digits. state->cur will be\n+ * set to the index of the first patch, and state->last will be set to the\n+ * index of the last patch.\n+ *\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int split_patches(struct am_state *state, enum patch_format patch_format,\n+\t\tstruct string_list *paths)\n+{\n+\tswitch (patch_format) {\n+\tcase PATCH_FORMAT_MBOX:\n+\t\treturn split_patches_mbox(state, paths);\n+\tdefault:\n+\t\tdie(\"BUG: invalid patch_format\");\n+\t}\n+\treturn -1;\n+}\n+\n+/**\n * Setup a new am session for applying patches\n */\n-static void am_setup(struct am_state *state)\n+static void am_setup(struct am_state *state, enum patch_format patch_format,\n+\t\tstruct string_list *paths)\n {\n \tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n \t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n \n+\tif (split_patches(state, patch_format, paths) < 0) {\n+\t\tam_destroy(state);\n+\t\tdie(_(\"Failed to split patches.\"));\n+\t}\n+\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n \n \twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n@@ -148,7 +215,23 @@ static void am_run(struct am_state *state)\n \tam_destroy(state);\n }\n \n+/**\n+ * parse_options() callback that validates and sets opt->value to the\n+ * PATCH_FORMAT_* enum value corresponding to `arg`.\n+ */\n+static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)\n+{\n+\tint *opt_value = opt->value;\n+\n+\tif (!strcmp(arg, \"mbox\"))\n+\t\t*opt_value = PATCH_FORMAT_MBOX;\n+\telse\n+\t\treturn -1;\n+\treturn 0;\n+}\n+\n static struct am_state state;\n+static int opt_patch_format;\n \n static const char * const am_usage[] = {\n \tN_(\"git am [options] [(|)...]\"),\n@@ -156,6 +239,8 @@ static const char * const am_usage[] = {\n };\n \n static struct option am_options[] = {\n+\tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n+\t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_END()\n };\n \n@@ -185,8 +270,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \tif (am_in_progress(&state))\n \t\tam_load(&state);\n-\telse\n-\t\tam_setup(&state);\n+\telse {\n+\t\tstruct string_list paths = STRING_LIST_INIT_DUP;\n+\t\tint i;\n+\n+\t\tfor (i = 0; i < argc; i++) {\n+\t\t\tif (is_absolute_path(argv[i]) || !prefix)\n+\t\t\t\tstring_list_append(&paths, argv[i]);\n+\t\t\telse\n+\t\t\t\tstring_list_append(&paths, mkpath(\"%s/%s\", prefix, argv[i]));\n+\t\t}\n+\n+\t\tam_setup(&state, opt_patch_format, &paths);\n+\n+\t\tstring_list_clear(&paths, 0);\n+\t}\n \n \tam_run(&state);\n \n-- \n2.1.4\n"},{"id":"264143","messageId":"1434626743-8552-7-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:18Z","receivedAt":"2015-06-18T11:25:18Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 15ced75 (git-am foreign patch support: autodetect some patch\nformats, 2009-05-27), git-am.sh is able to autodetect mbox, stgit and\nmercurial patches through heuristics.\n\nRe-implement support for autodetecting mbox/maildir files.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Paul Tan \n---\n builtin/am.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 98 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e9a3687..7b97ea8 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)\n \tstrbuf_release(&sb);\n }\n \n+/*\n+ * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.\n+ * We check this by grabbing all the non-indented lines and seeing if they look\n+ * like they begin with valid header field names.\n+ */\n+static int is_email(const char *filename)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tFILE *fp = xfopen(filename, \"r\");\n+\tint ret = 1;\n+\n+\twhile (!strbuf_getline(&sb, fp, '\\n')) {\n+\t\tconst char *x;\n+\n+\t\tstrbuf_rtrim(&sb);\n+\n+\t\tif (!sb.len)\n+\t\t\tbreak; /* End of header */\n+\n+\t\t/* Ignore indented folded lines */\n+\t\tif (*sb.buf == '\\t' || *sb.buf == ' ')\n+\t\t\tcontinue;\n+\n+\t\t/* It's a header if it matches the regexp \"^[!-9;-~]+:\" */\n+\t\tfor (x = sb.buf; *x; x++) {\n+\t\t\tif (('!' <= *x && *x <= '9') || (';' <= *x && *x <= '~'))\n+\t\t\t\tcontinue;\n+\t\t\tif (*x == ':' && x != sb.buf)\n+\t\t\t\tbreak;\n+\t\t\tret = 0;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+done:\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\n+\treturn ret;\n+}\n+\n+/**\n+ * Attempts to detect the patch_format of the patches contained in `paths`,\n+ * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if\n+ * detection fails.\n+ */\n+static int detect_patch_format(struct string_list *paths)\n+{\n+\tenum patch_format ret = PATCH_FORMAT_UNKNOWN;\n+\tstruct strbuf l1 = STRBUF_INIT;\n+\tstruct strbuf l2 = STRBUF_INIT;\n+\tstruct strbuf l3 = STRBUF_INIT;\n+\tFILE *fp;\n+\n+\t/*\n+\t * We default to mbox format if input is from stdin and for directories\n+\t */\n+\tif (!paths->nr || !strcmp(paths->items->string, \"-\") ||\n+\t is_directory(paths->items->string)) {\n+\t\tret = PATCH_FORMAT_MBOX;\n+\t\tgoto done;\n+\t}\n+\n+\t/*\n+\t * Otherwise, check the first 3 lines of the first patch, starting\n+\t * from the first non-blank line, to try to detect its format.\n+\t */\n+\tfp = xfopen(paths->items->string, \"r\");\n+\twhile (!strbuf_getline(&l1, fp, '\\n')) {\n+\t\tstrbuf_trim(&l1);\n+\t\tif (l1.len)\n+\t\t\tbreak;\n+\t}\n+\tstrbuf_getline(&l2, fp, '\\n');\n+\tstrbuf_trim(&l2);\n+\tstrbuf_getline(&l3, fp, '\\n');\n+\tstrbuf_trim(&l3);\n+\tfclose(fp);\n+\n+\tif (starts_with(l1.buf, \"From \") || starts_with(l1.buf, \"From: \"))\n+\t\tret = PATCH_FORMAT_MBOX;\n+\telse if (l1.len && l2.len && l3.len && is_email(paths->items->string))\n+\t\tret = PATCH_FORMAT_MBOX;\n+\n+done:\n+\tstrbuf_release(&l1);\n+\tstrbuf_release(&l2);\n+\tstrbuf_release(&l3);\n+\treturn ret;\n+}\n+\n /**\n * Splits out individual patches from `paths`, where each path is either a mbox\n * file or a Maildir. Return 0 on success, -1 on failure.\n@@ -177,6 +267,14 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,\n static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tstruct string_list *paths)\n {\n+\tif (!patch_format)\n+\t\tpatch_format = detect_patch_format(paths);\n+\n+\tif (!patch_format) {\n+\t\tfprintf_ln(stderr, _(\"Patch format detection failed.\"));\n+\t\texit(128);\n+\t}\n+\n \tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n \t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n \n-- \n2.1.4\n"},{"id":"264146","messageId":"1434626743-8552-8-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:19Z","receivedAt":"2015-06-18T11:25:19Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"For the purpose of applying the patch and committing the results,\nimplement extracting the patch data, commit message and authorship from\nan e-mail message using git-mailinfo.\n\ngit-mailinfo is run as a separate process, but ideally in the future,\nwe should be be able to access its functionality directly without\nspawning a new process.\n\nHelped-by: Junio C Hamano \nHelped-by: Jeff King \nSigned-off-by: Paul Tan \n---\nNotes:\n v3\n \n * Style fixes\n\n builtin/am.c | 232 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 232 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7b97ea8..d6434e4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -9,6 +9,23 @@\n #include \"parse-options.h\"\n #include \"dir.h\"\n #include \"run-command.h\"\n+#include \"quote.h\"\n+\n+/**\n+ * Returns 1 if the file is empty or does not exist, 0 otherwise.\n+ */\n+static int is_empty_file(const char *filename)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(filename, &st) < 0) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 1;\n+\t\tdie_errno(_(\"could not stat %s\"), filename);\n+\t}\n+\n+\treturn !st.st_size;\n+}\n \n enum patch_format {\n \tPATCH_FORMAT_UNKNOWN = 0,\n@@ -23,6 +40,12 @@ struct am_state {\n \tint cur;\n \tint last;\n \n+\t/* commit message and metadata */\n+\tstruct strbuf author_name;\n+\tstruct strbuf author_email;\n+\tstruct strbuf author_date;\n+\tstruct strbuf msg;\n+\n \t/* number of digits in patch filename */\n \tint prec;\n };\n@@ -36,6 +59,11 @@ static void am_state_init(struct am_state *state)\n \n \tstrbuf_init(&state->dir, 0);\n \n+\tstrbuf_init(&state->author_name, 0);\n+\tstrbuf_init(&state->author_email, 0);\n+\tstrbuf_init(&state->author_date, 0);\n+\tstrbuf_init(&state->msg, 0);\n+\n \tstate->prec = 4;\n }\n \n@@ -45,6 +73,10 @@ static void am_state_init(struct am_state *state)\n static void am_state_release(struct am_state *state)\n {\n \tstrbuf_release(&state->dir);\n+\tstrbuf_release(&state->author_name);\n+\tstrbuf_release(&state->author_email);\n+\tstrbuf_release(&state->author_date);\n+\tstrbuf_release(&state->msg);\n }\n \n /**\n@@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb, const char *file, size_t hint, int\n }\n \n /**\n+ * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE\n+ * in `value`. VALUE must be a quoted string, and the KEY must match `key`.\n+ * Returns 0 on success, -1 on failure.\n+ *\n+ * This is used by read_author_script() to read the GIT_AUTHOR_* variables from\n+ * the author-script.\n+ */\n+static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *str;\n+\n+\tif (strbuf_getline(&sb, fp, '\\n'))\n+\t\treturn -1;\n+\n+\tif (!skip_prefix(sb.buf, key, (const char **)&str))\n+\t\treturn -1;\n+\n+\tif (!skip_prefix(str, \"=\", (const char **)&str))\n+\t\treturn -1;\n+\n+\tstr = sq_dequote(str);\n+\tif (!str)\n+\t\treturn -1;\n+\n+\tstrbuf_reset(value);\n+\tstrbuf_addstr(value, str);\n+\n+\tstrbuf_release(&sb);\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Parses the \"author script\" `filename`, and sets state->author_name,\n+ * state->author_email and state->author_date accordingly. We are strict with\n+ * our parsing, as the author script is supposed to be eval'd, and loosely\n+ * parsing it may not give the results the user expects.\n+ *\n+ * The author script is of the format:\n+ *\n+ *\tGIT_AUTHOR_NAME='$author_name'\n+ *\tGIT_AUTHOR_EMAIL='$author_email'\n+ *\tGIT_AUTHOR_DATE='$author_date'\n+ *\n+ * where $author_name, $author_email and $author_date are quoted.\n+ */\n+static int read_author_script(struct am_state *state)\n+{\n+\tconst char *filename = am_path(state, \"author-script\");\n+\tFILE *fp = fopen(filename, \"r\");\n+\tif (!fp) {\n+\t\tif (errno == ENOENT)\n+\t\t\treturn 0;\n+\t\tdie_errno(_(\"could not open '%s' for reading\"), filename);\n+\t}\n+\n+\tif (read_shell_var(&state->author_name, fp, \"GIT_AUTHOR_NAME\"))\n+\t\treturn -1;\n+\n+\tif (read_shell_var(&state->author_email, fp, \"GIT_AUTHOR_EMAIL\"))\n+\t\treturn -1;\n+\n+\tif (read_shell_var(&state->author_date, fp, \"GIT_AUTHOR_DATE\"))\n+\t\treturn -1;\n+\n+\tif (fgetc(fp) != EOF)\n+\t\treturn -1;\n+\n+\tfclose(fp);\n+\treturn 0;\n+}\n+\n+/**\n+ * Saves state->author_name, state->author_email and state->author_date in\n+ * `filename` as an \"author script\", which is the format used by git-am.sh.\n+ */\n+static void write_author_script(const struct am_state *state)\n+{\n+\tstatic const char fmt[] = \"GIT_AUTHOR_NAME=%s\\n\"\n+\t\t\"GIT_AUTHOR_EMAIL=%s\\n\"\n+\t\t\"GIT_AUTHOR_DATE=%s\\n\";\n+\tstruct strbuf author_name = STRBUF_INIT;\n+\tstruct strbuf author_email = STRBUF_INIT;\n+\tstruct strbuf author_date = STRBUF_INIT;\n+\n+\tsq_quote_buf(&author_name, state->author_name.buf);\n+\tsq_quote_buf(&author_email, state->author_email.buf);\n+\tsq_quote_buf(&author_date, state->author_date.buf);\n+\n+\twrite_file(am_path(state, \"author-script\"), 1, fmt,\n+\t\t\tauthor_name.buf, author_email.buf, author_date.buf);\n+\n+\tstrbuf_release(&author_name);\n+\tstrbuf_release(&author_email);\n+\tstrbuf_release(&author_date);\n+}\n+\n+/**\n * Loads state from disk.\n */\n static void am_load(struct am_state *state)\n@@ -106,6 +237,11 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"last\"), 8, 1);\n \tstate->last = strtol(sb.buf, NULL, 10);\n \n+\tif (read_author_script(state) < 0)\n+\t\tdie(_(\"could not parse author script\"));\n+\n+\tread_state_file(&state->msg, am_path(state, \"final-commit\"), 0, 0);\n+\n \tstrbuf_release(&sb);\n }\n \n@@ -296,6 +432,91 @@ static void am_next(struct am_state *state)\n {\n \tstate->cur++;\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n+\n+\tstrbuf_reset(&state->author_name);\n+\tstrbuf_reset(&state->author_email);\n+\tstrbuf_reset(&state->author_date);\n+\tunlink(am_path(state, \"author-script\"));\n+\n+\tstrbuf_reset(&state->msg);\n+\tunlink(am_path(state, \"final-commit\"));\n+}\n+\n+/**\n+ * Returns the filename of the current patch.\n+ */\n+static const char *msgnum(const struct am_state *state)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, \"%0*d\", state->prec, state->cur);\n+\n+\treturn sb.buf;\n+}\n+\n+/**\n+ * Parses `patch` using git-mailinfo. state->msg will be set to the patch\n+ * message. state->author_name, state->author_email, state->author_date will be\n+ * set to the patch author's name, email and date respectively. The patch's\n+ * body will be written to \"$state_dir/patch\", where $state_dir is the state\n+ * directory.\n+ *\n+ * Returns 1 if the patch should be skipped, 0 otherwise.\n+ */\n+static int parse_patch(struct am_state *state, const char *patch)\n+{\n+\tFILE *fp;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tcp.git_cmd = 1;\n+\tcp.in = xopen(patch, O_RDONLY, 0);\n+\tcp.out = xopen(am_path(state, \"info\"), O_WRONLY | O_CREAT, 0777);\n+\n+\targv_array_push(&cp.args, \"mailinfo\");\n+\targv_array_push(&cp.args, am_path(state, \"msg\"));\n+\targv_array_push(&cp.args, am_path(state, \"patch\"));\n+\n+\tif (run_command(&cp) < 0)\n+\t\tdie(\"could not parse patch\");\n+\n+\tclose(cp.in);\n+\tclose(cp.out);\n+\n+\t/* Extract message and author information */\n+\tfp = xfopen(am_path(state, \"info\"), \"r\");\n+\twhile (!strbuf_getline(&sb, fp, '\\n')) {\n+\t\tconst char *x;\n+\n+\t\tif (skip_prefix(sb.buf, \"Subject: \", &x)) {\n+\t\t\tif (state->msg.len)\n+\t\t\t\tstrbuf_addch(&state->msg, '\\n');\n+\t\t\tstrbuf_addstr(&state->msg, x);\n+\t\t} else if (skip_prefix(sb.buf, \"Author: \", &x))\n+\t\t\tstrbuf_addstr(&state->author_name, x);\n+\t\telse if (skip_prefix(sb.buf, \"Email: \", &x))\n+\t\t\tstrbuf_addstr(&state->author_email, x);\n+\t\telse if (skip_prefix(sb.buf, \"Date: \", &x))\n+\t\t\tstrbuf_addstr(&state->author_date, x);\n+\t}\n+\tfclose(fp);\n+\n+\t/* Skip pine's internal folder data */\n+\tif (!strcmp(state->author_name.buf, \"Mail System Internal Data\"))\n+\t\treturn 1;\n+\n+\tif (is_empty_file(am_path(state, \"patch\")))\n+\t\tdie(_(\"Patch is empty. Was it split wrong?\\n\"\n+\t\t\"If you would prefer to skip this patch, instead run \\\"git am --skip\\\".\\n\"\n+\t\t\"To restore the original branch and stop patching run \\\"git am --abort\\\".\"));\n+\n+\tstrbuf_addstr(&state->msg, \"\\n\\n\");\n+\tif (strbuf_read_file(&state->msg, am_path(state, \"msg\"), 0) < 0)\n+\t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n+\tstripspace(&state->msg, 0);\n+\n+\treturn 0;\n }\n \n /**\n@@ -304,9 +525,20 @@ static void am_next(struct am_state *state)\n static void am_run(struct am_state *state)\n {\n \twhile (state->cur <= state->last) {\n+\t\tconst char *patch = am_path(state, msgnum(state));\n+\n+\t\tif (!file_exists(patch))\n+\t\t\tgoto next;\n+\n+\t\tif (parse_patch(state, patch))\n+\t\t\tgoto next; /* patch should be skipped */\n+\n+\t\twrite_author_script(state);\n+\t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n \n \t\t/* TODO: Patch application not implemented yet */\n \n+next:\n \t\tam_next(state);\n \t}\n \n-- \n2.1.4\n"},{"id":"264144","messageId":"1434626743-8552-9-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 08/31] am: apply patch with git-apply","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:20Z","receivedAt":"2015-06-18T11:25:20Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Implement applying the patch to the index using git-apply.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 56 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d6434e4..296a5fc 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -27,6 +27,18 @@ static int is_empty_file(const char *filename)\n \treturn !st.st_size;\n }\n \n+/**\n+ * Returns the first line of msg\n+ */\n+static const char *firstline(const char *msg)\n+{\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_add(&sb, msg, strchrnul(msg, '\\n') - msg);\n+\treturn sb.buf;\n+}\n+\n enum patch_format {\n \tPATCH_FORMAT_UNKNOWN = 0,\n \tPATCH_FORMAT_MBOX\n@@ -519,6 +531,31 @@ static int parse_patch(struct am_state *state, const char *patch)\n \treturn 0;\n }\n \n+/*\n+ * Applies current patch with git-apply. Returns 0 on success, -1 otherwise.\n+ */\n+static int run_apply(const struct am_state *state)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\n+\targv_array_push(&cp.args, \"apply\");\n+\n+\targv_array_push(&cp.args, \"--index\");\n+\n+\targv_array_push(&cp.args, am_path(state, \"patch\"));\n+\n+\tif (run_command(&cp))\n+\t\treturn -1;\n+\n+\t/* Reload index as git-apply will have modified it. */\n+\tdiscard_cache();\n+\tread_cache();\n+\n+\treturn 0;\n+}\n+\n /**\n * Applies all queued patches.\n */\n@@ -536,7 +573,25 @@ static void am_run(struct am_state *state)\n \t\twrite_author_script(state);\n \t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n \n-\t\t/* TODO: Patch application not implemented yet */\n+\t\tprintf_ln(_(\"Applying: %s\"), firstline(state->msg.buf));\n+\n+\t\tif (run_apply(state) < 0) {\n+\t\t\tint value;\n+\n+\t\t\tprintf_ln(_(\"Patch failed at %s %s\"), msgnum(state),\n+\t\t\t\t\tfirstline(state->msg.buf));\n+\n+\t\t\tif (!git_config_get_bool(\"advice.amworkdir\", &value) && !value)\n+\t\t\t\tprintf_ln(_(\"The copy of the patch that failed is found in: %s\"),\n+\t\t\t\t\t\tam_path(state, \"patch\"));\n+\n+\t\t\texit(128);\n+\t\t}\n+\n+\t\t/*\n+\t\t * TODO: After the patch has been applied to the index with\n+\t\t * git-apply, we need to make commit as well.\n+\t\t */\n \n next:\n \t\tam_next(state);\n-- \n2.1.4\n"},{"id":"264147","messageId":"1434626743-8552-10-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 09/31] am: commit applied patch","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:21Z","receivedAt":"2015-06-18T11:25:21Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Implement do_commit(), which commits the index which contains the\nresults of applying the patch, along with the extracted commit message\nand authorship information.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++----\n 1 file changed, 47 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 296a5fc..dfb6f7e 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -10,6 +10,9 @@\n #include \"dir.h\"\n #include \"run-command.h\"\n #include \"quote.h\"\n+#include \"cache-tree.h\"\n+#include \"refs.h\"\n+#include \"commit.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -557,6 +560,49 @@ static int run_apply(const struct am_state *state)\n }\n \n /**\n+ * Commits the current index with state->msg as the commit message and\n+ * state->author_name, state->author_email and state->author_date as the author\n+ * information.\n+ */\n+static void do_commit(const struct am_state *state)\n+{\n+\tunsigned char tree[GIT_SHA1_RAWSZ], parent[GIT_SHA1_RAWSZ],\n+\t\t commit[GIT_SHA1_RAWSZ];\n+\tunsigned char *ptr;\n+\tstruct commit_list *parents = NULL;\n+\tconst char *reflog_msg, *author;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tif (write_cache_as_tree(tree, 0, NULL))\n+\t\tdie(_(\"git write-tree failed to write a tree\"));\n+\n+\tif (!get_sha1_commit(\"HEAD\", parent)) {\n+\t\tptr = parent;\n+\t\tcommit_list_insert(lookup_commit(parent), &parents);\n+\t} else {\n+\t\tptr = NULL;\n+\t\tfprintf_ln(stderr, _(\"applying to an empty history\"));\n+\t}\n+\n+\tauthor = fmt_ident(state->author_name.buf, state->author_email.buf,\n+\t\t\tstate->author_date.buf, IDENT_STRICT);\n+\n+\tif (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,\n+\t\t\t\tauthor, NULL))\n+\t\tdie(_(\"failed to write commit object\"));\n+\n+\treflog_msg = getenv(\"GIT_REFLOG_ACTION\");\n+\tif (!reflog_msg)\n+\t\treflog_msg = \"am\";\n+\n+\tstrbuf_addf(&sb, \"%s: %s\", reflog_msg, firstline(state->msg.buf));\n+\n+\tupdate_ref(sb.buf, \"HEAD\", commit, ptr, 0, UPDATE_REFS_DIE_ON_ERR);\n+\n+\tstrbuf_release(&sb);\n+}\n+\n+/**\n * Applies all queued patches.\n */\n static void am_run(struct am_state *state)\n@@ -588,10 +634,7 @@ static void am_run(struct am_state *state)\n \t\t\texit(128);\n \t\t}\n \n-\t\t/*\n-\t\t * TODO: After the patch has been applied to the index with\n-\t\t * git-apply, we need to make commit as well.\n-\t\t */\n+\t\tdo_commit(state);\n \n next:\n \t\tam_next(state);\n-- \n2.1.4\n"},{"id":"264148","messageId":"1434626743-8552-11-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 10/31] am: refresh the index at start","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:22Z","receivedAt":"2015-06-18T11:25:22Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"If a file is unchanged but stat-dirty, git-apply may erroneously fail to\napply patches, thinking that they conflict with a dirty working tree.\n\nAs such, since 2a6f08a (am: refresh the index at start and --resolved,\n2011-08-15), git-am will refresh the index before applying patches.\nRe-implement this behavior.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 20 ++++++++++++++++++++\n 1 file changed, 20 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex dfb6f7e..a7efe85 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -13,6 +13,7 @@\n #include \"cache-tree.h\"\n #include \"refs.h\"\n #include \"commit.h\"\n+#include \"lockfile.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -471,6 +472,20 @@ static const char *msgnum(const struct am_state *state)\n }\n \n /**\n+ * Refresh and write index.\n+ */\n+static void refresh_and_write_cache(void)\n+{\n+\tstatic struct lock_file lock_file;\n+\n+\thold_locked_index(&lock_file, 1);\n+\trefresh_cache(REFRESH_QUIET);\n+\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n+\t\tdie(_(\"unable to write index file\"));\n+\trollback_lock_file(&lock_file);\n+}\n+\n+/**\n * Parses `patch` using git-mailinfo. state->msg will be set to the patch\n * message. state->author_name, state->author_email, state->author_date will be\n * set to the patch author's name, email and date respectively. The patch's\n@@ -607,6 +622,8 @@ static void do_commit(const struct am_state *state)\n */\n static void am_run(struct am_state *state)\n {\n+\trefresh_and_write_cache();\n+\n \twhile (state->cur <= state->last) {\n \t\tconst char *patch = am_path(state, msgnum(state));\n \n@@ -696,6 +713,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, am_options, am_usage, 0);\n \n+\tif (read_index_preload(&the_index, NULL) < 0)\n+\t\tdie(_(\"failed to read the index\"));\n+\n \tif (am_in_progress(&state))\n \t\tam_load(&state);\n \telse {\n-- \n2.1.4\n"},{"id":"264156","messageId":"1434626743-8552-12-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 11/31] am: refuse to apply patches if index is dirty","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:23Z","receivedAt":"2015-06-18T11:25:23Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am\nwill refuse to apply patches if the index is dirty. Re-implement this\nbehavior.\n\nSigned-off-by: Paul Tan \n---\n\nNotes:\n Note: no tests for this\n\n builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex a7efe85..9d6ab2a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -14,6 +14,8 @@\n #include \"refs.h\"\n #include \"commit.h\"\n #include \"lockfile.h\"\n+#include \"diff.h\"\n+#include \"diffcore.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -486,6 +488,43 @@ static void refresh_and_write_cache(void)\n }\n \n /**\n+ * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn\n+ * branch, returns 1 if there are entries in the index, 0 otherwise. If an\n+ * strbuf is provided, the space-separated list of files that differ will be\n+ * appended to it.\n+ */\n+static int index_has_changes(struct strbuf *sb)\n+{\n+\tunsigned char head[GIT_SHA1_RAWSZ];\n+\tint i;\n+\n+\tif (!get_sha1_tree(\"HEAD\", head)) {\n+\t\tstruct diff_options opt;\n+\n+\t\tdiff_setup(&opt);\n+\t\tDIFF_OPT_SET(&opt, EXIT_WITH_STATUS);\n+\t\tif (!sb)\n+\t\t\tDIFF_OPT_SET(&opt, QUICK);\n+\t\tdo_diff_cache(head, &opt);\n+\t\tdiffcore_std(&opt);\n+\t\tfor (i = 0; sb && i < diff_queued_diff.nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, diff_queued_diff.queue[i]->two->path);\n+\t\t}\n+\t\tdiff_flush(&opt);\n+\t\treturn DIFF_OPT_TST(&opt, HAS_CHANGES) != 0;\n+\t} else {\n+\t\tfor (i = 0; sb && i < active_nr; i++) {\n+\t\t\tif (i)\n+\t\t\t\tstrbuf_addch(sb, ' ');\n+\t\t\tstrbuf_addstr(sb, active_cache[i]->name);\n+\t\t}\n+\t\treturn !!active_nr;\n+\t}\n+}\n+\n+/**\n * Parses `patch` using git-mailinfo. state->msg will be set to the patch\n * message. state->author_name, state->author_email, state->author_date will be\n * set to the patch author's name, email and date respectively. The patch's\n@@ -622,8 +661,15 @@ static void do_commit(const struct am_state *state)\n */\n static void am_run(struct am_state *state)\n {\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n \trefresh_and_write_cache();\n \n+\tif (index_has_changes(&sb))\n+\t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n+\n+\tstrbuf_release(&sb);\n+\n \twhile (state->cur <= state->last) {\n \t\tconst char *patch = am_path(state, msgnum(state));\n \n-- \n2.1.4\n"},{"id":"264150","messageId":"1434626743-8552-13-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 12/31] am: implement --resolved/--continue","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:24Z","receivedAt":"2015-06-18T11:25:24Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 0c15cc9 (git-am: --resolved., 2005-11-16), git-am supported\nresuming from a failed patch application. The user will manually apply\nthe patch, and the run git am --resolved which will then commit the\nresulting index. Re-implement this feature by introducing am_resolve().\n\nSince it makes no sense for the user to run am --resolved when there is\nno session in progress, we error out in this case.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 54 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 9d6ab2a..4381164 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -707,6 +707,34 @@ next:\n }\n \n /**\n+ * Resume the current am session after patch application failure. The user did\n+ * all the hard work, and we do not have to do any patch application. Just\n+ * trust and commit what the user has in the index and working tree.\n+ */\n+static void am_resolve(struct am_state *state)\n+{\n+\tprintf_ln(_(\"Applying: %s\"), firstline(state->msg.buf));\n+\n+\tif (!index_has_changes(NULL)) {\n+\t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n+\t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n+\t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\n+\t\texit(128);\n+\t}\n+\n+\tif (unmerged_cache()) {\n+\t\tprintf_ln(_(\"You still have unmerged paths in your index.\\n\"\n+\t\t\t\"Did you forget to use 'git add'?\"));\n+\t\texit(128);\n+\t}\n+\n+\tdo_commit(state);\n+\n+\tam_next(state);\n+\tam_run(state);\n+}\n+\n+/**\n * parse_options() callback that validates and sets opt->value to the\n * PATCH_FORMAT_* enum value corresponding to `arg`.\n */\n@@ -721,17 +749,30 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n \treturn 0;\n }\n \n+enum resume_mode {\n+\tRESUME_FALSE = 0,\n+\tRESUME_RESOLVED\n+};\n+\n static struct am_state state;\n static int opt_patch_format;\n+static enum resume_mode opt_resume;\n \n static const char * const am_usage[] = {\n \tN_(\"git am [options] [(|)...]\"),\n+\tN_(\"git am [options] --continue\"),\n \tNULL\n };\n \n static struct option am_options[] = {\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n+\tOPT_CMDMODE(0, \"continue\", &opt_resume,\n+\t\tN_(\"continue applying patches after resolving a conflict\"),\n+\t\tRESUME_RESOLVED),\n+\tOPT_CMDMODE('r', \"resolved\", &opt_resume,\n+\t\tN_(\"synonyms for --continue\"),\n+\t\tRESUME_RESOLVED),\n \tOPT_END()\n };\n \n@@ -768,6 +809,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tstruct string_list paths = STRING_LIST_INIT_DUP;\n \t\tint i;\n \n+\t\tif (opt_resume)\n+\t\t\tdie(_(\"Resolve operation not in progress, we are not resuming.\"));\n+\n \t\tfor (i = 0; i < argc; i++) {\n \t\t\tif (is_absolute_path(argv[i]) || !prefix)\n \t\t\t\tstring_list_append(&paths, argv[i]);\n@@ -780,7 +824,16 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&paths, 0);\n \t}\n \n-\tam_run(&state);\n+\tswitch (opt_resume) {\n+\tcase RESUME_FALSE:\n+\t\tam_run(&state);\n+\t\tbreak;\n+\tcase RESUME_RESOLVED:\n+\t\tam_resolve(&state);\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: invalid resume value\");\n+\t}\n \n \tam_state_release(&state);\n \n-- \n2.1.4\n"},{"id":"264149","messageId":"1434626743-8552-14-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 13/31] am: implement --skip","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:25Z","receivedAt":"2015-06-18T11:25:25Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am\nsupported resuming from a failed patch application by skipping the\ncurrent patch. Re-implement this feature by introducing am_skip().\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 119 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4381164..0d7af19 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -16,6 +16,8 @@\n #include \"lockfile.h\"\n #include \"diff.h\"\n #include \"diffcore.h\"\n+#include \"unpack-trees.h\"\n+#include \"branch.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -735,6 +737,114 @@ static void am_resolve(struct am_state *state)\n }\n \n /**\n+ * Performs a checkout fast-forward from `head` to `remote`. If `reset` is\n+ * true, any unmerged entries will be discarded. Returns 0 on success, -1 on\n+ * failure.\n+ */\n+static int fast_forward_to(struct tree *head, struct tree *remote, int reset)\n+{\n+\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct unpack_trees_options opts;\n+\tstruct tree_desc t[2];\n+\n+\tif (parse_tree(head) || parse_tree(remote))\n+\t\treturn -1;\n+\n+\thold_locked_index(lock_file, 1);\n+\n+\trefresh_cache(REFRESH_QUIET);\n+\n+\tmemset(&opts, 0, sizeof(opts));\n+\topts.head_idx = 1;\n+\topts.src_index = &the_index;\n+\topts.dst_index = &the_index;\n+\topts.update = 1;\n+\topts.merge = 1;\n+\topts.reset = reset;\n+\topts.fn = twoway_merge;\n+\tinit_tree_desc(&t[0], head->buffer, head->size);\n+\tinit_tree_desc(&t[1], remote->buffer, remote->size);\n+\n+\tif (unpack_trees(2, t, &opts)) {\n+\t\trollback_lock_file(lock_file);\n+\t\treturn -1;\n+\t}\n+\n+\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tdie(_(\"unable to write new index file\"));\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Clean the index without touching entries that are not modified between\n+ * `head` and `remote`.\n+ */\n+static int clean_index(const unsigned char *head, const unsigned char *remote)\n+{\n+\tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n+\tstruct tree *head_tree, *remote_tree, *index_tree;\n+\tunsigned char index[GIT_SHA1_RAWSZ];\n+\tstruct pathspec pathspec;\n+\n+\thead_tree = parse_tree_indirect(head);\n+\tif (!head_tree)\n+\t\treturn error(_(\"Could not parse object '%s'.\"), sha1_to_hex(head));\n+\n+\tremote_tree = parse_tree_indirect(remote);\n+\tif (!remote_tree)\n+\t\treturn error(_(\"Could not parse object '%s'.\"), sha1_to_hex(remote));\n+\n+\tread_cache_unmerged();\n+\n+\tif (fast_forward_to(head_tree, head_tree, 1))\n+\t\treturn -1;\n+\n+\tif (write_cache_as_tree(index, 0, NULL))\n+\t\treturn -1;\n+\n+\tindex_tree = parse_tree_indirect(index);\n+\tif (!index_tree)\n+\t\treturn error(_(\"Could not parse object '%s'.\"), sha1_to_hex(index));\n+\n+\tif (fast_forward_to(index_tree, remote_tree, 0))\n+\t\treturn -1;\n+\n+\tmemset(&pathspec, 0, sizeof(pathspec));\n+\n+\thold_locked_index(lock_file, 1);\n+\n+\tif (read_tree(remote_tree, 0, &pathspec)) {\n+\t\trollback_lock_file(lock_file);\n+\t\treturn -1;\n+\t}\n+\n+\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\tdie(_(\"unable to write new index file\"));\n+\n+\tremove_branch_state();\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Resume the current am session by skipping the current patch.\n+ */\n+static void am_skip(struct am_state *state)\n+{\n+\tunsigned char head[GIT_SHA1_RAWSZ];\n+\n+\tif (get_sha1(\"HEAD\", head))\n+\t\thashcpy(head, EMPTY_TREE_SHA1_BIN);\n+\n+\tif (clean_index(head, head))\n+\t\tdie(_(\"failed to clean index\"));\n+\n+\tam_next(state);\n+\tam_run(state);\n+}\n+\n+/**\n * parse_options() callback that validates and sets opt->value to the\n * PATCH_FORMAT_* enum value corresponding to `arg`.\n */\n@@ -751,7 +861,8 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n \n enum resume_mode {\n \tRESUME_FALSE = 0,\n-\tRESUME_RESOLVED\n+\tRESUME_RESOLVED,\n+\tRESUME_SKIP\n };\n \n static struct am_state state;\n@@ -760,7 +871,7 @@ static enum resume_mode opt_resume;\n \n static const char * const am_usage[] = {\n \tN_(\"git am [options] [(|)...]\"),\n-\tN_(\"git am [options] --continue\"),\n+\tN_(\"git am [options] (--continue | --skip)\"),\n \tNULL\n };\n \n@@ -773,6 +884,9 @@ static struct option am_options[] = {\n \tOPT_CMDMODE('r', \"resolved\", &opt_resume,\n \t\tN_(\"synonyms for --continue\"),\n \t\tRESUME_RESOLVED),\n+\tOPT_CMDMODE(0, \"skip\", &opt_resume,\n+\t\tN_(\"skip the current patch\"),\n+\t\tRESUME_SKIP),\n \tOPT_END()\n };\n \n@@ -831,6 +945,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tcase RESUME_RESOLVED:\n \t\tam_resolve(&state);\n \t\tbreak;\n+\tcase RESUME_SKIP:\n+\t\tam_skip(&state);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: invalid resume value\");\n \t}\n-- \n2.1.4\n"},{"id":"264151","messageId":"1434626743-8552-15-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 14/31] am: implement --abort","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:26Z","receivedAt":"2015-06-18T11:25:26Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 3e5057a (git am --abort, 2008-07-16), git-am supported the --abort\noption that will rewind HEAD back to the original commit. Re-implement\nthis feature through am_abort().\n\nSince 7b3b7e3 (am --abort: keep unrelated commits since the last failure\nand warn, 2010-12-21), to prevent commits made since the last failure\nfrom being lost, git-am will not rewind HEAD back to the original\ncommit if HEAD moved since the last failure. Re-implement this through\nsafe_to_abort().\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 92 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0d7af19..7053b8f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -423,6 +423,8 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,\n static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tstruct string_list *paths)\n {\n+\tunsigned char curr_head[GIT_SHA1_RAWSZ];\n+\n \tif (!patch_format)\n \t\tpatch_format = detect_patch_format(paths);\n \n@@ -442,6 +444,14 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n \n \twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n+\n+\tif (!get_sha1(\"HEAD\", curr_head)) {\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n+\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n+\t} else {\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n+\t\tdelete_ref(\"ORIG_HEAD\", NULL, 0);\n+\t}\n }\n \n /**\n@@ -450,6 +460,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n */\n static void am_next(struct am_state *state)\n {\n+\tunsigned char head[GIT_SHA1_RAWSZ];\n+\n \tstate->cur++;\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n \n@@ -460,6 +472,11 @@ static void am_next(struct am_state *state)\n \n \tstrbuf_reset(&state->msg);\n \tunlink(am_path(state, \"final-commit\"));\n+\n+\tif (!get_sha1(\"HEAD\", head))\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(head));\n+\telse\n+\t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n }\n \n /**\n@@ -665,10 +682,14 @@ static void am_run(struct am_state *state)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \n+\tunlink(am_path(state, \"dirtyindex\"));\n+\n \trefresh_and_write_cache();\n \n-\tif (index_has_changes(&sb))\n+\tif (index_has_changes(&sb)) {\n+\t\twrite_file(am_path(state, \"dirtyindex\"), 1, \"t\");\n \t\tdie(_(\"Dirty index: cannot apply patches (dirty: %s)\"), sb.buf);\n+\t}\n \n \tstrbuf_release(&sb);\n \n@@ -844,6 +865,67 @@ static void am_skip(struct am_state *state)\n \tam_run(state);\n }\n \n+static int safe_to_abort(const struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tunsigned char abort_safety[GIT_SHA1_RAWSZ], head[GIT_SHA1_RAWSZ];\n+\n+\tif (file_exists(am_path(state, \"dirtyindex\")))\n+\t\treturn 0;\n+\n+\tif (read_state_file(&sb, am_path(state, \"abort-safety\"), 40, 1) > 0) {\n+\t\tif (get_sha1_hex(sb.buf, abort_safety))\n+\t\t\tdie(_(\"could not parse %s\"), am_path(state, \"abort_safety\"));\n+\t} else\n+\t\thashclr(abort_safety);\n+\n+\tif (get_sha1(\"HEAD\", head))\n+\t\thashclr(head);\n+\n+\tif (!hashcmp(head, abort_safety))\n+\t\treturn 1;\n+\n+\terror(_(\"You seem to have moved HEAD since the last 'am' failure.\\n\"\n+\t\t\"Not rewinding to ORIG_HEAD\"));\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Aborts the current am session if it is safe to do so.\n+ */\n+static void am_abort(struct am_state *state)\n+{\n+\tunsigned char curr_head[GIT_SHA1_RAWSZ], orig_head[GIT_SHA1_RAWSZ];\n+\tint has_curr_head, has_orig_head;\n+\tconst char *curr_branch;\n+\n+\tif (!safe_to_abort(state)) {\n+\t\tam_destroy(state);\n+\t\treturn;\n+\t}\n+\n+\tcurr_branch = resolve_refdup(\"HEAD\", 0, curr_head, NULL);\n+\thas_curr_head = !is_null_sha1(curr_head);\n+\tif (!has_curr_head)\n+\t\thashcpy(curr_head, EMPTY_TREE_SHA1_BIN);\n+\n+\thas_orig_head = !get_sha1(\"ORIG_HEAD\", orig_head);\n+\tif (!has_orig_head)\n+\t\thashcpy(orig_head, EMPTY_TREE_SHA1_BIN);\n+\n+\tclean_index(curr_head, orig_head);\n+\n+\tif (has_orig_head)\n+\t\tupdate_ref(\"am --abort\", \"HEAD\", orig_head,\n+\t\t\t\thas_curr_head ? curr_head : NULL, 0,\n+\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n+\telse if (curr_branch)\n+\t\tdelete_ref(curr_branch, NULL, REF_NODEREF);\n+\n+\tam_destroy(state);\n+}\n+\n /**\n * parse_options() callback that validates and sets opt->value to the\n * PATCH_FORMAT_* enum value corresponding to `arg`.\n@@ -862,7 +944,8 @@ static int parse_opt_patchformat(const struct option *opt, const char *arg, int\n enum resume_mode {\n \tRESUME_FALSE = 0,\n \tRESUME_RESOLVED,\n-\tRESUME_SKIP\n+\tRESUME_SKIP,\n+\tRESUME_ABORT\n };\n \n static struct am_state state;\n@@ -871,7 +954,7 @@ static enum resume_mode opt_resume;\n \n static const char * const am_usage[] = {\n \tN_(\"git am [options] [(|)...]\"),\n-\tN_(\"git am [options] (--continue | --skip)\"),\n+\tN_(\"git am [options] (--continue | --skip | --abort)\"),\n \tNULL\n };\n \n@@ -887,6 +970,9 @@ static struct option am_options[] = {\n \tOPT_CMDMODE(0, \"skip\", &opt_resume,\n \t\tN_(\"skip the current patch\"),\n \t\tRESUME_SKIP),\n+\tOPT_CMDMODE(0, \"abort\", &opt_resume,\n+\t\tN_(\"restore the original branch and abort the patching operation.\"),\n+\t\tRESUME_ABORT),\n \tOPT_END()\n };\n \n@@ -948,6 +1034,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tcase RESUME_SKIP:\n \t\tam_skip(&state);\n \t\tbreak;\n+\tcase RESUME_ABORT:\n+\t\tam_abort(&state);\n+\t\tbreak;\n \tdefault:\n \t\tdie(\"BUG: invalid resume value\");\n \t}\n-- \n2.1.4\n"},{"id":"264152","messageId":"1434626743-8552-16-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 15/31] am: don't accept patches when there's a session in progress","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:27Z","receivedAt":"2015-06-18T11:25:27Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am\nwould error out if the user gave it mbox(s) on the command-line, but\nthere was a session in progress.\n\nSince c95b138 (Fix git-am safety checks, 2006-09-15), git-am would\ndetect if the user attempted to feed it a mbox via stdin, by checking if\nstdin is not a tty and there is no resume command given.\n\nRe-implement the above two safety checks.\n\nSigned-off-by: Paul Tan \n---\n\nNotes:\n NOTE: there's no test for this\n\n builtin/am.c | 19 +++++++++++++++++--\n 1 file changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7053b8f..4adc487 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1003,9 +1003,24 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\tdie(_(\"failed to read the index\"));\n \n-\tif (am_in_progress(&state))\n+\tif (am_in_progress(&state)) {\n+\t\t/*\n+\t\t * Catch user error to feed us patches when there is a session\n+\t\t * in progress:\n+\t\t *\n+\t\t * 1. mbox path(s) are provided on the command-line.\n+\t\t * 2. stdin is not a tty: the user is trying to feed us a patch\n+\t\t * from standard input. This is somewhat unreliable -- stdin\n+\t\t * could be /dev/null for example and the caller did not\n+\t\t * intend to feed us a patch but wanted to continue\n+\t\t * unattended.\n+\t\t */\n+\t\tif (argc || (!opt_resume && !isatty(0)))\n+\t\t\tdie(_(\"previous rebase directory %s still exists but mbox given.\"),\n+\t\t\t\tstate.dir.buf);\n+\n \t\tam_load(&state);\n-\telse {\n+\t} else {\n \t\tstruct string_list paths = STRING_LIST_INIT_DUP;\n \t\tint i;\n \n-- \n2.1.4\n"},{"id":"264154","messageId":"1434626743-8552-17-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 16/31] am: implement quiet option","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:28Z","receivedAt":"2015-06-18T11:25:28Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 0e987a1 (am, rebase: teach quiet option, 2009-06-16), git-am\nsupported the --quiet option and GIT_QUIET environment variable, and\nwhen told to be quiet, would only speak on failure. Re-implement this by\nintroducing the say() function, which works like fprintf_ln(), but would\nonly write to the stream when state->quiet is false.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 36 +++++++++++++++++++++++++++++++++---\n 1 file changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4adc487..5f38264 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -68,6 +68,8 @@ struct am_state {\n \n \t/* number of digits in patch filename */\n \tint prec;\n+\n+\tint quiet;\n };\n \n /**\n@@ -75,6 +77,8 @@ struct am_state {\n */\n static void am_state_init(struct am_state *state)\n {\n+\tconst char *quiet;\n+\n \tmemset(state, 0, sizeof(*state));\n \n \tstrbuf_init(&state->dir, 0);\n@@ -85,6 +89,10 @@ static void am_state_init(struct am_state *state)\n \tstrbuf_init(&state->msg, 0);\n \n \tstate->prec = 4;\n+\n+\tquiet = getenv(\"GIT_QUIET\");\n+\tif (quiet && *quiet)\n+\t\tstate->quiet = 1;\n }\n \n /**\n@@ -108,6 +116,22 @@ static inline const char *am_path(const struct am_state *state, const char *path\n }\n \n /**\n+ * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline\n+ * at the end.\n+ */\n+static void say(const struct am_state *state, FILE *fp, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\n+\tva_start(ap, fmt);\n+\tif (!state->quiet) {\n+\t\tvfprintf(fp, fmt, ap);\n+\t\tputc('\\n', fp);\n+\t}\n+\tva_end(ap);\n+}\n+\n+/**\n * Returns 1 if there is an am session in progress, 0 otherwise.\n */\n static int am_in_progress(const struct am_state *state)\n@@ -262,6 +286,9 @@ static void am_load(struct am_state *state)\n \n \tread_state_file(&state->msg, am_path(state, \"final-commit\"), 0, 0);\n \n+\tread_state_file(&sb, am_path(state, \"quiet\"), 2, 1);\n+\tstate->quiet = !strcmp(sb.buf, \"t\");\n+\n \tstrbuf_release(&sb);\n }\n \n@@ -445,6 +472,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n \n+\twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n+\n \tif (!get_sha1(\"HEAD\", curr_head)) {\n \t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n \t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n@@ -654,7 +683,7 @@ static void do_commit(const struct am_state *state)\n \t\tcommit_list_insert(lookup_commit(parent), &parents);\n \t} else {\n \t\tptr = NULL;\n-\t\tfprintf_ln(stderr, _(\"applying to an empty history\"));\n+\t\tsay(state, stderr, _(\"applying to an empty history\"));\n \t}\n \n \tauthor = fmt_ident(state->author_name.buf, state->author_email.buf,\n@@ -705,7 +734,7 @@ static void am_run(struct am_state *state)\n \t\twrite_author_script(state);\n \t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n \n-\t\tprintf_ln(_(\"Applying: %s\"), firstline(state->msg.buf));\n+\t\tsay(state, stdout, _(\"Applying: %s\"), firstline(state->msg.buf));\n \n \t\tif (run_apply(state) < 0) {\n \t\t\tint value;\n@@ -736,7 +765,7 @@ next:\n */\n static void am_resolve(struct am_state *state)\n {\n-\tprintf_ln(_(\"Applying: %s\"), firstline(state->msg.buf));\n+\tsay(state, stdout, _(\"Applying: %s\"), firstline(state->msg.buf));\n \n \tif (!index_has_changes(NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n@@ -959,6 +988,7 @@ static const char * const am_usage[] = {\n };\n \n static struct option am_options[] = {\n+\tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_CMDMODE(0, \"continue\", &opt_resume,\n-- \n2.1.4\n"},{"id":"264153","messageId":"1434626743-8552-18-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 17/31] am: exit with user friendly message on patch failure","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:29Z","receivedAt":"2015-06-18T11:25:29Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since ced9456 (Give the user a hint for how to continue in the case that\ngit-am fails because it requires user intervention, 2006-05-02), git-am\nprints additional information on how the user can re-invoke git-am to\nresume patch application after resolving the failure. Re-implement this\nthrough the die_user_resolve() function.\n\nSince cc12005 (Make git rebase interactive help match documentation.,\n2006-05-13), git-am supports the --resolvemsg option which is used by\ngit-rebase to override the message printed out when git-am fails.\nRe-implement this option.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 26 +++++++++++++++++++++++---\n 1 file changed, 23 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 5f38264..1807d12 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -70,6 +70,9 @@ struct am_state {\n \tint prec;\n \n \tint quiet;\n+\n+\t/* override error message when patch failure occurs */\n+\tconst char *resolvemsg;\n };\n \n /**\n@@ -636,6 +639,21 @@ static int parse_patch(struct am_state *state, const char *patch)\n \treturn 0;\n }\n \n+/**\n+ * Dies with a user-friendly message on how to proceed after resolving the\n+ * problem. This message can be overridden with state->resolvemsg.\n+ */\n+static void NORETURN die_user_resolve(const struct am_state *state)\n+{\n+\tif (state->resolvemsg)\n+\t\tprintf_ln(\"%s\", state->resolvemsg);\n+\telse\n+\t\tprintf_ln(_(\"When you have resolved this problem, run \\\"git am --continue\\\".\\n\"\n+\t\t\t\"If you prefer to skip this patch, run \\\"git am --skip\\\" instead.\\n\"\n+\t\t\t\"To restore the original branch and stop patching, run \\\"git am --abort\\\".\"));\n+\texit(128);\n+}\n+\n /*\n * Applies current patch with git-apply. Returns 0 on success, -1 otherwise.\n */\n@@ -746,7 +764,7 @@ static void am_run(struct am_state *state)\n \t\t\t\tprintf_ln(_(\"The copy of the patch that failed is found in: %s\"),\n \t\t\t\t\t\tam_path(state, \"patch\"));\n \n-\t\t\texit(128);\n+\t\t\tdie_user_resolve(state);\n \t\t}\n \n \t\tdo_commit(state);\n@@ -771,13 +789,13 @@ static void am_resolve(struct am_state *state)\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n \t\t\t\"If there is nothing left to stage, chances are that something else\\n\"\n \t\t\t\"already introduced the same changes; you might want to skip this patch.\"));\n-\t\texit(128);\n+\t\tdie_user_resolve(state);\n \t}\n \n \tif (unmerged_cache()) {\n \t\tprintf_ln(_(\"You still have unmerged paths in your index.\\n\"\n \t\t\t\"Did you forget to use 'git add'?\"));\n-\t\texit(128);\n+\t\tdie_user_resolve(state);\n \t}\n \n \tdo_commit(state);\n@@ -991,6 +1009,8 @@ static struct option am_options[] = {\n \tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n+\tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n+\t\tN_(\"override error message when patch failure occurs\")),\n \tOPT_CMDMODE(0, \"continue\", &opt_resume,\n \t\tN_(\"continue applying patches after resolving a conflict\"),\n \t\tRESUME_RESOLVED),\n-- \n2.1.4\n"},{"id":"264157","messageId":"1434626743-8552-19-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 18/31] am: implement am --signoff","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:30Z","receivedAt":"2015-06-18T11:25:30Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am\nsupported the --signoff option which will append a signoff at the end of\nthe commit messsage. Re-implement this feature by calling\nappend_signoff() if the option is set.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 13 +++++++++++++\n 1 file changed, 13 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1807d12..d45dd41 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -18,6 +18,7 @@\n #include \"diffcore.h\"\n #include \"unpack-trees.h\"\n #include \"branch.h\"\n+#include \"sequencer.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -71,6 +72,8 @@ struct am_state {\n \n \tint quiet;\n \n+\tint sign;\n+\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n };\n@@ -292,6 +295,9 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"quiet\"), 2, 1);\n \tstate->quiet = !strcmp(sb.buf, \"t\");\n \n+\tread_state_file(&sb, am_path(state, \"sign\"), 2, 1);\n+\tstate->sign = !strcmp(sb.buf, \"t\");\n+\n \tstrbuf_release(&sb);\n }\n \n@@ -477,6 +483,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n \n+\twrite_file(am_path(state, \"sign\"), 1, state->sign ? \"t\" : \"f\");\n+\n \tif (!get_sha1(\"HEAD\", curr_head)) {\n \t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n \t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n@@ -636,6 +644,9 @@ static int parse_patch(struct am_state *state, const char *patch)\n \t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n \tstripspace(&state->msg, 0);\n \n+\tif (state->sign)\n+\t\tappend_signoff(&state->msg, 0, 0);\n+\n \treturn 0;\n }\n \n@@ -1007,6 +1018,8 @@ static const char * const am_usage[] = {\n \n static struct option am_options[] = {\n \tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n+\tOPT_BOOL('s', \"signoff\", &state.sign,\n+\t\tN_(\"add a Signed-off-by line to the commit message\")),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n-- \n2.1.4\n"},{"id":"264155","messageId":"1434626743-8552-20-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 19/31] cache-tree: introduce write_index_as_tree()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:31Z","receivedAt":"2015-06-18T11:25:31Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"A caller may wish to write a temporary index as a tree. However,\nwrite_cache_as_tree() assumes that the index was read from, and will\nwrite to, the default index file path. Introduce write_index_as_tree()\nwhich removes this limitation by allowing the caller to specify its own\nindex_state and index file path.\n\nSigned-off-by: Paul Tan \n---\n cache-tree.c | 29 +++++++++++++++++------------\n cache-tree.h | 1 +\n 2 files changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 32772b9..feace8b 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -592,7 +592,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n \treturn it;\n }\n \n-int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n+int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix)\n {\n \tint entries, was_valid, newfd;\n \tstruct lock_file *lock_file;\n@@ -603,23 +603,23 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \t */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tnewfd = hold_locked_index(lock_file, 1);\n+\tnewfd = hold_lock_file_for_update(lock_file, index_path, LOCK_DIE_ON_ERROR);\n \n-\tentries = read_cache();\n+\tentries = read_index_from(index_state, index_path);\n \tif (entries < 0)\n \t\treturn WRITE_TREE_UNREADABLE_INDEX;\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE)\n-\t\tcache_tree_free(&(active_cache_tree));\n+\t\tcache_tree_free(&index_state->cache_tree);\n \n-\tif (!active_cache_tree)\n-\t\tactive_cache_tree = cache_tree();\n+\tif (!index_state->cache_tree)\n+\t\tindex_state->cache_tree = cache_tree();\n \n-\twas_valid = cache_tree_fully_valid(active_cache_tree);\n+\twas_valid = cache_tree_fully_valid(index_state->cache_tree);\n \tif (!was_valid) {\n-\t\tif (cache_tree_update(&the_index, flags) < 0)\n+\t\tif (cache_tree_update(index_state, flags) < 0)\n \t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n \t\tif (0 <= newfd) {\n-\t\t\tif (!write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n+\t\t\tif (!write_locked_index(index_state, lock_file, COMMIT_LOCK))\n \t\t\t\tnewfd = -1;\n \t\t}\n \t\t/* Not being able to write is fine -- we are only interested\n@@ -631,14 +631,14 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \t}\n \n \tif (prefix) {\n-\t\tstruct cache_tree *subtree =\n-\t\t\tcache_tree_find(active_cache_tree, prefix);\n+\t\tstruct cache_tree *subtree;\n+\t\tsubtree = cache_tree_find(index_state->cache_tree, prefix);\n \t\tif (!subtree)\n \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n \t\thashcpy(sha1, subtree->sha1);\n \t}\n \telse\n-\t\thashcpy(sha1, active_cache_tree->sha1);\n+\t\thashcpy(sha1, index_state->cache_tree->sha1);\n \n \tif (0 <= newfd)\n \t\trollback_lock_file(lock_file);\n@@ -646,6 +646,11 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \treturn 0;\n }\n \n+int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n+{\n+\treturn write_index_as_tree(sha1, &the_index, get_index_file(), flags, prefix);\n+}\n+\n static void prime_cache_tree_rec(struct cache_tree *it, struct tree *tree)\n {\n \tstruct tree_desc desc;\ndiff --git a/cache-tree.h b/cache-tree.h\nindex aa7b3e4..41c5746 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -46,6 +46,7 @@ int update_main_cache_tree(int);\n #define WRITE_TREE_UNMERGED_INDEX (-2)\n #define WRITE_TREE_PREFIX_ERROR (-3)\n \n+int write_index_as_tree(unsigned char *sha1, struct index_state *index_state, const char *index_path, int flags, const char *prefix);\n int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix);\n void prime_cache_tree(struct index_state *, struct tree *);\n \n-- \n2.1.4\n"},{"id":"264158","messageId":"1434626743-8552-21-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 20/31] am: implement 3-way merge","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:32Z","receivedAt":"2015-06-18T11:25:32Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07),\ngit-am.sh supported the --3way option, and if set, would attempt to do a\n3-way merge if the initial patch application fails.\n\nSince 5d86861 (am -3: list the paths that needed 3-way fallback,\n2012-03-28), in a 3-way merge git-am.sh would list the paths that needed\n3-way fallback, so that the user can review them more carefully to spot\nmismerges.\n\nRe-implement the above in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 143 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d45dd41..e154c87 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -19,6 +19,8 @@\n #include \"unpack-trees.h\"\n #include \"branch.h\"\n #include \"sequencer.h\"\n+#include \"revision.h\"\n+#include \"merge-recursive.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -70,6 +72,8 @@ struct am_state {\n \t/* number of digits in patch filename */\n \tint prec;\n \n+\tint threeway;\n+\n \tint quiet;\n \n \tint sign;\n@@ -292,6 +296,9 @@ static void am_load(struct am_state *state)\n \n \tread_state_file(&state->msg, am_path(state, \"final-commit\"), 0, 0);\n \n+\tread_state_file(&sb, am_path(state, \"threeway\"), 2, 1);\n+\tstate->threeway = !strcmp(sb.buf, \"t\");\n+\n \tread_state_file(&sb, am_path(state, \"quiet\"), 2, 1);\n \tstate->quiet = !strcmp(sb.buf, \"t\");\n \n@@ -481,6 +488,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n \n+\twrite_file(am_path(state, \"threeway\"), 1, state->threeway ? \"t\" : \"f\");\n+\n \twrite_file(am_path(state, \"quiet\"), 1, state->quiet ? \"t\" : \"f\");\n \n \twrite_file(am_path(state, \"sign\"), 1, state->sign ? \"t\" : \"f\");\n@@ -666,17 +675,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n }\n \n /*\n- * Applies current patch with git-apply. Returns 0 on success, -1 otherwise.\n+ * Applies current patch with git-apply. Returns 0 on success, -1 otherwise. If\n+ * index_file is not NULL, the patch will be applied to that index.\n */\n-static int run_apply(const struct am_state *state)\n+static int run_apply(const struct am_state *state, const char *index_file)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n \tcp.git_cmd = 1;\n \n+\tif (index_file)\n+\t\targv_array_pushf(&cp.env_array, \"GIT_INDEX_FILE=%s\", index_file);\n+\n+\t/*\n+\t * If we are allowed to fall back on 3-way merge, don't give false\n+\t * errors during the initial attempt.\n+\t */\n+\tif (state->threeway && !index_file) {\n+\t\tcp.no_stdout = 1;\n+\t\tcp.no_stderr = 1;\n+\t}\n+\n \targv_array_push(&cp.args, \"apply\");\n \n-\targv_array_push(&cp.args, \"--index\");\n+\tif (index_file)\n+\t\targv_array_push(&cp.args, \"--cached\");\n+\telse\n+\t\targv_array_push(&cp.args, \"--index\");\n \n \targv_array_push(&cp.args, am_path(state, \"patch\"));\n \n@@ -685,8 +710,100 @@ static int run_apply(const struct am_state *state)\n \n \t/* Reload index as git-apply will have modified it. */\n \tdiscard_cache();\n+\tread_cache_from(index_file ? index_file : get_index_file());\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Builds a index that contains just the blobs needed for a 3way merge.\n+ */\n+static int build_fake_ancestor(const struct am_state *state, const char *index_file)\n+{\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tcp.git_cmd = 1;\n+\targv_array_push(&cp.args, \"apply\");\n+\targv_array_pushf(&cp.args, \"--build-fake-ancestor=%s\", index_file);\n+\targv_array_push(&cp.args, am_path(state, \"patch\"));\n+\n+\tif (run_command(&cp))\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Attempt a threeway merge, using index_path as the temporary index.\n+ */\n+static int fall_back_threeway(const struct am_state *state, const char *index_path)\n+{\n+\tunsigned char orig_tree[GIT_SHA1_RAWSZ], his_tree[GIT_SHA1_RAWSZ],\n+\t\t our_tree[GIT_SHA1_RAWSZ];\n+\tconst unsigned char *bases[1] = {orig_tree};\n+\tstruct merge_options o;\n+\tstruct commit *result;\n+\n+\tif (get_sha1(\"HEAD\", our_tree) < 0)\n+\t\thashcpy(our_tree, EMPTY_TREE_SHA1_BIN);\n+\n+\tif (build_fake_ancestor(state, index_path))\n+\t\treturn error(\"could not build fake ancestor\");\n+\n+\tdiscard_cache();\n+\tread_cache_from(index_path);\n+\n+\tif (write_index_as_tree(orig_tree, &the_index, index_path, 0, NULL))\n+\t\treturn error(_(\"Repository lacks necessary blobs to fall back on 3-way merge.\"));\n+\n+\tsay(state, stdout, _(\"Using index info to reconstruct a base tree...\"));\n+\n+\tif (!state->quiet) {\n+\t\t/*\n+\t\t * List paths that needed 3-way fallback, so that the user can\n+\t\t * review them with extra care to spot mismerges.\n+\t\t */\n+\t\tstruct rev_info rev_info;\n+\t\tconst char *diff_filter_str = \"--diff-filter=AM\";\n+\n+\t\tinit_revisions(&rev_info, NULL);\n+\t\trev_info.diffopt.output_format = DIFF_FORMAT_NAME_STATUS;\n+\t\tdiff_opt_parse(&rev_info.diffopt, &diff_filter_str, 1);\n+\t\tadd_pending_sha1(&rev_info, \"HEAD\", our_tree, 0);\n+\t\tdiff_setup_done(&rev_info.diffopt);\n+\t\trun_diff_index(&rev_info, 1);\n+\t}\n+\n+\tif (run_apply(state, index_path))\n+\t\treturn error(_(\"Did you hand edit your patch?\\n\"\n+\t\t\t\t\"It does not apply to blobs recorded in its index.\"));\n+\n+\tif (write_index_as_tree(his_tree, &the_index, index_path, 0, NULL))\n+\t\treturn error(\"could not write tree\");\n+\n+\tsay(state, stdout, _(\"Falling back to patching base and 3-way merge...\"));\n+\n+\tdiscard_cache();\n \tread_cache();\n \n+\t/*\n+\t * This is not so wrong. Depending on which base we picked, orig_tree\n+\t * may be wildly different from ours, but his_tree has the same set of\n+\t * wildly different changes in parts the patch did not touch, so\n+\t * recursive ends up canceling them, saying that we reverted all those\n+\t * changes.\n+\t */\n+\n+\tinit_merge_options(&o);\n+\n+\to.branch1 = \"HEAD\";\n+\to.branch2 = firstline(state->msg.buf);\n+\tif (state->quiet)\n+\t\to.verbosity = 0;\n+\n+\tif (merge_recursive_generic(&o, our_tree, his_tree, 1, bases, &result))\n+\t\treturn error(_(\"Failed to merge in the changes.\"));\n+\n \treturn 0;\n }\n \n@@ -753,6 +870,7 @@ static void am_run(struct am_state *state)\n \n \twhile (state->cur <= state->last) {\n \t\tconst char *patch = am_path(state, msgnum(state));\n+\t\tint apply_status;\n \n \t\tif (!file_exists(patch))\n \t\t\tgoto next;\n@@ -765,7 +883,26 @@ static void am_run(struct am_state *state)\n \n \t\tsay(state, stdout, _(\"Applying: %s\"), firstline(state->msg.buf));\n \n-\t\tif (run_apply(state) < 0) {\n+\t\tapply_status = run_apply(state, NULL);\n+\n+\t\tif (apply_status && state->threeway) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\tstrbuf_addstr(&sb, am_path(state, \"patch-merge-index\"));\n+\t\t\tapply_status = fall_back_threeway(state, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\n+\t\t\t/*\n+\t\t\t * Applying the patch to an earlier tree and merging\n+\t\t\t * the result may have produced the same tree as ours.\n+\t\t\t */\n+\t\t\tif (!apply_status && !index_has_changes(NULL)) {\n+\t\t\t\tsay(state, stdout, _(\"No changes -- Patch already applied.\"));\n+\t\t\t\tgoto next;\n+\t\t\t}\n+\t\t}\n+\n+\t\tif (apply_status) {\n \t\t\tint value;\n \n \t\t\tprintf_ln(_(\"Patch failed at %s %s\"), msgnum(state),\n@@ -1017,6 +1154,8 @@ static const char * const am_usage[] = {\n };\n \n static struct option am_options[] = {\n+\tOPT_BOOL('3', \"3way\", &state.threeway,\n+\t\tN_(\"allow fall back on 3way merging if needed\")),\n \tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n \tOPT_BOOL('s', \"signoff\", &state.sign,\n \t\tN_(\"add a Signed-off-by line to the commit message\")),\n-- \n2.1.4\n"},{"id":"264161","messageId":"1434626743-8552-22-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 21/31] am: --rebasing","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:33Z","receivedAt":"2015-06-18T11:25:33Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 3041c32 (am: --rebasing, 2008-03-04), git-am.sh supported the\n--rebasing option, which is used internally by git-rebase to tell git-am\nthat it is being used for its purpose. It would create the empty file\n$state_dir/rebasing to help \"completion\" scripts tell if the ongoing\noperation is am or rebase.\n\nAs of 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26),\n--rebasing also implies --3way as well.\n\nSince a1549e1 (am: return control to caller, for housekeeping,\n2013-05-12), git-am.sh would only clean up the state directory when it\nis not --rebasing, instead deferring cleanup to git-rebase.sh.\n\nRe-implement the above in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex e154c87..9afa3bb 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -80,6 +80,8 @@ struct am_state {\n \n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n+\n+\tint rebasing;\n };\n \n /**\n@@ -305,6 +307,8 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"sign\"), 2, 1);\n \tstate->sign = !strcmp(sb.buf, \"t\");\n \n+\tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n+\n \tstrbuf_release(&sb);\n }\n \n@@ -484,6 +488,9 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tdie(_(\"Failed to split patches.\"));\n \t}\n \n+\tif (state->rebasing)\n+\t\tstate->threeway = 1;\n+\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n \n \twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n@@ -494,12 +501,20 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"sign\"), 1, state->sign ? \"t\" : \"f\");\n \n+\tif (state->rebasing)\n+\t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n+\telse\n+\t\twrite_file(am_path(state, \"applying\"), 1, \"%s\", \"\");\n+\n \tif (!get_sha1(\"HEAD\", curr_head)) {\n \t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", sha1_to_hex(curr_head));\n-\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n+\t\tif (!state->rebasing)\n+\t\t\tupdate_ref(\"am\", \"ORIG_HEAD\", curr_head, NULL, 0,\n+\t\t\t\t\tUPDATE_REFS_DIE_ON_ERR);\n \t} else {\n \t\twrite_file(am_path(state, \"abort-safety\"), 1, \"%s\", \"\");\n-\t\tdelete_ref(\"ORIG_HEAD\", NULL, 0);\n+\t\tif (!state->rebasing)\n+\t\t\tdelete_ref(\"ORIG_HEAD\", NULL, 0);\n \t}\n }\n \n@@ -921,7 +936,8 @@ next:\n \t\tam_next(state);\n \t}\n \n-\tam_destroy(state);\n+\tif (!state->rebasing)\n+\t\tam_destroy(state);\n }\n \n /**\n@@ -1175,6 +1191,8 @@ static struct option am_options[] = {\n \tOPT_CMDMODE(0, \"abort\", &opt_resume,\n \t\tN_(\"restore the original branch and abort the patching operation.\"),\n \t\tRESUME_ABORT),\n+\tOPT_HIDDEN_BOOL(0, \"rebasing\", &state.rebasing,\n+\t\tN_(\"(internal use for git-rebase)\")),\n \tOPT_END()\n };\n \n-- \n2.1.4\n"},{"id":"264163","messageId":"1434626743-8552-23-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 22/31] am: don't use git-mailinfo if --rebasing","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:34Z","receivedAt":"2015-06-18T11:25:34Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 5e835ca (rebase: do not munge commit log message, 2008-04-16),\ngit am --rebasing no longer gets the commit log message from the patch,\nbut reads it directly from the commit identified by the \"From \" header\nline.\n\nSince 43c2325 (am: use get_author_ident_from_commit instead of mailinfo\nwhen rebasing, 2010-06-16), git am --rebasing also gets the author name,\nemail and date directly from the commit.\n\nSince 0fbb95d (am: don't call mailinfo if $rebasing, 2012-06-26), git am\n--rebasing does not use git-mailinfo to get the patch body, but rather\ngenerates it directly from the commit itself.\n\nThe above 3 commits introduced a separate parse_patch() code path in\ngit-am.sh's --rebasing mode that bypasses git-mailinfo. Re-implement\nthis code path in builtin/am.c as parse_patch_rebase().\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 153 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 9afa3bb..0d7e37c 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -21,6 +21,8 @@\n #include \"sequencer.h\"\n #include \"revision.h\"\n #include \"merge-recursive.h\"\n+#include \"revision.h\"\n+#include \"log-tree.h\"\n \n /**\n * Returns 1 if the file is empty or does not exist, 0 otherwise.\n@@ -50,6 +52,30 @@ static const char *firstline(const char *msg)\n \treturn sb.buf;\n }\n \n+/**\n+ * Iterate over lines, each terminated by c, in a string, without modifying the\n+ * string itself. begin will be set to the beginning of the line, while end\n+ * will be set to the line termination or null character at the end of the line\n+ * or string. On the first call, begin and end must be initialized to the\n+ * string to be iterated over. E.g.:\n+ *\n+ *\tconst char *begin, *end, *str = \"1\\n2\\n3\";\n+ *\tbegin = end = str;\n+ *\twhile (!getbufline(&begin, &end, '\\n'))\n+ *\t\t...\n+ */\n+static int getbufline(const char **begin, const char **end, int c)\n+{\n+\tif (!**end)\n+\t\treturn EOF;\n+\telse if (**end == c)\n+\t\t*begin = *end + 1;\n+\telse\n+\t\t*begin = *end;\n+\t*end = strchrnul(*begin, c);\n+\treturn 0;\n+}\n+\n enum patch_format {\n \tPATCH_FORMAT_UNKNOWN = 0,\n \tPATCH_FORMAT_MBOX\n@@ -675,6 +701,126 @@ static int parse_patch(struct am_state *state, const char *patch)\n }\n \n /**\n+ * Sets commit_id to the commit hash where the patch was generated from.\n+ * Returns 0 on success, -1 on failure.\n+ */\n+static int get_patch_commit_sha1(unsigned char *commit_id, const char *patch)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tFILE *fp = xfopen(patch, \"r\");\n+\tconst char *x;\n+\n+\tif (strbuf_getline(&sb, fp, '\\n'))\n+\t\treturn -1;\n+\n+\tif (!skip_prefix(sb.buf, \"From \", &x))\n+\t\treturn -1;\n+\n+\tif (get_sha1_hex(x, commit_id) < 0)\n+\t\treturn -1;\n+\n+\tstrbuf_release(&sb);\n+\tfclose(fp);\n+\treturn 0;\n+}\n+\n+/**\n+ * Sets state->msg, state->author_name, state->author_email, state->author_date\n+ * to the commit's respective info.\n+ */\n+static void get_commit_info(struct am_state *state, struct commit *commit)\n+{\n+\tconst char *buf, *begin, *end;\n+\n+\tbuf = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n+\tbegin = end = buf;\n+\n+\twhile (!getbufline(&begin, &end, '\\n')) {\n+\t\tconst char *x;\n+\n+\t\tif (skip_prefix(begin, \"author \", &x)) {\n+\t\t\tstruct ident_split split;\n+\t\t\tconst char *date;\n+\n+\t\t\tif (split_ident_line(&split, x, end - x) < 0) {\n+\t\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\t\t\tstrbuf_add(&sb, x, end - x);\n+\t\t\t\tdie(_(\"invalid ident line: %s\"), sb.buf);\n+\t\t\t}\n+\n+\t\t\tif (split.name_begin)\n+\t\t\t\tstrbuf_add(&state->author_name,\n+\t\t\t\t\tsplit.name_begin,\n+\t\t\t\t\tsplit.name_end - split.name_begin);\n+\n+\t\t\tif (split.mail_begin)\n+\t\t\t\tstrbuf_add(&state->author_email,\n+\t\t\t\t\tsplit.mail_begin,\n+\t\t\t\t\tsplit.mail_end - split.mail_begin);\n+\n+\t\t\tdate = show_ident_date(&split, DATE_NORMAL);\n+\t\t\tstrbuf_addstr(&state->author_date, date);\n+\t\t} else if (begin == end) {\n+\t\t\tend++;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\tstrbuf_addstr(&state->msg, end);\n+}\n+\n+/**\n+ * Writes commit as a patch to $state_dir/patch.\n+ */\n+static void write_commit_patch(const struct am_state *state, struct commit *commit)\n+{\n+\tstruct rev_info rev_info;\n+\tFILE *fp;\n+\n+\tfp = xfopen(am_path(state, \"patch\"), \"w\");\n+\tinit_revisions(&rev_info, NULL);\n+\trev_info.diff = 1;\n+\trev_info.abbrev = 0;\n+\trev_info.disable_stdin = 1;\n+\trev_info.show_root_diff = 1;\n+\trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n+\trev_info.no_commit_id = 1;\n+\tDIFF_OPT_SET(&rev_info.diffopt, BINARY);\n+\tDIFF_OPT_SET(&rev_info.diffopt, FULL_INDEX);\n+\trev_info.diffopt.use_color = 0;\n+\trev_info.diffopt.file = fp;\n+\trev_info.diffopt.close_file = 1;\n+\tadd_pending_object(&rev_info, &commit->object, \"\");\n+\tdiff_setup_done(&rev_info.diffopt);\n+\tlog_tree_commit(&rev_info, commit);\n+}\n+\n+/**\n+ * Like parse_patch(), but parses the patch by looking up its commit ID\n+ * directly. This is used in --rebasing mode to bypass git-mailinfo's munging\n+ * of patches.\n+ *\n+ * Will always return 0 as the patch should never be skipped.\n+ */\n+static int parse_patch_rebase(struct am_state *state, const char *patch)\n+{\n+\tstruct commit *commit;\n+\tunsigned char commit_sha1[GIT_SHA1_RAWSZ];\n+\n+\tif (get_patch_commit_sha1(commit_sha1, patch) < 0)\n+\t\tdie(_(\"could not parse %s\"), patch);\n+\n+\tcommit = lookup_commit_or_die(commit_sha1, patch);\n+\n+\tget_commit_info(state, commit);\n+\n+\twrite_commit_patch(state, commit);\n+\n+\treturn 0;\n+}\n+\n+/**\n * Dies with a user-friendly message on how to proceed after resolving the\n * problem. This message can be overridden with state->resolvemsg.\n */\n@@ -885,12 +1031,17 @@ static void am_run(struct am_state *state)\n \n \twhile (state->cur <= state->last) {\n \t\tconst char *patch = am_path(state, msgnum(state));\n-\t\tint apply_status;\n+\t\tint apply_status, skip;\n \n \t\tif (!file_exists(patch))\n \t\t\tgoto next;\n \n-\t\tif (parse_patch(state, patch))\n+\t\tif (state->rebasing)\n+\t\t\tskip = parse_patch_rebase(state, patch);\n+\t\telse\n+\t\t\tskip = parse_patch(state, patch);\n+\n+\t\tif (skip)\n \t\t\tgoto next; /* patch should be skipped */\n \n \t\twrite_author_script(state);\n-- \n2.1.4\n"},{"id":"264160","messageId":"1434626743-8552-24-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 23/31] am: handle stray state directory","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:35Z","receivedAt":"2015-06-18T11:25:35Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Should git-am terminate unexpectedly between the point where the state\ndirectory is created, but the \"next\" and \"last\" files are not written\nyet, a stray state directory will be left behind.\n\nAs such, since b141f3c (am: handle stray $dotest directory, 2013-06-15),\ngit-am.sh explicitly recognizes such a stray directory, and allows the\nuser to remove it with am --abort.\n\nRe-implement this feature in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0d7e37c..bbef91f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1395,6 +1395,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\tstruct string_list paths = STRING_LIST_INIT_DUP;\n \t\tint i;\n \n+\t\t/*\n+\t\t * Possible stray dotest directory in the independent-run case.\n+\t\t */\n+\t\tif (file_exists(state.dir.buf) && !state.rebasing) {\n+\t\t\tif (opt_resume == RESUME_ABORT) {\n+\t\t\t\tam_destroy(&state);\n+\t\t\t\tam_state_release(&state);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\n+\t\t\tdie(_(\"Stray %s directory found.\\n\"\n+\t\t\t\t\"Use \\\"git am --abort\\\" to remove it.\"),\n+\t\t\t\tstate.dir.buf);\n+\t\t}\n+\n \t\tif (opt_resume)\n \t\t\tdie(_(\"Resolve operation not in progress, we are not resuming.\"));\n \n-- \n2.1.4\n"},{"id":"264159","messageId":"1434626743-8552-25-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 24/31] am: implement -k/--keep, --keep-non-patch","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:36Z","receivedAt":"2015-06-18T11:25:36Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07),\ngit-am.sh supported the -k/--keep option to pass the -k option to\ngit-mailsplit.\n\nSince f7e5ea1 (am: learn passing -b to mailinfo, 2012-01-16), git-am.sh\nsupported the --keep-non-patch option to pass the -b option to\ngit-mailsplit.\n\nRe-implement these two options in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 51 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex bbef91f..b73549f 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -81,6 +81,12 @@ enum patch_format {\n \tPATCH_FORMAT_MBOX\n };\n \n+enum keep_type {\n+\tKEEP_FALSE = 0,\n+\tKEEP_TRUE, /* pass -k flag to git-mailinfo */\n+\tKEEP_NON_PATCH /* pass -b flag to git-mailinfo */\n+};\n+\n struct am_state {\n \t/* state directory path */\n \tstruct strbuf dir;\n@@ -104,6 +110,9 @@ struct am_state {\n \n \tint sign;\n \n+\t/* one of the enum keep_type values */\n+\tint keep;\n+\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n@@ -333,6 +342,14 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"sign\"), 2, 1);\n \tstate->sign = !strcmp(sb.buf, \"t\");\n \n+\tread_state_file(&sb, am_path(state, \"keep\"), 2, 1);\n+\tif (!strcmp(sb.buf, \"t\"))\n+\t\tstate->keep = KEEP_TRUE;\n+\telse if (!strcmp(sb.buf, \"b\"))\n+\t\tstate->keep = KEEP_NON_PATCH;\n+\telse\n+\t\tstate->keep = KEEP_FALSE;\n+\n \tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n \n \tstrbuf_release(&sb);\n@@ -497,6 +514,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t\tstruct string_list *paths)\n {\n \tunsigned char curr_head[GIT_SHA1_RAWSZ];\n+\tconst char *str;\n \n \tif (!patch_format)\n \t\tpatch_format = detect_patch_format(paths);\n@@ -527,6 +545,21 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"sign\"), 1, state->sign ? \"t\" : \"f\");\n \n+\tswitch (state->keep) {\n+\tcase KEEP_FALSE:\n+\t\tstr = \"f\";\n+\t\tbreak;\n+\tcase KEEP_TRUE:\n+\t\tstr = \"t\";\n+\t\tbreak;\n+\tcase KEEP_NON_PATCH:\n+\t\tstr = \"b\";\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: invalid value for state->keep\");\n+\t}\n+\twrite_file(am_path(state, \"keep\"), 1, \"%s\", str);\n+\n \tif (state->rebasing)\n \t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n \telse\n@@ -653,6 +686,20 @@ static int parse_patch(struct am_state *state, const char *patch)\n \tcp.out = xopen(am_path(state, \"info\"), O_WRONLY | O_CREAT, 0777);\n \n \targv_array_push(&cp.args, \"mailinfo\");\n+\n+\tswitch (state->keep) {\n+\tcase KEEP_FALSE:\n+\t\tbreak;\n+\tcase KEEP_TRUE:\n+\t\targv_array_push(&cp.args, \"-k\");\n+\t\tbreak;\n+\tcase KEEP_NON_PATCH:\n+\t\targv_array_push(&cp.args, \"-b\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: invalid value for state->keep\");\n+\t}\n+\n \targv_array_push(&cp.args, am_path(state, \"msg\"));\n \targv_array_push(&cp.args, am_path(state, \"patch\"));\n \n@@ -1326,6 +1373,10 @@ static struct option am_options[] = {\n \tOPT__QUIET(&state.quiet, N_(\"be quiet\")),\n \tOPT_BOOL('s', \"signoff\", &state.sign,\n \t\tN_(\"add a Signed-off-by line to the commit message\")),\n+\tOPT_SET_INT('k', \"keep\", &state.keep,\n+\t\tN_(\"pass -k flag to git-mailinfo\"), KEEP_TRUE),\n+\tOPT_SET_INT(0, \"keep-non-patch\", &state.keep,\n+\t\tN_(\"pass -b flag to git-mailinfo\"), KEEP_NON_PATCH),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n-- \n2.1.4\n"},{"id":"264164","messageId":"1434626743-8552-26-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 25/31] am: implement --[no-]message-id, am.messageid","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:37Z","receivedAt":"2015-06-18T11:25:37Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since a078f73 (git-am: add --message-id/--no-message-id, 2014-11-25),\ngit-am.sh supported the --[no-]message-id options, and the\n\"am.messageid\" setting which specifies the default option.\n\n--[no-]message-id tells git-am whether or not the -m option should be\npassed to git-mailinfo.\n\nRe-implement this option in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n\nNotes:\n No test for am.messageid\n\n builtin/am.c | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex b73549f..4cec380 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -113,6 +113,9 @@ struct am_state {\n \t/* one of the enum keep_type values */\n \tint keep;\n \n+\t/* pass -m flag to git-mailinfo */\n+\tint message_id;\n+\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n@@ -140,6 +143,8 @@ static void am_state_init(struct am_state *state)\n \tquiet = getenv(\"GIT_QUIET\");\n \tif (quiet && *quiet)\n \t\tstate->quiet = 1;\n+\n+\tgit_config_get_bool(\"am.messageid\", &state->message_id);\n }\n \n /**\n@@ -350,6 +355,9 @@ static void am_load(struct am_state *state)\n \telse\n \t\tstate->keep = KEEP_FALSE;\n \n+\tread_state_file(&sb, am_path(state, \"messageid\"), 2, 1);\n+\tstate->message_id = !strcmp(sb.buf, \"t\");\n+\n \tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n \n \tstrbuf_release(&sb);\n@@ -560,6 +568,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_file(am_path(state, \"keep\"), 1, \"%s\", str);\n \n+\twrite_file(am_path(state, \"messageid\"), 1, state->message_id ? \"t\" : \"f\");\n+\n \tif (state->rebasing)\n \t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n \telse\n@@ -700,6 +710,9 @@ static int parse_patch(struct am_state *state, const char *patch)\n \t\tdie(\"BUG: invalid value for state->keep\");\n \t}\n \n+\tif (state->message_id)\n+\t\targv_array_push(&cp.args, \"-m\");\n+\n \targv_array_push(&cp.args, am_path(state, \"msg\"));\n \targv_array_push(&cp.args, am_path(state, \"patch\"));\n \n@@ -1377,6 +1390,8 @@ static struct option am_options[] = {\n \t\tN_(\"pass -k flag to git-mailinfo\"), KEEP_TRUE),\n \tOPT_SET_INT(0, \"keep-non-patch\", &state.keep,\n \t\tN_(\"pass -b flag to git-mailinfo\"), KEEP_NON_PATCH),\n+\tOPT_BOOL('m', \"message-id\", &state.message_id,\n+\t\tN_(\"pass -m flag to git-mailinfo\")),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n-- \n2.1.4\n"},{"id":"264162","messageId":"1434626743-8552-27-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 26/31] am: support --keep-cr, am.keepcr","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:38Z","receivedAt":"2015-06-18T11:25:38Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since ad2c928 (git-am: Add command line parameter `--keep-cr` passing it\nto git-mailsplit, 2010-02-27), git-am.sh supported the --keep-cr option\nand would pass it to git-mailsplit.\n\nSince e80d4cb (git-am: Add am.keepcr and --no-keep-cr to override it,\n2010-02-27), git-am.sh supported the am.keepcr config setting, which\ncontrols whether --keep-cr is on by default.\n\nRe-implement the above in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 30 ++++++++++++++++++++++++------\n 1 file changed, 24 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 4cec380..1991f36 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -469,7 +469,8 @@ done:\n * Splits out individual patches from `paths`, where each path is either a mbox\n * file or a Maildir. Return 0 on success, -1 on failure.\n */\n-static int split_patches_mbox(struct am_state *state, struct string_list *paths)\n+static int split_patches_mbox(struct am_state *state, struct string_list *paths,\n+\t\tint keep_cr)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct string_list_item *item;\n@@ -480,6 +481,8 @@ static int split_patches_mbox(struct am_state *state, struct string_list *paths)\n \targv_array_pushf(&cp.args, \"-d%d\", state->prec);\n \targv_array_pushf(&cp.args, \"-o%s\", state->dir.buf);\n \targv_array_push(&cp.args, \"-b\");\n+\tif (keep_cr)\n+\t\targv_array_push(&cp.args, \"--keep-cr\");\n \targv_array_push(&cp.args, \"--\");\n \n \tfor_each_string_list_item(item, paths)\n@@ -501,14 +504,22 @@ static int split_patches_mbox(struct am_state *state, struct string_list *paths)\n * set to the index of the first patch, and state->last will be set to the\n * index of the last patch.\n *\n+ * Set keep_cr to 0 to convert all lines ending with \\r\\n to end with \\n, 1\n+ * to disable this behavior, -1 to use the default configured setting.\n+ *\n * Returns 0 on success, -1 on failure.\n */\n static int split_patches(struct am_state *state, enum patch_format patch_format,\n-\t\tstruct string_list *paths)\n+\t\tstruct string_list *paths, int keep_cr)\n {\n+\tif (keep_cr < 0) {\n+\t\tkeep_cr = 0;\n+\t\tgit_config_get_bool(\"am.keepcr\", &keep_cr);\n+\t}\n+\n \tswitch (patch_format) {\n \tcase PATCH_FORMAT_MBOX:\n-\t\treturn split_patches_mbox(state, paths);\n+\t\treturn split_patches_mbox(state, paths, keep_cr);\n \tdefault:\n \t\tdie(\"BUG: invalid patch_format\");\n \t}\n@@ -519,7 +530,7 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,\n * Setup a new am session for applying patches\n */\n static void am_setup(struct am_state *state, enum patch_format patch_format,\n-\t\tstruct string_list *paths)\n+\t\tstruct string_list *paths, int keep_cr)\n {\n \tunsigned char curr_head[GIT_SHA1_RAWSZ];\n \tconst char *str;\n@@ -535,7 +546,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n \t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n \n-\tif (split_patches(state, patch_format, paths) < 0) {\n+\tif (split_patches(state, patch_format, paths, keep_cr) < 0) {\n \t\tam_destroy(state);\n \t\tdie(_(\"Failed to split patches.\"));\n \t}\n@@ -1371,6 +1382,7 @@ enum resume_mode {\n };\n \n static struct am_state state;\n+static int opt_keep_cr = -1;\n static int opt_patch_format;\n static enum resume_mode opt_resume;\n \n@@ -1392,6 +1404,12 @@ static struct option am_options[] = {\n \t\tN_(\"pass -b flag to git-mailinfo\"), KEEP_NON_PATCH),\n \tOPT_BOOL('m', \"message-id\", &state.message_id,\n \t\tN_(\"pass -m flag to git-mailinfo\")),\n+\t{ OPTION_SET_INT, 0, \"keep-cr\", &opt_keep_cr, NULL,\n+\t N_(\"pass --keep-cr flag to git-mailsplit for mbox format\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 1},\n+\t{ OPTION_SET_INT, 0, \"no-keep-cr\", &opt_keep_cr, NULL,\n+\t N_(\"do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\"),\n+\t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n@@ -1486,7 +1504,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \t\t\t\tstring_list_append(&paths, mkpath(\"%s/%s\", prefix, argv[i]));\n \t\t}\n \n-\t\tam_setup(&state, opt_patch_format, &paths);\n+\t\tam_setup(&state, opt_patch_format, &paths, opt_keep_cr);\n \n \t\tstring_list_clear(&paths, 0);\n \t}\n-- \n2.1.4\n"},{"id":"264165","messageId":"1434626743-8552-28-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 27/31] am: implement --[no-]scissors","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:39Z","receivedAt":"2015-06-18T11:25:39Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 017678b (am/mailinfo: Disable scissors processing by default,\n2009-08-26), git-am supported the --[no-]scissors option, passing it to\ngit-mailinfo.\n\nRe-implement support for this option.\n\nSigned-off-by: Paul Tan \n---\n\nNotes:\n There are tests for mailinfo --scissors, but not am --scissors, or\n mailinfo.scissors.\n\n builtin/am.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 49 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1991f36..42efce1 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -87,6 +87,12 @@ enum keep_type {\n \tKEEP_NON_PATCH /* pass -b flag to git-mailinfo */\n };\n \n+enum scissors_type {\n+\tSCISSORS_UNSET = -1,\n+\tSCISSORS_TRUE, /* pass --scissors to git-mailinfo */\n+\tSCISSORS_FALSE /* pass --no-scissors to git-mailinfo */\n+};\n+\n struct am_state {\n \t/* state directory path */\n \tstruct strbuf dir;\n@@ -116,6 +122,9 @@ struct am_state {\n \t/* pass -m flag to git-mailinfo */\n \tint message_id;\n \n+\t/* one of the enum scissors_type values */\n+\tint scissors;\n+\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n@@ -145,6 +154,8 @@ static void am_state_init(struct am_state *state)\n \t\tstate->quiet = 1;\n \n \tgit_config_get_bool(\"am.messageid\", &state->message_id);\n+\n+\tstate->scissors = SCISSORS_UNSET;\n }\n \n /**\n@@ -358,6 +369,14 @@ static void am_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"messageid\"), 2, 1);\n \tstate->message_id = !strcmp(sb.buf, \"t\");\n \n+\tread_state_file(&sb, am_path(state, \"scissors\"), 2, 1);\n+\tif (!strcmp(sb.buf, \"t\"))\n+\t\tstate->scissors = SCISSORS_TRUE;\n+\telse if (!strcmp(sb.buf, \"f\"))\n+\t\tstate->scissors = SCISSORS_FALSE;\n+\telse\n+\t\tstate->scissors = SCISSORS_UNSET;\n+\n \tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n \n \tstrbuf_release(&sb);\n@@ -581,6 +600,21 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \n \twrite_file(am_path(state, \"messageid\"), 1, state->message_id ? \"t\" : \"f\");\n \n+\tswitch (state->scissors) {\n+\tcase SCISSORS_UNSET:\n+\t\tstr = \"\";\n+\t\tbreak;\n+\tcase SCISSORS_FALSE:\n+\t\tstr = \"f\";\n+\t\tbreak;\n+\tcase SCISSORS_TRUE:\n+\t\tstr = \"t\";\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: invalid value for state->scissors\");\n+\t}\n+\twrite_file(am_path(state, \"scissors\"), 1, \"%s\", str);\n+\n \tif (state->rebasing)\n \t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n \telse\n@@ -724,6 +758,19 @@ static int parse_patch(struct am_state *state, const char *patch)\n \tif (state->message_id)\n \t\targv_array_push(&cp.args, \"-m\");\n \n+\tswitch (state->scissors) {\n+\tcase SCISSORS_UNSET:\n+\t\tbreak;\n+\tcase SCISSORS_FALSE:\n+\t\targv_array_push(&cp.args, \"--no-scissors\");\n+\t\tbreak;\n+\tcase SCISSORS_TRUE:\n+\t\targv_array_push(&cp.args, \"--scissors\");\n+\t\tbreak;\n+\tdefault:\n+\t\tdie(\"BUG: invalid value for state->scissors\");\n+\t}\n+\n \targv_array_push(&cp.args, am_path(state, \"msg\"));\n \targv_array_push(&cp.args, am_path(state, \"patch\"));\n \n@@ -1410,6 +1457,8 @@ static struct option am_options[] = {\n \t{ OPTION_SET_INT, 0, \"no-keep-cr\", &opt_keep_cr, NULL,\n \t N_(\"do not pass --keep-cr flag to git-mailsplit independent of am.keepcr\"),\n \t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n+\tOPT_BOOL('c', \"scissors\", &state.scissors,\n+\t\tN_(\"strip everything before a scissors line\")),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n-- \n2.1.4\n"},{"id":"264168","messageId":"1434626743-8552-29-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 28/31] am: pass git-apply's options to git-apply","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:40Z","receivedAt":"2015-06-18T11:25:40Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"git-am.sh recognizes some of git-apply's options, and would pass them to\ngit-apply:\n\n* --whitespace, since 8c31cb8 (git-am: --whitespace=x option.,\n 2006-02-28)\n\n* -C, since 67dad68 (add -C[NUM] to git-am, 2007-02-08)\n\n* -p, since 2092a1f (Teach git-am to pass -p option down to git-apply,\n 2007-02-11)\n\n* --directory, since b47dfe9 (git-am: add --directory= option,\n 2009-01-11)\n\n* --reject, since b80da42 (git-am: implement --reject option passed to\n git-apply, 2009-01-23)\n\n* --ignore-space-change, --ignore-whitespace, since 86c91f9 (git apply:\n option to ignore whitespace differences, 2009-08-04)\n\n* --exclude, since 77e9e49 (am: pass exclude down to apply, 2011-08-03)\n\n* --include, since 58725ef (am: support --include option, 2012-03-28)\n\nRe-implement support for these options in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 42efce1..3556765 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -125,6 +125,8 @@ struct am_state {\n \t/* one of the enum scissors_type values */\n \tint scissors;\n \n+\tstruct argv_array git_apply_opts;\n+\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n@@ -156,6 +158,8 @@ static void am_state_init(struct am_state *state)\n \tgit_config_get_bool(\"am.messageid\", &state->message_id);\n \n \tstate->scissors = SCISSORS_UNSET;\n+\n+\targv_array_init(&state->git_apply_opts);\n }\n \n /**\n@@ -168,6 +172,8 @@ static void am_state_release(struct am_state *state)\n \tstrbuf_release(&state->author_email);\n \tstrbuf_release(&state->author_date);\n \tstrbuf_release(&state->msg);\n+\n+\targv_array_clear(&state->git_apply_opts);\n }\n \n /**\n@@ -377,6 +383,11 @@ static void am_load(struct am_state *state)\n \telse\n \t\tstate->scissors = SCISSORS_UNSET;\n \n+\tread_state_file(&sb, am_path(state, \"apply-opt\"), 128, 1);\n+\targv_array_clear(&state->git_apply_opts);\n+\tif (sq_dequote_to_argv_array(sb.buf, &state->git_apply_opts) < 0)\n+\t\tdie(_(\"could not parse %s\"), am_path(state, \"apply-opt\"));\n+\n \tstate->rebasing = !!file_exists(am_path(state, \"rebasing\"));\n \n \tstrbuf_release(&sb);\n@@ -553,6 +564,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n {\n \tunsigned char curr_head[GIT_SHA1_RAWSZ];\n \tconst char *str;\n+\tstruct strbuf sb = STRBUF_INIT;\n \n \tif (!patch_format)\n \t\tpatch_format = detect_patch_format(paths);\n@@ -615,6 +627,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,\n \t}\n \twrite_file(am_path(state, \"scissors\"), 1, \"%s\", str);\n \n+\tsq_quote_argv(&sb, state->git_apply_opts.argv, 0);\n+\twrite_file(am_path(state, \"apply-opt\"), 1, \"%s\", sb.buf);\n+\tstrbuf_release(&sb);\n+\n \tif (state->rebasing)\n \t\twrite_file(am_path(state, \"rebasing\"), 1, \"%s\", \"\");\n \telse\n@@ -977,6 +993,8 @@ static int run_apply(const struct am_state *state, const char *index_file)\n \n \targv_array_push(&cp.args, \"apply\");\n \n+\targv_array_pushv(&cp.args, state->git_apply_opts.argv);\n+\n \tif (index_file)\n \t\targv_array_push(&cp.args, \"--cached\");\n \telse\n@@ -1003,6 +1021,7 @@ static int build_fake_ancestor(const struct am_state *state, const char *index_f\n \n \tcp.git_cmd = 1;\n \targv_array_push(&cp.args, \"apply\");\n+\targv_array_pushv(&cp.args, state->git_apply_opts.argv);\n \targv_array_pushf(&cp.args, \"--build-fake-ancestor=%s\", index_file);\n \targv_array_push(&cp.args, am_path(state, \"patch\"));\n \n@@ -1459,8 +1478,35 @@ static struct option am_options[] = {\n \t PARSE_OPT_NOARG | PARSE_OPT_NONEG, NULL, 0},\n \tOPT_BOOL('c', \"scissors\", &state.scissors,\n \t\tN_(\"strip everything before a scissors line\")),\n+\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &state.git_apply_opts, N_(\"action\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n+\tOPT_PASSTHRU_ARGV(0, \"ignore-space-change\", &state.git_apply_opts, NULL,\n+\t\tN_(\"pass it through git-apply\"),\n+\t\tPARSE_OPT_NOARG),\n+\tOPT_PASSTHRU_ARGV(0, \"ignore-whitespace\", &state.git_apply_opts, NULL,\n+\t\tN_(\"pass it through git-apply\"),\n+\t\tPARSE_OPT_NOARG),\n+\tOPT_PASSTHRU_ARGV(0, \"directory\", &state.git_apply_opts, N_(\"root\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n+\tOPT_PASSTHRU_ARGV(0, \"exclude\", &state.git_apply_opts, N_(\"path\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n+\tOPT_PASSTHRU_ARGV(0, \"include\", &state.git_apply_opts, N_(\"path\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n+\tOPT_PASSTHRU_ARGV('C', NULL, &state.git_apply_opts, N_(\"n\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n+\tOPT_PASSTHRU_ARGV('p', NULL, &state.git_apply_opts, N_(\"num\"),\n+\t\tN_(\"pass it through git-apply\"),\n+\t\t0),\n \tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n \t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n+\tOPT_PASSTHRU_ARGV(0, \"reject\", &state.git_apply_opts, NULL,\n+\t\tN_(\"pass it through git-apply\"),\n+\t\tPARSE_OPT_NOARG),\n \tOPT_STRING(0, \"resolvemsg\", &state.resolvemsg, NULL,\n \t\tN_(\"override error message when patch failure occurs\")),\n \tOPT_CMDMODE(0, \"continue\", &opt_resume,\n-- \n2.1.4\n"},{"id":"264166","messageId":"1434626743-8552-30-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 29/31] am: implement --ignore-date","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:41Z","receivedAt":"2015-06-18T11:25:41Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since a79ec62 (git-am: Add --ignore-date option, 2009-01-24), git-am.sh\nsupported the --ignore-date option, and would use the current timestamp\ninstead of the one provided in the patch if the option was set.\n\nRe-implement this option in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 3556765..6623b49 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -130,6 +130,8 @@ struct am_state {\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n+\tint ignore_date;\n+\n \tint rebasing;\n };\n \n@@ -1131,7 +1133,8 @@ static void do_commit(const struct am_state *state)\n \t}\n \n \tauthor = fmt_ident(state->author_name.buf, state->author_email.buf,\n-\t\t\tstate->author_date.buf, IDENT_STRICT);\n+\t\t\tstate->ignore_date ? NULL : state->author_date.buf,\n+\t\t\tIDENT_STRICT);\n \n \tif (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,\n \t\t\t\tauthor, NULL))\n@@ -1521,6 +1524,8 @@ static struct option am_options[] = {\n \tOPT_CMDMODE(0, \"abort\", &opt_resume,\n \t\tN_(\"restore the original branch and abort the patching operation.\"),\n \t\tRESUME_ABORT),\n+\tOPT_BOOL(0, \"ignore-date\", &state.ignore_date,\n+\t\tN_(\"use current timestamp for author date\")),\n \tOPT_HIDDEN_BOOL(0, \"rebasing\", &state.rebasing,\n \t\tN_(\"(internal use for git-rebase)\")),\n \tOPT_END()\n-- \n2.1.4\n"},{"id":"264169","messageId":"1434626743-8552-31-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 30/31] am: implement --committer-date-is-author-date","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:42Z","receivedAt":"2015-06-18T11:25:42Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 3f01ad6 (am: Add --committer-date-is-author-date option,\n2009-01-22), git-am.sh implemented the --committer-date-is-author-date\noption, which tells git-am to use the timestamp recorded in the email\nmessage as both author and committer date.\n\nRe-implement this option in builtin/am.c.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6623b49..608a2da 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -130,6 +130,8 @@ struct am_state {\n \t/* override error message when patch failure occurs */\n \tconst char *resolvemsg;\n \n+\tint committer_date_is_author_date;\n+\n \tint ignore_date;\n \n \tint rebasing;\n@@ -1136,6 +1138,10 @@ static void do_commit(const struct am_state *state)\n \t\t\tstate->ignore_date ? NULL : state->author_date.buf,\n \t\t\tIDENT_STRICT);\n \n+\tif (state->committer_date_is_author_date)\n+\t\tsetenv(\"GIT_COMMITTER_DATE\",\n+\t\t\tstate->ignore_date ? \"\" : state->author_date.buf, 1);\n+\n \tif (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,\n \t\t\t\tauthor, NULL))\n \t\tdie(_(\"failed to write commit object\"));\n@@ -1524,6 +1530,9 @@ static struct option am_options[] = {\n \tOPT_CMDMODE(0, \"abort\", &opt_resume,\n \t\tN_(\"restore the original branch and abort the patching operation.\"),\n \t\tRESUME_ABORT),\n+\tOPT_BOOL(0, \"committer-date-is-author-date\",\n+\t\t&state.committer_date_is_author_date,\n+\t\tN_(\"lie about committer date\")),\n \tOPT_BOOL(0, \"ignore-date\", &state.ignore_date,\n \t\tN_(\"use current timestamp for author date\")),\n \tOPT_HIDDEN_BOOL(0, \"rebasing\", &state.rebasing,\n-- \n2.1.4\n"},{"id":"264167","messageId":"1434626743-8552-32-git-send-email-pyokagan@gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP v3 31/31] am: implement -S/--gpg-sign, commit.gpgsign","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-18T11:25:43Z","receivedAt":"2015-06-18T11:25:43Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Since 3b4e395 (am: add the --gpg-sign option, 2014-02-01), git-am.sh\nsupported the --gpg-sign option, and would pass it to git-commit-tree,\nthus GPG-signing the commit object.\n\nRe-implement this option in builtin/am.c.\n\ngit-commit-tree would also sign the commit by default if the\ncommit.gpgsign setting is true. Since we do not run commit-tree, we\nre-implement this behavior by handling the commit.gpgsign setting\nourselves.\n\nSigned-off-by: Paul Tan \n---\n builtin/am.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 608a2da..91dae92 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -134,6 +134,8 @@ struct am_state {\n \n \tint ignore_date;\n \n+\tconst char *sign_commit;\n+\n \tint rebasing;\n };\n \n@@ -143,6 +145,7 @@ struct am_state {\n static void am_state_init(struct am_state *state)\n {\n \tconst char *quiet;\n+\tint sign_commit;\n \n \tmemset(state, 0, sizeof(*state));\n \n@@ -164,6 +167,9 @@ static void am_state_init(struct am_state *state)\n \tstate->scissors = SCISSORS_UNSET;\n \n \targv_array_init(&state->git_apply_opts);\n+\n+\tif (!git_config_get_bool(\"commit.gpgsign\", &sign_commit))\n+\t\tstate->sign_commit = sign_commit ? \"\" : NULL;\n }\n \n /**\n@@ -1143,7 +1149,7 @@ static void do_commit(const struct am_state *state)\n \t\t\tstate->ignore_date ? \"\" : state->author_date.buf, 1);\n \n \tif (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,\n-\t\t\t\tauthor, NULL))\n+\t\t\t\tauthor, state->sign_commit))\n \t\tdie(_(\"failed to write commit object\"));\n \n \treflog_msg = getenv(\"GIT_REFLOG_ACTION\");\n@@ -1535,6 +1541,9 @@ static struct option am_options[] = {\n \t\tN_(\"lie about committer date\")),\n \tOPT_BOOL(0, \"ignore-date\", &state.ignore_date,\n \t\tN_(\"use current timestamp for author date\")),\n+\t{ OPTION_STRING, 'S', \"gpg-sign\", &state.sign_commit, N_(\"key-id\"),\n+\t N_(\"GPG-sign commits\"),\n+\t PARSE_OPT_OPTARG, NULL, (intptr_t) \"\" },\n \tOPT_HIDDEN_BOOL(0, \"rebasing\", &state.rebasing,\n \t\tN_(\"(internal use for git-rebase)\")),\n \tOPT_END()\n-- \n2.1.4\n"},{"id":"264204","messageId":"CAGZ79kbdUhH9=Xui6jeco-H6ZGaiC5jtLdBp9K7xv7AnsmmeKQ@mail.gmail.com","threadId":"39670","inReplyTo":"1434626743-8552-5-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-18T17:47:19Z","receivedAt":"2015-06-18T17:47:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Jun 18, 2015 at 4:25 AM, Paul Tan wrote:\n> +/**\n> + * Reads the contents of `file`. The third argument can be used to give a hint\n\nI would avoid `third` here. (I needed to count twice to be sure which\nargument you\nwere referring to, as I was confused.) Also how do you abstain from\ngiving a hint?\n(0 or negative or MAX_INT?)\n\nSo maybe\n\n /**\n * Reads the contents of `file`. Returns number of bytes read on success,\n * -1 if the file does not exist. If trim is set, trailing\nwhitespace will be removed\n * from the file contents. If `hint` is non-zero, it is used as a\nhint for initial\n * allocation to avoid reallocs.\n */\n\n> + * about the file size, to avoid reallocs. Returns number of bytes read on\n> + * success, -1 if the file does not exist. If trim is set, trailing whitespace\n> + * will be removed from the file contents.\n> + */\n> +static int read_state_file(struct strbuf *sb, const char *file, size_t hint, int trim)\n> +{\n> + strbuf_reset(sb);\n> + if (strbuf_read_file(sb, file, hint) >= 0) {\n> + if (trim)\n> + strbuf_trim(sb);\n> +\n> + return sb->len;\n> + }\n> +\n> + if (errno == ENOENT)\n> + return -1;\n> +\n> + die_errno(_(\"could not read '%s'\"), file);\n> +}\n> +\n> +/**\n> + * Loads state from disk.\n> + */\n> +static void am_load(struct am_state *state)\n> +{\n> + struct strbuf sb = STRBUF_INIT;\n> +\n> + read_state_file(&sb, am_path(state, \"next\"), 8, 1);\n> + state->cur = strtol(sb.buf, NULL, 10);\n> +\n> + read_state_file(&sb, am_path(state, \"last\"), 8, 1);\n> + state->last = strtol(sb.buf, NULL, 10);\n> +\n> + strbuf_release(&sb);\n> +}\n> +\n> +/**\n> + * Remove the am_state directory.\n> + */\n> +static void am_destroy(const struct am_state *state)\n> +{\n> + struct strbuf sb = STRBUF_INIT;\n> +\n> + strbuf_addstr(&sb, state->dir.buf);\n> + remove_dir_recursively(&sb, 0);\n> + strbuf_release(&sb);\n> +}\n> +\n> +/**\n> + * Setup a new am session for applying patches\n> + */\n> +static void am_setup(struct am_state *state)\n> +{\n> + if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n> + die_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n> +\n> + write_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n> +\n> + write_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n> +}\n> +\n> +/**\n> + * Increments the patch pointer, and cleans am_state for the application of the\n> + * next patch.\n> + */\n> +static void am_next(struct am_state *state)\n> +{\n> + state->cur++;\n> + write_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n> +}\n> +\n> +/**\n> + * Applies all queued patches.\n> + */\n> +static void am_run(struct am_state *state)\n> +{\n> + while (state->cur <= state->last) {\n> +\n> + /* TODO: Patch application not implemented yet */\n> +\n> + am_next(state);\n> + }\n> +\n> + am_destroy(state);\n> +}\n> +\n> +static struct am_state state;\n> +\n> +static const char * const am_usage[] = {\n> + N_(\"git am [options] [(|)...]\"),\n> + NULL\n> +};\n> +\n> +static struct option am_options[] = {\n> + OPT_END()\n> +};\n>\n> int cmd_am(int argc, const char **argv, const char *prefix)\n> {\n> @@ -24,5 +176,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> setup_work_tree();\n> }\n>\n> + git_config(git_default_config, NULL);\n> +\n> + am_state_init(&state);\n> + strbuf_addstr(&state.dir, git_path(\"rebase-apply\"));\n> +\n> + argc = parse_options(argc, argv, prefix, am_options, am_usage, 0);\n> +\n> + if (am_in_progress(&state))\n> + am_load(&state);\n> + else\n> + am_setup(&state);\n> +\n> + am_run(&state);\n> +\n> + am_state_release(&state);\n> +\n> return 0;\n> }\n> --\n> 2.1.4\n>\n"},{"id":"264241","messageId":"xmqqbngcepb3.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 03/31] am: implement skeletal builtin am","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T20:26:08Z","receivedAt":"2015-06-18T20:26:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> For the purpose of rewriting git-am.sh into a C builtin, implement a\n> skeletal builtin/am.c that redirects to $GIT_EXEC_PATH/git-am if the\n> environment variable _GIT_USE_BUILTIN_AM is not defined. Since in the\n> Makefile git-am.sh takes precedence over builtin/am.c,\n> $GIT_EXEC_PATH/git-am will contain the shell script git-am.sh, and thus\n> this allows us to fall back on the functional git-am.sh when running the\n> test suite for tests that depend on a working git-am implementation.\n>\n> Since git-am.sh cannot handle any environment modifications by\n> setup_git_directory(), \"am\" has to be declared as NO_SETUP in git.c. On\n> the other hand, to re-implement git-am.sh in builtin/am.c, we do need to\n> run all the git dir and work tree setup logic that git.c does for us. As\n> such, we work around this temporarily by copying the logic in git.c's\n> run_builtin(), which amounts to:\n>\n> \tprefix = setup_git_directory();\n> \ttrace_repo_setup(prefix);\n> \tsetup_work_tree();\n>\n> This redirection should be removed when all the features of git-am.sh\n> have been re-implemented in builtin/am.c.\n>\n> Helped-by: Junio C Hamano \n> Signed-off-by: Paul Tan \n> ---\n>\n> Notes:\n> v3\n> \n> * Style fixes\n> \n> * git-am.sh cannot handle the chdir() and GIT_DIR envionment variable\n> that setup_git_directory() sets, so we work around it by copying the\n> logic of git.c's run_builtin(), and running it only when we are using\n> the builtin am.\n>\n> Makefile | 1 +\n> builtin.h | 1 +\n> builtin/am.c | 28 ++++++++++++++++++++++++++++\n> git.c | 1 +\n> 4 files changed, 31 insertions(+)\n> create mode 100644 builtin/am.c\n>\n> diff --git a/Makefile b/Makefile\n> index 93e4fa2..ff9bdc0 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -811,6 +811,7 @@ LIB_OBJS += xdiff-interface.o\n> LIB_OBJS += zlib.o\n> \n> BUILTIN_OBJS += builtin/add.o\n> +BUILTIN_OBJS += builtin/am.o\n> BUILTIN_OBJS += builtin/annotate.o\n> BUILTIN_OBJS += builtin/apply.o\n> BUILTIN_OBJS += builtin/archive.o\n> diff --git a/builtin.h b/builtin.h\n> index ea3c834..f30cf00 100644\n> --- a/builtin.h\n> +++ b/builtin.h\n> @@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, const unsigned char\n> extern int is_builtin(const char *s);\n> \n> extern int cmd_add(int argc, const char **argv, const char *prefix);\n> +extern int cmd_am(int argc, const char **argv, const char *prefix);\n> extern int cmd_annotate(int argc, const char **argv, const char *prefix);\n> extern int cmd_apply(int argc, const char **argv, const char *prefix);\n> extern int cmd_archive(int argc, const char **argv, const char *prefix);\n> diff --git a/builtin/am.c b/builtin/am.c\n> new file mode 100644\n> index 0000000..dbc8836\n> --- /dev/null\n> +++ b/builtin/am.c\n> @@ -0,0 +1,28 @@\n> +/*\n> + * Builtin \"git am\"\n> + *\n> + * Based on git-am.sh by Junio C Hamano.\n> + */\n> +#include \"cache.h\"\n> +#include \"builtin.h\"\n> +#include \"exec_cmd.h\"\n> +\n> +int cmd_am(int argc, const char **argv, const char *prefix)\n> +{\n> +\t/*\n> +\t * FIXME: Once all the features of git-am.sh have been re-implemented\n> +\t * in builtin/am.c, this preamble can be removed.\n> +\t */\n\nIt's not broken, so \"FIXME\" is not quite appropriate (and that is\nwhy I sent you \"NEEDSWORK\"). Also mention that the entry in the\ncommands[] array needs \"RUN_SETUP | NEED_WORK_TREE\" added, I think.\n\n> +\tif (!getenv(\"_GIT_USE_BUILTIN_AM\")) {\n> +\t\tconst char *path = mkpath(\"%s/git-am\", git_exec_path());\n> +\n> +\t\tif (sane_execvp(path, (char **)argv) < 0)\n> +\t\t\tdie_errno(\"could not exec %s\", path);\n> +\t} else {\n> +\t\tprefix = setup_git_directory();\n> +\t\ttrace_repo_setup(prefix);\n> +\t\tsetup_work_tree();\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> diff --git a/git.c b/git.c\n> index e7a7713..a671535 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -370,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n> \n> static struct cmd_struct commands[] = {\n> \t{ \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n> +\t{ \"am\", cmd_am, NO_SETUP },\n\nNO_SETUP is for things like init and clone that start without a\nrepository and then work in the one that they create. I think\nimitating \"archive\" or \"diff\" is more appropriate.\n\n> \t{ \"annotate\", cmd_annotate, RUN_SETUP },\n> \t{ \"apply\", cmd_apply, RUN_SETUP_GENTLY },\n> \t{ \"archive\", cmd_archive },\n"},{"id":"264243","messageId":"xmqq7fr0eoho.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-5-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T20:43:47Z","receivedAt":"2015-06-18T20:43:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index dbc8836..af68c51 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -6,6 +6,158 @@\n> #include \"cache.h\"\n> #include \"builtin.h\"\n> #include \"exec_cmd.h\"\n> +#include \"parse-options.h\"\n> +#include \"dir.h\"\n> +\n> +struct am_state {\n> +\t/* state directory path */\n> +\tstruct strbuf dir;\n\nIs this a temporary variable you will append \"/patch\", etc. to form\na different string to use for fopen() etc., or do you use separate\nstrbufs for things like that and this is only used to initialize\nthem?\n\n - If the former then \"dir\" is a misnomer.\n\n - If the latter, then it probably does not have to be a strbuf;\n rather, it should probably be a \"const char *\". Unless you pass\n this directly to functions that take a strbuf, such as\n remove_dir_recursively(), that is.\n\n> +/**\n> + * Release memory allocated by an am_state.\n> + */\n\nEverybody else in this file seems to say \"Initializes\", \"Returns\",\n\"Reads\", etc. While I personally prefer to use imperative\n(i.e. give command to this function to \"release memory allocated\"),\nyou would want to be consistent throughout the file; \"Releases\nmemory\" would make it so.\n\n> +/**\n> + * Setup a new am session for applying patches\n> + */\n> +static void am_setup(struct am_state *state)\n> +{\n> +\tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n> +\t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n> +\n> +\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n> +\n> +\twrite_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n\nThese two lines are closely related pair; drop the blank in between.\n\nI am tno sure if write_file() is an appropriate thing to use,\nthough. What happens when you get interrupted after opening the\nfile but before you manage to write and close? Shouldn't we be\ndoing the usual \"write to temp, close and then rename to final\"\ndance? This comment applies to all the other use of write_file().\n"},{"id":"264245","messageId":"xmqq381oeo35.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-6-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 05/31] am: split out mbox/maildir patches with git-mailsplit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T20:52:30Z","receivedAt":"2015-06-18T20:52:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> @@ -111,13 +122,69 @@ static void am_destroy(const struct am_state *state)\n> }\n> \n> /**\n> + * Splits out individual patches from `paths`, where each path is either a mbox\n> + * file or a Maildir. Return 0 on success, -1 on failure.\n> + */\n\n\"Splits\" and then \"Return\"? Be consistent.\n\n> +static int split_patches_mbox(struct am_state *state, struct string_list *paths)\n> +{\n> ...\n> +}\n\nLooks straightforward ;-)\n\n> +/**\n> + * parse_options() callback that validates and sets opt->value to the\n> + * PATCH_FORMAT_* enum value corresponding to `arg`.\n> + */\n> +static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tint *opt_value = opt->value;\n> +\n> +\tif (!strcmp(arg, \"mbox\"))\n> +\t\t*opt_value = PATCH_FORMAT_MBOX;\n> +\telse\n> +\t\treturn -1;\n> +\treturn 0;\n> +}\n> +\n> static struct am_state state;\n> +static int opt_patch_format;\n> \n> static const char * const am_usage[] = {\n> \tN_(\"git am [options] [(|)...]\"),\n> @@ -156,6 +239,8 @@ static const char * const am_usage[] = {\n> };\n> \n> static struct option am_options[] = {\n> +\tOPT_CALLBACK(0, \"patch-format\", &opt_patch_format, N_(\"format\"),\n> +\t\tN_(\"format the patch(es) are in\"), parse_opt_patchformat),\n> \tOPT_END()\n> };\n\nLooking good ;-).\n\nJust FYI, you do not have to make am_options[], and the variables\nthat are referenced from there, e.g. opt_patch_format, etc., global\nvariables (instead you can have them all in the function scope of\ncmd_am()).\n"},{"id":"264247","messageId":"xmqqy4jgd92l.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-7-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T21:02:10Z","receivedAt":"2015-06-18T21:02:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> +static int is_email(const char *filename)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tFILE *fp = xfopen(filename, \"r\");\n> +\tint ret = 1;\n> +\n> +\twhile (!strbuf_getline(&sb, fp, '\\n')) {\n> +\t\tconst char *x;\n> +\n> +\t\tstrbuf_rtrim(&sb);\n\nIs this a good thing? strbuf_getline() already has stripped the LF\nat the end, so you'd be treating a line with only whitespaces as if\nit is a truly empty line.\n\nI know the series is about literal translation and the script may\nlose the distinction between the two, but I do not think you need\n(or want) to be literally same for things like this.\n\nSame comment applies to other uses of \"trim\" in this patch.\n\n> @@ -177,6 +267,14 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,\n> static void am_setup(struct am_state *state, enum patch_format patch_format,\n> \t\tstruct string_list *paths)\n> {\n> +\tif (!patch_format)\n> +\t\tpatch_format = detect_patch_format(paths);\n> +\n> +\tif (!patch_format) {\n> +\t\tfprintf_ln(stderr, _(\"Patch format detection failed.\"));\n> +\t\texit(128);\n> +\t}\n> +\n> \tif (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n> \t\tdie_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n\nI really like the way this keeps building incrementally ;-)\nThe series is an enjoyable read.\n"},{"id":"264248","messageId":"xmqqtwu4d8pg.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-8-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T21:10:03Z","receivedAt":"2015-06-18T21:10:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> +\t/* commit message and metadata */\n> +\tstruct strbuf author_name;\n> +\tstruct strbuf author_email;\n> +\tstruct strbuf author_date;\n> +\tstruct strbuf msg;\n\nSame comment as \"dir\" in the earlier patch applies to these. If the\nfields are read or computed and then kept constant, use a temporary\nvariable that is a strbuf to read/compute the final value, and then\ndetach to a \"const char *\" field. If they are constantly changing\nand in-place updates are vital, then they can and should be strbufs,\nbut I do not think that is the case for these.\n\nFor example...\n\n> +/**\n> + * Saves state->author_name, state->author_email and state->author_date in\n> + * `filename` as an \"author script\", which is the format used by git-am.sh.\n> + */\n> +static void write_author_script(const struct am_state *state)\n> +{\n> +\tstatic const char fmt[] = \"GIT_AUTHOR_NAME=%s\\n\"\n> +\t\t\"GIT_AUTHOR_EMAIL=%s\\n\"\n> +\t\t\"GIT_AUTHOR_DATE=%s\\n\";\n> +\tstruct strbuf author_name = STRBUF_INIT;\n> +\tstruct strbuf author_email = STRBUF_INIT;\n> +\tstruct strbuf author_date = STRBUF_INIT;\n> +\n> +\tsq_quote_buf(&author_name, state->author_name.buf);\n> +\tsq_quote_buf(&author_email, state->author_email.buf);\n> +\tsq_quote_buf(&author_date, state->author_date.buf);\n\nAs you use a separate author_name variable here, what gets sq-quoted\nthat is in *state does not have to be strbuf.\n\nThe code to read is the same story:\n\n> +static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *str;\n> +\n> +\tif (strbuf_getline(&sb, fp, '\\n'))\n> +\t\treturn -1;\n> +...\n> +\tstrbuf_reset(value);\n> +\tstrbuf_addstr(value, str);\n> +\n> +\tstrbuf_release(&sb);\n> +\n> +\treturn 0;\n> +}\n\nAs you use a separate sb strbuf variable here, there is no need for\n\"value\" to be pointing at a strbuf; it could be \"char **\" that sb's\ncontents is detached into.\n"},{"id":"264249","messageId":"xmqqpp4sd83f.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-9-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 08/31] am: apply patch with git-apply","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T21:23:16Z","receivedAt":"2015-06-18T21:23:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> Implement applying the patch to the index using git-apply.\n>\n> Signed-off-by: Paul Tan \n> ---\n> builtin/am.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 56 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index d6434e4..296a5fc 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -27,6 +27,18 @@ static int is_empty_file(const char *filename)\n> \treturn !st.st_size;\n> }\n> \n> +/**\n> + * Returns the first line of msg\n> + */\n> +static const char *firstline(const char *msg)\n> +{\n> +\tstatic struct strbuf sb = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&sb);\n> +\tstrbuf_add(&sb, msg, strchrnul(msg, '\\n') - msg);\n> +\treturn sb.buf;\n> +}\n\nHmm. This is not wrong per-se but a more efficient way to do it may\nbe to have a helper function that returns a bytecount of the first\nline of the msg, i.e. strchrnul(msg, '\\n') - msg. Then a caller can\ndo\n\n\tprintf(\"Applying: %.*s\", linelen(msg), msg);\n\ninstead of\n\n\tprintf(\"Applying: %s\", firstline(msg));\n\nrelying on that the firstline() copies the contents to a static\nstrbuf that does not have to be freed.\n\n> +\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\n> +\tcp.git_cmd = 1;\n> +\n> +\targv_array_push(&cp.args, \"apply\");\n> +\n> +\targv_array_push(&cp.args, \"--index\");\n> +\n> +\targv_array_push(&cp.args, am_path(state, \"patch\"));\n\nYou seem to like blank lines a lot ;-) While it is a good tool to\nseparate different groups while grouping related things together,\nthese three argv-push calls are intimately related, and reads better\nwithout blanks in between.\n\nLooks nicely done so far...\n"},{"id":"264254","messageId":"xmqqlhfgd7vf.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"1434626743-8552-11-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 10/31] am: refresh the index at start","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-18T21:28:04Z","receivedAt":"2015-06-18T21:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> If a file is unchanged but stat-dirty, git-apply may erroneously fail to\n> apply patches, thinking that they conflict with a dirty working tree.\n>\n> As such, since 2a6f08a (am: refresh the index at start and --resolved,\n> 2011-08-15), git-am will refresh the index before applying patches.\n> Re-implement this behavior.\n\nGood.\n\nI would actually have expected to see this as part of 08/31, though.\n\n>\n> Signed-off-by: Paul Tan \n> ---\n> builtin/am.c | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index dfb6f7e..a7efe85 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -13,6 +13,7 @@\n> #include \"cache-tree.h\"\n> #include \"refs.h\"\n> #include \"commit.h\"\n> +#include \"lockfile.h\"\n> \n> /**\n> * Returns 1 if the file is empty or does not exist, 0 otherwise.\n> @@ -471,6 +472,20 @@ static const char *msgnum(const struct am_state *state)\n> }\n> \n> /**\n> + * Refresh and write index.\n> + */\n> +static void refresh_and_write_cache(void)\n> +{\n> +\tstatic struct lock_file lock_file;\n> +\n> +\thold_locked_index(&lock_file, 1);\n> +\trefresh_cache(REFRESH_QUIET);\n> +\tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n> +\t\tdie(_(\"unable to write index file\"));\n> +\trollback_lock_file(&lock_file);\n> +}\n> +\n> +/**\n> * Parses `patch` using git-mailinfo. state->msg will be set to the patch\n> * message. state->author_name, state->author_email, state->author_date will be\n> * set to the patch author's name, email and date respectively. The patch's\n> @@ -607,6 +622,8 @@ static void do_commit(const struct am_state *state)\n> */\n> static void am_run(struct am_state *state)\n> {\n> +\trefresh_and_write_cache();\n> +\n> \twhile (state->cur <= state->last) {\n> \t\tconst char *patch = am_path(state, msgnum(state));\n> \n> @@ -696,6 +713,9 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n> \n> \targc = parse_options(argc, argv, prefix, am_options, am_usage, 0);\n> \n> +\tif (read_index_preload(&the_index, NULL) < 0)\n> +\t\tdie(_(\"failed to read the index\"));\n> +\n> \tif (am_in_progress(&state))\n> \t\tam_load(&state);\n> \telse {\n"},{"id":"264280","messageId":"CACRoPnRRPLRTsLkKem3QsGiLV3BpEBXNdrtoDHWfXfoR2yCDYg@mail.gmail.com","threadId":"39670","inReplyTo":"xmqqlhfgd7vf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 10/31] am: refresh the index at start","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-19T08:07:05Z","receivedAt":"2015-06-19T08:07:05Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 5:28 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> If a file is unchanged but stat-dirty, git-apply may erroneously fail to\n>> apply patches, thinking that they conflict with a dirty working tree.\n>>\n>> As such, since 2a6f08a (am: refresh the index at start and --resolved,\n>> 2011-08-15), git-am will refresh the index before applying patches.\n>> Re-implement this behavior.\n>\n> Good.\n>\n> I would actually have expected to see this as part of 08/31, though.\n\nAh right, makes sense since this patch is primarily about git-apply.\n\nI've squashed this patch into 08/31.\n\nRegards,\nPaul\n"},{"id":"264284","messageId":"CACRoPnS9eyBF5NEM7sKvep+A8aKUNLJDaV-1c_oWDBwMcv26Bg@mail.gmail.com","threadId":"39670","inReplyTo":"xmqqtwu4d8pg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-19T09:22:11Z","receivedAt":"2015-06-19T09:22:11Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 5:10 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> + /* commit message and metadata */\n>> + struct strbuf author_name;\n>> + struct strbuf author_email;\n>> + struct strbuf author_date;\n>> + struct strbuf msg;\n>\n> Same comment as \"dir\" in the earlier patch applies to these. If the\n> fields are read or computed and then kept constant, use a temporary\n> variable that is a strbuf to read/compute the final value, and then\n> detach to a \"const char *\" field. If they are constantly changing\n> and in-place updates are vital, then they can and should be strbufs,\n> but I do not think that is the case for these.\n\nI do think it is the case here. The commit message and metadata fields\nchange for every patch we process, and we could be processing a large\nvolume of patches, so we must be careful to not leak any memory.\n\nWe could use a char* field, but then to prevent memory leaks we would\nthen have to free() it and malloc() a new string for every patch we\nprocess, which will lead to lots of malloc()s and free()s over a large\nvolume of patches that can lead to memory fragmentation.\n\nRegards,\nPaul\n"},{"id":"264291","messageId":"CACRoPnTnajSmiZtfONZdn-a2Jy90NVOQ5qrUm0p75VgY1W4Kwg@mail.gmail.com","threadId":"39670","inReplyTo":"xmqqbngcepb3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 03/31] am: implement skeletal builtin am","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-19T09:56:33Z","receivedAt":"2015-06-19T09:56:33Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 4:26 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>> @@ -0,0 +1,28 @@\n>> +/*\n>> + * Builtin \"git am\"\n>> + *\n>> + * Based on git-am.sh by Junio C Hamano.\n>> + */\n>> +#include \"cache.h\"\n>> +#include \"builtin.h\"\n>> +#include \"exec_cmd.h\"\n>> +\n>> +int cmd_am(int argc, const char **argv, const char *prefix)\n>> +{\n>> + /*\n>> + * FIXME: Once all the features of git-am.sh have been re-implemented\n>> + * in builtin/am.c, this preamble can be removed.\n>> + */\n>\n> It's not broken, so \"FIXME\" is not quite appropriate (and that is\n> why I sent you \"NEEDSWORK\").\n\nOK.\n\n> Also mention that the entry in the\n> commands[] array needs \"RUN_SETUP | NEED_WORK_TREE\" added, I think.\n\nOK.\n\n>> + if (!getenv(\"_GIT_USE_BUILTIN_AM\")) {\n>> + const char *path = mkpath(\"%s/git-am\", git_exec_path());\n>> +\n>> + if (sane_execvp(path, (char **)argv) < 0)\n>> + die_errno(\"could not exec %s\", path);\n>> + } else {\n>> + prefix = setup_git_directory();\n>> + trace_repo_setup(prefix);\n>> + setup_work_tree();\n>> + }\n>> +\n>> + return 0;\n>> +}\n>> diff --git a/git.c b/git.c\n>> index e7a7713..a671535 100644\n>> --- a/git.c\n>> +++ b/git.c\n>> @@ -370,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n>>\n>> static struct cmd_struct commands[] = {\n>> { \"add\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n>> + { \"am\", cmd_am, NO_SETUP },\n>\n> NO_SETUP is for things like init and clone that start without a\n> repository and then work in the one that they create. I think\n> imitating \"archive\" or \"diff\" is more appropriate.\n\nAh OK. I thought that handle_alias() in git.c would chdir() and set\nGIT_DIR because it called setup_git_directory_gently(), and thus\nrequires NO_SETUP to restore the initial environment, but turns out it\nchdir()s back to the original directory, and sets GIT_DIR\nappropriately, so we're OK.\n\nThanks,\nPaul\n"},{"id":"264306","messageId":"CACRoPnSVWNOPPJAhJS1S-HbHUEVJ1p_jz-dnFmuk8Hh1NaMLwg@mail.gmail.com","threadId":"39670","inReplyTo":"xmqq7fr0eoho.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-19T12:49:29Z","receivedAt":"2015-06-19T12:49:29Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 4:43 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index dbc8836..af68c51 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -6,6 +6,158 @@\n>> #include \"cache.h\"\n>> #include \"builtin.h\"\n>> #include \"exec_cmd.h\"\n>> +#include \"parse-options.h\"\n>> +#include \"dir.h\"\n>> +\n>> +struct am_state {\n>> + /* state directory path */\n>> + struct strbuf dir;\n>\n> Is this a temporary variable you will append \"/patch\", etc. to form\n> a different string to use for fopen() etc., or do you use separate\n> strbufs for things like that and this is only used to initialize\n> them?\n>\n> - If the former then \"dir\" is a misnomer.\n>\n> - If the latter, then it probably does not have to be a strbuf;\n> rather, it should probably be a \"const char *\". Unless you pass\n> this directly to functions that take a strbuf, such as\n> remove_dir_recursively(), that is.\n\nIt's the latter, and yes it does not need to be an strbuf since it\nwill not usually change.\n\nHowever, I don't think it should be a const char*, as it means that\nthe API user has to keep track of the lifetime of the \"dir\" string.\nNote that we will have to git_pathdup() as git_path() returns a static\nbuffer:\n\nchar *dir = git_pathdup(\"rebase-apply\");\nstruct am_state state;\nam_state_init(&state);\nstate.dir = dir;\n...stuff...\nif (foo) {\n ... separate code path ...\n am_state_release(&state);\n free(dir);\n return 0;\n}\n... separate code path ...\nam_state_release(&state);\nfree(dir);\nreturn 0;\n\nNote the additional memory bookkeeping.\n\nInstead, I would rather the am_state struct take ownership of the\n\"dir\" string and free it in am_state_release(). So dir will be a\nchar*:\n\nstruct am_state state;\nam_state_init(&state, git_path(\"rebase-apply\"));\n... stuff ...\nif (foo) {\n ... separate code path ...\n am_state_release(&state);\n return 0;\n}\n... separate code path ...\nam_state_release(&state);\nreturn 0;\n\n>> +/**\n>> + * Release memory allocated by an am_state.\n>> + */\n>\n> Everybody else in this file seems to say \"Initializes\", \"Returns\",\n> \"Reads\", etc. While I personally prefer to use imperative\n> (i.e. give command to this function to \"release memory allocated\"),\n> you would want to be consistent throughout the file; \"Releases\n> memory\" would make it so.\n\nOK\n\n>> +/**\n>> + * Setup a new am session for applying patches\n>> + */\n>> +static void am_setup(struct am_state *state)\n>> +{\n>> + if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n>> + die_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n>> +\n>> + write_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n>> +\n>> + write_file(am_path(state, \"last\"), 1, \"%d\", state->last);\n>\n> These two lines are closely related pair; drop the blank in between.\n>\n> I am tno sure if write_file() is an appropriate thing to use,\n> though. What happens when you get interrupted after opening the\n> file but before you manage to write and close? Shouldn't we be\n> doing the usual \"write to temp, close and then rename to final\"\n> dance? This comment applies to all the other use of write_file().\n\nWe could, although I don't think it would help us much. The state\ndirectory is made up of many different files, so even if we made\nmodifications to single files atomic, there's no point if we terminate\nunexpectedly in the middle of writing multiple files to the state\ndirectory as the state, as a whole, is corrupted anyway.\n\nSo, we must be able to make updates to the entire state directory as a\nsingle transaction, which is a more difficult problem...\n\nFurthermore, while applying patches, we may face abnormal termination\nbetween e.g. the patch application and commit, so I think it is safe\nto say that if abnormal termination occurs, we will not be able to\nreliably --resume or --skip anyway, and the user should just run\n--abort to go back to the last known state.\n\nHowever, it would be an improvement if we wrote the \"next\" and \"last\"\nfiles last, as we use their presence to determine if we have an am\nsession in progress or not, so if we terminate in am_setup() we will\njust have a stray directory. I will make this change in the next\niteration.\n\nRegards,\nPaul\n"},{"id":"264337","messageId":"xmqq1th7brsj.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"CACRoPnS9eyBF5NEM7sKvep+A8aKUNLJDaV-1c_oWDBwMcv26Bg@mail.gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-19T16:13:00Z","receivedAt":"2015-06-19T16:13:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> On Fri, Jun 19, 2015 at 5:10 AM, Junio C Hamano wrote:\n>> Paul Tan writes:\n>>\n>>> + /* commit message and metadata */\n>>> + struct strbuf author_name;\n>>> + struct strbuf author_email;\n>>> + struct strbuf author_date;\n>>> + struct strbuf msg;\n>>\n>> Same comment as \"dir\" in the earlier patch applies to these. If the\n>> fields are read or computed and then kept constant, use a temporary\n>> variable that is a strbuf to read/compute the final value, and then\n>> detach to a \"const char *\" field. If they are constantly changing\n>> and in-place updates are vital, then they can and should be strbufs,\n>> but I do not think that is the case for these.\n>\n> I do think it is the case here. The commit message and metadata fields\n> change for every patch we process, and we could be processing a large\n> volume of patches, so we must be careful to not leak any memory.\n\nWith the above fields, it is clear that the above fields are\nper-message thing. So the loop to process multiple messages is\nconceptually:\n\n\n\tset up the entire am_state (for things like cur=1, last=N)\n for each message {\n\t\tupdate per-message fields like cur, author, msg, etc.\n\t\tprocess the single message\n clean up per-message fileds like cur++, free(msg), etc.\n\t}\n\ttear down the entire am_state\n\nReusing the same strbuf and rely on them to clean up after\nthemselves is simply a bad taste.\n\nMore importantly, we'd want to make sure that people who will be\ntouching the \"process the single message\" part in the above loop to\nthink twice before mucking with read-only fields like author.\n\nIf they are \"char *\", they would need to allocate new storage\nthemselves, format a new value into there, free the original, and\nreplace the field with the new value. It takes a conscious effort\nand very much visible code and would be clear to reviewers what is\ngoing on, and gives them a chance to question if it is a good idea\nto update things in-place in the first place.\n\nIf you left it in strbuf, that invites \"let's extend it temporarily\nwith strbuf_add() and then return to the original once I am done\nwith this single step\", which is an entry to a slippery slope to\n\"let's extend it with strbuf_add() for my purpose, and I do not even\nbother to clean up because I know I am the last person to touch\nthis\".\n\nSo, no, please don't leave a field that won't change during the bulk\nof the processing in strbuf, unless you have a compelling reason to\ndo so (and \"compelling reasons\" are pretty much limited to \"after\nall, that field we originally thought it won't change is something\nwe need to change very often\").\n\nThanks.\n"},{"id":"264338","messageId":"CACRoPnQdR7qC6gH7j9_8gQUYKShLRQCr6GNtmn+XCpe9P4P-nA@mail.gmail.com","threadId":"39670","inReplyTo":"CAPc5daVbpB_T4cY1xvLrBKPUZw0JNMXqNAOsKE-R7NPO2nrnZA@mail.gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-19T16:15:41Z","receivedAt":"2015-06-19T16:15:41Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 11:09 PM, Junio C Hamano wrote:\n> You do realize that strbuf internally does alloc/free so as a solution to\n> fragmentation issue you are at the mercy of the same alloc/free, don't you?\n\nYes, of course, but it has the \"alloc\" variable to keep track of the\nsize of the allocated buffer, and provided that ALLOC_GROW() uses a\nsensible factor to grow the buffer, it won't be calling malloc/free\nthat much to be a problem.\n\nOf course, we could do the same and just add a \"alloc\" variable as\nwell and then use it with strbuf_attach()/strbuf_detach(), but at this\npoint why not just store it in an strbuf then and avoid the extra\n\"alloc\" struct member?\n\n> The primary thing I care about is to discourage callers of the API element\n> am_state from touching these fields with strbuf functions. If it is char * then\n> the would think twice before modifying (which would involve allocating the\n> new buffer, forming the new value and assigning into it). With the fields left\n> as strbuf after they are read or formed by the API (by reading by the API\n> implementation from $GIT_DIR/rebase-apply/), it is too tempting to do\n> strbuf_add(), strbuf_trim(), etc., without restoring the value to the original\n> saying \"I'm the last user of it anyway\"--that is the sloppiness we can prevent\n> by not leaving it as strbuf.\n\nI don't think this is a good deterrent. If the code wanted to, it\ncould just use strbuf_attach()/strbuf_detach() as well, no?\n\nI believe this kind of issue would be better solved with a big WARNING\ncomment and code review.\n\nAlso, there may be use cases where the user may wish to modify the\ncommit message/author fields. E.g., user may wish to modify the\nmessage of the commit that results from am --continue to take into\naccount the conflicts that caused the am to fail.\n\nRegards,\nPaul\n"},{"id":"264365","messageId":"87fa23332f7435292bcac1b0235d77bd@www.dscho.org","threadId":"39670","inReplyTo":"CACRoPnQdR7qC6gH7j9_8gQUYKShLRQCr6GNtmn+XCpe9P4P-nA@mail.gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-19T20:12:42Z","receivedAt":"2015-06-19T20:12:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-06-19 18:15, Paul Tan wrote:\n> On Fri, Jun 19, 2015 at 11:09 PM, Junio C Hamano wrote:\n> \n>> The primary thing I care about is to discourage callers of the API element\n>> am_state from touching these fields with strbuf functions. If it is char * then\n>> the would think twice before modifying (which would involve allocating the\n>> new buffer, forming the new value and assigning into it). With the fields left\n>> as strbuf after they are read or formed by the API (by reading by the API\n>> implementation from $GIT_DIR/rebase-apply/), it is too tempting to do\n>> strbuf_add(), strbuf_trim(), etc., without restoring the value to the original\n>> saying \"I'm the last user of it anyway\"--that is the sloppiness we can prevent\n>> by not leaving it as strbuf.\n> \n> I don't think this is a good deterrent. If the code wanted to, it\n> could just use strbuf_attach()/strbuf_detach() as well, no?\n\nSadly, I am a bit too busy with some loose Git for Windows ends, so I haven't had the chance to look at your code.\n\nBut I still wanted to throw this idea out there: would it maybe possible to avoid having those values as global variables, and instead pass them as const char * parameters to the respective functions? That should help resolve the concerns of both sides because it would allow us to keep the strbuf instances, just as local variables.\n\nCiao,\nDscho\n"},{"id":"264709","messageId":"20150624075446.GA1964@yoshi.chippynet.com","threadId":"39670","inReplyTo":"xmqq1th7brsj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-24T07:54:46Z","receivedAt":"2015-06-24T07:54:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Okay, let's focus only on the API design issues.\n\nFor the record, I'm not completely satisfied with the current code\norganization and API, however I don't have really good ideas at hand to\nimprove it, so any ideas with examples would be appreciated.\n\nOn Fri, Jun 19, 2015 at 09:13:00AM -0700, Junio C Hamano wrote:\n> Paul Tan writes:\n> With the above fields, it is clear that the above fields are\n> per-message thing. So the loop to process multiple messages is\n> conceptually:\n> \n> \n> \tset up the entire am_state (for things like cur=1, last=N)\n> for each message {\n> \t\tupdate per-message fields like cur, author, msg, etc.\n> \t\tprocess the single message\n> clean up per-message fileds like cur++, free(msg), etc.\n> \t}\n> \ttear down the entire am_state\n> \n> Reusing the same strbuf and rely on them to clean up after\n> themselves is simply a bad taste.\n\nHmm, reading \"reusing the same strbuf\" makes me wonder if I'm\nmisunderstanding something. Do you mean that:\n\n1. We should release the memory in state->msg, state->author_* after\n processing each message? Because that is what am_next() already does.\n Granted, it does strbuf_reset(), but it could just as well be\n strbuf_release().\n\nor\n\n2. The per-message fields (state->msg, state->author_*) should become\n local variables in am_run()?\n\nor\n\n3. I'm over-thinking this and you just want the \"struct strbufs\" in the\n struct am_state to be switched to \"char*\"s? If that is so, I've\n attached a sample patch below.\n\nFor (2), my idea is that am_state represents the contents of the state\ndirectory at any point in time -- a design feature carried over from\ngit-am.sh. This is why parse_patch() modifies the am_state struct\ndirectly -- the \"msg\" and \"author_*\" fields would be written to the\n\"final-commit\" and \"author-script\" files directly after.\n\nI think it would be confusing if we don't update the am_state struct\ndirectly, as we will never know if the \"am_state struct\" contains the\nactual current state of the patch application session.\n\n(On a somewhat related thought, currently we do write_file() all over\nthe place, which is really ugly. I'm leaning heavily on introducing an\nam_save() function, for \"I don't care how it is done but just update the\ncontents of the am state directory so that it matches the contents of\nthe struct am_state\". To save pointless writes, we can compare the\nam_state with the last-written/loaded am_state, so we only write the\nfiles that have changed. What do others think?)\n\n> More importantly, we'd want to make sure that people who will be\n> touching the \"process the single message\" part in the above loop to\n> think twice before mucking with read-only fields like author.\n\nOkay, I understand and agree with your reasoning.\n\nHowever, note that this \"process the single message\" part consists\nsolely of run_apply() and do_commit(). Both of them take a\n\"const struct am_state*\", which means that the compiler will warn/fail\nif anybody tries to touch any of the fields in the am_state. This\nextends to strbufs as well. Isn't this already safe enough? Or do you\nwant this safety to extend throughout am_run() as well?\n\n> If they are \"char *\", they would need to allocate new storage\n> themselves, format a new value into there, free the original, and\n> replace the field with the new value. It takes a conscious effort\n> and very much visible code and would be clear to reviewers what is\n> going on, and gives them a chance to question if it is a good idea\n> to update things in-place in the first place.\n\nOkay, although personally I think reviewers may tend to miss out that a\nsingle line like:\n\n state->msg = strbuf_detach(&sb, NULL);\n\nintroduces a memory leak.\n\n> If you left it in strbuf, that invites \"let's extend it temporarily\n> with strbuf_add() and then return to the original once I am done\n> with this single step\", which is an entry to a slippery slope to\n> \"let's extend it with strbuf_add() for my purpose, and I do not even\n> bother to clean up because I know I am the last person to touch\n> this\".\n> \n> So, no, please don't leave a field that won't change during the bulk\n> of the processing in strbuf, unless you have a compelling reason to\n> do so (and \"compelling reasons\" are pretty much limited to \"after\n> all, that field we originally thought it won't change is something\n> we need to change very often\").\n\nAnyway, assuming that you just want the strbufs in the am_state struct\nto be switched to \"char*\"s, here's a quick diff of the result. Can I\nconfirm that this is the direction that you want to take? (since the\nchanges are throughout the entire patch series)\n\nRegards,\nPaul\n\n--- >8 ---\n builtin/am.c | 202 ++++++++++++++++++++++++++++++++++++++---------------------\n 1 file changed, 131 insertions(+), 71 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex f55dd14..944e35a 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -98,10 +98,10 @@ struct am_state {\n \tint last;\n \n \t/* commit message and metadata */\n-\tstruct strbuf author_name;\n-\tstruct strbuf author_email;\n-\tstruct strbuf author_date;\n-\tstruct strbuf msg;\n+\tchar *author_name;\n+\tchar *author_email;\n+\tchar *author_date;\n+\tchar *msg;\n \n \t/* number of digits in patch filename */\n \tint prec;\n@@ -148,10 +148,10 @@ static void am_state_init(struct am_state *state, const char *dir)\n \n \tstate->dir = xstrdup_or_null(dir);\n \n-\tstrbuf_init(&state->author_name, 0);\n-\tstrbuf_init(&state->author_email, 0);\n-\tstrbuf_init(&state->author_date, 0);\n-\tstrbuf_init(&state->msg, 0);\n+\tstate->author_name = NULL;\n+\tstate->author_email = NULL;\n+\tstate->author_date = NULL;\n+\tstate->msg = NULL;\n \n \tstate->prec = 4;\n \n@@ -177,10 +177,17 @@ static void am_state_release(struct am_state *state)\n \tif (state->dir)\n \t\tfree(state->dir);\n \n-\tstrbuf_release(&state->author_name);\n-\tstrbuf_release(&state->author_email);\n-\tstrbuf_release(&state->author_date);\n-\tstrbuf_release(&state->msg);\n+\tif (state->author_name)\n+\t\tfree(state->author_name);\n+\n+\tif (state->author_email)\n+\t\tfree(state->author_email);\n+\n+\tif (state->author_date)\n+\t\tfree(state->author_date);\n+\n+\tif (state->msg)\n+\t\tfree(state->msg);\n \n \targv_array_clear(&state->git_apply_opts);\n }\n@@ -250,37 +257,32 @@ static int read_state_file(struct strbuf *sb, const char *file, size_t hint, int\n }\n \n /**\n- * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE\n- * in `value`. VALUE must be a quoted string, and the KEY must match `key`.\n- * Returns 0 on success, -1 on failure.\n+ * Reads a KEY=VALUE shell variable assignment from fp, returning the VALUE as\n+ * a newly-allocated string. VALUE must be a quoted string, and the KEY must\n+ * match `key`. Returns NULL on failure.\n *\n * This is used by read_author_script() to read the GIT_AUTHOR_* variables from\n * the author-script.\n */\n-static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)\n+static char *read_shell_var(FILE *fp, const char *key)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tchar *str;\n \n \tif (strbuf_getline(&sb, fp, '\\n'))\n-\t\treturn -1;\n+\t\treturn NULL;\n \n \tif (!skip_prefix(sb.buf, key, (const char **)&str))\n-\t\treturn -1;\n+\t\treturn NULL;\n \n \tif (!skip_prefix(str, \"=\", (const char **)&str))\n-\t\treturn -1;\n+\t\treturn NULL;\n \n \tstr = sq_dequote(str);\n \tif (!str)\n-\t\treturn -1;\n-\n-\tstrbuf_reset(value);\n-\tstrbuf_addstr(value, str);\n+\t\treturn NULL;\n \n-\tstrbuf_release(&sb);\n-\n-\treturn 0;\n+\treturn strbuf_detach(&sb, NULL);\n }\n \n /**\n@@ -307,13 +309,25 @@ static int read_author_script(struct am_state *state)\n \t\tdie_errno(_(\"could not open '%s' for reading\"), filename);\n \t}\n \n-\tif (read_shell_var(&state->author_name, fp, \"GIT_AUTHOR_NAME\"))\n+\tif (state->author_name)\n+\t\tfree(state->author_name);\n+\n+\tstate->author_name = read_shell_var(fp, \"GIT_AUTHOR_NAME\");\n+\tif (!state->author_name)\n \t\treturn -1;\n \n-\tif (read_shell_var(&state->author_email, fp, \"GIT_AUTHOR_EMAIL\"))\n+\tif (state->author_email)\n+\t\tfree(state->author_email);\n+\n+\tstate->author_email = read_shell_var(fp, \"GIT_AUTHOR_EMAIL\");\n+\tif (!state->author_email)\n \t\treturn -1;\n \n-\tif (read_shell_var(&state->author_date, fp, \"GIT_AUTHOR_DATE\"))\n+\tif (state->author_date)\n+\t\tfree(state->author_date);\n+\n+\tstate->author_date = read_shell_var(fp, \"GIT_AUTHOR_DATE\");\n+\tif (!state->author_date)\n \t\treturn -1;\n \n \tif (fgetc(fp) != EOF)\n@@ -336,9 +350,9 @@ static void write_author_script(const struct am_state *state)\n \tstruct strbuf author_email = STRBUF_INIT;\n \tstruct strbuf author_date = STRBUF_INIT;\n \n-\tsq_quote_buf(&author_name, state->author_name.buf);\n-\tsq_quote_buf(&author_email, state->author_email.buf);\n-\tsq_quote_buf(&author_date, state->author_date.buf);\n+\tsq_quote_buf(&author_name, state->author_name);\n+\tsq_quote_buf(&author_email, state->author_email);\n+\tsq_quote_buf(&author_date, state->author_date);\n \n \twrite_file(am_path(state, \"author-script\"), 1, fmt,\n \t\t\tauthor_name.buf, author_email.buf, author_date.buf);\n@@ -364,7 +378,10 @@ static void am_load(struct am_state *state)\n \tif (read_author_script(state) < 0)\n \t\tdie(_(\"could not parse author script\"));\n \n-\tread_state_file(&state->msg, am_path(state, \"final-commit\"), 0, 0);\n+\tread_state_file(&sb, am_path(state, \"final-commit\"), 0, 0);\n+\tif (state->msg)\n+\t\tfree(state->msg);\n+\tstate->msg = strbuf_detach(&sb, NULL);\n \n \tread_state_file(&sb, am_path(state, \"threeway\"), 2, 1);\n \tstate->threeway = !strcmp(sb.buf, \"t\");\n@@ -670,12 +687,24 @@ static void am_next(struct am_state *state)\n \tstate->cur++;\n \twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n \n-\tstrbuf_reset(&state->author_name);\n-\tstrbuf_reset(&state->author_email);\n-\tstrbuf_reset(&state->author_date);\n+\tif (state->author_name)\n+\t\tfree(state->author_name);\n+\tstate->author_name = NULL;\n+\n+\tif (state->author_email)\n+\t\tfree(state->author_email);\n+\tstate->author_email = NULL;\n+\n+\tif (state->author_date)\n+\t\tfree(state->author_date);\n+\tstate->author_date = NULL;\n+\n \tunlink(am_path(state, \"author-script\"));\n \n-\tstrbuf_reset(&state->msg);\n+\tif (state->msg)\n+\t\tfree(state->msg);\n+\tstate->msg = NULL;\n+\n \tunlink(am_path(state, \"final-commit\"));\n \n \tif (!get_sha1(\"HEAD\", head))\n@@ -762,6 +791,10 @@ static int parse_patch(struct am_state *state, const char *patch)\n \tFILE *fp;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf msg = STRBUF_INIT;\n+\tstruct strbuf author_name = STRBUF_INIT;\n+\tstruct strbuf author_date = STRBUF_INIT;\n+\tstruct strbuf author_email = STRBUF_INIT;\n \n \tcp.git_cmd = 1;\n \tcp.in = xopen(patch, O_RDONLY, 0);\n@@ -813,20 +846,32 @@ static int parse_patch(struct am_state *state, const char *patch)\n \t\tconst char *x;\n \n \t\tif (skip_prefix(sb.buf, \"Subject: \", &x)) {\n-\t\t\tif (state->msg.len)\n-\t\t\t\tstrbuf_addch(&state->msg, '\\n');\n-\t\t\tstrbuf_addstr(&state->msg, x);\n+\t\t\tif (msg.len)\n+\t\t\t\tstrbuf_addch(&msg, '\\n');\n+\t\t\tstrbuf_addstr(&msg, x);\n \t\t} else if (skip_prefix(sb.buf, \"Author: \", &x))\n-\t\t\tstrbuf_addstr(&state->author_name, x);\n+\t\t\tstrbuf_addstr(&author_name, x);\n \t\telse if (skip_prefix(sb.buf, \"Email: \", &x))\n-\t\t\tstrbuf_addstr(&state->author_email, x);\n+\t\t\tstrbuf_addstr(&author_email, x);\n \t\telse if (skip_prefix(sb.buf, \"Date: \", &x))\n-\t\t\tstrbuf_addstr(&state->author_date, x);\n+\t\t\tstrbuf_addstr(&author_date, x);\n \t}\n \tfclose(fp);\n \n+\tif (state->author_name)\n+\t\tfree(state->author_name);\n+\tstate->author_name = strbuf_detach(&author_name, NULL);\n+\n+\tif (state->author_email)\n+\t\tfree(state->author_email);\n+\tstate->author_email = strbuf_detach(&author_email, NULL);\n+\n+\tif (state->author_date)\n+\t\tfree(state->author_date);\n+\tstate->author_date = strbuf_detach(&author_date, NULL);\n+\n \t/* Skip pine's internal folder data */\n-\tif (!strcmp(state->author_name.buf, \"Mail System Internal Data\"))\n+\tif (!strcmp(state->author_name, \"Mail System Internal Data\"))\n \t\treturn 1;\n \n \tif (is_empty_file(am_path(state, \"patch\")))\n@@ -834,13 +879,17 @@ static int parse_patch(struct am_state *state, const char *patch)\n \t\t\"If you would prefer to skip this patch, instead run \\\"git am --skip\\\".\\n\"\n \t\t\"To restore the original branch and stop patching run \\\"git am --abort\\\".\"));\n \n-\tstrbuf_addstr(&state->msg, \"\\n\\n\");\n-\tif (strbuf_read_file(&state->msg, am_path(state, \"msg\"), 0) < 0)\n+\tstrbuf_addstr(&msg, \"\\n\\n\");\n+\tif (strbuf_read_file(&msg, am_path(state, \"msg\"), 0) < 0)\n \t\tdie_errno(_(\"could not read '%s'\"), am_path(state, \"msg\"));\n-\tstripspace(&state->msg, 0);\n+\tstripspace(&msg, 0);\n \n \tif (state->sign)\n-\t\tappend_signoff(&state->msg, 0, 0);\n+\t\tappend_signoff(&msg, 0, 0);\n+\n+\tif (state->msg)\n+\t\tfree(state->msg);\n+\tstate->msg = strbuf_detach(&msg, NULL);\n \n \treturn 0;\n }\n@@ -876,6 +925,9 @@ static int get_patch_commit_sha1(unsigned char *commit_id, const char *patch)\n static void get_commit_info(struct am_state *state, struct commit *commit)\n {\n \tconst char *buf, *begin, *end;\n+\tstruct strbuf author_name = STRBUF_INIT;\n+\tstruct strbuf author_email = STRBUF_INIT;\n+\tstruct strbuf author_date = STRBUF_INIT;\n \n \tbuf = logmsg_reencode(commit, NULL, get_commit_output_encoding());\n \tbegin = end = buf;\n@@ -895,24 +947,36 @@ static void get_commit_info(struct am_state *state, struct commit *commit)\n \t\t\t}\n \n \t\t\tif (split.name_begin)\n-\t\t\t\tstrbuf_add(&state->author_name,\n-\t\t\t\t\tsplit.name_begin,\n+\t\t\t\tstrbuf_add(&author_name, split.name_begin,\n \t\t\t\t\tsplit.name_end - split.name_begin);\n \n \t\t\tif (split.mail_begin)\n-\t\t\t\tstrbuf_add(&state->author_email,\n-\t\t\t\t\tsplit.mail_begin,\n+\t\t\t\tstrbuf_add(&author_email, split.mail_begin,\n \t\t\t\t\tsplit.mail_end - split.mail_begin);\n \n \t\t\tdate = show_ident_date(&split, DATE_NORMAL);\n-\t\t\tstrbuf_addstr(&state->author_date, date);\n+\t\t\tstrbuf_addstr(&author_date, date);\n \t\t} else if (begin == end) {\n \t\t\tend++;\n \t\t\tbreak;\n \t\t}\n \t}\n \n-\tstrbuf_addstr(&state->msg, end);\n+\tif (state->author_name)\n+\t\tfree(state->author_name);\n+\tstate->author_name = strbuf_detach(&author_name, NULL);\n+\n+\tif (state->author_email)\n+\t\tfree(state->author_email);\n+\tstate->author_email = strbuf_detach(&author_email, NULL);\n+\n+\tif (state->author_date)\n+\t\tfree(state->author_date);\n+\tstate->author_date = strbuf_detach(&author_date, NULL);\n+\n+\tif (state->msg)\n+\t\tfree(state->msg);\n+\tstate->msg = xstrdup(end);\n }\n \n /**\n@@ -1107,7 +1171,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa\n \tinit_merge_options(&o);\n \n \to.branch1 = \"HEAD\";\n-\this_tree_name = xstrfmt(\"%.*s\", linelen(state->msg.buf), state->msg.buf);\n+\this_tree_name = xstrfmt(\"%.*s\", linelen(state->msg), state->msg);\n \to.branch2 = his_tree_name;\n \n \tif (state->quiet)\n@@ -1147,15 +1211,15 @@ static void do_commit(const struct am_state *state)\n \t\tsay(state, stderr, _(\"applying to an empty history\"));\n \t}\n \n-\tauthor = fmt_ident(state->author_name.buf, state->author_email.buf,\n-\t\t\tstate->ignore_date ? NULL : state->author_date.buf,\n+\tauthor = fmt_ident(state->author_name, state->author_email,\n+\t\t\tstate->ignore_date ? NULL : state->author_date,\n \t\t\tIDENT_STRICT);\n \n \tif (state->committer_date_is_author_date)\n \t\tsetenv(\"GIT_COMMITTER_DATE\",\n-\t\t\tstate->ignore_date ? \"\" : state->author_date.buf, 1);\n+\t\t\tstate->ignore_date ? \"\" : state->author_date, 1);\n \n-\tif (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,\n+\tif (commit_tree(state->msg, strlen(state->msg), tree, parents, commit,\n \t\t\t\tauthor, state->sign_commit))\n \t\tdie(_(\"failed to write commit object\"));\n \n@@ -1163,8 +1227,8 @@ static void do_commit(const struct am_state *state)\n \tif (!reflog_msg)\n \t\treflog_msg = \"am\";\n \n-\tstrbuf_addf(&sb, \"%s: %.*s\", reflog_msg, linelen(state->msg.buf),\n-\t\t\tstate->msg.buf);\n+\tstrbuf_addf(&sb, \"%s: %.*s\", reflog_msg, linelen(state->msg),\n+\t\t\tstate->msg);\n \n \tupdate_ref(sb.buf, \"HEAD\", commit, ptr, 0, UPDATE_REFS_DIE_ON_ERR);\n \n@@ -1191,7 +1255,7 @@ static void am_run(struct am_state *state)\n \tstrbuf_release(&sb);\n \n \twhile (state->cur <= state->last) {\n-\t\tconst char *patch, *msg;\n+\t\tconst char *patch;\n \t\tint apply_status, skip;\n \n \t\tpatch = am_path(state, msgnum(state));\n@@ -1208,10 +1272,10 @@ static void am_run(struct am_state *state)\n \t\t\tgoto next; /* patch should be skipped */\n \n \t\twrite_author_script(state);\n-\t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n+\t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg);\n \n-\t\tmsg = state->msg.buf;\n-\t\tsay(state, stdout, _(\"Applying: %.*s\"), linelen(msg), msg);\n+\t\tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg),\n+\t\t\tstate->msg);\n \n \t\tapply_status = run_apply(state, NULL);\n \n@@ -1235,9 +1299,8 @@ static void am_run(struct am_state *state)\n \t\tif (apply_status) {\n \t\t\tint advice_amworkdir = 1;\n \n-\t\t\tmsg = state->msg.buf;\n \t\t\tprintf_ln(_(\"Patch failed at %s %.*s\"), msgnum(state),\n-\t\t\t\t\tlinelen(msg), msg);\n+\t\t\t\tlinelen(state->msg), state->msg);\n \n \t\t\tgit_config_get_bool(\"advice.amworkdir\", &advice_amworkdir);\n \n@@ -1267,10 +1330,7 @@ next:\n */\n static void am_resolve(struct am_state *state)\n {\n-\tconst char *msg;\n-\n-\tmsg = state->msg.buf;\n-\tsay(state, stdout, _(\"Applying: %.*s\"), linelen(msg), msg);\n+\tsay(state, stdout, _(\"Applying: %.*s\"), linelen(state->msg), state->msg);\n \n \tif (!index_has_changes(NULL)) {\n \t\tprintf_ln(_(\"No changes - did you forget to use 'git add'?\\n\"\n"},{"id":"264712","messageId":"CACRoPnSoFDnuJC-q3nR7xwpqOcpmzYReDPPPao1NJdY11TV0cQ@mail.gmail.com","threadId":"39670","inReplyTo":"xmqqy4jgd92l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-24T08:41:09Z","receivedAt":"2015-06-24T08:41:09Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, Jun 19, 2015 at 5:02 AM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> +static int is_email(const char *filename)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + FILE *fp = xfopen(filename, \"r\");\n>> + int ret = 1;\n>> +\n>> + while (!strbuf_getline(&sb, fp, '\\n')) {\n>> + const char *x;\n>> +\n>> + strbuf_rtrim(&sb);\n>\n> Is this a good thing? strbuf_getline() already has stripped the LF\n> at the end, so you'd be treating a line with only whitespaces as if\n> it is a truly empty line.\n>\n> I know the series is about literal translation and the script may\n> lose the distinction between the two, but I do not think you need\n> (or want) to be literally same for things like this.\n>\n> Same comment applies to other uses of \"trim\" in this patch.\n\nNo, the uses of strbuf_trim() are not good at all. Will remove them.\n\nThanks,\nPaul\n"},{"id":"264724","messageId":"6560adbc8b1842dd369628980f1264c3@www.dscho.org","threadId":"39670","inReplyTo":"1434626743-8552-5-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-24T14:59:48Z","receivedAt":"2015-06-24T14:59:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-06-18 13:25, Paul Tan wrote:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index dbc8836..af68c51 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -6,6 +6,158 @@\n> #include \"cache.h\"\n> #include \"builtin.h\"\n> #include \"exec_cmd.h\"\n> +#include \"parse-options.h\"\n> +#include \"dir.h\"\n> +\n> +struct am_state {\n> +\t/* state directory path */\n> +\tstruct strbuf dir;\n> +\n> +\t/* current and last patch numbers, 1-indexed */\n> +\tint cur;\n> +\tint last;\n> +};\n> +\n> +/**\n> + * Initializes am_state with the default values.\n> + */\n> +static void am_state_init(struct am_state *state)\n> +{\n> +\tmemset(state, 0, sizeof(*state));\n> +\n> +\tstrbuf_init(&state->dir, 0);\n> +}\n\nWith strbufs, we use the initializer STRBUF_INIT. How about using\n\n#define AM_STATE_INIT { STRBUF_INIT, 0, 0 }\n\nhere?\n\n> +/**\n> + * Reads the contents of `file`. The third argument can be used to give a hint\n> + * about the file size, to avoid reallocs. Returns number of bytes read on\n> + * success, -1 if the file does not exist. If trim is set, trailing whitespace\n> + * will be removed from the file contents.\n> + */\n> +static int read_state_file(struct strbuf *sb, const char *file,\n> size_t hint, int trim)\n> +{\n> +\tstrbuf_reset(sb);\n> +\tif (strbuf_read_file(sb, file, hint) >= 0) {\n> +\t\tif (trim)\n> +\t\t\tstrbuf_trim(sb);\n> +\n> +\t\treturn sb->len;\n> +\t}\n> +\n> +\tif (errno == ENOENT)\n> +\t\treturn -1;\n> +\n> +\tdie_errno(_(\"could not read '%s'\"), file);\n> +}\n\nA couple of thoughts:\n\n- 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.\n\n- 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.\n\n- 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).\n\n- If you agree with these suggestions, the signature would become something like\n\nstatic void read_state_file(struct am_state *state, const char *basename, int trim);\n\n> +/**\n> + * Remove the am_state directory.\n> + */\n> +static void am_destroy(const struct am_state *state)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\tstrbuf_addstr(&sb, state->dir.buf);\n> +\tremove_dir_recursively(&sb, 0);\n> +\tstrbuf_release(&sb);\n> +}\n\nGiven 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\n\n remove_dir_recursively(&state->dir, 0);\n\nand do not need another temporary strbuf.\n\n> +/**\n> + * Increments the patch pointer, and cleans am_state for the application of the\n> + * next patch.\n> + */\n> +static void am_next(struct am_state *state)\n> +{\n> +\tstate->cur++;\n> +\twrite_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n> +}\n\nLocking and re-checking the contents of \"next\" before writing the incremented value would probably be a little too paranoid...\n\n(Just saying it out loud, the current code is fine by me.)\n\nCiao,\nDscho\n"},{"id":"264725","messageId":"334feea4cee88d06a10a7363956d2bfe@www.dscho.org","threadId":"39670","inReplyTo":"1434626743-8552-7-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-24T15:10:27Z","receivedAt":"2015-06-24T15:10:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-06-18 13:25, Paul Tan wrote:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index e9a3687..7b97ea8 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)\n> \tstrbuf_release(&sb);\n> }\n> \n> +/*\n> + * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.\n> + * We check this by grabbing all the non-indented lines and seeing if they look\n> + * like they begin with valid header field names.\n> + */\n> +static int is_email(const char *filename)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tFILE *fp = xfopen(filename, \"r\");\n> +\tint ret = 1;\n> +\n> +\twhile (!strbuf_getline(&sb, fp, '\\n')) {\n> +\t\tconst char *x;\n> +\n> +\t\tstrbuf_rtrim(&sb);\n> +\n> +\t\tif (!sb.len)\n> +\t\t\tbreak; /* End of header */\n> +\n> +\t\t/* Ignore indented folded lines */\n> +\t\tif (*sb.buf == '\\t' || *sb.buf == ' ')\n> +\t\t\tcontinue;\n> +\n> +\t\t/* It's a header if it matches the regexp \"^[!-9;-~]+:\" */\n\nWhy not just compile a regex and use it here? We use regexes elsewhere anyway...\n\n> +/**\n> + * Attempts to detect the patch_format of the patches contained in `paths`,\n> + * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if\n> + * detection fails.\n> + */\n> +static int detect_patch_format(struct string_list *paths)\n> +{\n> +\tenum patch_format ret = PATCH_FORMAT_UNKNOWN;\n> +\tstruct strbuf l1 = STRBUF_INIT;\n> +\tstruct strbuf l2 = STRBUF_INIT;\n> +\tstruct strbuf l3 = STRBUF_INIT;\n> +\tFILE *fp;\n> +\n> +\t/*\n> +\t * We default to mbox format if input is from stdin and for directories\n> +\t */\n> +\tif (!paths->nr || !strcmp(paths->items->string, \"-\") ||\n> +\t is_directory(paths->items->string)) {\n> +\t\tret = PATCH_FORMAT_MBOX;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\t/*\n> +\t * Otherwise, check the first 3 lines of the first patch, starting\n> +\t * from the first non-blank line, to try to detect its format.\n> +\t */\n> +\tfp = xfopen(paths->items->string, \"r\");\n> +\twhile (!strbuf_getline(&l1, fp, '\\n')) {\n> +\t\tstrbuf_trim(&l1);\n> +\t\tif (l1.len)\n> +\t\t\tbreak;\n> +\t}\n> +\tstrbuf_getline(&l2, fp, '\\n');\n\nWe 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.\n\n> +\tstrbuf_trim(&l2);\n> +\tstrbuf_getline(&l3, fp, '\\n');\n> +\tstrbuf_trim(&l3);\n> +\tfclose(fp);\n> +\n> +\tif (starts_with(l1.buf, \"From \") || starts_with(l1.buf, \"From: \"))\n> +\t\tret = PATCH_FORMAT_MBOX;\n\nHmm. We can test that earlier and return without reading from the file any further, I think.\n\n> +\telse if (l1.len && l2.len && l3.len && is_email(paths->items->string))\n> +\t\tret = PATCH_FORMAT_MBOX;\n\nMaybe 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.\n\nThe rest looks very nice!\nDscho\n"},{"id":"264733","messageId":"xmqqa8vpayhg.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"20150624075446.GA1964@yoshi.chippynet.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-24T15:59:39Z","receivedAt":"2015-06-24T15:59:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> 3. I'm over-thinking this and you just want the \"struct strbufs\" in the\n> struct am_state to be switched to \"char*\"s?\n\nYes, everybody interacts with am_state, and these fields are\nsupposed to be constant during the processing of each patch input\nmessage; they should be simple strings, not strbufs, to make sure if\nanybody _does_ muck with them in-place, that would be very visible.\nThe helpers to initialize them are free to use strbuf API to prepare\nthese simple string fields, of course.\n\n> (On a somewhat related thought, currently we do write_file() all over\n> the place, which is really ugly. I'm leaning heavily on introducing an\n> am_save() function, for \"I don't care how it is done but just update the\n> contents of the am state directory so that it matches the contents of\n> the struct am_state\".\n\nSure; the scripted Porcelain may have done \"echo here, echo there\"\ninstead of \"concatenate into a $var and then 'echo $var' at end\" as\nthat is more natural way to program in that environment. You are\ndoing this in C and \"prepare the thing in-core and write it all at\nthe point to snapshot\" may well be the more natural way to program.\n\nAs long as a process that stops in the middle does not leave on-disk\nstate inconsistent, batching would be fine. For example, you may\napply and commit two (or more) patches without updating the on-disk\nstate as you do not see need to give control back to the user\n(i.e. they applied cleanly) and then write out the on-disk state\nwith .next incremented by two (or more) before giving the control\nback could be a valid optimization (take this example with a grain\nof salt, though; I haven't thought too deeply about what should\nhappen if you Ctrl-C the process in the middle).\n"},{"id":"264735","messageId":"ed4f6565ed610a337244e06cc15b41bb@www.dscho.org","threadId":"39670","inReplyTo":"1434626743-8552-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-24T16:28:48Z","receivedAt":"2015-06-24T16:28:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-06-18 13:25, Paul Tan wrote:\n\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 0cc7ae8..bc77d77 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -719,6 +719,7 @@ extern void *xrealloc(void *ptr, size_t size);\n> extern void *xcalloc(size_t nmemb, size_t size);\n> extern void *xmmap(void *start, size_t length, int prot, int flags,\n> int fd, off_t offset);\n> extern void *xmmap_gently(void *start, size_t length, int prot, int\n> flags, int fd, off_t offset);\n> +extern int xopen(const char *path, int flags, ...);\n\nI 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?\n\n> diff --git a/wrapper.c b/wrapper.c\n> index c1a663f..82658b3 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)\n> # endif\n> #endif\n> \n> +/**\n> + * xopen() is the same as open(), but it die()s if the open() fails.\n> + */\n> +int xopen(const char *path, int oflag, ...)\n> +{\n> +\tmode_t mode = 0;\n> +\tva_list ap;\n> +\n> +\tva_start(ap, oflag);\n> +\tif (oflag & O_CREAT)\n> +\t\tmode = va_arg(ap, mode_t);\n> +\tva_end(ap);\n> +\n> +\tassert(path);\n> +\n> +\tfor (;;) {\n> +\t\tint fd = open(path, oflag, mode);\n> +\t\tif (fd >= 0)\n> +\t\t\treturn fd;\n> +\t\tif (errno == EINTR)\n> +\t\t\tcontinue;\n> +\t\tdie_errno(_(\"could not open '%s'\"), path);\n\nIt is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like\n\nif (oflag & O_WRITE)\n die_errno(_(\"could not open '%s' for writing\"), path);\nelse if (oflag & O_READ)\n die_errno(_(\"could not open '%s' for reading\"), path);\nelse\n die_errno(_(\"could not open '%s'\"), path);\n\n? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.\n\nCiao,\nDscho\n"},{"id":"264736","messageId":"72c470f3a6890dfcb66f1439383d2278@www.dscho.org","threadId":"39670","inReplyTo":"1434626743-8552-8-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-24T16:36:16Z","receivedAt":"2015-06-24T16:36:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Paul,\n\nOn 2015-06-18 13:25, Paul Tan wrote:\n\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 7b97ea8..d6434e4 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb,\n> const char *file, size_t hint, int\n> }\n> \n> /**\n> + * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE\n> + * in `value`. VALUE must be a quoted string, and the KEY must match `key`.\n> + * Returns 0 on success, -1 on failure.\n> + *\n> + * This is used by read_author_script() to read the GIT_AUTHOR_* variables from\n> + * the author-script.\n> + */\n> +static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar *str;\n> +\n> +\tif (strbuf_getline(&sb, fp, '\\n'))\n> +\t\treturn -1;\n> +\n> +\tif (!skip_prefix(sb.buf, key, (const char **)&str))\n> +\t\treturn -1;\n> +\n> +\tif (!skip_prefix(str, \"=\", (const char **)&str))\n> +\t\treturn -1;\n> +\n> +\tstr = sq_dequote(str);\n> +\tif (!str)\n> +\t\treturn -1;\n> +\n> +\tstrbuf_reset(value);\n> +\tstrbuf_addstr(value, str);\n> +\n> +\tstrbuf_release(&sb);\n> +\n> +\treturn 0;\n> +}\n\nHow 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:\n\nstatic int read_author_script(struct am_state *state)\n{\n\tstruct strbuf sb = STRBUF_INIT;\n\tconst char *filename = am_path(state, \"author-script\");\n\tFILE *fp = fopen(filename, \"r\");\n\tif (!fp) {\n\t\tif (errno == ENOENT)\n\t\t\treturn 0;\n\t\tdie_errno(_(\"could not open '%s' for reading\"), filename);\n\t}\n\n\twhile (!strbuf_getline(&sb, fp, '\\n')) {\n\t\tchar *equal = strchr(sb.buf, '='), **var;\n\n\t\tif (!equal) {\nerror:\n\t\t\tfclose(fp);\n\t\t\treturn -1;\n\t\t}\n\t\t*equal = '\\0';\n\t\tif (!strcmp(sb.buf, \"GIT_AUTHOR_NAME\"))\n\t\t\tvar = &state->author_name;\n\t\telse if (!strcmp(sb.buf, \"GIT_AUTHOR_EMAIL\"))\n\t\t\tvar = &state->author_email;\n\t\telse if (!strcmp(sb.buf, \"GIT_AUTHOR_DATE\"))\n\t\t\tvar = &state->author_date;\n\t\telse\n\t\t\tgoto error;\n\t\t*var = xstrdup(sq_dequote(equal + 1));\n\t}\n\n\tfclose(fp);\n\treturn -1;\n}\n\nIf you follow my earlier suggestion to keep a strbuf inside the am_state, you could reuse that here, too.\n\n> +/**\n> + * Saves state->author_name, state->author_email and state->author_date in\n> + * `filename` as an \"author script\", which is the format used by git-am.sh.\n> + */\n> +static void write_author_script(const struct am_state *state)\n> +{\n> +\tstatic const char fmt[] = \"GIT_AUTHOR_NAME=%s\\n\"\n> +\t\t\"GIT_AUTHOR_EMAIL=%s\\n\"\n> +\t\t\"GIT_AUTHOR_DATE=%s\\n\";\n> +\tstruct strbuf author_name = STRBUF_INIT;\n> +\tstruct strbuf author_email = STRBUF_INIT;\n> +\tstruct strbuf author_date = STRBUF_INIT;\n> +\n> +\tsq_quote_buf(&author_name, state->author_name.buf);\n> +\tsq_quote_buf(&author_email, state->author_email.buf);\n> +\tsq_quote_buf(&author_date, state->author_date.buf);\n\nThe `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.\n\nThat scratch pad could come in handy in a couple of other places in the rest of this patch.\n\nCiao,\nDscho\n"},{"id":"264737","messageId":"CAGZ79kYtF+rHozVQhNz8uguLZvArPiB2UdkPTFn=Vc+YCsA=UQ@mail.gmail.com","threadId":"39670","inReplyTo":"ed4f6565ed610a337244e06cc15b41bb@www.dscho.org","subject":"Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-06-24T16:59:26Z","receivedAt":"2015-06-24T16:59:26Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Jun 24, 2015 at 9:28 AM, Johannes Schindelin\n wrote:\n> Hi Paul,\n>\n> On 2015-06-18 13:25, Paul Tan wrote:\n>\n>> diff --git a/git-compat-util.h b/git-compat-util.h\n>> index 0cc7ae8..bc77d77 100644\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -719,6 +719,7 @@ extern void *xrealloc(void *ptr, size_t size);\n>> extern void *xcalloc(size_t nmemb, size_t size);\n>> extern void *xmmap(void *start, size_t length, int prot, int flags,\n>> int fd, off_t offset);\n>> extern void *xmmap_gently(void *start, size_t length, int prot, int\n>> flags, int fd, off_t offset);\n>> +extern int xopen(const char *path, int flags, ...);\n>\n> 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?\n>\n>> diff --git a/wrapper.c b/wrapper.c\n>> index c1a663f..82658b3 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -189,6 +189,31 @@ void *xcalloc(size_t nmemb, size_t size)\n>> # endif\n>> #endif\n>>\n>> +/**\n>> + * xopen() is the same as open(), but it die()s if the open() fails.\n>> + */\n>> +int xopen(const char *path, int oflag, ...)\n>> +{\n>> + mode_t mode = 0;\n>> + va_list ap;\n>> +\n>> + va_start(ap, oflag);\n>> + if (oflag & O_CREAT)\n>> + mode = va_arg(ap, mode_t);\n>> + va_end(ap);\n>> +\n>> + assert(path);\n>> +\n>> + for (;;) {\n>> + int fd = open(path, oflag, mode);\n>> + if (fd >= 0)\n>> + return fd;\n>> + if (errno == EINTR)\n>> + continue;\n>> + die_errno(_(\"could not open '%s'\"), path);\n>\n> It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like\n>\n> if (oflag & O_WRITE)\n> die_errno(_(\"could not open '%s' for writing\"), path);\n> else if (oflag & O_READ)\n> die_errno(_(\"could not open '%s' for reading\"), path);\n> else\n> die_errno(_(\"could not open '%s'\"), path);\n>\n> ? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.\n\nThis distinction was part of earlier series, but Torsten Boegershausen\nsuggested to leave\nit out. [compare\nhttp://thread.gmane.org/gmane.comp.version-control.git/270048/focus=270049]\n\n>\n> Ciao,\n> Dscho\n>\n>\n"},{"id":"264749","messageId":"40fbc9dd1a066ba137647ff384aa6395@www.dscho.org","threadId":"39670","inReplyTo":"CAGZ79kYtF+rHozVQhNz8uguLZvArPiB2UdkPTFn=Vc+YCsA=UQ@mail.gmail.com","subject":"Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2015-06-24T18:39:33Z","receivedAt":"2015-06-24T18:39:33Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Stefan,\n\nOn 2015-06-24 18:59, Stefan Beller wrote:\n> On Wed, Jun 24, 2015 at 9:28 AM, Johannes Schindelin\n> wrote:\n>>\n>> On 2015-06-18 13:25, Paul Tan wrote:\n>>\n>>> + int fd = open(path, oflag, mode);\n>>> + if (fd >= 0)\n>>> + return fd;\n>>> + if (errno == EINTR)\n>>> + continue;\n>>> + die_errno(_(\"could not open '%s'\"), path);\n>>\n>> It is often helpful to know whether a path was opened for reading or writing, so maybe we should have something like\n>>\n>> if (oflag & O_WRITE)\n>> die_errno(_(\"could not open '%s' for writing\"), path);\n>> else if (oflag & O_READ)\n>> die_errno(_(\"could not open '%s' for reading\"), path);\n>> else\n>> die_errno(_(\"could not open '%s'\"), path);\n>>\n>> ? I know it is a bit of duplication, but I fear we cannot get around that without breaking i18n support.\n> \n> This distinction was part of earlier series, but Torsten Boegershausen\n> suggested to leave\n> it out. [compare\n> http://thread.gmane.org/gmane.comp.version-control.git/270048/focus=270049]\n\nSo 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.\n\nIMO 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).\n\nCiao,\nDscho\n"},{"id":"264840","messageId":"CACRoPnSD5kHof6yeRLqP6E7LcBjiHR1it3cadcRUCA3T-g=kUw@mail.gmail.com","threadId":"39670","inReplyTo":"xmqqa8vpayhg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-25T11:54:47Z","receivedAt":"2015-06-25T11:54:47Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Jun 24, 2015 at 11:59 PM, Junio C Hamano wrote:\n> Paul Tan writes:\n>\n>> 3. I'm over-thinking this and you just want the \"struct strbufs\" in the\n>> struct am_state to be switched to \"char*\"s?\n>\n> Yes, everybody interacts with am_state, and these fields are\n> supposed to be constant during the processing of each patch input\n> message; they should be simple strings, not strbufs, to make sure if\n> anybody _does_ muck with them in-place, that would be very visible.\n> The helpers to initialize them are free to use strbuf API to prepare\n> these simple string fields, of course.\n\nOK. I've implemented it on my end.\n\n>> (On a somewhat related thought, currently we do write_file() all over\n>> the place, which is really ugly. I'm leaning heavily on introducing an\n>> am_save() function, for \"I don't care how it is done but just update the\n>> contents of the am state directory so that it matches the contents of\n>> the struct am_state\".\n>\n> Sure; the scripted Porcelain may have done \"echo here, echo there\"\n> instead of \"concatenate into a $var and then 'echo $var' at end\" as\n> that is more natural way to program in that environment. You are\n> doing this in C and \"prepare the thing in-core and write it all at\n> the point to snapshot\" may well be the more natural way to program.\n>\n> As long as a process that stops in the middle does not leave on-disk\n> state inconsistent, batching would be fine. For example, you may\n> apply and commit two (or more) patches without updating the on-disk\n> state as you do not see need to give control back to the user\n> (i.e. they applied cleanly) and then write out the on-disk state\n> with .next incremented by two (or more) before giving the control\n> back could be a valid optimization (take this example with a grain\n> of salt, though; I haven't thought too deeply about what should\n> happen if you Ctrl-C the process in the middle).\n\nRight, I briefly wondered if we could hold off writing the am_state to\ndisk as much as possible, and only write to disk when we are about to\nterminate. This would involve installing an atexit() and SIGTERM\nsignal handler, but I haven't thought too deeply about that.\n\nAnyway, moving all the \"writing am_state to disk\" logic to a central\nfunction am_save() would be a good immediate step, I think, so I've\nimplemented it on my end.\n\nThanks,\nPaul\n"},{"id":"264848","messageId":"CACRoPnQAGt_mLbG-EtJ+qTpD2OvNgi-MZfU=NoKCoSmhCOnBEw@mail.gmail.com","threadId":"39670","inReplyTo":"334feea4cee88d06a10a7363956d2bfe@www.dscho.org","subject":"Re: [PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-25T13:40:30Z","receivedAt":"2015-06-25T13:40:30Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Jun 24, 2015 at 11:10 PM, Johannes Schindelin\n wrote:\n> Hi Paul,\n>\n> On 2015-06-18 13:25, Paul Tan wrote:\n>\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index e9a3687..7b97ea8 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -121,6 +121,96 @@ static void am_destroy(const struct am_state *state)\n>> strbuf_release(&sb);\n>> }\n>>\n>> +/*\n>> + * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.\n>> + * We check this by grabbing all the non-indented lines and seeing if they look\n>> + * like they begin with valid header field names.\n>> + */\n>> +static int is_email(const char *filename)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + FILE *fp = xfopen(filename, \"r\");\n>> + int ret = 1;\n>> +\n>> + while (!strbuf_getline(&sb, fp, '\\n')) {\n>> + const char *x;\n>> +\n>> + strbuf_rtrim(&sb);\n>> +\n>> + if (!sb.len)\n>> + break; /* End of header */\n>> +\n>> + /* Ignore indented folded lines */\n>> + if (*sb.buf == '\\t' || *sb.buf == ' ')\n>> + continue;\n>> +\n>> + /* It's a header if it matches the regexp \"^[!-9;-~]+:\" */\n>\n> Why not just compile a regex and use it here? We use regexes elsewhere anyway...\n\nAh, you're right. A regular expression would definitely be clearer.\nI've fixed it on my end.\n\n>> +/**\n>> + * Attempts to detect the patch_format of the patches contained in `paths`,\n>> + * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if\n>> + * detection fails.\n>> + */\n>> +static int detect_patch_format(struct string_list *paths)\n>> +{\n>> + enum patch_format ret = PATCH_FORMAT_UNKNOWN;\n>> + struct strbuf l1 = STRBUF_INIT;\n>> + struct strbuf l2 = STRBUF_INIT;\n>> + struct strbuf l3 = STRBUF_INIT;\n>> + FILE *fp;\n>> +\n>> + /*\n>> + * We default to mbox format if input is from stdin and for directories\n>> + */\n>> + if (!paths->nr || !strcmp(paths->items->string, \"-\") ||\n>> + is_directory(paths->items->string)) {\n>> + ret = PATCH_FORMAT_MBOX;\n>> + goto done;\n>> + }\n>> +\n>> + /*\n>> + * Otherwise, check the first 3 lines of the first patch, starting\n>> + * from the first non-blank line, to try to detect its format.\n>> + */\n>> + fp = xfopen(paths->items->string, \"r\");\n>> + while (!strbuf_getline(&l1, fp, '\\n')) {\n>> + strbuf_trim(&l1);\n>> + if (l1.len)\n>> + break;\n>> + }\n>> + strbuf_getline(&l2, fp, '\\n');\n>\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.\n\nAh OK. I'll vote for doing a strbuf_reset() just before the\nstrbuf_getline() though.\n\n>> + strbuf_trim(&l2);\n>> + strbuf_getline(&l3, fp, '\\n');\n>> + strbuf_trim(&l3);\n>> + fclose(fp);\n>> +\n>> + if (starts_with(l1.buf, \"From \") || starts_with(l1.buf, \"From: \"))\n>> + ret = PATCH_FORMAT_MBOX;\n>\n> Hmm. We can test that earlier and return without reading from the file any further, I think.\n\nThe \"reading 3 lines at the beginning\" logic is meant to support a\nlater patch where support for StGit and mercurial patches is added.\nThat said, it's true that we don't need to read 3 lines in this patch,\nso I think I will remove it in this patch.\n\n>> + else if (l1.len && l2.len && l3.len && is_email(paths->items->string))\n>> + ret = PATCH_FORMAT_MBOX;\n>\n> 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.\n\nOkay, how about every time we call a strbuf_getline(), we save the\nline to a string_list as well? Like string_list_getline_crlf() below:\n\n/**\n * Like strbuf_getline(), but supports both '\\n' and \"\\r\\n\" as line\n * terminators.\n */\nstatic int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)\n{\n if (strbuf_getwholeline(sb, fp, '\\n'))\n return EOF;\n if (sb->buf[sb->len - 1] == '\\n') {\n strbuf_setlen(sb, sb->len - 1);\n if (sb->len > 0 && sb->buf[sb->len - 1] == '\\r')\n strbuf_setlen(sb, sb->len - 1);\n }\n return 0;\n}\n\n/**\n * Like strbuf_getline_crlf(), but appends the line to a string_list and\n * returns it as a string. Returns NULL on EOF.\n */\nstatic const char *string_list_getline_crlf(struct string_list *list, FILE *fp)\n{\n struct strbuf sb = STRBUF_INIT;\n struct string_list_item *item;\n\n if (strbuf_getline_crlf(&sb, fp))\n return NULL;\n item = string_list_append_nodup(list, strbuf_detach(&sb, NULL));\n return item->string;\n}\n\nSo now, is_email() can have access to previously-read lines, and if it\nneeds some more, it can read more as well:\n\nstatic int is_email(struct string_list *lines, FILE *fp)\n{\n const char *header_regex = \"^[!-9;-~]+:\";\n regex_t regex;\n int ret = 1;\n size_t i;\n\n if (regcomp(®ex, header_regex, REG_NOSUB | REG_EXTENDED))\n die(\"Invalid search pattern: %s\", header_regex);\n\n for (i = 0; i < lines->nr || string_list_getline_crlf(lines, fp); i++) {\n const char *line = lines->items[i].string;\n\n if (!*line)\n break; /* End of header */\n\n /* Ignore indented folded lines */\n if (*line == '\\t' || *line == ' ')\n continue;\n\n /* It's a header if it matches header_regex */\n if (regexec(®ex, line, 0, NULL, 0)) {\n ret = 0;\n goto done;\n }\n }\n\ndone:\n regfree(®ex);\n return ret;\n}\n\nWhich solves the problem of opening the file 2 times. What do you think?\n\nRegards,\nPaul\n"},{"id":"264857","messageId":"CACRoPnSZn=8keW3meUvLY-B_4vQigMTQW_VVvY2rU8ctjcxcaQ@mail.gmail.com","threadId":"39670","inReplyTo":"6560adbc8b1842dd369628980f1264c3@www.dscho.org","subject":"Re: [PATCH/WIP v3 04/31] am: implement patch queue mechanism","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-25T15:16:18Z","receivedAt":"2015-06-25T15:16:18Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Jun 24, 2015 at 10:59 PM, Johannes Schindelin\n wrote:\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index dbc8836..af68c51 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -6,6 +6,158 @@\n>> #include \"cache.h\"\n>> #include \"builtin.h\"\n>> #include \"exec_cmd.h\"\n>> +#include \"parse-options.h\"\n>> +#include \"dir.h\"\n>> +\n>> +struct am_state {\n>> + /* state directory path */\n>> + struct strbuf dir;\n>> +\n>> + /* current and last patch numbers, 1-indexed */\n>> + int cur;\n>> + int last;\n>> +};\n>> +\n>> +/**\n>> + * Initializes am_state with the default values.\n>> + */\n>> +static void am_state_init(struct am_state *state)\n>> +{\n>> + memset(state, 0, sizeof(*state));\n>> +\n>> + strbuf_init(&state->dir, 0);\n>> +}\n>\n> With strbufs, we use the initializer STRBUF_INIT. How about using\n>\n> #define AM_STATE_INIT { STRBUF_INIT, 0, 0 }\n>\n> here?\n\nLater in the patch series am_state_init() will also take into account\nconfig settings when filling up the default values. e.g. see patches\n25/31[1] or 31/31[2]. I think that is reasonable: the purpose of\nam_state_init() is to initialize the am_state struct with the default\nvalues, and the default values can be set by the user through the\nconfig settings.\n\nThis means, though, that we can't use initializers without introducing\nglobal variables.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972\n[2] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972\n\n>> +/**\n>> + * Reads the contents of `file`. The third argument can be used to give a hint\n>> + * about the file size, to avoid reallocs. Returns number of bytes read on\n>> + * success, -1 if the file does not exist. If trim is set, trailing whitespace\n>> + * will be removed from the file contents.\n>> + */\n>> +static int read_state_file(struct strbuf *sb, const char *file,\n>> size_t hint, int trim)\n>> +{\n>> + strbuf_reset(sb);\n>> + if (strbuf_read_file(sb, file, hint) >= 0) {\n>> + if (trim)\n>> + strbuf_trim(sb);\n>> +\n>> + return sb->len;\n>> + }\n>> +\n>> + if (errno == ENOENT)\n>> + return -1;\n>> +\n>> + die_errno(_(\"could not read '%s'\"), file);\n>> +}\n>\n> A couple of thoughts:\n>\n> - 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.\n\nBut too much usage of this temporary strbuf may lead to a situation\nwhere one function calls another, and both functions write to the\nstrbuf and clobber its contents.\n\nBesides, if we are talking about read_state_file(), it takes an\nexternal strbuf, so it gives the caller the freedom to choose which\nstrbuf it uses (e.g. if it is stack allocated or in the am_state\nstruct). I think it's more flexible this way.\n\n> - 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.\n\nDoh! Right, the hint parameter is quite useless, since in am_load() we\nuse the same strbuf anyway. (And strbuf_init() can set a hint as well)\nI've removed it on my side.\n\n> - 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).\n\nMakes sense. After all, this function is called read_STATE_file() ;-)\n\n> - If you agree with these suggestions, the signature would become something like\n>\n> static void read_state_file(struct am_state *state, const char *basename, int trim);\n\nSo for now, my function signature is\n\nstatic void read_state_file(struct strbuf *sb, const struct am_state\n*state, const char *basename, int trim);\n\n>> +/**\n>> + * Remove the am_state directory.\n>> + */\n>> +static void am_destroy(const struct am_state *state)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> +\n>> + strbuf_addstr(&sb, state->dir.buf);\n>> + remove_dir_recursively(&sb, 0);\n>> + strbuf_release(&sb);\n>> +}\n>\n> 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\n>\n> remove_dir_recursively(&state->dir, 0);\n>\n> and do not need another temporary strbuf.\n\nAh right. Although, state->dir is not an strbuf anymore on my side. As\nJunio[3] rightfully noted, state->dir is not modified by the am_*()\nAPI, so it's been changed to a char*. Which means an strbuf is\nrequired to be passed to remove_dir_recursively();\n\n>> +/**\n>> + * Increments the patch pointer, and cleans am_state for the application of the\n>> + * next patch.\n>> + */\n>> +static void am_next(struct am_state *state)\n>> +{\n>> + state->cur++;\n>> + write_file(am_path(state, \"next\"), 1, \"%d\", state->cur);\n>> +}\n>\n> Locking and re-checking the contents of \"next\" before writing the incremented value would probably be a little too paranoid...\n\nYeah, Junio did bring something like that[3]. I'm still thinking about\nit, though I don't think I would like this issue to block the patch\nseries since it's a delicate issue, and git-am.sh does not do anything\nspecial either.\n\nFor now though, I've moved all the write_file()s into a central\nam_save() function, so if we want to do any locking or syncing it\nwould be easy to modify am_save(), and then all the callers will\nbenefit.\n\n[3] http://thread.gmane.org/gmane.comp.version-control.git/271967/focus=271972\n"},{"id":"264936","messageId":"CACRoPnQgy2C__912pVx+Xn2YTgMgfV8ukLtbd8f5uvk82EjnAw@mail.gmail.com","threadId":"39670","inReplyTo":"CACRoPnQAGt_mLbG-EtJ+qTpD2OvNgi-MZfU=NoKCoSmhCOnBEw@mail.gmail.com","subject":"Re: [PATCH/WIP v3 06/31] am: detect mbox patches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-26T07:42:46Z","receivedAt":"2015-06-26T07:42:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, Jun 25, 2015 at 9:40 PM, Paul Tan wrote:\n> On Wed, Jun 24, 2015 at 11:10 PM, Johannes Schindelin\n> wrote:\n>>> + else if (l1.len && l2.len && l3.len && is_email(paths->items->string))\n>>> + ret = PATCH_FORMAT_MBOX;\n>>\n>> 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.\n>\n> Okay, how about every time we call a strbuf_getline(), we save the\n> line to a string_list as well? Like string_list_getline_crlf() below:\n> [...]\n\nHmm, on second thought, I don't think it's worth the code complexity.\nWhile I agree it's desirable to not open the file twice, I don't think\ndetecting the patch format is so IO intensive that it needs to be\noptimized to that extent.\n\nInstead, we should probably just modify is_email() to take a FILE*,\nand then fseek(fp, 0L, SEEK_SET) to the beginning.\n\nI think the logic of is_email() is complex and so it should not be\nfolded into the detect_patch_format() function, especially since we\nmay add detection of other patch formats in the future, and may need\nmore complex heuristics.\n\nRegards,\nPaul\n"},{"id":"264938","messageId":"CACRoPnSyu6or8nkMQ0yS_KcLwumJVNv3pfSFGcSDyk4-P_VZ_A@mail.gmail.com","threadId":"39670","inReplyTo":"72c470f3a6890dfcb66f1439383d2278@www.dscho.org","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-26T08:11:09Z","receivedAt":"2015-06-26T08:11:09Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, Jun 25, 2015 at 12:36 AM, Johannes Schindelin\n wrote:\n> On 2015-06-18 13:25, Paul Tan wrote:\n>> diff --git a/builtin/am.c b/builtin/am.c\n>> index 7b97ea8..d6434e4 100644\n>> --- a/builtin/am.c\n>> +++ b/builtin/am.c\n>> @@ -94,6 +126,105 @@ static int read_state_file(struct strbuf *sb,\n>> const char *file, size_t hint, int\n>> }\n>>\n>> /**\n>> + * Reads a KEY=VALUE shell variable assignment from fp, and returns the VALUE\n>> + * in `value`. VALUE must be a quoted string, and the KEY must match `key`.\n>> + * Returns 0 on success, -1 on failure.\n>> + *\n>> + * This is used by read_author_script() to read the GIT_AUTHOR_* variables from\n>> + * the author-script.\n>> + */\n>> +static int read_shell_var(struct strbuf *value, FILE *fp, const char *key)\n>> +{\n>> + struct strbuf sb = STRBUF_INIT;\n>> + char *str;\n>> +\n>> + if (strbuf_getline(&sb, fp, '\\n'))\n>> + return -1;\n>> +\n>> + if (!skip_prefix(sb.buf, key, (const char **)&str))\n>> + return -1;\n>> +\n>> + if (!skip_prefix(str, \"=\", (const char **)&str))\n>> + return -1;\n>> +\n>> + str = sq_dequote(str);\n>> + if (!str)\n>> + return -1;\n>> +\n>> + strbuf_reset(value);\n>> + strbuf_addstr(value, str);\n>> +\n>> + strbuf_release(&sb);\n>> +\n>> + return 0;\n>> +}\n>\n> How about using `strbuf_remove()` and keeping `str` as `const char *`?\n\nOK, I'll try that out. Looks like this now:\n\nstatic char *read_shell_var(FILE *fp, const char *key)\n{\n struct strbuf sb = STRBUF_INIT;\n const char *str;\n\n if (strbuf_getline(&sb, fp, '\\n'))\n return NULL;\n\n if (!skip_prefix(sb.buf, key, &str))\n return NULL;\n\n if (!skip_prefix(str, \"=\", &str))\n return NULL;\n\n strbuf_remove(&sb, 0, str - sb.buf);\n\n str = sq_dequote(sb.buf);\n if (!str)\n return NULL;\n\n return strbuf_detach(&sb, NULL);\n}\n\n> 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:\n> [...]\n\nHmm, I think we should be very strict about parsing the author-script\nfile though. As explained in read_author_script(), git-am.sh evals the\nauthor-script, which we can't in C. I would much rather we barf at the\nfirst sign that the author-script is not what we expect, rather than\nattempt to parse it as much as possible, but end up with the wrong\nresults as compared to git-am.sh.\n\nBesides, currently git-am.sh will always write the author-script with\nthe order of GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL and GIT_AUTHOR_DATE. If\nthe order is wrong, I would think it means that something is messing\nwith the author-script, and it would be better if we bail out\nimmediately, instead of potentially doing something wrong.\n\n>> +/**\n>> + * Saves state->author_name, state->author_email and state->author_date in\n>> + * `filename` as an \"author script\", which is the format used by git-am.sh.\n>> + */\n>> +static void write_author_script(const struct am_state *state)\n>> +{\n>> + static const char fmt[] = \"GIT_AUTHOR_NAME=%s\\n\"\n>> + \"GIT_AUTHOR_EMAIL=%s\\n\"\n>> + \"GIT_AUTHOR_DATE=%s\\n\";\n>> + struct strbuf author_name = STRBUF_INIT;\n>> + struct strbuf author_email = STRBUF_INIT;\n>> + struct strbuf author_date = STRBUF_INIT;\n>> +\n>> + sq_quote_buf(&author_name, state->author_name.buf);\n>> + sq_quote_buf(&author_email, state->author_email.buf);\n>> + sq_quote_buf(&author_date, state->author_date.buf);\n>\n> 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.\n\nRight, makes sense. I've implemented it on my end.\n\nThanks,\nPaul\n"},{"id":"264994","messageId":"xmqqbng2td62.fsf@gitster.dls.corp.google.com","threadId":"39670","inReplyTo":"CACRoPnSyu6or8nkMQ0yS_KcLwumJVNv3pfSFGcSDyk4-P_VZ_A@mail.gmail.com","subject":"Re: [PATCH/WIP v3 07/31] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-26T20:41:57Z","receivedAt":"2015-06-26T20:41:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> OK, I'll try that out. Looks like this now:\n>\n> static char *read_shell_var(FILE *fp, const char *key)\n> {\n> ...\n> str = sq_dequote(sb.buf);\n> if (!str)\n> return NULL;\n\nYou are unlikely to get !str, but if it does, you leak sb here,\ndon't you?\n\n> return strbuf_detach(&sb, NULL);\n\nThis call is OK; if you passed the &length to detach, you're likely\nto get a wrong result, though ;-)\n\nsq_dequote() is one of the older parts of the API set and its \"we\nknow it cannot do anything but shrink, so we'd do it in-place\"\nattitude, which may be vastly useful in practice, is now showing\nsome impedance mismatch with newer parts of the API like strbuf.\n\n>>> +/**\n>>> + * Saves state->author_name, state->author_email and state->author_date in\n>>> + * `filename` as an \"author script\", which is the format used by git-am.sh.\n>>> + */\n>>> +static void write_author_script(const struct am_state *state)\n>>> +{\n>>> + static const char fmt[] = \"GIT_AUTHOR_NAME=%s\\n\"\n>>> + \"GIT_AUTHOR_EMAIL=%s\\n\"\n>>> + \"GIT_AUTHOR_DATE=%s\\n\";\n>>> + struct strbuf author_name = STRBUF_INIT;\n>>> + struct strbuf author_email = STRBUF_INIT;\n>>> + struct strbuf author_date = STRBUF_INIT;\n>>> +\n>>> + sq_quote_buf(&author_name, state->author_name.buf);\n>>> + sq_quote_buf(&author_email, state->author_email.buf);\n>>> + sq_quote_buf(&author_date, state->author_date.buf);\n>>\n>> The `sq_quote_buf()` function does not call\n>> strbuf_reset()`. Therefore you could just use a single strbuf to\n>> construct the entire three lines and then write that out.\n\nYup. \"quote\" appends to the output, so you could do this:\n\n\tadd(&out, \"GIT_AUTHOR_NAME=\");\n quote(&out, state->author_name);\n add(&out, \"\\\"\\nGIT_AUTHOR_EMAIL=\");\n quote(&out, state->author_email);\n ...\n\nI am not sure if that is easier to read than what you have, though.\n\n>> Again, if\n>> you follow my suggestion to keep a \"scratch pad\" strbuf in am_state,\n>> you could reuse that.\n\nDon't do \"scratch pad\" in a structure that is passed around to\nvarious people. Somebody may be tempted to use the scratch pad\nwhile he has the control, but as soon as he becomes complex enough\nto require calling some helper functions, the ownership rules of the\nscratch pad will become cumbersome to manage and understandable only\nby the person who originally wrote the codepath.\n"},{"id":"265314","messageId":"CACRoPnRqnFESb4tom1FLWxpumLz-BxfBksG=gMePqSzV44=_CQ@mail.gmail.com","threadId":"39670","inReplyTo":"40fbc9dd1a066ba137647ff384aa6395@www.dscho.org","subject":"Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-07-01T09:41:32Z","receivedAt":"2015-07-01T09:41:32Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, Jun 25, 2015 at 2:39 AM, Johannes Schindelin\n wrote:\n> 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!)\n\nI think that since xopen() is a wrapper around open(), it is best to\nfollow its interface (as defined by the POSIX spec) as much as\npossible. It's important to note that the POSIX spec explicitly\ndefines that open() takes a variable number of arguments, and that the\n`mode` argument is only used if O_CREAT is set. This means that if we\ncement xopen() to take 3 arguments, and the third is a mode_t (or an\nint), then we may not be able to keep up with changes in the POSIX\nspec which e.g. in the future may specify that open() takes 4\narguments if certain flags are set.\n\n> 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).\n\nGood point, I agree with this. I'll look into putting the error messages back.\n\nThanks,\nPaul\n"},{"id":"265315","messageId":"CACRoPnR4-N6i_budk+HHoYoBxPQJELp_-Rmsm=JZwfVQGNJgBQ@mail.gmail.com","threadId":"39670","inReplyTo":"CACRoPnRqnFESb4tom1FLWxpumLz-BxfBksG=gMePqSzV44=_CQ@mail.gmail.com","subject":"Re: [PATCH/WIP v3 01/31] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-07-01T09:53:49Z","receivedAt":"2015-07-01T09:53:49Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Wed, Jul 1, 2015 at 5:41 PM, Paul Tan wrote:\n> Good point, I agree with this. I'll look into putting the error messages back.\n\nThis should work I think. It should take into account that O_RDONLY,\nO_WRONLY, O_RDWR is defines as 0, 1, 2 on glibc, while the POSIX spec\nalso defines that O_RDONLY | O_WRONLY == O_RDWR.\n\ndiff --git a/wrapper.c b/wrapper.c\nindex c867ca9..e451463 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -214,7 +214,13 @@ int xopen(const char *path, int oflag, ...)\n return fd;\n if (errno == EINTR)\n continue;\n- die_errno(_(\"could not open '%s'\"), path);\n+\n+ if ((oflag & O_RDWR) == O_RDWR)\n+ die_errno(_(\"could not open '%s' for reading and writing\"), path);\n+ else if ((oflag & O_WRONLY) == O_WRONLY)\n+ die_errno(_(\"could not open '%s' for writing\"), path);\n+ else\n+ die_errno(_(\"could not open '%s' for reading\"), path);\n }\n }\n\n@@ -351,7 +357,13 @@ FILE *xfopen(const char *path, const char *mode)\n return fp;\n if (errno == EINTR)\n continue;\n- die_errno(_(\"could not open '%s'\"), path);\n+\n+ if (*mode && mode[1] == '+')\n+ die_errno(_(\"could not open '%s' for reading and writing\"), path);\n+ else if (*mode == 'w' || *mode == 'a')\n+ die_errno(_(\"could not open '%s' for writing\"), path);\n+ else\n+ die_errno(_(\"could not open '%s' for reading\"), path);\n }\n }\n"}]}