{"thread":{"id":"39438","subject":"[PATCH/WIP 2/8] wrapper: implement xfopen()","startedAt":"2015-05-27T13:33:30Z","lastAt":"2015-06-04T12:05:22Z","messageCount":24,"participants":["Paul Tan","Stefan Beller","Torsten Bögershausen","Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"262230","messageId":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":null,"subject":"[PATCH/WIP 0/8] Make git-am a builtin","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:30Z","receivedAt":"2015-05-27T13:33:30Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"git-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. It is\nbased on my finished prototype of the rewrite[1], and over the next 5 weeks I\nwill be cutting out small patches from the prototype to make it easier to\nreview and refine the patch series.\n\nThis is part of my GSoC project to rewrite git-pull.sh into git-am.sh into C\nbuiltins[2].\n\nA small benchmark that applies 50 patches[3]:\n\n\t#!/bin/sh\n\tgit init &&\n\techo initial >file &&\n\tgit add file &&\n\tgit commit -m initial &&\n\tgit branch before-am &&\n\tfor x in $(seq 50)\n\tdo\n\t    echo $x >>file &&\n\t    git commit -a -m $x\n\tdone &&\n\tgit format-patch --stdout before-am.. >patches &&\n\tgit checkout before-am &&\n\ttime git patches >/dev/null\n\nI ran this benchmark on my *Linux* system.\n\nTimings for git on master:\n\n1.40s, 1.42s, 1.25s, 1.32s, 1.10s. Avg: ~1.30s\n\nTimings for git on master + this patch series applied:\n\n0.24s, 0.22s, 0.22s, 0.19s, 0.25s. Avg: ~0.22s\n\nThis is around a 6x speedup. It's not because this patch series does less than\ngit-am.sh -- similar speedups can be observed with the prototype, which passes\nthe test suite[4].\n\n(Sorry for leaving the other reviews hanging. I was too preoccupied with the\ngit-am rewrite, and was afraid of forgetting important details should I context\nswitch. Now that the prototype is finished I can deal with the other patch\nseries'.)\n\n[1] https://github.com/pyokagan/git/compare/master...pyokagan:pt/ref-builtin-am\n[2] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1\n[3] Since a 56-patch series was posted recently ;-)\n[4] The subset of the test suite that calls git-rebase and git-am.\n\nPaul Tan (8):\n  wrapper: implement xopen()\n  wrapper: implement xfopen()\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\n Makefile          |   2 +-\n builtin.h         |   1 +\n builtin/am.c      | 687 ++++++++++++++++++++++++++++++++++++++++++++++++++++++\n git-compat-util.h |   2 +\n git.c             |   1 +\n wrapper.c         |  37 +++\n 6 files changed, 729 insertions(+), 1 deletion(-)\n create mode 100644 builtin/am.c\n\n-- \n2.1.4\n"},{"id":"262231","messageId":"1432733618-25629-2-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:31Z","receivedAt":"2015-05-27T13:33:31Z","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\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\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 17584ad..9745962 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);\n 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 int xopen(const char *path, int flags, mode_t mode);\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..971665a 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -189,6 +189,24 @@ 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 flags, mode_t mode)\n+{\n+\tint fd;\n+\n+\tassert(path);\n+\tfd = open(path, flags, mode);\n+\tif (fd < 0) {\n+\t\tif ((flags & O_WRONLY) || (flags & O_RDWR))\n+\t\t\tdie_errno(_(\"could not open '%s' for writing\"), path);\n+\t\telse\n+\t\t\tdie_errno(_(\"could not open '%s' for reading\"), path);\n+\t}\n+\treturn fd;\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":"262229","messageId":"1432733618-25629-3-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 2/8] wrapper: implement xfopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:32Z","receivedAt":"2015-05-27T13:33:32Z","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 <pyokagan@gmail.com>\n---\n git-compat-util.h |  1 +\n wrapper.c         | 19 +++++++++++++++++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 9745962..914d450 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -723,6 +723,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 971665a..d5ed780 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -329,6 +329,25 @@ 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+\tFILE *fp;\n+\n+\tassert(path);\n+\tassert(mode);\n+\tfp = fopen(path, mode);\n+\tif (!fp) {\n+\t\tif (*mode == 'w' || *mode == 'a')\n+\t\t\tdie_errno(_(\"could not open '%s' for writing\"), path);\n+\t\telse\n+\t\t\tdie_errno(_(\"could not open '%s' for reading\"), path);\n+\t}\n+\treturn fp;\n+}\n+\n FILE *xfdopen(int fd, const char *mode)\n {\n \tFILE *stream = fdopen(fd, mode);\n-- \n2.1.4\n"},{"id":"262234","messageId":"1432733618-25629-4-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 3/8] am: implement patch queue mechanism","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:33Z","receivedAt":"2015-05-27T13:33:33Z","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_state_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 <pyokagan@gmail.com>\n---\n Makefile     |   2 +-\n builtin.h    |   1 +\n builtin/am.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n git.c        |   1 +\n 4 files changed, 170 insertions(+), 1 deletion(-)\n create mode 100644 builtin/am.c\n\ndiff --git a/Makefile b/Makefile\nindex 323c401..57a7c8c 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -466,7 +466,6 @@ TEST_PROGRAMS_NEED_X =\n # interactive shell sessions without exporting it.\n unexport CDPATH\n \n-SCRIPT_SH += git-am.sh\n SCRIPT_SH += git-bisect.sh\n SCRIPT_SH += git-difftool--helper.sh\n SCRIPT_SH += git-filter-branch.sh\n@@ -812,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 b87df70..d50c9d1 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..6c00009\n--- /dev/null\n+++ b/builtin/am.c\n@@ -0,0 +1,167 @@\n+/*\n+ * Builtin \"git am\"\n+ *\n+ * Based on git-am.sh by Junio C Hamano.\n+ */\n+#include \"cache.h\"\n+#include \"parse-options.h\"\n+#include \"dir.h\"\n+\n+struct am_state {\n+\tstruct strbuf dir;            /* state directory path */\n+\tint cur;                      /* current patch number */\n+\tint last;                     /* last patch number */\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 0 on success, -1 if the file\n+ * does not exist.\n+ */\n+static int read_state_file(struct strbuf *sb, const char *file, size_t hint) {\n+\tstrbuf_reset(sb);\n+\n+\tif (!strbuf_read_file(sb, file, hint))\n+\t\treturn 0;\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_state_load(struct am_state *state)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\n+\tread_state_file(&sb, am_path(state, \"next\"), 8);\n+\tstate->cur = strtol(sb.buf, NULL, 10);\n+\n+\tread_state_file(&sb, am_path(state, \"last\"), 8);\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+\t\tam_next(state);\n+\n+\tam_destroy(state);\n+}\n+\n+struct am_state state;\n+\n+static const char * const am_usage[] = {\n+\tN_(\"git am [options] [(<mbox>|<Maildir>)...]\"),\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+\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_state_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+}\ndiff --git a/git.c b/git.c\nindex 44374b1..42328ed 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, RUN_SETUP | NEED_WORK_TREE },\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":"262232","messageId":"1432733618-25629-5-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:34Z","receivedAt":"2015-05-27T13:33:34Z","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 <pyokagan@gmail.com>\n---\n builtin/am.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 100 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 6c00009..9c7b058 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -6,11 +6,18 @@\n #include \"cache.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 \tstruct strbuf dir;            /* state directory path */\n \tint cur;                      /* current patch number */\n \tint last;                     /* last patch number */\n+\tint prec;                     /* number of digits in patch filename */\n };\n \n /**\n@@ -21,6 +28,7 @@ static void am_state_init(struct am_state *state)\n \tmemset(state, 0, sizeof(*state));\n \n \tstrbuf_init(&state->dir, 0);\n+\tstate->prec = 4;\n }\n \n /**\n@@ -101,13 +109,67 @@ 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. 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+\t\tcase PATCH_FORMAT_MBOX:\n+\t\t\treturn split_patches_mbox(state, paths);\n+\t\tdefault:\n+\t\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@@ -128,13 +190,32 @@ static void am_next(struct am_state *state)\n  */\n static void am_run(struct am_state *state)\n {\n-\twhile (state->cur <= state->last)\n+\twhile (state->cur <= state->last) {\n+\t\t/* patch application not implemented yet */\n+\n \t\tam_next(state);\n+\t}\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+ */\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 struct am_state state;\n+int opt_patch_format;\n \n static const char * const am_usage[] = {\n \tN_(\"git am [options] [(<mbox>|<Maildir>)...]\"),\n@@ -142,6 +223,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@@ -156,8 +239,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)\n \n \tif (am_in_progress(&state))\n \t\tam_state_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":"262233","messageId":"1432733618-25629-6-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 5/8] am: detect mbox patches","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:35Z","receivedAt":"2015-05-27T13:33:35Z","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\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n builtin/am.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 99 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 9c7b058..d589ec5 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -108,6 +108,97 @@ 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 fail;\n+\t\t}\n+\t}\n+\n+fail:\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\tstrbuf_release(&l1);\n+\t\tstrbuf_release(&l2);\n+\t\tstrbuf_release(&l3);\n+\t\treturn PATCH_FORMAT_MBOX;\n+\t}\n+\n+\t/*\n+\t * Otherwise, check the first few 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+\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@@ -162,6 +253,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":"262237","messageId":"1432733618-25629-7-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:36Z","receivedAt":"2015-05-27T13:33:36Z","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\nSigned-off-by: Paul Tan <pyokagan@gmail.com>\n---\n builtin/am.c | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 231 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d589ec5..0b8a42d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -7,6 +7,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@@ -17,6 +34,10 @@ struct am_state {\n \tstruct strbuf dir;            /* state directory path */\n \tint cur;                      /* current patch number */\n \tint last;                     /* last patch number */\n+\tstruct strbuf msg;            /* commit message */\n+\tstruct strbuf author_name;    /* commit author's name */\n+\tstruct strbuf author_email;   /* commit author's email */\n+\tstruct strbuf author_date;    /* commit author's date */\n \tint prec;                     /* number of digits in patch filename */\n };\n \n@@ -28,6 +49,10 @@ static void am_state_init(struct am_state *state)\n \tmemset(state, 0, sizeof(*state));\n \n \tstrbuf_init(&state->dir, 0);\n+\tstrbuf_init(&state->msg, 0);\n+\tstrbuf_init(&state->author_name, 0);\n+\tstrbuf_init(&state->author_email, 0);\n+\tstrbuf_init(&state->author_date, 0);\n \tstate->prec = 4;\n }\n \n@@ -37,6 +62,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->msg);\n+\tstrbuf_release(&state->author_name);\n+\tstrbuf_release(&state->author_email);\n+\tstrbuf_release(&state->author_date);\n }\n \n /**\n@@ -81,6 +110,92 @@ static int read_state_file(struct strbuf *sb, const char *file, size_t hint) {\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+ *         GIT_AUTHOR_NAME='$author_name'\n+ *         GIT_AUTHOR_EMAIL='$author_email'\n+ *\t   GIT_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+\tchar *value;\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(_(\"could not open '%s' for reading\"), filename);\n+\t}\n+\n+\tif (strbuf_getline(&sb, fp, '\\n'))\n+\t\treturn -1;\n+\tif (!skip_prefix(sb.buf, \"GIT_AUTHOR_NAME=\", (const char**) &value))\n+\t\treturn -1;\n+\tvalue = sq_dequote(value);\n+\tif (!value)\n+\t\treturn -1;\n+\tstrbuf_addstr(&state->author_name, value);\n+\n+\tif (strbuf_getline(&sb, fp, '\\n'))\n+\t\treturn -1;\n+\tif (!skip_prefix(sb.buf, \"GIT_AUTHOR_EMAIL=\", (const char**) &value))\n+\t\treturn -1;\n+\tvalue = sq_dequote(value);\n+\tif (!value)\n+\t\treturn -1;\n+\tstrbuf_addstr(&state->author_email, value);\n+\n+\tif (strbuf_getline(&sb, fp, '\\n'))\n+\t\treturn -1;\n+\tif (!skip_prefix(sb.buf, \"GIT_AUTHOR_DATE=\", (const char**) &value))\n+\t\treturn -1;\n+\tvalue = sq_dequote(value);\n+\tif (!value)\n+\t\treturn -1;\n+\tstrbuf_addstr(&state->author_date, value);\n+\n+\tif (fgetc(fp) != EOF)\n+\t\treturn -1;\n+\n+\tfclose(fp);\n+\tstrbuf_release(&sb);\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_state_load(struct am_state *state)\n@@ -93,6 +208,14 @@ static void am_state_load(struct am_state *state)\n \tread_state_file(&sb, am_path(state, \"last\"), 8);\n \tstate->last = strtol(sb.buf, NULL, 10);\n \n+\tread_state_file(&state->msg, am_path(state, \"final-commit\"), 0);\n+\n+\tstrbuf_reset(&state->author_name);\n+\tstrbuf_reset(&state->author_email);\n+\tstrbuf_reset(&state->author_date);\n+\tif (read_author_script(state) < 0)\n+\t\tdie(_(\"could not parse author script\"));\n+\n \tstrbuf_release(&sb);\n }\n \n@@ -282,6 +405,102 @@ 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->msg);\n+\tunlink(am_path(state, \"final-commit\"));\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+\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 fmt = STRBUF_INIT;\n+\tstatic struct strbuf sb = STRBUF_INIT;\n+\n+\tstrbuf_reset(&fmt);\n+\tstrbuf_addf(&fmt, \"%%0%dd\", state->prec);\n+\n+\tstrbuf_reset(&sb);\n+\tstrbuf_addf(&sb, fmt.buf, 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\tif (state->author_name.len)\n+\t\t\t\tstrbuf_addch(&state->author_name, '\\n');\n+\t\t\tstrbuf_addstr(&state->author_name, x);\n+\t\t} else if (skip_prefix(sb.buf, \"Email: \", &x)) {\n+\t\t\tif (state->author_email.len)\n+\t\t\t\tstrbuf_addch(&state->author_email, '\\n');\n+\t\t\tstrbuf_addstr(&state->author_email, x);\n+\t\t} else if (skip_prefix(sb.buf, \"Date: \", &x)) {\n+\t\t\tif (state->author_date.len)\n+\t\t\t\tstrbuf_addch(&state->author_date, '\\n');\n+\t\t\tstrbuf_addstr(&state->author_date, x);\n+\t\t}\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@@ -290,8 +509,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_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n+\t\twrite_author_script(state);\n+\n \t\t/* patch application not implemented yet */\n \n+next:\n \t\tam_next(state);\n \t}\n \n-- \n2.1.4\n"},{"id":"262235","messageId":"1432733618-25629-8-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 7/8] am: apply patch with git-apply","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:37Z","receivedAt":"2015-05-27T13:33:37Z","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 <pyokagan@gmail.com>\n---\n builtin/am.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 49 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b8a42d..7126df3 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -25,6 +25,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@@ -503,6 +515,29 @@ 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+\targv_array_push(&cp.args, \"--index\");\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@@ -520,7 +555,20 @@ static void am_run(struct am_state *state)\n \t\twrite_file(am_path(state, \"final-commit\"), 1, \"%s\", state->msg.buf);\n \t\twrite_author_script(state);\n \n-\t\t/* 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 next:\n \t\tam_next(state);\n-- \n2.1.4\n"},{"id":"262236","messageId":"1432733618-25629-9-git-send-email-pyokagan@gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-1-git-send-email-pyokagan@gmail.com","subject":"[PATCH/WIP 8/8] am: commit applied patch","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-05-27T13:33:38Z","receivedAt":"2015-05-27T13:33:38Z","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 <pyokagan@gmail.com>\n---\n builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 7126df3..3174327 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -8,6 +8,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@@ -539,6 +542,48 @@ 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[20], parent[20], commit[20];\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@@ -570,6 +615,7 @@ static void am_run(struct am_state *state)\n \t\t\texit(128);\n \t\t}\n \n+\t\tdo_commit(state);\n next:\n \t\tam_next(state);\n \t}\n-- \n2.1.4\n"},{"id":"262244","messageId":"CAGZ79kYqfgj6A_2GV3xY-j16KM0GAGEmnxSHjZXrC=WJ7=HRaQ@mail.gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-27T17:52:16Z","receivedAt":"2015-05-27T17:52:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, May 27, 2015 at 6:33 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> A common usage pattern of open() is to check if it was successful, and\n> die() if it was not:\n>\n>         int fd = open(path, O_WRONLY | O_CREAT, 0777);\n>         if (fd < 0)\n>                 die_errno(_(\"Could not open '%s' for writing.\"), path);\n>\n> Implement a wrapper function xopen() that does the above so that we can\n> save a few lines of code, and make the die() messages consistent.\n\nAs a mental todo note for whomever wants to pick up some work: This patch\nseries indicates to only touch git-am. The first 2 patches introduce\nnew x-wrappers,\nso maybe we'd need to grep through the whole code base and convert the all\nthese file opening code to the new wrapper.\n\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n>  git-compat-util.h |  1 +\n>  wrapper.c         | 18 ++++++++++++++++++\n>  2 files changed, 19 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 17584ad..9745962 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);\n>  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 int xopen(const char *path, int flags, mode_t mode);\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);\n> diff --git a/wrapper.c b/wrapper.c\n> index c1a663f..971665a 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -189,6 +189,24 @@ 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 flags, mode_t mode)\n> +{\n> +       int fd;\n> +\n> +       assert(path);\n> +       fd = open(path, flags, mode);\n> +       if (fd < 0) {\n> +               if ((flags & O_WRONLY) || (flags & O_RDWR))\n> +                       die_errno(_(\"could not open '%s' for writing\"), path);\n> +               else\n> +                       die_errno(_(\"could not open '%s' for reading\"), path);\n> +       }\n> +       return fd;\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> --\n> 2.1.4\n>\n"},{"id":"262246","messageId":"55661513.8000306@web.de","threadId":"39438","inReplyTo":"1432733618-25629-2-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-05-27T19:03:47Z","receivedAt":"2015-05-27T19:03:47Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-05-27 15.33, Paul Tan wrote:\n> A common usage pattern of open() is to check if it was successful, and\n> die() 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> \n> Implement a wrapper function xopen() that does the above so that we can\n> save a few lines of code, and make the die() messages consistent.\n> \n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n>  git-compat-util.h |  1 +\n>  wrapper.c         | 18 ++++++++++++++++++\n>  2 files changed, 19 insertions(+)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index 17584ad..9745962 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);\n>  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 int xopen(const char *path, int flags, mode_t mode);\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);\n> diff --git a/wrapper.c b/wrapper.c\n> index c1a663f..971665a 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -189,6 +189,24 @@ void *xcalloc(size_t nmemb, size_t size)\n>  # endif\n>  #endif\n>  \nThe original open can take 2 or 3 parameters, how about this:\nint xopen(const char *path, int oflag, ... )\n{\n        va_list params;\n        int mode;\n        int fd;\n\n        va_start(params, oflag);\n        mode = va_arg(params, int);\n        va_end(params);\n\n        fd = open(path, oflag, mode);\n\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 flags, mode_t mode)\n> +{\n> +\tint fd;\n> +\n> +\tassert(path);\n> +\tfd = open(path, flags, mode);\n> +\tif (fd < 0) {\n> +\t\tif ((flags & O_WRONLY) || (flags & O_RDWR))\n> +\t\t\tdie_errno(_(\"could not open '%s' for writing\"), path);\nThis is only partly true:\nit could be either \"writing\" or \"read write\".\nI don't know if the info \"for reading\" or \"for writing\" is needed/helpful at all,\nor if a simple \"could not open\" would be enough.\n\n\nAnother thing:\nshould we handle EINTR ?\n(Somewhere in the back of my head I remember that some OS\n returned EINTR when handling some foreign file system\n Mac OS / NTFS ?)\n"},{"id":"262269","messageId":"xmqqy4k9kb6s.fsf@gitster.dls.corp.google.com","threadId":"39438","inReplyTo":"1432733618-25629-4-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 3/8] am: implement patch queue mechanism","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T20:38:51Z","receivedAt":"2015-05-27T20:38:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n>  Makefile     |   2 +-\n>  builtin.h    |   1 +\n>  builtin/am.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n>  git.c        |   1 +\n>  4 files changed, 170 insertions(+), 1 deletion(-)\n>  create mode 100644 builtin/am.c\n>\n> diff --git a/Makefile b/Makefile\n> index 323c401..57a7c8c 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -466,7 +466,6 @@ TEST_PROGRAMS_NEED_X =\n>  # interactive shell sessions without exporting it.\n>  unexport CDPATH\n>  \n> -SCRIPT_SH += git-am.sh\n\nIf you are building this \"new am\" incrementally (and for something\ncomplex like \"am\", that's the only sensible way), perhaps it is a\ngood idea to do the \"competing implementation\" trick I suggested\nearlier when we were discussing your \"new pull\" patches, to allow\nyou to keep both versions and switch between them at runtime.  That\nway, you can run tests, both existing ones and new ones you add, on\nboth versions to make sure they behave the same way.\n"},{"id":"262270","messageId":"xmqqtwuxkaxh.fsf@gitster.dls.corp.google.com","threadId":"39438","inReplyTo":"1432733618-25629-7-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T20:44:26Z","receivedAt":"2015-05-27T20:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> @@ -17,6 +34,10 @@ struct am_state {\n>  \tstruct strbuf dir;            /* state directory path */\n>  \tint cur;                      /* current patch number */\n>  \tint last;                     /* last patch number */\n> +\tstruct strbuf msg;            /* commit message */\n> +\tstruct strbuf author_name;    /* commit author's name */\n> +\tstruct strbuf author_email;   /* commit author's email */\n> +\tstruct strbuf author_date;    /* commit author's date */\n>  \tint prec;                     /* number of digits in patch filename */\n>  };\n\nI always get suspicious when structure fields are overly commented,\nwondering if it is a sign of naming fields poorly.  All of the above\nfields look quite self-explanatory and I am not sure if it is worth\nhaving these comments, spending efforts to type SP many times to\nalign them and all.\n\nBy the way, the overall structure of the series look sensible.\n\n> +static int read_author_script(struct am_state *state)\n> +{\n> +\tchar *value;\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(_(\"could not open '%s' for reading\"), filename);\n\nHmph, do we want to report with die_errno()?\n\n> +\t}\n> +\n> +\tif (strbuf_getline(&sb, fp, '\\n'))\n> +\t\treturn -1;\n> +\tif (!skip_prefix(sb.buf, \"GIT_AUTHOR_NAME=\", (const char**) &value))\n\nThis cast is unfortunate; can't \"value\" be of \"const char *\" type?\n"},{"id":"262284","messageId":"20150527215307.GC23259@peff.net","threadId":"39438","inReplyTo":"55661513.8000306@web.de","subject":"Re: [PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T21:53:07Z","receivedAt":"2015-05-27T21:53:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 09:03:47PM +0200, Torsten Bögershausen wrote:\n\n> The original open can take 2 or 3 parameters, how about this:\n> int xopen(const char *path, int oflag, ... )\n> {\n>         va_list params;\n>         int mode;\n>         int fd;\n> \n>         va_start(params, oflag);\n>         mode = va_arg(params, int);\n>         va_end(params);\n> \n>         fd = open(path, oflag, mode);\n\nDon't you need a conditional on pulling the mode arg off the stack\n(i.e., if O_CREAT is in the flags)?\n\n-Peff\n"},{"id":"262286","messageId":"20150527215507.GD23259@peff.net","threadId":"39438","inReplyTo":"1432733618-25629-3-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 2/8] wrapper: implement xfopen()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T21:55:07Z","receivedAt":"2015-05-27T21:55:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 09:33:32PM +0800, Paul Tan wrote:\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> +\tFILE *fp;\n> +\n> +\tassert(path);\n> +\tassert(mode);\n> +\tfp = fopen(path, mode);\n> +\tif (!fp) {\n> +\t\tif (*mode == 'w' || *mode == 'a')\n> +\t\t\tdie_errno(_(\"could not open '%s' for writing\"), path);\n\nThis misses \"r+\". I don't think we use that in our code currently, but\nif we're going to introduce a wrapper like this, I think it makes sense\nto cover all cases.\n\n-Peff\n"},{"id":"262290","messageId":"xmqqh9qxk6tb.fsf@gitster.dls.corp.google.com","threadId":"39438","inReplyTo":"1432733618-25629-7-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T22:13:20Z","receivedAt":"2015-05-27T22:13:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan <pyokagan@gmail.com> writes:\n\n> +static const char *msgnum(const struct am_state *state)\n> +{\n> +\tstatic struct strbuf fmt = STRBUF_INIT;\n> +\tstatic struct strbuf sb = STRBUF_INIT;\n> +\n> +\tstrbuf_reset(&fmt);\n> +\tstrbuf_addf(&fmt, \"%%0%dd\", state->prec);\n> +\n> +\tstrbuf_reset(&sb);\n> +\tstrbuf_addf(&sb, fmt.buf, state->cur);\n\nHmph, wouldn't (\"%*d\", state->prec, state->cur) work or am I missing\nsomething?\n"},{"id":"262291","messageId":"20150527221356.GE23259@peff.net","threadId":"39438","inReplyTo":"xmqqtwuxkaxh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-05-27T22:13:56Z","receivedAt":"2015-05-27T22:13:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:\n\n> Paul Tan <pyokagan@gmail.com> writes:\n> \n> > @@ -17,6 +34,10 @@ struct am_state {\n> >  \tstruct strbuf dir;            /* state directory path */\n> >  \tint cur;                      /* current patch number */\n> >  \tint last;                     /* last patch number */\n> > +\tstruct strbuf msg;            /* commit message */\n> > +\tstruct strbuf author_name;    /* commit author's name */\n> > +\tstruct strbuf author_email;   /* commit author's email */\n> > +\tstruct strbuf author_date;    /* commit author's date */\n> >  \tint prec;                     /* number of digits in patch filename */\n> >  };\n> \n> I always get suspicious when structure fields are overly commented,\n> wondering if it is a sign of naming fields poorly.  All of the above\n> fields look quite self-explanatory and I am not sure if it is worth\n> having these comments, spending efforts to type SP many times to\n> align them and all.\n\nJust my 2 cents, but I think that grouping and overhead comments can\noften make things more obvious. For example:\n\n  struct am_state {\n\t/* state directory path */\n\tstruct strbuf dir;\n\n\t/*\n\t * current and last patch numbers; are these 1-indexed\n\t * or 0-indexed?\n\t */\n\tint cur;\n\tint last;\n\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\nNote that by grouping \"cur\" and \"last\", we can discuss them as a group,\nand the overhead comment gives us room to mention their shared\nproperties (my indexing question is a real question, btw, whose answer I\nthink would be useful to mention in a comment).\n\nLikewise, by grouping the \"msg\" strbuf with the author information, it\nbecomes much more clear that they are all about the commit metadata\n(though I would not be opposed to a comment above the whole block,\neither).\n\n-Peff\n"},{"id":"262386","messageId":"CAPig+cSK_yEabXJFF4UvKkbcisgpZGV0aX7jExcnK=Q-Ozm-eg@mail.gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-5-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-28T23:05:43Z","receivedAt":"2015-05-28T23:05:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> git-am.sh supports mbox, stgit and mercurial patches. Re-implement\n> support for splitting out mbox/maildirs using git-mailsplit, while also\n> implementing the framework required to support other patch formats in\n> the future.\n>\n> Re-implement support for the --patch-format option (since a5a6755\n> (git-am foreign patch support: introduce patch_format, 2009-05-27)) to\n> allow the user to choose between the different patch formats.\n>\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)\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> +       int *opt_value = opt->value;\n> +\n> +       if (!strcmp(arg, \"mbox\"))\n> +               *opt_value = PATCH_FORMAT_MBOX;\n> +       else\n> +               return -1;\n> +       return 0;\n> +}\n> +\n>  struct am_state state;\n> +int opt_patch_format;\n\nShould these two variables be static?\n\n>  static const char * const am_usage[] = {\n>         N_(\"git am [options] [(<mbox>|<Maildir>)...]\"),\n"},{"id":"262514","messageId":"CAPig+cQc6mgfXB3nz01xu8QArPv7zNOVXwb+WskBn1z2RyJCYQ@mail.gmail.com","threadId":"39438","inReplyTo":"1432733618-25629-6-git-send-email-pyokagan@gmail.com","subject":"Re: [PATCH/WIP 5/8] am: detect mbox patches","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-05-31T17:16:28Z","receivedAt":"2015-05-31T17:16:28Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:\n> Since 15ced75 (git-am foreign patch support: autodetect some patch\n> formats, 2009-05-27), git-am.sh is able to autodetect mbox, stgit and\n> mercurial patches through heuristics.\n>\n> Re-implement support for autodetecting mbox/maildir files.\n\nA few minor comments below...\n\n> Signed-off-by: Paul Tan <pyokagan@gmail.com>\n> ---\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 9c7b058..d589ec5 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -108,6 +108,97 @@ 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> +               for (x = sb.buf; *x; x++) {\n> +                       if (('!' <= *x && *x <= '9') || (';' <= *x && *x <= '~'))\n> +                               continue;\n> +                       if (*x == ':' && x != sb.buf)\n> +                               break;\n> +                       ret = 0;\n> +                       goto fail;\n> +               }\n> +       }\n> +\n> +fail:\n\nIt's a bit odd seeing a label named \"fail\" through which both the\nsuccessful and failing cases flow. Upon seeing it, I had expected to\nfind a \"return 1\" somewhere above for the successful case. Perhaps a\nname such as \"done\" would be clearer.\n\n> +       fclose(fp);\n> +       strbuf_release(&sb);\n> +       return 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> +       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> +               strbuf_release(&l1);\n> +               strbuf_release(&l2);\n> +               strbuf_release(&l3);\n> +               return PATCH_FORMAT_MBOX;\n\nA couple observations:\n\nSome people will argue that these strbuf_release() calls are pointless\nsince the strbufs haven't been used yet, thus should be dropped. (I\ndon't care strongly.)\n\nPerhaps this case could be a bit less noisy if coded like this:\n\n    if (... || ...) {\n        ret = PATCH_FORMAT_MBOX;\n        goto done;\n    }\n\nwith a corresponding \"done\" label added before the strbuf_release()\ncalls near the bottom of the function.\n\n> +       }\n> +\n> +       /*\n> +        * Otherwise, check the first few 3 lines of the first patch, starting\n\nEither: s/few 3/few/ or s/few 3/3/\n\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> +       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> +       else if (l1.len && l2.len && l3.len && is_email(paths->items->string))\n> +               ret = PATCH_FORMAT_MBOX;\n> +\n> +       strbuf_release(&l1);\n> +       strbuf_release(&l2);\n> +       strbuf_release(&l3);\n> +       return 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> @@ -162,6 +253,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>                 struct string_list *paths)\n>  {\n> +       if (!patch_format)\n> +               patch_format = detect_patch_format(paths);\n> +\n> +       if (!patch_format) {\n> +               fprintf_ln(stderr, _(\"Patch format detection failed.\"));\n> +               exit(128);\n> +       }\n> +\n>         if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)\n>                 die_errno(_(\"failed to create directory '%s'\"), state->dir.buf);\n>\n> --\n> 2.1.4\n"},{"id":"262729","messageId":"CACRoPnRei3VPDSrD1ppkr_YoeiC-V5qp=KXkaO-OyMz9YkX8-Q@mail.gmail.com","threadId":"39438","inReplyTo":"CAPig+cSK_yEabXJFF4UvKkbcisgpZGV0aX7jExcnK=Q-Ozm-eg@mail.gmail.com","subject":"Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-02T14:27:41Z","receivedAt":"2015-06-02T14:27:41Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Fri, May 29, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:\n>> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)\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>> +       int *opt_value = opt->value;\n>> +\n>> +       if (!strcmp(arg, \"mbox\"))\n>> +               *opt_value = PATCH_FORMAT_MBOX;\n>> +       else\n>> +               return -1;\n>> +       return 0;\n>> +}\n>> +\n>>  struct am_state state;\n>> +int opt_patch_format;\n>\n> Should these two variables be static?\n\nWhoops. Yes, they should.\n\nThanks,\nPaul\n"},{"id":"262842","messageId":"CACRoPnQA9SyNzudEdw60UWsMFNQt90AGoi=ge3Oh-LQB02=cFg@mail.gmail.com","threadId":"39438","inReplyTo":"xmqqtwuxkaxh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-03T07:56:46Z","receivedAt":"2015-06-03T07:56:46Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> @@ -17,6 +34,10 @@ struct am_state {\n>>       struct strbuf dir;            /* state directory path */\n>>       int cur;                      /* current patch number */\n>>       int last;                     /* last patch number */\n>> +     struct strbuf msg;            /* commit message */\n>> +     struct strbuf author_name;    /* commit author's name */\n>> +     struct strbuf author_email;   /* commit author's email */\n>> +     struct strbuf author_date;    /* commit author's date */\n>>       int prec;                     /* number of digits in patch filename */\n>>  };\n>\n> I always get suspicious when structure fields are overly commented,\n> wondering if it is a sign of naming fields poorly.  All of the above\n> fields look quite self-explanatory and I am not sure if it is worth\n> having these comments, spending efforts to type SP many times to\n> align them and all.\n\nOkay, I'll take Jeff's suggestion to organize them into blocks.\n\n>> +static int read_author_script(struct am_state *state)\n>> +{\n>> +     char *value;\n>> +     struct strbuf sb = STRBUF_INIT;\n>> +     const char *filename = am_path(state, \"author-script\");\n>> +     FILE *fp = fopen(filename, \"r\");\n>> +     if (!fp) {\n>> +             if (errno == ENOENT)\n>> +                     return 0;\n>> +             die(_(\"could not open '%s' for reading\"), filename);\n>\n> Hmph, do we want to report with die_errno()?\n\nYes, we do.\n\n>> +     }\n>> +\n>> +     if (strbuf_getline(&sb, fp, '\\n'))\n>> +             return -1;\n>> +     if (!skip_prefix(sb.buf, \"GIT_AUTHOR_NAME=\", (const char**) &value))\n>\n> This cast is unfortunate; can't \"value\" be of \"const char *\" type?\n\nWe can't, because sq_dequote() modifies the string directly. I would\nthink casting from non-const to const is safer than the other way\naround.\n\nThanks,\nPaul\n"},{"id":"262843","messageId":"CACRoPnQVfUke3eGbTE-sQbV8drH7tDcE+YOpSP0xGtgk9z_FTA@mail.gmail.com","threadId":"39438","inReplyTo":"xmqqh9qxk6tb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-03T07:57:34Z","receivedAt":"2015-06-03T07:57:34Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, May 28, 2015 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Paul Tan <pyokagan@gmail.com> writes:\n>\n>> +static const char *msgnum(const struct am_state *state)\n>> +{\n>> +     static struct strbuf fmt = STRBUF_INIT;\n>> +     static struct strbuf sb = STRBUF_INIT;\n>> +\n>> +     strbuf_reset(&fmt);\n>> +     strbuf_addf(&fmt, \"%%0%dd\", state->prec);\n>> +\n>> +     strbuf_reset(&sb);\n>> +     strbuf_addf(&sb, fmt.buf, state->cur);\n>\n> Hmph, wouldn't (\"%*d\", state->prec, state->cur) work or am I missing\n> something?\n\nYeah, I think it would. I justs didn't know about the existence of '*'.\n\nThanks,\nPaul\n"},{"id":"262845","messageId":"CACRoPnSk7vgFwTazEz3yytsdBC0CT+Yjs3JZ_oX_kuR2yyZhRA@mail.gmail.com","threadId":"39438","inReplyTo":"20150527215307.GC23259@peff.net","subject":"Re: [PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-03T08:16:57Z","receivedAt":"2015-06-03T08:16:57Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, May 28, 2015 at 5:53 AM, Jeff King <peff@peff.net> wrote:\n> On Wed, May 27, 2015 at 09:03:47PM +0200, Torsten Bögershausen wrote:\n>\n>> The original open can take 2 or 3 parameters, how about this:\n>> int xopen(const char *path, int oflag, ... )\n>> {\n>>         va_list params;\n>>         int mode;\n>>         int fd;\n>>\n>>         va_start(params, oflag);\n>>         mode = va_arg(params, int);\n>>         va_end(params);\n>>\n>>         fd = open(path, oflag, mode);\n>\n> Don't you need a conditional on pulling the mode arg off the stack\n> (i.e., if O_CREAT is in the flags)?\n\nYeah, we do, as va_arg()'s behavior is undefined if we do not have the\nnext argument.\n\nThe POSIX spec[1] only mentions O_CREAT as requiring the extra\nargument, so I guess we'll only need to check for that.\n\n[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html\n\nThanks,\nPaul\n"},{"id":"262949","messageId":"CACRoPnRn5ynmt1UXct6BzXpr_aioAtLpXfwjZKMBUs4-sGP63A@mail.gmail.com","threadId":"39438","inReplyTo":"55661513.8000306@web.de","subject":"Re: [PATCH/WIP 1/8] wrapper: implement xopen()","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-06-04T12:05:22Z","receivedAt":"2015-06-04T12:05:22Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Thu, May 28, 2015 at 3:03 AM, Torsten Bögershausen <tboegi@web.de> wrote:\n> On 2015-05-27 15.33, Paul Tan wrote:\n>> +/**\n>> + * xopen() is the same as open(), but it die()s if the open() fails.\n>> + */\n>> +int xopen(const char *path, int flags, mode_t mode)\n>> +{\n>> +     int fd;\n>> +\n>> +     assert(path);\n>> +     fd = open(path, flags, mode);\n>> +     if (fd < 0) {\n>> +             if ((flags & O_WRONLY) || (flags & O_RDWR))\n>> +                     die_errno(_(\"could not open '%s' for writing\"), path);\n> This is only partly true:\n> it could be either \"writing\" or \"read write\".\n\nAh right, I see now that the POSIX spec allows for, and encourages\nO_RDONLY | O_WRONLY == O_RDWR.\n\n> I don't know if the info \"for reading\" or \"for writing\" is needed/helpful at all,\n> or if a simple \"could not open\" would be enough.\n\nYeah, I agree that it may not be helpful, but I noticed that most\nerror messages in git are of the form \"unable to open X for writing\",\n\"unable to open X for reading\", \"could not create X\" etc. Or rather I\nthought I noticed, but it now seems to me that there are quite a lot\nof uses of \"could not open X\" as well.\n\nI guess I will remove the distinction.\n\nThanks,\nPaul\n"}]}