threads / patch / 39438

patch, 8 partswrapper: implement xfopen()

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

## tl;dr

24 messages between May 27, 2015 and Jun 4, 2015. Diffs are folded; open one to read it.

replies: 23people: 6as markdown or json

Paul Tan· May 27, 2015, 13:33 UTC · lore

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

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
Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(+)
Show changes to 2 files +19 −0

git-compat-util.h, wrapper.c

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
Stefan Beller· May 27, 2015, 17:52 UTC · re: Paul Tan · lore

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

On Wed, May 27, 2015 at 6:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 9 quoted lines
> 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.

Show 51 quoted lines
>
> 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
>
Torsten Bögershausen· May 27, 2015, 19:03 UTC · re: Paul Tan · lore

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

On 2015-05-27 15.33, Paul Tan wrote:
Show 36 quoted lines
> 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);
Show 12 quoted lines
> +/**
> + * 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 ?)
Jeff King· May 27, 2015, 21:53 UTC · re: Torsten Bögershausen · lore

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

On Wed, May 27, 2015 at 09:03:47PM +0200, Torsten Bögershausen wrote:
Show 12 quoted lines
> 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
Paul Tan· Jun 3, 2015, 08:16 UTC · re: Jeff King · lore

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

On Thu, May 28, 2015 at 5:53 AM, Jeff King <peff@peff.net> wrote:
Show 17 quoted lines
> 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

Paul Tan· Jun 4, 2015, 12:05 UTC · re: Torsten Bögershausen · lore

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

On Thu, May 28, 2015 at 3:03 AM, Torsten Bögershausen <tboegi@web.de> wrote:
Show 15 quoted lines
> 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

Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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(+)
Show changes to 2 files +20 −0

git-compat-util.h, wrapper.c

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
Jeff King· May 27, 2015, 21:55 UTC · re: Paul Tan · lore

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

On Wed, May 27, 2015 at 09:33:32PM +0800, Paul Tan wrote:
Show 13 quoted lines
> +/**
> + * 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
Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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
Show changes to 4 files +170 −1

Makefile, builtin.h, builtin/am.c, git.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
Junio C Hamano· May 27, 2015, 20:38 UTC · re: Paul Tan · lore

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

Paul Tan <pyokagan@gmail.com> writes:
Show 16 quoted lines
>  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.

Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(-)
Show changes to builtin/am.c +100 −4
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
Eric Sunshine· May 28, 2015, 23:05 UTC · re: Paul Tan · lore

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

On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 30 quoted lines
> 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>)...]"),
Paul Tan· Jun 2, 2015, 14:27 UTC · re: Eric Sunshine · lore

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

On Fri, May 29, 2015 at 7:05 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 22 quoted lines
> 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

Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(+)
Show changes to builtin/am.c +99 −0
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
Eric Sunshine· May 31, 2015, 17:16 UTC · re: Paul Tan · lore

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

On Wed, May 27, 2015 at 9:33 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 5 quoted lines
> 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...
Show 45 quoted lines
> 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.

Show 27 quoted lines
> +       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/
Show 45 quoted lines
> +        * 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
Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(+)
Show changes to builtin/am.c +231 −0
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
Junio C Hamano· May 27, 2015, 20:44 UTC · re: Paul Tan · lore

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

Paul Tan <pyokagan@gmail.com> writes:
Show 10 quoted lines
> @@ -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.
Show 10 quoted lines
> +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()?
Show 5 quoted lines
> +	}
> +
> +	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?
Jeff King· May 27, 2015, 22:13 UTC · re: Junio C Hamano · lore

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

On Wed, May 27, 2015 at 01:44:26PM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> 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
Paul Tan· Jun 3, 2015, 07:56 UTC · re: Junio C Hamano · lore

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

On Thu, May 28, 2015 at 4:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> 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.
Show 12 quoted lines
>> +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.
Show 7 quoted lines
>> +     }
>> +
>> +     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

Junio C Hamano· May 27, 2015, 22:13 UTC · re: Paul Tan · lore

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

Paul Tan <pyokagan@gmail.com> writes:
Show 10 quoted lines
> +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?

Paul Tan· Jun 3, 2015, 07:57 UTC · re: Junio C Hamano · lore

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

On Thu, May 28, 2015 at 6:13 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> 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

Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(-)
Show changes to builtin/am.c +49 −1
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
Paul Tan· May 27, 2015, 13:33 UTC · re: Paul Tan · lore

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

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(+)
Show changes to builtin/am.c +46 −0
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

← back to recent threads