[PATCH v4 00/45] Massive improvents to rebase and cherry-pick

[PATCH v4 00/45] Massive improvents to rebase and cherry-pick

[Felipe Contreras]

Hi,

These are improvements to 'git rebase' by using a much improved 'git
cherry-pick'. I already sent some of these, but they have been revamped.

These changes require reorganization of the code in order to have a
builtin/lib.a library.

A new builtin/rewrite.c helper is added, and builtin/commit updated to use
that.

A new git-rebase--cherypick mode is added, and it replaces git-rebase--am and
git-rebase--merge.

I also added the new rebase tests by Martin von Zweigbergk to make sure
everything works, and in fact, it works better than before, since now all the
rebase modes are consistent with each other.

I don't have any hopes of these getting merged, because people have no interest
in fixing the ./*.o ./builin/*.o divide problem, only interested in arguing
that libgit.a is not a real library, and it should never be. So my obviously
cleanup is rejected.

Felipe Contreras (38):
  build: generate and clean test scripts
  build: do not install git-remote-testgit
  build: trivial cleanup
  build: add builtin lib
  log-tree: remove dependency from sequencer
  Move sequencer to builtin
  unpack-trees: plug a memory leak
  read-cache: plug a few leaks
  sequencer: remove useless indentation
  sequencer: trivial fix
  cherry-pick: don't barf when there's nothing to do
  cherry-pick: add --skip-empty option
  revert/cherry-pick: add --quiet option
  revert/cherry-pick: add --skip option
  builtin: add rewrite helper
  cherry-pick: store rewritten commits
  cherry-pick: don't store skipped commit
  builtin: move run_rewrite_hook() to rewrite.c
  builtin: add copy_rewrite_notes()
  cherry-pick: copy notes and run hooks
  cherry-pick: add --action-name option
  cherry-pick: remember rerere-autoupdate
  rebase: split the cherry-pick stuff
  rebase: cherry-pick: fix mode storage
  rebase: cherry-pick: fix sequence continuation
  rebase: cherry-pick: fix abort of cherry mode
  rebase: cherry-pick: fix command invocations
  rebase: cherry-pick: fix status messages
  rebase: cherry-pick: automatically commit stage
  rebase: cherry-pick: set correct action-name
  rebase: trivial cleanup
  rebase: use 'cherrypick' mode instead of 'am'
  rebase: cherry-pick: fix for shell prompt
  rebase: cherry-pick: add merge options
  rebase: remove merge mode
  rebase: cherry-pick: add copyright
  tests: fix autostash
  tests: update topology tests

Martin von Zweigbergk (7):
  add simple tests of consistency across rebase types
  add tests for rebasing with patch-equivalence present
  add tests for rebasing of empty commits
  add tests for rebasing root
  add tests for rebasing merged history
  t3406: modernize style
  tests: move test for rebase messages from t3400 to t3406

 .gitignore                             |   1 +
 Documentation/git-cherry-pick.txt      |  10 +-
 Documentation/git-revert.txt           |   7 +-
 Documentation/sequencer.txt            |   3 +
 Makefile                               |  31 +--
 builtin/commit.c                       |  46 +----
 builtin/revert.c                       |  17 ++
 builtin/rewrite.c                      | 124 ++++++++++++
 builtin/rewrite.h                      |  20 ++
 sequencer.c => builtin/sequencer.c     | 263 ++++++++++---------------
 sequencer.h => builtin/sequencer.h     |  12 +-
 contrib/completion/git-prompt.sh       |   4 +-
 git-rebase--am.sh                      |  12 +-
 git-rebase--cherrypick.sh              |  72 +++++++
 git-rebase--interactive.sh             |   4 +-
 git-rebase--merge.sh                   | 151 --------------
 git-rebase.sh                          |  16 +-
 log-tree.c                             | 161 ++++++++++++++-
 log-tree.h                             |   3 +
 read-cache.c                           |   4 +
 t/lib-rebase.sh                        |  33 ++++
 t/t3400-rebase.sh                      |  53 +----
 t/t3401-rebase-partial.sh              |  69 -------
 t/t3404-rebase-interactive.sh          |  10 +-
 t/t3406-rebase-message.sh              |  56 +++---
 t/t3407-rebase-abort.sh                |   2 +-
 t/t3409-rebase-preserve-merges.sh      |  53 -----
 t/t3420-rebase-autostash.sh            |   5 +-
 t/t3421-rebase-topology-linear.sh      | 350 +++++++++++++++++++++++++++++++++
 t/t3425-rebase-topology-merges.sh      | 255 ++++++++++++++++++++++++
 t/t3508-cherry-pick-many-commits.sh    |  13 ++
 t/t3510-cherry-pick-sequence.sh        |  14 +-
 t/t5520-pull.sh                        |   2 +-
 t/t9106-git-svn-commit-diff-clobber.sh |   2 +-
 t/t9903-bash-prompt.sh                 |   2 +-
 unpack-trees.c                         |   4 +-
 36 files changed, 1268 insertions(+), 616 deletions(-)
 create mode 100644 builtin/rewrite.c
 create mode 100644 builtin/rewrite.h
 rename sequencer.c => builtin/sequencer.c (86%)
 rename sequencer.h => builtin/sequencer.h (86%)
 create mode 100644 git-rebase--cherrypick.sh
 delete mode 100644 git-rebase--merge.sh
 delete mode 100755 t/t3401-rebase-partial.sh
 create mode 100755 t/t3421-rebase-topology-linear.sh
 create mode 100755 t/t3425-rebase-topology-merges.sh

-- 
1.8.3.698.g079b096

[PATCH v4 01/45] build: generate and clean test scripts

[Felipe Contreras]

Commit 416fda6 (build: do not install git-remote-testpy) made it so
git-remote-testpy is not only not installed, but also not generated by
default, let's make sure tests scripts (NO_INSTALL) are generated as
ell.

Comments-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 03524d0..51c812d 100644
--- a/Makefile
+++ b/Makefile
@@ -2232,6 +2232,7 @@ endif
 
 test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
 
+all:: $(NO_INSTALL)
 all:: $(TEST_PROGRAMS) $(test_bindir_programs)
 
 bin-wrappers/%: wrap-for-bin.sh
@@ -2482,7 +2483,7 @@ clean: profile-clean coverage-clean
 	$(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
 		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
-	$(RM) $(TEST_PROGRAMS)
+	$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
 	$(RM) -r bin-wrappers $(dep_dirs)
 	$(RM) -r po/build/
 	$(RM) *.spec *.pyc *.pyo */*.pyc */*.pyo common-cmds.h $(ETAGS_TARGET) tags cscope*
-- 
1.8.3.698.g079b096

[PATCH v4 02/45] build: do not install git-remote-testgit

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 51c812d..34f1240 100644
--- a/Makefile
+++ b/Makefile
@@ -491,6 +491,7 @@ SCRIPT_PERL += git-svn.perl
 SCRIPT_PYTHON += git-remote-testpy.py
 SCRIPT_PYTHON += git-p4.py
 
+NO_INSTALL += git-remote-testgit
 NO_INSTALL += git-remote-testpy
 
 # Generated files for scripts
-- 
1.8.3.698.g079b096

[PATCH v4 03/45] build: trivial cleanup

[Felipe Contreras]

There's no need to list again the prerequisites.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 34f1240..3b6cd5a 100644
--- a/Makefile
+++ b/Makefile
@@ -2068,13 +2068,13 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
 $(LIB_FILE): $(LIB_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(XDIFF_OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
 $(VCSSVN_LIB): $(VCSSVN_OBJS)
-	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(VCSSVN_OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
 export DEFAULT_EDITOR DEFAULT_PAGER
 
-- 
1.8.3.698.g079b096

[PATCH v4 04/45] build: add builtin lib

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index 3b6cd5a..d2af207 100644
--- a/Makefile
+++ b/Makefile
@@ -631,6 +631,7 @@ export PERL_PATH
 export PYTHON_PATH
 
 LIB_FILE = libgit.a
+BUILTIN_LIB = builtin/lib.a
 XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
@@ -1713,9 +1714,9 @@ git.sp git.s git.o: EXTRA_CPPFLAGS = \
 	'-DGIT_MAN_PATH="$(mandir_relative_SQ)"' \
 	'-DGIT_INFO_PATH="$(infodir_relative_SQ)"'
 
-git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
+git$X: git.o GIT-LDFLAGS $(BUILTIN_LIB) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ git.o \
-		$(BUILTIN_OBJS) $(ALL_LDFLAGS) $(LIBS)
+		$(ALL_LDFLAGS) $(BUILTIN_LIB) $(LIBS)
 
 help.sp help.s help.o: common-cmds.h
 
@@ -2070,6 +2071,9 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
+$(BUILTIN_LIB): $(BUILTIN_OBJS)
+	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
+
 $(XDIFF_LIB): $(XDIFF_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
@@ -2482,7 +2486,7 @@ profile-clean:
 
 clean: profile-clean coverage-clean
 	$(RM) *.o *.res block-sha1/*.o ppc/*.o compat/*.o compat/*/*.o xdiff/*.o vcs-svn/*.o \
-		builtin/*.o $(LIB_FILE) $(XDIFF_LIB) $(VCSSVN_LIB)
+		builtin/*.o $(LIB_FILE) $(BUILTIN_LIB) $(XDIFF_LIB) $(VCSSVN_LIB)
 	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) git$X
 	$(RM) $(TEST_PROGRAMS) $(NO_INSTALL)
 	$(RM) -r bin-wrappers $(dep_dirs)
-- 
1.8.3.698.g079b096

[PATCH v4 05/45] log-tree: remove dependency from sequencer

[Felipe Contreras]

Move the relevant code from sequencer to log-tree. This code is not
specific to sequencer, and this allows the sequencer to move out of
libgit.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 log-tree.c  | 161 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 log-tree.h  |   3 ++
 sequencer.c | 160 ++---------------------------------------------------------
 sequencer.h |   4 --
 4 files changed, 166 insertions(+), 162 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 2eb69bc..654f5db 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,10 +9,13 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
-#include "sequencer.h"
 #include "line-log.h"
 
+#define APPEND_SIGNOFF_DEDUP (1u << 0)
+
 struct decoration name_decoration = { "object names" };
+const char sign_off_header[] = "Signed-off-by: ";
+static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 enum decoration_type {
 	DECORATION_NONE = 0,
@@ -472,6 +475,162 @@ static void show_mergetag(struct rev_info *opt, struct commit *commit)
 	free_commit_extra_headers(to_free);
 }
 
+static int is_rfc2822_line(const char *buf, int len)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		int ch = buf[i];
+		if (ch == ':')
+			return 1;
+		if (!isalnum(ch) && ch != '-')
+			break;
+	}
+
+	return 0;
+}
+
+static int is_cherry_picked_from_line(const char *buf, int len)
+{
+	/*
+	 * We only care that it looks roughly like (cherry picked from ...)
+	 */
+	return len > strlen(cherry_picked_prefix) + 1 &&
+		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
+}
+
+/*
+ * Returns 0 for non-conforming footer
+ * Returns 1 for conforming footer
+ * Returns 2 when sob exists within conforming footer
+ * Returns 3 when sob exists within conforming footer as last entry
+ */
+int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
+	int ignore_footer)
+{
+	char prev;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	const char *buf = sb->buf;
+	int found_sob = 0;
+
+	/* footer must end with newline */
+	if (!len || buf[len - 1] != '\n')
+		return 0;
+
+	prev = '\0';
+	for (i = len - 1; i > 0; i--) {
+		char ch = buf[i];
+		if (prev == '\n' && ch == '\n') /* paragraph break */
+			break;
+		prev = ch;
+	}
+
+	/* require at least one blank line */
+	if (prev != '\n' || buf[i] != '\n')
+		return 0;
+
+	/* advance to start of last paragraph */
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		int found_rfc2822;
+
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
+		if (found_rfc2822 && sob &&
+		    !strncmp(buf + i, sob->buf, sob->len))
+			found_sob = k;
+
+		if (!(found_rfc2822 ||
+		      is_cherry_picked_from_line(buf + i, k - i - 1)))
+			return 0;
+	}
+	if (found_sob == i)
+		return 3;
+	if (found_sob)
+		return 2;
+	return 1;
+}
+
+void append_cherrypick(struct strbuf *msgbuf, struct object *obj)
+{
+	if (!has_conforming_footer(msgbuf, NULL, 0))
+		strbuf_addch(msgbuf, '\n');
+	strbuf_addstr(msgbuf, cherry_picked_prefix);
+	strbuf_addstr(msgbuf, sha1_to_hex(obj->sha1));
+	strbuf_addstr(msgbuf, ")\n");
+}
+
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
+{
+	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
+	struct strbuf sob = STRBUF_INIT;
+	int has_footer;
+
+	strbuf_addstr(&sob, sign_off_header);
+	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
+				getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sob, '\n');
+
+	/*
+	 * If the whole message buffer is equal to the sob, pretend that we
+	 * found a conforming footer with a matching sob
+	 */
+	if (msgbuf->len - ignore_footer == sob.len &&
+	    !strncmp(msgbuf->buf, sob.buf, sob.len))
+		has_footer = 3;
+	else
+		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
+
+	if (!has_footer) {
+		const char *append_newlines = NULL;
+		size_t len = msgbuf->len - ignore_footer;
+
+		if (!len) {
+			/*
+			 * The buffer is completely empty.  Leave foom for
+			 * the title and body to be filled in by the user.
+			 */
+			append_newlines = "\n\n";
+		} else if (msgbuf->buf[len - 1] != '\n') {
+			/*
+			 * Incomplete line.  Complete the line and add a
+			 * blank one so that there is an empty line between
+			 * the message body and the sob.
+			 */
+			append_newlines = "\n\n";
+		} else if (len == 1) {
+			/*
+			 * Buffer contains a single newline.  Add another
+			 * so that we leave room for the title and body.
+			 */
+			append_newlines = "\n";
+		} else if (msgbuf->buf[len - 2] != '\n') {
+			/*
+			 * Buffer ends with a single newline.  Add another
+			 * so that there is an empty line between the message
+			 * body and the sob.
+			 */
+			append_newlines = "\n";
+		} /* else, the buffer already ends with two newlines. */
+
+		if (append_newlines)
+			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				append_newlines, strlen(append_newlines));
+	}
+
+	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				sob.buf, sob.len);
+
+	strbuf_release(&sob);
+}
+
 void show_log(struct rev_info *opt)
 {
 	struct strbuf msgbuf = STRBUF_INIT;
diff --git a/log-tree.h b/log-tree.h
index d6ecd4d..1039e49 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -25,4 +25,7 @@ void load_ref_decorations(int flags);
 void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 
+void append_cherrypick(struct strbuf *msgbuf, struct object *obj);
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
+
 #endif
diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..e92e039 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -14,94 +14,10 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "log-tree.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
-const char sign_off_header[] = "Signed-off-by: ";
-static const char cherry_picked_prefix[] = "(cherry picked from commit ";
-
-static int is_rfc2822_line(const char *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		int ch = buf[i];
-		if (ch == ':')
-			return 1;
-		if (!isalnum(ch) && ch != '-')
-			break;
-	}
-
-	return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
-	/*
-	 * We only care that it looks roughly like (cherry picked from ...)
-	 */
-	return len > strlen(cherry_picked_prefix) + 1 &&
-		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
-/*
- * Returns 0 for non-conforming footer
- * Returns 1 for conforming footer
- * Returns 2 when sob exists within conforming footer
- * Returns 3 when sob exists within conforming footer as last entry
- */
-static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
-	int ignore_footer)
-{
-	char prev;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-	int found_sob = 0;
-
-	/* footer must end with newline */
-	if (!len || buf[len - 1] != '\n')
-		return 0;
-
-	prev = '\0';
-	for (i = len - 1; i > 0; i--) {
-		char ch = buf[i];
-		if (prev == '\n' && ch == '\n') /* paragraph break */
-			break;
-		prev = ch;
-	}
-
-	/* require at least one blank line */
-	if (prev != '\n' || buf[i] != '\n')
-		return 0;
-
-	/* advance to start of last paragraph */
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		int found_rfc2822;
-
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
-		if (found_rfc2822 && sob &&
-		    !strncmp(buf + i, sob->buf, sob->len))
-			found_sob = k;
-
-		if (!(found_rfc2822 ||
-		      is_cherry_picked_from_line(buf + i, k - i - 1)))
-			return 0;
-	}
-	if (found_sob == i)
-		return 3;
-	if (found_sob)
-		return 2;
-	return 1;
-}
-
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -578,13 +494,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 			strbuf_addstr(&msgbuf, p);
 		}
 
-		if (opts->record_origin) {
-			if (!has_conforming_footer(&msgbuf, NULL, 0))
-				strbuf_addch(&msgbuf, '\n');
-			strbuf_addstr(&msgbuf, cherry_picked_prefix);
-			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
-			strbuf_addstr(&msgbuf, ")\n");
-		}
+		if (opts->record_origin)
+			append_cherrypick(&msgbuf, &commit->object);
 	}
 
 	if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REPLAY_REVERT) {
@@ -1123,68 +1034,3 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	save_opts(opts);
 	return pick_commits(todo_list, opts);
 }
-
-void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
-{
-	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
-	struct strbuf sob = STRBUF_INIT;
-	int has_footer;
-
-	strbuf_addstr(&sob, sign_off_header);
-	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
-				getenv("GIT_COMMITTER_EMAIL")));
-	strbuf_addch(&sob, '\n');
-
-	/*
-	 * If the whole message buffer is equal to the sob, pretend that we
-	 * found a conforming footer with a matching sob
-	 */
-	if (msgbuf->len - ignore_footer == sob.len &&
-	    !strncmp(msgbuf->buf, sob.buf, sob.len))
-		has_footer = 3;
-	else
-		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
-
-	if (!has_footer) {
-		const char *append_newlines = NULL;
-		size_t len = msgbuf->len - ignore_footer;
-
-		if (!len) {
-			/*
-			 * The buffer is completely empty.  Leave foom for
-			 * the title and body to be filled in by the user.
-			 */
-			append_newlines = "\n\n";
-		} else if (msgbuf->buf[len - 1] != '\n') {
-			/*
-			 * Incomplete line.  Complete the line and add a
-			 * blank one so that there is an empty line between
-			 * the message body and the sob.
-			 */
-			append_newlines = "\n\n";
-		} else if (len == 1) {
-			/*
-			 * Buffer contains a single newline.  Add another
-			 * so that we leave room for the title and body.
-			 */
-			append_newlines = "\n";
-		} else if (msgbuf->buf[len - 2] != '\n') {
-			/*
-			 * Buffer ends with a single newline.  Add another
-			 * so that there is an empty line between the message
-			 * body and the sob.
-			 */
-			append_newlines = "\n";
-		} /* else, the buffer already ends with two newlines. */
-
-		if (append_newlines)
-			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-				append_newlines, strlen(append_newlines));
-	}
-
-	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
-		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-				sob.buf, sob.len);
-
-	strbuf_release(&sob);
-}
diff --git a/sequencer.h b/sequencer.h
index 1fc22dc..c341918 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -6,8 +6,6 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
-#define APPEND_SIGNOFF_DEDUP (1u << 0)
-
 enum replay_action {
 	REPLAY_REVERT,
 	REPLAY_PICK
@@ -50,6 +48,4 @@ int sequencer_pick_revisions(struct replay_opts *opts);
 
 extern const char sign_off_header[];
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
-
 #endif
-- 
1.8.3.698.g079b096

[PATCH v4 06/45] Move sequencer to builtin

[Felipe Contreras]

This code is only useful for cherry-pick and revert built-ins, nothing
else, so let's make it a builtin object.

The first source file that doesn't generate a git-foo builtin, but does
go into the builtin library. Hopefully the first of many to clean
libgit.a.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile                           | 10 ++++++----
 sequencer.c => builtin/sequencer.c |  0
 sequencer.h => builtin/sequencer.h |  0
 3 files changed, 6 insertions(+), 4 deletions(-)
 rename sequencer.c => builtin/sequencer.c (100%)
 rename sequencer.h => builtin/sequencer.h (100%)

diff --git a/Makefile b/Makefile
index d2af207..4c7bb88 100644
--- a/Makefile
+++ b/Makefile
@@ -716,7 +716,6 @@ LIB_H += resolve-undo.h
 LIB_H += revision.h
 LIB_H += run-command.h
 LIB_H += send-pack.h
-LIB_H += sequencer.h
 LIB_H += sha1-array.h
 LIB_H += sha1-lookup.h
 LIB_H += shortlog.h
@@ -858,7 +857,6 @@ LIB_OBJS += resolve-undo.o
 LIB_OBJS += revision.o
 LIB_OBJS += run-command.o
 LIB_OBJS += send-pack.o
-LIB_OBJS += sequencer.o
 LIB_OBJS += server-info.o
 LIB_OBJS += setup.o
 LIB_OBJS += sha1-array.o
@@ -992,6 +990,9 @@ BUILTIN_OBJS += builtin/verify-pack.o
 BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
 
+BUILTIN_LIB_OBJS += builtin/sequencer.o
+BUILTIN_LIB_OBJS += $(BUILTIN_OBJS)
+
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
 EXTLIBS =
 
@@ -1894,7 +1895,8 @@ VCSSVN_OBJS += vcs-svn/svndiff.o
 VCSSVN_OBJS += vcs-svn/svndump.o
 
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
-OBJECTS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
+OBJECTS := $(LIB_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
+	$(BUILTIN_LIB_OBJS) \
 	$(XDIFF_OBJS) \
 	$(VCSSVN_OBJS) \
 	git.o
@@ -2071,7 +2073,7 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o GIT-LDFLAGS $(GITLIBS
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
-$(BUILTIN_LIB): $(BUILTIN_OBJS)
+$(BUILTIN_LIB): $(BUILTIN_LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $^
 
 $(XDIFF_LIB): $(XDIFF_OBJS)
diff --git a/sequencer.c b/builtin/sequencer.c
similarity index 100%
rename from sequencer.c
rename to builtin/sequencer.c
diff --git a/sequencer.h b/builtin/sequencer.h
similarity index 100%
rename from sequencer.h
rename to builtin/sequencer.h
-- 
1.8.3.698.g079b096

[PATCH v4 07/45] unpack-trees: plug a memory leak

[Felipe Contreras]

Before overwriting the destination index, first let's discard its
contents.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 unpack-trees.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 57b4074..abe2576 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1166,8 +1166,10 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options
 
 	o->src_index = NULL;
 	ret = check_updates(o) ? (-2) : 0;
-	if (o->dst_index)
+	if (o->dst_index) {
+		discard_index(o->dst_index);
 		*o->dst_index = o->result;
+	}
 
 done:
 	clear_exclude_list(&el);
-- 
1.8.3.698.g079b096

[PATCH v4 08/45] read-cache: plug a few leaks

[Felipe Contreras]

We are not freeing 'istate->cache' properly.

We can't rely on 'initialized' to keep track of the 'istate->cache',
because it doesn't really mean it's initialized. So assume it always has
data, and free it before overwriting it.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 read-cache.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index 5e30746..a1dd04d 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1451,6 +1451,7 @@ int read_index_from(struct index_state *istate, const char *path)
 	istate->version = ntohl(hdr->hdr_version);
 	istate->cache_nr = ntohl(hdr->hdr_entries);
 	istate->cache_alloc = alloc_nr(istate->cache_nr);
+	free(istate->cache);
 	istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
 	istate->initialized = 1;
 
@@ -1512,6 +1513,9 @@ int discard_index(struct index_state *istate)
 
 	for (i = 0; i < istate->cache_nr; i++)
 		free(istate->cache[i]);
+	free(istate->cache);
+	istate->cache = NULL;
+	istate->cache_alloc = 0;
 	resolve_undo_clear_index(istate);
 	istate->cache_nr = 0;
 	istate->cache_changed = 0;
-- 
1.8.3.698.g079b096

[PATCH v4 09/45] sequencer: remove useless indentation

[Felipe Contreras]

By using good ol' goto.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index e92e039..b2c8c94 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -390,7 +390,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
-	int res, unborn = 0;
+	int res, unborn = 0, allow;
 
 	if (opts->no_commit) {
 		/*
@@ -535,14 +535,16 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		      msg.subject);
 		print_advice(res == 1, opts);
 		rerere(opts->allow_rerere_auto);
-	} else {
-		int allow = allow_empty(opts, commit);
+		goto leave;
+	}
+
+	allow = allow_empty(opts, commit);
 	if (allow < 0)
 		return allow;
 	if (!opts->no_commit)
 		res = run_git_commit(defmsg, opts, allow);
-	}
 
+leave:
 	free_message(&msg);
 	free(defmsg);
 
-- 
1.8.3.698.g079b096

[PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

We should free objects before leaving.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index b2c8c94..23b01b7 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -539,8 +539,10 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}
 
 	allow = allow_empty(opts, commit);
-	if (allow < 0)
-		return allow;
+	if (allow < 0) {
+		res = allow;
+		goto leave;
+	}
 	if (!opts->no_commit)
 		res = run_git_commit(defmsg, opts, allow);
 
-- 
1.8.3.698.g079b096

[PATCH v4 11/45] cherry-pick: don't barf when there's nothing to do

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c             | 4 ++--
 t/t3510-cherry-pick-sequence.sh | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 23b01b7..4d7dc8b 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -565,8 +565,8 @@ static void prepare_revs(struct replay_opts *opts)
 	if (prepare_revision_walk(opts->revs))
 		die(_("revision walk setup failed"));
 
-	if (!opts->revs->commits)
-		die(_("empty commit set passed"));
+	if (!opts->revs->commits && !opts->quiet)
+		error(_("empty commit set passed"));
 }
 
 static void read_and_refresh_cache(struct replay_opts *opts)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89d..33c5512 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -472,7 +472,7 @@ test_expect_success 'malformed instruction sheet 2' '
 
 test_expect_success 'empty commit set' '
 	pristine_detach initial &&
-	test_expect_code 128 git cherry-pick base..base
+	git cherry-pick base..base
 '
 
 test_expect_success 'malformed instruction sheet 3' '
-- 
1.8.3.698.g079b096

[PATCH v4 12/45] cherry-pick: add --skip-empty option

[Felipe Contreras]

Pretty much what it says on the tin.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt   |  3 +++
 builtin/revert.c                    |  8 ++++++++
 builtin/sequencer.c                 |  6 ++++++
 builtin/sequencer.h                 |  1 +
 t/t3508-cherry-pick-many-commits.sh | 13 +++++++++++++
 5 files changed, 31 insertions(+)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index c205d23..fccd936 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -129,6 +129,9 @@ effect to your index in a row.
 	redundant commits are ignored.  This option overrides that behavior and
 	creates an empty commit object.  Implies `--allow-empty`.
 
+--skip-empty::
+	Instead of failing, skip commits that are or become empty.
+
 --strategy=<strategy>::
 	Use the given merge strategy.  Should only be used once.
 	See the MERGE STRATEGIES section in linkgit:git-merge[1]
diff --git a/builtin/revert.c b/builtin/revert.c
index 0401fdb..5a8453d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -118,6 +118,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
@@ -127,6 +128,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOLEAN(0, "allow-empty", &opts->allow_empty, N_("preserve initially empty commits")),
 			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
+			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
@@ -144,6 +146,12 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 				"--abort", rollback,
 				NULL);
 
+	verify_opt_mutually_compatible(me,
+				"--allow-empty", opts->allow_empty,
+				"--skip-empty", opts->skip_empty,
+				"--keep-redundant-commits", opts->keep_redundant_commits,
+				NULL);
+
 	/* implies allow_empty */
 	if (opts->keep_redundant_commits)
 		opts->allow_empty = 1;
diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 4d7dc8b..56551bb 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -538,6 +538,12 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		goto leave;
 	}
 
+	if (opts->skip_empty && is_index_unchanged() == 1) {
+		warning(_("skipping %s... %s"),
+			find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
+			msg.subject);
+		goto leave;
+	}
 	allow = allow_empty(opts, commit);
 	if (allow < 0) {
 		res = allow;
diff --git a/builtin/sequencer.h b/builtin/sequencer.h
index c341918..86e2eee 100644
--- a/builtin/sequencer.h
+++ b/builtin/sequencer.h
@@ -32,6 +32,7 @@ struct replay_opts {
 	int allow_empty;
 	int allow_empty_message;
 	int keep_redundant_commits;
+	int skip_empty;
 
 	int mainline;
 
diff --git a/t/t3508-cherry-pick-many-commits.sh b/t/t3508-cherry-pick-many-commits.sh
index 19c99d7..3dc19c6 100755
--- a/t/t3508-cherry-pick-many-commits.sh
+++ b/t/t3508-cherry-pick-many-commits.sh
@@ -187,4 +187,17 @@ test_expect_success 'cherry-pick --stdin works' '
 	check_head_differs_from fourth
 '
 
+test_expect_success 'cherry-pick skip empty' '
+	git clean -fxd &&
+	git checkout -b empty fourth &&
+	git commit --allow-empty -m empty &&
+	test_commit ontop &&
+	git checkout -f master &&
+	git reset --hard fourth &&
+	git cherry-pick --skip-empty fourth..empty &&
+	echo ontop > expected &&
+	git log --format=%s fourth..HEAD > actual
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.3.698.g079b096

[PATCH v4 13/45] revert/cherry-pick: add --quiet option

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt | 6 +++++-
 Documentation/git-revert.txt      | 6 +++++-
 builtin/revert.c                  | 1 +
 builtin/sequencer.c               | 3 +++
 builtin/sequencer.h               | 1 +
 5 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index fccd936..da0bd81 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -8,7 +8,7 @@ git-cherry-pick - Apply the changes introduced by some existing commits
 SYNOPSIS
 --------
 [verse]
-'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
+'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --continue
 'git cherry-pick' --quit
 'git cherry-pick' --abort
@@ -51,6 +51,10 @@ OPTIONS
 	feed all <commit>... arguments to a single revision walk
 	(see a later example that uses 'maint master..next').
 
+-q::
+--quiet::
+	Quiet, suppress feedback messages.
+
 -e::
 --edit::
 	With this option, 'git cherry-pick' will let you edit the commit
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index f79c9d8..98a8e7a 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -8,7 +8,7 @@ git-revert - Revert some existing commits
 SYNOPSIS
 --------
 [verse]
-'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
+'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --continue
 'git revert' --quit
 'git revert' --abort
@@ -40,6 +40,10 @@ OPTIONS
 	default, see linkgit:git-rev-list[1] and its '--no-walk'
 	option.
 
+-q::
+--quiet::
+	Quiet, suppress feedback messages.
+
 -e::
 --edit::
 	With this option, 'git revert' will let you edit the commit
diff --git a/builtin/revert.c b/builtin/revert.c
index 5a8453d..ec83748 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -100,6 +100,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	int contin = 0;
 	int rollback = 0;
 	struct option options[] = {
+		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
 		OPT_BOOLEAN(0, "quit", &remove_state, N_("end revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "continue", &contin, N_("resume revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "abort", &rollback, N_("cancel revert or cherry-pick sequence")),
diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 56551bb..0f50942 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -305,6 +305,8 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
 	argv_array_init(&array);
 	argv_array_push(&array, "commit");
 	argv_array_push(&array, "-n");
+	if (opts->quiet)
+		argv_array_push(&array, "-q");
 
 	if (opts->signoff)
 		argv_array_push(&array, "-s");
@@ -539,6 +541,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	}
 
 	if (opts->skip_empty && is_index_unchanged() == 1) {
+		if (!opts->quiet)
 			warning(_("skipping %s... %s"),
 				find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
 				msg.subject);
diff --git a/builtin/sequencer.h b/builtin/sequencer.h
index 86e2eee..e45411c 100644
--- a/builtin/sequencer.h
+++ b/builtin/sequencer.h
@@ -33,6 +33,7 @@ struct replay_opts {
 	int allow_empty_message;
 	int keep_redundant_commits;
 	int skip_empty;
+	int quiet;
 
 	int mainline;
 
-- 
1.8.3.698.g079b096

[PATCH v4 14/45] revert/cherry-pick: add --skip option

[Felipe Contreras]

Akin to 'am --skip' and 'rebase --skip'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-cherry-pick.txt |  1 +
 Documentation/git-revert.txt      |  1 +
 Documentation/sequencer.txt       |  3 +++
 builtin/revert.c                  |  6 ++++++
 builtin/sequencer.c               | 24 ++++++++++++++++++++++++
 builtin/sequencer.h               |  3 ++-
 t/t3510-cherry-pick-sequence.sh   | 12 ++++++++++++
 7 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index da0bd81..d95c63c 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git cherry-pick' [-q] [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
 'git cherry-pick' --continue
+'git cherry-pick' --skip
 'git cherry-pick' --quit
 'git cherry-pick' --abort
 
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 98a8e7a..52e146e 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -10,6 +10,7 @@ SYNOPSIS
 [verse]
 'git revert' [-q] [--[no-]edit] [-n] [-m parent-number] [-s] <commit>...
 'git revert' --continue
+'git revert' --skip
 'git revert' --quit
 'git revert' --abort
 
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 5747f44..df2d355 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -3,6 +3,9 @@
 	'.git/sequencer'.  Can be used to continue after resolving
 	conflicts in a failed cherry-pick or revert.
 
+--skip::
+	Skip the current commit, and then continue.
+
 --quit::
 	Forget about the current operation in progress.  Can be used
 	to clear the sequencer state after a failed cherry-pick or
diff --git a/builtin/revert.c b/builtin/revert.c
index ec83748..d3d5600 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -99,11 +99,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 	int remove_state = 0;
 	int contin = 0;
 	int rollback = 0;
+	int skip = 0;
 	struct option options[] = {
 		OPT__QUIET(&opts->quiet, N_("suppress progress reporting")),
 		OPT_BOOLEAN(0, "quit", &remove_state, N_("end revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "continue", &contin, N_("resume revert or cherry-pick sequence")),
 		OPT_BOOLEAN(0, "abort", &rollback, N_("cancel revert or cherry-pick sequence")),
+		OPT_BOOLEAN(0, "skip", &skip, N_("skip current commit in the sequence")),
 		OPT_BOOLEAN('n', "no-commit", &opts->no_commit, N_("don't automatically commit")),
 		OPT_BOOLEAN('e', "edit", &opts->edit, N_("edit the commit message")),
 		OPT_NOOP_NOARG('r', NULL),
@@ -164,6 +166,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		opts->subcommand = REPLAY_CONTINUE;
 	else if (rollback)
 		opts->subcommand = REPLAY_ROLLBACK;
+	else if (skip)
+		opts->subcommand = REPLAY_SKIP;
 	else
 		opts->subcommand = REPLAY_NONE;
 
@@ -174,6 +178,8 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			this_operation = "--quit";
 		else if (opts->subcommand == REPLAY_CONTINUE)
 			this_operation = "--continue";
+		else if (opts->subcommand == REPLAY_SKIP)
+			this_operation = "--skip";
 		else {
 			assert(opts->subcommand == REPLAY_ROLLBACK);
 			this_operation = "--abort";
diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 0f50942..2b1b30a 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -961,6 +961,28 @@ static int sequencer_continue(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
+static int sequencer_skip(struct replay_opts *opts)
+{
+	const char *argv[4]; /* reset --hard HEAD + NULL */
+	struct string_list merge_rr = STRING_LIST_INIT_DUP;
+	int ret;
+
+	if (setup_rerere(&merge_rr, 0) >= 0) {
+		rerere_clear(&merge_rr);
+		string_list_clear(&merge_rr, 1);
+	}
+
+	argv[0] = "reset";
+	argv[1] = "--hard";
+	argv[2] = "HEAD";
+	argv[3] = NULL;
+	ret = run_command_v_opt(argv, RUN_GIT_CMD);
+	if (ret)
+		return ret;
+
+	return sequencer_continue(opts);
+}
+
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
@@ -991,6 +1013,8 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 		return sequencer_rollback(opts);
 	if (opts->subcommand == REPLAY_CONTINUE)
 		return sequencer_continue(opts);
+	if (opts->subcommand == REPLAY_SKIP)
+		return sequencer_skip(opts);
 
 	for (i = 0; i < opts->revs->pending.nr; i++) {
 		unsigned char sha1[20];
diff --git a/builtin/sequencer.h b/builtin/sequencer.h
index e45411c..e69495a 100644
--- a/builtin/sequencer.h
+++ b/builtin/sequencer.h
@@ -15,7 +15,8 @@ enum replay_subcommand {
 	REPLAY_NONE,
 	REPLAY_REMOVE_STATE,
 	REPLAY_CONTINUE,
-	REPLAY_ROLLBACK
+	REPLAY_ROLLBACK,
+	REPLAY_SKIP
 };
 
 struct replay_opts {
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 33c5512..c43c327 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -511,4 +511,16 @@ test_expect_success 'commit descriptions in insn sheet are optional' '
 	test_line_count = 4 commits
 '
 
+test_expect_success 'skip' '
+	pristine_detach conflicting &&
+	test_must_fail git cherry-pick initial..picked &&
+
+	git checkout HEAD -- unrelated &&
+	test_must_fail git cherry-pick --continue &&
+	git cherry-pick --skip &&
+
+	git rev-list initial..HEAD >commits &&
+	test_line_count = 3 commits
+'
+
 test_done
-- 
1.8.3.698.g079b096

[PATCH v4 15/45] builtin: add rewrite helper

[Felipe Contreras]

So that we can load and store rewrites, as well as other operations on a
list of rewritten commits.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile          |  1 +
 builtin/rewrite.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 builtin/rewrite.h | 18 ++++++++++++++
 3 files changed, 93 insertions(+)
 create mode 100644 builtin/rewrite.c
 create mode 100644 builtin/rewrite.h

diff --git a/Makefile b/Makefile
index 4c7bb88..a167e68 100644
--- a/Makefile
+++ b/Makefile
@@ -991,6 +991,7 @@ BUILTIN_OBJS += builtin/verify-tag.o
 BUILTIN_OBJS += builtin/write-tree.o
 
 BUILTIN_LIB_OBJS += builtin/sequencer.o
+BUILTIN_LIB_OBJS += builtin/rewrite.o
 BUILTIN_LIB_OBJS += $(BUILTIN_OBJS)
 
 GITLIBS = $(LIB_FILE) $(XDIFF_LIB)
diff --git a/builtin/rewrite.c b/builtin/rewrite.c
new file mode 100644
index 0000000..2519352
--- /dev/null
+++ b/builtin/rewrite.c
@@ -0,0 +1,74 @@
+#include "cache.h"
+#include "rewrite.h"
+
+void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *to)
+{
+	struct rewritten_item *item;
+	if (list->nr + 1 >= list->alloc) {
+		list->alloc += 32;
+		list->items = xrealloc(list->items, list->alloc * sizeof(*list->items));
+	}
+	item = &list->items[list->nr];
+	hashcpy(item->from, from);
+	hashcpy(item->to, to);
+	list->nr++;
+}
+
+int store_rewritten(struct rewritten *list, const char *file)
+{
+	static struct lock_file lock;
+	struct strbuf buf = STRBUF_INIT;
+	int fd, i, ret = 0;
+
+	fd = hold_lock_file_for_update(&lock, file, LOCK_DIE_ON_ERROR);
+	for (i = 0; i < list->nr; i++) {
+		struct rewritten_item *item = &list->items[i];
+		strbuf_addf(&buf, "%s %s\n", sha1_to_hex(item->from), sha1_to_hex(item->to));
+	}
+	if (write_in_full(fd, buf.buf, buf.len) < 0) {
+		error(_("Could not write to %s"), file);
+		ret = 1;
+		goto leave;
+	}
+	if (commit_lock_file(&lock) < 0) {
+		error(_("Error wrapping up %s."), file);
+		ret = 1;
+		goto leave;
+	}
+leave:
+	strbuf_release(&buf);
+	return ret;
+}
+
+void load_rewritten(struct rewritten *list, const char *file)
+{
+	struct strbuf buf = STRBUF_INIT;
+	char *p;
+	int fd;
+
+	fd = open(file, O_RDONLY);
+	if (fd < 0)
+		return;
+	if (strbuf_read(&buf, fd, 0) < 0) {
+		close(fd);
+		strbuf_release(&buf);
+		return;
+	}
+	close(fd);
+
+	for (p = buf.buf; *p;) {
+		unsigned char from[20];
+		unsigned char to[20];
+		char *eol = strchrnul(p, '\n');
+		if (eol - p != 81)
+			/* wrong size */
+			break;
+		if (get_sha1_hex(p, from))
+			break;
+		if (get_sha1_hex(p + 41, to))
+			break;
+		add_rewritten(list, from, to);
+		p = *eol ? eol + 1 : eol;
+	}
+	strbuf_release(&buf);
+}
diff --git a/builtin/rewrite.h b/builtin/rewrite.h
new file mode 100644
index 0000000..09e7222
--- /dev/null
+++ b/builtin/rewrite.h
@@ -0,0 +1,18 @@
+#ifndef REWRITE_H
+#define REWRITE_H
+
+struct rewritten_item {
+	unsigned char from[20];
+	unsigned char to[20];
+};
+
+struct rewritten {
+	struct rewritten_item *items;
+	unsigned int nr, alloc;
+};
+
+void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *to);
+int store_rewritten(struct rewritten *list, const char *file);
+void load_rewritten(struct rewritten *list, const char *file);
+
+#endif
-- 
1.8.3.698.g079b096

[PATCH v4 16/45] cherry-pick: store rewritten commits

[Felipe Contreras]

Will be useful for the next commits.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c | 23 ++++++++++++++++++++++-
 builtin/sequencer.h |  1 +
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 2b1b30a..24034f2 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -15,9 +15,12 @@
 #include "refs.h"
 #include "argv-array.h"
 #include "log-tree.h"
+#include "rewrite.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
+static struct rewritten rewritten;
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -555,6 +558,14 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 	if (!opts->no_commit)
 		res = run_git_commit(defmsg, opts, allow);
 
+	if (!res && opts->action == REPLAY_PICK) {
+		unsigned char to[20];
+
+		if (read_ref("HEAD", to))
+			goto leave;
+
+		add_rewritten(&rewritten, commit->object.sha1, to);
+	}
 leave:
 	free_message(&msg);
 	free(defmsg);
@@ -917,9 +928,12 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 	for (cur = todo_list; cur; cur = cur->next) {
 		save_todo(cur, opts);
 		res = do_pick_commit(cur->item, opts);
-		if (res)
+		if (res) {
+			if (opts->action == REPLAY_PICK)
+				store_rewritten(&rewritten, git_path(SEQ_REWR_FILE));
 			return res;
 		}
+	}
 
 	/*
 	 * Sequence of picks finished successfully; cleanup by
@@ -947,6 +961,8 @@ static int sequencer_continue(struct replay_opts *opts)
 		return continue_single_pick();
 	read_populate_opts(&opts);
 	read_populate_todo(&todo_list, opts);
+	if (opts->action == REPLAY_PICK)
+		load_rewritten(&rewritten, git_path(SEQ_REWR_FILE));
 
 	/* Verify that the conflict has been resolved */
 	if (file_exists(git_path("CHERRY_PICK_HEAD")) ||
@@ -957,6 +973,11 @@ static int sequencer_continue(struct replay_opts *opts)
 	}
 	if (index_differs_from("HEAD", 0))
 		return error_dirty_index(opts);
+	if (opts->action == REPLAY_PICK) {
+		unsigned char to[20];
+		if (!read_ref("HEAD", to))
+			add_rewritten(&rewritten, todo_list->item->object.sha1, to);
+	}
 	todo_list = todo_list->next;
 	return pick_commits(todo_list, opts);
 }
diff --git a/builtin/sequencer.h b/builtin/sequencer.h
index e69495a..63ba274 100644
--- a/builtin/sequencer.h
+++ b/builtin/sequencer.h
@@ -5,6 +5,7 @@
 #define SEQ_HEAD_FILE	"sequencer/head"
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
+#define SEQ_REWR_FILE	"sequencer/rewritten"
 
 enum replay_action {
 	REPLAY_REVERT,
-- 
1.8.3.698.g079b096

[PATCH v4 17/45] cherry-pick: don't store skipped commit

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index 24034f2..d40fda9 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -953,7 +953,7 @@ static int continue_single_pick(void)
 	return run_command_v_opt(argv, RUN_GIT_CMD);
 }
 
-static int sequencer_continue(struct replay_opts *opts)
+static int sequencer_continue(struct replay_opts *opts, int skip)
 {
 	struct commit_list *todo_list = NULL;
 
@@ -973,7 +973,7 @@ static int sequencer_continue(struct replay_opts *opts)
 	}
 	if (index_differs_from("HEAD", 0))
 		return error_dirty_index(opts);
-	if (opts->action == REPLAY_PICK) {
+	if (opts->action == REPLAY_PICK && !skip) {
 		unsigned char to[20];
 		if (!read_ref("HEAD", to))
 			add_rewritten(&rewritten, todo_list->item->object.sha1, to);
@@ -1001,7 +1001,7 @@ static int sequencer_skip(struct replay_opts *opts)
 	if (ret)
 		return ret;
 
-	return sequencer_continue(opts);
+	return sequencer_continue(opts, 1);
 }
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
@@ -1033,7 +1033,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	if (opts->subcommand == REPLAY_ROLLBACK)
 		return sequencer_rollback(opts);
 	if (opts->subcommand == REPLAY_CONTINUE)
-		return sequencer_continue(opts);
+		return sequencer_continue(opts, 0);
 	if (opts->subcommand == REPLAY_SKIP)
 		return sequencer_skip(opts);
 
-- 
1.8.3.698.g079b096

[PATCH v4 18/45] builtin: move run_rewrite_hook() to rewrite.c

[Felipe Contreras]

And use struct rewrite.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/commit.c  | 38 +++++---------------------------------
 builtin/rewrite.c | 32 ++++++++++++++++++++++++++++++++
 builtin/rewrite.h |  1 +
 3 files changed, 38 insertions(+), 33 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1621dfc..56dab4f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -29,6 +29,7 @@
 #include "gpg-interface.h"
 #include "column.h"
 #include "sequencer.h"
+#include "rewrite.h"
 
 static const char * const builtin_commit_usage[] = {
 	N_("git commit [options] [--] <pathspec>..."),
@@ -1323,38 +1324,6 @@ static int git_commit_config(const char *k, const char *v, void *cb)
 	return git_status_config(k, v, s);
 }
 
-static int run_rewrite_hook(const unsigned char *oldsha1,
-			    const unsigned char *newsha1)
-{
-	/* oldsha1 SP newsha1 LF NUL */
-	static char buf[2*40 + 3];
-	struct child_process proc;
-	const char *argv[3];
-	int code;
-	size_t n;
-
-	argv[0] = find_hook("post-rewrite");
-	if (!argv[0])
-		return 0;
-
-	argv[1] = "amend";
-	argv[2] = NULL;
-
-	memset(&proc, 0, sizeof(proc));
-	proc.argv = argv;
-	proc.in = -1;
-	proc.stdout_to_stderr = 1;
-
-	code = start_command(&proc);
-	if (code)
-		return code;
-	n = snprintf(buf, sizeof(buf), "%s %s\n",
-		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
-	write_in_full(proc.in, buf, n);
-	close(proc.in);
-	return finish_command(&proc);
-}
-
 int cmd_commit(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
@@ -1589,13 +1558,16 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
 		struct notes_rewrite_cfg *cfg;
+		struct rewritten rewrite;
+		memset(&rewrite, 0, sizeof(rewrite));
 		cfg = init_copy_notes_for_rewrite("amend");
 		if (cfg) {
 			/* we are amending, so current_head is not NULL */
 			copy_note_for_rewrite(cfg, current_head->object.sha1, sha1);
 			finish_copy_notes_for_rewrite(cfg);
 		}
-		run_rewrite_hook(current_head->object.sha1, sha1);
+		add_rewritten(&rewrite, current_head->object.sha1, sha1);
+		run_rewrite_hook(&rewrite, "amend");
 	}
 	if (!quiet)
 		print_summary(prefix, sha1, !current_head);
diff --git a/builtin/rewrite.c b/builtin/rewrite.c
index 2519352..15fcd1a 100644
--- a/builtin/rewrite.c
+++ b/builtin/rewrite.c
@@ -1,5 +1,6 @@
 #include "cache.h"
 #include "rewrite.h"
+#include "run-command.h"
 
 void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *to)
 {
@@ -72,3 +73,34 @@ void load_rewritten(struct rewritten *list, const char *file)
 	}
 	strbuf_release(&buf);
 }
+
+int run_rewrite_hook(struct rewritten *list, const char *name)
+{
+	struct strbuf buf = STRBUF_INIT;
+	struct child_process proc;
+	const char *argv[3];
+	int code, i;
+
+	argv[0] = find_hook("post-rewrite");
+	if (!argv[0])
+		return 0;
+
+	argv[1] = name;
+	argv[2] = NULL;
+
+	memset(&proc, 0, sizeof(proc));
+	proc.argv = argv;
+	proc.in = -1;
+	proc.stdout_to_stderr = 1;
+
+	code = start_command(&proc);
+	if (code)
+		return code;
+	for (i = 0; i < list->nr; i++) {
+		struct rewritten_item *item = &list->items[i];
+		strbuf_addf(&buf, "%s %s\n", sha1_to_hex(item->from), sha1_to_hex(item->to));
+	}
+	write_in_full(proc.in, buf.buf, buf.len);
+	close(proc.in);
+	return finish_command(&proc);
+}
diff --git a/builtin/rewrite.h b/builtin/rewrite.h
index 09e7222..fd00e66 100644
--- a/builtin/rewrite.h
+++ b/builtin/rewrite.h
@@ -14,5 +14,6 @@ struct rewritten {
 void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *to);
 int store_rewritten(struct rewritten *list, const char *file);
 void load_rewritten(struct rewritten *list, const char *file);
+int run_rewrite_hook(struct rewritten *list, const char *name);
 
 #endif
-- 
1.8.3.698.g079b096

[PATCH v4 19/45] builtin: add copy_rewrite_notes()

[Felipe Contreras]

And use it on commit.c.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/commit.c  |  8 +-------
 builtin/rewrite.c | 17 +++++++++++++++++
 builtin/rewrite.h |  1 +
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 56dab4f..4f35794 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1557,16 +1557,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	rerere(0);
 	run_hook(get_index_file(), "post-commit", NULL);
 	if (amend && !no_post_rewrite) {
-		struct notes_rewrite_cfg *cfg;
 		struct rewritten rewrite;
 		memset(&rewrite, 0, sizeof(rewrite));
-		cfg = init_copy_notes_for_rewrite("amend");
-		if (cfg) {
-			/* we are amending, so current_head is not NULL */
-			copy_note_for_rewrite(cfg, current_head->object.sha1, sha1);
-			finish_copy_notes_for_rewrite(cfg);
-		}
 		add_rewritten(&rewrite, current_head->object.sha1, sha1);
+		copy_rewrite_notes(&rewrite, "amend");
 		run_rewrite_hook(&rewrite, "amend");
 	}
 	if (!quiet)
diff --git a/builtin/rewrite.c b/builtin/rewrite.c
index 15fcd1a..02e0ea9 100644
--- a/builtin/rewrite.c
+++ b/builtin/rewrite.c
@@ -104,3 +104,20 @@ int run_rewrite_hook(struct rewritten *list, const char *name)
 	close(proc.in);
 	return finish_command(&proc);
 }
+
+void copy_rewrite_notes(struct rewritten *list, const char *name)
+{
+	struct notes_rewrite_cfg *cfg;
+	int i;
+
+	cfg = init_copy_notes_for_rewrite(name);
+	if (!cfg)
+		return;
+
+	for (i = 0; i < list->nr; i++) {
+		struct rewritten_item *item = &list->items[i];
+		copy_note_for_rewrite(cfg, item->from, item->to);
+	}
+
+	finish_copy_notes_for_rewrite(cfg);
+}
diff --git a/builtin/rewrite.h b/builtin/rewrite.h
index fd00e66..38b2caf 100644
--- a/builtin/rewrite.h
+++ b/builtin/rewrite.h
@@ -15,5 +15,6 @@ void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *t
 int store_rewritten(struct rewritten *list, const char *file);
 void load_rewritten(struct rewritten *list, const char *file);
 int run_rewrite_hook(struct rewritten *list, const char *name);
+void copy_rewrite_notes(struct rewritten *list, const char *name);
 
 #endif
-- 
1.8.3.698.g079b096

[PATCH v4 20/45] cherry-pick: copy notes and run hooks

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/rewrite.c   |  1 +
 builtin/sequencer.c | 18 +++++++++++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/builtin/rewrite.c b/builtin/rewrite.c
index 02e0ea9..acbad44 100644
--- a/builtin/rewrite.c
+++ b/builtin/rewrite.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "rewrite.h"
 #include "run-command.h"
+#include "builtin.h"
 
 void add_rewritten(struct rewritten *list, unsigned char *from, unsigned char *to)
 {
diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index d40fda9..fe96f1f 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -21,6 +21,15 @@
 
 static struct rewritten rewritten;
 
+static void finish(struct replay_opts *opts)
+{
+	if (opts->action != REPLAY_PICK)
+		return;
+
+	run_rewrite_hook(&rewritten, "cherry-pick");
+	copy_rewrite_notes(&rewritten, "cherry-pick");
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -935,6 +944,8 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
 		}
 	}
 
+	finish(opts);
+
 	/*
 	 * Sequence of picks finished successfully; cleanup by
 	 * removing the .git/sequencer directory
@@ -1006,8 +1017,13 @@ static int sequencer_skip(struct replay_opts *opts)
 
 static int single_pick(struct commit *cmit, struct replay_opts *opts)
 {
+	int ret;
 	setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
-	return do_pick_commit(cmit, opts);
+	ret = do_pick_commit(cmit, opts);
+	if (ret)
+		return ret;
+	finish(opts);
+	return 0;
 }
 
 int sequencer_pick_revisions(struct replay_opts *opts)
-- 
1.8.3.698.g079b096

[PATCH v4 21/45] cherry-pick: add --action-name option

[Felipe Contreras]

So it can be used by other tools (e.g. git rebase), and the right action
is passed to the hooks and notes rewrite stuff.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/revert.c           |  2 ++
 builtin/sequencer.c        | 17 ++++++++++++++---
 builtin/sequencer.h        |  2 ++
 git-rebase--interactive.sh |  4 ++--
 4 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index d3d5600..6066ad2 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -122,6 +122,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 		OPT_END(),
 		OPT_END(),
 		OPT_END(),
+		OPT_END(),
 	};
 
 	if (opts->action == REPLAY_PICK) {
@@ -132,6 +133,7 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
 			OPT_BOOLEAN(0, "allow-empty-message", &opts->allow_empty_message, N_("allow commits with empty messages")),
 			OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_redundant_commits, N_("keep redundant, empty commits")),
 			OPT_BOOLEAN(0, "skip-empty", &opts->skip_empty, N_("skip empty commits")),
+			OPT_STRING(0, "action-name", &opts->action_name, N_("name"), N_("action name")),
 			OPT_END(),
 		};
 		if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index fe96f1f..a419387 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -23,11 +23,18 @@ static struct rewritten rewritten;
 
 static void finish(struct replay_opts *opts)
 {
+	const char *name;
+
 	if (opts->action != REPLAY_PICK)
 		return;
 
-	run_rewrite_hook(&rewritten, "cherry-pick");
-	copy_rewrite_notes(&rewritten, "cherry-pick");
+	name = opts->action_name ? opts->action_name : "cherry-pick";
+
+	if (!*name)
+		return;
+
+	run_rewrite_hook(&rewritten, name);
+	copy_rewrite_notes(&rewritten, name);
 }
 
 static void remove_sequencer_state(void)
@@ -744,7 +751,9 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 	else if (!strcmp(key, "options.strategy-option")) {
 		ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
 		opts->xopts[opts->xopts_nr++] = xstrdup(value);
-	} else
+	} else if (!strcmp(key, "options.action-name"))
+		git_config_string(&opts->action_name, key, value);
+	else
 		return error(_("Invalid key: %s"), key);
 
 	if (!error_flag)
@@ -921,6 +930,8 @@ static void save_opts(struct replay_opts *opts)
 							"options.strategy-option",
 							opts->xopts[i], "^$", 0);
 	}
+	if (opts->action_name)
+		git_config_set_in_file(opts_file, "options.action-name", opts->action_name);
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
diff --git a/builtin/sequencer.h b/builtin/sequencer.h
index 63ba274..c69eeff 100644
--- a/builtin/sequencer.h
+++ b/builtin/sequencer.h
@@ -46,6 +46,8 @@ struct replay_opts {
 
 	/* Only used by REPLAY_NONE */
 	struct rev_info *revs;
+
+	const char *action_name;
 };
 
 int sequencer_pick_revisions(struct replay_opts *opts);
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..9f42a71 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -239,7 +239,7 @@ pick_one () {
 
 	test -d "$rewritten" &&
 		pick_one_preserving_merges "$@" && return
-	output git cherry-pick $empty_args $ff "$@"
+	output git cherry-pick --action-name '' $empty_args $ff "$@"
 }
 
 pick_one_preserving_merges () {
@@ -350,7 +350,7 @@ pick_one_preserving_merges () {
 			echo "$sha1 $(git rev-parse HEAD^0)" >> "$rewritten_list"
 			;;
 		*)
-			output git cherry-pick "$@" ||
+			output git cherry-pick --action-name '' "$@" ||
 				die_with_patch $sha1 "Could not pick $sha1"
 			;;
 		esac
-- 
1.8.3.698.g079b096

[PATCH v4 22/45] cherry-pick: remember rerere-autoupdate

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/sequencer.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/builtin/sequencer.c b/builtin/sequencer.c
index a419387..ddd369f 100644
--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -753,6 +753,8 @@ static int populate_opts_cb(const char *key, const char *value, void *data)
 		opts->xopts[opts->xopts_nr++] = xstrdup(value);
 	} else if (!strcmp(key, "options.action-name"))
 		git_config_string(&opts->action_name, key, value);
+	else if (!strcmp(key, "options.allow-rerere-auto"))
+		opts->allow_rerere_auto = git_config_int(key, value);
 	else
 		return error(_("Invalid key: %s"), key);
 
@@ -932,6 +934,12 @@ static void save_opts(struct replay_opts *opts)
 	}
 	if (opts->action_name)
 		git_config_set_in_file(opts_file, "options.action-name", opts->action_name);
+	if (opts->allow_rerere_auto) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "%d", opts->allow_rerere_auto);
+		git_config_set_in_file(opts_file, "options.allow-rerere-auto", buf.buf);
+		strbuf_release(&buf);
+	}
 }
 
 static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
-- 
1.8.3.698.g079b096

[PATCH v4 23/45] rebase: split the cherry-pick stuff

[Felipe Contreras]

They do something completely different from 'git am', it belongs in a
different file.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 .gitignore                |  1 +
 Makefile                  |  1 +
 git-rebase--am.sh         | 11 +----------
 git-rebase--cherrypick.sh | 30 ++++++++++++++++++++++++++++++
 git-rebase.sh             |  4 ++++
 5 files changed, 37 insertions(+), 10 deletions(-)
 create mode 100644 git-rebase--cherrypick.sh

diff --git a/.gitignore b/.gitignore
index 1640c3a..2a3dbae 100644
--- a/.gitignore
+++ b/.gitignore
@@ -113,6 +113,7 @@
 /git-read-tree
 /git-rebase
 /git-rebase--am
+/git-rebase--cherrypick
 /git-rebase--interactive
 /git-rebase--merge
 /git-receive-pack
diff --git a/Makefile b/Makefile
index a167e68..4719979 100644
--- a/Makefile
+++ b/Makefile
@@ -473,6 +473,7 @@ SCRIPT_SH += git-web--browse.sh
 SCRIPT_LIB += git-mergetool--lib
 SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
+SCRIPT_LIB += git-rebase--cherrypick
 SCRIPT_LIB += git-rebase--interactive
 SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 34e3102..6460028 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -19,15 +19,7 @@ esac
 test -n "$rebase_root" && root_flag=--root
 
 ret=0
-if test -n "$keep_empty"
-then
-	# we have to do this the hard way.  git format-patch completely squashes
-	# empty commits and even if it didn't the format doesn't really lend
-	# itself well to recording empty patches.  fortunately, cherry-pick
-	# makes this easy
-	git cherry-pick --allow-empty "$revisions"
-	ret=$?
-else
+
 rm -f "$GIT_DIR/rebased-patches"
 
 git format-patch -k --stdout --full-index --ignore-if-in-upstream \
@@ -63,7 +55,6 @@ else
 ret=$?
 
 rm -f "$GIT_DIR/rebased-patches"
-fi
 
 if test 0 != $ret
 then
diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
new file mode 100644
index 0000000..2c16995
--- /dev/null
+++ b/git-rebase--cherrypick.sh
@@ -0,0 +1,30 @@
+#!/bin/sh
+#
+# Copyright (c) 2010 Junio C Hamano.
+#
+
+case "$action" in
+continue)
+	git am --resolved --resolvemsg="$resolvemsg" &&
+	move_to_original_branch
+	return
+	;;
+skip)
+	git am --skip --resolvemsg="$resolvemsg" &&
+	move_to_original_branch
+	return
+	;;
+esac
+
+test -n "$rebase_root" && root_flag=--root
+
+git cherry-pick --allow-empty "$revisions"
+ret=$?
+
+if test 0 != $ret
+then
+	test -d "$state_dir" && write_basic_state
+	return $ret
+fi
+
+move_to_original_branch
diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..70762f1 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -413,6 +413,10 @@ elif test -n "$do_merge"
 then
 	type=merge
 	state_dir="$merge_dir"
+elif test -n "$keep_empty"
+then
+	type=cherrypick
+	state_dir="$apply_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.698.g079b096

[PATCH v4 24/45] rebase: cherry-pick: fix mode storage

[Felipe Contreras]

We don't use the 'rebase-apply'.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 5 ++++-
 git-rebase.sh             | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 2c16995..e142cfb 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -18,12 +18,15 @@ esac
 
 test -n "$rebase_root" && root_flag=--root
 
+mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
+: > "$state_dir"/cherrypick || die "Could not mark as cherrypick"
+
 git cherry-pick --allow-empty "$revisions"
 ret=$?
 
 if test 0 != $ret
 then
-	test -d "$state_dir" && write_basic_state
+	write_basic_state
 	return $ret
 fi
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 70762f1..4465daf 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -205,6 +205,9 @@ then
 	then
 		type=interactive
 		interactive_rebase=explicit
+	elif test -f "$merge_dir"/cherrypick
+	then
+		type=cherrypick
 	else
 		type=merge
 	fi
@@ -416,7 +419,7 @@ then
 elif test -n "$keep_empty"
 then
 	type=cherrypick
-	state_dir="$apply_dir"
+	state_dir="$merge_dir"
 else
 	type=am
 	state_dir="$apply_dir"
-- 
1.8.3.698.g079b096

[PATCH v4 25/45] rebase: cherry-pick: fix sequence continuation

[Felipe Contreras]

We are not in am mode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index e142cfb..d8d32fe 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -5,12 +5,12 @@
 
 case "$action" in
 continue)
-	git am --resolved --resolvemsg="$resolvemsg" &&
+	git cherry-pick --continue &&
 	move_to_original_branch
 	return
 	;;
 skip)
-	git am --skip --resolvemsg="$resolvemsg" &&
+	git cherry-pick --skip &&
 	move_to_original_branch
 	return
 	;;
-- 
1.8.3.698.g079b096

[PATCH v4 26/45] rebase: cherry-pick: fix abort of cherry mode

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index 4465daf..0937e2c 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -369,6 +369,7 @@ skip)
 	run_specific_rebase
 	;;
 abort)
+	test "$type" == "cherrypick" && git cherry-pick --abort
 	git rerere clear
 	read_basic_state
 	case "$head_name" in
-- 
1.8.3.698.g079b096

[PATCH v4 27/45] rebase: cherry-pick: fix command invocations

[Felipe Contreras]

So that all the tests pass.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index d8d32fe..e9ecccc 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -21,7 +21,22 @@ test -n "$rebase_root" && root_flag=--root
 mkdir -p "$state_dir" || die "Could not create temporary $state_dir"
 : > "$state_dir"/cherrypick || die "Could not mark as cherrypick"
 
-git cherry-pick --allow-empty "$revisions"
+if test -n "$rebase_root"
+then
+	revisions="$onto...$orig_head"
+else
+	revisions="$upstream...$orig_head"
+fi
+
+if test -n "$keep_empty"
+then
+	extra="--allow-empty"
+else
+	extra="--skip-empty --cherry-pick"
+fi
+test -n "$GIT_QUIET" && extra="$extra -q"
+test -z "$force_rebase" && extra="$extra --ff"
+git cherry-pick --no-merges --right-only --topo-order --do-walk $extra "$revisions"
 ret=$?
 
 if test 0 != $ret
-- 
1.8.3.698.g079b096

[PATCH v4 28/45] rebase: cherry-pick: fix status messages

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index e9ecccc..be17ec4 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -3,6 +3,9 @@
 # Copyright (c) 2010 Junio C Hamano.
 #
 
+GIT_CHERRY_PICK_HELP="$resolvemsg"
+export GIT_CHERRY_PICK_HELP
+
 case "$action" in
 continue)
 	git cherry-pick --continue &&
-- 
1.8.3.698.g079b096

[PATCH v4 29/45] rebase: cherry-pick: automatically commit stage

[Felipe Contreras]

When there's changes in the staging area. Just like the other rebase
modes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index be17ec4..241cda7 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -8,6 +8,12 @@ export GIT_CHERRY_PICK_HELP
 
 case "$action" in
 continue)
+	# do we have anything to commit?
+	if ! git diff-index --cached --quiet HEAD --
+	then
+		git commit --no-verify -e ||
+			die "Could not commit staged changes."
+	fi
 	git cherry-pick --continue &&
 	move_to_original_branch
 	return
-- 
1.8.3.698.g079b096

[PATCH v4 30/45] rebase: cherry-pick: set correct action-name

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 241cda7..d36b0dc 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -45,7 +45,7 @@ else
 fi
 test -n "$GIT_QUIET" && extra="$extra -q"
 test -z "$force_rebase" && extra="$extra --ff"
-git cherry-pick --no-merges --right-only --topo-order --do-walk $extra "$revisions"
+git cherry-pick --no-merges --right-only --topo-order --do-walk --action-name rebase $extra "$revisions"
 ret=$?
 
 if test 0 != $ret
-- 
1.8.3.698.g079b096

[PATCH v4 31/45] rebase: trivial cleanup

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--am.sh | 1 +
 git-rebase.sh     | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index 6460028..2ce7570 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -51,6 +51,7 @@ then
 	return $?
 fi
 
+test -n "$GIT_QUIET" && git_am_opt="$git_am_opt -q"
 git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" <"$GIT_DIR/rebased-patches"
 ret=$?
 
diff --git a/git-rebase.sh b/git-rebase.sh
index 0937e2c..6be247d 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -285,7 +285,6 @@ do
 		;;
 	-q)
 		GIT_QUIET=t
-		git_am_opt="$git_am_opt -q"
 		verbose=
 		diffstat=
 		;;
-- 
1.8.3.698.g079b096

[PATCH v4 32/45] rebase: use 'cherrypick' mode instead of 'am'

[Felipe Contreras]

Unless any specific 'git am' options are used.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-prompt.sh       | 2 ++
 git-rebase.sh                          | 8 ++++----
 t/t3407-rebase-abort.sh                | 2 +-
 t/t3420-rebase-autostash.sh            | 2 +-
 t/t5520-pull.sh                        | 2 +-
 t/t9106-git-svn-commit-diff-clobber.sh | 2 +-
 6 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 86a4f3f..3a14665 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -352,6 +352,8 @@ __git_ps1 ()
 			total=$(cat "$g/rebase-merge/end")
 			if [ -f "$g/rebase-merge/interactive" ]; then
 				r="|REBASE-i"
+			elif [ -f "$g/rebase-merge/cherrypick" ]; then
+				r="|REBASE"
 			else
 				r="|REBASE-m"
 			fi
diff --git a/git-rebase.sh b/git-rebase.sh
index 6be247d..f2efff9 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -416,13 +416,13 @@ elif test -n "$do_merge"
 then
 	type=merge
 	state_dir="$merge_dir"
-elif test -n "$keep_empty"
+elif test -n "$git_am_opt"
 then
-	type=cherrypick
-	state_dir="$merge_dir"
-else
 	type=am
 	state_dir="$apply_dir"
+else
+	type=cherrypick
+	state_dir="$merge_dir"
 fi
 
 if test -z "$rebase_root"
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index a6a6c40..2699b08 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -96,7 +96,7 @@ testrebase() {
 	'
 }
 
-testrebase "" .git/rebase-apply
+testrebase "" .git/rebase-merge
 testrebase " --merge" .git/rebase-merge
 
 test_done
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 479cbb2..a5e69f3 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -141,7 +141,7 @@ testrebase() {
 	'
 }
 
-testrebase "" .git/rebase-apply
+testrebase "" .git/rebase-merge
 testrebase " --merge" .git/rebase-merge
 testrebase " --interactive" .git/rebase-merge
 
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 6af6c63..ec2373b 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -244,7 +244,7 @@ test_expect_success 'setup for avoiding reapplying old patches' '
 test_expect_success 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
-	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
+	 test 1 = $(cat .git/sequencer/todo | wc -l)
 	)
 '
 
diff --git a/t/t9106-git-svn-commit-diff-clobber.sh b/t/t9106-git-svn-commit-diff-clobber.sh
index f6d7ac7..b9cec33 100755
--- a/t/t9106-git-svn-commit-diff-clobber.sh
+++ b/t/t9106-git-svn-commit-diff-clobber.sh
@@ -92,7 +92,7 @@ test_expect_success 'multiple dcommit from git svn will not clobber svn' "
 
 
 test_expect_success 'check that rebase really failed' '
-	test -d .git/rebase-apply
+	test -d .git/rebase-merge
 '
 
 test_expect_success 'resolve, continue the rebase and dcommit' "
-- 
1.8.3.698.g079b096

[PATCH v4 33/45] rebase: cherry-pick: fix for shell prompt

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 contrib/completion/git-prompt.sh |  2 ++
 git-rebase--cherrypick.sh        | 11 +++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 3a14665..3d10f21 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -354,6 +354,8 @@ __git_ps1 ()
 				r="|REBASE-i"
 			elif [ -f "$g/rebase-merge/cherrypick" ]; then
 				r="|REBASE"
+				step=$(cat "$g/sequencer/rewritten" | wc -l)
+				let step+=1
 			else
 				r="|REBASE-m"
 			fi
diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index d36b0dc..6f63618 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -37,19 +37,26 @@ else
 	revisions="$upstream...$orig_head"
 fi
 
+rev_args="--no-merges --right-only --topo-order --do-walk "$revisions""
+
 if test -n "$keep_empty"
 then
 	extra="--allow-empty"
 else
-	extra="--skip-empty --cherry-pick"
+	extra="--skip-empty"
+	rev_args="--cherry-pick $rev_args"
 fi
 test -n "$GIT_QUIET" && extra="$extra -q"
 test -z "$force_rebase" && extra="$extra --ff"
-git cherry-pick --no-merges --right-only --topo-order --do-walk --action-name rebase $extra "$revisions"
+
+git rev-list $rev_args > "$state_dir"/list
+git cherry-pick --action-name rebase $extra $rev_args
 ret=$?
 
 if test 0 != $ret
 then
+	# for shell prompt
+	cat "$state_dir"/list | wc -l > "$state_dir"/end
 	write_basic_state
 	return $ret
 fi
-- 
1.8.3.698.g079b096

[PATCH v4 34/45] rebase: cherry-pick: add merge options

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 6f63618..644d45e 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -48,6 +48,14 @@ else
 fi
 test -n "$GIT_QUIET" && extra="$extra -q"
 test -z "$force_rebase" && extra="$extra --ff"
+test -n "$strategy" && extra="$extra --strategy=$strategy"
+for x in "$strategy_opts"
+do
+	test -z "$x" && continue
+	x=$(eval "echo $x")
+	extra="$extra -X${x#--}"
+done
+test -n "$allow_rerere_autoupdate" && extra="$extra $allow_rerere_autoupdate"
 
 git rev-list $rev_args > "$state_dir"/list
 git cherry-pick --action-name rebase $extra $rev_args
-- 
1.8.3.698.g079b096

[PATCH v4 35/45] rebase: remove merge mode

[Felipe Contreras]

The cherrypick mode does the job.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Makefile                         |   1 -
 contrib/completion/git-prompt.sh |   4 +-
 git-rebase--merge.sh             | 151 ---------------------------------------
 git-rebase.sh                    |  13 +---
 t/t3406-rebase-message.sh        |  15 ----
 t/t9903-bash-prompt.sh           |   2 +-
 6 files changed, 3 insertions(+), 183 deletions(-)
 delete mode 100644 git-rebase--merge.sh

diff --git a/Makefile b/Makefile
index 4719979..609fa9e 100644
--- a/Makefile
+++ b/Makefile
@@ -475,7 +475,6 @@ SCRIPT_LIB += git-parse-remote
 SCRIPT_LIB += git-rebase--am
 SCRIPT_LIB += git-rebase--cherrypick
 SCRIPT_LIB += git-rebase--interactive
-SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 3d10f21..5036795 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -352,12 +352,10 @@ __git_ps1 ()
 			total=$(cat "$g/rebase-merge/end")
 			if [ -f "$g/rebase-merge/interactive" ]; then
 				r="|REBASE-i"
-			elif [ -f "$g/rebase-merge/cherrypick" ]; then
+			else
 				r="|REBASE"
 				step=$(cat "$g/sequencer/rewritten" | wc -l)
 				let step+=1
-			else
-				r="|REBASE-m"
 			fi
 		else
 			if [ -d "$g/rebase-apply" ]; then
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
deleted file mode 100644
index 16d1817..0000000
--- a/git-rebase--merge.sh
+++ /dev/null
@@ -1,151 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2010 Junio C Hamano.
-#
-
-prec=4
-
-read_state () {
-	onto_name=$(cat "$state_dir"/onto_name) &&
-	end=$(cat "$state_dir"/end) &&
-	msgnum=$(cat "$state_dir"/msgnum)
-}
-
-continue_merge () {
-	test -d "$state_dir" || die "$state_dir directory does not exist"
-
-	unmerged=$(git ls-files -u)
-	if test -n "$unmerged"
-	then
-		echo "You still have unmerged paths in your index"
-		echo "did you forget to use git add?"
-		die "$resolvemsg"
-	fi
-
-	cmt=`cat "$state_dir/current"`
-	if ! git diff-index --quiet --ignore-submodules HEAD --
-	then
-		if ! git commit --no-verify -C "$cmt"
-		then
-			echo "Commit failed, please do not call \"git commit\""
-			echo "directly, but instead do one of the following: "
-			die "$resolvemsg"
-		fi
-		if test -z "$GIT_QUIET"
-		then
-			printf "Committed: %0${prec}d " $msgnum
-		fi
-		echo "$cmt $(git rev-parse HEAD^0)" >> "$state_dir/rewritten"
-	else
-		if test -z "$GIT_QUIET"
-		then
-			printf "Already applied: %0${prec}d " $msgnum
-		fi
-	fi
-	test -z "$GIT_QUIET" &&
-	GIT_PAGER='' git log --format=%s -1 "$cmt"
-
-	# onto the next patch:
-	msgnum=$(($msgnum + 1))
-	echo "$msgnum" >"$state_dir/msgnum"
-}
-
-call_merge () {
-	cmt="$(cat "$state_dir/cmt.$1")"
-	echo "$cmt" > "$state_dir/current"
-	hd=$(git rev-parse --verify HEAD)
-	cmt_name=$(git symbolic-ref HEAD 2> /dev/null || echo HEAD)
-	msgnum=$(cat "$state_dir/msgnum")
-	eval GITHEAD_$cmt='"${cmt_name##refs/heads/}~$(($end - $msgnum))"'
-	eval GITHEAD_$hd='$onto_name'
-	export GITHEAD_$cmt GITHEAD_$hd
-	if test -n "$GIT_QUIET"
-	then
-		GIT_MERGE_VERBOSITY=1 && export GIT_MERGE_VERBOSITY
-	fi
-	test -z "$strategy" && strategy=recursive
-	eval 'git-merge-$strategy' $strategy_opts '"$cmt^" -- "$hd" "$cmt"'
-	rv=$?
-	case "$rv" in
-	0)
-		unset GITHEAD_$cmt GITHEAD_$hd
-		return
-		;;
-	1)
-		git rerere $allow_rerere_autoupdate
-		die "$resolvemsg"
-		;;
-	2)
-		echo "Strategy: $strategy failed, try another" 1>&2
-		die "$resolvemsg"
-		;;
-	*)
-		die "Unknown exit code ($rv) from command:" \
-			"git-merge-$strategy $cmt^ -- HEAD $cmt"
-		;;
-	esac
-}
-
-finish_rb_merge () {
-	move_to_original_branch
-	if test -s "$state_dir"/rewritten
-	then
-		git notes copy --for-rewrite=rebase <"$state_dir"/rewritten
-		if test -x "$GIT_DIR"/hooks/post-rewrite
-		then
-			"$GIT_DIR"/hooks/post-rewrite rebase <"$state_dir"/rewritten
-		fi
-	fi
-	say All done.
-}
-
-case "$action" in
-continue)
-	read_state
-	continue_merge
-	while test "$msgnum" -le "$end"
-	do
-		call_merge "$msgnum"
-		continue_merge
-	done
-	finish_rb_merge
-	return
-	;;
-skip)
-	read_state
-	git rerere clear
-	msgnum=$(($msgnum + 1))
-	while test "$msgnum" -le "$end"
-	do
-		call_merge "$msgnum"
-		continue_merge
-	done
-	finish_rb_merge
-	return
-	;;
-esac
-
-mkdir -p "$state_dir"
-echo "$onto_name" > "$state_dir/onto_name"
-write_basic_state
-
-msgnum=0
-for cmt in `git rev-list --reverse --no-merges "$revisions"`
-do
-	msgnum=$(($msgnum + 1))
-	echo "$cmt" > "$state_dir/cmt.$msgnum"
-done
-
-echo 1 >"$state_dir/msgnum"
-echo $msgnum >"$state_dir/end"
-
-end=$msgnum
-msgnum=1
-
-while test "$msgnum" -le "$end"
-do
-	call_merge "$msgnum"
-	continue_merge
-done
-
-finish_rb_merge
diff --git a/git-rebase.sh b/git-rebase.sh
index f2efff9..8ddf270 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -59,7 +59,6 @@ unset onto
 cmd=
 strategy=
 strategy_opts=
-do_merge=
 merge_dir="$GIT_DIR"/rebase-merge
 apply_dir="$GIT_DIR"/rebase-apply
 verbose=
@@ -205,11 +204,8 @@ then
 	then
 		type=interactive
 		interactive_rebase=explicit
-	elif test -f "$merge_dir"/cherrypick
-	then
-		type=cherrypick
 	else
-		type=merge
+		type=cherrypick
 	fi
 	state_dir="$merge_dir"
 fi
@@ -256,18 +252,15 @@ do
 		autosquash=
 		;;
 	-M|-m)
-		do_merge=t
 		;;
 	-X)
 		shift
 		strategy_opts="$strategy_opts $(git rev-parse --sq-quote "--$1")"
-		do_merge=t
 		test -z "$strategy" && strategy=recursive
 		;;
 	-s)
 		shift
 		strategy="$1"
-		do_merge=t
 		;;
 	-n)
 		diffstat=
@@ -412,10 +405,6 @@ if test -n "$interactive_rebase"
 then
 	type=interactive
 	state_dir="$merge_dir"
-elif test -n "$do_merge"
-then
-	type=merge
-	state_dir="$merge_dir"
 elif test -n "$git_am_opt"
 then
 	type=am
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index e6a9a0d..37d19c5 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -27,21 +27,6 @@ test_expect_success setup '
 
 '
 
-cat >expect <<\EOF
-Already applied: 0001 A
-Already applied: 0002 B
-Committed: 0003 Z
-EOF
-
-test_expect_success 'rebase -m' '
-
-	git rebase -m master >report &&
-	sed -n -e "/^Already applied: /p" \
-		-e "/^Committed: /p" report >actual &&
-	test_cmp expect actual
-
-'
-
 test_expect_success 'rebase --stat' '
 	git reset --hard start &&
         git rebase --stat master >diffstat.txt &&
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 15521cc..2e46f93 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -266,7 +266,7 @@ EOF
 '
 
 test_expect_success 'prompt - rebase merge' '
-	printf " (b2|REBASE-m 1/3)" > expected &&
+	printf " (b2|REBASE 1/3)" > expected &&
 	git checkout b2 &&
 	test_when_finished "git checkout master" &&
 	test_must_fail git rebase --merge b1 b2 &&
-- 
1.8.3.698.g079b096

[PATCH v4 36/45] rebase: cherry-pick: add copyright

[Felipe Contreras]

Probably enough changes to warrant that.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 git-rebase--cherrypick.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--cherrypick.sh b/git-rebase--cherrypick.sh
index 644d45e..e36e104 100644
--- a/git-rebase--cherrypick.sh
+++ b/git-rebase--cherrypick.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# Copyright (c) 2010 Junio C Hamano.
+# Copyright (c) 2013 Felipe Contreras
 #
 
 GIT_CHERRY_PICK_HELP="$resolvemsg"
-- 
1.8.3.698.g079b096

[PATCH v4 37/45] tests: fix autostash

[Felipe Contreras]

We should call 'git rebase --abort', like a normal user would do.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3420-rebase-autostash.sh | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index a5e69f3..ff370a3 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -71,8 +71,7 @@ testrebase() {
 		test_must_fail git rebase$type related-onto-branch &&
 		test_path_is_file $dotest/autostash &&
 		! grep dirty file3 &&
-		rm -rf $dotest &&
-		git reset --hard &&
+		git rebase --abort &&
 		git checkout feature-branch
 	'
 
-- 
1.8.3.698.g079b096

[PATCH v4 38/45] add simple tests of consistency across rebase types

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Helped-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-rebase.sh                   | 16 ++++++++
 t/t3421-rebase-topology-linear.sh | 78 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 94 insertions(+)
 create mode 100755 t/t3421-rebase-topology-linear.sh

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 6ccf797..1e0ff28 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -65,3 +65,19 @@ EOF
 	test_set_editor "$(pwd)/fake-editor.sh"
 	chmod a+x fake-editor.sh
 }
+
+# checks that the revisions in "$2" represent a linear range with the
+# subjects in "$1"
+test_linear_range () {
+	revlist_merges=$(git rev-list --merges "$2") &&
+	test -z "$revlist_merges" &&
+	expected=$1
+	set -- $(git log --reverse --format=%s "$2")
+	test "$expected" = "$*"
+}
+
+reset_rebase () {
+	test_might_fail git rebase --abort &&
+	git reset --hard &&
+	git clean -f
+}
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
new file mode 100755
index 0000000..60365d1
--- /dev/null
+++ b/t/t3421-rebase-topology-linear.sh
@@ -0,0 +1,78 @@
+#!/bin/sh
+
+test_description='basic rebase topology tests'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+# a---b---c
+#      \
+#       d---e
+test_expect_success 'setup' '
+	test_commit a &&
+	test_commit b &&
+	test_commit c &&
+	git checkout b &&
+	test_commit d &&
+	test_commit e
+'
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "simple rebase $*" "
+		reset_rebase &&
+		git rebase $* c e &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'd e' c..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* is no-op if upstream is an ancestor" "
+		reset_rebase &&
+		git rebase $* b e &&
+		test_cmp_rev e HEAD
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* -f rewrites even if upstream is an ancestor" "
+		reset_rebase &&
+		git rebase $* -f b e &&
+		! test_cmp_rev e HEAD &&
+		test_cmp_rev b HEAD~2 &&
+		test_linear_range 'd e' b..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* fast-forwards from ancestor of upstream" "
+		reset_rebase &&
+		git rebase $* e b &&
+		test_cmp_rev e HEAD
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_done
-- 
1.8.3.698.g079b096

[PATCH v4 39/45] add tests for rebasing with patch-equivalence present

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/lib-rebase.sh                   | 17 ++++++++
 t/t3421-rebase-topology-linear.sh | 85 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 102 insertions(+)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 1e0ff28..4b74ae4 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -81,3 +81,20 @@ reset_rebase () {
 	git reset --hard &&
 	git clean -f
 }
+
+cherry_pick () {
+	git cherry-pick -n "$2" &&
+	git commit -m "$1" &&
+	git tag "$1"
+}
+
+revert () {
+	git revert -n "$2" &&
+	git commit -m "$1" &&
+	git tag "$1"
+}
+
+make_empty () {
+	git commit --allow-empty -m "$1" &&
+	git tag "$1"
+}
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index 60365d1..ddcbfc6 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -75,4 +75,89 @@ test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
+#       f
+#      /
+# a---b---c---g---h
+#      \
+#       d---G---i
+#
+# uppercase = cherry-picked
+# h = reverted g
+#
+# Reverted patches are there for tests to be able to check if a commit
+# that introduced the same change as another commit is
+# dropped. Without reverted commits, we could get false positives
+# because applying the patch succeeds, but simply results in no
+# changes.
+test_expect_success 'setup of linear history for range selection tests' '
+	git checkout c &&
+	test_commit g &&
+	revert h g &&
+	git checkout d &&
+	cherry_pick G g &&
+	test_commit i &&
+	git checkout b &&
+	test_commit f
+'
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* drops patches in upstream" "
+		reset_rebase &&
+		git rebase $* h i &&
+		test_cmp_rev h HEAD~2 &&
+		test_linear_range 'd i' h..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* can drop last patch if in upstream" "
+		reset_rebase &&
+		git rebase $* h G &&
+		test_cmp_rev h HEAD^ &&
+		test_linear_range 'd' h..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --onto drops patches in upstream" "
+		reset_rebase &&
+		git rebase $* --onto f h i &&
+		test_cmp_rev f HEAD~2 &&
+		test_linear_range 'd i' f..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --onto does not drop patches in onto" "
+		reset_rebase &&
+		git rebase $* --onto h f i &&
+		test_cmp_rev h HEAD~3 &&
+		test_linear_range 'd G i' h..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
 test_done
-- 
1.8.3.698.g079b096

[PATCH v4 40/45] add tests for rebasing of empty commits

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3401-rebase-partial.sh         | 24 ----------------
 t/t3421-rebase-topology-linear.sh | 58 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 24 deletions(-)

diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
index 58f4823..7ba1797 100755
--- a/t/t3401-rebase-partial.sh
+++ b/t/t3401-rebase-partial.sh
@@ -42,28 +42,4 @@ test_expect_success 'rebase --merge topic branch that was partially merged upstr
 	test_path_is_missing .git/rebase-merge
 '
 
-test_expect_success 'rebase ignores empty commit' '
-	git reset --hard A &&
-	git commit --allow-empty -m empty &&
-	test_commit D &&
-	git rebase C &&
-	test "$(git log --format=%s C..)" = "D"
-'
-
-test_expect_success 'rebase --keep-empty' '
-	git reset --hard D &&
-	git rebase --keep-empty C &&
-	test "$(git log --format=%s C..)" = "D
-empty"
-'
-
-test_expect_success 'rebase --keep-empty keeps empty even if already in upstream' '
-	git reset --hard A &&
-	git commit --allow-empty -m also-empty &&
-	git rebase --keep-empty D &&
-	test "$(git log --format=%s A..)" = "also-empty
-D
-empty"
-'
-
 test_done
diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index ddcbfc6..f19f0d0 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -160,4 +160,62 @@ test_run_rebase success -m
 test_run_rebase success -i
 test_run_rebase success -p
 
+# a---b---c---j!
+#      \
+#       d---k!--l
+#
+# ! = empty
+test_expect_success 'setup of linear history for empty commit tests' '
+	git checkout c &&
+	make_empty j &&
+	git checkout d &&
+	make_empty k &&
+	test_commit l
+'
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* drops empty commit" "
+		reset_rebase &&
+		git rebase $* c l &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'd l' c..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --keep-empty" "
+		reset_rebase &&
+		git rebase $* --keep-empty c l &&
+		test_cmp_rev c HEAD~3 &&
+		test_linear_range 'd k l' c..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --keep-empty keeps empty even if already in upstream" "
+		reset_rebase &&
+		git rebase $* --keep-empty j l &&
+		test_cmp_rev j HEAD~3 &&
+		test_linear_range 'd k l' j..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase failure -i
+test_run_rebase failure -p
+
 test_done
-- 
1.8.3.698.g079b096

[PATCH v4 41/45] add tests for rebasing root

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3421-rebase-topology-linear.sh | 129 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 129 insertions(+)

diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
index f19f0d0..e67add6 100755
--- a/t/t3421-rebase-topology-linear.sh
+++ b/t/t3421-rebase-topology-linear.sh
@@ -218,4 +218,133 @@ test_run_rebase failure -m
 test_run_rebase failure -i
 test_run_rebase failure -p
 
+#       m
+#      /
+# a---b---c---g
+#
+# x---y---B
+#
+# uppercase = cherry-picked
+# m = reverted b
+#
+# Reverted patches are there for tests to be able to check if a commit
+# that introduced the same change as another commit is
+# dropped. Without reverted commits, we could get false positives
+# because applying the patch succeeds, but simply results in no
+# changes.
+test_expect_success 'setup of linear history for test involving root' '
+	git checkout b &&
+	revert m b &&
+	git checkout --orphan disjoint &&
+	git rm -rf . &&
+	test_commit x &&
+	test_commit y &&
+	cherry_pick B b
+'
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --onto --root" "
+		reset_rebase &&
+		git rebase $* --onto c --root y &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'x y' c..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* without --onto --root with disjoint history" "
+		reset_rebase &&
+		git rebase $* c y &&
+		test_cmp_rev c HEAD~2 &&
+		test_linear_range 'x y' c..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --onto --root drops patch in onto" "
+		reset_rebase &&
+		git rebase $* --onto m --root B &&
+		test_cmp_rev m HEAD~2 &&
+		test_linear_range 'x y' m..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase success -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --onto --root with merge-base does not go to root" "
+		reset_rebase &&
+		git rebase $* --onto m --root g &&
+		test_cmp_rev m HEAD~2 &&
+		test_linear_range 'c g' m..
+	"
+}
+
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* without --onto --root with disjoint history drops patch in onto" "
+		reset_rebase &&
+		git rebase $* m B &&
+		test_cmp_rev m HEAD~2 &&
+		test_linear_range 'x y' m..
+	"
+}
+test_run_rebase success ''
+test_run_rebase failure -m
+test_run_rebase success -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* --root on linear history is a no-op" "
+		reset_rebase &&
+		git rebase $* --root c &&
+		test_cmp_rev c HEAD
+	"
+}
+test_run_rebase failure ''
+test_run_rebase failure -m
+test_run_rebase failure -i
+test_run_rebase failure -p
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* -f --root on linear history causes re-write" "
+		reset_rebase &&
+		git rebase $* -f --root c &&
+		! test_cmp_rev a HEAD~2 &&
+		test_linear_range 'a b c' HEAD
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+test_run_rebase success -p
+
 test_done
-- 
1.8.3.698.g079b096

[PATCH v4 42/45] add tests for rebasing merged history

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t3400-rebase.sh                 |  31 +----
 t/t3401-rebase-partial.sh         |  45 -------
 t/t3404-rebase-interactive.sh     |  10 +-
 t/t3409-rebase-preserve-merges.sh |  53 --------
 t/t3425-rebase-topology-merges.sh | 258 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 260 insertions(+), 137 deletions(-)
 delete mode 100755 t/t3401-rebase-partial.sh
 create mode 100755 t/t3425-rebase-topology-merges.sh

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b58fa1a..b436ef4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -40,13 +40,6 @@ test_expect_success 'prepare repository with topic branches' '
 	echo Side >>C &&
 	git add C &&
 	git commit -m "Add C" &&
-	git checkout -b nonlinear my-topic-branch &&
-	echo Edit >>B &&
-	git add B &&
-	git commit -m "Modify B" &&
-	git merge side &&
-	git checkout -b upstream-merged-nonlinear &&
-	git merge master &&
 	git checkout -f my-topic-branch &&
 	git tag topic
 '
@@ -106,31 +99,9 @@ test_expect_success 'rebase from ambiguous branch name' '
 	git rebase master
 '
 
-test_expect_success 'rebase after merge master' '
-	git checkout --detach refs/tags/topic &&
-	git branch -D topic &&
-	git reset --hard topic &&
-	git merge master &&
-	git rebase master &&
-	! (git show | grep "^Merge:")
-'
-
-test_expect_success 'rebase of history with merges is linearized' '
-	git checkout nonlinear &&
-	test 4 = $(git rev-list master.. | wc -l) &&
-	git rebase master &&
-	test 3 = $(git rev-list master.. | wc -l)
-'
-
-test_expect_success 'rebase of history with merges after upstream merge is linearized' '
-	git checkout upstream-merged-nonlinear &&
-	test 5 = $(git rev-list master.. | wc -l) &&
-	git rebase master &&
-	test 3 = $(git rev-list master.. | wc -l)
-'
-
 test_expect_success 'rebase a single mode change' '
 	git checkout master &&
+	git branch -D topic &&
 	echo 1 >X &&
 	git add X &&
 	test_tick &&
diff --git a/t/t3401-rebase-partial.sh b/t/t3401-rebase-partial.sh
deleted file mode 100755
index 7ba1797..0000000
--- a/t/t3401-rebase-partial.sh
+++ /dev/null
@@ -1,45 +0,0 @@
-#!/bin/sh
-#
-# Copyright (c) 2006 Yann Dirson, based on t3400 by Amos Waterland
-#
-
-test_description='git rebase should detect patches integrated upstream
-
-This test cherry-picks one local change of two into master branch, and
-checks that git rebase succeeds with only the second patch in the
-local branch.
-'
-. ./test-lib.sh
-
-test_expect_success 'prepare repository with topic branch' '
-	test_commit A &&
-	git checkout -b my-topic-branch &&
-	test_commit B &&
-	test_commit C &&
-	git checkout -f master &&
-	test_commit A2 A.t
-'
-
-test_expect_success 'pick top patch from topic branch into master' '
-	git cherry-pick C &&
-	git checkout -f my-topic-branch
-'
-
-test_debug '
-	git cherry master &&
-	git format-patch -k --stdout --full-index master >/dev/null &&
-	gitk --all & sleep 1
-'
-
-test_expect_success 'rebase topic branch against new master and check git am did not get halted' '
-	git rebase master &&
-	test_path_is_missing .git/rebase-apply
-'
-
-test_expect_success 'rebase --merge topic branch that was partially merged upstream' '
-	git reset --hard C &&
-	git rebase --merge master &&
-	test_path_is_missing .git/rebase-merge
-'
-
-test_done
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 79e8d3c..0d3c573 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -477,19 +477,11 @@ test_expect_success 'interrupted squash works as expected (case 2)' '
 	test $one = $(git rev-parse HEAD~2)
 '
 
-test_expect_success 'ignore patch if in upstream' '
-	HEAD=$(git rev-parse HEAD) &&
-	git checkout -b has-cherry-picked HEAD^ &&
+test_expect_success '--continue tries to commit, even for "edit"' '
 	echo unrelated > file7 &&
 	git add file7 &&
 	test_tick &&
 	git commit -m "unrelated change" &&
-	git cherry-pick $HEAD &&
-	EXPECT_COUNT=1 git rebase -i $HEAD &&
-	test $HEAD = $(git rev-parse HEAD^)
-'
-
-test_expect_success '--continue tries to commit, even for "edit"' '
 	parent=$(git rev-parse HEAD^) &&
 	test_tick &&
 	FAKE_LINES="edit 1" git rebase -i HEAD^ &&
diff --git a/t/t3409-rebase-preserve-merges.sh b/t/t3409-rebase-preserve-merges.sh
index 6de4e22..2e0c364 100755
--- a/t/t3409-rebase-preserve-merges.sh
+++ b/t/t3409-rebase-preserve-merges.sh
@@ -11,14 +11,6 @@ Run "git rebase -p" and check that merges are properly carried along
 GIT_AUTHOR_EMAIL=bogus_email_address
 export GIT_AUTHOR_EMAIL
 
-# Clone 1 (trivial merge):
-#
-# A1--A2  <-- origin/master
-#  \   \
-#   B1--M  <-- topic
-#    \
-#     B2  <-- origin/topic
-#
 # Clone 2 (conflicting merge):
 #
 # A1--A2--B3   <-- origin/master
@@ -36,16 +28,6 @@ export GIT_AUTHOR_EMAIL
 #     \--A3    <-- topic2
 #      \
 #       B2     <-- origin/topic
-#
-# Clone 4 (merge using second parent as base):
-#
-# A1--A2--B3   <-- origin/master
-#  \
-#   B1--A3--M  <-- topic
-#    \     /
-#     \--A4    <-- topic2
-#      \
-#       B2     <-- origin/topic
 
 test_expect_success 'setup for merge-preserving rebase' \
 	'echo First > A &&
@@ -58,20 +40,6 @@ test_expect_success 'setup for merge-preserving rebase' \
 	git checkout -f master &&
 	echo Third >> A &&
 	git commit -a -m "Modify A2" &&
-
-	git clone ./. clone1 &&
-	(cd clone1 &&
-	git checkout -b topic origin/topic &&
-	git merge origin/master
-	) &&
-
-	git clone ./. clone4 &&
-	(
-		cd clone4 &&
-		git checkout -b topic origin/topic &&
-		git merge origin/master
-	) &&
-
 	echo Fifth > B &&
 	git add B &&
 	git commit -m "Add different B" &&
@@ -101,16 +69,6 @@ test_expect_success 'setup for merge-preserving rebase' \
 	git commit -a -m "Modify B2"
 '
 
-test_expect_success 'rebase -p fakes interactive rebase' '
-	(
-	cd clone1 &&
-	git fetch &&
-	git rebase -p origin/topic &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote-tracking branch " | wc -l)
-	)
-'
-
 test_expect_success '--continue works after a conflict' '
 	(
 	cd clone2 &&
@@ -138,15 +96,4 @@ test_expect_success 'rebase -p preserves no-ff merges' '
 	)
 '
 
-test_expect_success 'rebase -p works when base inside second parent' '
-	(
-	cd clone4 &&
-	git fetch &&
-	git rebase -p HEAD^2 &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify A" | wc -l) &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Modify B" | wc -l) &&
-	test 1 = $(git rev-list --all --pretty=oneline | grep "Merge remote-tracking branch " | wc -l)
-	)
-'
-
 test_done
diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
new file mode 100755
index 0000000..5400a05
--- /dev/null
+++ b/t/t3425-rebase-topology-merges.sh
@@ -0,0 +1,258 @@
+#!/bin/sh
+
+test_description='rebase topology tests with merges'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_revision_subjects () {
+	expected="$1"
+	shift
+	set -- $(git log --format=%s --no-walk=unsorted "$@")
+	test "$expected" = "$*"
+}
+
+# a---b-----------c
+#      \           \
+#       d-------e   \
+#        \       \   \
+#         n---o---w---v
+#              \
+#               z
+test_expect_success 'setup of non-linear-history' '
+	test_commit a &&
+	test_commit b &&
+	test_commit c &&
+	git checkout b &&
+	test_commit d &&
+	test_commit e
+
+	git checkout c &&
+	test_commit g &&
+	revert h g &&
+	git checkout d &&
+	cherry_pick G g &&
+	test_commit i &&
+	git checkout b &&
+	test_commit f
+
+	git checkout d &&
+	test_commit n &&
+	test_commit o &&
+	test_merge w e &&
+	test_merge v c &&
+	git checkout o &&
+	test_commit z
+'
+
+test_run_rebase () {
+	result=$1
+	shift
+	test_expect_$result "rebase $* after merge from upstream" "
+		reset_rebase &&
+		git rebase $* e w &&
+		test_cmp_rev e HEAD~2 &&
+		test_linear_range 'n o' e..
+	"
+}
+test_run_rebase success ''
+test_run_rebase success -m
+test_run_rebase success -i
+
+test_run_rebase () {
+	result=$1
+	shift
+	expected=$1
+	shift
+	test_expect_$result "rebase $* of non-linear history is linearized in place" "
+		reset_rebase &&
+		git rebase $* d w &&
+		test_cmp_rev d HEAD~3 &&
+		test_linear_range "\'"$expected"\'" d..
+	"
+}
+#TODO: make order consistent across all flavors of rebase
+test_run_rebase success 'e n o' ''
+test_run_rebase success 'e n o' -m
+test_run_rebase success 'n o e' -i
+
+test_run_rebase () {
+	result=$1
+	shift
+	expected=$1
+	shift
+	test_expect_$result "rebase $* of non-linear history is linearized upstream" "
+		reset_rebase &&
+		git rebase $* c w &&
+		test_cmp_rev c HEAD~4 &&
+		test_linear_range "\'"$expected"\'" c..
+	"
+}
+#TODO: make order consistent across all flavors of rebase
+test_run_rebase success 'd e n o' ''
+test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' -i
+
+test_run_rebase () {
+	result=$1
+	shift
+	expected=$1
+	shift
+	test_expect_$result "rebase $* of non-linear history with merges after upstream merge is linearized" "
+		reset_rebase &&
+		git rebase $* c v &&
+		test_cmp_rev c HEAD~4 &&
+		test_linear_range "\'"$expected"\'" c..
+	"
+}
+#TODO: make order consistent across all flavors of rebase
+test_run_rebase success 'd e n o' ''
+test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' -i
+
+test_expect_success "rebase -p is no-op in non-linear history" "
+	reset_rebase &&
+	git rebase -p d w &&
+	test_cmp_rev w HEAD
+"
+
+test_expect_success "rebase -p is no-op when base inside second parent" "
+	reset_rebase &&
+	git rebase -p e w &&
+	test_cmp_rev w HEAD
+"
+
+test_expect_failure "rebase -p --root on non-linear history is a no-op" "
+	reset_rebase &&
+	git rebase -p --root w &&
+	test_cmp_rev w HEAD
+"
+
+test_expect_success "rebase -p re-creates merge from side branch" "
+	reset_rebase &&
+	git rebase -p z w &&
+	test_cmp_rev z HEAD^ &&
+	test_cmp_rev w^2 HEAD^2
+"
+
+test_expect_success "rebase -p re-creates internal merge" "
+	reset_rebase &&
+	git rebase -p c w &&
+	test_cmp_rev c HEAD~4 &&
+	test_cmp_rev HEAD^2^ HEAD~3 &&
+	test_revision_subjects 'd n e o w' HEAD~3 HEAD~2 HEAD^2 HEAD^ HEAD
+"
+
+test_expect_success "rebase -p can re-create two branches on onto" "
+	reset_rebase &&
+	git rebase -p --onto c d w &&
+	test_cmp_rev c HEAD~3 &&
+	test_cmp_rev c HEAD^2^ &&
+	test_revision_subjects 'n e o w' HEAD~2 HEAD^2 HEAD^ HEAD
+"
+
+#       f
+#      /
+# a---b---c---g---h
+#      \
+#       d---G---i
+#        \       \
+#         e-------u
+#
+# uppercase = cherry-picked
+# h = reverted g
+test_expect_success 'setup of non-linear-history for patch-equivalence tests' '
+	git checkout e &&
+	test_merge u i
+'
+
+test_expect_success "rebase -p re-creates history around dropped commit matching upstream" "
+	reset_rebase &&
+	git rebase -p h u &&
+	test_cmp_rev h HEAD~3 &&
+	test_cmp_rev HEAD^2^ HEAD~2 &&
+	test_revision_subjects 'd i e u' HEAD~2 HEAD^2 HEAD^ HEAD
+"
+
+test_expect_success "rebase -p --onto in merged history drops patches in upstream" "
+	reset_rebase &&
+	git rebase -p --onto f h u &&
+	test_cmp_rev f HEAD~3 &&
+	test_cmp_rev HEAD^2^ HEAD~2 &&
+	test_revision_subjects 'd i e u' HEAD~2 HEAD^2 HEAD^ HEAD
+"
+
+test_expect_success "rebase -p --onto in merged history does not drop patches in onto" "
+	reset_rebase &&
+	git rebase -p --onto h f u &&
+	test_cmp_rev h HEAD~3 &&
+	test_cmp_rev HEAD^2~2 HEAD~2 &&
+	test_revision_subjects 'd G i e u' HEAD~2 HEAD^2^ HEAD^2 HEAD^ HEAD
+"
+
+# a---b---c---g---h
+#      \
+#       d---G---s
+#        \   \ /
+#         \   X
+#          \ / \
+#           e---t
+#
+# uppercase = cherry-picked
+# h = reverted g
+test_expect_success 'setup of non-linear-history for dropping whole side' '
+	git checkout G &&
+	test_merge s e &&
+	git checkout e &&
+	test_merge t G
+'
+
+test_expect_failure "rebase -p drops merge commit when entire first-parent side is dropped" "
+	reset_rebase &&
+	git rebase -p h s &&
+	test_cmp_rev h HEAD~2 &&
+	test_linear_range 'd e' h..
+"
+
+test_expect_success "rebase -p drops merge commit when entire second-parent side is dropped" "
+	reset_rebase &&
+	git rebase -p h t &&
+	test_cmp_rev h HEAD~2 &&
+	test_linear_range 'd e' h..
+"
+
+# a---b---c
+#      \
+#       d---e
+#        \   \
+#         n---r
+#          \
+#           o
+#
+# r = tree-same with n
+test_expect_success 'setup of non-linear-history for empty commits' '
+	git checkout n &&
+	git merge --no-commit e &&
+	git reset n . &&
+	git commit -m r &&
+	git reset --hard &&
+	git clean -f &&
+	git tag r
+'
+
+test_expect_success "rebase -p re-creates empty internal merge commit" "
+	reset_rebase &&
+	git rebase -p c r &&
+	test_cmp_rev c HEAD~3 &&
+	test_cmp_rev HEAD^2^ HEAD~2 &&
+	test_revision_subjects 'd e n r' HEAD~2 HEAD^2 HEAD^ HEAD
+"
+
+test_expect_success "rebase -p re-creates empty merge commit" "
+	reset_rebase &&
+	git rebase -p o r &&
+	test_cmp_rev e HEAD^2 &&
+	test_cmp_rev o HEAD^ &&
+	test_revision_subjects 'r' HEAD
+"
+
+test_done
-- 
1.8.3.698.g079b096

[PATCH v4 43/45] t3406: modernize style

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

Update the following:

 - Quote 'setup'
 - Remove blank lines within test case body
 - Use test_commit instead of custom quick_one
 - Create branch "topic" from tag created by test_commit

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Conflicts:
	t/t3406-rebase-message.sh
---
 t/t3406-rebase-message.sh | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 37d19c5..7f48d0f 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -4,27 +4,17 @@ test_description='messages from rebase operation'
 
 . ./test-lib.sh
 
-quick_one () {
-	echo "$1" >"file$1" &&
-	git add "file$1" &&
-	test_tick &&
-	git commit -m "$1"
-}
-
-test_expect_success setup '
-	quick_one O &&
-	git branch topic &&
-	quick_one X &&
-	quick_one A &&
-	quick_one B &&
-	quick_one Y &&
-
-	git checkout topic &&
-	quick_one A &&
-	quick_one B &&
-	quick_one Z &&
+test_expect_success 'setup' '
+	test_commit O fileO &&
+	test_commit X fileX &&
+	test_commit A fileA &&
+	test_commit B fileB &&
+	test_commit Y fileY &&
+
+	git checkout -b topic O &&
+	git cherry-pick A B &&
+	test_commit Z fileZ &&
 	git tag start
-
 '
 
 test_expect_success 'rebase --stat' '
-- 
1.8.3.698.g079b096

[PATCH v4 44/45] tests: move test for rebase messages from t3400 to t3406

[Felipe Contreras]

From: Martin von Zweigbergk <martinvonz@gmail.com>

t3406 is supposed to test "messages from rebase operation", so let's
move tests in t3400 that fit that description into 3406. Most of the
functionality they tested, except for the messages, has now been
subsumed by t3420.

Signed-off-by: Martin von Zweigbergk <martinvonz@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

Conflicts:
	t/t3406-rebase-message.sh
---
 t/t3400-rebase.sh         | 22 ----------------------
 t/t3406-rebase-message.sh | 23 +++++++++++++++++++++++
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index b436ef4..45a55e9 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -59,28 +59,6 @@ test_expect_success 'rebase against master' '
 	git rebase master
 '
 
-test_expect_success 'rebase against master twice' '
-	git rebase master >out &&
-	test_i18ngrep "Current branch my-topic-branch is up to date" out
-'
-
-test_expect_success 'rebase against master twice with --force' '
-	git rebase --force-rebase master >out &&
-	test_i18ngrep "Current branch my-topic-branch is up to date, rebase forced" out
-'
-
-test_expect_success 'rebase against master twice from another branch' '
-	git checkout my-topic-branch^ &&
-	git rebase master my-topic-branch >out &&
-	test_i18ngrep "Current branch my-topic-branch is up to date" out
-'
-
-test_expect_success 'rebase fast-forward to master' '
-	git checkout my-topic-branch^ &&
-	git rebase my-topic-branch >out &&
-	test_i18ngrep "Fast-forwarded HEAD to my-topic-branch" out
-'
-
 test_expect_success 'the rebase operation should not have destroyed author information' '
 	! (git log | grep "Author:" | grep "<>")
 '
diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
index 7f48d0f..f03cc5f 100755
--- a/t/t3406-rebase-message.sh
+++ b/t/t3406-rebase-message.sh
@@ -17,6 +17,29 @@ test_expect_success 'setup' '
 	git tag start
 '
 
+test_expect_success 'rebase against master twice' '
+	git rebase -m master &&
+	git rebase master >out &&
+	test_i18ngrep "Current branch topic is up to date" out
+'
+
+test_expect_success 'rebase against master twice with --force' '
+	git rebase --force-rebase master >out &&
+	test_i18ngrep "Current branch topic is up to date, rebase forced" out
+'
+
+test_expect_success 'rebase against master twice from another branch' '
+	git checkout topic^ &&
+	git rebase master topic >out &&
+	test_i18ngrep "Current branch topic is up to date" out
+'
+
+test_expect_success 'rebase fast-forward to master' '
+	git checkout topic^ &&
+	git rebase topic >out &&
+	test_i18ngrep "Fast-forwarded HEAD to topic" out
+'
+
 test_expect_success 'rebase --stat' '
 	git reset --hard start &&
         git rebase --stat master >diffstat.txt &&
-- 
1.8.3.698.g079b096

[PATCH v4 45/45] tests: update topology tests

[Felipe Contreras]

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t3425-rebase-topology-merges.sh | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
index 5400a05..96cc479 100755
--- a/t/t3425-rebase-topology-merges.sh
+++ b/t/t3425-rebase-topology-merges.sh
@@ -70,9 +70,8 @@ test_run_rebase () {
 		test_linear_range "\'"$expected"\'" d..
 	"
 }
-#TODO: make order consistent across all flavors of rebase
-test_run_rebase success 'e n o' ''
-test_run_rebase success 'e n o' -m
+test_run_rebase success 'n o e' ''
+test_run_rebase success 'n o e' -m
 test_run_rebase success 'n o e' -i
 
 test_run_rebase () {
@@ -87,9 +86,8 @@ test_run_rebase () {
 		test_linear_range "\'"$expected"\'" c..
 	"
 }
-#TODO: make order consistent across all flavors of rebase
-test_run_rebase success 'd e n o' ''
-test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' ''
+test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
 test_run_rebase () {
@@ -104,9 +102,8 @@ test_run_rebase () {
 		test_linear_range "\'"$expected"\'" c..
 	"
 }
-#TODO: make order consistent across all flavors of rebase
-test_run_rebase success 'd e n o' ''
-test_run_rebase success 'd e n o' -m
+test_run_rebase success 'd n o e' ''
+test_run_rebase success 'd n o e' -m
 test_run_rebase success 'd n o e' -i
 
 test_expect_success "rebase -p is no-op in non-linear history" "
-- 
1.8.3.698.g079b096

Re: [PATCH v4 06/45] Move sequencer to builtin

[Antoine Pelisse]

On Sun, Jun 9, 2013 at 6:40 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> This code is only useful for cherry-pick and revert built-ins, nothing
> else, so let's make it a builtin object.
>
> The first source file that doesn't generate a git-foo builtin, but does
> go into the builtin library. Hopefully the first of many to clean
> libgit.a.

Hey Felipe,
I don't understand why the code doesn't belong to libgit.a, and how
it's gonna make it more "clean". I can see that it is needed only by
revert and cherry-pick, but is that the only reason ?

Thanks for taking the time to enlighten me :)

Re: [PATCH v4 06/45] Move sequencer to builtin

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:02 PM, Antoine Pelisse <apelisse@gmail.com> wrote:
> On Sun, Jun 9, 2013 at 6:40 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>>
>> This code is only useful for cherry-pick and revert built-ins, nothing
>> else, so let's make it a builtin object.
>>
>> The first source file that doesn't generate a git-foo builtin, but does
>> go into the builtin library. Hopefully the first of many to clean
>> libgit.a.
>
> Hey Felipe,
> I don't understand why the code doesn't belong to libgit.a, and how
> it's gonna make it more "clean". I can see that it is needed only by
> revert and cherry-pick, but is that the only reason ?
>
> Thanks for taking the time to enlighten me :)

A libgit library should be useful for things other than builtin
commands. Eventually libgit.a should be similar to libgit2, therefore
if libgit2 wouldn't want sequencer.c in it (it doesn't) neither should
we want this code in libgit.a. It belongs in builtin/lib.a.

If we don't start moving non-library stuff out of libgit.a, libgit.a
will never be a library.

-- 
Felipe Contreras

Re: [PATCH v4 06/45] Move sequencer to builtin

[Ramkumar Ramachandra]

Antoine Pelisse wrote:
> I don't understand why the code doesn't belong to libgit.a, and how
> it's gonna make it more "clean". I can see that it is needed only by
> revert and cherry-pick, but is that the only reason ?

Perhaps this will help? [1].  Join into the discussion.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/226845

Re: [PATCH v4 06/45] Move sequencer to builtin

[Antoine Pelisse]

On Sun, Jun 9, 2013 at 7:07 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
> Antoine Pelisse wrote:
>> I don't understand why the code doesn't belong to libgit.a, and how
>> it's gonna make it more "clean". I can see that it is needed only by
>> revert and cherry-pick, but is that the only reason ?
>
> Perhaps this will help? [1].  Join into the discussion.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/226845

OK Thanks, missed that. I'm catching up the last two hours of discussions.

Re: [PATCH v4 03/45] build: trivial cleanup

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 11:40:15AM -0500, Felipe Contreras wrote:
> There's no need to list again the prerequisites.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

Please write more shortlog-friendly subject lines describing what the
actual change is.

In this case for example: "build: substitute library prerequisites
using $^"

Re: [PATCH v4 10/45] sequencer: trivial fix

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
> We should free objects before leaving.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

A shortlog-friendlier subject could be: "sequencer: free objects
before leaving".

Re: [PATCH v4 20/45] cherry-pick: copy notes and run hooks

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +static void finish(struct replay_opts *opts)
> +{
> +	if (opts->action != REPLAY_PICK)
> +		return;
> +
> +	run_rewrite_hook(&rewritten, "cherry-pick");
> +	copy_rewrite_notes(&rewritten, "cherry-pick");
> +}
> +

Ok, so I see that with the previous two commits, you automatically get
handling of the notes.rewrite.cherry-pick variable and friends.  This is
good.

However, there are some open points:

* The docs in git-config(1) "notes.rewrite.cherry-pick" and githooks(5)
  "post-rewrite" and are now stale in so far as they contain a list of
  commands doing rewriting.

* This pretends to be cherry-pick even when the hook is called from
  rebase.

  We could claim (and document) that git-rebase with certain options
  shall be the same as running cherry-pick with some other options.
  However, git-am already goes out of its way to ensure that it only
  does the rewriting/post-rewrite if called from rebase (so that the
  user-facing git-am command is not affected).  So it's more consistent
  to ensure that git-cherry-pick, when called from rebase, also pretends
  to be rebase.

* githooks(5) documents explicitly that by the time post-rewrite is
  called, the notes have been rewritten.  Your change does it in the
  opposite order.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>> We should free objects before leaving.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> A shortlog-friendlier subject could be: "sequencer: free objects
> before leaving".

I already defended my rationale for this succinct commit message:

http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610

-- 
Felipe Contreras

Re: [PATCH v4 42/45] add tests for rebasing merged history

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> +#TODO: make order consistent across all flavors of rebase
> +test_run_rebase success 'd e n o' ''
> +test_run_rebase success 'd e n o' -m
> +test_run_rebase success 'd n o e' -i

[45/45] would seem to imply that these tests fail as of this patch.  Is
that correct?

If so, please either squash that change or move the test further up
marked as 'failure'.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v4 20/45] cherry-pick: copy notes and run hooks

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:22 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +static void finish(struct replay_opts *opts)
>> +{
>> +     if (opts->action != REPLAY_PICK)
>> +             return;
>> +
>> +     run_rewrite_hook(&rewritten, "cherry-pick");
>> +     copy_rewrite_notes(&rewritten, "cherry-pick");
>> +}
>> +
>
> Ok, so I see that with the previous two commits, you automatically get
> handling of the notes.rewrite.cherry-pick variable and friends.  This is
> good.
>
> However, there are some open points:
>
> * The docs in git-config(1) "notes.rewrite.cherry-pick" and githooks(5)
>   "post-rewrite" and are now stale in so far as they contain a list of
>   commands doing rewriting.

Fine.

--- a/builtin/sequencer.c
+++ b/builtin/sequencer.c
@@ -28,9 +28,9 @@ static void finish(struct replay_opts *opts)
        if (opts->action != REPLAY_PICK)
                return;

-       name = opts->action_name ? opts->action_name : "cherry-pick";
+       name = opts->action_name

-       if (!*name)
+       if (!name || !*name)
                return;

        run_rewrite_hook(&rewritten, name);

Now, we won't run when 'git cherry-pick' is called, only when an
action-name is specified; when called from 'git rebase'.

> * This pretends to be cherry-pick even when the hook is called from
>   rebase.

No.

http://mid.gmane.org/1370796057-25312-31-git-send-email-felipe.contreras@gmail.com

> * githooks(5) documents explicitly that by the time post-rewrite is
>   called, the notes have been rewritten.  Your change does it in the
>   opposite order.

OK.

But it doesn't matter, because the patch won't be applied.

-- 
Felipe Contreras

Re: [PATCH v4 42/45] add tests for rebasing merged history

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:25 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> +#TODO: make order consistent across all flavors of rebase
>> +test_run_rebase success 'd e n o' ''
>> +test_run_rebase success 'd e n o' -m
>> +test_run_rebase success 'd n o e' -i
>
> [45/45] would seem to imply that these tests fail as of this patch.  Is
> that correct?
>
> If so, please either squash that change or move the test further up
> marked as 'failure'.

I won't. The whole purpose of the last patch is to show these tests are fixed.

Martin von Zweigbergk's patch series is independent of this one, I
merely added it to show the above.

Eventually Martin's patch series will be merged, while mine probably won't.

-- 
Felipe Contreras

Re: [PATCH v4 42/45] add tests for rebasing merged history

[Thomas Rast]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> On Sun, Jun 9, 2013 at 12:25 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> +#TODO: make order consistent across all flavors of rebase
>>> +test_run_rebase success 'd e n o' ''
>>> +test_run_rebase success 'd e n o' -m
>>> +test_run_rebase success 'd n o e' -i
>>
>> [45/45] would seem to imply that these tests fail as of this patch.  Is
>> that correct?
>>
>> If so, please either squash that change or move the test further up
>> marked as 'failure'.
>
> I won't. The whole purpose of the last patch is to show these tests are fixed.
>
> Martin von Zweigbergk's patch series is independent of this one, I
> merely added it to show the above.

So you would deliberately break a bisection on this test file?

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v4 10/45] sequencer: trivial fix

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
> >> We should free objects before leaving.
> >>
> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >
> > A shortlog-friendlier subject could be: "sequencer: free objects
> > before leaving".
> 
> I already defended my rationale for this succinct commit message:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610

Your arguments were unconvincing.  The mere fact that I raised this
issue unbeknownst to the earlier posting clearly shows that there's
demand for descriptive subjects.

Re: [PATCH v4 10/45] sequencer: trivial fix

[John Keeping]

On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote:
> On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
> > On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
> > >> We should free objects before leaving.
> > >>
> > >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > >
> > > A shortlog-friendlier subject could be: "sequencer: free objects
> > > before leaving".
> > 
> > I already defended my rationale for this succinct commit message:
> > 
> > http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
> 
> Your arguments were unconvincing.  The mere fact that I raised this
> issue unbeknownst to the earlier posting clearly shows that there's
> demand for descriptive subjects.

Not to mention that with your subject no body is needed, making the
overall message more succinct.

When reading a log, as soon as I see "trivial" I become suspicious that
someone is trying to cover something up, much like "left as an exercise
for the reader".  If the subject says "fix memory leak" then it's
obvious what the patch is meant to do, and when there is no subtlety to
be explained (as there isn't in this patch) there is no need for a body.

Re: [PATCH v4 42/45] add tests for rebasing merged history

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:32 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> On Sun, Jun 9, 2013 at 12:25 PM, Thomas Rast <trast@inf.ethz.ch> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> +#TODO: make order consistent across all flavors of rebase
>>>> +test_run_rebase success 'd e n o' ''
>>>> +test_run_rebase success 'd e n o' -m
>>>> +test_run_rebase success 'd n o e' -i
>>>
>>> [45/45] would seem to imply that these tests fail as of this patch.  Is
>>> that correct?
>>>
>>> If so, please either squash that change or move the test further up
>>> marked as 'failure'.
>>
>> I won't. The whole purpose of the last patch is to show these tests are fixed.
>>
>> Martin von Zweigbergk's patch series is independent of this one, I
>> merely added it to show the above.
>
> So you would deliberately break a bisection on this test file?

No, this patch series won't be applied.

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:33 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
>> On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>> >> We should free objects before leaving.
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >
>> > A shortlog-friendlier subject could be: "sequencer: free objects
>> > before leaving".
>>
>> I already defended my rationale for this succinct commit message:
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
>
> Your arguments were unconvincing.

That's your opinion. The commit message I write and send is my decision.

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:37 PM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote:
>> On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
>> > On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> > > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>> > >> We should free objects before leaving.
>> > >>
>> > >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> > >
>> > > A shortlog-friendlier subject could be: "sequencer: free objects
>> > > before leaving".
>> >
>> > I already defended my rationale for this succinct commit message:
>> >
>> > http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
>>
>> Your arguments were unconvincing.  The mere fact that I raised this
>> issue unbeknownst to the earlier posting clearly shows that there's
>> demand for descriptive subjects.
>
> Not to mention that with your subject no body is needed, making the
> overall message more succinct.

It's not succinct at all, because there's no short and quick
description of what the patch actually is; a trivial fix.

> When reading a log, as soon as I see "trivial" I become suspicious that
> someone is trying to cover something up, much like "left as an exercise
> for the reader".  If the subject says "fix memory leak" then it's
> obvious what the patch is meant to do, and when there is no subtlety to
> be explained (as there isn't in this patch) there is no need for a body.

You are not a rational person then. The commit message has absolutely
no bearing on the quality of the code. If you are less suspicious of a
commit message that says "fix memory leak", you are being completely
biased.

Whether the commit message says "fix memory leak", or "trivial fix",
or "foobar", the code might still be doing something wrong, and you
can't decide that until you look at the code.

If you don't care about the code, but still want to know what the
patch is doing, then you can look at the whole commit message, and "We
should free objects before leaving." explains that perfectly.

For the people that only read the summary, the vast majority of them
need to know what this patch is, not what it does, and when they see
"trivial fix" they most likely can skip it.

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 12:47:16PM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 12:33 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> > On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
> >> On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> >> > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
> >> >> We should free objects before leaving.
> >> >>
> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> >
> >> > A shortlog-friendlier subject could be: "sequencer: free objects
> >> > before leaving".
> >>
> >> I already defended my rationale for this succinct commit message:
> >>
> >> http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
> >
> > Your arguments were unconvincing.
> 
> That's your opinion.

And Duy's.  And John's, too, apparently.

> The commit message I write and send is my decision.

It's always a pleasure to work with you.

Guess I should've known better.


Bye,
Gábor

Re: [PATCH v4 09/45] sequencer: remove useless indentation

[Fredrik Gustafsson]

On Sun, Jun 09, 2013 at 11:40:21AM -0500, Felipe Contreras wrote:
> By using good ol' goto.
> 
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

I don't see this as an indentation change but as a restructing of the
code. I would prefer something more like
"Replace early return with goto cleanup" (but better phrased).

The same goes for the next patch in this serie, I would prefer if the
shortlog tells what have been done, not how hard it was to do. Even
trivial changes can introduce bugs and when the commit message is just
that "this is trivial" it forces me to read the diff to know if that's
something that can effect me or not.

> +leave:
>  	free_message(&msg);
>  	free(defmsg);

leave: should be cleanup: or out: to conform with already written code. I
suppose there will be a lot of those changes from now on and it will be
easier if the name of the cleanup-label always is the same.

Maybe small things to review, but I think those things will lead to a
better code-base in the long term.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 09/45] sequencer: remove useless indentation

[Felipe Contreras]

On Sun, Jun 9, 2013 at 1:17 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> On Sun, Jun 09, 2013 at 11:40:21AM -0500, Felipe Contreras wrote:
>> By using good ol' goto.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>
> I don't see this as an indentation change but as a restructing of the
> code. I would prefer something more like
> "Replace early return with goto cleanup" (but better phrased).

The explains what the patch is doing, but not why. Why is more important.

> The same goes for the next patch in this serie, I would prefer if the
> shortlog tells what have been done, not how hard it was to do.

It explains what it is, and why it makes sense.

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

On Sun, Jun 9, 2013 at 12:53 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sun, Jun 09, 2013 at 12:47:16PM -0500, Felipe Contreras wrote:
>> On Sun, Jun 9, 2013 at 12:33 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> > On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
>> >> On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> >> > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>> >> >> We should free objects before leaving.
>> >> >>
>> >> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> >
>> >> > A shortlog-friendlier subject could be: "sequencer: free objects
>> >> > before leaving".
>> >>
>> >> I already defended my rationale for this succinct commit message:
>> >>
>> >> http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
>> >
>> > Your arguments were unconvincing.
>>
>> That's your opinion.
>
> And Duy's.  And John's, too, apparently.

And it could the opinion of a thousand more people, it doesn't make it
anything more than an opinion. To think anything else is to fall in
argumentum ad populum.

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[John Keeping]

On Sun, Jun 09, 2013 at 12:53:38PM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 12:37 PM, John Keeping <john@keeping.me.uk> wrote:
> > On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote:
> >> On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
> >> > On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> >> > > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
> >> > >> We should free objects before leaving.
> >> > >>
> >> > >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> >> > >
> >> > > A shortlog-friendlier subject could be: "sequencer: free objects
> >> > > before leaving".
> >> >
> >> > I already defended my rationale for this succinct commit message:
> >> >
> >> > http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
> >>
> >> Your arguments were unconvincing.  The mere fact that I raised this
> >> issue unbeknownst to the earlier posting clearly shows that there's
> >> demand for descriptive subjects.
> >
> > Not to mention that with your subject no body is needed, making the
> > overall message more succinct.
> 
> It's not succinct at all, because there's no short and quick
> description of what the patch actually is; a trivial fix.

Is it not equally succinct to say "fix memory leak"?

> > When reading a log, as soon as I see "trivial" I become suspicious that
> > someone is trying to cover something up, much like "left as an exercise
> > for the reader".  If the subject says "fix memory leak" then it's
> > obvious what the patch is meant to do, and when there is no subtlety to
> > be explained (as there isn't in this patch) there is no need for a body.
> 
> You are not a rational person then. The commit message has absolutely
> no bearing on the quality of the code. If you are less suspicious of a
> commit message that says "fix memory leak", you are being completely
> biased.
>
> Whether the commit message says "fix memory leak", or "trivial fix",
> or "foobar", the code might still be doing something wrong, and you
> can't decide that until you look at the code.

I have a certain level of trust that commit summaries in git.git will be
accurate.  If I want to know what has changed, then "fix memory leak"
implies "no functional change"; if I see "trivial fix" then how do I
know what that is?  It could be a whitespace change, a fix to a memory
leak, a typo correction, a change to a separator in a message shown to
the user, or even a small change to corner case behaviour.

> If you don't care about the code, but still want to know what the
> patch is doing, then you can look at the whole commit message, and "We
> should free objects before leaving." explains that perfectly.

The short message is what appears in "What's Cooking", why should I need
to break out of my mail client to find out what it means?

Re: [PATCH v4 09/45] sequencer: remove useless indentation

[Fredrik Gustafsson]

On Sun, Jun 09, 2013 at 01:19:03PM -0500, Felipe Contreras wrote:
> The explains what the patch is doing, but not why. Why is more important.

You're right. Why are the indentation useless? It doesn't seem to be
useless until you added goto. So why is your goto solution better than
the previous existing solution?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 10/45] sequencer: trivial fix

[Felipe Contreras]

On Sun, Jun 9, 2013 at 2:01 PM, John Keeping <john@keeping.me.uk> wrote:
> On Sun, Jun 09, 2013 at 12:53:38PM -0500, Felipe Contreras wrote:
>> On Sun, Jun 9, 2013 at 12:37 PM, John Keeping <john@keeping.me.uk> wrote:
>> > On Sun, Jun 09, 2013 at 07:33:42PM +0200, SZEDER Gábor wrote:
>> >> On Sun, Jun 09, 2013 at 12:23:01PM -0500, Felipe Contreras wrote:
>> >> > On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> >> > > On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>> >> > >> We should free objects before leaving.
>> >> > >>
>> >> > >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> > >
>> >> > > A shortlog-friendlier subject could be: "sequencer: free objects
>> >> > > before leaving".
>> >> >
>> >> > I already defended my rationale for this succinct commit message:
>> >> >
>> >> > http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
>> >>
>> >> Your arguments were unconvincing.  The mere fact that I raised this
>> >> issue unbeknownst to the earlier posting clearly shows that there's
>> >> demand for descriptive subjects.
>> >
>> > Not to mention that with your subject no body is needed, making the
>> > overall message more succinct.
>>
>> It's not succinct at all, because there's no short and quick
>> description of what the patch actually is; a trivial fix.
>
> Is it not equally succinct to say "fix memory leak"?

Almost. "fix memory leak" doesn't say anything about the scope; it can
be a huge change, or a trivial one.

Perhaps "trivial memory leak fix" would be better.

>> > When reading a log, as soon as I see "trivial" I become suspicious that
>> > someone is trying to cover something up, much like "left as an exercise
>> > for the reader".  If the subject says "fix memory leak" then it's
>> > obvious what the patch is meant to do, and when there is no subtlety to
>> > be explained (as there isn't in this patch) there is no need for a body.
>>
>> You are not a rational person then. The commit message has absolutely
>> no bearing on the quality of the code. If you are less suspicious of a
>> commit message that says "fix memory leak", you are being completely
>> biased.
>>
>> Whether the commit message says "fix memory leak", or "trivial fix",
>> or "foobar", the code might still be doing something wrong, and you
>> can't decide that until you look at the code.
>
> I have a certain level of trust that commit summaries in git.git will be
> accurate.  If I want to know what has changed, then "fix memory leak"
> implies "no functional change"; if I see "trivial fix" then how do I
> know what that is?

It is a trivial fix, that's what it is. You don't need to bother
yourself with it. Unless you plan to see the code.

> It could be a whitespace change,

That's not a fix, that's a cleanup.

> a fix to a memory leak, a typo correction, a change to a separator in a message shown to
> the user,

You might be right, but I don't think you _need_ to know which one of
them it is; they are all trivial. In 90% of the cases you want to skip
them and keep reading. In the 10% where you do need more, well, you
probably need to look at the code either way.

> or even a small change to corner case behaviour.

That's not trivial.

>> If you don't care about the code, but still want to know what the
>> patch is doing, then you can look at the whole commit message, and "We
>> should free objects before leaving." explains that perfectly.
>
> The short message is what appears in "What's Cooking", why should I need
> to break out of my mail client to find out what it means?

You don't, it's a trivial fix, and you said you have a certain level
of trust on commit summaries ;)

-- 
Felipe Contreras

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Fredrik Gustafsson]

On Sun, Jun 09, 2013 at 11:40:43AM -0500, Felipe Contreras wrote:
> -		git_am_opt="$git_am_opt -q"

This one makes me wonder a bit. I'm not sure about how this works in
git, so I would appriciate if someone can explain. I might also have
misunderstood something.

Here we have two sh-scripts (git rebase
and git am) interacting witch eachother. Both uses GIT_QUIET, so if
GIT_QUIET already is set by the caller (git rebase) the callee doesn't
have to set it to.

However GIT_QUIET is undocumented for git-am (in Git Manual). If we
translate git am to C/ruby/python/perl/etc. will we catch this?

This raises a few more generall questions:
do we already pass information between processes(!) with enviroment
variables? And is this documented the way it should be?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 09/45] sequencer: remove useless indentation

[Felipe Contreras]

On Sun, Jun 9, 2013 at 2:08 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> On Sun, Jun 09, 2013 at 01:19:03PM -0500, Felipe Contreras wrote:
>> The explains what the patch is doing, but not why. Why is more important.
>
> You're right. Why are the indentation useless? It doesn't seem to be
> useless until you added goto. So why is your goto solution better than
> the previous existing solution?

Because it removes useless indentation :)

This is what they do in the Linux kernel, you tell me which looks better:

a)

	if (function1())
		goto leave;
	if (function2())
		goto leave;
	if (function3())
		goto leave;
	if (function4())
		goto leave;
	good_stuff();
leave:
	final_stuff();

or b)

	if (!function1()) {
		if (!function2()) {
			if (!function3()) {
				if (!function4()) {
					good_stuff();
				}
			}
		}
	}
	final_stuff();

-- 
Felipe Contreras

Re: [PATCH v4 10/45] sequencer: trivial fix

[SZEDER Gábor]

On Sun, Jun 09, 2013 at 02:11:18PM -0500, Felipe Contreras wrote:
> Perhaps "trivial memory leak fix" would be better.

That's fine with me, thanks.

If only we could have got there like 10 emails ago.


Gábor

Re: [PATCH v4 11/45] cherry-pick: don't barf when there's nothing to do

[Fredrik Gustafsson]

On Sun, Jun 09, 2013 at 11:40:23AM -0500, Felipe Contreras wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/sequencer.c             | 4 ++--
>  t/t3510-cherry-pick-sequence.sh | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/sequencer.c b/builtin/sequencer.c
> index 23b01b7..4d7dc8b 100644
> --- a/builtin/sequencer.c
> +++ b/builtin/sequencer.c
> @@ -565,8 +565,8 @@ static void prepare_revs(struct replay_opts *opts)
>  	if (prepare_revision_walk(opts->revs))
>  		die(_("revision walk setup failed"));
>  
> -	if (!opts->revs->commits)
> -		die(_("empty commit set passed"));
> +	if (!opts->revs->commits && !opts->quiet)
> +		error(_("empty commit set passed"));
>  }
>  
>  static void read_and_refresh_cache(struct replay_opts *opts)
> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
> index 7b7a89d..33c5512 100755
> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -472,7 +472,7 @@ test_expect_success 'malformed instruction sheet 2' '
>  
>  test_expect_success 'empty commit set' '
>  	pristine_detach initial &&
> -	test_expect_code 128 git cherry-pick base..base
> +	git cherry-pick base..base

Shouldn't this result in the error "empty commit set passed"? If so,
shouldn't that be checked to actually print that error?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 11/45] cherry-pick: don't barf when there's nothing to do

[Felipe Contreras]

On Sun, Jun 9, 2013 at 2:21 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> On Sun, Jun 09, 2013 at 11:40:23AM -0500, Felipe Contreras wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  builtin/sequencer.c             | 4 ++--
>>  t/t3510-cherry-pick-sequence.sh | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/builtin/sequencer.c b/builtin/sequencer.c
>> index 23b01b7..4d7dc8b 100644
>> --- a/builtin/sequencer.c
>> +++ b/builtin/sequencer.c
>> @@ -565,8 +565,8 @@ static void prepare_revs(struct replay_opts *opts)
>>       if (prepare_revision_walk(opts->revs))
>>               die(_("revision walk setup failed"));
>>
>> -     if (!opts->revs->commits)
>> -             die(_("empty commit set passed"));
>> +     if (!opts->revs->commits && !opts->quiet)
>> +             error(_("empty commit set passed"));
>>  }
>>
>>  static void read_and_refresh_cache(struct replay_opts *opts)
>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>> index 7b7a89d..33c5512 100755
>> --- a/t/t3510-cherry-pick-sequence.sh
>> +++ b/t/t3510-cherry-pick-sequence.sh
>> @@ -472,7 +472,7 @@ test_expect_success 'malformed instruction sheet 2' '
>>
>>  test_expect_success 'empty commit set' '
>>       pristine_detach initial &&
>> -     test_expect_code 128 git cherry-pick base..base
>> +     git cherry-pick base..base
>
> Shouldn't this result in the error "empty commit set passed"? If so,
> shouldn't that be checked to actually print that error?

Probably, yeah.

-- 
Felipe Contreras

Re: [PATCH v4 09/45] sequencer: remove useless indentation

[Fredrik Gustafsson]

On Sun, Jun 09, 2013 at 02:16:29PM -0500, Felipe Contreras wrote:
> On Sun, Jun 9, 2013 at 2:08 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> > On Sun, Jun 09, 2013 at 01:19:03PM -0500, Felipe Contreras wrote:
> >> The explains what the patch is doing, but not why. Why is more important.
> >
> > You're right. Why are the indentation useless? It doesn't seem to be
> > useless until you added goto. So why is your goto solution better than
> > the previous existing solution?
> 
> Because it removes useless indentation :)
> 
> This is what they do in the Linux kernel, you tell me which looks better:
> 
> a)
> 
> 	if (function1())
> 		goto leave;
> 	if (function2())
> 		goto leave;
> 	if (function3())
> 		goto leave;
> 	if (function4())
> 		goto leave;
> 	good_stuff();
> leave:
> 	final_stuff();
> 
> or b)
> 
> 	if (!function1()) {
> 		if (!function2()) {
> 			if (!function3()) {
> 				if (!function4()) {
> 					good_stuff();
> 				}
> 			}
> 		}
> 	}
> 	final_stuff();
> 
> -- 
> Felipe Contreras

Oh, so this is purely a "this code style is better than the current
code style"-patch? I won't argue with that. I simply trust Junio in such
cases, I had such discussions with him before.

I thought it was partly to increase cleanup capabilities to. For
example, why isn't msg and defmsg freed when "return allow"?

Still wonder about introducing a new label name for cleanup.

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 10/45] sequencer: trivial fix

[Philip Oakley]

From: "Felipe Contreras" <felipe.contreras@gmail.com>
Sent: Sunday, June 09, 2013 6:23 PM
> On Sun, Jun 9, 2013 at 12:18 PM, SZEDER Gábor <szeder@ira.uka.de> 
> wrote:
>> On Sun, Jun 09, 2013 at 11:40:22AM -0500, Felipe Contreras wrote:
>>> We should free objects before leaving.
>>>
>>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>
>> A shortlog-friendlier subject could be: "sequencer: free objects
>> before leaving".
>
> I already defended my rationale for this succinct commit message:
>
> http://thread.gmane.org/gmane.comp.version-control.git/225609/focus=225610
>
> -- 
> Felipe Contreras

I think I'd prefer a mixture of both.

"sequencer: trivial fix: free objects before leaving".
This gives the best of both worlds in that the 'triviality' is plainly 
there to see, and so is the type of triviality, just in case it has some 
un-noticed side effect that someone is looking for at a leter date

Same goes for "build: trivial cleanup: don't repeat prerequisites" 
[PATCH v4 03/45]

All the best,

Philip

Re: [PATCH v4 45/45] tests: update topology tests

[Martin von Zweigbergk]

On Sun, Jun 9, 2013 at 9:40 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  t/t3425-rebase-topology-merges.sh | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 5400a05..96cc479 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -70,9 +70,8 @@ test_run_rebase () {
>                 test_linear_range "\'"$expected"\'" d..
>         "
>  }
> -#TODO: make order consistent across all flavors of rebase
> -test_run_rebase success 'e n o' ''
> -test_run_rebase success 'e n o' -m
> +test_run_rebase success 'n o e' ''
> +test_run_rebase success 'n o e' -m
>  test_run_rebase success 'n o e' -i

If you do end up re-sending the series on top of my series, I'd prefer
to see the end result having the first argument inlined, so these few
lines become simply:

test_run_rebase success ''
test_run_rebase success -m
test_run_rebase success -i

Re: [PATCH v4 45/45] tests: update topology tests

[Felipe Contreras]

On Mon, Jun 10, 2013 at 11:37 PM, Martin von Zweigbergk
<martinvonz@gmail.com> wrote:
> On Sun, Jun 9, 2013 at 9:40 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t3425-rebase-topology-merges.sh | 15 ++++++---------
>>  1 file changed, 6 insertions(+), 9 deletions(-)
>>
>> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
>> index 5400a05..96cc479 100755
>> --- a/t/t3425-rebase-topology-merges.sh
>> +++ b/t/t3425-rebase-topology-merges.sh
>> @@ -70,9 +70,8 @@ test_run_rebase () {
>>                 test_linear_range "\'"$expected"\'" d..
>>         "
>>  }
>> -#TODO: make order consistent across all flavors of rebase
>> -test_run_rebase success 'e n o' ''
>> -test_run_rebase success 'e n o' -m
>> +test_run_rebase success 'n o e' ''
>> +test_run_rebase success 'n o e' -m
>>  test_run_rebase success 'n o e' -i
>
> If you do end up re-sending the series on top of my series, I'd prefer
> to see the end result having the first argument inlined, so these few
> lines become simply:

Somebody else would need to do that, there's no point in me sending
these patches, just to increase the number of my patches that get
completely ignored by Junio.

-- 
Felipe Contreras

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Junio C Hamano]

Fredrik Gustafsson <iveqy@iveqy.com> writes:

> Here we have two sh-scripts (git rebase
> and git am) interacting with each other.
> Both uses GIT_QUIET,

Correct.

> so if GIT_QUIET already is set by the caller (git rebase) the
> callee doesn't have to set it to.

Incorrect.  "git rebase" invokes "git am", not dot-sources it, so it
does not propagate.

Both do dot-source git-sh-setup, which clears GIT_QUIET pretty
early, so even GIT_QUIET exported into the environment by mistake
will not affect them.

> However GIT_QUIET is undocumented for git-am (in Git Manual). If we
> translate git am to C/ruby/python/perl/etc. will we catch this?

I think it is irrelevant.  git-am uses many shell variables,
e.g. HAS_HEAD, interactive, abort_safety, etc. but they are
implementation details of the program and there is no point
documenting it, and somebody rewriting it in C does not have to
stick to the same name.

What may deserve to be documented in Documentation/technical/
somewhere is the way how some shell variables (OPTIONS_SPEC,
OPTIONS_KEEPDASHDASH, LONG_USAGE, USAGE, GIT_REFLOG_ACTION, etc.)
are used in git-sh-setup, which is a collection of helper shell
functions for use in scripted Porcelains that dot-source it.  It
allows the scripted Porcelains to say things like:

	die "message"
        say "message"

and the latter pays attention to GIT_QUIET.

> This raises a few more generall questions:
> do we already pass information between processes(!) with enviroment
> variables? And is this documented the way it should be?

There are a few used as implementation details, I think.  GIT_QUIET
is not among them.

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Fredrik Gustafsson]

Thank you for your reply. Although I agree with everything you say, it
raised a few more questions.

On Tue, Jun 11, 2013 at 09:12:59AM -0700, Junio C Hamano wrote:
> Incorrect.  "git rebase" invokes "git am", not dot-sources it, so it
> does not propagate.

Then git am wouldn't be quite and this patch is doing wrong by removing
the -q argument from the git am invokation. Correct?

> I think it is irrelevant.  git-am uses many shell variables,
> e.g. HAS_HEAD, interactive, abort_safety, etc. but they are
> implementation details of the program and there is no point
> documenting it, and somebody rewriting it in C does not have to
> stick to the same name.

<--snip-->

> There are a few used as implementation details, I think.  GIT_QUIET
> is not among them.

I made a mistake here thinking that git rebase set an enviromental
variable that git am later red. That would have been a way to
communicate, that the first processes that got the -q argument sets the
enviromental variabel GIT_QUIET and all other git invokations read that
enviromental variable.

I do not like that solution. When communicating between different git
commands I prefer, pipes, files and command line arguments. I think it
will be dangerous to include temporary enviromental variables (if we
don't stick to a few of them and document them very well).

In the case of passing information with temporary enviromental variables
it's no longer an implementation detail, but something a rewrite needs
to honor.

I hope I was able to make myself somewhat clear, and if I understand you
correctly we are already agreeing on this?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Felipe Contreras]

On Tue, Jun 11, 2013 at 12:08 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> Thank you for your reply. Although I agree with everything you say, it
> raised a few more questions.
>
> On Tue, Jun 11, 2013 at 09:12:59AM -0700, Junio C Hamano wrote:
>> Incorrect.  "git rebase" invokes "git am", not dot-sources it, so it
>> does not propagate.
>
> Then git am wouldn't be quite and this patch is doing wrong by removing
> the -q argument from the git am invokation. Correct?

It's not removed. It's simply moved.

-- 
Felipe Contreras

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Fredrik Gustafsson]

On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote:
> It's not removed. It's simply moved.

Sorry about that, I wasn't paying enough attention. But why are you
moving it?

All other arguments to git am is set in git-rebase.sh, why just set
-q just before the invokation in git-rebase--am.sh?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Felipe Contreras]

On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote:
>> It's not removed. It's simply moved.
>
> Sorry about that, I wasn't paying enough attention. But why are you
> moving it?
>
> All other arguments to git am is set in git-rebase.sh, why just set
> -q just before the invokation in git-rebase--am.sh?

Because the next patch checks if there's any arguments meant for 'git
am' to switch to am rebase mode. We shouldn't switch to that mode if
the only argument to 'git am' is going to be -q.

-- 
Felipe Contreras

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Fredrik Gustafsson]

On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote:
> On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> > On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote:
> >> It's not removed. It's simply moved.
> >
> > Sorry about that, I wasn't paying enough attention. But why are you
> > moving it?
> >
> > All other arguments to git am is set in git-rebase.sh, why just set
> > -q just before the invokation in git-rebase--am.sh?
> 
> Because the next patch checks if there's any arguments meant for 'git
> am' to switch to am rebase mode. We shouldn't switch to that mode if
> the only argument to 'git am' is going to be -q.

Okay, that make sense. How about rephrase the commit message and add
this explanation. It's really not a cleanup but a preparation for the
next patch.

If I was a maintainer and only got this patch I would reject it. Every
patch in a patch serie should be justified to be applied as a single
patch, yes?

-- 
Med vänliga hälsningar
Fredrik Gustafsson

tel: 0733-608274
e-post: iveqy@iveqy.com

Re: [PATCH v4 31/45] rebase: trivial cleanup

[Felipe Contreras]

On Tue, Jun 11, 2013 at 12:41 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
> On Tue, Jun 11, 2013 at 12:26:42PM -0500, Felipe Contreras wrote:
>> On Tue, Jun 11, 2013 at 12:24 PM, Fredrik Gustafsson <iveqy@iveqy.com> wrote:
>> > On Tue, Jun 11, 2013 at 12:09:32PM -0500, Felipe Contreras wrote:
>> >> It's not removed. It's simply moved.
>> >
>> > Sorry about that, I wasn't paying enough attention. But why are you
>> > moving it?
>> >
>> > All other arguments to git am is set in git-rebase.sh, why just set
>> > -q just before the invokation in git-rebase--am.sh?
>>
>> Because the next patch checks if there's any arguments meant for 'git
>> am' to switch to am rebase mode. We shouldn't switch to that mode if
>> the only argument to 'git am' is going to be -q.
>
> Okay, that make sense. How about rephrase the commit message and add
> this explanation. It's really not a cleanup but a preparation for the
> next patch.
>
> If I was a maintainer and only got this patch I would reject it. Every
> patch in a patch serie should be justified to be applied as a single
> patch, yes?

Yes, but it doesn't matter, because the maintainer has made it clear
he is not going to even read this patch series.

-- 
Felipe Contreras