[PATCH/WIP 0/8] Make git-am a builtin

[PATCH/WIP 0/8] Make git-am a builtin

[Paul Tan]

git-am is a commonly used command for applying a series of patches from a
mailbox to the current branch. Currently, it is implemented by the shell script
git-am.sh. However, compared to C, shell scripts have certain deficiencies:
they need to spawn a lot of processes, introduce a lot of dependencies and
cannot take advantage of git's internal caches.

This WIP patch series rewrites git-am.sh into optimized C builtin/am.c. It is
based on my finished prototype of the rewrite[1], and over the next 5 weeks I
will be cutting out small patches from the prototype to make it easier to
review and refine the patch series.

This is part of my GSoC project to rewrite git-pull.sh into git-am.sh into C
builtins[2].

A small benchmark that applies 50 patches[3]:

	#!/bin/sh
	git init &&
	echo initial >file &&
	git add file &&
	git commit -m initial &&
	git branch before-am &&
	for x in $(seq 50)
	do
	    echo $x >>file &&
	    git commit -a -m $x
	done &&
	git format-patch --stdout before-am.. >patches &&
	git checkout before-am &&
	time git patches >/dev/null

I ran this benchmark on my *Linux* system.

Timings for git on master:

1.40s, 1.42s, 1.25s, 1.32s, 1.10s. Avg: ~1.30s

Timings for git on master + this patch series applied:

0.24s, 0.22s, 0.22s, 0.19s, 0.25s. Avg: ~0.22s

This is around a 6x speedup. It's not because this patch series does less than
git-am.sh -- similar speedups can be observed with the prototype, which passes
the test suite[4].

(Sorry for leaving the other reviews hanging. I was too preoccupied with the
git-am rewrite, and was afraid of forgetting important details should I context
switch. Now that the prototype is finished I can deal with the other patch
series'.)

[1] https://github.com/pyokagan/git/compare/master...pyokagan:pt/ref-builtin-am
[2] https://gist.github.com/pyokagan/1b7b0d1f4dab6ba3cef1
[3] Since a 56-patch series was posted recently ;-)
[4] The subset of the test suite that calls git-rebase and git-am.

Paul Tan (8):
  wrapper: implement xopen()
  wrapper: implement xfopen()
  am: implement patch queue mechanism
  am: split out mbox/maildir patches with git-mailsplit
  am: detect mbox patches
  am: extract patch, message and authorship with git-mailinfo
  am: apply patch with git-apply
  am: commit applied patch

 Makefile          |   2 +-
 builtin.h         |   1 +
 builtin/am.c      | 687 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git-compat-util.h |   2 +
 git.c             |   1 +
 wrapper.c         |  37 +++
 6 files changed, 729 insertions(+), 1 deletion(-)
 create mode 100644 builtin/am.c

-- 
2.1.4

[PATCH/WIP 1/8] wrapper: implement xopen()

[Paul Tan]

A common usage pattern of open() is to check if it was successful, and
die() if it was not:

	int fd = open(path, O_WRONLY | O_CREAT, 0777);
	if (fd < 0)
		die_errno(_("Could not open '%s' for writing."), path);

Implement a wrapper function xopen() that does the above so that we can
save a few lines of code, and make the die() messages consistent.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-compat-util.h |  1 +
 wrapper.c         | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 17584ad..9745962 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);
 extern void *xrealloc(void *ptr, size_t size);
 extern void *xcalloc(size_t nmemb, size_t size);
 extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
+extern int xopen(const char *path, int flags, mode_t mode);
 extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
diff --git a/wrapper.c b/wrapper.c
index c1a663f..971665a 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -189,6 +189,24 @@ void *xcalloc(size_t nmemb, size_t size)
 # endif
 #endif
 
+/**
+ * xopen() is the same as open(), but it die()s if the open() fails.
+ */
+int xopen(const char *path, int flags, mode_t mode)
+{
+	int fd;
+
+	assert(path);
+	fd = open(path, flags, mode);
+	if (fd < 0) {
+		if ((flags & O_WRONLY) || (flags & O_RDWR))
+			die_errno(_("could not open '%s' for writing"), path);
+		else
+			die_errno(_("could not open '%s' for reading"), path);
+	}
+	return fd;
+}
+
 /*
  * xread() is the same a read(), but it automatically restarts read()
  * operations with a recoverable error (EAGAIN and EINTR). xread()
-- 
2.1.4

[PATCH/WIP 2/8] wrapper: implement xfopen()

[Paul Tan]

A common usage pattern of fopen() is to check if it succeeded, and die()
if it failed:

	FILE *fp = fopen(path, "w");
	if (!fp)
		die_errno(_("could not open '%s' for writing"), path);

Implement a wrapper function xfopen() for the above, so that we can save
a few lines of code and make the die() messages consistent.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 git-compat-util.h |  1 +
 wrapper.c         | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index 9745962..914d450 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -723,6 +723,7 @@ extern ssize_t xread(int fd, void *buf, size_t len);
 extern ssize_t xwrite(int fd, const void *buf, size_t len);
 extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
 extern int xdup(int fd);
+extern FILE *xfopen(const char *path, const char *mode);
 extern FILE *xfdopen(int fd, const char *mode);
 extern int xmkstemp(char *template);
 extern int xmkstemp_mode(char *template, int mode);
diff --git a/wrapper.c b/wrapper.c
index 971665a..d5ed780 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -329,6 +329,25 @@ int xdup(int fd)
 	return ret;
 }
 
+/**
+ * xfopen() is the same as fopen(), but it die()s if the fopen() fails.
+ */
+FILE *xfopen(const char *path, const char *mode)
+{
+	FILE *fp;
+
+	assert(path);
+	assert(mode);
+	fp = fopen(path, mode);
+	if (!fp) {
+		if (*mode == 'w' || *mode == 'a')
+			die_errno(_("could not open '%s' for writing"), path);
+		else
+			die_errno(_("could not open '%s' for reading"), path);
+	}
+	return fp;
+}
+
 FILE *xfdopen(int fd, const char *mode)
 {
 	FILE *stream = fdopen(fd, mode);
-- 
2.1.4

[PATCH/WIP 3/8] am: implement patch queue mechanism

[Paul Tan]

git-am applies a series of patches. If the process terminates
abnormally, we want to be able to resume applying the series of patches.
This requires the session state to be saved in a persistent location.

Implement the mechanism of a "patch queue", represented by 2 integers --
the index of the current patch we are applying and the index of the last
patch, as well as its lifecycle through the following functions:

* am_setup(), which will set up the state directory
  $GIT_DIR/rebase-apply. As such, even if the process exits abnormally,
  the last-known state will still persist.

* am_state_load(), which is called if there is an am session in
  progress, to load the last known state from the state directory so we
  can resume applying patches.

* am_run(), which will do the actual patch application. After applying a
  patch, it calls am_next() to increment the current patch index. The
  logic for applying and committing a patch is not implemented yet.

* am_destroy(), which is finally called when we successfully applied all
  the patches in the queue, to clean up by removing the state directory
  and its contents.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile     |   2 +-
 builtin.h    |   1 +
 builtin/am.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 git.c        |   1 +
 4 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 builtin/am.c

diff --git a/Makefile b/Makefile
index 323c401..57a7c8c 100644
--- a/Makefile
+++ b/Makefile
@@ -466,7 +466,6 @@ TEST_PROGRAMS_NEED_X =
 # interactive shell sessions without exporting it.
 unexport CDPATH
 
-SCRIPT_SH += git-am.sh
 SCRIPT_SH += git-bisect.sh
 SCRIPT_SH += git-difftool--helper.sh
 SCRIPT_SH += git-filter-branch.sh
@@ -812,6 +811,7 @@ LIB_OBJS += xdiff-interface.o
 LIB_OBJS += zlib.o
 
 BUILTIN_OBJS += builtin/add.o
+BUILTIN_OBJS += builtin/am.o
 BUILTIN_OBJS += builtin/annotate.o
 BUILTIN_OBJS += builtin/apply.o
 BUILTIN_OBJS += builtin/archive.o
diff --git a/builtin.h b/builtin.h
index b87df70..d50c9d1 100644
--- a/builtin.h
+++ b/builtin.h
@@ -30,6 +30,7 @@ extern int textconv_object(const char *path, unsigned mode, const unsigned char
 extern int is_builtin(const char *s);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
+extern int cmd_am(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
 extern int cmd_apply(int argc, const char **argv, const char *prefix);
 extern int cmd_archive(int argc, const char **argv, const char *prefix);
diff --git a/builtin/am.c b/builtin/am.c
new file mode 100644
index 0000000..6c00009
--- /dev/null
+++ b/builtin/am.c
@@ -0,0 +1,167 @@
+/*
+ * Builtin "git am"
+ *
+ * Based on git-am.sh by Junio C Hamano.
+ */
+#include "cache.h"
+#include "parse-options.h"
+#include "dir.h"
+
+struct am_state {
+	struct strbuf dir;            /* state directory path */
+	int cur;                      /* current patch number */
+	int last;                     /* last patch number */
+};
+
+/**
+ * Initializes am_state with the default values.
+ */
+static void am_state_init(struct am_state *state)
+{
+	memset(state, 0, sizeof(*state));
+
+	strbuf_init(&state->dir, 0);
+}
+
+/**
+ * Release memory allocated by an am_state.
+ */
+static void am_state_release(struct am_state *state)
+{
+	strbuf_release(&state->dir);
+}
+
+/**
+ * Returns path relative to the am_state directory.
+ */
+static inline const char *am_path(const struct am_state *state, const char *path)
+{
+	return mkpath("%s/%s", state->dir.buf, path);
+}
+
+/**
+ * Returns 1 if there is an am session in progress, 0 otherwise.
+ */
+static int am_in_progress(const struct am_state *state)
+{
+	struct stat st;
+
+	if (lstat(state->dir.buf, &st) < 0 || !S_ISDIR(st.st_mode))
+		return 0;
+	if (lstat(am_path(state, "last"), &st) || !S_ISREG(st.st_mode))
+		return 0;
+	if (lstat(am_path(state, "next"), &st) || !S_ISREG(st.st_mode))
+		return 0;
+	return 1;
+}
+
+/**
+ * Reads the contents of `file`. The third argument can be used to give a hint
+ * about the file size, to avoid reallocs. Returns 0 on success, -1 if the file
+ * does not exist.
+ */
+static int read_state_file(struct strbuf *sb, const char *file, size_t hint) {
+	strbuf_reset(sb);
+
+	if (!strbuf_read_file(sb, file, hint))
+		return 0;
+
+	if (errno == ENOENT)
+		return -1;
+
+	die_errno(_("could not read '%s'"), file);
+}
+
+/**
+ * Loads state from disk.
+ */
+static void am_state_load(struct am_state *state)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	read_state_file(&sb, am_path(state, "next"), 8);
+	state->cur = strtol(sb.buf, NULL, 10);
+
+	read_state_file(&sb, am_path(state, "last"), 8);
+	state->last = strtol(sb.buf, NULL, 10);
+
+	strbuf_release(&sb);
+}
+
+/**
+ * Remove the am_state directory.
+ */
+static void am_destroy(const struct am_state *state)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	strbuf_addstr(&sb, state->dir.buf);
+	remove_dir_recursively(&sb, 0);
+	strbuf_release(&sb);
+}
+
+/**
+ * Setup a new am session for applying patches
+ */
+static void am_setup(struct am_state *state)
+{
+	if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
+		die_errno(_("failed to create directory '%s'"), state->dir.buf);
+
+	write_file(am_path(state, "next"), 1, "%d", state->cur);
+
+	write_file(am_path(state, "last"), 1, "%d", state->last);
+}
+
+/**
+ * Increments the patch pointer, and cleans am_state for the application of the
+ * next patch.
+ */
+static void am_next(struct am_state *state)
+{
+	state->cur++;
+	write_file(am_path(state, "next"), 1, "%d", state->cur);
+}
+
+/**
+ * Applies all queued patches.
+ */
+static void am_run(struct am_state *state)
+{
+	while (state->cur <= state->last)
+		am_next(state);
+
+	am_destroy(state);
+}
+
+struct am_state state;
+
+static const char * const am_usage[] = {
+	N_("git am [options] [(<mbox>|<Maildir>)...]"),
+	NULL
+};
+
+static struct option am_options[] = {
+	OPT_END()
+};
+
+int cmd_am(int argc, const char **argv, const char *prefix)
+{
+	git_config(git_default_config, NULL);
+
+	am_state_init(&state);
+	strbuf_addstr(&state.dir, git_path("rebase-apply"));
+
+	argc = parse_options(argc, argv, prefix, am_options, am_usage, 0);
+
+	if (am_in_progress(&state))
+		am_state_load(&state);
+	else
+		am_setup(&state);
+
+	am_run(&state);
+
+	am_state_release(&state);
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 44374b1..42328ed 100644
--- a/git.c
+++ b/git.c
@@ -370,6 +370,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 
 static struct cmd_struct commands[] = {
 	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+	{ "am", cmd_am, RUN_SETUP | NEED_WORK_TREE },
 	{ "annotate", cmd_annotate, RUN_SETUP },
 	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
 	{ "archive", cmd_archive },
-- 
2.1.4

[PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit

[Paul Tan]

git-am.sh supports mbox, stgit and mercurial patches. Re-implement
support for splitting out mbox/maildirs using git-mailsplit, while also
implementing the framework required to support other patch formats in
the future.

Re-implement support for the --patch-format option (since a5a6755
(git-am foreign patch support: introduce patch_format, 2009-05-27)) to
allow the user to choose between the different patch formats.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 104 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 100 insertions(+), 4 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6c00009..9c7b058 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -6,11 +6,18 @@
 #include "cache.h"
 #include "parse-options.h"
 #include "dir.h"
+#include "run-command.h"
+
+enum patch_format {
+	PATCH_FORMAT_UNKNOWN = 0,
+	PATCH_FORMAT_MBOX
+};
 
 struct am_state {
 	struct strbuf dir;            /* state directory path */
 	int cur;                      /* current patch number */
 	int last;                     /* last patch number */
+	int prec;                     /* number of digits in patch filename */
 };
 
 /**
@@ -21,6 +28,7 @@ static void am_state_init(struct am_state *state)
 	memset(state, 0, sizeof(*state));
 
 	strbuf_init(&state->dir, 0);
+	state->prec = 4;
 }
 
 /**
@@ -101,13 +109,67 @@ static void am_destroy(const struct am_state *state)
 }
 
 /**
+ * Splits out individual patches from `paths`, where each path is either a mbox
+ * file or a Maildir. Return 0 on success, -1 on failure.
+ */
+static int split_patches_mbox(struct am_state *state, struct string_list *paths)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct string_list_item *item;
+	struct strbuf last = STRBUF_INIT;
+
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "mailsplit");
+	argv_array_pushf(&cp.args, "-d%d", state->prec);
+	argv_array_pushf(&cp.args, "-o%s", state->dir.buf);
+	argv_array_push(&cp.args, "-b");
+	argv_array_push(&cp.args, "--");
+
+	for_each_string_list_item(item, paths)
+		argv_array_push(&cp.args, item->string);
+
+	if (capture_command(&cp, &last, 8))
+		return -1;
+
+	state->cur = 1;
+	state->last = strtol(last.buf, NULL, 10);
+
+	return 0;
+}
+
+/**
+ * Splits out individual patches, of patch_format, contained within paths.
+ * These patches will be stored in the state directory, with each patch's
+ * filename being its index, padded to state->prec digits. state->cur will be
+ * set to the index of the first patch, and state->last will be set to the
+ * index of the last patch. Returns 0 on success, -1 on failure.
+ */
+static int split_patches(struct am_state *state, enum patch_format patch_format,
+		struct string_list *paths)
+{
+	switch (patch_format) {
+		case PATCH_FORMAT_MBOX:
+			return split_patches_mbox(state, paths);
+		default:
+			die("BUG: invalid patch_format");
+	}
+	return -1;
+}
+
+/**
  * Setup a new am session for applying patches
  */
-static void am_setup(struct am_state *state)
+static void am_setup(struct am_state *state, enum patch_format patch_format,
+		struct string_list *paths)
 {
 	if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
 		die_errno(_("failed to create directory '%s'"), state->dir.buf);
 
+	if (split_patches(state, patch_format, paths) < 0) {
+		am_destroy(state);
+		die(_("Failed to split patches."));
+	}
+
 	write_file(am_path(state, "next"), 1, "%d", state->cur);
 
 	write_file(am_path(state, "last"), 1, "%d", state->last);
@@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
  */
 static void am_run(struct am_state *state)
 {
-	while (state->cur <= state->last)
+	while (state->cur <= state->last) {
+		/* patch application not implemented yet */
+
 		am_next(state);
+	}
 
 	am_destroy(state);
 }
 
+/**
+ * parse_options() callback that validates and sets opt->value to the
+ * PATCH_FORMAT_* enum value corresponding to `arg`.
+ */
+static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)
+{
+	int *opt_value = opt->value;
+
+	if (!strcmp(arg, "mbox"))
+		*opt_value = PATCH_FORMAT_MBOX;
+	else
+		return -1;
+	return 0;
+}
+
 struct am_state state;
+int opt_patch_format;
 
 static const char * const am_usage[] = {
 	N_("git am [options] [(<mbox>|<Maildir>)...]"),
@@ -142,6 +223,8 @@ static const char * const am_usage[] = {
 };
 
 static struct option am_options[] = {
+	OPT_CALLBACK(0, "patch-format", &opt_patch_format, N_("format"),
+		N_("format the patch(es) are in"), parse_opt_patchformat),
 	OPT_END()
 };
 
@@ -156,8 +239,21 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 
 	if (am_in_progress(&state))
 		am_state_load(&state);
-	else
-		am_setup(&state);
+	else {
+		struct string_list paths = STRING_LIST_INIT_DUP;
+		int i;
+
+		for (i = 0; i < argc; i++) {
+			if (is_absolute_path(argv[i]) || !prefix)
+				string_list_append(&paths, argv[i]);
+			else
+				string_list_append(&paths, mkpath("%s/%s", prefix, argv[i]));
+		}
+
+		am_setup(&state, opt_patch_format, &paths);
+
+		string_list_clear(&paths, 0);
+	}
 
 	am_run(&state);
 
-- 
2.1.4

[PATCH/WIP 5/8] am: detect mbox patches

[Paul Tan]

Since 15ced75 (git-am foreign patch support: autodetect some patch
formats, 2009-05-27), git-am.sh is able to autodetect mbox, stgit and
mercurial patches through heuristics.

Re-implement support for autodetecting mbox/maildir files.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index 9c7b058..d589ec5 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -108,6 +108,97 @@ static void am_destroy(const struct am_state *state)
 	strbuf_release(&sb);
 }
 
+/*
+ * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.
+ * We check this by grabbing all the non-indented lines and seeing if they look
+ * like they begin with valid header field names.
+ */
+static int is_email(const char *filename)
+{
+	struct strbuf sb = STRBUF_INIT;
+	FILE *fp = xfopen(filename, "r");
+	int ret = 1;
+
+	while (!strbuf_getline(&sb, fp, '\n')) {
+		const char *x;
+
+		strbuf_rtrim(&sb);
+
+		if (!sb.len)
+			break; /* End of header */
+
+		/* Ignore indented folded lines */
+		if (*sb.buf == '\t' || *sb.buf == ' ')
+			continue;
+
+		/* It's a header if it matches the regexp "^[!-9;-~]+:" */
+		for (x = sb.buf; *x; x++) {
+			if (('!' <= *x && *x <= '9') || (';' <= *x && *x <= '~'))
+				continue;
+			if (*x == ':' && x != sb.buf)
+				break;
+			ret = 0;
+			goto fail;
+		}
+	}
+
+fail:
+	fclose(fp);
+	strbuf_release(&sb);
+	return ret;
+}
+
+/**
+ * Attempts to detect the patch_format of the patches contained in `paths`,
+ * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if
+ * detection fails.
+ */
+static int detect_patch_format(struct string_list *paths)
+{
+	enum patch_format ret = PATCH_FORMAT_UNKNOWN;
+	struct strbuf l1 = STRBUF_INIT;
+	struct strbuf l2 = STRBUF_INIT;
+	struct strbuf l3 = STRBUF_INIT;
+	FILE *fp;
+
+	/*
+	 * We default to mbox format if input is from stdin and for directories
+	 */
+	if (!paths->nr || !strcmp(paths->items->string, "-") ||
+	    is_directory(paths->items->string)) {
+		strbuf_release(&l1);
+		strbuf_release(&l2);
+		strbuf_release(&l3);
+		return PATCH_FORMAT_MBOX;
+	}
+
+	/*
+	 * Otherwise, check the first few 3 lines of the first patch, starting
+	 * from the first non-blank line, to try to detect its format.
+	 */
+	fp = xfopen(paths->items->string, "r");
+	while (!strbuf_getline(&l1, fp, '\n')) {
+		strbuf_trim(&l1);
+		if (l1.len)
+			break;
+	}
+	strbuf_getline(&l2, fp, '\n');
+	strbuf_trim(&l2);
+	strbuf_getline(&l3, fp, '\n');
+	strbuf_trim(&l3);
+	fclose(fp);
+
+	if (starts_with(l1.buf, "From ") || starts_with(l1.buf, "From: "))
+		ret = PATCH_FORMAT_MBOX;
+	else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
+		ret = PATCH_FORMAT_MBOX;
+
+	strbuf_release(&l1);
+	strbuf_release(&l2);
+	strbuf_release(&l3);
+	return ret;
+}
+
 /**
  * Splits out individual patches from `paths`, where each path is either a mbox
  * file or a Maildir. Return 0 on success, -1 on failure.
@@ -162,6 +253,14 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,
 static void am_setup(struct am_state *state, enum patch_format patch_format,
 		struct string_list *paths)
 {
+	if (!patch_format)
+		patch_format = detect_patch_format(paths);
+
+	if (!patch_format) {
+		fprintf_ln(stderr, _("Patch format detection failed."));
+		exit(128);
+	}
+
 	if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
 		die_errno(_("failed to create directory '%s'"), state->dir.buf);
 
-- 
2.1.4

[PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Paul Tan]

For the purpose of applying the patch and committing the results,
implement extracting the patch data, commit message and authorship from
an e-mail message using git-mailinfo.

git-mailinfo is run as a separate process, but ideally in the future,
we should be be able to access its functionality directly without
spawning a new process.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 231 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 231 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index d589ec5..0b8a42d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -7,6 +7,23 @@
 #include "parse-options.h"
 #include "dir.h"
 #include "run-command.h"
+#include "quote.h"
+
+/**
+ * Returns 1 if the file is empty or does not exist, 0 otherwise.
+ */
+static int is_empty_file(const char *filename)
+{
+	struct stat st;
+
+	if (stat(filename, &st) < 0) {
+		if (errno == ENOENT)
+			return 1;
+		die_errno(_("could not stat %s"), filename);
+	}
+
+	return !st.st_size;
+}
 
 enum patch_format {
 	PATCH_FORMAT_UNKNOWN = 0,
@@ -17,6 +34,10 @@ struct am_state {
 	struct strbuf dir;            /* state directory path */
 	int cur;                      /* current patch number */
 	int last;                     /* last patch number */
+	struct strbuf msg;            /* commit message */
+	struct strbuf author_name;    /* commit author's name */
+	struct strbuf author_email;   /* commit author's email */
+	struct strbuf author_date;    /* commit author's date */
 	int prec;                     /* number of digits in patch filename */
 };
 
@@ -28,6 +49,10 @@ static void am_state_init(struct am_state *state)
 	memset(state, 0, sizeof(*state));
 
 	strbuf_init(&state->dir, 0);
+	strbuf_init(&state->msg, 0);
+	strbuf_init(&state->author_name, 0);
+	strbuf_init(&state->author_email, 0);
+	strbuf_init(&state->author_date, 0);
 	state->prec = 4;
 }
 
@@ -37,6 +62,10 @@ static void am_state_init(struct am_state *state)
 static void am_state_release(struct am_state *state)
 {
 	strbuf_release(&state->dir);
+	strbuf_release(&state->msg);
+	strbuf_release(&state->author_name);
+	strbuf_release(&state->author_email);
+	strbuf_release(&state->author_date);
 }
 
 /**
@@ -81,6 +110,92 @@ static int read_state_file(struct strbuf *sb, const char *file, size_t hint) {
 }
 
 /**
+ * Parses the "author script" `filename`, and sets state->author_name,
+ * state->author_email and state->author_date accordingly. We are strict with
+ * our parsing, as the author script is supposed to be eval'd, and loosely
+ * parsing it may not give the results the user expects.
+ *
+ * The author script is of the format:
+ *
+ *         GIT_AUTHOR_NAME='$author_name'
+ *         GIT_AUTHOR_EMAIL='$author_email'
+ *	   GIT_AUTHOR_DATE='$author_date'
+ *
+ * where $author_name, $author_email and $author_date are quoted.
+ */
+static int read_author_script(struct am_state *state)
+{
+	char *value;
+	struct strbuf sb = STRBUF_INIT;
+	const char *filename = am_path(state, "author-script");
+	FILE *fp = fopen(filename, "r");
+	if (!fp) {
+		if (errno == ENOENT)
+			return 0;
+		die(_("could not open '%s' for reading"), filename);
+	}
+
+	if (strbuf_getline(&sb, fp, '\n'))
+		return -1;
+	if (!skip_prefix(sb.buf, "GIT_AUTHOR_NAME=", (const char**) &value))
+		return -1;
+	value = sq_dequote(value);
+	if (!value)
+		return -1;
+	strbuf_addstr(&state->author_name, value);
+
+	if (strbuf_getline(&sb, fp, '\n'))
+		return -1;
+	if (!skip_prefix(sb.buf, "GIT_AUTHOR_EMAIL=", (const char**) &value))
+		return -1;
+	value = sq_dequote(value);
+	if (!value)
+		return -1;
+	strbuf_addstr(&state->author_email, value);
+
+	if (strbuf_getline(&sb, fp, '\n'))
+		return -1;
+	if (!skip_prefix(sb.buf, "GIT_AUTHOR_DATE=", (const char**) &value))
+		return -1;
+	value = sq_dequote(value);
+	if (!value)
+		return -1;
+	strbuf_addstr(&state->author_date, value);
+
+	if (fgetc(fp) != EOF)
+		return -1;
+
+	fclose(fp);
+	strbuf_release(&sb);
+	return 0;
+}
+
+/**
+ * Saves state->author_name, state->author_email and state->author_date in
+ * `filename` as an "author script", which is the format used by git-am.sh.
+ */
+static void write_author_script(const struct am_state *state)
+{
+	static const char fmt[] = "GIT_AUTHOR_NAME='%s'\n"
+		"GIT_AUTHOR_EMAIL='%s'\n"
+		"GIT_AUTHOR_DATE='%s'\n";
+	struct strbuf author_name = STRBUF_INIT;
+	struct strbuf author_email = STRBUF_INIT;
+	struct strbuf author_date = STRBUF_INIT;
+
+	sq_quote_buf(&author_name, state->author_name.buf);
+	sq_quote_buf(&author_email, state->author_email.buf);
+	sq_quote_buf(&author_date, state->author_date.buf);
+
+	write_file(am_path(state, "author-script"), 1, fmt,
+			author_name.buf, author_email.buf, author_date.buf);
+
+	strbuf_release(&author_name);
+	strbuf_release(&author_email);
+	strbuf_release(&author_date);
+}
+
+/**
  * Loads state from disk.
  */
 static void am_state_load(struct am_state *state)
@@ -93,6 +208,14 @@ static void am_state_load(struct am_state *state)
 	read_state_file(&sb, am_path(state, "last"), 8);
 	state->last = strtol(sb.buf, NULL, 10);
 
+	read_state_file(&state->msg, am_path(state, "final-commit"), 0);
+
+	strbuf_reset(&state->author_name);
+	strbuf_reset(&state->author_email);
+	strbuf_reset(&state->author_date);
+	if (read_author_script(state) < 0)
+		die(_("could not parse author script"));
+
 	strbuf_release(&sb);
 }
 
@@ -282,6 +405,102 @@ static void am_next(struct am_state *state)
 {
 	state->cur++;
 	write_file(am_path(state, "next"), 1, "%d", state->cur);
+
+	strbuf_reset(&state->msg);
+	unlink(am_path(state, "final-commit"));
+
+	strbuf_reset(&state->author_name);
+	strbuf_reset(&state->author_email);
+	strbuf_reset(&state->author_date);
+	unlink(am_path(state, "author-script"));
+}
+
+/**
+ * Returns the filename of the current patch.
+ */
+static const char *msgnum(const struct am_state *state)
+{
+	static struct strbuf fmt = STRBUF_INIT;
+	static struct strbuf sb = STRBUF_INIT;
+
+	strbuf_reset(&fmt);
+	strbuf_addf(&fmt, "%%0%dd", state->prec);
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, fmt.buf, state->cur);
+
+	return sb.buf;
+}
+
+/**
+ * Parses `patch` using git-mailinfo. state->msg will be set to the patch
+ * message. state->author_name, state->author_email, state->author_date will be
+ * set to the patch author's name, email and date respectively. The patch's
+ * body will be written to "$state_dir/patch", where $state_dir is the state
+ * directory.
+ *
+ * Returns 1 if the patch should be skipped, 0 otherwise.
+ */
+static int parse_patch(struct am_state *state, const char *patch)
+{
+	FILE *fp;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	cp.git_cmd = 1;
+	cp.in = xopen(patch, O_RDONLY, 0);
+	cp.out = xopen(am_path(state, "info"), O_WRONLY | O_CREAT, 0777);
+
+	argv_array_push(&cp.args, "mailinfo");
+	argv_array_push(&cp.args, am_path(state, "msg"));
+	argv_array_push(&cp.args, am_path(state, "patch"));
+
+	if (run_command(&cp) < 0)
+		die("could not parse patch");
+
+	close(cp.in);
+	close(cp.out);
+
+	/* Extract message and author information */
+	fp = xfopen(am_path(state, "info"), "r");
+	while (!strbuf_getline(&sb, fp, '\n')) {
+		const char *x;
+
+		if (skip_prefix(sb.buf, "Subject: ", &x)) {
+			if (state->msg.len)
+				strbuf_addch(&state->msg, '\n');
+			strbuf_addstr(&state->msg, x);
+		} else if (skip_prefix(sb.buf, "Author: ", &x)) {
+			if (state->author_name.len)
+				strbuf_addch(&state->author_name, '\n');
+			strbuf_addstr(&state->author_name, x);
+		} else if (skip_prefix(sb.buf, "Email: ", &x)) {
+			if (state->author_email.len)
+				strbuf_addch(&state->author_email, '\n');
+			strbuf_addstr(&state->author_email, x);
+		} else if (skip_prefix(sb.buf, "Date: ", &x)) {
+			if (state->author_date.len)
+				strbuf_addch(&state->author_date, '\n');
+			strbuf_addstr(&state->author_date, x);
+		}
+	}
+	fclose(fp);
+
+	/* Skip pine's internal folder data */
+	if (!strcmp(state->author_name.buf, "Mail System Internal Data"))
+		return 1;
+
+	if (is_empty_file(am_path(state, "patch")))
+		die(_("Patch is empty. Was it split wrong?\n"
+		"If you would prefer to skip this patch, instead run \"git am --skip\".\n"
+		"To restore the original branch and stop patching run \"git am --abort\"."));
+
+	strbuf_addstr(&state->msg, "\n\n");
+	if (strbuf_read_file(&state->msg, am_path(state, "msg"), 0) < 0)
+		die_errno(_("could not read '%s'"), am_path(state, "msg"));
+	stripspace(&state->msg, 0);
+
+	return 0;
 }
 
 /**
@@ -290,8 +509,20 @@ static void am_next(struct am_state *state)
 static void am_run(struct am_state *state)
 {
 	while (state->cur <= state->last) {
+		const char *patch = am_path(state, msgnum(state));
+
+		if (!file_exists(patch))
+			goto next;
+
+		if (parse_patch(state, patch))
+			goto next; /* patch should be skipped */
+
+		write_file(am_path(state, "final-commit"), 1, "%s", state->msg.buf);
+		write_author_script(state);
+
 		/* patch application not implemented yet */
 
+next:
 		am_next(state);
 	}
 
-- 
2.1.4

[PATCH/WIP 7/8] am: apply patch with git-apply

[Paul Tan]

Implement applying the patch to the index using git-apply.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 0b8a42d..7126df3 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -25,6 +25,18 @@ static int is_empty_file(const char *filename)
 	return !st.st_size;
 }
 
+/**
+ * Returns the first line of msg
+ */
+static const char *firstline(const char *msg)
+{
+	static struct strbuf sb = STRBUF_INIT;
+
+	strbuf_reset(&sb);
+	strbuf_add(&sb, msg, strchrnul(msg, '\n') - msg);
+	return sb.buf;
+}
+
 enum patch_format {
 	PATCH_FORMAT_UNKNOWN = 0,
 	PATCH_FORMAT_MBOX
@@ -503,6 +515,29 @@ static int parse_patch(struct am_state *state, const char *patch)
 	return 0;
 }
 
+/*
+ * Applies current patch with git-apply. Returns 0 on success, -1 otherwise.
+ */
+static int run_apply(const struct am_state *state)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	cp.git_cmd = 1;
+
+	argv_array_push(&cp.args, "apply");
+	argv_array_push(&cp.args, "--index");
+	argv_array_push(&cp.args, am_path(state, "patch"));
+
+	if (run_command(&cp))
+		return -1;
+
+	/* Reload index as git-apply will have modified it. */
+	discard_cache();
+	read_cache();
+
+	return 0;
+}
+
 /**
  * Applies all queued patches.
  */
@@ -520,7 +555,20 @@ static void am_run(struct am_state *state)
 		write_file(am_path(state, "final-commit"), 1, "%s", state->msg.buf);
 		write_author_script(state);
 
-		/* patch application not implemented yet */
+		printf_ln(_("Applying: %s"), firstline(state->msg.buf));
+
+		if (run_apply(state) < 0) {
+			int value;
+
+			printf_ln(_("Patch failed at %s %s"), msgnum(state),
+					firstline(state->msg.buf));
+
+			if (!git_config_get_bool("advice.amworkdir", &value) && !value)
+				printf_ln(_("The copy of the patch that failed is found in: %s"),
+						am_path(state, "patch"));
+
+			exit(128);
+		}
 
 next:
 		am_next(state);
-- 
2.1.4

[PATCH/WIP 8/8] am: commit applied patch

[Paul Tan]

Implement do_commit(), which commits the index which contains the
results of applying the patch, along with the extracted commit message
and authorship information.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/builtin/am.c b/builtin/am.c
index 7126df3..3174327 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -8,6 +8,9 @@
 #include "dir.h"
 #include "run-command.h"
 #include "quote.h"
+#include "cache-tree.h"
+#include "refs.h"
+#include "commit.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -539,6 +542,48 @@ static int run_apply(const struct am_state *state)
 }
 
 /**
+ * Commits the current index with state->msg as the commit message and
+ * state->author_name, state->author_email and state->author_date as the author
+ * information.
+ */
+static void do_commit(const struct am_state *state)
+{
+	unsigned char tree[20], parent[20], commit[20];
+	unsigned char *ptr;
+	struct commit_list *parents = NULL;
+	const char *reflog_msg, *author;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (write_cache_as_tree(tree, 0, NULL))
+		die(_("git write-tree failed to write a tree"));
+
+	if (!get_sha1_commit("HEAD", parent)) {
+		ptr = parent;
+		commit_list_insert(lookup_commit(parent), &parents);
+	} else {
+		ptr = NULL;
+		fprintf_ln(stderr, _("applying to an empty history"));
+	}
+
+	author = fmt_ident(state->author_name.buf, state->author_email.buf,
+			state->author_date.buf, IDENT_STRICT);
+
+	if (commit_tree(state->msg.buf, state->msg.len, tree, parents, commit,
+				author, NULL))
+		die(_("failed to write commit object"));
+
+	reflog_msg = getenv("GIT_REFLOG_ACTION");
+	if (!reflog_msg)
+		reflog_msg = "am";
+
+	strbuf_addf(&sb, "%s: %s", reflog_msg, firstline(state->msg.buf));
+
+	update_ref(sb.buf, "HEAD", commit, ptr, 0, UPDATE_REFS_DIE_ON_ERR);
+
+	strbuf_release(&sb);
+}
+
+/**
  * Applies all queued patches.
  */
 static void am_run(struct am_state *state)
@@ -570,6 +615,7 @@ static void am_run(struct am_state *state)
 			exit(128);
 		}
 
+		do_commit(state);
 next:
 		am_next(state);
 	}
-- 
2.1.4

Re: [PATCH/WIP 1/8] wrapper: implement xopen()

[Stefan Beller]

On Wed, May 27, 2015 at 6:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
> A common usage pattern of open() is to check if it was successful, and
> die() if it was not:
>
>         int fd = open(path, O_WRONLY | O_CREAT, 0777);
>         if (fd < 0)
>                 die_errno(_("Could not open '%s' for writing."), path);
>
> Implement a wrapper function xopen() that does the above so that we can
> save a few lines of code, and make the die() messages consistent.

As a mental todo note for whomever wants to pick up some work: This patch
series indicates to only touch git-am. The first 2 patches introduce
new x-wrappers,
so maybe we'd need to grep through the whole code base and convert the all
these file opening code to the new wrapper.

>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  git-compat-util.h |  1 +
>  wrapper.c         | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 17584ad..9745962 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);
>  extern void *xrealloc(void *ptr, size_t size);
>  extern void *xcalloc(size_t nmemb, size_t size);
>  extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
> +extern int xopen(const char *path, int flags, mode_t mode);
>  extern ssize_t xread(int fd, void *buf, size_t len);
>  extern ssize_t xwrite(int fd, const void *buf, size_t len);
>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
> diff --git a/wrapper.c b/wrapper.c
> index c1a663f..971665a 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -189,6 +189,24 @@ void *xcalloc(size_t nmemb, size_t size)
>  # endif
>  #endif
>
> +/**
> + * xopen() is the same as open(), but it die()s if the open() fails.
> + */
> +int xopen(const char *path, int flags, mode_t mode)
> +{
> +       int fd;
> +
> +       assert(path);
> +       fd = open(path, flags, mode);
> +       if (fd < 0) {
> +               if ((flags & O_WRONLY) || (flags & O_RDWR))
> +                       die_errno(_("could not open '%s' for writing"), path);
> +               else
> +                       die_errno(_("could not open '%s' for reading"), path);
> +       }
> +       return fd;
> +}
> +
>  /*
>   * xread() is the same a read(), but it automatically restarts read()
>   * operations with a recoverable error (EAGAIN and EINTR). xread()
> --
> 2.1.4
>

Re: [PATCH/WIP 1/8] wrapper: implement xopen()

[Torsten Bögershausen]

On 2015-05-27 15.33, Paul Tan wrote:
> A common usage pattern of open() is to check if it was successful, and
> die() if it was not:
> 
> 	int fd = open(path, O_WRONLY | O_CREAT, 0777);
> 	if (fd < 0)
> 		die_errno(_("Could not open '%s' for writing."), path);
> 
> Implement a wrapper function xopen() that does the above so that we can
> save a few lines of code, and make the die() messages consistent.
> 
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  git-compat-util.h |  1 +
>  wrapper.c         | 18 ++++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 17584ad..9745962 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -718,6 +718,7 @@ extern char *xstrndup(const char *str, size_t len);
>  extern void *xrealloc(void *ptr, size_t size);
>  extern void *xcalloc(size_t nmemb, size_t size);
>  extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);
> +extern int xopen(const char *path, int flags, mode_t mode);
>  extern ssize_t xread(int fd, void *buf, size_t len);
>  extern ssize_t xwrite(int fd, const void *buf, size_t len);
>  extern ssize_t xpread(int fd, void *buf, size_t len, off_t offset);
> diff --git a/wrapper.c b/wrapper.c
> index c1a663f..971665a 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -189,6 +189,24 @@ void *xcalloc(size_t nmemb, size_t size)
>  # endif
>  #endif
>  
The original open can take 2 or 3 parameters, how about this:
int xopen(const char *path, int oflag, ... )
{
        va_list params;
        int mode;
        int fd;

        va_start(params, oflag);
        mode = va_arg(params, int);
        va_end(params);

        fd = open(path, oflag, mode);


> +/**
> + * xopen() is the same as open(), but it die()s if the open() fails.
> + */
> +int xopen(const char *path, int flags, mode_t mode)
> +{
> +	int fd;
> +
> +	assert(path);
> +	fd = open(path, flags, mode);
> +	if (fd < 0) {
> +		if ((flags & O_WRONLY) || (flags & O_RDWR))
> +			die_errno(_("could not open '%s' for writing"), path);
This is only partly true:
it could be either "writing" or "read write".
I don't know if the info "for reading" or "for writing" is needed/helpful at all,
or if a simple "could not open" would be enough.


Another thing:
should we handle EINTR ?
(Somewhere in the back of my head I remember that some OS
 returned EINTR when handling some foreign file system
 Mac OS / NTFS ?)

Re: [PATCH/WIP 3/8] am: implement patch queue mechanism

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

>  Makefile     |   2 +-
>  builtin.h    |   1 +
>  builtin/am.c | 167 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  git.c        |   1 +
>  4 files changed, 170 insertions(+), 1 deletion(-)
>  create mode 100644 builtin/am.c
>
> diff --git a/Makefile b/Makefile
> index 323c401..57a7c8c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -466,7 +466,6 @@ TEST_PROGRAMS_NEED_X =
>  # interactive shell sessions without exporting it.
>  unexport CDPATH
>  
> -SCRIPT_SH += git-am.sh

If you are building this "new am" incrementally (and for something
complex like "am", that's the only sensible way), perhaps it is a
good idea to do the "competing implementation" trick I suggested
earlier when we were discussing your "new pull" patches, to allow
you to keep both versions and switch between them at runtime.  That
way, you can run tests, both existing ones and new ones you add, on
both versions to make sure they behave the same way.

Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> @@ -17,6 +34,10 @@ struct am_state {
>  	struct strbuf dir;            /* state directory path */
>  	int cur;                      /* current patch number */
>  	int last;                     /* last patch number */
> +	struct strbuf msg;            /* commit message */
> +	struct strbuf author_name;    /* commit author's name */
> +	struct strbuf author_email;   /* commit author's email */
> +	struct strbuf author_date;    /* commit author's date */
>  	int prec;                     /* number of digits in patch filename */
>  };

I always get suspicious when structure fields are overly commented,
wondering if it is a sign of naming fields poorly.  All of the above
fields look quite self-explanatory and I am not sure if it is worth
having these comments, spending efforts to type SP many times to
align them and all.

By the way, the overall structure of the series look sensible.

> +static int read_author_script(struct am_state *state)
> +{
> +	char *value;
> +	struct strbuf sb = STRBUF_INIT;
> +	const char *filename = am_path(state, "author-script");
> +	FILE *fp = fopen(filename, "r");
> +	if (!fp) {
> +		if (errno == ENOENT)
> +			return 0;
> +		die(_("could not open '%s' for reading"), filename);

Hmph, do we want to report with die_errno()?

> +	}
> +
> +	if (strbuf_getline(&sb, fp, '\n'))
> +		return -1;
> +	if (!skip_prefix(sb.buf, "GIT_AUTHOR_NAME=", (const char**) &value))

This cast is unfortunate; can't "value" be of "const char *" type?

Re: [PATCH/WIP 1/8] wrapper: implement xopen()

[Jeff King]

On Wed, May 27, 2015 at 09:03:47PM +0200, Torsten Bögershausen wrote:

> The original open can take 2 or 3 parameters, how about this:
> int xopen(const char *path, int oflag, ... )
> {
>         va_list params;
>         int mode;
>         int fd;
> 
>         va_start(params, oflag);
>         mode = va_arg(params, int);
>         va_end(params);
> 
>         fd = open(path, oflag, mode);

Don't you need a conditional on pulling the mode arg off the stack
(i.e., if O_CREAT is in the flags)?

-Peff

Re: [PATCH/WIP 2/8] wrapper: implement xfopen()

[Jeff King]

On Wed, May 27, 2015 at 09:33:32PM +0800, Paul Tan wrote:

> +/**
> + * xfopen() is the same as fopen(), but it die()s if the fopen() fails.
> + */
> +FILE *xfopen(const char *path, const char *mode)
> +{
> +	FILE *fp;
> +
> +	assert(path);
> +	assert(mode);
> +	fp = fopen(path, mode);
> +	if (!fp) {
> +		if (*mode == 'w' || *mode == 'a')
> +			die_errno(_("could not open '%s' for writing"), path);

This misses "r+". I don't think we use that in our code currently, but
if we're going to introduce a wrapper like this, I think it makes sense
to cover all cases.

-Peff

Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> +static const char *msgnum(const struct am_state *state)
> +{
> +	static struct strbuf fmt = STRBUF_INIT;
> +	static struct strbuf sb = STRBUF_INIT;
> +
> +	strbuf_reset(&fmt);
> +	strbuf_addf(&fmt, "%%0%dd", state->prec);
> +
> +	strbuf_reset(&sb);
> +	strbuf_addf(&sb, fmt.buf, state->cur);

Hmph, wouldn't ("%*d", state->prec, state->cur) work or am I missing
something?

Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Jeff King]

On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:

> Paul Tan <pyokagan@gmail.com> writes:
> 
> > @@ -17,6 +34,10 @@ struct am_state {
> >  	struct strbuf dir;            /* state directory path */
> >  	int cur;                      /* current patch number */
> >  	int last;                     /* last patch number */
> > +	struct strbuf msg;            /* commit message */
> > +	struct strbuf author_name;    /* commit author's name */
> > +	struct strbuf author_email;   /* commit author's email */
> > +	struct strbuf author_date;    /* commit author's date */
> >  	int prec;                     /* number of digits in patch filename */
> >  };
> 
> I always get suspicious when structure fields are overly commented,
> wondering if it is a sign of naming fields poorly.  All of the above
> fields look quite self-explanatory and I am not sure if it is worth
> having these comments, spending efforts to type SP many times to
> align them and all.

Just my 2 cents, but I think that grouping and overhead comments can
often make things more obvious. For example:

  struct am_state {
	/* state directory path */
	struct strbuf dir;

	/*
	 * current and last patch numbers; are these 1-indexed
	 * or 0-indexed?
	 */
	int cur;
	int last;

	struct strbuf author_name;
	struct strbuf author_email;
	struct strbuf author_date;
	struct strbuf msg;

	/* number of digits in patch filename */
	int prec;
  }

Note that by grouping "cur" and "last", we can discuss them as a group,
and the overhead comment gives us room to mention their shared
properties (my indexing question is a real question, btw, whose answer I
think would be useful to mention in a comment).

Likewise, by grouping the "msg" strbuf with the author information, it
becomes much more clear that they are all about the commit metadata
(though I would not be opposed to a comment above the whole block,
either).

-Peff

Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit

[Eric Sunshine]

On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
> git-am.sh supports mbox, stgit and mercurial patches. Re-implement
> support for splitting out mbox/maildirs using git-mailsplit, while also
> implementing the framework required to support other patch formats in
> the future.
>
> Re-implement support for the --patch-format option (since a5a6755
> (git-am foreign patch support: introduce patch_format, 2009-05-27)) to
> allow the user to choose between the different patch formats.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
>   */
> +/**
> + * parse_options() callback that validates and sets opt->value to the
> + * PATCH_FORMAT_* enum value corresponding to `arg`.
> + */
> +static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)
> +{
> +       int *opt_value = opt->value;
> +
> +       if (!strcmp(arg, "mbox"))
> +               *opt_value = PATCH_FORMAT_MBOX;
> +       else
> +               return -1;
> +       return 0;
> +}
> +
>  struct am_state state;
> +int opt_patch_format;

Should these two variables be static?

>  static const char * const am_usage[] = {
>         N_("git am [options] [(<mbox>|<Maildir>)...]"),

Re: [PATCH/WIP 5/8] am: detect mbox patches

[Eric Sunshine]

On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Since 15ced75 (git-am foreign patch support: autodetect some patch
> formats, 2009-05-27), git-am.sh is able to autodetect mbox, stgit and
> mercurial patches through heuristics.
>
> Re-implement support for autodetecting mbox/maildir files.

A few minor comments below...

> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> diff --git a/builtin/am.c b/builtin/am.c
> index 9c7b058..d589ec5 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -108,6 +108,97 @@ static void am_destroy(const struct am_state *state)
>         strbuf_release(&sb);
>  }
>
> +/*
> + * Returns 1 if the file looks like a piece of email a-la RFC2822, 0 otherwise.
> + * We check this by grabbing all the non-indented lines and seeing if they look
> + * like they begin with valid header field names.
> + */
> +static int is_email(const char *filename)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       FILE *fp = xfopen(filename, "r");
> +       int ret = 1;
> +
> +       while (!strbuf_getline(&sb, fp, '\n')) {
> +               const char *x;
> +
> +               strbuf_rtrim(&sb);
> +
> +               if (!sb.len)
> +                       break; /* End of header */
> +
> +               /* Ignore indented folded lines */
> +               if (*sb.buf == '\t' || *sb.buf == ' ')
> +                       continue;
> +
> +               /* It's a header if it matches the regexp "^[!-9;-~]+:" */
> +               for (x = sb.buf; *x; x++) {
> +                       if (('!' <= *x && *x <= '9') || (';' <= *x && *x <= '~'))
> +                               continue;
> +                       if (*x == ':' && x != sb.buf)
> +                               break;
> +                       ret = 0;
> +                       goto fail;
> +               }
> +       }
> +
> +fail:

It's a bit odd seeing a label named "fail" through which both the
successful and failing cases flow. Upon seeing it, I had expected to
find a "return 1" somewhere above for the successful case. Perhaps a
name such as "done" would be clearer.

> +       fclose(fp);
> +       strbuf_release(&sb);
> +       return ret;
> +}
> +
> +/**
> + * Attempts to detect the patch_format of the patches contained in `paths`,
> + * returning the PATCH_FORMAT_* enum value. Returns PATCH_FORMAT_UNKNOWN if
> + * detection fails.
> + */
> +static int detect_patch_format(struct string_list *paths)
> +{
> +       enum patch_format ret = PATCH_FORMAT_UNKNOWN;
> +       struct strbuf l1 = STRBUF_INIT;
> +       struct strbuf l2 = STRBUF_INIT;
> +       struct strbuf l3 = STRBUF_INIT;
> +       FILE *fp;
> +
> +       /*
> +        * We default to mbox format if input is from stdin and for directories
> +        */
> +       if (!paths->nr || !strcmp(paths->items->string, "-") ||
> +           is_directory(paths->items->string)) {
> +               strbuf_release(&l1);
> +               strbuf_release(&l2);
> +               strbuf_release(&l3);
> +               return PATCH_FORMAT_MBOX;

A couple observations:

Some people will argue that these strbuf_release() calls are pointless
since the strbufs haven't been used yet, thus should be dropped. (I
don't care strongly.)

Perhaps this case could be a bit less noisy if coded like this:

    if (... || ...) {
        ret = PATCH_FORMAT_MBOX;
        goto done;
    }

with a corresponding "done" label added before the strbuf_release()
calls near the bottom of the function.

> +       }
> +
> +       /*
> +        * Otherwise, check the first few 3 lines of the first patch, starting

Either: s/few 3/few/ or s/few 3/3/

> +        * from the first non-blank line, to try to detect its format.
> +        */
> +       fp = xfopen(paths->items->string, "r");
> +       while (!strbuf_getline(&l1, fp, '\n')) {
> +               strbuf_trim(&l1);
> +               if (l1.len)
> +                       break;
> +       }
> +       strbuf_getline(&l2, fp, '\n');
> +       strbuf_trim(&l2);
> +       strbuf_getline(&l3, fp, '\n');
> +       strbuf_trim(&l3);
> +       fclose(fp);
> +
> +       if (starts_with(l1.buf, "From ") || starts_with(l1.buf, "From: "))
> +               ret = PATCH_FORMAT_MBOX;
> +       else if (l1.len && l2.len && l3.len && is_email(paths->items->string))
> +               ret = PATCH_FORMAT_MBOX;
> +
> +       strbuf_release(&l1);
> +       strbuf_release(&l2);
> +       strbuf_release(&l3);
> +       return ret;
> +}
> +
>  /**
>   * Splits out individual patches from `paths`, where each path is either a mbox
>   * file or a Maildir. Return 0 on success, -1 on failure.
> @@ -162,6 +253,14 @@ static int split_patches(struct am_state *state, enum patch_format patch_format,
>  static void am_setup(struct am_state *state, enum patch_format patch_format,
>                 struct string_list *paths)
>  {
> +       if (!patch_format)
> +               patch_format = detect_patch_format(paths);
> +
> +       if (!patch_format) {
> +               fprintf_ln(stderr, _("Patch format detection failed."));
> +               exit(128);
> +       }
> +
>         if (mkdir(state->dir.buf, 0777) < 0 && errno != EEXIST)
>                 die_errno(_("failed to create directory '%s'"), state->dir.buf);
>
> --
> 2.1.4

Re: [PATCH/WIP 4/8] am: split out mbox/maildir patches with git-mailsplit

[Paul Tan]

On Fri, May 29, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> @@ -128,13 +190,32 @@ static void am_next(struct am_state *state)
>>   */
>> +/**
>> + * parse_options() callback that validates and sets opt->value to the
>> + * PATCH_FORMAT_* enum value corresponding to `arg`.
>> + */
>> +static int parse_opt_patchformat(const struct option *opt, const char *arg, int unset)
>> +{
>> +       int *opt_value = opt->value;
>> +
>> +       if (!strcmp(arg, "mbox"))
>> +               *opt_value = PATCH_FORMAT_MBOX;
>> +       else
>> +               return -1;
>> +       return 0;
>> +}
>> +
>>  struct am_state state;
>> +int opt_patch_format;
>
> Should these two variables be static?

Whoops. Yes, they should.

Thanks,
Paul

Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Paul Tan]

On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> @@ -17,6 +34,10 @@ struct am_state {
>>       struct strbuf dir;            /* state directory path */
>>       int cur;                      /* current patch number */
>>       int last;                     /* last patch number */
>> +     struct strbuf msg;            /* commit message */
>> +     struct strbuf author_name;    /* commit author's name */
>> +     struct strbuf author_email;   /* commit author's email */
>> +     struct strbuf author_date;    /* commit author's date */
>>       int prec;                     /* number of digits in patch filename */
>>  };
>
> I always get suspicious when structure fields are overly commented,
> wondering if it is a sign of naming fields poorly.  All of the above
> fields look quite self-explanatory and I am not sure if it is worth
> having these comments, spending efforts to type SP many times to
> align them and all.

Okay, I'll take Jeff's suggestion to organize them into blocks.

>> +static int read_author_script(struct am_state *state)
>> +{
>> +     char *value;
>> +     struct strbuf sb = STRBUF_INIT;
>> +     const char *filename = am_path(state, "author-script");
>> +     FILE *fp = fopen(filename, "r");
>> +     if (!fp) {
>> +             if (errno == ENOENT)
>> +                     return 0;
>> +             die(_("could not open '%s' for reading"), filename);
>
> Hmph, do we want to report with die_errno()?

Yes, we do.

>> +     }
>> +
>> +     if (strbuf_getline(&sb, fp, '\n'))
>> +             return -1;
>> +     if (!skip_prefix(sb.buf, "GIT_AUTHOR_NAME=", (const char**) &value))
>
> This cast is unfortunate; can't "value" be of "const char *" type?

We can't, because sq_dequote() modifies the string directly. I would
think casting from non-const to const is safer than the other way
around.

Thanks,
Paul

Re: [PATCH/WIP 6/8] am: extract patch, message and authorship with git-mailinfo

[Paul Tan]

On Thu, May 28, 2015 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> +static const char *msgnum(const struct am_state *state)
>> +{
>> +     static struct strbuf fmt = STRBUF_INIT;
>> +     static struct strbuf sb = STRBUF_INIT;
>> +
>> +     strbuf_reset(&fmt);
>> +     strbuf_addf(&fmt, "%%0%dd", state->prec);
>> +
>> +     strbuf_reset(&sb);
>> +     strbuf_addf(&sb, fmt.buf, state->cur);
>
> Hmph, wouldn't ("%*d", state->prec, state->cur) work or am I missing
> something?

Yeah, I think it would. I justs didn't know about the existence of '*'.

Thanks,
Paul

Re: [PATCH/WIP 1/8] wrapper: implement xopen()

[Paul Tan]

On Thu, May 28, 2015 at 5:53 AM, Jeff King <peff@peff.net> wrote:
> On Wed, May 27, 2015 at 09:03:47PM +0200, Torsten Bögershausen wrote:
>
>> The original open can take 2 or 3 parameters, how about this:
>> int xopen(const char *path, int oflag, ... )
>> {
>>         va_list params;
>>         int mode;
>>         int fd;
>>
>>         va_start(params, oflag);
>>         mode = va_arg(params, int);
>>         va_end(params);
>>
>>         fd = open(path, oflag, mode);
>
> Don't you need a conditional on pulling the mode arg off the stack
> (i.e., if O_CREAT is in the flags)?

Yeah, we do, as va_arg()'s behavior is undefined if we do not have the
next argument.

The POSIX spec[1] only mentions O_CREAT as requiring the extra
argument, so I guess we'll only need to check for that.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html

Thanks,
Paul

Re: [PATCH/WIP 1/8] wrapper: implement xopen()

[Paul Tan]

On Thu, May 28, 2015 at 3:03 AM, Torsten Bögershausen <tboegi@web.de> wrote:
> On 2015-05-27 15.33, Paul Tan wrote:
>> +/**
>> + * xopen() is the same as open(), but it die()s if the open() fails.
>> + */
>> +int xopen(const char *path, int flags, mode_t mode)
>> +{
>> +     int fd;
>> +
>> +     assert(path);
>> +     fd = open(path, flags, mode);
>> +     if (fd < 0) {
>> +             if ((flags & O_WRONLY) || (flags & O_RDWR))
>> +                     die_errno(_("could not open '%s' for writing"), path);
> This is only partly true:
> it could be either "writing" or "read write".

Ah right, I see now that the POSIX spec allows for, and encourages
O_RDONLY | O_WRONLY == O_RDWR.

> I don't know if the info "for reading" or "for writing" is needed/helpful at all,
> or if a simple "could not open" would be enough.

Yeah, I agree that it may not be helpful, but I noticed that most
error messages in git are of the form "unable to open X for writing",
"unable to open X for reading", "could not create X" etc. Or rather I
thought I noticed, but it now seems to me that there are quite a lot
of uses of "could not open X" as well.

I guess I will remove the distinction.

Thanks,
Paul