[PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Paul Tan]

Hi all,

Last year I rewrote git-am from shell script to C. This succeeded in speeding
up a non-interactive git-rebase by 6-7x[1], which is really handly when rebasing
multiple topic branches.

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

However, it turns out that when working on a topic branch, I frequently use
interactive rebase instead to edit and squash commits. Unfortunately, as
git-rebase--interactive.sh is still a shell script, it is a bit slower (e.g.
taking a few seconds longer compared to non-interactive rebase when rebasing
big topic branches).

The situation is much worse on Windows, as from the invocation of git rebase -i,
it takes a few seconds before the editor even pops up, and the actual
rebase proceeds at a snails pace, taking around 3 minutes for a 50-patch
series, which is a huge deal-breaker since my workflow depends on frequent
commits and squashes.

As such, this year I would like to apply for GSoC to work on a rewrite of
git-rebase to C. It is slightly hefty, as there are three backends (am, merge
and interactive), along with the git-rebase.sh script.

To get a gauge of how much code is needed for the rewrite, I explored rewriting
the scripts into C, and then extracted some bits out and polished them a bit to
make a barebones git-rebase in C, creating this patch series:

[01/17] perf: introduce performance tests for git-rebase

A simple performance test for the three rebase backends so we can compare this
C version and the shell version below.

[02/17] sha1_name: implement get_oid() and friends
[03/17] builtin-rebase: implement skeletal builtin rebase
[04/17] builtin-rebase: parse rebase arguments into a common rebase_options struct
[05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

The three rebase backends (am, merge, interactive) have vastly different
capabilities, so I did not try to shoehorn them into the same interface.
However, they do share a few common options and functionality, so I introduced
the common rebase-common.c library and rebase_options struct.

In the above patches we implement the essential arguments for a rebase: the
upstream, branch_name and --onto <newbase>.

[06/17] rebase-am: introduce am backend for builtin rebase

This patch implements a barebones rebase-am backend.

[07/17] rebase-common: implement refresh_and_write_cache()
[08/17] rebase-common: let refresh_and_write_cache() take a flags argument
[09/17] rebase-common: implement cache_has_unstaged_changes()
[10/17] rebase-common: implement cache_has_uncommitted_changes()
[11/17] rebase-merge: introduce merge backend for builtin rebase

These patches implement a barebones rebase-merge backend.

[12/17] rebase-todo: introduce rebase_todo_item
[13/17] rebase-todo: introduce rebase_todo_list
[14/17] status: use rebase_todo_list
[15/17] wrapper: implement append_file()
[16/17] editor: implement git_sequence_editor() and launch_sequence_editor()
[17/17] rebase-interactive: introduce interactive backend for builtin rebase

And these patches implement a barebones rebase-interactive backend.

With these patches the performance numbers when rebasing 50 commits on the
git.git repository are, on Linux,

Before patch series:

Test                               this tree
--------------------------------------------------
3400.2: rebase --onto master^      1.10(0.84+0.06)
3402.2: rebase -m --onto master^   2.38(1.38+0.13)
3404.2: rebase -i --onto master^   3.11(1.37+0.27)

After patch series:

Test                               this tree
--------------------------------------------------
3400.2: rebase --onto master^      0.74(0.51+0.08)
3402.2: rebase -m --onto master^   1.72(1.26+0.17)
3404.2: rebase -i --onto master^   1.74(1.20+0.18)

And on Windows,

Before patch series:

Test                               this tree
----------------------------------------------------
3400.2: rebase --onto master^      10.90(0.06+0.47)
3402.2: rebase -m --onto master^   86.87(0.04+0.47)
3404.2: rebase -i --onto master^   191.65(0.09+0.44)

After patch series:

Test                               this tree
---------------------------------------------------
3400.2: rebase --onto master^      6.45(0.13+0.40)
3402.2: rebase -m --onto master^   12.32(0.13+0.40)
3404.2: rebase -i --onto master^   14.16(0.15+0.40)

(Thanks to the git-am rewrite, non-interactive rebase on Windows is already
relatively fast ;-) )

So, we have around a 1.4x-1.8x speedup for Linux users, and a 1.7x-13x speedup
for Windows users. The annoying long delay before the interactive editor is
launched on Windows is gotten rid of, which I'm very happy about :-)

On the code side, we do get some nice things with a rewrite to C. For example,
we get the rebase-todo library for parsing and writing git-rebase-todo files,
which means that wt-status.c and rebase-interactive.c can share the same
parsing code. Although not in this patch series, rebase-interactive.c can also
now share the same author-script parsing and writing code from builtin/am.c as
well.

Regards,
Paul

Paul Tan (17):
  perf: introduce performance tests for git-rebase
  sha1_name: implement get_oid() and friends
  builtin-rebase: implement skeletal builtin rebase
  builtin-rebase: parse rebase arguments into a common rebase_options
    struct
  rebase-options: implement rebase_options_load() and
    rebase_options_save()
  rebase-am: introduce am backend for builtin rebase
  rebase-common: implement refresh_and_write_cache()
  rebase-common: let refresh_and_write_cache() take a flags argument
  rebase-common: implement cache_has_unstaged_changes()
  rebase-common: implement cache_has_uncommitted_changes()
  rebase-merge: introduce merge backend for builtin rebase
  rebase-todo: introduce rebase_todo_item
  rebase-todo: introduce rebase_todo_list
  status: use rebase_todo_list
  wrapper: implement append_file()
  editor: implement git_sequence_editor() and launch_sequence_editor()
  rebase-interactive: introduce interactive backend for builtin rebase

 Makefile                           |  10 +-
 builtin.h                          |   1 +
 builtin/am.c                       |  16 +-
 builtin/pull.c                     |  41 +---
 builtin/rebase.c                   | 264 ++++++++++++++++++++++++++
 cache.h                            |   8 +
 editor.c                           |  27 ++-
 git.c                              |   1 +
 rebase-am.c                        | 110 +++++++++++
 rebase-am.h                        |  22 +++
 rebase-common.c                    | 220 ++++++++++++++++++++++
 rebase-common.h                    |  48 +++++
 rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
 rebase-interactive.h               |  33 ++++
 rebase-merge.c                     | 256 +++++++++++++++++++++++++
 rebase-merge.h                     |  28 +++
 rebase-todo.c                      | 251 +++++++++++++++++++++++++
 rebase-todo.h                      |  55 ++++++
 sha1_name.c                        |  30 +++
 strbuf.h                           |   1 +
 t/perf/p3400-rebase.sh             |  25 +++
 t/perf/p3402-rebase-merge.sh       |  25 +++
 t/perf/p3404-rebase-interactive.sh |  26 +++
 wrapper.c                          |  23 +++
 wt-status.c                        | 100 +++-------
 25 files changed, 1863 insertions(+), 133 deletions(-)
 create mode 100644 builtin/rebase.c
 create mode 100644 rebase-am.c
 create mode 100644 rebase-am.h
 create mode 100644 rebase-common.c
 create mode 100644 rebase-common.h
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h
 create mode 100644 rebase-merge.c
 create mode 100644 rebase-merge.h
 create mode 100644 rebase-todo.c
 create mode 100644 rebase-todo.h
 create mode 100755 t/perf/p3400-rebase.sh
 create mode 100755 t/perf/p3402-rebase-merge.sh
 create mode 100755 t/perf/p3404-rebase-interactive.sh

-- 
2.7.0

[PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Paul Tan]

To determine the speedup (or slowdown) of the upcoming git-rebase
rewrite to C, add a simple performance test for each of the 3 git-rebase
backends (am, merge and interactive).

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/perf/p3400-rebase.sh             | 25 +++++++++++++++++++++++++
 t/perf/p3402-rebase-merge.sh       | 25 +++++++++++++++++++++++++
 t/perf/p3404-rebase-interactive.sh | 26 ++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100755 t/perf/p3400-rebase.sh
 create mode 100755 t/perf/p3402-rebase-merge.sh
 create mode 100755 t/perf/p3404-rebase-interactive.sh

diff --git a/t/perf/p3400-rebase.sh b/t/perf/p3400-rebase.sh
new file mode 100755
index 0000000..f172a64
--- /dev/null
+++ b/t/perf/p3400-rebase.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description="Tests rebase performance with am backend"
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+test_checkout_worktree
+
+# Setup a topic branch with 50 commits
+test_expect_success 'setup topic branch' '
+	git checkout -b perf-topic-branch master &&
+	for i in $(test_seq 50); do
+		test_commit perf-$i file
+	done &&
+	git tag perf-topic-branch-initial
+'
+
+test_perf 'rebase --onto master^' '
+	git checkout perf-topic-branch &&
+	git reset --hard perf-topic-branch-initial &&
+	git rebase --onto master^ master
+'
+
+test_done
diff --git a/t/perf/p3402-rebase-merge.sh b/t/perf/p3402-rebase-merge.sh
new file mode 100755
index 0000000..b71ce12
--- /dev/null
+++ b/t/perf/p3402-rebase-merge.sh
@@ -0,0 +1,25 @@
+#!/bin/sh
+
+test_description="Tests rebase performance with merge backend"
+
+. ./perf-lib.sh
+
+test_perf_default_repo
+test_checkout_worktree
+
+# Setup a topic branch with 50 commits
+test_expect_success 'setup topic branch' '
+	git checkout -b perf-topic-branch master &&
+	for i in $(test_seq 50); do
+		test_commit perf-$i file
+	done &&
+	git tag perf-topic-branch-initial
+'
+
+test_perf 'rebase -m --onto master^' '
+	git checkout perf-topic-branch &&
+	git reset --hard perf-topic-branch-initial &&
+	git rebase -m --onto master^ master
+'
+
+test_done
diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
new file mode 100755
index 0000000..aaca105
--- /dev/null
+++ b/t/perf/p3404-rebase-interactive.sh
@@ -0,0 +1,26 @@
+#!/bin/sh
+
+test_description="Tests interactive rebase performance"
+
+. ./perf-lib.sh
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_perf_default_repo
+test_checkout_worktree
+
+# Setup a topic branch with 50 commits
+test_expect_success 'setup topic branch' '
+	git checkout -b perf-topic-branch master &&
+	for i in $(test_seq 50); do
+		test_commit perf-$i file
+	done &&
+	git tag perf-topic-branch-initial
+'
+
+test_perf 'rebase -i --onto master^' '
+	git checkout perf-topic-branch &&
+	git reset --hard perf-topic-branch-initial &&
+	GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
+'
+
+test_done
-- 
2.7.0

[PATCH/RFC/GSoC 02/17] sha1_name: implement get_oid() and friends

[Paul Tan]

5f7817c (define a structure for object IDs, 2015-03-13) introduced the
object_id struct to replace the used of unsigned char[] arrays to hold
object IDs. This gives us the benefit of compile-time checking for
misuse.

To fully take advantage of compile-time type-checking, introduce the
get_oid_*() functions which wrap the corresponding get_sha1_*()
functions.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 cache.h     |  6 ++++++
 sha1_name.c | 30 ++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/cache.h b/cache.h
index b829410..55d443e 100644
--- a/cache.h
+++ b/cache.h
@@ -1116,11 +1116,17 @@ struct object_context {
 #define GET_SHA1_ONLY_TO_DIE    04000
 
 extern int get_sha1(const char *str, unsigned char *sha1);
+extern int get_oid(const char *str, struct object_id *oid);
 extern int get_sha1_commit(const char *str, unsigned char *sha1);
+extern int get_oid_commit(const char *str, struct object_id *oid);
 extern int get_sha1_committish(const char *str, unsigned char *sha1);
+extern int get_oid_committish(const char *str, struct object_id *oid);
 extern int get_sha1_tree(const char *str, unsigned char *sha1);
+extern int get_oid_tree(const char *str, struct object_id *oid);
 extern int get_sha1_treeish(const char *str, unsigned char *sha1);
+extern int get_oid_treeish(const char *str, struct object_id *oid);
 extern int get_sha1_blob(const char *str, unsigned char *sha1);
+extern int get_oid_blob(const char *str, struct object_id *oid);
 extern void maybe_die_on_misspelt_object_name(const char *name, const char *prefix);
 extern int get_sha1_with_context(const char *str, unsigned flags, unsigned char *sha1, struct object_context *orc);
 
diff --git a/sha1_name.c b/sha1_name.c
index 3acf221..307dfad 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1214,6 +1214,11 @@ int get_sha1(const char *name, unsigned char *sha1)
 	return get_sha1_with_context(name, 0, sha1, &unused);
 }
 
+int get_oid(const char *name, struct object_id *oid)
+{
+	return get_sha1(name, oid->hash);
+}
+
 /*
  * Many callers know that the user meant to name a commit-ish by
  * syntactical positions where the object name appears.  Calling this
@@ -1231,6 +1236,11 @@ int get_sha1_committish(const char *name, unsigned char *sha1)
 				     sha1, &unused);
 }
 
+int get_oid_committish(const char *name, struct object_id *oid)
+{
+	return get_sha1_committish(name, oid->hash);
+}
+
 int get_sha1_treeish(const char *name, unsigned char *sha1)
 {
 	struct object_context unused;
@@ -1238,6 +1248,11 @@ int get_sha1_treeish(const char *name, unsigned char *sha1)
 				     sha1, &unused);
 }
 
+int get_oid_treeish(const char *name, struct object_id *oid)
+{
+	return get_sha1_treeish(name, oid->hash);
+}
+
 int get_sha1_commit(const char *name, unsigned char *sha1)
 {
 	struct object_context unused;
@@ -1245,6 +1260,11 @@ int get_sha1_commit(const char *name, unsigned char *sha1)
 				     sha1, &unused);
 }
 
+int get_oid_commit(const char *name, struct object_id *oid)
+{
+	return get_sha1_commit(name, oid->hash);
+}
+
 int get_sha1_tree(const char *name, unsigned char *sha1)
 {
 	struct object_context unused;
@@ -1252,6 +1272,11 @@ int get_sha1_tree(const char *name, unsigned char *sha1)
 				     sha1, &unused);
 }
 
+int get_oid_tree(const char *name, struct object_id *oid)
+{
+	return get_sha1_tree(name, oid->hash);
+}
+
 int get_sha1_blob(const char *name, unsigned char *sha1)
 {
 	struct object_context unused;
@@ -1259,6 +1284,11 @@ int get_sha1_blob(const char *name, unsigned char *sha1)
 				     sha1, &unused);
 }
 
+int get_oid_blob(const char *name, struct object_id *oid)
+{
+	return get_sha1_blob(name, oid->hash);
+}
+
 /* Must be called only when object_name:filename doesn't exist. */
 static void diagnose_invalid_sha1_path(const char *prefix,
 				       const char *filename,
-- 
2.7.0

[PATCH/RFC/GSoC 03/17] builtin-rebase: implement skeletal builtin rebase

[Paul Tan]

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile         |  5 +----
 builtin.h        |  1 +
 builtin/rebase.c | 31 +++++++++++++++++++++++++++++++
 git.c            |  1 +
 4 files changed, 34 insertions(+), 4 deletions(-)
 create mode 100644 builtin/rebase.c

diff --git a/Makefile b/Makefile
index 24bef8d..ad98714 100644
--- a/Makefile
+++ b/Makefile
@@ -496,7 +496,6 @@ SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
 SCRIPT_SH += git-quiltimport.sh
-SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
 SCRIPT_SH += git-request-pull.sh
 SCRIPT_SH += git-stash.sh
@@ -505,9 +504,6 @@ 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--interactive
-SCRIPT_LIB += git-rebase--merge
 SCRIPT_LIB += git-sh-setup
 SCRIPT_LIB += git-sh-i18n
 
@@ -909,6 +905,7 @@ BUILTIN_OBJS += builtin/prune.o
 BUILTIN_OBJS += builtin/pull.o
 BUILTIN_OBJS += builtin/push.o
 BUILTIN_OBJS += builtin/read-tree.o
+BUILTIN_OBJS += builtin/rebase.o
 BUILTIN_OBJS += builtin/receive-pack.o
 BUILTIN_OBJS += builtin/reflog.o
 BUILTIN_OBJS += builtin/remote.o
diff --git a/builtin.h b/builtin.h
index 6b95006..a184a58 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_prune_packed(int argc, const char **argv, const char *prefix);
 extern int cmd_pull(int argc, const char **argv, const char *prefix);
 extern int cmd_push(int argc, const char **argv, const char *prefix);
 extern int cmd_read_tree(int argc, const char **argv, const char *prefix);
+extern int cmd_rebase(int argc, const char **argv, const char *prefix);
 extern int cmd_receive_pack(int argc, const char **argv, const char *prefix);
 extern int cmd_reflog(int argc, const char **argv, const char *prefix);
 extern int cmd_remote(int argc, const char **argv, const char *prefix);
diff --git a/builtin/rebase.c b/builtin/rebase.c
new file mode 100644
index 0000000..04cc1bd
--- /dev/null
+++ b/builtin/rebase.c
@@ -0,0 +1,31 @@
+/*
+ * Builtin "git rebase"
+ */
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+
+static int git_rebase_config(const char *k, const char *v, void *cb)
+{
+	return git_default_config(k, v, NULL);
+}
+
+int cmd_rebase(int argc, const char **argv, const char *prefix)
+{
+	const char * const usage[] = {
+		N_("git rebase [options]"),
+		NULL
+	};
+	struct option options[] = {
+		OPT_END()
+	};
+
+	git_config(git_rebase_config, NULL);
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (read_cache_preload(NULL) < 0)
+		die(_("failed to read the index"));
+
+	return 0;
+}
diff --git a/git.c b/git.c
index 6cc0c07..f9b7033 100644
--- a/git.c
+++ b/git.c
@@ -452,6 +452,7 @@ static struct cmd_struct commands[] = {
 	{ "pull", cmd_pull, RUN_SETUP | NEED_WORK_TREE },
 	{ "push", cmd_push, RUN_SETUP },
 	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "rebase", cmd_rebase, RUN_SETUP | NEED_WORK_TREE },
 	{ "receive-pack", cmd_receive_pack },
 	{ "reflog", cmd_reflog, RUN_SETUP },
 	{ "remote", cmd_remote, RUN_SETUP },
-- 
2.7.0

[PATCH/RFC/GSoC 04/17] builtin-rebase: parse rebase arguments into a common rebase_options struct

[Paul Tan]

A non-root rebase takes 3 arguments:

* branch_name -- the branch or commit that will be rebased. If it is not
  specified, the current branch is used.

* upstream -- The upstream commit to compare against. If it is not
  specified, the configured upstream for the current branch is used.

* onto (or newbase) -- The commit to be used as the starting point to
  re-apply the commits. If it is not specified, `upstream` is used.

Since these parameters are used by all 3 rebase backends, introduce a
common rebase_options struct to hold all these options. Teach
builtin/rebase.c to handle the above arguments and store them in a
rebase_options struct. In later patches we will pass the rebase_options
struct to the appropriate backend to perform the rebase.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile         |   1 +
 builtin/rebase.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 rebase-common.c  |  28 +++++++++
 rebase-common.h  |  23 +++++++
 4 files changed, 235 insertions(+), 1 deletion(-)
 create mode 100644 rebase-common.c
 create mode 100644 rebase-common.h

diff --git a/Makefile b/Makefile
index ad98714..b29c672 100644
--- a/Makefile
+++ b/Makefile
@@ -779,6 +779,7 @@ LIB_OBJS += prompt.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-common.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 04cc1bd..40176ca 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -4,6 +4,112 @@
 #include "cache.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "rebase-common.h"
+#include "remote.h"
+#include "branch.h"
+#include "refs.h"
+
+/**
+ * Used by get_curr_branch_upstream_name() as a for_each_remote() callback to
+ * retrieve the name of the remote if the repository only has one remote.
+ */
+static int get_only_remote(struct remote *remote, void *cb_data)
+{
+	const char **remote_name = cb_data;
+
+	if (*remote_name)
+		return -1;
+
+	*remote_name = remote->name;
+	return 0;
+}
+
+const char *get_curr_branch_upstream_name(void)
+{
+	const char *upstream_name;
+	struct branch *curr_branch;
+
+	curr_branch = branch_get("HEAD");
+	if (!curr_branch) {
+		fprintf_ln(stderr, _("You are not currently on a branch."));
+		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
+		fprintf_ln(stderr, _("See git-rebase(1) for details."));
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, "    git rebase <branch>");
+		fprintf(stderr, "\n");
+		exit(1);
+	}
+
+	upstream_name = branch_get_upstream(curr_branch, NULL);
+	if (!upstream_name) {
+		const char *remote_name = NULL;
+
+		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
+			remote_name = "<remote>";
+
+		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
+		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
+		fprintf_ln(stderr, _("See git-rebase(1) for details."));
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, "    git rebase <branch>");
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
+		fprintf(stderr, "\n");
+		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:\n"
+		"\n"
+		"    git branch --set-upstream-to=%s/<branch> %s\n"),
+		remote_name, curr_branch->name);
+		exit(1);
+	}
+
+	return upstream_name;
+}
+
+/**
+ * Given the --onto <name>, return the onto hash
+ */
+static void get_onto_oid(const char *_onto_name, struct object_id *onto)
+{
+	char *onto_name = xstrdup(_onto_name);
+	struct commit *onto_commit;
+	char *dotdot;
+
+	dotdot = strstr(onto_name, "...");
+	if (dotdot) {
+		const char *left = onto_name;
+		const char *right = dotdot + 3;
+		struct commit *left_commit, *right_commit;
+		struct commit_list *merge_bases;
+
+		*dotdot = 0;
+		if (!*left)
+			left = "HEAD";
+		if (!*right)
+			right = "HEAD";
+
+		/* git merge-base --all $left $right */
+		left_commit = lookup_commit_reference_by_name(left);
+		right_commit = lookup_commit_reference_by_name(right);
+		if (!left_commit || !right_commit)
+			die(_("%s: there is no merge base"), _onto_name);
+
+		merge_bases = get_merge_bases(left_commit, right_commit);
+		if (merge_bases && merge_bases->next)
+			die(_("%s: there are more than one merge bases"), _onto_name);
+		else if (!merge_bases)
+			die(_("%s: there is no merge base"), _onto_name);
+
+		onto_commit = merge_bases->item;
+		free_commit_list(merge_bases);
+	} else {
+		onto_commit = lookup_commit_reference_by_name(onto_name);
+		if (!onto_commit)
+			die(_("invalid upstream %s"), onto_name);
+	}
+
+	free(onto_name);
+	oidcpy(onto, &onto_commit->object.oid);
+}
 
 static int git_rebase_config(const char *k, const char *v, void *cb)
 {
@@ -12,20 +118,96 @@ static int git_rebase_config(const char *k, const char *v, void *cb)
 
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options rebase_opts;
+	const char *onto_name = NULL;
+	const char *branch_name;
+
 	const char * const usage[] = {
-		N_("git rebase [options]"),
+		N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
 		NULL
 	};
 	struct option options[] = {
+		OPT_GROUP(N_("Available options are")),
+		OPT_STRING(0, "onto", &onto_name, NULL,
+			N_("rebase onto given branch instead of upstream")),
 		OPT_END()
 	};
 
 	git_config(git_rebase_config, NULL);
+	rebase_options_init(&rebase_opts);
+	rebase_opts.resolvemsg = _("\nWhen you have resolved this problem, run \"git rebase --continue\".\n"
+			"If you prefer to skip this patch, run \"git rebase --skip\" instead.\n"
+			"To check out the original branch and stop rebasing, run \"git rebase --abort\".");
 
 	argc = parse_options(argc, argv, prefix, options, usage, 0);
 
 	if (read_cache_preload(NULL) < 0)
 		die(_("failed to read the index"));
 
+	/*
+	 * Parse command-line arguments:
+	 *    rebase [<options>] [<upstream_name>] [<branch_name>]
+	 */
+
+	/* Parse <upstream_name> into rebase_opts.upstream */
+	{
+		const char *upstream_name;
+		if (argc > 2)
+			usage_with_options(usage, options);
+		if (!argc) {
+			upstream_name = get_curr_branch_upstream_name();
+		} else {
+			upstream_name = argv[0];
+			argv++, argc--;
+			if (!strcmp(upstream_name, "-"))
+				upstream_name = "@{-1}";
+		}
+		if (get_oid_commit(upstream_name, &rebase_opts.upstream))
+			die(_("invalid upstream %s"), upstream_name);
+		if (!onto_name)
+			onto_name = upstream_name;
+	}
+
+	/*
+	 * Parse --onto <onto_name> into rebase_opts.onto and
+	 * rebase_opts.onto_name
+	 */
+	get_onto_oid(onto_name, &rebase_opts.onto);
+	rebase_opts.onto_name = xstrdup(onto_name);
+
+	/*
+	 * Parse <branch_name> into rebase_opts.orig_head and
+	 * rebase_opts.orig_refname
+	 */
+	branch_name = argv[0];
+	if (branch_name) {
+		/* Is branch_name a branch or commit? */
+		char *ref_name = xstrfmt("refs/heads/%s", branch_name);
+		struct object_id orig_head_id;
+
+		if (!read_ref(ref_name, orig_head_id.hash)) {
+			rebase_opts.orig_refname = ref_name;
+			if (get_oid_commit(ref_name, &rebase_opts.orig_head))
+				die("get_sha1_commit failed");
+		} else if (!get_oid_commit(branch_name, &rebase_opts.orig_head)) {
+			rebase_opts.orig_refname = NULL;
+			free(ref_name);
+		} else {
+			die(_("no such branch: %s"), branch_name);
+		}
+	} else {
+		/* Do not need to switch branches, we are already on it */
+		struct branch *curr_branch = branch_get("HEAD");
+
+		if (curr_branch)
+			rebase_opts.orig_refname = xstrdup(curr_branch->refname);
+		else
+			rebase_opts.orig_refname = NULL;
+
+		if (get_oid_commit("HEAD", &rebase_opts.orig_head))
+			die(_("Failed to resolve '%s' as a valid revision."), "HEAD");
+	}
+
+	rebase_options_release(&rebase_opts);
 	return 0;
 }
diff --git a/rebase-common.c b/rebase-common.c
new file mode 100644
index 0000000..5a49ac4
--- /dev/null
+++ b/rebase-common.c
@@ -0,0 +1,28 @@
+#include "cache.h"
+#include "rebase-common.h"
+
+void rebase_options_init(struct rebase_options *opts)
+{
+	oidclr(&opts->onto);
+	opts->onto_name = NULL;
+
+	oidclr(&opts->upstream);
+
+	oidclr(&opts->orig_head);
+	opts->orig_refname = NULL;
+
+	opts->resolvemsg = NULL;
+}
+
+void rebase_options_release(struct rebase_options *opts)
+{
+	free(opts->onto_name);
+	free(opts->orig_refname);
+}
+
+void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
+{
+	struct rebase_options tmp = *dst;
+	*dst = *src;
+	*src = tmp;
+}
diff --git a/rebase-common.h b/rebase-common.h
new file mode 100644
index 0000000..db5146a
--- /dev/null
+++ b/rebase-common.h
@@ -0,0 +1,23 @@
+#ifndef REBASE_COMMON_H
+#define REBASE_COMMON_H
+
+/* common rebase backend options */
+struct rebase_options {
+	struct object_id onto;
+	char *onto_name;
+
+	struct object_id upstream;
+
+	struct object_id orig_head;
+	char *orig_refname;
+
+	const char *resolvemsg;
+};
+
+void rebase_options_init(struct rebase_options *);
+
+void rebase_options_release(struct rebase_options *);
+
+void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
+
+#endif /* REBASE_COMMON_H */
-- 
2.7.0

[PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Paul Tan]

These functions can be used for loading and saving common rebase options
into a state directory.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 rebase-common.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-common.h |  4 ++++
 2 files changed, 73 insertions(+)

diff --git a/rebase-common.c b/rebase-common.c
index 5a49ac4..1835f08 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
 	*dst = *src;
 	*src = tmp;
 }
+
+static int state_file_exists(const char *dir, const char *file)
+{
+	return file_exists(mkpath("%s/%s", dir, file));
+}
+
+static int read_state_file(struct strbuf *sb, const char *dir, const char *file)
+{
+	const char *path = mkpath("%s/%s", dir, file);
+	strbuf_reset(sb);
+	if (strbuf_read_file(sb, path, 0) >= 0)
+		return sb->len;
+	else
+		return error(_("could not read '%s'"), path);
+}
+
+int rebase_options_load(struct rebase_options *opts, const char *dir)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *filename;
+
+	/* opts->orig_refname */
+	if (read_state_file(&sb, dir, "head-name") < 0)
+		return -1;
+	strbuf_trim(&sb);
+	if (starts_with(sb.buf, "refs/heads/"))
+		opts->orig_refname = strbuf_detach(&sb, NULL);
+	else if (!strcmp(sb.buf, "detached HEAD"))
+		opts->orig_refname = NULL;
+	else
+		return error(_("could not parse %s"), mkpath("%s/%s", dir, "head-name"));
+
+	/* opts->onto */
+	if (read_state_file(&sb, dir, "onto") < 0)
+		return -1;
+	strbuf_trim(&sb);
+	if (get_oid_hex(sb.buf, &opts->onto) < 0)
+		return error(_("could not parse %s"), mkpath("%s/%s", dir, "onto"));
+
+	/*
+	 * We always write to orig-head, but interactive rebase used to write
+	 * to head. Fall back to reading from head to cover for the case that
+	 * the user upgraded git with an ongoing interactive rebase.
+	 */
+	filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
+	if (read_state_file(&sb, dir, filename) < 0)
+		return -1;
+	strbuf_trim(&sb);
+	if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
+		return error(_("could not parse %s"), mkpath("%s/%s", dir, filename));
+
+	strbuf_release(&sb);
+	return 0;
+}
+
+static int write_state_text(const char *dir, const char *file, const char *string)
+{
+	return write_file(mkpath("%s/%s", dir, file), "%s", string);
+}
+
+void rebase_options_save(const struct rebase_options *opts, const char *dir)
+{
+	const char *head_name = opts->orig_refname;
+	if (!head_name)
+		head_name = "detached HEAD";
+	write_state_text(dir, "head-name", head_name);
+	write_state_text(dir, "onto", oid_to_hex(&opts->onto));
+	write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
+}
diff --git a/rebase-common.h b/rebase-common.h
index db5146a..051c056 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -20,4 +20,8 @@ void rebase_options_release(struct rebase_options *);
 
 void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
 
+int rebase_options_load(struct rebase_options *, const char *dir);
+
+void rebase_options_save(const struct rebase_options *, const char *dir);
+
 #endif /* REBASE_COMMON_H */
-- 
2.7.0

[PATCH/RFC/GSoC 06/17] rebase-am: introduce am backend for builtin rebase

[Paul Tan]

Since 7f59dbb (Rewrite rebase to use git-format-patch piped to git-am.,
2005-11-14), git-rebase will by default use "git am" to rebase commits.
This is done by first checking out to the new base commit, generating a
series of patches with the commits to replay, and then applying them
with git-am. Finally, if orig_head is a branch, it is updated to point
to the tip of the new rebased commit history.

Implement a skeletal version of this method of rebasing commits by
introducing a new rebase-am backend for our builtin-rebase. This
skeletal version can only call git-format-patch and git-am to perform a
rebase, and is unable to resume from a failed rebase. Subsequent
patches will re-implement all the missing features.

The symmetric difference between upstream...orig_head is used because in
a later patch, we will add an additional exclusion revision in order to
handle fork points correctly.  See b6266dc (rebase--am: use
--cherry-pick instead of --ignore-if-in-upstream, 2014-07-15).

The initial steps of checking out the new base commit, and the final
cleanup steps of updating refs are common between the am backend and
merge backend. As such, we implement the common setup and teardown
sequence in the shared functions rebase_common_setup() and
rebase_common_finish(), so we can share code with the merge backend when
it is implemented in a later patch.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile         |   1 +
 builtin/rebase.c |  25 +++++++++++++
 rebase-am.c      | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-am.h      |  22 +++++++++++
 rebase-common.c  |  81 ++++++++++++++++++++++++++++++++++++++++
 rebase-common.h  |   6 +++
 6 files changed, 245 insertions(+)
 create mode 100644 rebase-am.c
 create mode 100644 rebase-am.h

diff --git a/Makefile b/Makefile
index b29c672..a2618ea 100644
--- a/Makefile
+++ b/Makefile
@@ -779,6 +779,7 @@ LIB_OBJS += prompt.o
 LIB_OBJS += quote.o
 LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
+LIB_OBJS += rebase-am.o
 LIB_OBJS += rebase-common.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 40176ca..ec63d3b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -8,6 +8,22 @@
 #include "remote.h"
 #include "branch.h"
 #include "refs.h"
+#include "rebase-am.h"
+
+enum rebase_type {
+	REBASE_TYPE_NONE = 0,
+	REBASE_TYPE_AM
+};
+
+static const char *rebase_dir(enum rebase_type type)
+{
+	switch (type) {
+	case REBASE_TYPE_AM:
+		return git_path_rebase_am_dir();
+	default:
+		die("BUG: invalid rebase_type %d", type);
+	}
+}
 
 /**
  * Used by get_curr_branch_upstream_name() as a for_each_remote() callback to
@@ -208,6 +224,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("Failed to resolve '%s' as a valid revision."), "HEAD");
 	}
 
+	/* Run the appropriate rebase backend */
+	{
+		struct rebase_am state;
+		rebase_am_init(&state, rebase_dir(REBASE_TYPE_AM));
+		rebase_options_swap(&state.opts, &rebase_opts);
+		rebase_am_run(&state);
+		rebase_am_release(&state);
+	}
+
 	rebase_options_release(&rebase_opts);
 	return 0;
 }
diff --git a/rebase-am.c b/rebase-am.c
new file mode 100644
index 0000000..53e8798
--- /dev/null
+++ b/rebase-am.c
@@ -0,0 +1,110 @@
+#include "cache.h"
+#include "rebase-am.h"
+#include "run-command.h"
+
+GIT_PATH_FUNC(git_path_rebase_am_dir, "rebase-apply");
+
+void rebase_am_init(struct rebase_am *state, const char *dir)
+{
+	if (!dir)
+		dir = git_path_rebase_am_dir();
+	rebase_options_init(&state->opts);
+	state->dir = xstrdup(dir);
+}
+
+void rebase_am_release(struct rebase_am *state)
+{
+	rebase_options_release(&state->opts);
+	free(state->dir);
+}
+
+int rebase_am_in_progress(const struct rebase_am *state)
+{
+	const char *dir = state ? state->dir : git_path_rebase_am_dir();
+	struct stat st;
+
+	return !lstat(dir, &st) && S_ISDIR(st.st_mode);
+}
+
+int rebase_am_load(struct rebase_am *state)
+{
+	if (rebase_options_load(&state->opts, state->dir) < 0)
+		return -1;
+
+	return 0;
+}
+
+static int run_format_patch(const char *patches, const struct object_id *left,
+		const struct object_id *right)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int ret;
+
+	cp.git_cmd = 1;
+	cp.out = xopen(patches, O_WRONLY | O_CREAT, 0777);
+	argv_array_push(&cp.args, "format-patch");
+	argv_array_push(&cp.args, "-k");
+	argv_array_push(&cp.args, "--stdout");
+	argv_array_push(&cp.args, "--full-index");
+	argv_array_push(&cp.args, "--cherry-pick");
+	argv_array_push(&cp.args, "--right-only");
+	argv_array_push(&cp.args, "--src-prefix=a/");
+	argv_array_push(&cp.args, "--dst-prefix=b/");
+	argv_array_push(&cp.args, "--no-renames");
+	argv_array_push(&cp.args, "--no-cover-letter");
+	argv_array_pushf(&cp.args, "%s...%s", oid_to_hex(left), oid_to_hex(right));
+
+	ret = run_command(&cp);
+	close(cp.out);
+	return ret;
+}
+
+static int run_am(const struct rebase_am *state, const char *patches)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int ret;
+
+	cp.git_cmd = 1;
+	cp.in = xopen(patches, O_RDONLY);
+	argv_array_push(&cp.args, "am");
+	argv_array_push(&cp.args, "--rebasing");
+	if (state->opts.resolvemsg)
+		argv_array_pushf(&cp.args, "--resolvemsg=%s", state->opts.resolvemsg);
+
+	ret = run_command(&cp);
+	close(cp.in);
+	return ret;
+}
+
+void rebase_am_run(struct rebase_am *state)
+{
+	char *patches;
+	int ret;
+
+	rebase_common_setup(&state->opts, state->dir);
+
+	patches = git_pathdup("rebased-patches");
+	ret = run_format_patch(patches, &state->opts.upstream, &state->opts.orig_head);
+	if (ret) {
+		unlink_or_warn(patches);
+		fprintf_ln(stderr, _("\ngit encountered an error while preparing the patches to replay\n"
+			"these revisions:\n"
+			"\n"
+			"    %s...%s\n"
+			"\n"
+			"As a result, git cannot rebase them."),
+				oid_to_hex(&state->opts.upstream),
+				oid_to_hex(&state->opts.orig_head));
+		exit(ret);
+	}
+
+	ret = run_am(state, patches);
+	unlink_or_warn(patches);
+	if (ret) {
+		rebase_options_save(&state->opts, state->dir);
+		exit(ret);
+	}
+
+	free(patches);
+	rebase_common_finish(&state->opts, state->dir);
+}
diff --git a/rebase-am.h b/rebase-am.h
new file mode 100644
index 0000000..0b4348c
--- /dev/null
+++ b/rebase-am.h
@@ -0,0 +1,22 @@
+#ifndef REBASE_AM_H
+#define REBASE_AM_H
+#include "rebase-common.h"
+
+const char *git_path_rebase_am_dir(void);
+
+struct rebase_am {
+	struct rebase_options opts;
+	char *dir;
+};
+
+void rebase_am_init(struct rebase_am *, const char *dir);
+
+void rebase_am_release(struct rebase_am *);
+
+int rebase_am_in_progress(const struct rebase_am *);
+
+int rebase_am_load(struct rebase_am *);
+
+void rebase_am_run(struct rebase_am *);
+
+#endif /* REBASE_AM_H */
diff --git a/rebase-common.c b/rebase-common.c
index 1835f08..8169fb6 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -1,5 +1,8 @@
 #include "cache.h"
 #include "rebase-common.h"
+#include "dir.h"
+#include "run-command.h"
+#include "refs.h"
 
 void rebase_options_init(struct rebase_options *opts)
 {
@@ -95,3 +98,81 @@ void rebase_options_save(const struct rebase_options *opts, const char *dir)
 	write_state_text(dir, "onto", oid_to_hex(&opts->onto));
 	write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
 }
+
+static int detach_head(const struct object_id *commit, const char *onto_name)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+	const char *reflog_action = getenv("GIT_REFLOG_ACTION");
+	if (!reflog_action || !*reflog_action)
+		reflog_action = "rebase";
+	cp.git_cmd = 1;
+	argv_array_pushf(&cp.env_array, "GIT_REFLOG_ACTION=%s: checkout %s",
+			reflog_action, onto_name ? onto_name : oid_to_hex(commit));
+	argv_array_push(&cp.args, "checkout");
+	argv_array_push(&cp.args, "-q");
+	argv_array_push(&cp.args, "--detach");
+	argv_array_push(&cp.args, oid_to_hex(commit));
+	status = run_command(&cp);
+
+	/* reload cache as checkout will have modified it */
+	discard_cache();
+	read_cache();
+
+	return status;
+}
+
+void rebase_common_setup(struct rebase_options *opts, const char *dir)
+{
+	/* Detach HEAD and reset the tree */
+	printf_ln(_("First, rewinding head to replay your work on top of it..."));
+	if (detach_head(&opts->onto, opts->onto_name))
+		die(_("could not detach HEAD"));
+	update_ref("rebase", "ORIG_HEAD", opts->orig_head.hash, NULL, 0,
+			UPDATE_REFS_DIE_ON_ERR);
+}
+
+void rebase_common_destroy(struct rebase_options *opts, const char *dir)
+{
+	struct strbuf sb = STRBUF_INIT;
+	strbuf_addstr(&sb, dir);
+	remove_dir_recursively(&sb, 0);
+	strbuf_release(&sb);
+}
+
+static void move_to_original_branch(struct rebase_options *opts)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct object_id curr_head;
+
+	if (!opts->orig_refname || !starts_with(opts->orig_refname, "refs/"))
+		return;
+
+	if (get_sha1("HEAD", curr_head.hash) < 0)
+		die("get_sha1() failed");
+
+	strbuf_addf(&sb, "rebase finished: %s onto %s", opts->orig_refname, oid_to_hex(&opts->onto));
+	if (update_ref(sb.buf, opts->orig_refname, curr_head.hash, opts->orig_head.hash, 0, UPDATE_REFS_MSG_ON_ERR))
+		goto fail;
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "rebase finished: returning to %s", opts->orig_refname);
+	if (create_symref("HEAD", opts->orig_refname, sb.buf))
+		goto fail;
+
+	strbuf_release(&sb);
+
+	return;
+fail:
+	die(_("Could not move back to %s"), opts->orig_refname);
+}
+
+void rebase_common_finish(struct rebase_options *opts, const char *dir)
+{
+	const char *argv_gc_auto[] = {"gc", "--auto", NULL};
+
+	move_to_original_branch(opts);
+	close_all_packs();
+	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
+	rebase_common_destroy(opts, dir);
+}
diff --git a/rebase-common.h b/rebase-common.h
index 051c056..067ad0b 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -24,4 +24,10 @@ int rebase_options_load(struct rebase_options *, const char *dir);
 
 void rebase_options_save(const struct rebase_options *, const char *dir);
 
+void rebase_common_setup(struct rebase_options *, const char *dir);
+
+void rebase_common_destroy(struct rebase_options *, const char *dir);
+
+void rebase_common_finish(struct rebase_options *, const char *dir);
+
 #endif /* REBASE_COMMON_H */
-- 
2.7.0

[PATCH/RFC/GSoC 07/17] rebase-common: implement refresh_and_write_cache()

[Paul Tan]

In the upcoming git-rebase to C rewrite, it is a common operation to
refresh the index and write the resulting index.

builtin/am.c already implements refresh_and_write_cache(), which is what
we want. Move it to rebase-common.c, so that it can be shared with all
the rebase backends, including git-am.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c    | 14 +-------------
 rebase-common.c | 11 +++++++++++
 rebase-common.h |  5 +++++
 3 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d003939..504b604 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -28,6 +28,7 @@
 #include "rerere.h"
 #include "prompt.h"
 #include "mailinfo.h"
+#include "rebase-common.h"
 
 /**
  * Returns 1 if the file is empty or does not exist, 0 otherwise.
@@ -1125,19 +1126,6 @@ static const char *msgnum(const struct am_state *state)
 }
 
 /**
- * Refresh and write index.
- */
-static void refresh_and_write_cache(void)
-{
-	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
-
-	hold_locked_index(lock_file, 1);
-	refresh_cache(REFRESH_QUIET);
-	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
-		die(_("unable to write index file"));
-}
-
-/**
  * Returns 1 if the index differs from HEAD, 0 otherwise. When on an unborn
  * branch, returns 1 if there are entries in the index, 0 otherwise. If an
  * strbuf is provided, the space-separated list of files that differ will be
diff --git a/rebase-common.c b/rebase-common.c
index 8169fb6..b07e1f1 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -3,6 +3,17 @@
 #include "dir.h"
 #include "run-command.h"
 #include "refs.h"
+#include "lockfile.h"
+
+void refresh_and_write_cache(void)
+{
+	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
+
+	hold_locked_index(lock_file, 1);
+	refresh_cache(REFRESH_QUIET);
+	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
+		die(_("unable to write index file"));
+}
 
 void rebase_options_init(struct rebase_options *opts)
 {
diff --git a/rebase-common.h b/rebase-common.h
index 067ad0b..8620e8c 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -1,6 +1,11 @@
 #ifndef REBASE_COMMON_H
 #define REBASE_COMMON_H
 
+/**
+ * Refresh and write index.
+ */
+void refresh_and_write_cache(void);
+
 /* common rebase backend options */
 struct rebase_options {
 	struct object_id onto;
-- 
2.7.0

[PATCH/RFC/GSoC 08/17] rebase-common: let refresh_and_write_cache() take a flags argument

[Paul Tan]

refresh_and_write_cache() is a handy function for refreshing the index
and writing the resulting index back to the filesystem. However, it
always calls refresh_cache() with REFRESH_QUIET. Allow callers to modify
the behavior of refresh_cache() by allowing callers to pass a flags
argument to refresh_cache().

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/am.c    | 2 +-
 rebase-common.c | 4 ++--
 rebase-common.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 504b604..5185719 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1815,7 +1815,7 @@ static void am_run(struct am_state *state, int resume)
 
 	unlink(am_path(state, "dirtyindex"));
 
-	refresh_and_write_cache();
+	refresh_and_write_cache(REFRESH_QUIET);
 
 	if (index_has_changes(&sb)) {
 		write_state_bool(state, "dirtyindex", 1);
diff --git a/rebase-common.c b/rebase-common.c
index b07e1f1..97b0687 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -5,12 +5,12 @@
 #include "refs.h"
 #include "lockfile.h"
 
-void refresh_and_write_cache(void)
+void refresh_and_write_cache(unsigned int flags)
 {
 	struct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));
 
 	hold_locked_index(lock_file, 1);
-	refresh_cache(REFRESH_QUIET);
+	refresh_cache(flags);
 	if (write_locked_index(&the_index, lock_file, COMMIT_LOCK))
 		die(_("unable to write index file"));
 }
diff --git a/rebase-common.h b/rebase-common.h
index 8620e8c..4586f03 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -4,7 +4,7 @@
 /**
  * Refresh and write index.
  */
-void refresh_and_write_cache(void);
+void refresh_and_write_cache(unsigned int);
 
 /* common rebase backend options */
 struct rebase_options {
-- 
2.7.0

[PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Paul Tan]

In the upcoming git-rebase-to-C rewrite, it is a common operation to
check if the worktree has unstaged changes, so that it can complain that
the worktree is dirty.

builtin/pull.c already implements this function. Move it to
rebase-common.c so that it can be shared between all rebase backends and
git-pull.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 19 ++-----------------
 rebase-common.c | 14 ++++++++++++++
 rebase-common.h |  5 +++++
 3 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 10eff03..9e65dc9 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -17,6 +17,7 @@
 #include "revision.h"
 #include "tempfile.h"
 #include "lockfile.h"
+#include "rebase-common.h"
 
 enum rebase_type {
 	REBASE_INVALID = -1,
@@ -306,22 +307,6 @@ static enum rebase_type config_get_rebase(void)
 }
 
 /**
- * Returns 1 if there are unstaged changes, 0 otherwise.
- */
-static int has_unstaged_changes(const char *prefix)
-{
-	struct rev_info rev_info;
-	int result;
-
-	init_revisions(&rev_info, prefix);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_files(&rev_info, 0);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
  * Returns 1 if there are uncommitted changes, 0 otherwise.
  */
 static int has_uncommitted_changes(const char *prefix)
@@ -355,7 +340,7 @@ static void die_on_unclean_work_tree(const char *prefix)
 	update_index_if_able(&the_index, lock_file);
 	rollback_lock_file(lock_file);
 
-	if (has_unstaged_changes(prefix)) {
+	if (cache_has_unstaged_changes()) {
 		error(_("Cannot pull with rebase: You have unstaged changes."));
 		do_die = 1;
 	}
diff --git a/rebase-common.c b/rebase-common.c
index 97b0687..61be8f1 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -4,6 +4,7 @@
 #include "run-command.h"
 #include "refs.h"
 #include "lockfile.h"
+#include "revision.h"
 
 void refresh_and_write_cache(unsigned int flags)
 {
@@ -15,6 +16,19 @@ void refresh_and_write_cache(unsigned int flags)
 		die(_("unable to write index file"));
 }
 
+int cache_has_unstaged_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_files(&rev_info, 0);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
 void rebase_options_init(struct rebase_options *opts)
 {
 	oidclr(&opts->onto);
diff --git a/rebase-common.h b/rebase-common.h
index 4586f03..9d14e25 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -6,6 +6,11 @@
  */
 void refresh_and_write_cache(unsigned int);
 
+/**
+ * Returns 1 if there are unstaged changes, 0 otherwise.
+ */
+int cache_has_unstaged_changes(void);
+
 /* common rebase backend options */
 struct rebase_options {
 	struct object_id onto;
-- 
2.7.0

[PATCH/RFC/GSoC 10/17] rebase-common: implement cache_has_uncommitted_changes()

[Paul Tan]

In the upcoming git-rebase-to-C rewrite, it is a common opertation to
check if the index has uncommitted changes, so that rebase can complain
that the index is dirty, or commit the uncommitted changes in the index.

builtin/pull.c already implements the function we want. Move it to
rebase-common.c so that it can be shared between all rebase backends and
git-pull.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 builtin/pull.c  | 22 +---------------------
 rebase-common.c | 17 +++++++++++++++++
 rebase-common.h |  5 +++++
 3 files changed, 23 insertions(+), 21 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 9e65dc9..6be4213 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -307,26 +307,6 @@ static enum rebase_type config_get_rebase(void)
 }
 
 /**
- * Returns 1 if there are uncommitted changes, 0 otherwise.
- */
-static int has_uncommitted_changes(const char *prefix)
-{
-	struct rev_info rev_info;
-	int result;
-
-	if (is_cache_unborn())
-		return 0;
-
-	init_revisions(&rev_info, prefix);
-	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
-	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
-	add_head_to_pending(&rev_info);
-	diff_setup_done(&rev_info.diffopt);
-	result = run_diff_index(&rev_info, 1);
-	return diff_result_code(&rev_info.diffopt, result);
-}
-
-/**
  * If the work tree has unstaged or uncommitted changes, dies with the
  * appropriate message.
  */
@@ -345,7 +325,7 @@ static void die_on_unclean_work_tree(const char *prefix)
 		do_die = 1;
 	}
 
-	if (has_uncommitted_changes(prefix)) {
+	if (cache_has_uncommitted_changes()) {
 		if (do_die)
 			error(_("Additionally, your index contains uncommitted changes."));
 		else
diff --git a/rebase-common.c b/rebase-common.c
index 61be8f1..94783a9 100644
--- a/rebase-common.c
+++ b/rebase-common.c
@@ -29,6 +29,23 @@ int cache_has_unstaged_changes(void)
 	return diff_result_code(&rev_info.diffopt, result);
 }
 
+int cache_has_uncommitted_changes(void)
+{
+	struct rev_info rev_info;
+	int result;
+
+	if (is_cache_unborn())
+		return 0;
+
+	init_revisions(&rev_info, NULL);
+	DIFF_OPT_SET(&rev_info.diffopt, IGNORE_SUBMODULES);
+	DIFF_OPT_SET(&rev_info.diffopt, QUICK);
+	add_head_to_pending(&rev_info);
+	diff_setup_done(&rev_info.diffopt);
+	result = run_diff_index(&rev_info, 1);
+	return diff_result_code(&rev_info.diffopt, result);
+}
+
 void rebase_options_init(struct rebase_options *opts)
 {
 	oidclr(&opts->onto);
diff --git a/rebase-common.h b/rebase-common.h
index 9d14e25..97d9a5b 100644
--- a/rebase-common.h
+++ b/rebase-common.h
@@ -11,6 +11,11 @@ void refresh_and_write_cache(unsigned int);
  */
 int cache_has_unstaged_changes(void);
 
+/**
+ * Returns 1 if there are uncommitted changes, 0 otherwise.
+ */
+int cache_has_uncommitted_changes(void);
+
 /* common rebase backend options */
 struct rebase_options {
 	struct object_id onto;
-- 
2.7.0

[PATCH/RFC/GSoC 11/17] rebase-merge: introduce merge backend for builtin rebase

[Paul Tan]

Since 58634db (rebase: Allow merge strategies to be used when rebasing,
2006-06-21), git-rebase supported rebasing with a merge strategy when
the -m switch is used.

Re-implement a skeletal version of the above method of rebasing in a new
rebase-merge backend for our builtin-rebase. This skeletal version is
only able to re-apply commits using the merge-recursive strategy, and is
unable to resume from a conflict. Subsequent patches will re-implement
all the missing features.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile         |   1 +
 builtin/rebase.c |  17 +++-
 rebase-merge.c   | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-merge.h   |  28 ++++++
 4 files changed, 300 insertions(+), 2 deletions(-)
 create mode 100644 rebase-merge.c
 create mode 100644 rebase-merge.h

diff --git a/Makefile b/Makefile
index a2618ea..d43e068 100644
--- a/Makefile
+++ b/Makefile
@@ -781,6 +781,7 @@ LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
 LIB_OBJS += rebase-am.o
 LIB_OBJS += rebase-common.o
+LIB_OBJS += rebase-merge.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/builtin/rebase.c b/builtin/rebase.c
index ec63d3b..6d42115 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -9,10 +9,12 @@
 #include "branch.h"
 #include "refs.h"
 #include "rebase-am.h"
+#include "rebase-merge.h"
 
 enum rebase_type {
 	REBASE_TYPE_NONE = 0,
-	REBASE_TYPE_AM
+	REBASE_TYPE_AM,
+	REBASE_TYPE_MERGE
 };
 
 static const char *rebase_dir(enum rebase_type type)
@@ -20,6 +22,8 @@ static const char *rebase_dir(enum rebase_type type)
 	switch (type) {
 	case REBASE_TYPE_AM:
 		return git_path_rebase_am_dir();
+	case REBASE_TYPE_MERGE:
+		return git_path_rebase_merge_dir();
 	default:
 		die("BUG: invalid rebase_type %d", type);
 	}
@@ -137,6 +141,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct rebase_options rebase_opts;
 	const char *onto_name = NULL;
 	const char *branch_name;
+	int do_merge = 0;
 
 	const char * const usage[] = {
 		N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
@@ -146,6 +151,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_GROUP(N_("Available options are")),
 		OPT_STRING(0, "onto", &onto_name, NULL,
 			N_("rebase onto given branch instead of upstream")),
+		OPT_BOOL('m', "merge", &do_merge,
+			N_("use merging strategies to rebase")),
 		OPT_END()
 	};
 
@@ -225,7 +232,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Run the appropriate rebase backend */
-	{
+	if (do_merge) {
+		struct rebase_merge state;
+		rebase_merge_init(&state, rebase_dir(REBASE_TYPE_MERGE));
+		rebase_options_swap(&state.opts, &rebase_opts);
+		rebase_merge_run(&state);
+		rebase_merge_release(&state);
+	} else {
 		struct rebase_am state;
 		rebase_am_init(&state, rebase_dir(REBASE_TYPE_AM));
 		rebase_options_swap(&state.opts, &rebase_opts);
diff --git a/rebase-merge.c b/rebase-merge.c
new file mode 100644
index 0000000..dc96faf
--- /dev/null
+++ b/rebase-merge.c
@@ -0,0 +1,256 @@
+#include "cache.h"
+#include "rebase-merge.h"
+#include "run-command.h"
+#include "dir.h"
+#include "revision.h"
+
+GIT_PATH_FUNC(git_path_rebase_merge_dir, "rebase-merge");
+
+void rebase_merge_init(struct rebase_merge *state, const char *dir)
+{
+	if (!dir)
+		dir = git_path_rebase_merge_dir();
+	rebase_options_init(&state->opts);
+	state->dir = xstrdup(dir);
+	state->msgnum = 0;
+	state->end = 0;
+	state->prec = 4;
+}
+
+void rebase_merge_release(struct rebase_merge *state)
+{
+	rebase_options_release(&state->opts);
+	free(state->dir);
+}
+
+int rebase_merge_in_progress(const struct rebase_merge *state)
+{
+	const char *dir = state ? state->dir : git_path_rebase_merge_dir();
+	struct stat st;
+
+	if (lstat(dir, &st) || !S_ISDIR(st.st_mode))
+		return 0;
+
+	if (file_exists(mkpath("%s/interactive", dir)))
+		return 0;
+
+	return 1;
+}
+
+static const char *state_path(const struct rebase_merge *state, const char *filename)
+{
+	return mkpath("%s/%s", state->dir, filename);
+}
+
+static int read_state_file(const struct rebase_merge *state, const char *filename, struct strbuf *sb)
+{
+	const char *path = state_path(state, filename);
+	if (strbuf_read_file(sb, path, 0) < 0)
+		return error(_("could not read file %s"), path);
+	strbuf_trim(sb);
+	return 0;
+}
+
+static int read_state_ui(const struct rebase_merge *state, const char *filename, unsigned int *ui)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (read_state_file(state, filename, &sb) < 0) {
+		strbuf_release(&sb);
+		return -1;
+	}
+	if (strtoul_ui(sb.buf, 10, ui) < 0) {
+		strbuf_release(&sb);
+		return error(_("could not parse %s"), state_path(state, filename));
+	}
+	strbuf_release(&sb);
+	return 0;
+}
+
+static int read_state_oid(const struct rebase_merge *state, const char *filename, struct object_id *oid)
+{
+	struct strbuf sb = STRBUF_INIT;
+	if (read_state_file(state, filename, &sb) < 0) {
+		strbuf_release(&sb);
+		return -1;
+	}
+	if (sb.len != GIT_SHA1_HEXSZ || get_oid_hex(sb.buf, oid)) {
+		strbuf_release(&sb);
+		return error(_("could not parse %s"), state_path(state, filename));
+	}
+	strbuf_release(&sb);
+	return 0;
+}
+
+static int read_state_msgnum(const struct rebase_merge *state, unsigned int msgnum, struct object_id *oid)
+{
+	char *filename = xstrfmt("cmt.%u", msgnum);
+	int ret = read_state_oid(state, filename, oid);
+	free(filename);
+	return ret;
+}
+
+int rebase_merge_load(struct rebase_merge *state)
+{
+	struct strbuf sb = STRBUF_INIT;
+
+	if (rebase_options_load(&state->opts, state->dir) < 0)
+		return -1;
+
+	if (read_state_file(state, "onto_name", &sb) < 0)
+		return -1;
+	free(state->opts.onto_name);
+	state->opts.onto_name = strbuf_detach(&sb, NULL);
+
+	if (read_state_ui(state, "msgnum", &state->msgnum) < 0)
+		return -1;
+
+	if (read_state_ui(state, "end", &state->end) < 0)
+		return -1;
+
+	strbuf_release(&sb);
+	return 0;
+}
+
+static void write_state_text(const struct rebase_merge *state, const char *filename, const char *text)
+{
+	write_file(state_path(state, filename), "%s", text);
+}
+
+static void write_state_ui(const struct rebase_merge *state, const char *filename, unsigned int ui)
+{
+	write_file(state_path(state, filename), "%u", ui);
+}
+
+static void write_state_oid(const struct rebase_merge *state, const char *filename, const struct object_id *oid)
+{
+	write_file(state_path(state, filename), "%s", oid_to_hex(oid));
+}
+
+static void rebase_merge_finish(struct rebase_merge *state)
+{
+	rebase_common_finish(&state->opts, state->dir);
+	printf_ln(_("All done."));
+}
+
+/**
+ * Setup commits to be rebased.
+ */
+static unsigned int setup_commits(const char *dir, const struct object_id *upstream, const struct object_id *head)
+{
+	struct rev_info revs;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct commit *commit;
+	unsigned int msgnum = 0;
+
+	init_revisions(&revs, NULL);
+	argv_array_pushl(&args, "rev-list", "--reverse", "--no-merges", NULL);
+	argv_array_pushf(&args, "%s..%s", oid_to_hex(upstream), oid_to_hex(head));
+	setup_revisions(args.argc, args.argv, &revs, NULL);
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+	while ((commit = get_revision(&revs)))
+		write_file(mkpath("%s/cmt.%u", dir, ++msgnum), "%s", oid_to_hex(&commit->object.oid));
+	reset_revision_walk();
+	argv_array_clear(&args);
+	return msgnum;
+}
+
+/**
+ * Merge HEAD with oid
+ */
+static void do_merge(struct rebase_merge *state, unsigned int msgnum, const struct object_id *oid)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int ret;
+
+	cp.git_cmd = 1;
+	argv_array_pushf(&cp.env_array, "GITHEAD_%s=HEAD~%u", oid_to_hex(oid), state->end - msgnum);
+	argv_array_pushf(&cp.env_array, "GITHEAD_HEAD=%s", state->opts.onto_name ? state->opts.onto_name : oid_to_hex(&state->opts.onto));
+	argv_array_push(&cp.args, "merge-recursive");
+	argv_array_pushf(&cp.args, "%s^", oid_to_hex(oid));
+	argv_array_push(&cp.args, "--");
+	argv_array_push(&cp.args, "HEAD");
+	argv_array_push(&cp.args, oid_to_hex(oid));
+	ret = run_command(&cp);
+	switch (ret) {
+	case 0:
+		break;
+	case 1:
+		if (state->opts.resolvemsg)
+			fprintf_ln(stderr, "%s", state->opts.resolvemsg);
+		exit(1);
+	case 2:
+		fprintf_ln(stderr, _("Strategy: recursive failed, try another"));
+		if (state->opts.resolvemsg)
+			fprintf_ln(stderr, "%s", state->opts.resolvemsg);
+		exit(1);
+	default:
+		fprintf_ln(stderr, _("Unknown exit code (%d) from command"), ret);
+		exit(1);
+	}
+
+	discard_cache();
+	read_cache();
+}
+
+/**
+ * Commit index
+ */
+static void do_commit(struct rebase_merge *state, unsigned int msgnum, const struct object_id *oid)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	if (!cache_has_uncommitted_changes()) {
+		printf_ln(_("Already applied: %0*d"), state->prec, msgnum);
+		return;
+	}
+
+	cp.git_cmd = 1;
+	argv_array_push(&cp.args, "commit");
+	argv_array_push(&cp.args, "--no-verify");
+	argv_array_pushl(&cp.args, "-C", oid_to_hex(oid), NULL);
+	if (run_command(&cp)) {
+
+		fprintf_ln(stderr, _("Commit failed, please do not call \"git commit\"\n"
+					"directly, but instead do one of the following:"));
+		if (state->opts.resolvemsg)
+			fprintf_ln(stderr, "%s", state->opts.resolvemsg);
+		exit(1);
+	}
+	printf_ln(_("Committed: %0*d"), state->prec, msgnum);
+}
+
+static void do_rest(struct rebase_merge *state)
+{
+	while (state->msgnum <= state->end) {
+		struct object_id oid;
+
+		if (read_state_msgnum(state, state->msgnum, &oid) < 0)
+			die("could not read msgnum commit");
+		write_state_oid(state, "current", &oid);
+		do_merge(state, state->msgnum, &oid);
+		do_commit(state, state->msgnum, &oid);
+		write_state_ui(state, "msgnum", ++state->msgnum);
+	}
+
+	rebase_merge_finish(state);
+}
+
+void rebase_merge_run(struct rebase_merge *state)
+{
+	rebase_common_setup(&state->opts, state->dir);
+
+	if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
+		die_errno(_("failed to create directory '%s'"), state->dir);
+
+	rebase_options_save(&state->opts, state->dir);
+	write_state_text(state, "onto_name", state->opts.onto_name ? state->opts.onto_name : oid_to_hex(&state->opts.onto));
+
+	state->msgnum = 1;
+	write_state_ui(state, "msgnum", state->msgnum);
+
+	state->end = setup_commits(state->dir, &state->opts.upstream, &state->opts.orig_head);
+	write_state_ui(state, "end", state->end);
+
+	do_rest(state);
+}
diff --git a/rebase-merge.h b/rebase-merge.h
new file mode 100644
index 0000000..f0b54ef
--- /dev/null
+++ b/rebase-merge.h
@@ -0,0 +1,28 @@
+#ifndef REBASE_MERGE_H
+#define REBASE_MERGE_H
+#include "rebase-common.h"
+
+const char *git_path_rebase_merge_dir(void);
+
+/*
+ * The rebase_merge backend is a merge-based non-interactive mode that copes
+ * well with renamed files.
+ */
+struct rebase_merge {
+	struct rebase_options opts;
+	char *dir;
+	unsigned int msgnum, end;
+	int prec;
+};
+
+void rebase_merge_init(struct rebase_merge *, const char *dir);
+
+void rebase_merge_release(struct rebase_merge *);
+
+int rebase_merge_in_progress(const struct rebase_merge *);
+
+int rebase_merge_load(struct rebase_merge *);
+
+void rebase_merge_run(struct rebase_merge *);
+
+#endif /* REBASE_MERGE_H */
-- 
2.7.0

[PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

[Paul Tan]

In an interactive rebase, commands are read and executed from a todo
list (.git/rebase-merge/git-rebase-todo) to perform the rebase.

In the upcoming re-implementation of git-rebase -i in C, it is useful to
be able to parse each command into a data structure which can then be
operated on. Implement rebase_todo_item for this.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile      |   1 +
 rebase-todo.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-todo.h |  28 ++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 rebase-todo.c
 create mode 100644 rebase-todo.h

diff --git a/Makefile b/Makefile
index d43e068..8b928e4 100644
--- a/Makefile
+++ b/Makefile
@@ -782,6 +782,7 @@ LIB_OBJS += read-cache.o
 LIB_OBJS += rebase-am.o
 LIB_OBJS += rebase-common.o
 LIB_OBJS += rebase-merge.o
+LIB_OBJS += rebase-todo.o
 LIB_OBJS += reflog-walk.o
 LIB_OBJS += refs.o
 LIB_OBJS += refs/files-backend.o
diff --git a/rebase-todo.c b/rebase-todo.c
new file mode 100644
index 0000000..ac6b222
--- /dev/null
+++ b/rebase-todo.c
@@ -0,0 +1,144 @@
+#include "cache.h"
+#include "rebase-todo.h"
+
+/*
+ * Used as the default `rest` value, so that users can always assume `rest` is
+ * non NULL and `rest` is NUL terminated even for a freshly initialized
+ * rebase_todo_item.
+ */
+static char rebase_todo_item_slopbuf[1];
+
+void rebase_todo_item_init(struct rebase_todo_item *item)
+{
+	item->action = REBASE_TODO_NONE;
+	oidclr(&item->oid);
+	item->rest = rebase_todo_item_slopbuf;
+}
+
+void rebase_todo_item_release(struct rebase_todo_item *item)
+{
+	if (item->rest != rebase_todo_item_slopbuf)
+		free(item->rest);
+	rebase_todo_item_init(item);
+}
+
+void rebase_todo_item_copy(struct rebase_todo_item *dst, const struct rebase_todo_item *src)
+{
+	if (dst->rest != rebase_todo_item_slopbuf)
+		free(dst->rest);
+	*dst = *src;
+	dst->rest = xstrdup(src->rest);
+}
+
+static const char *next_word(struct strbuf *sb, const char *str)
+{
+	const char *end;
+
+	while (*str && isspace(*str))
+		str++;
+
+	end = str;
+	while (*end && !isspace(*end))
+		end++;
+
+	strbuf_reset(sb);
+	strbuf_add(sb, str, end - str);
+	return end;
+}
+
+int rebase_todo_item_parse(struct rebase_todo_item *item, const char *line, int abbrev)
+{
+	struct strbuf word = STRBUF_INIT;
+	const char *str = line;
+	int has_oid = 1, ret = 0;
+
+	while (*str && isspace(*str))
+		str++;
+
+	if (!*str || *str == comment_line_char) {
+		item->action = REBASE_TODO_NONE;
+		oidclr(&item->oid);
+		if (item->rest != rebase_todo_item_slopbuf)
+			free(item->rest);
+		item->rest = *str ? xstrdup(str) : rebase_todo_item_slopbuf;
+		return 0;
+	}
+
+	str = next_word(&word, str);
+	if (!strcmp(word.buf, "noop")) {
+		item->action = REBASE_TODO_NOOP;
+		has_oid = 0;
+	} else if (!strcmp(word.buf, "pick") || !strcmp(word.buf, "p")) {
+		item->action = REBASE_TODO_PICK;
+	} else {
+		ret = error(_("Unknown command: %s"), word.buf);
+		goto finish;
+	}
+
+	if (has_oid) {
+		str = next_word(&word, str);
+		if (abbrev) {
+			/* accept abbreviated object ids */
+			if (get_oid_commit(word.buf, &item->oid)) {
+				ret = error(_("Not a commit: %s"), word.buf);
+				goto finish;
+			}
+		} else {
+			if (word.len != GIT_SHA1_HEXSZ || get_oid_hex(word.buf, &item->oid)) {
+				ret = error(_("Invalid line: %s"), line);
+				goto finish;
+			}
+		}
+	} else {
+		oidclr(&item->oid);
+	}
+
+	if (*str && isspace(*str))
+		str++;
+	if (*str) {
+		if (item->rest != rebase_todo_item_slopbuf)
+			free(item->rest);
+		item->rest = xstrdup(str);
+	}
+
+finish:
+	strbuf_release(&word);
+	return ret;
+}
+
+void strbuf_add_rebase_todo_item(struct strbuf *sb,
+				 const struct rebase_todo_item *item, int abbrev)
+{
+	int has_oid = 1;
+
+	switch (item->action) {
+	case REBASE_TODO_NONE:
+		has_oid = 0;
+		break;
+	case REBASE_TODO_NOOP:
+		strbuf_addstr(sb, "noop");
+		has_oid = 0;
+		break;
+	case REBASE_TODO_PICK:
+		strbuf_addstr(sb, "pick");
+		break;
+	default:
+		die("BUG: invalid rebase_todo_item action %d", item->action);
+	}
+
+	if (has_oid) {
+		strbuf_addch(sb, ' ');
+		if (abbrev)
+			strbuf_addstr(sb, find_unique_abbrev((unsigned char *)&item->oid.hash, DEFAULT_ABBREV));
+		else
+			strbuf_addstr(sb, oid_to_hex(&item->oid));
+	}
+
+	if (*item->rest) {
+		if (item->action != REBASE_TODO_NONE)
+			strbuf_addch(sb, ' ');
+		strbuf_addstr(sb, item->rest);
+	}
+
+	strbuf_addch(sb, '\n');
+}
diff --git a/rebase-todo.h b/rebase-todo.h
new file mode 100644
index 0000000..2eedbb0
--- /dev/null
+++ b/rebase-todo.h
@@ -0,0 +1,28 @@
+#ifndef REBASE_TODO_H
+#define REBASE_TODO_H
+
+struct strbuf;
+
+enum rebase_todo_action {
+	REBASE_TODO_NONE = 0,
+	REBASE_TODO_NOOP,
+	REBASE_TODO_PICK
+};
+
+struct rebase_todo_item {
+	enum rebase_todo_action action;
+	struct object_id oid;
+	char *rest;
+};
+
+void rebase_todo_item_init(struct rebase_todo_item *);
+
+void rebase_todo_item_release(struct rebase_todo_item *);
+
+void rebase_todo_item_copy(struct rebase_todo_item *, const struct rebase_todo_item *);
+
+int rebase_todo_item_parse(struct rebase_todo_item *, const char *line, int abbrev);
+
+void strbuf_add_rebase_todo_item(struct strbuf *, const struct rebase_todo_item *, int abbrev);
+
+#endif /* REBASE_TODO_H */
-- 
2.7.0

[PATCH/RFC/GSoC 13/17] rebase-todo: introduce rebase_todo_list

[Paul Tan]

Implement rebase_todo_list, which is a resizable array of
rebase_todo_items.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 rebase-todo.c | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-todo.h |  27 +++++++++++++++
 2 files changed, 134 insertions(+)

diff --git a/rebase-todo.c b/rebase-todo.c
index ac6b222..4f14638 100644
--- a/rebase-todo.c
+++ b/rebase-todo.c
@@ -142,3 +142,110 @@ void strbuf_add_rebase_todo_item(struct strbuf *sb,
 
 	strbuf_addch(sb, '\n');
 }
+
+void rebase_todo_list_init(struct rebase_todo_list *list)
+{
+	list->items = NULL;
+	list->nr = 0;
+	list->alloc = 0;
+}
+
+void rebase_todo_list_clear(struct rebase_todo_list *list)
+{
+	unsigned int i;
+
+	for (i = 0; i < list->nr; i++)
+		rebase_todo_item_release(&list->items[i]);
+	free(list->items);
+	rebase_todo_list_init(list);
+}
+
+void rebase_todo_list_swap(struct rebase_todo_list *dst,
+			   struct rebase_todo_list *src)
+{
+	struct rebase_todo_list tmp = *dst;
+
+	*dst = *src;
+	*src = tmp;
+}
+
+unsigned int rebase_todo_list_count(const struct rebase_todo_list *list)
+{
+	unsigned int i, count = 0;
+
+	for (i = 0; i < list->nr; i++)
+		if (list->items[i].action != REBASE_TODO_NONE)
+			count++;
+	return count;
+}
+
+struct rebase_todo_item *rebase_todo_list_push(struct rebase_todo_list *list, const struct rebase_todo_item *src_item)
+{
+	struct rebase_todo_item *item = rebase_todo_list_push_empty(list);
+
+	rebase_todo_item_copy(item, src_item);
+	return item;
+}
+
+struct rebase_todo_item *rebase_todo_list_push_empty(struct rebase_todo_list *list)
+{
+	struct rebase_todo_item *item;
+
+	ALLOC_GROW(list->items, list->nr + 1, list->alloc);
+	item = &list->items[list->nr++];
+	rebase_todo_item_init(item);
+	return item;
+}
+
+struct rebase_todo_item *rebase_todo_list_push_noop(struct rebase_todo_list *list)
+{
+	struct rebase_todo_item *item = rebase_todo_list_push_empty(list);
+
+	item->action = REBASE_TODO_NOOP;
+	return item;
+}
+
+int rebase_todo_list_load(struct rebase_todo_list *list, const char *path, int abbrev)
+{
+	struct strbuf sb = STRBUF_INIT;
+	FILE *fp;
+
+	fp = fopen(path, "r");
+	if (!fp)
+		return error(_("could not open %s for reading"), path);
+
+	while (strbuf_getline(&sb, fp) != EOF) {
+		struct rebase_todo_item *item = rebase_todo_list_push_empty(list);
+		if (rebase_todo_item_parse(item, sb.buf, abbrev) < 0) {
+			rebase_todo_item_release(item);
+			list->nr--;
+			strbuf_release(&sb);
+			fclose(fp);
+			return -1;
+		}
+	}
+	strbuf_release(&sb);
+	fclose(fp);
+	return 0;
+}
+
+void rebase_todo_list_save(const struct rebase_todo_list *list, const char *filename, unsigned int offset, int abbrev)
+{
+	char *tmpfile = mkpathdup("%s.new", filename);
+	struct strbuf sb = STRBUF_INIT;
+	int fd;
+
+	for (; offset < list->nr; offset++)
+		strbuf_add_rebase_todo_item(&sb, &list->items[offset], abbrev);
+
+	fd = xopen(tmpfile, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+	if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		die_errno(_("could not write to %s"), tmpfile);
+	close(fd);
+	strbuf_release(&sb);
+
+	if (rename(tmpfile, filename))
+		die_errno(_("rename failed"));
+
+	free(tmpfile);
+}
diff --git a/rebase-todo.h b/rebase-todo.h
index 2eedbb0..f602fd2 100644
--- a/rebase-todo.h
+++ b/rebase-todo.h
@@ -25,4 +25,31 @@ int rebase_todo_item_parse(struct rebase_todo_item *, const char *line, int abbr
 
 void strbuf_add_rebase_todo_item(struct strbuf *, const struct rebase_todo_item *, int abbrev);
 
+struct rebase_todo_list {
+	struct rebase_todo_item *items;
+	unsigned int nr, alloc;
+};
+
+#define REBASE_TODO_LIST_INIT { NULL, 0, 0 }
+
+void rebase_todo_list_init(struct rebase_todo_list *);
+
+void rebase_todo_list_clear(struct rebase_todo_list *);
+
+void rebase_todo_list_swap(struct rebase_todo_list *dst, struct rebase_todo_list *src);
+
+unsigned int rebase_todo_list_count(const struct rebase_todo_list *);
+
+struct rebase_todo_item *rebase_todo_list_push(struct rebase_todo_list *,
+					       const struct rebase_todo_item *);
+
+struct rebase_todo_item *rebase_todo_list_push_empty(struct rebase_todo_list *);
+
+struct rebase_todo_item *rebase_todo_list_push_noop(struct rebase_todo_list *);
+
+int rebase_todo_list_load(struct rebase_todo_list *, const char *path, int abbrev);
+
+void rebase_todo_list_save(const struct rebase_todo_list *, const char *path,
+			   unsigned int offset, int abbrev);
+
 #endif /* REBASE_TODO_H */
-- 
2.7.0

[PATCH/RFC/GSoC 14/17] status: use rebase_todo_list

[Paul Tan]

Since 84e6fb9 (status: give more information during rebase -i,
2015-07-06), git status during an interactive rebase will show the list
of commands that are done and yet to be done. It implemented its own
hand-rolled parser in order to achieve this.

Now that we are able to fully parse interactive rebase's todo lists with
rebase_todo_list_parse(), use it in wt-status.c to reduce the amount of
code needed to implement this feature.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

This patch is just an illustration, and is not quite right as it does not strip
comments and blank lines like the original did.

 wt-status.c | 100 +++++++++++++++---------------------------------------------
 1 file changed, 25 insertions(+), 75 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index ab4f80d..96b82ef 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -15,6 +15,7 @@
 #include "column.h"
 #include "strbuf.h"
 #include "utf8.h"
+#include "rebase-todo.h"
 
 static const char cut_line[] =
 "------------------------ >8 ------------------------\n";
@@ -1026,94 +1027,39 @@ static int split_commit_in_progress(struct wt_status *s)
 	return split_in_progress;
 }
 
-/*
- * Turn
- * "pick d6a2f0303e897ec257dd0e0a39a5ccb709bc2047 some message"
- * into
- * "pick d6a2f03 some message"
- *
- * The function assumes that the line does not contain useless spaces
- * before or after the command.
- */
-static void abbrev_sha1_in_line(struct strbuf *line)
-{
-	struct strbuf **split;
-	int i;
-
-	if (starts_with(line->buf, "exec ") ||
-	    starts_with(line->buf, "x "))
-		return;
-
-	split = strbuf_split_max(line, ' ', 3);
-	if (split[0] && split[1]) {
-		unsigned char sha1[20];
-		const char *abbrev;
-
-		/*
-		 * strbuf_split_max left a space. Trim it and re-add
-		 * it after abbreviation.
-		 */
-		strbuf_trim(split[1]);
-		if (!get_sha1(split[1]->buf, sha1)) {
-			abbrev = find_unique_abbrev(sha1, DEFAULT_ABBREV);
-			strbuf_reset(split[1]);
-			strbuf_addf(split[1], "%s ", abbrev);
-			strbuf_reset(line);
-			for (i = 0; split[i]; i++)
-				strbuf_addf(line, "%s", split[i]->buf);
-		}
-	}
-	for (i = 0; split[i]; i++)
-		strbuf_release(split[i]);
-
-}
-
-static void read_rebase_todolist(const char *fname, struct string_list *lines)
-{
-	struct strbuf line = STRBUF_INIT;
-	FILE *f = fopen(git_path("%s", fname), "r");
-
-	if (!f)
-		die_errno("Could not open file %s for reading",
-			  git_path("%s", fname));
-	while (!strbuf_getline_lf(&line, f)) {
-		if (line.len && line.buf[0] == comment_line_char)
-			continue;
-		strbuf_trim(&line);
-		if (!line.len)
-			continue;
-		abbrev_sha1_in_line(&line);
-		string_list_append(lines, line.buf);
-	}
-}
-
 static void show_rebase_information(struct wt_status *s,
 					struct wt_status_state *state,
 					const char *color)
 {
 	if (state->rebase_interactive_in_progress) {
-		int i;
-		int nr_lines_to_show = 2;
+		unsigned int i;
+		unsigned int nr_lines_to_show = 2;
+		struct strbuf sb = STRBUF_INIT;
 
-		struct string_list have_done = STRING_LIST_INIT_DUP;
-		struct string_list yet_to_do = STRING_LIST_INIT_DUP;
+		struct rebase_todo_list have_done = REBASE_TODO_LIST_INIT;
+		struct rebase_todo_list yet_to_do = REBASE_TODO_LIST_INIT;
 
-		read_rebase_todolist("rebase-merge/done", &have_done);
-		read_rebase_todolist("rebase-merge/git-rebase-todo", &yet_to_do);
+		if (rebase_todo_list_load(&have_done, git_path("rebase-merge/done"), 1) < 0)
+			return;
+		if (rebase_todo_list_load(&yet_to_do, git_path("rebase-merge/git-rebase-todo"), 1) < 0)
+			return;
 
 		if (have_done.nr == 0)
 			status_printf_ln(s, color, _("No commands done."));
 		else {
 			status_printf_ln(s, color,
-				Q_("Last command done (%d command done):",
-					"Last commands done (%d commands done):",
+				Q_("Last command done (%u command done):",
+					"Last commands done (%u commands done):",
 					have_done.nr),
 				have_done.nr);
 			for (i = (have_done.nr > nr_lines_to_show)
 				? have_done.nr - nr_lines_to_show : 0;
 				i < have_done.nr;
-				i++)
-				status_printf_ln(s, color, "   %s", have_done.items[i].string);
+				i++) {
+				strbuf_reset(&sb);
+				strbuf_add_rebase_todo_item(&sb, &have_done.items[i], 1);
+				status_printf(s, color, "   %s", sb.buf);
+			}
 			if (have_done.nr > nr_lines_to_show && s->hints)
 				status_printf_ln(s, color,
 					_("  (see more in file %s)"), git_path("rebase-merge/done"));
@@ -1128,14 +1074,18 @@ static void show_rebase_information(struct wt_status *s,
 					"Next commands to do (%d remaining commands):",
 					yet_to_do.nr),
 				yet_to_do.nr);
-			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
-				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
+			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++) {
+				strbuf_reset(&sb);
+				strbuf_add_rebase_todo_item(&sb, &yet_to_do.items[i], 1);
+				status_printf(s, color, "   %s", sb.buf);
+			}
 			if (s->hints)
 				status_printf_ln(s, color,
 					_("  (use \"git rebase --edit-todo\" to view and edit)"));
 		}
-		string_list_clear(&yet_to_do, 0);
-		string_list_clear(&have_done, 0);
+		rebase_todo_list_clear(&yet_to_do);
+		rebase_todo_list_clear(&have_done);
+		strbuf_release(&sb);
 	}
 }
 
-- 
2.7.0

[PATCH/RFC/GSoC 15/17] wrapper: implement append_file()

[Paul Tan]

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 cache.h   |  1 +
 wrapper.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/cache.h b/cache.h
index 55d443e..aa5e97c 100644
--- a/cache.h
+++ b/cache.h
@@ -1700,6 +1700,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 
 extern int write_file(const char *path, const char *fmt, ...);
 extern int write_file_gently(const char *path, const char *fmt, ...);
+extern void append_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 9afc1a0..cd77e94 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -709,6 +709,29 @@ int write_file_gently(const char *path, const char *fmt, ...)
 	return status;
 }
 
+void append_file(const char *path, const char *fmt, ...)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int fd = open(path, O_WRONLY | O_CREAT | O_APPEND, 0666);
+	va_list params;
+	if (fd < 0)
+		die_errno(_("could not open %s for appending"), path);
+	va_start(params, fmt);
+	strbuf_vaddf(&sb, fmt, params);
+	va_end(params);
+	strbuf_complete_line(&sb);
+	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
+		int err = errno;
+		close(fd);
+		strbuf_release(&sb);
+		errno = err;
+		die_errno(_("could not write to %s"), path);
+	}
+	strbuf_release(&sb);
+	if (close(fd))
+		die_errno(_("could not close %s"), path);
+}
+
 void sleep_millisec(int millisec)
 {
 	poll(NULL, 0, millisec);
-- 
2.7.0

[PATCH/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()

[Paul Tan]

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 cache.h  |  1 +
 editor.c | 27 +++++++++++++++++++++++++--
 strbuf.h |  1 +
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/cache.h b/cache.h
index aa5e97c..d7a6fc6 100644
--- a/cache.h
+++ b/cache.h
@@ -1222,6 +1222,7 @@ extern const char *fmt_name(const char *name, const char *email);
 extern const char *ident_default_name(void);
 extern const char *ident_default_email(void);
 extern const char *git_editor(void);
+extern const char *git_sequence_editor(void);
 extern const char *git_pager(int stdout_is_tty);
 extern int git_ident_config(const char *, const char *, void *);
 
diff --git a/editor.c b/editor.c
index 01c644c..4c5874b 100644
--- a/editor.c
+++ b/editor.c
@@ -29,10 +29,22 @@ const char *git_editor(void)
 	return editor;
 }
 
-int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+const char *git_sequence_editor(void)
 {
-	const char *editor = git_editor();
+	const char *sequence_editor = getenv("GIT_SEQUENCE_EDITOR");
+
+	if (sequence_editor && *sequence_editor)
+		return sequence_editor;
 
+	git_config_get_string_const("sequence.editor", &sequence_editor);
+	if (sequence_editor && *sequence_editor)
+		return sequence_editor;
+
+	return git_editor();
+}
+
+static int launch_specific_editor(const char *editor, const char *path, struct strbuf *buffer, const char *const *env)
+{
 	if (!editor)
 		return error("Terminal is dumb, but EDITOR unset");
 
@@ -65,5 +77,16 @@ int launch_editor(const char *path, struct strbuf *buffer, const char *const *en
 	if (strbuf_read_file(buffer, path, 0) < 0)
 		return error("could not read file '%s': %s",
 				path, strerror(errno));
+
 	return 0;
 }
+
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	return launch_specific_editor(git_editor(), path, buffer, env);
+}
+
+int launch_sequence_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	return launch_specific_editor(git_sequence_editor(), path, buffer, env);
+}
diff --git a/strbuf.h b/strbuf.h
index f72fd14..aebdcd7 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -524,6 +524,7 @@ extern void strbuf_add_unique_abbrev(struct strbuf *sb,
  * file's contents are not read into the buffer upon completion.
  */
 extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_sequence_editor(const char *path, struct strbuf *buffer, const char *const *env);
 
 extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);
 
-- 
2.7.0

[PATCH/RFC/GSoC 17/17] rebase-interactive: introduce interactive backend for builtin rebase

[Paul Tan]

Since 1b1dce4 (Teach rebase an interactive mode, 2007-06-25), git-rebase
supports an interactive mode when passed the -i switch.

In interactive mode, git-rebase allows users to edit the list of patches
(using the user's GIT_SEQUENCE_EDITOR), so that the user can reorder,
edit and delete patches.

Re-implement a skeletal version of the above feature by introducing a
rebase-interactive backend for our builtin-rebase. This skeletal
implementation is only able to pick and re-order commits.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 Makefile             |   1 +
 builtin/rebase.c     |  17 ++-
 rebase-interactive.c | 375 +++++++++++++++++++++++++++++++++++++++++++++++++++
 rebase-interactive.h |  33 +++++
 4 files changed, 424 insertions(+), 2 deletions(-)
 create mode 100644 rebase-interactive.c
 create mode 100644 rebase-interactive.h

diff --git a/Makefile b/Makefile
index 8b928e4..3bd3127 100644
--- a/Makefile
+++ b/Makefile
@@ -781,6 +781,7 @@ LIB_OBJS += reachable.o
 LIB_OBJS += read-cache.o
 LIB_OBJS += rebase-am.o
 LIB_OBJS += rebase-common.o
+LIB_OBJS += rebase-interactive.o
 LIB_OBJS += rebase-merge.o
 LIB_OBJS += rebase-todo.o
 LIB_OBJS += reflog-walk.o
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6d42115..d811a44 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -10,11 +10,13 @@
 #include "refs.h"
 #include "rebase-am.h"
 #include "rebase-merge.h"
+#include "rebase-interactive.h"
 
 enum rebase_type {
 	REBASE_TYPE_NONE = 0,
 	REBASE_TYPE_AM,
-	REBASE_TYPE_MERGE
+	REBASE_TYPE_MERGE,
+	REBASE_TYPE_INTERACTIVE
 };
 
 static const char *rebase_dir(enum rebase_type type)
@@ -24,6 +26,8 @@ static const char *rebase_dir(enum rebase_type type)
 		return git_path_rebase_am_dir();
 	case REBASE_TYPE_MERGE:
 		return git_path_rebase_merge_dir();
+	case REBASE_TYPE_INTERACTIVE:
+		return git_path_rebase_interactive_dir();
 	default:
 		die("BUG: invalid rebase_type %d", type);
 	}
@@ -142,6 +146,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	const char *onto_name = NULL;
 	const char *branch_name;
 	int do_merge = 0;
+	int interactive = 0;
 
 	const char * const usage[] = {
 		N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
@@ -153,6 +158,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 			N_("rebase onto given branch instead of upstream")),
 		OPT_BOOL('m', "merge", &do_merge,
 			N_("use merging strategies to rebase")),
+		OPT_BOOL('i', "interactive", &interactive,
+			N_("let the user edit the list of commits to rebase")),
 		OPT_END()
 	};
 
@@ -232,7 +239,13 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	/* Run the appropriate rebase backend */
-	if (do_merge) {
+	if (interactive) {
+		struct rebase_interactive state;
+		rebase_interactive_init(&state, rebase_dir(REBASE_TYPE_INTERACTIVE));
+		rebase_options_swap(&state.opts, &rebase_opts);
+		rebase_interactive_run(&state);
+		rebase_interactive_release(&state);
+	} else if (do_merge) {
 		struct rebase_merge state;
 		rebase_merge_init(&state, rebase_dir(REBASE_TYPE_MERGE));
 		rebase_options_swap(&state.opts, &rebase_opts);
diff --git a/rebase-interactive.c b/rebase-interactive.c
new file mode 100644
index 0000000..342a6fe
--- /dev/null
+++ b/rebase-interactive.c
@@ -0,0 +1,375 @@
+#include "cache.h"
+#include "rebase-interactive.h"
+#include "argv-array.h"
+#include "revision.h"
+#include "dir.h"
+#include "run-command.h"
+
+static int is_empty_commit(struct commit *commit)
+{
+	if (commit->parents)
+		return !oidcmp(&commit->object.oid, &commit->parents->item->object.oid);
+	else
+		return !hashcmp(commit->object.oid.hash, EMPTY_TREE_SHA1_BIN);
+}
+
+GIT_PATH_FUNC(git_path_rebase_interactive_dir, "rebase-merge")
+
+void rebase_interactive_init(struct rebase_interactive *state, const char *dir)
+{
+	rebase_options_init(&state->opts);
+	if (!dir)
+		dir = git_path_rebase_interactive_dir();
+	state->dir = xstrdup(dir);
+
+	state->todo_file = mkpathdup("%s/git-rebase-todo", state->dir);
+	rebase_todo_list_init(&state->todo);
+	state->todo_offset = 0;
+	state->todo_count = 0;
+
+	state->done_file = mkpathdup("%s/done", state->dir);
+	state->done_count = 0;
+
+	state->instruction_format = NULL;
+	git_config_get_value("rebase.instructionFormat", &state->instruction_format);
+}
+
+void rebase_interactive_release(struct rebase_interactive *state)
+{
+	rebase_options_release(&state->opts);
+	free(state->dir);
+
+	free(state->todo_file);
+	rebase_todo_list_clear(&state->todo);
+
+	free(state->done_file);
+}
+
+int rebase_interactive_in_progress(const struct rebase_interactive *state)
+{
+	const char *dir = state ? state->dir : git_path_rebase_interactive_dir();
+	struct stat st;
+
+	if (lstat(dir, &st) || !S_ISDIR(st.st_mode))
+		return 0;
+
+	if (lstat(mkpath("%s/interactive", dir), &st) || !S_ISREG(st.st_mode))
+		return 0;
+
+	return 1;
+}
+
+int rebase_interactive_load(struct rebase_interactive *state)
+{
+	struct rebase_todo_list done;
+
+	/* common rebase options */
+	if (rebase_options_load(&state->opts, state->dir) < 0)
+		return -1;
+
+	/* todo list */
+	rebase_todo_list_clear(&state->todo);
+	if (rebase_todo_list_load(&state->todo, state->todo_file, 0) < 0)
+		return -1;
+	state->todo_offset = 0;
+	state->todo_count = rebase_todo_list_count(&state->todo);
+
+	/* done list */
+	rebase_todo_list_init(&done);
+	if (file_exists(state->done_file) && rebase_todo_list_load(&done, state->done_file, 0) < 0)
+		return -1;
+	state->done_count = rebase_todo_list_count(&done);
+	rebase_todo_list_clear(&done);
+
+	return 0;
+}
+
+static int run_command_without_output(const struct rebase_interactive *state,
+				      struct child_process *cp)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int status;
+
+	cp->stdout_to_stderr = 1;
+	cp->err = -1;
+	if (start_command(cp) < 0)
+		return -1;
+
+	if (strbuf_read(&sb, cp->err, 0) < 0) {
+		strbuf_release(&sb);
+		close(cp->err);
+		finish_command(cp);
+		return -1;
+	}
+
+	close(cp->err);
+	status = finish_command(cp);
+	if (status)
+		fputs(sb.buf, stderr);
+	strbuf_release(&sb);
+	return status;
+}
+
+static int detach_head(const struct rebase_interactive *state, const struct object_id *onto, const char *onto_name)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	const char *reflog_action = getenv("GIT_REFLOG_ACTION");
+
+	if (!reflog_action)
+		reflog_action = "";
+	if (!onto_name)
+		onto_name = oid_to_hex(onto);
+	cp.git_cmd = 1;
+	argv_array_pushf(&cp.env_array, "GIT_REFLOG_ACTION=%s: checkout %s",
+			reflog_action, onto_name);
+	argv_array_push(&cp.args, "checkout");
+	argv_array_push(&cp.args, oid_to_hex(onto));
+
+	if (run_command_without_output(state, &cp))
+		return -1;
+
+	discard_cache();
+	read_cache();
+
+	return 0;
+}
+
+static int gen_todo_list(struct rebase_interactive *state,
+			 const struct object_id *left,
+			 const struct object_id *right)
+{
+	struct rev_info revs;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct pretty_print_context pretty_ctx = {};
+	struct commit *commit;
+	const char *instruction_format;
+
+	init_revisions(&revs, NULL);
+	argv_array_push(&args, "rev-list");
+	argv_array_pushl(&args, "--no-merges", "--cherry-pick", NULL);
+	argv_array_pushl(&args, "--reverse", "--right-only", "--topo-order", NULL);
+	argv_array_pushf(&args, "%s...%s", oid_to_hex(left), oid_to_hex(right));
+	setup_revisions(args.argc, args.argv, &revs, NULL);
+
+	if (prepare_revision_walk(&revs))
+		die("revision walk setup failed");
+
+	pretty_ctx.fmt = CMIT_FMT_USERFORMAT;
+	pretty_ctx.abbrev = revs.abbrev;
+	pretty_ctx.output_encoding = get_commit_output_encoding();
+	pretty_ctx.color = 0;
+	instruction_format = state->instruction_format;
+	if (!instruction_format)
+		instruction_format = "%s";
+
+	while ((commit = get_revision(&revs))) {
+		struct rebase_todo_item *item;
+		struct strbuf sb = STRBUF_INIT;
+
+		item = rebase_todo_list_push_empty(&state->todo);
+		item->action = REBASE_TODO_PICK;
+		oidcpy(&item->oid, &commit->object.oid);
+
+		if (is_empty_commit(commit) && single_parent(commit))
+			item->action = REBASE_TODO_NONE;
+
+		format_commit_message(commit, instruction_format, &sb, &pretty_ctx);
+		strbuf_setlen(&sb, strcspn(sb.buf, "\n"));
+		if (item->action == REBASE_TODO_PICK)
+			item->rest = strbuf_detach(&sb, NULL);
+		else
+			item->rest = xstrfmt("%c pick %s %s", comment_line_char,
+					     oid_to_hex(&item->oid), sb.buf);
+		strbuf_release(&sb);
+	}
+
+	if (!state->todo.nr)
+		rebase_todo_list_push_noop(&state->todo);
+
+	reset_revision_walk();
+	argv_array_clear(&args);
+	return 0;
+}
+
+/**
+ * Mark the current action as done.
+ */
+static void mark_action_done(struct rebase_interactive *state)
+{
+	const struct rebase_todo_item *done_item = &state->todo.items[state->todo_offset++];
+	struct strbuf sb = STRBUF_INIT;
+
+	/* update todo file */
+	rebase_todo_list_save(&state->todo, state->todo_file, state->todo_offset, 0);
+
+	/* update done file */
+	strbuf_add_rebase_todo_item(&sb, done_item, 0);
+	append_file(state->done_file, "%s", sb.buf);
+	strbuf_release(&sb);
+
+	/* update todo and done counts if item is not none */
+	if (done_item->action != REBASE_TODO_NONE) {
+		unsigned int total = state->todo_count + state->done_count;
+
+		state->todo_count--;
+		state->done_count++;
+
+		printf(_("Rebasing (%u/%u)\r"), state->done_count, total);
+	}
+}
+
+/**
+ * Put the last action marked done at the beginning of the todo list again. If
+ * there has not been an action marked done yet, leave the list of items on the
+ * todo list unchanged.
+ */
+static void reschedule_last_action(struct rebase_interactive *state)
+{
+	struct strbuf sb = STRBUF_INIT;
+	const char *last_line;
+
+	if (!state->todo_offset)
+		return; /* no action marked done yet */
+
+	/* update todo file */
+	rebase_todo_list_save(&state->todo, state->todo_file, --state->todo_offset, 0);
+
+	/* remove the last line from the done file */
+	if (strbuf_read_file(&sb, state->done_file, 0) < 0)
+		die_errno(_("failed to read %s"), state->done_file);
+	last_line = sb.buf + sb.len;
+	if (*last_line == '\n')
+		last_line--;
+	last_line = strrchr(last_line, '\n');
+	if (last_line)
+		strbuf_setlen(&sb, last_line - sb.buf);
+	else
+		strbuf_reset(&sb);
+	write_file(state->done_file, "%s", sb.buf);
+	strbuf_release(&sb);
+}
+
+/**
+ * Pick a non-merge commit.
+ */
+static int pick_one_non_merge(struct rebase_interactive *state,
+			      const struct object_id *oid, int no_commit)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	int status;
+
+	cp.git_cmd = 1;
+	if (state->opts.resolvemsg)
+		argv_array_pushf(&cp.env_array, "GIT_CHERRY_PICK_HELP=%s", state->opts.resolvemsg);
+	argv_array_push(&cp.args, "cherry-pick");
+	argv_array_push(&cp.args, "--allow-empty");
+	if (no_commit)
+		argv_array_push(&cp.args, "-n");
+	else
+		argv_array_push(&cp.args, "--ff");
+	argv_array_push(&cp.args, oid_to_hex(oid));
+	status = run_command_without_output(state, &cp);
+
+	/* Reload index as cherry-pick will have modified it */
+	discard_cache();
+	read_cache();
+
+	return status;
+}
+
+/**
+ * Pick a commit.
+ */
+static int pick_one(struct rebase_interactive *state, const struct object_id *oid,
+		    int no_commit)
+{
+	return pick_one_non_merge(state, oid, no_commit);
+}
+
+static void do_pick(struct rebase_interactive *state,
+		    const struct rebase_todo_item *item)
+{
+	int ret;
+	struct object_id head;
+
+	if (get_oid("HEAD", &head))
+		die("invalid head");
+
+	mark_action_done(state);
+	ret = pick_one(state, &item->oid, 0);
+	if (ret != 0 && ret != 1)
+		reschedule_last_action(state);
+	if (ret)
+		die(_("Could not apply %s... %s"), oid_to_hex(&item->oid), item->rest);
+}
+
+static void do_item(struct rebase_interactive *state)
+{
+	const struct rebase_todo_item *item = &state->todo.items[state->todo_offset];
+
+	switch (item->action) {
+	case REBASE_TODO_NONE:
+	case REBASE_TODO_NOOP:
+		mark_action_done(state);
+		break;
+	case REBASE_TODO_PICK:
+		do_pick(state, item);
+		break;
+	default:
+		die("BUG: invalid action %d", item->action);
+	}
+}
+
+static void do_rest(struct rebase_interactive *state)
+{
+	while (state->todo_offset < state->todo.nr)
+		do_item(state);
+	rebase_common_finish(&state->opts, state->dir);
+}
+
+void rebase_interactive_run(struct rebase_interactive *state)
+{
+	if (mkdir(state->dir, 0777) < 0 && errno != EEXIST)
+		die_errno(_("failed to create directory '%s'"), state->dir);
+
+	write_file(mkpath("%s/interactive", state->dir), "%s", "");
+	rebase_options_save(&state->opts, state->dir);
+
+	/* generate initial todo list contents */
+	if (gen_todo_list(state, &state->opts.upstream, &state->opts.orig_head) < 0) {
+		rebase_common_destroy(&state->opts, state->dir);
+		die("could not generate todo list");
+	}
+
+	/* open editor on todo list */
+	rebase_todo_list_save(&state->todo, state->todo_file, 0, 1);
+	if (launch_sequence_editor(state->todo_file, NULL, NULL) < 0) {
+		rebase_common_destroy(&state->opts, state->dir);
+		die("Could not execute editor");
+	}
+
+	/* re-read todo list (which will check the todo list format) */
+	rebase_todo_list_clear(&state->todo);
+	if (rebase_todo_list_load(&state->todo, state->todo_file, 1) < 0)
+		die(_("You can fix this with 'git rebase --edit-todo'"));
+
+	/* count the number of actions in todo list; exit if there are none */
+	state->todo_count = rebase_todo_list_count(&state->todo);
+	if (!state->todo_count) {
+		fprintf_ln(stderr, _("Nothing to do"));
+		rebase_common_destroy(&state->opts, state->dir);
+		exit(2);
+	}
+
+	/* expand todo ids */
+	state->todo_count = rebase_todo_list_count(&state->todo);
+	rebase_todo_list_save(&state->todo, state->todo_file, 0, 0);
+
+	/* checkout onto */
+	if (detach_head(state, &state->opts.onto, state->opts.onto_name) < 0) {
+		rebase_common_destroy(&state->opts, state->dir);
+		die(_("could not detach HEAD"));
+	}
+
+	do_rest(state);
+}
diff --git a/rebase-interactive.h b/rebase-interactive.h
new file mode 100644
index 0000000..bb64203
--- /dev/null
+++ b/rebase-interactive.h
@@ -0,0 +1,33 @@
+#ifndef REBASE_INTERACTIVE_H
+#define REBASE_INTERACTIVE_H
+#include "rebase-common.h"
+#include "rebase-todo.h"
+
+const char *git_path_rebase_interactive_dir(void);
+
+struct rebase_interactive {
+    struct rebase_options opts;
+    char *dir;
+
+    char *todo_file;
+    struct rebase_todo_list todo;
+    unsigned int todo_offset;
+    unsigned int todo_count;
+
+    char *done_file;
+    unsigned int done_count;
+
+    const char *instruction_format;
+};
+
+void rebase_interactive_init(struct rebase_interactive *, const char *);
+
+void rebase_interactive_release(struct rebase_interactive *);
+
+int rebase_interactive_in_progress(const struct rebase_interactive *);
+
+int rebase_interactive_load(struct rebase_interactive *);
+
+void rebase_interactive_run(struct rebase_interactive *);
+
+#endif /* REBASE_INTERACTIVE_H */
-- 
2.7.0

Re: [PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Duy Nguyen]

On Sat, Mar 12, 2016 at 5:46 PM, Paul Tan <pyokagan@gmail.com> wrote:
> So, we have around a 1.4x-1.8x speedup for Linux users, and a 1.7x-13x speedup
> for Windows users. The annoying long delay before the interactive editor is
> launched on Windows is gotten rid of, which I'm very happy about :-)

Nice numbers :-) Sorry I can't look at your patches yet. Just a very
minor comment from diffstat..

>  rebase-am.c                        | 110 +++++++++++
>  rebase-am.h                        |  22 +++
>  rebase-common.c                    | 220 ++++++++++++++++++++++
>  rebase-common.h                    |  48 +++++
>  rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
>  rebase-interactive.h               |  33 ++++
>  rebase-merge.c                     | 256 +++++++++++++++++++++++++
>  rebase-merge.h                     |  28 +++
>  rebase-todo.c                      | 251 +++++++++++++++++++++++++
>  rebase-todo.h                      |  55 ++++++

topdir is already very crowded. Maybe you could move all these files
to "rebase" subdir.
-- 
Duy

Re: [PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

[Christian Couder]

On Sat, Mar 12, 2016 at 11:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> In an interactive rebase, commands are read and executed from a todo
> list (.git/rebase-merge/git-rebase-todo) to perform the rebase.
>
> In the upcoming re-implementation of git-rebase -i in C, it is useful to
> be able to parse each command into a data structure which can then be
> operated on. Implement rebase_todo_item for this.

sequencer.{c,h} already has some code to parse and create todo lists
for cherry-picking or reverting multiple commits, so I am wondering if
it would be possible to share some code?

Thanks!

Re: [PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Stefan Beller]

On Mon, Mar 14, 2016 at 5:15 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Sat, Mar 12, 2016 at 5:46 PM, Paul Tan <pyokagan@gmail.com> wrote:
>> So, we have around a 1.4x-1.8x speedup for Linux users, and a 1.7x-13x speedup
>> for Windows users. The annoying long delay before the interactive editor is
>> launched on Windows is gotten rid of, which I'm very happy about :-)
>
> Nice numbers :-) Sorry I can't look at your patches yet. Just a very
> minor comment from diffstat..
>
>>  rebase-am.c                        | 110 +++++++++++
>>  rebase-am.h                        |  22 +++
>>  rebase-common.c                    | 220 ++++++++++++++++++++++
>>  rebase-common.h                    |  48 +++++
>>  rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
>>  rebase-interactive.h               |  33 ++++
>>  rebase-merge.c                     | 256 +++++++++++++++++++++++++
>>  rebase-merge.h                     |  28 +++
>>  rebase-todo.c                      | 251 +++++++++++++++++++++++++
>>  rebase-todo.h                      |  55 ++++++
>
> topdir is already very crowded. Maybe you could move all these files
> to "rebase" subdir.

or builtin/ (or even builtin/rebase) ?

I thought the toplevel being crowded is by design such that it doesn't
feel like a java project where each file has 5 directories for itself.

I'll look at the series later today.

Stefan

> --
> Duy

Re: [PATCH/RFC/GSoC 03/17] builtin-rebase: implement skeletal builtin rebase

[Stefan Beller]

On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:

   "Unlike when doing git-am, "git am" kept working during the whole series
    and being switched from the shell to the C implementation in the
last commit,
    this time rebase is broken in the series, and built up from here again" ?

> @@ -505,9 +504,6 @@ 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--interactive
> -SCRIPT_LIB += git-rebase--merge

You would also want to retire these files to the contrib directory in
the same patch as you unlink them from the build process?

>         { "read-tree", cmd_read_tree, RUN_SETUP },
> +       { "rebase", cmd_rebase, RUN_SETUP | NEED_WORK_TREE },

#TIL you cannot run rebase in a bare repository(, yet). I would have
assumed you could.

Re: [PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Junio C Hamano]

Duy Nguyen <pclouds@gmail.com> writes:

> On Sat, Mar 12, 2016 at 5:46 PM, Paul Tan <pyokagan@gmail.com> wrote:
>> So, we have around a 1.4x-1.8x speedup for Linux users, and a 1.7x-13x speedup
>> for Windows users. The annoying long delay before the interactive editor is
>> launched on Windows is gotten rid of, which I'm very happy about :-)
>
> Nice numbers :-) Sorry I can't look at your patches yet. Just a very
> minor comment from diffstat..
>
>>  rebase-am.c                        | 110 +++++++++++
>>  rebase-am.h                        |  22 +++
>>  rebase-common.c                    | 220 ++++++++++++++++++++++
>>  rebase-common.h                    |  48 +++++
>>  rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
>>  rebase-interactive.h               |  33 ++++
>>  rebase-merge.c                     | 256 +++++++++++++++++++++++++
>>  rebase-merge.h                     |  28 +++
>>  rebase-todo.c                      | 251 +++++++++++++++++++++++++
>>  rebase-todo.h                      |  55 ++++++
>
> topdir is already very crowded. Maybe you could move all these files
> to "rebase" subdir.

I think that makes sense.  I do not expect people depending on being
able to say "git rebase--am" and have it do something useful, so
they won't belong to builtin/, but rebase/{am,common,...}.[ch] makes
sense.

While this series is still in the rough-outline phase, we can review
the patches without such movement, though ;-)

Re: [PATCH/RFC/GSoC 04/17] builtin-rebase: parse rebase arguments into a common rebase_options struct

[Stefan Beller]

On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> A non-root rebase takes 3 arguments:
>
> * branch_name -- the branch or commit that will be rebased. If it is not
>   specified, the current branch is used.
>
> * upstream -- The upstream commit to compare against. If it is not
>   specified, the configured upstream for the current branch is used.
>
> * onto (or newbase) -- The commit to be used as the starting point to
>   re-apply the commits. If it is not specified, `upstream` is used.
>
> Since these parameters are used by all 3 rebase backends, introduce a
> common rebase_options struct to hold all these options. Teach
> builtin/rebase.c to handle the above arguments and store them in a
> rebase_options struct. In later patches we will pass the rebase_options
> struct to the appropriate backend to perform the rebase.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  Makefile         |   1 +
>  builtin/rebase.c | 184 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  rebase-common.c  |  28 +++++++++
>  rebase-common.h  |  23 +++++++
>  4 files changed, 235 insertions(+), 1 deletion(-)
>  create mode 100644 rebase-common.c
>  create mode 100644 rebase-common.h
>
> diff --git a/Makefile b/Makefile
> index ad98714..b29c672 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -779,6 +779,7 @@ LIB_OBJS += prompt.o
>  LIB_OBJS += quote.o
>  LIB_OBJS += reachable.o
>  LIB_OBJS += read-cache.o
> +LIB_OBJS += rebase-common.o
>  LIB_OBJS += reflog-walk.o
>  LIB_OBJS += refs.o
>  LIB_OBJS += refs/files-backend.o
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 04cc1bd..40176ca 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -4,6 +4,112 @@
>  #include "cache.h"
>  #include "builtin.h"
>  #include "parse-options.h"
> +#include "rebase-common.h"
> +#include "remote.h"
> +#include "branch.h"
> +#include "refs.h"
> +
> +/**
> + * Used by get_curr_branch_upstream_name() as a for_each_remote() callback to
> + * retrieve the name of the remote if the repository only has one remote.
> + */
> +static int get_only_remote(struct remote *remote, void *cb_data)
> +{
> +       const char **remote_name = cb_data;
> +
> +       if (*remote_name)
> +               return -1;
> +
> +       *remote_name = remote->name;
> +       return 0;
> +}
> +
> +const char *get_curr_branch_upstream_name(void)
> +{
> +       const char *upstream_name;
> +       struct branch *curr_branch;
> +
> +       curr_branch = branch_get("HEAD");
> +       if (!curr_branch) {
> +               fprintf_ln(stderr, _("You are not currently on a branch."));
> +               fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +               fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +               fprintf(stderr, "\n");
> +               fprintf_ln(stderr, "    git rebase <branch>");
> +               fprintf(stderr, "\n");
> +               exit(1);
> +       }
> +
> +       upstream_name = branch_get_upstream(curr_branch, NULL);
> +       if (!upstream_name) {
> +               const char *remote_name = NULL;
> +
> +               if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
> +                       remote_name = "<remote>";
> +
> +               fprintf_ln(stderr, _("There is no tracking information for the current branch."));
> +               fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +               fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +               fprintf(stderr, "\n");
> +               fprintf_ln(stderr, "    git rebase <branch>");
> +               fprintf(stderr, "\n");
> +               fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
> +               fprintf(stderr, "\n");
> +               fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:\n"
> +               "\n"
> +               "    git branch --set-upstream-to=%s/<branch> %s\n"),
> +               remote_name, curr_branch->name);
> +               exit(1);
> +       }
> +
> +       return upstream_name;
> +}
> +
> +/**
> + * Given the --onto <name>, return the onto hash
> + */
> +static void get_onto_oid(const char *_onto_name, struct object_id *onto)
> +{
> +       char *onto_name = xstrdup(_onto_name);
> +       struct commit *onto_commit;
> +       char *dotdot;
> +
> +       dotdot = strstr(onto_name, "...");

So either the variable should be "dotdotdot" or "threedots",
or there is a dot too much in the strstr.

"dotdot" was is used in some directory handling code, which
this reminded me of. So I first wondered why we need to take
care of parent directories.

> +       if (dotdot) {
> +               const char *left = onto_name;
> +               const char *right = dotdot + 3;
> +               struct commit *left_commit, *right_commit;
> +               struct commit_list *merge_bases;
> +
> +               *dotdot = 0;
> +               if (!*left)
> +                       left = "HEAD";
> +               if (!*right)
> +                       right = "HEAD";
> +
> +               /* git merge-base --all $left $right */
> +               left_commit = lookup_commit_reference_by_name(left);
> +               right_commit = lookup_commit_reference_by_name(right);
> +               if (!left_commit || !right_commit)
> +                       die(_("%s: there is no merge base"), _onto_name);

_onto_name seems to be different than what was passed in,
because of *dotdot = 0 before, is that intentional?

> +
> +               merge_bases = get_merge_bases(left_commit, right_commit);
> +               if (merge_bases && merge_bases->next)
> +                       die(_("%s: there are more than one merge bases"), _onto_name);
> +               else if (!merge_bases)
> +                       die(_("%s: there is no merge base"), _onto_name);
> +
> +               onto_commit = merge_bases->item;
> +               free_commit_list(merge_bases);
> +       } else {
> +               onto_commit = lookup_commit_reference_by_name(onto_name);
> +               if (!onto_commit)
> +                       die(_("invalid upstream %s"), onto_name);
> +       }
> +
> +       free(onto_name);
> +       oidcpy(onto, &onto_commit->object.oid);
> +}
>
>  static int git_rebase_config(const char *k, const char *v, void *cb)
>  {
> @@ -12,20 +118,96 @@ static int git_rebase_config(const char *k, const char *v, void *cb)
>
>  int cmd_rebase(int argc, const char **argv, const char *prefix)
>  {
> +       struct rebase_options rebase_opts;
> +       const char *onto_name = NULL;
> +       const char *branch_name;
> +
>         const char * const usage[] = {
> -               N_("git rebase [options]"),
> +               N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
>                 NULL
>         };
>         struct option options[] = {
> +               OPT_GROUP(N_("Available options are")),
> +               OPT_STRING(0, "onto", &onto_name, NULL,
> +                       N_("rebase onto given branch instead of upstream")),
>                 OPT_END()
>         };
>
>         git_config(git_rebase_config, NULL);
> +       rebase_options_init(&rebase_opts);
> +       rebase_opts.resolvemsg = _("\nWhen you have resolved this problem, run \"git rebase --continue\".\n"
> +                       "If you prefer to skip this patch, run \"git rebase --skip\" instead.\n"
> +                       "To check out the original branch and stop rebasing, run \"git rebase --abort\".");
>
>         argc = parse_options(argc, argv, prefix, options, usage, 0);
>
>         if (read_cache_preload(NULL) < 0)
>                 die(_("failed to read the index"));
>
> +       /*
> +        * Parse command-line arguments:
> +        *    rebase [<options>] [<upstream_name>] [<branch_name>]
> +        */
> +
> +       /* Parse <upstream_name> into rebase_opts.upstream */
> +       {
> +               const char *upstream_name;
> +               if (argc > 2)
> +                       usage_with_options(usage, options);
> +               if (!argc) {
> +                       upstream_name = get_curr_branch_upstream_name();
> +               } else {
> +                       upstream_name = argv[0];
> +                       argv++, argc--;
> +                       if (!strcmp(upstream_name, "-"))
> +                               upstream_name = "@{-1}";
> +               }

So first we extract the upstream_name then branch_name,
so the parsing is rather

     rebase [<options>] [<upstream_name> [<branch_name>]]

i.e. you cannot give branch name only?

> +               if (get_oid_commit(upstream_name, &rebase_opts.upstream))
> +                       die(_("invalid upstream %s"), upstream_name);
> +               if (!onto_name)
> +                       onto_name = upstream_name;
> +       }
> +
> +       /*
> +        * Parse --onto <onto_name> into rebase_opts.onto and
> +        * rebase_opts.onto_name
> +        */
> +       get_onto_oid(onto_name, &rebase_opts.onto);
> +       rebase_opts.onto_name = xstrdup(onto_name);
> +
> +       /*
> +        * Parse <branch_name> into rebase_opts.orig_head and
> +        * rebase_opts.orig_refname
> +        */
> +       branch_name = argv[0];
> +       if (branch_name) {
> +               /* Is branch_name a branch or commit? */
> +               char *ref_name = xstrfmt("refs/heads/%s", branch_name);
> +               struct object_id orig_head_id;
> +
> +               if (!read_ref(ref_name, orig_head_id.hash)) {
> +                       rebase_opts.orig_refname = ref_name;
> +                       if (get_oid_commit(ref_name, &rebase_opts.orig_head))
> +                               die("get_sha1_commit failed");
> +               } else if (!get_oid_commit(branch_name, &rebase_opts.orig_head)) {
> +                       rebase_opts.orig_refname = NULL;
> +                       free(ref_name);
> +               } else {
> +                       die(_("no such branch: %s"), branch_name);
> +               }
> +       } else {
> +               /* Do not need to switch branches, we are already on it */
> +               struct branch *curr_branch = branch_get("HEAD");
> +
> +               if (curr_branch)
> +                       rebase_opts.orig_refname = xstrdup(curr_branch->refname);
> +               else
> +                       rebase_opts.orig_refname = NULL;
> +
> +               if (get_oid_commit("HEAD", &rebase_opts.orig_head))
> +                       die(_("Failed to resolve '%s' as a valid revision."), "HEAD");
> +       }
> +
> +       rebase_options_release(&rebase_opts);
>         return 0;
>  }
> diff --git a/rebase-common.c b/rebase-common.c
> new file mode 100644
> index 0000000..5a49ac4
> --- /dev/null
> +++ b/rebase-common.c
> @@ -0,0 +1,28 @@
> +#include "cache.h"
> +#include "rebase-common.h"
> +
> +void rebase_options_init(struct rebase_options *opts)
> +{
> +       oidclr(&opts->onto);
> +       opts->onto_name = NULL;
> +
> +       oidclr(&opts->upstream);
> +
> +       oidclr(&opts->orig_head);
> +       opts->orig_refname = NULL;
> +
> +       opts->resolvemsg = NULL;
> +}
> +
> +void rebase_options_release(struct rebase_options *opts)
> +{
> +       free(opts->onto_name);
> +       free(opts->orig_refname);
> +}
> +
> +void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
> +{
> +       struct rebase_options tmp = *dst;
> +       *dst = *src;
> +       *src = tmp;
> +}
> diff --git a/rebase-common.h b/rebase-common.h
> new file mode 100644
> index 0000000..db5146a
> --- /dev/null
> +++ b/rebase-common.h
> @@ -0,0 +1,23 @@
> +#ifndef REBASE_COMMON_H
> +#define REBASE_COMMON_H
> +
> +/* common rebase backend options */
> +struct rebase_options {
> +       struct object_id onto;
> +       char *onto_name;
> +
> +       struct object_id upstream;
> +
> +       struct object_id orig_head;
> +       char *orig_refname;
> +
> +       const char *resolvemsg;
> +};
> +
> +void rebase_options_init(struct rebase_options *);
> +
> +void rebase_options_release(struct rebase_options *);
> +
> +void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
> +
> +#endif /* REBASE_COMMON_H */
> --
> 2.7.0
>

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Stefan Beller]

On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> These functions can be used for loading and saving common rebase options
> into a state directory.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  rebase-common.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  rebase-common.h |  4 ++++
>  2 files changed, 73 insertions(+)
>
> diff --git a/rebase-common.c b/rebase-common.c
> index 5a49ac4..1835f08 100644
> --- a/rebase-common.c
> +++ b/rebase-common.c
> @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
>         *dst = *src;
>         *src = tmp;
>  }
> +
> +static int state_file_exists(const char *dir, const char *file)
> +{
> +       return file_exists(mkpath("%s/%s", dir, file));
> +}

How is this specific to the state file? All it does is create the
leading directory
if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
have the same
result without actually creating the directory if it doesn't exist as
a side effect?

If the dir doesn't exist it can be created in rebase_options_load explicitly?


> +
> +static int read_state_file(struct strbuf *sb, const char *dir, const char *file)
> +{
> +       const char *path = mkpath("%s/%s", dir, file);
> +       strbuf_reset(sb);
> +       if (strbuf_read_file(sb, path, 0) >= 0)
> +               return sb->len;
> +       else
> +               return error(_("could not read '%s'"), path);
> +}
> +
> +int rebase_options_load(struct rebase_options *opts, const char *dir)
> +{
> +       struct strbuf sb = STRBUF_INIT;
> +       const char *filename;
> +
> +       /* opts->orig_refname */
> +       if (read_state_file(&sb, dir, "head-name") < 0)
> +               return -1;
> +       strbuf_trim(&sb);
> +       if (starts_with(sb.buf, "refs/heads/"))
> +               opts->orig_refname = strbuf_detach(&sb, NULL);
> +       else if (!strcmp(sb.buf, "detached HEAD"))
> +               opts->orig_refname = NULL;
> +       else
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "head-name"));
> +
> +       /* opts->onto */
> +       if (read_state_file(&sb, dir, "onto") < 0)
> +               return -1;
> +       strbuf_trim(&sb);
> +       if (get_oid_hex(sb.buf, &opts->onto) < 0)
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "onto"));
> +
> +       /*
> +        * We always write to orig-head, but interactive rebase used to write
> +        * to head. Fall back to reading from head to cover for the case that
> +        * the user upgraded git with an ongoing interactive rebase.
> +        */
> +       filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
> +       if (read_state_file(&sb, dir, filename) < 0)
> +               return -1;

So from here on we always use "orig-head" instead of "head" for
interactive rebase.
Would people ever rely on the (internal) file name and have e.g.
scripts which operate
on the "head" file ?


> +       strbuf_trim(&sb);
> +       if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, filename));
> +
> +       strbuf_release(&sb);
> +       return 0;
> +}
> +
> +static int write_state_text(const char *dir, const char *file, const char *string)
> +{
> +       return write_file(mkpath("%s/%s", dir, file), "%s", string);
> +}

Same comment as on checking the state files existence. I'm not sure if the side
effect of creating the dir is better done explicitly where it is used.
The concat of dir and
file name can still be done in the helper though? (If the helper is
needed at all then)

> +
> +void rebase_options_save(const struct rebase_options *opts, const char *dir)
> +{
> +       const char *head_name = opts->orig_refname;
> +       if (!head_name)
> +               head_name = "detached HEAD";
> +       write_state_text(dir, "head-name", head_name);
> +       write_state_text(dir, "onto", oid_to_hex(&opts->onto));
> +       write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
> +}
> diff --git a/rebase-common.h b/rebase-common.h
> index db5146a..051c056 100644
> --- a/rebase-common.h
> +++ b/rebase-common.h
> @@ -20,4 +20,8 @@ void rebase_options_release(struct rebase_options *);
>
>  void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
>
> +int rebase_options_load(struct rebase_options *, const char *dir);
> +
> +void rebase_options_save(const struct rebase_options *, const char *dir);
> +
>  #endif /* REBASE_COMMON_H */
> --
> 2.7.0
>

Re: [PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

[Johannes Schindelin]

Hi Christian,

On Mon, 14 Mar 2016, Christian Couder wrote:

> On Sat, Mar 12, 2016 at 11:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> > In an interactive rebase, commands are read and executed from a todo
> > list (.git/rebase-merge/git-rebase-todo) to perform the rebase.
> >
> > In the upcoming re-implementation of git-rebase -i in C, it is useful to
> > be able to parse each command into a data structure which can then be
> > operated on. Implement rebase_todo_item for this.
> 
> sequencer.{c,h} already has some code to parse and create todo lists
> for cherry-picking or reverting multiple commits, so I am wondering if
> it would be possible to share some code?

I did exactly that and plan to work full steam on that because I need it
way before GSoC starts:

https://github.com/git/git/compare/master...dscho:interactive-rebase

Note: there are still a couple of things to be done. I will take care of
them.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Johannes Schindelin]

Hi Duy,

On Mon, 14 Mar 2016, Duy Nguyen wrote:

> On Sat, Mar 12, 2016 at 5:46 PM, Paul Tan <pyokagan@gmail.com> wrote:
> 
> >  rebase-am.c                        | 110 +++++++++++
> >  rebase-am.h                        |  22 +++
> >  rebase-common.c                    | 220 ++++++++++++++++++++++
> >  rebase-common.h                    |  48 +++++
> >  rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
> >  rebase-interactive.h               |  33 ++++
> >  rebase-merge.c                     | 256 +++++++++++++++++++++++++
> >  rebase-merge.h                     |  28 +++
> >  rebase-todo.c                      | 251 +++++++++++++++++++++++++
> >  rebase-todo.h                      |  55 ++++++
> 
> topdir is already very crowded. Maybe you could move all these files
> to "rebase" subdir.

Yes, I had mentioned a couple times that my preference would be to
introduce a rebase--helper builtin and move functionality into it one by
one, which is incidentally exactly what I already did in my
'interactive-rebase' branch. It still has a couple of very rough edges,
but I could not work on it today (because v2.7.3 surprised me and shuffled
my tasks around, and then I hunted two bugs the entire day and have to
continue tomorrow).

Paul, may I ask you to concentrate on the parts that are *not* the
interactive rebase?

Ciao,
Johannes

Re: [PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> In the upcoming git-rebase-to-C rewrite, it is a common operation to
> check if the worktree has unstaged changes, so that it can complain that
> the worktree is dirty.
> 
> builtin/pull.c already implements this function. Move it to
> rebase-common.c so that it can be shared between all rebase backends and
> git-pull.

This function is not specific to rebases, even if you only use it for
those purposes for the moment.

In my 'interactive-rebase' branch, I moved it to wt-status (which is a
more logical place, methinks).

Also, you might want to join my discussion with Junio about the sense or
nonsense of keeping the prefix parameter instead of silently removing it
while moving the functions.

Furthermore, it is not really the cache (which I thought we settled on
calling "index" these days) that has unstaged changes, but the working
directory.

For simplicity's sake, I therefore kept the has_unstaged_changes() name
(it is not like there is a lot of confusion *what* can have unstaged
changes).

Ciao,
Johannes

Re: [PATCH/RFC/GSoC 07/17] rebase-common: implement refresh_and_write_cache()

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> In the upcoming git-rebase to C rewrite, it is a common operation to
> refresh the index and write the resulting index.
>
> builtin/am.c already implements refresh_and_write_cache(), which is what
> we want. Move it to rebase-common.c, so that it can be shared with all
> the rebase backends, including git-am.

Your rebase-am might be one of the rebase backends, but git-am is
not, so it is misleading to count it among "all the rebase
backends".

I would think that a better home for refresh_and_write_index() is
right next to write_locked_index(), with #define in cache.h for
refresh_and_write_cache(), just like others.

Re: [PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Also, you might want to join my discussion with Junio about the sense or
> nonsense of keeping the prefix parameter instead of silently removing it
> while moving the functions.

This is a tangent to Paul's topic, but it is an important tangent in
the other thread, in that you didn't mention one thing there that I
needed to make an accurate assessment.  I wasn't aware of your plan
of moving it and use it in a context unrelated to "git pull", hence
keeping the prefix would made perfect sense, as the enhanced error
reporting (i.e. "not only I am saying that you have modified files
and hence you cannot proceed, I can tell you which paths have been
modified") would happen inside the function if done in that context.

If the function will be made a public helper that may be called by
anybody, a possible error reporting mechanism would be to give a
list of modified paths to the caller that asks them, and have the
caller apply its own "prefix" processing to make them relative.  The
public helper function will not even be a position to say "you have
modified files and hence you cannot proceed"--it will not be in a
position to even issue an error message.  The only thing it should
do is to communicate to the caller if there are modified files or
not (and leaving the decision on what to do to the caller--after
all, the caller may want to do something only when there are
modified files, e.g. "add ." may decide not to do anything when
there is no change--so "hence you cannot proceed" is not its
business), and if the caller wants to see them, which paths are
dirty.

Incidentally, that is how wt_shortstatus_status() reports the list
of modified paths, using s->prefix.

Re: [PATCH/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> Signed-off-by: Paul Tan <pyokagan@gmail.com>

This commit message is very short.

> ---
>  cache.h  |  1 +

No need to clutter cache.h with a function that is only to be used by the
sequencer. IOW let's make this static in sequencer.c.

I would also prefer pairing this short function with the change that
actually uses it (in my topic branches, I like to compile with -Werror,
which would result in a failure due to an unused function), in the same
patch.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 17/17] rebase-interactive: introduce interactive backend for builtin rebase

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> Since 1b1dce4 (Teach rebase an interactive mode, 2007-06-25), git-rebase
> supports an interactive mode when passed the -i switch.
> 
> In interactive mode, git-rebase allows users to edit the list of patches
> (using the user's GIT_SEQUENCE_EDITOR), so that the user can reorder,
> edit and delete patches.
> 
> Re-implement a skeletal version of the above feature by introducing a
> rebase-interactive backend for our builtin-rebase. This skeletal
> implementation is only able to pick and re-order commits.
> 
> Signed-off-by: Paul Tan <pyokagan@gmail.com>

It is a pity that both of us worked on overlapping projects in stealth
mode. Inevitably, some of the work is now wasted :-(

Not all is lost, though.

Much of the code can be salvaged, although I really want to reiterate
that an all-or-nothing conversion of the rebase command is not going to
fly.

For several reasons: it would be rather disruptive, huge and hard to
review. It would not let anybody else work on that huge task. And you're
prone to fall behind due to Git's source code being in constant flux
(including the rebase bits).

There is another, really important reason: if you package the conversion
into small, neat bundles, it is much easier to avoid too narrow a focus
that would tuck perfectly useful functions away in a location where it
cannot be reused and where it is likely to be missed by other developers
who need the same, or similar functionality (point in case:
has_uncommitted_changes()). And we know that this happened in the past,
and sometimes resulted in near-duplicated code, hence Karthik's Herculean,
still ongoing work.

Lastly, I need to point out that the conversion of rebase into a builtin
is not the end game, it is the game's opening.

I could imagine that other Git oldtimers are perfectly happy with the
state of the rebase family of commands. I am not. The user interface is
klunky, some parts are designed wrong (--preserve-merges, I am looking at
you!), the *name* is completely unintuitive, for crying out loud! Just to
name a *few* things, there is much more.

I worked around the limitations of the --preserve-merges feature (yeah,
blame me...) by inventing the "Garden Shears" [*1*] that can re-plant an
entire thicket of topic branches on top of a moving upstream branch. It is
similar in spirit to Junio's custom tools that recreate his 'pu' branch
over and over again, but uses the interactive rebase as work horse.

The shears can also be used to fix up commits in the middle of a thicket
of branches, which is why I wrote the shears script in the first place.

The fact that the interactive rebase does not do the job with which pretty
much every project maintainer is faced points out two things: interactive
rebase needs to learn new tricks, and we need plumbing to do interactive
rebase's bidding (so that we/others can build better UIs, hopefully with
much better names, too).

I already know pretty well how I want to implement the shears as a new
mode of the interactive rebase, once I finished teaching the sequencer how
to process rebase -i's edit scripts (which somebody decided to name
"instruction sheets" in the sequencer, to add confusion to poor naming).

All of this let's me think that there is just too much to do for a single
developer, and therefore whatever needs to be done must be done in a way
that allows more than a single person to complete the whole shebang, or
at least their part of it.

So you see, there was a much larger master plan behind my recommendation
to go the rebase--helper route.

As to my current state: Junio put me into quite a fix (without knowing it)
by releasing 2.7.3 just after I took off for an extended offline weekend,
and now I am scrambling because a change in MSYS2's runtime (actually,
probably more like: an update of the GCC that is used to compile the
runtime, that now causes a regression) is keeping me away from my work on
the interactive rebase. Even so, I am pretty far along; There are only
three major things left to do: 1) fix fixups/squashes with fast-forwarding
picks, 2) implement 'reword', 3) display the progress.  And of course 4)
clean up the fallout. ;-)

At this point, I'd rather finish this myself than falling prey to Brooks'
Law.

I also have to admit that I would love to give you a project over the
summer whose logical children are exciting enough to dabble with even
during the winter. And somehow I do not see that excitement in the boring
conversion from shell to C (even if its outcome is well-needed).

Ciao,
Dscho

Footnote *1*:
https://github.com/git-for-windows/build-extra/blob/master/shears.sh

Re: [PATCH/RFC/GSoC 03/17] builtin-rebase: implement skeletal builtin rebase

[Johannes Schindelin]

Hi Stefan,

On Mon, 14 Mar 2016, Stefan Beller wrote:

> #TIL you cannot run rebase in a bare repository(, yet). I would have
> assumed you could.

Every rebase bears the chance of merge conflicts. You need a working
directory to resolve those.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 04/17] builtin-rebase: parse rebase arguments into a common rebase_options struct

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 04cc1bd..40176ca 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -4,6 +4,112 @@
>  #include "cache.h"
>  #include "builtin.h"
>  #include "parse-options.h"
> +#include "rebase-common.h"
> +#include "remote.h"
> +#include "branch.h"
> +#include "refs.h"
> +
> +/**
> + * Used by get_curr_branch_upstream_name() as a for_each_remote() callback to
> + * retrieve the name of the remote if the repository only has one remote.
> + */
> +static int get_only_remote(struct remote *remote, void *cb_data)
> +{
> +	const char **remote_name = cb_data;
> +
> +	if (*remote_name)
> +		return -1;
> +
> +	*remote_name = remote->name;
> +	return 0;
> +}

This function gets only the remote's name, not only the remote. And this
is not really a functionality specific to rebase, is it?

> +const char *get_curr_branch_upstream_name(void)
> +{
> +	const char *upstream_name;
> +	struct branch *curr_branch;
> +
> +	curr_branch = branch_get("HEAD");
> +	if (!curr_branch) {
> +		fprintf_ln(stderr, _("You are not currently on a branch."));
> +		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +		fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, "    git rebase <branch>");
> +		fprintf(stderr, "\n");
> +		exit(1);
> +	}

Urgh. Elswhere we have _("Blabla\nBlublub\n") constructs, which is already
a little bit ugly, but this mix of fprintf_ln() and fprintf() together
with adding a whopping 3 strings (for the price of 1) for the translators
(and missing one...) is too ugly for my taste.

Also, there is a horrible, horrible, horrible exit(1) there. I know, you
put this into builtin/ and so we assume it is okay to just exit() left and
right, but *why*? Is this not a function we might want to reuse elsewhere?
As such, it should live in remote.[ch], take a "hint" parameter in case
there is no current branch (and BTW "HEAD" should not be hard-coded to
begin with, but instead be another parameter), and it should return -1 on
error, not exit.

> +
> +	upstream_name = branch_get_upstream(curr_branch, NULL);
> +	if (!upstream_name) {
> +		const char *remote_name = NULL;
> +
> +		if (for_each_remote(get_only_remote, &remote_name) || !remote_name)
> +			remote_name = "<remote>";
> +
> +		fprintf_ln(stderr, _("There is no tracking information for the current branch."));
> +		fprintf_ln(stderr, _("Please specify which branch you want to rebase against."));
> +		fprintf_ln(stderr, _("See git-rebase(1) for details."));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, "    git rebase <branch>");
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:"));
> +		fprintf(stderr, "\n");
> +		fprintf_ln(stderr, _("If you wish to set tracking information for this branch you can do so with:\n"
> +		"\n"
> +		"    git branch --set-upstream-to=%s/<branch> %s\n"),
> +		remote_name, curr_branch->name);
> +		exit(1);
> +	}

Same here. The rebase-specific part of the hint should be a parameter, the
thing should not die at all, and it really wants to live in remote.[ch].

> +/**
> + * Given the --onto <name>, return the onto hash
> + */
> +static void get_onto_oid(const char *_onto_name, struct object_id *onto)
> +{
> +	char *onto_name = xstrdup(_onto_name);

By convention, variable names starting with an underscore are reserved for
use by the standard library.

> +	struct commit *onto_commit;
> +	char *dotdot;
> +
> +	dotdot = strstr(onto_name, "...");
> +	if (dotdot) {
> +		const char *left = onto_name;
> +		const char *right = dotdot + 3;
> +		struct commit *left_commit, *right_commit;
> +		struct commit_list *merge_bases;
> +
> +		*dotdot = 0;
> +		if (!*left)
> +			left = "HEAD";
> +		if (!*right)
> +			right = "HEAD";
> +
> +		/* git merge-base --all $left $right */
> +		left_commit = lookup_commit_reference_by_name(left);
> +		right_commit = lookup_commit_reference_by_name(right);
> +		if (!left_commit || !right_commit)
> +			die(_("%s: there is no merge base"), _onto_name);
> +
> +		merge_bases = get_merge_bases(left_commit, right_commit);
> +		if (merge_bases && merge_bases->next)
> +			die(_("%s: there are more than one merge bases"), _onto_name);
> +		else if (!merge_bases)
> +			die(_("%s: there is no merge base"), _onto_name);
> +
> +		onto_commit = merge_bases->item;
> +		free_commit_list(merge_bases);
> +	} else {
> +		onto_commit = lookup_commit_reference_by_name(onto_name);
> +		if (!onto_commit)
> +			die(_("invalid upstream %s"), onto_name);
> +	}
> +
> +	free(onto_name);
> +	oidcpy(onto, &onto_commit->object.oid);
> +}

A lot of this looks *awfully* like the parameters we throw at rev-list (or
for that matter, log). Why can't we reuse that machinery?

> @@ -12,20 +118,96 @@ static int git_rebase_config(const char *k, const char *v, void *cb)
>  
>  int cmd_rebase(int argc, const char **argv, const char *prefix)
>  {
> +	struct rebase_options rebase_opts;
> +	const char *onto_name = NULL;
> +	const char *branch_name;
> +
>  	const char * const usage[] = {
> -		N_("git rebase [options]"),
> +		N_("git rebase [options] [--onto <newbase>] [<upstream>] [<branch>]"),
>  		NULL
>  	};
>  	struct option options[] = {
> +		OPT_GROUP(N_("Available options are")),
> +		OPT_STRING(0, "onto", &onto_name, NULL,
> +			N_("rebase onto given branch instead of upstream")),
>  		OPT_END()
>  	};
>  
>  	git_config(git_rebase_config, NULL);
> +	rebase_options_init(&rebase_opts);
> +	rebase_opts.resolvemsg = _("\nWhen you have resolved this problem, run \"git rebase --continue\".\n"
> +			"If you prefer to skip this patch, run \"git rebase --skip\" instead.\n"
> +			"To check out the original branch and stop rebasing, run \"git rebase --abort\".");
>  
>  	argc = parse_options(argc, argv, prefix, options, usage, 0);
>  
>  	if (read_cache_preload(NULL) < 0)
>  		die(_("failed to read the index"));
>  
> +	/*
> +	 * Parse command-line arguments:
> +	 *    rebase [<options>] [<upstream_name>] [<branch_name>]
> +	 */
> +
> +	/* Parse <upstream_name> into rebase_opts.upstream */
> +	{

In Git, unless there are very compelling reasons, we avoid non-conditional
blocks. Probably you did that to have this local declaration:

> +		const char *upstream_name;

But that declaration can easily live in the cmd_rebase() scope,
simplifying the code and being easier on the reader's eyes.

> +	/*
> +	 * Parse --onto <onto_name> into rebase_opts.onto and
> +	 * rebase_opts.onto_name
> +	 */
> +	get_onto_oid(onto_name, &rebase_opts.onto);
> +	rebase_opts.onto_name = xstrdup(onto_name);

My, this onto_name() sure gets strdup()ed a lot... Maybe we can avoid
that?

> +	/*
> +	 * Parse <branch_name> into rebase_opts.orig_head and
> +	 * rebase_opts.orig_refname
> +	 */
> +	branch_name = argv[0];
> +	if (branch_name) {

In Git's source code, we appear to rely on argc instead on argv[argc]
being NULL.

> +		/* Is branch_name a branch or commit? */
> +		char *ref_name = xstrfmt("refs/heads/%s", branch_name);
> +		struct object_id orig_head_id;
> +
> +		if (!read_ref(ref_name, orig_head_id.hash)) {
> +			rebase_opts.orig_refname = ref_name;
> +			if (get_oid_commit(ref_name, &rebase_opts.orig_head))
> +				die("get_sha1_commit failed");
> +		} else if (!get_oid_commit(branch_name, &rebase_opts.orig_head)) {
> +			rebase_opts.orig_refname = NULL;
> +			free(ref_name);
> +		} else {
> +			die(_("no such branch: %s"), branch_name);
> +		}

Here, ref_name does not get free()d. It lives on as
rebase_opts.orig_refname but it gets increasingly fiddly to reason about
the correctness of the code.

A better idea would be to leave the responsibility of keeping track
completely with the caller, i.e. have the fields of the options struct as
const char *. Then you can make the values strbufs as needed and in the
case of a builtin that exits anyway, you do not even need to release in
the end.

> diff --git a/rebase-common.c b/rebase-common.c

As pointed out elsewhere, it is not a good idea to put stuff used by the
rebase into rebase-common.c. Either it is so specific to rebase that it
can go into rebase.c, or it is so not specific to rebase that it can go
into path.c, wt-status.c, diff.c etc

Ciao,
Johannes

Re: [PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Duy Nguyen]

On Tue, Mar 15, 2016 at 3:54 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> In my 'interactive-rebase' branch...

64 commits! Maybe next time we should announce wip branches like this
when we start doing stuff so people don't overlap. Of course these
branches do not have to be perfect (and can be force pushed from time
to time, even).
-- 
Duy

Re: [PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Johannes Schindelin]

Hi Junio,

On Mon, 14 Mar 2016, Junio C Hamano wrote:

> If the [has_uncommitted_changes()] function will be made a public helper
> that may be called by anybody, a possible error reporting mechanism
> would be to give a list of modified paths to the caller that asks them,
> and have the caller apply its own "prefix" processing to make them
> relative.

But the point of the has_uncommitted_changes() is to figure out as fast as
possible whether there are uncommitted changes, not the exact list. In
fact, we want to return with a "yes" as soon as we encounter the first
uncommitted change.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 09/17] rebase-common: implement cache_has_unstaged_changes()

[Johannes Schindelin]

Hi Duy,

On Tue, 15 Mar 2016, Duy Nguyen wrote:

> On Tue, Mar 15, 2016 at 3:54 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > In my 'interactive-rebase' branch...
> 
> 64 commits! Maybe next time we should announce wip branches like this
> when we start doing stuff so people don't overlap. Of course these
> branches do not have to be perfect (and can be force pushed from time
> to time, even).

Much of this is cleanup in the beginning. And I tried to split out a patch
I thought was uncontentious, and promptly ran into a philosophical
discussion I did not seek ;-)

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 17/17] rebase-interactive: introduce interactive backend for builtin rebase

[Paul Tan]

Hi Dscho,

On Tue, Mar 15, 2016 at 3:57 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sat, 12 Mar 2016, Paul Tan wrote:
>
>> Since 1b1dce4 (Teach rebase an interactive mode, 2007-06-25), git-rebase
>> supports an interactive mode when passed the -i switch.
>>
>> In interactive mode, git-rebase allows users to edit the list of patches
>> (using the user's GIT_SEQUENCE_EDITOR), so that the user can reorder,
>> edit and delete patches.
>>
>> Re-implement a skeletal version of the above feature by introducing a
>> rebase-interactive backend for our builtin-rebase. This skeletal
>> implementation is only able to pick and re-order commits.
>>
>> Signed-off-by: Paul Tan <pyokagan@gmail.com>
>
> It is a pity that both of us worked on overlapping projects in stealth
> mode. Inevitably, some of the work is now wasted :-(

No worries, I did this series for my own interest, especially to get a
gauge of the speedup between rebase in shell and C. GSoC applications
have opened and will close in 10 days time, so I wanted to get some
data before the deadline at least :-).

> Not all is lost, though.
>
> Much of the code can be salvaged, although I really want to reiterate
> that an all-or-nothing conversion of the rebase command is not going to
> fly.

Sure. I admit that I concentrated more on how the "final code" would
look like, and not so much how the rewrite would be built upon in
pieces.

> For several reasons: it would be rather disruptive, huge and hard to
> review. It would not let anybody else work on that huge task. And you're
> prone to fall behind due to Git's source code being in constant flux
> (including the rebase bits).
>
> There is another, really important reason: if you package the conversion
> into small, neat bundles, it is much easier to avoid too narrow a focus
> that would tuck perfectly useful functions away in a location where it
> cannot be reused and where it is likely to be missed by other developers
> who need the same, or similar functionality (point in case:
> has_uncommitted_changes()). And we know that this happened in the past,
> and sometimes resulted in near-duplicated code, hence Karthik's Herculean,
> still ongoing work.
>
> Lastly, I need to point out that the conversion of rebase into a builtin
> is not the end game, it is the game's opening.
>
> [...]
>
> So you see, there was a much larger master plan behind my recommendation
> to go the rebase--helper route.

Ah I see, thanks for publishing your branch and sharing your plans.

Originally I was thinking smaller -- rewrite git-rebase first,
following its shell script closely, and then doing the libification
and optimization after that. However, I see now that you have grander
plans than that :-).

>
> As to my current state: Junio put me into quite a fix (without knowing it)
> by releasing 2.7.3 just after I took off for an extended offline weekend,
> and now I am scrambling because a change in MSYS2's runtime (actually,
> probably more like: an update of the GCC that is used to compile the
> runtime, that now causes a regression) is keeping me away from my work on
> the interactive rebase. Even so, I am pretty far along; There are only
> three major things left to do: 1) fix fixups/squashes with fast-forwarding
> picks, 2) implement 'reword', 3) display the progress.  And of course 4)
> clean up the fallout. ;-)
>
> At this point, I'd rather finish this myself than falling prey to Brooks'
> Law.

Okay, I won't touch interactive rebase then.

> I also have to admit that I would love to give you a project over the
> summer whose logical children are exciting enough to dabble with even
> during the winter. And somehow I do not see that excitement in the boring
> conversion from shell to C (even if its outcome is well-needed).

Well, that is subjective ;-).

Even with interactive rebase out-of-bounds, I don't think it's a dead
end though:

1. git-rebase--am.sh, git-rebase--merge.sh and git-rebase.sh can be
rewritten to C, and call git-rebase--interactive.sh externally, like
what Duy demonstrated in his patch series. The timings show that am
and merge rebase still benefit, and that way we will be closer to a
git-rebase in full C.

2. git-commit can be libified, so that we can access its functionality
directly. (sequencer.c runs it once per commit, rebase-interactive
uses it for squashes etc.)

Or would that be stepping on your toes?

> Ciao,
> Dscho
>
> Footnote *1*:
> https://github.com/git-for-windows/build-extra/blob/master/shears.sh

Regards,
Paul

Re: [PATCH/RFC/GSoC 17/17] rebase-interactive: introduce interactive backend for builtin rebase

[Johannes Schindelin]

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

> Even with interactive rebase out-of-bounds,

It is not really "out-of-bounds". It's more like: hold off integrating it
until I'm done with v1 of the rebase--helper that does interactive
rebase's heavy lifting (which should happen pretty soon).

> I don't think it's a dead end though:
> 
> 1. git-rebase--am.sh, git-rebase--merge.sh and git-rebase.sh can be
> rewritten to C, and call git-rebase--interactive.sh externally, like
> what Duy demonstrated in his patch series. The timings show that am
> and merge rebase still benefit, and that way we will be closer to a
> git-rebase in full C.
> 
> 2. git-commit can be libified, so that we can access its functionality
> directly. (sequencer.c runs it once per commit, rebase-interactive
> uses it for squashes etc.)

Both look sound. With 1) I would also suggest a rebase--helper approach,
though. That way, you do not need to break the test suite at all but can
flip the switch at the end of the patch series.

> Or would that be stepping on your toes?

Nope. 2) is related to what I do, but I modified sequencer.c's
run_git_commit() (which uses run_command()). Your plan would make that
even better (although the edit phase is of course not the
performance-critical part that we want to enhance).

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> new file mode 100755
> index 0000000..aaca105
> --- /dev/null
> +++ b/t/perf/p3404-rebase-interactive.sh
> @@ -0,0 +1,26 @@
>
> [...]
>
> +test_perf 'rebase -i --onto master^' '
> +	git checkout perf-topic-branch &&
> +	git reset --hard perf-topic-branch-initial &&
> +	GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
> +'

This measures the performance of checkout && reset && rebase -i. Maybe we
should only test rebase -i?

Also, I would strongly recommend an extra test_commit after reset;
Otherwise you would only test the logic that verifies that it can simply
fast-forward instead of cherry-picking.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Johannes Schindelin]

Hi Paul,

On Mon, 14 Mar 2016, Stefan Beller wrote:

> On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> > These functions can be used for loading and saving common rebase options
> > into a state directory.
> >
> > Signed-off-by: Paul Tan <pyokagan@gmail.com>
> > ---
> >  rebase-common.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  rebase-common.h |  4 ++++
> >  2 files changed, 73 insertions(+)
> >
> > diff --git a/rebase-common.c b/rebase-common.c
> > index 5a49ac4..1835f08 100644
> > --- a/rebase-common.c
> > +++ b/rebase-common.c
> > @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
> >         *dst = *src;
> >         *src = tmp;
> >  }
> > +
> > +static int state_file_exists(const char *dir, const char *file)
> > +{
> > +       return file_exists(mkpath("%s/%s", dir, file));
> > +}
> 
> How is this specific to the state file? All it does is create the
> leading directory
> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
> have the same
> result without actually creating the directory if it doesn't exist as
> a side effect?
> 
> If the dir doesn't exist it can be created in rebase_options_load explicitly?

In addition I want to point out that sequencer's replay_opts seem to be at
least related, but the patch shares none of its code with the sequencer.
Let's avoid that.

In other words, let's try to add as little code as possible when we can
enhance existing code.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Paul Tan]

Hi Dscho,

On Wed, Mar 16, 2016 at 3:58 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Hi Paul,
>
> On Sat, 12 Mar 2016, Paul Tan wrote:
>
>> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
>> new file mode 100755
>> index 0000000..aaca105
>> --- /dev/null
>> +++ b/t/perf/p3404-rebase-interactive.sh
>> @@ -0,0 +1,26 @@
>>
>> [...]
>>
>> +test_perf 'rebase -i --onto master^' '
>> +     git checkout perf-topic-branch &&
>> +     git reset --hard perf-topic-branch-initial &&
>> +     GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
>> +'
>
> This measures the performance of checkout && reset && rebase -i. Maybe we
> should only test rebase -i?

test_perf runs the same script multiple times, so we need to reset
--hard at least to undo the changes of the rebase.

I think we can remove the reset if we use rebase -f and rebase onto
the same base, but -f was not implemented in this patch series.

> Also, I would strongly recommend an extra test_commit after reset;
> Otherwise you would only test the logic that verifies that it can simply
> fast-forward instead of cherry-picking.

Or, we could use the -f flag, I think.

Thanks,
Paul

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Paul Tan]

Hi Stefan,

On Tue, Mar 15, 2016 at 4:30 AM, Stefan Beller <sbeller@google.com> wrote:
> On Sat, Mar 12, 2016 at 2:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> These functions can be used for loading and saving common rebase options
>> into a state directory.
>>
>> Signed-off-by: Paul Tan <pyokagan@gmail.com>
>> ---
>>  rebase-common.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  rebase-common.h |  4 ++++
>>  2 files changed, 73 insertions(+)
>>
>> diff --git a/rebase-common.c b/rebase-common.c
>> index 5a49ac4..1835f08 100644
>> --- a/rebase-common.c
>> +++ b/rebase-common.c
>> @@ -26,3 +26,72 @@ void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src)
>>         *dst = *src;
>>         *src = tmp;
>>  }
>> +
>> +static int state_file_exists(const char *dir, const char *file)
>> +{
>> +       return file_exists(mkpath("%s/%s", dir, file));
>> +}
>
> How is this specific to the state file? All it does is create the
> leading directory
> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
> have the same
> result without actually creating the directory if it doesn't exist as
> a side effect?

I don't quite understand, AFAIK mkpath() does not create any
directories as a side-effect. And yes, I just wanted a short way to
say file_exists(concat(dir, file)) or file_exists(mkpath("%s/%s", dir,
file)) without cluttering up the code.

> If the dir doesn't exist it can be created in rebase_options_load explicitly?

I don't intend to create any directories if they do not exist.

>> +
>> +static int read_state_file(struct strbuf *sb, const char *dir, const char *file)
>> +{
>> +       const char *path = mkpath("%s/%s", dir, file);
>> +       strbuf_reset(sb);
>> +       if (strbuf_read_file(sb, path, 0) >= 0)
>> +               return sb->len;
>> +       else
>> +               return error(_("could not read '%s'"), path);
>> +}
>> +
>> +int rebase_options_load(struct rebase_options *opts, const char *dir)
>> +{
>> +       struct strbuf sb = STRBUF_INIT;
>> +       const char *filename;
>> +
>> +       /* opts->orig_refname */
>> +       if (read_state_file(&sb, dir, "head-name") < 0)
>> +               return -1;
>> +       strbuf_trim(&sb);
>> +       if (starts_with(sb.buf, "refs/heads/"))
>> +               opts->orig_refname = strbuf_detach(&sb, NULL);
>> +       else if (!strcmp(sb.buf, "detached HEAD"))
>> +               opts->orig_refname = NULL;
>> +       else
>> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "head-name"));
>> +
>> +       /* opts->onto */
>> +       if (read_state_file(&sb, dir, "onto") < 0)
>> +               return -1;
>> +       strbuf_trim(&sb);
>> +       if (get_oid_hex(sb.buf, &opts->onto) < 0)
>> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "onto"));
>> +
>> +       /*
>> +        * We always write to orig-head, but interactive rebase used to write
>> +        * to head. Fall back to reading from head to cover for the case that
>> +        * the user upgraded git with an ongoing interactive rebase.
>> +        */
>> +       filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
>> +       if (read_state_file(&sb, dir, filename) < 0)
>> +               return -1;
>
> So from here on we always use "orig-head" instead of "head" for
> interactive rebase.
> Would people ever rely on the (internal) file name and have e.g.
> scripts which operate
> on the "head" file ?

This backwards-compatibility code is just a straight port from the
code in git-rebase.sh.

The usage of orig-head has been around since 2011 with 84df456
(rebase: extract code for writing basic state, 2011-02-06), so I guess
if people had issues with it, it would have been reported.

>
>
>> +       strbuf_trim(&sb);
>> +       if (get_oid_hex(sb.buf, &opts->orig_head) < 0)
>> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, filename));
>> +
>> +       strbuf_release(&sb);
>> +       return 0;
>> +}
>> +
>> +static int write_state_text(const char *dir, const char *file, const char *string)
>> +{
>> +       return write_file(mkpath("%s/%s", dir, file), "%s", string);
>> +}
>
> Same comment as on checking the state files existence. I'm not sure if the side
> effect of creating the dir is better done explicitly where it is used.
> The concat of dir and
> file name can still be done in the helper though? (If the helper is
> needed at all then)

Same as above -- AFAIK I don't think mkpath() creates any directories
as a side-effect.

>
>> +
>> +void rebase_options_save(const struct rebase_options *opts, const char *dir)
>> +{
>> +       const char *head_name = opts->orig_refname;
>> +       if (!head_name)
>> +               head_name = "detached HEAD";
>> +       write_state_text(dir, "head-name", head_name);
>> +       write_state_text(dir, "onto", oid_to_hex(&opts->onto));
>> +       write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
>> +}
>> diff --git a/rebase-common.h b/rebase-common.h
>> index db5146a..051c056 100644
>> --- a/rebase-common.h
>> +++ b/rebase-common.h
>> @@ -20,4 +20,8 @@ void rebase_options_release(struct rebase_options *);
>>
>>  void rebase_options_swap(struct rebase_options *dst, struct rebase_options *src);
>>
>> +int rebase_options_load(struct rebase_options *, const char *dir);
>> +
>> +void rebase_options_save(const struct rebase_options *, const char *dir);
>> +
>>  #endif /* REBASE_COMMON_H */
>> --
>> 2.7.0

Thanks,
Paul

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Paul Tan]

Hi Dscho,

On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> In addition I want to point out that sequencer's replay_opts seem to be at
> least related, but the patch shares none of its code with the sequencer.
> Let's avoid that.
>
> In other words, let's try to add as little code as possible when we can
> enhance existing code.

Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the
sequencer functionality at all, and we don't see git-am for example
needing to be aware of onto, orig-head, head-name etc.

Besides, I don't see why the sequencer needs to be aware of these
rebase-specific options. For simplicity[1], I would think the
sequencer would only need to be aware of what the todo list is, since
that is common to cherry-pick/revert and rebase-i, and all the other
non-sequencer related stuff like checking out the --onto <newbase>,
updating refs can be done at the rebase-interactive.c or
git-rebase--interactive.sh layer.

[1] Of course, it's kind of unfortunate that sequencer.c has to be
aware of whether it is being called as cherry-pick or revert, but I
don't see why implementing interactive rebase functionality needs to
make the same mistake.

Thanks,
Paul

Re: [PATCH/RFC/GSoC 00/17] A barebones git-rebase in C

[Paul Tan]

On Tue, Mar 15, 2016 at 2:43 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Duy Nguyen <pclouds@gmail.com> writes:
>
>> On Sat, Mar 12, 2016 at 5:46 PM, Paul Tan <pyokagan@gmail.com> wrote:
>>> So, we have around a 1.4x-1.8x speedup for Linux users, and a 1.7x-13x speedup
>>> for Windows users. The annoying long delay before the interactive editor is
>>> launched on Windows is gotten rid of, which I'm very happy about :-)
>>
>> Nice numbers :-) Sorry I can't look at your patches yet. Just a very
>> minor comment from diffstat..
>>
>>>  rebase-am.c                        | 110 +++++++++++
>>>  rebase-am.h                        |  22 +++
>>>  rebase-common.c                    | 220 ++++++++++++++++++++++
>>>  rebase-common.h                    |  48 +++++
>>>  rebase-interactive.c               | 375 +++++++++++++++++++++++++++++++++++++
>>>  rebase-interactive.h               |  33 ++++
>>>  rebase-merge.c                     | 256 +++++++++++++++++++++++++
>>>  rebase-merge.h                     |  28 +++
>>>  rebase-todo.c                      | 251 +++++++++++++++++++++++++
>>>  rebase-todo.h                      |  55 ++++++
>>
>> topdir is already very crowded. Maybe you could move all these files
>> to "rebase" subdir.
>
> I think that makes sense.  I do not expect people depending on being
> able to say "git rebase--am" and have it do something useful, so
> they won't belong to builtin/, but rebase/{am,common,...}.[ch] makes
> sense.

Sure, I'll do that.

Regards,
Paul

Re: [PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

[Paul Tan]

Hi Christian,

On Mon, Mar 14, 2016 at 9:43 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Sat, Mar 12, 2016 at 11:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> In an interactive rebase, commands are read and executed from a todo
>> list (.git/rebase-merge/git-rebase-todo) to perform the rebase.
>>
>> In the upcoming re-implementation of git-rebase -i in C, it is useful to
>> be able to parse each command into a data structure which can then be
>> operated on. Implement rebase_todo_item for this.
>
> sequencer.{c,h} already has some code to parse and create todo lists
> for cherry-picking or reverting multiple commits, so I am wondering if
> it would be possible to share some code?

AFAIK, sequencer.c as it is in master parses the todo list
destructively and does not keep the associated action for each commit
and the "rest" string. interactive rebase does keep those, so I needed
a different data structure rather than the one currently being used in
sequencer.c.

As I said in another thread, originally I wanted to keep the scope
simple, and just do the rewrite of rebase from C to shell, and let any
further libifications and optimizations come after, so I didn't want
to touch sequencer for now. However, it turns out that Dscho has grand
plans for the unification of sequencer and interactive rebase, so I'll
go with that :-)

Regards,
Paul

Re: [PATCH/RFC/GSoC 07/17] rebase-common: implement refresh_and_write_cache()

[Paul Tan]

On Tue, Mar 15, 2016 at 5:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> In the upcoming git-rebase to C rewrite, it is a common operation to
>> refresh the index and write the resulting index.
>>
>> builtin/am.c already implements refresh_and_write_cache(), which is what
>> we want. Move it to rebase-common.c, so that it can be shared with all
>> the rebase backends, including git-am.
>
> Your rebase-am might be one of the rebase backends, but git-am is
> not, so it is misleading to count it among "all the rebase
> backends".
>
> I would think that a better home for refresh_and_write_index() is
> right next to write_locked_index(), with #define in cache.h for
> refresh_and_write_cache(), just like others.

Okay, thanks for suggesting a better location.

Regards,
Paul

Re: [PATCH/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()

[Paul Tan]

Hi Dscho,

On Tue, Mar 15, 2016 at 3:00 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Sat, 12 Mar 2016, Paul Tan wrote:
>> ---
>>  cache.h  |  1 +
>
> No need to clutter cache.h with a function that is only to be used by the
> sequencer. IOW let's make this static in sequencer.c.

The function needs to be implemented in editor.c because it would be
better for it to share the same code that launch_editor() uses
(implemented in this patch by splitting the logic into a static
function launch_specific_editor())

We could move the declaration to sequencer.h though.

> I would also prefer pairing this short function with the change that
> actually uses it (in my topic branches, I like to compile with -Werror,
> which would result in a failure due to an unused function), in the same
> patch.

Sure.

Regards,
Paul

Re: [PATCH/RFC/GSoC 06/17] rebase-am: introduce am backend for builtin rebase

[Johannes Schindelin]

Hi Paul,

On Sat, 12 Mar 2016, Paul Tan wrote:

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 40176ca..ec63d3b 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -8,6 +8,22 @@
>  #include "remote.h"
>  #include "branch.h"
>  #include "refs.h"
> +#include "rebase-am.h"
> +
> +enum rebase_type {
> +	REBASE_TYPE_NONE = 0,
> +	REBASE_TYPE_AM
> +};
> +
> +static const char *rebase_dir(enum rebase_type type)
> +{
> +	switch (type) {
> +	case REBASE_TYPE_AM:
> +		return git_path_rebase_am_dir();
> +	default:
> +		die("BUG: invalid rebase_type %d", type);
> +	}
> +}

This is awfully close to what the sequencer sports. So close that I would
call it technical debt.

It would most likely result in easier-to-maintain source code if the
sequencer and the rebase code shared as much as possible (in
sequencer.[ch], for historical reasons).


> diff --git a/rebase-am.c b/rebase-am.c
> new file mode 100644
> index 0000000..53e8798
> --- /dev/null
> +++ b/rebase-am.c
> @@ -0,0 +1,110 @@
> +#include "cache.h"
> +#include "rebase-am.h"
> +#include "run-command.h"
> +
> +GIT_PATH_FUNC(git_path_rebase_am_dir, "rebase-apply");
> +
> +void rebase_am_init(struct rebase_am *state, const char *dir)
> +{
> +	if (!dir)
> +		dir = git_path_rebase_am_dir();
> +	rebase_options_init(&state->opts);
> +	state->dir = xstrdup(dir);
> +}

Does it really make sense to have completely separate structs for the
different rebase types? I think not. It would not hurt IMO to have a
couple of fields that are only used for certain rebase types but not
others. The benefit of being able to reuse, code would far outweigh that
minimal cost.

It all comes back to my favorite paradigm: DRY. Don't Repeat Yourself.

Another important paradigm is: avoid feautures that you do not use. In
this instance, I have to ask why the init function accepts the "dir"
parameter? Do we ever need it? And if yes, would it make more sense to
introduce the parameter with the patch that actually uses it?

> +
> +void rebase_am_release(struct rebase_am *state)
> +{
> +	rebase_options_release(&state->opts);
> +	free(state->dir);

Urgh. The only reason for this free() and the corresponding xstrdup() is
so that the caller *may* release the directory before finishing the rebase
*if* it overrides the directory. That's not very elegant.

Why not simply state (by declaring the field as const char *) that it is
*not* the rebase machinery's duty to take care of the memory management of
this string?

This would simplify the common code flow, especially if it was done to
*all* strings in the state struct.

> +int rebase_am_in_progress(const struct rebase_am *state)
> +{
> +	const char *dir = state ? state->dir : git_path_rebase_am_dir();
> +	struct stat st;
> +
> +	return !lstat(dir, &st) && S_ISDIR(st.st_mode);
> +}

This function is sobbing inconsolably for being stuck into the rebase-am
part of the code, with a name that ensures that it will never grow up and
become more useful. Between its miserable life, it dreams of being named
dir_exists() and living the high life next to its buddy, file_exists().

> +int rebase_am_load(struct rebase_am *state)
> +{
> +	if (rebase_options_load(&state->opts, state->dir) < 0)
> +		return -1;
> +
> +	return 0;
> +}

:-(

This looks like adding code for adding code's sake. Not only does it craft
its own return value instead of reusing rebase_options_load()'s, it is
just wrapping a single, simple statement, therefore its only use is to add
one layer of indirection.

> +static int run_format_patch(const char *patches, const struct object_id *left,
> +		const struct object_id *right)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	int ret;
> +
> +	cp.git_cmd = 1;
> +	cp.out = xopen(patches, O_WRONLY | O_CREAT, 0777);
> +	argv_array_push(&cp.args, "format-patch");
> +	argv_array_push(&cp.args, "-k");
> +	argv_array_push(&cp.args, "--stdout");
> +	argv_array_push(&cp.args, "--full-index");
> +	argv_array_push(&cp.args, "--cherry-pick");
> +	argv_array_push(&cp.args, "--right-only");
> +	argv_array_push(&cp.args, "--src-prefix=a/");
> +	argv_array_push(&cp.args, "--dst-prefix=b/");
> +	argv_array_push(&cp.args, "--no-renames");
> +	argv_array_push(&cp.args, "--no-cover-letter");
> +	argv_array_pushf(&cp.args, "%s...%s", oid_to_hex(left), oid_to_hex(right));
> +
> +	ret = run_command(&cp);
> +	close(cp.out);
> +	return ret;
> +}
> +
> +static int run_am(const struct rebase_am *state, const char *patches)
> +{
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	int ret;
> +
> +	cp.git_cmd = 1;
> +	cp.in = xopen(patches, O_RDONLY);
> +	argv_array_push(&cp.args, "am");
> +	argv_array_push(&cp.args, "--rebasing");
> +	if (state->opts.resolvemsg)
> +		argv_array_pushf(&cp.args, "--resolvemsg=%s", state->opts.resolvemsg);
> +
> +	ret = run_command(&cp);
> +	close(cp.in);
> +	return ret;
> +}

Yeah, these functions really want to use libification for the full, raw
speed improvement that we are going for.

And rather than following the shell script slavishly, we should *really*
consider doing better: the shell script cannot access Git's data
structures directly, therefore it has to use this roundabout way: format
patches as a mailbox only to parse it back into the individual patches
that libgit.a already had available when it formatted the mailbox.

This consideration is pretty important: I do not think that the current
function signatures are correct with that end goal in mind.

> +void rebase_am_run(struct rebase_am *state)
> +{
> +	char *patches;
> +	int ret;
> +
> +	rebase_common_setup(&state->opts, state->dir);
> +
> +	patches = git_pathdup("rebased-patches");
> +	ret = run_format_patch(patches, &state->opts.upstream, &state->opts.orig_head);

Let's wrap the lines at 80 columns/row.

> +	if (ret) {
> +		unlink_or_warn(patches);
> +		fprintf_ln(stderr, _("\ngit encountered an error while preparing the patches to replay\n"
> +			"these revisions:\n"

Also here, the common way to do this is:

		fprintf_ln(stderr, _("\ngit encountered an error while "
				"preparing the patches to replay\n"
			"these revisions:\n"
			[...]

> diff --git a/rebase-common.c b/rebase-common.c
> index 1835f08..8169fb6 100644
> --- a/rebase-common.c
> +++ b/rebase-common.c
> @@ -1,5 +1,8 @@
>  #include "cache.h"
>  #include "rebase-common.h"
> +#include "dir.h"
> +#include "run-command.h"
> +#include "refs.h"
>  
>  void rebase_options_init(struct rebase_options *opts)
>  {
> @@ -95,3 +98,81 @@ void rebase_options_save(const struct rebase_options *opts, const char *dir)
>  	write_state_text(dir, "onto", oid_to_hex(&opts->onto));
>  	write_state_text(dir, "orig-head", oid_to_hex(&opts->orig_head));
>  }
> +
> +static int detach_head(const struct object_id *commit, const char *onto_name)
> +{

Again, this is a very sad function. It would like to work for so many
commands, but it is stuck into the rebase-common.c file where nobody ever
cares about it.

A better place might be branch.[ch], maybe even under a better name
(although this is a matter of contention, as many Git old-timers have an
intuitive understanding of what "detached HEAD" means that is not at all
shared by new Git users).

> +	const char *reflog_action = getenv("GIT_REFLOG_ACTION");
> +	if (!reflog_action || !*reflog_action)
> +		reflog_action = "rebase";
> +	cp.git_cmd = 1;
> +	argv_array_pushf(&cp.env_array, "GIT_REFLOG_ACTION=%s: checkout %s",
> +			reflog_action, onto_name ? onto_name : oid_to_hex(commit));

The REFLOG_ACTION code seems to be a prime candidate for simplification
through a simple, small function owning a static strbuf.

> +void rebase_common_setup(struct rebase_options *opts, const char *dir)
> +{
> +	/* Detach HEAD and reset the tree */
> +	printf_ln(_("First, rewinding head to replay your work on top of it..."));
> +	if (detach_head(&opts->onto, opts->onto_name))
> +		die(_("could not detach HEAD"));
> +	update_ref("rebase", "ORIG_HEAD", opts->orig_head.hash, NULL, 0,
> +			UPDATE_REFS_DIE_ON_ERR);
> +}

If we were to truly reuse the setup between rebase types (and really
preferably extend sequencer's already existing code), we could imitate the
existing "rebase (am)" reflog message.

> +void rebase_common_destroy(struct rebase_options *opts, const char *dir)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	strbuf_addstr(&sb, dir);
> +	remove_dir_recursively(&sb, 0);
> +	strbuf_release(&sb);
> +}

It is *really* fragile to separate the directory from the rebase options.
That makes it *way* too easy to (attempt to) remove the wrong directory
(that might not even exist, but we'll never get notified about that
error).

> +static void move_to_original_branch(struct rebase_options *opts)
> +{

This function does not really *move* to the original branch. Instead, it
*updates* the original branch to the current HEAD and then "un-detaches"
the HEAD.

In addition, if the function states that it wants to work with an original
branch, it should accept the name of the original branch, not some
rebase_options.

> +	struct strbuf sb = STRBUF_INIT;
> +	struct object_id curr_head;
> +
> +	if (!opts->orig_refname || !starts_with(opts->orig_refname, "refs/"))
> +		return;

The caller should take care of this. Alternatively, this function should
return with an error.

> +	if (get_sha1("HEAD", curr_head.hash) < 0)
> +		die("get_sha1() failed");

Nope. No die() in library functions, please.

> +	strbuf_addf(&sb, "rebase finished: %s onto %s", opts->orig_refname, oid_to_hex(&opts->onto));
> +	if (update_ref(sb.buf, opts->orig_refname, curr_head.hash, opts->orig_head.hash, 0, UPDATE_REFS_MSG_ON_ERR))
> +		goto fail;

Overly long lines.

And here you see how beautiful a simple

	static const char *reflog_message(const char *action,
		const char *fmt, ...)
	{
		static strbuf buf = STRBUF_INIT;

		va_list ap;
		va_start(ap, fmt);

		strbuf_reset(&buf);
		strbuf_addf(_("rebase %s"), action);
		strbuf_vaddf(sb, fmt, ap);

		va_end(ap);
		return &buf.buf;
	}

would make this code and pretty much all of the other places where a
reflog message is needed.

FWIW I think this could be of even more general use, outside of rebase.

> +	strbuf_reset(&sb);
> +	strbuf_addf(&sb, "rebase finished: returning to %s", opts->orig_refname);
> +	if (create_symref("HEAD", opts->orig_refname, sb.buf))
> +		goto fail;
> +
> +	strbuf_release(&sb);
> +
> +	return;
> +fail:
> +	die(_("Could not move back to %s"), opts->orig_refname);

Again, no die(), please. Besides, we should tell the user what went wrong:
there are two possibilities here, after all (updating the branch or
updating HEAD).

> +void rebase_common_finish(struct rebase_options *opts, const char *dir)
> +{
> +	const char *argv_gc_auto[] = {"gc", "--auto", NULL};
> +
> +	move_to_original_branch(opts);
> +	close_all_packs();
> +	run_command_v_opt(argv_gc_auto, RUN_GIT_CMD);
> +	rebase_common_destroy(opts, dir);
> +}

So now we have *two* functions to clean up afterwards. Better to have a
single one.

Besides, this would be a perfect opportunity to refactor out gc_auto()
(that closes all packs and then runs the command) and to update all code
locations that can make use of this function, opening the door for a
single point of libification of auto-gc'ing.

> diff --git a/rebase-common.h b/rebase-common.h
> index 051c056..067ad0b 100644
> --- a/rebase-common.h
> +++ b/rebase-common.h
> @@ -24,4 +24,10 @@ int rebase_options_load(struct rebase_options *, const char *dir);
>  
>  void rebase_options_save(const struct rebase_options *, const char *dir);
>  
> +void rebase_common_setup(struct rebase_options *, const char *dir);
> +
> +void rebase_common_destroy(struct rebase_options *, const char *dir);
> +
> +void rebase_common_finish(struct rebase_options *, const char *dir);

Again, this is not very DRY. Does it really have to repeat that it is the
*common* rebase functionality? Why not simply say init_rebase()? And why
pass that dir all the time? This looks really more like copy-edited code
than like a carefully designed API...

For one, the rebase_options should actually be the replay_options (the
entire original idea of the sequencer was to be the plumbing behind the
rebase, after all, not that that idea was implemented very well).

I would also much prefer to extend the sequencer functionality through
callbacks (e.g. for updating, and switching back to, the original branch
at the end, skipping the updating step in case the user wants to abort)
than repeat essentially the same calls in all rebase varieties. Just think
about it: *all* of them have to call rebase_common_setup() in *their own*
setup() functions, same for destroy() and for finish().

If there was a set of callbacks, depending on the replay type, all of this
could be neatly tucked away behind a common interface.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 12/17] rebase-todo: introduce rebase_todo_item

[Johannes Schindelin]

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

> On Mon, Mar 14, 2016 at 9:43 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
> > On Sat, Mar 12, 2016 at 11:46 AM, Paul Tan <pyokagan@gmail.com> wrote:
> >> In an interactive rebase, commands are read and executed from a todo
> >> list (.git/rebase-merge/git-rebase-todo) to perform the rebase.
> >>
> >> In the upcoming re-implementation of git-rebase -i in C, it is useful
> >> to be able to parse each command into a data structure which can then
> >> be operated on. Implement rebase_todo_item for this.
> >
> > sequencer.{c,h} already has some code to parse and create todo lists
> > for cherry-picking or reverting multiple commits, so I am wondering if
> > it would be possible to share some code?
> 
> AFAIK, sequencer.c as it is in master parses the todo list
> destructively and does not keep the associated action for each commit
> and the "rest" string.

This is a *serious* mistake in the implementation of the sequencer, I
agree.

Therefore it is a good idea to fix that mistake instead of leaving it in
place.

And that is exactly what I did:

	https://github.com/dscho/git/commit/b9b5b7351

Please note that the commit is marked as a "TODO" because it has to
reintroduce a stupidly strict behavior (the sequencer expects the commands
in the todo script to agree with the overall action, i.e. if the action is
REPLAY_REVERT, the todo script can only contain "revert" commands, if the
action is REPLAY_PICK, the todo list can only contain "pick" commands). Of
course this restriction is totally arbitrary and even unwanted, so I will
lift it after this commit. Or maybe I'll just lift it before this commit.
Yeah, I'll do that instead.

> As I said in another thread, originally I wanted to keep the scope
> simple, and just do the rewrite of rebase from C to shell, and let any
> further libifications and optimizations come after, so I didn't want
> to touch sequencer for now.

We know, however, how leaving technical debt for later will just make sure
that technical debt accumulates...

And while I have a *tremendous* respect for what Karthik did (and
continues to do, even long after his GSoC project ended!), I do not want
to ask any future GSoC student to clean up the mess that we produce right
now... So let's just not add more technical debt than we absolutely have
to.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Johannes Schindelin]

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

> On Wed, Mar 16, 2016 at 3:58 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 12 Mar 2016, Paul Tan wrote:
> >
> >> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> >> new file mode 100755
> >> index 0000000..aaca105
> >> --- /dev/null
> >> +++ b/t/perf/p3404-rebase-interactive.sh
> >> @@ -0,0 +1,26 @@
> >>
> >> [...]
> >>
> >> +test_perf 'rebase -i --onto master^' '
> >> +     git checkout perf-topic-branch &&
> >> +     git reset --hard perf-topic-branch-initial &&
> >> +     GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
> >> +'
> >
> > This measures the performance of checkout && reset && rebase -i. Maybe we
> > should only test rebase -i?
> 
> test_perf runs the same script multiple times, so we need to reset
> --hard at least to undo the changes of the rebase.
> 
> I think we can remove the reset if we use rebase -f and rebase onto
> the same base, but -f was not implemented in this patch series.

Hrm. rebase -f just makes the reset an implicit part of the rebase, so it
seems we cannot perf *just* the rebase. We are stuck with perf'ing also
the reset. Sad.

> > Also, I would strongly recommend an extra test_commit after reset;
> > Otherwise you would only test the logic that verifies that it can simply
> > fast-forward instead of cherry-picking.
> 
> Or, we could use the -f flag, I think.

Yeah, we can do that, too.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Stefan Beller]

On Wed, Mar 16, 2016 at 5:04 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>
>> How is this specific to the state file? All it does is create the
>> leading directory
>> if it doesn't exist? (So I'd expect file_exists(concat(dir, file)) to
>> have the same
>> result without actually creating the directory if it doesn't exist as
>> a side effect?
>
> I don't quite understand, AFAIK mkpath() does not create any
> directories as a side-effect. And yes, I just wanted a short way to
> say file_exists(concat(dir, file)) or file_exists(mkpath("%s/%s", dir,
> file)) without cluttering up the code.

My bad. I should not assume functions doing stuff as their name might suggest.
(It "makes the path" but only in terms of creating the right string, not on the
file system, where you'd use functions like safe_create_leading_directories.
I thought all that was implied in mkpath).

>
>> If the dir doesn't exist it can be created in rebase_options_load explicitly?
>
> I don't intend to create any directories if they do not exist.
>
>>> +
>>> +static int read_state_file(struct strbuf *sb, const char *dir, const char *file)
>>> +{
>>> +       const char *path = mkpath("%s/%s", dir, file);
>>> +       strbuf_reset(sb);
>>> +       if (strbuf_read_file(sb, path, 0) >= 0)
>>> +               return sb->len;
>>> +       else
>>> +               return error(_("could not read '%s'"), path);
>>> +}
>>> +
>>> +int rebase_options_load(struct rebase_options *opts, const char *dir)
>>> +{
>>> +       struct strbuf sb = STRBUF_INIT;
>>> +       const char *filename;
>>> +
>>> +       /* opts->orig_refname */
>>> +       if (read_state_file(&sb, dir, "head-name") < 0)
>>> +               return -1;
>>> +       strbuf_trim(&sb);
>>> +       if (starts_with(sb.buf, "refs/heads/"))
>>> +               opts->orig_refname = strbuf_detach(&sb, NULL);
>>> +       else if (!strcmp(sb.buf, "detached HEAD"))
>>> +               opts->orig_refname = NULL;
>>> +       else
>>> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "head-name"));
>>> +
>>> +       /* opts->onto */
>>> +       if (read_state_file(&sb, dir, "onto") < 0)
>>> +               return -1;
>>> +       strbuf_trim(&sb);
>>> +       if (get_oid_hex(sb.buf, &opts->onto) < 0)
>>> +               return error(_("could not parse %s"), mkpath("%s/%s", dir, "onto"));
>>> +
>>> +       /*
>>> +        * We always write to orig-head, but interactive rebase used to write
>>> +        * to head. Fall back to reading from head to cover for the case that
>>> +        * the user upgraded git with an ongoing interactive rebase.
>>> +        */
>>> +       filename = state_file_exists(dir, "orig-head") ? "orig-head" : "head";
>>> +       if (read_state_file(&sb, dir, filename) < 0)
>>> +               return -1;
>>
>> So from here on we always use "orig-head" instead of "head" for
>> interactive rebase.
>> Would people ever rely on the (internal) file name and have e.g.
>> scripts which operate
>> on the "head" file ?
>
> This backwards-compatibility code is just a straight port from the
> code in git-rebase.sh.
>
> The usage of orig-head has been around since 2011 with 84df456
> (rebase: extract code for writing basic state, 2011-02-06), so I guess
> if people had issues with it, it would have been reported.

I did not read the rebase shell code, but commented on the C code only.
If this is already in there, let's keep it.
Sorry for the confusion.

Thanks,
Stefan

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Johannes Schindelin]

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

> On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > In addition I want to point out that sequencer's replay_opts seem to be at
> > least related, but the patch shares none of its code with the sequencer.
> > Let's avoid that.
> >
> > In other words, let's try to add as little code as possible when we can
> > enhance existing code.
> 
> Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the
> sequencer functionality at all, and we don't see git-am for example
> needing to be aware of onto, orig-head, head-name etc.

That is arguing that the implementation of --am and --merge is too far
away from the sequencer and therefore should not be made closer.

By that token, has_unstaged_changes() should never be allowed to call
init_revisions(): it *never* looks at any revisions at all!

And the idea of the sequencer is so much more related to --am and --merge
than unstaged changes are to revisions: the entire purpose of the
sequencer (no matter the *current* implementation with all its
limitations) is to apply a bunch of patches, in sequence. That is pretty
much precisely what *all* members of the rebase family are about.

In other words: please do not let current limitations dictate that we
should introduce diverging code for essentially the same workflow.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 16/17] editor: implement git_sequence_editor() and launch_sequence_editor()

[Johannes Schindelin]

Hi Paul,

On Wed, 16 Mar 2016, Paul Tan wrote:

> Hi Dscho,
> 
> On Tue, Mar 15, 2016 at 3:00 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sat, 12 Mar 2016, Paul Tan wrote:
> >> ---
> >>  cache.h  |  1 +
> >
> > No need to clutter cache.h with a function that is only to be used by the
> > sequencer. IOW let's make this static in sequencer.c.
> 
> The function needs to be implemented in editor.c

No, *another* function needs to be implemented in editor.c: one that
accepts the editor itself as parameter. You did that, but then you wrapped
it as git_sequencer_editor() and left the *really* useful function *still*
static to editor.c.

Or maybe the best solution would be to simply extend git_editor() to
accept the editor as an additional, first parameter, falling back to the
current behavior if NULL is passed (and then change all callers to pass
NULL).

I guess my preference would be with the latter, that would make for the
most elegant, minimally invasive and most reusable solution.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Thomas Gummerer]

On 03/16, Johannes Schindelin wrote:
> Hi Paul,
> 
> On Wed, 16 Mar 2016, Paul Tan wrote:
> 
> > On Wed, Mar 16, 2016 at 3:58 PM, Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > On Sat, 12 Mar 2016, Paul Tan wrote:
> > >
> > >> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> > >> new file mode 100755
> > >> index 0000000..aaca105
> > >> --- /dev/null
> > >> +++ b/t/perf/p3404-rebase-interactive.sh
> > >> @@ -0,0 +1,26 @@
> > >>
> > >> [...]
> > >>
> > >> +test_perf 'rebase -i --onto master^' '
> > >> +     git checkout perf-topic-branch &&
> > >> +     git reset --hard perf-topic-branch-initial &&
> > >> +     GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
> > >> +'
> > >
> > > This measures the performance of checkout && reset && rebase -i. Maybe we
> > > should only test rebase -i?
> > 
> > test_perf runs the same script multiple times, so we need to reset
> > --hard at least to undo the changes of the rebase.
> > 
> > I think we can remove the reset if we use rebase -f and rebase onto
> > the same base, but -f was not implemented in this patch series.
> 
> Hrm. rebase -f just makes the reset an implicit part of the rebase, so it
> seems we cannot perf *just* the rebase. We are stuck with perf'ing also
> the reset. Sad.

I had the same problem back when I was working on index-v5 and posted
a patch series.  The discussion about it is at [1].  Maybe it could be
worth resurrecting?

[1] http://thread.gmane.org/gmane.comp.version-control.git/1379419842-32627-1-git-send-email-t.gummerer@gmail.com

-- 
Thomas

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Johannes Schindelin]

Hi Thomas,

On Fri, 18 Mar 2016, Thomas Gummerer wrote:

> On 03/16, Johannes Schindelin wrote:
> > 
> > On Wed, 16 Mar 2016, Paul Tan wrote:
> > 
> > > On Wed, Mar 16, 2016 at 3:58 PM, Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Sat, 12 Mar 2016, Paul Tan wrote:
> > > >
> > > >> diff --git a/t/perf/p3404-rebase-interactive.sh b/t/perf/p3404-rebase-interactive.sh
> > > >> new file mode 100755
> > > >> index 0000000..aaca105
> > > >> --- /dev/null
> > > >> +++ b/t/perf/p3404-rebase-interactive.sh
> > > >> @@ -0,0 +1,26 @@
> > > >>
> > > >> [...]
> > > >>
> > > >> +test_perf 'rebase -i --onto master^' '
> > > >> +     git checkout perf-topic-branch &&
> > > >> +     git reset --hard perf-topic-branch-initial &&
> > > >> +     GIT_SEQUENCE_EDITOR=: git rebase -i --onto master^ master
> > > >> +'
> > > >
> > > > This measures the performance of checkout && reset && rebase -i. Maybe we
> > > > should only test rebase -i?
> > > 
> > > test_perf runs the same script multiple times, so we need to reset
> > > --hard at least to undo the changes of the rebase.
> > > 
> > > I think we can remove the reset if we use rebase -f and rebase onto
> > > the same base, but -f was not implemented in this patch series.
> > 
> > Hrm. rebase -f just makes the reset an implicit part of the rebase, so it
> > seems we cannot perf *just* the rebase. We are stuck with perf'ing also
> > the reset. Sad.
> 
> I had the same problem back when I was working on index-v5 and posted
> a patch series.  The discussion about it is at [1].  Maybe it could be
> worth resurrecting?
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/1379419842-32627-1-git-send-email-t.gummerer@gmail.com

Yes, I agree that something like that is needed. The proposed commit
message suggests that things get simpler, though, while I would contend
that timings get more accurate.

And I think you could simply move the test_start command, but that's just
from a *very* cursory reading of the patch.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Thomas Gummerer]

On 03/18, Johannes Schindelin wrote:
> Hi Thomas,
> 
> On Fri, 18 Mar 2016, Thomas Gummerer wrote:
> 
> > On 03/16, Johannes Schindelin wrote:
> > > Hrm. rebase -f just makes the reset an implicit part of the rebase, so it
> > > seems we cannot perf *just* the rebase. We are stuck with perf'ing also
> > > the reset. Sad.
> > 
> > I had the same problem back when I was working on index-v5 and posted
> > a patch series.  The discussion about it is at [1].  Maybe it could be
> > worth resurrecting?
> > 
> > [1] http://thread.gmane.org/gmane.comp.version-control.git/1379419842-32627-1-git-send-email-t.gummerer@gmail.com
> 
> Yes, I agree that something like that is needed. The proposed commit
> message suggests that things get simpler, though, while I would contend
> that timings get more accurate.
> 
> And I think you could simply move the test_start command, but that's just
> from a *very* cursory reading of the patch.

Is it possible that you might have missed patch 2/2 [1]?  The first
patch there is only the preparation for making the timings more
acurate when some cleanup is involved.  Just moving the test_start
command wouldn't do anything for the timings to get more acurate, as
the timings are measured in the test_run_perf_ function (it's outside
of the diff in the patch series).

I'm also not sure whether the test_perf_cleanup or if the other series
in the thread that came out of a suggestion by Junio makes more sense
[2]. (That is minus patch 3/3 in that series, which was added so we
could have a user of the new series, but if it's going to be used here
that's unnecessary).

[1] http://thread.gmane.org/gmane.comp.version-control.git/234874/focus=234875
[2] http://thread.gmane.org/gmane.comp.version-control.git/234874/focus=235241


> Ciao,
> Dscho

-- 
Thomas

Re: [PATCH/RFC/GSoC 01/17] perf: introduce performance tests for git-rebase

[Johannes Schindelin]

Hi Thomas,

On Sun, 20 Mar 2016, Thomas Gummerer wrote:

> On 03/18, Johannes Schindelin wrote:
> > 
> > On Fri, 18 Mar 2016, Thomas Gummerer wrote:
> > 
> > > [1] http://thread.gmane.org/gmane.comp.version-control.git/1379419842-32627-1-git-send-email-t.gummerer@gmail.com
> > 
> > Yes, I agree that something like that is needed. The proposed commit
> > message suggests that things get simpler, though, while I would
> > contend that timings get more accurate.
> > 
> > And I think you could simply move the test_start command, but that's
> > just from a *very* cursory reading of the patch.
> 
> Is it possible that you might have missed patch 2/2 [1]?

Yes, I did not read that patch. Sorry for the noise.

Ciao,
Dscho

Re: [PATCH/RFC/GSoC 05/17] rebase-options: implement rebase_options_load() and rebase_options_save()

[Paul Tan]

Hi Dscho,

(Sorry for the very late reply, I got caught up with some unexpected
work and am still clearing my inbox ><)

On Thu, Mar 17, 2016 at 1:11 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> On Wed, 16 Mar 2016, Paul Tan wrote:
>> On Wed, Mar 16, 2016 at 4:04 PM, Johannes Schindelin
>> <Johannes.Schindelin@gmx.de> wrote:
>> > In addition I want to point out that sequencer's replay_opts seem to be at
>> > least related, but the patch shares none of its code with the sequencer.
>> > Let's avoid that.
>> >
>> > In other words, let's try to add as little code as possible when we can
>> > enhance existing code.
>>
>> Well, both git-rebase--am.sh and git-rebase--merge.sh do not use the
>> sequencer functionality at all, and we don't see git-am for example
>> needing to be aware of onto, orig-head, head-name etc.
>
> That is arguing that the implementation of --am and --merge is too far
> away from the sequencer and therefore should not be made closer.
>
> By that token, has_unstaged_changes() should never be allowed to call
> init_revisions(): it *never* looks at any revisions at all!
>
> And the idea of the sequencer is so much more related to --am and --merge
> than unstaged changes are to revisions: the entire purpose of the
> sequencer (no matter the *current* implementation with all its
> limitations) is to apply a bunch of patches, in sequence. That is pretty
> much precisely what *all* members of the rebase family are about.
>
> In other words: please do not let current limitations dictate that we
> should introduce diverging code for essentially the same workflow.

Ah, so you are thinking of replacing the --am and --merge scripts with
sequencer? That sounds great :-)

I'll wait for your sequencer patch series then.

Thanks!
Paul