Re: [PATCH 11/14] pull: teach git pull about --rebase

Hi Paul,

I would like to add more suggestions to Stefan's comments, please find them below.

On 2015-05-18 17:06, Paul Tan wrote:

A better name might be `get_upstream_branch(const char *repo)`, in line with the function in `sha1_name.c` that implements the `@{upstream}` functionality.

+{
+	struct remote *rm;
+	struct branch *curr_branch;
+
+	if (repo && !(rm = remote_get(repo)))
+		return NULL;
+	if (!(curr_branch = branch_get("HEAD")))
+		return NULL;
+	if (repo && curr_branch->remote != rm)
+		return NULL;
+	if (!curr_branch->merge_nr)
+		return NULL;
+	return xstrdup(curr_branch->merge[0]->dst);
+}
+
+/**
+ * Given a refspec, returns the merge branch. Returns NULL if the refspec src
+ * does not refer to a branch.
+ *
+ * FIXME: It should return the tracking branch. Currently only works with the
+ * default mapping.
+ */
+static char *get_merge_branch_2(const char *repo, const char *refspec)
+{
+	struct refspec *spec;
+	const char *remote;
+	char *merge_branch;
+
+	spec = parse_fetch_refspec(1, &refspec);
+	remote = spec->src;
+	if (!*remote || !strcmp(remote, "HEAD"))
+		remote = "HEAD";
+	else if (skip_prefix(remote, "heads/", &remote))
+		;
+	else if (skip_prefix(remote, "refs/heads/", &remote))
+		;
+	else if (starts_with(remote, "refs/") ||
+		starts_with(remote, "tags/") ||
+		starts_with(remote, "remotes/"))
+		remote = "";
+
+	if (*remote) {
+		if (!strcmp(repo, "."))
+			merge_branch = mkpathdup("refs/heads/%s", remote);
+		else
+			merge_branch = mkpathdup("refs/remotes/%s/%s", repo, remote);
+	} else
+		merge_branch = NULL;
+
+	free_refspec(1, spec);
+	return merge_branch;
+}

I have to admit that it took me a substantial amount of time to deduce from the code what `get_merge_branch_2()` really does (judging from the description, I thought that it would be essentially the same as `get_merge_branch_1()` without the hard-coded "HEAD"). If the comment above the function would have said something like:

/**
 * Given a refspec, returns the name of the local tracking ref.
 */

I would have had an easier time. Also, I wonder if something like this would do the job:

	spec = parse_fetch_refspec(1, &refspec);
	if (spec->dst)
		return spec->dst;
	if (!(remote = get_remote(remote_name)))
		return NULL;
	if (remote_find_tracking(remote, spec))
		return spec->dst;
	return NULL;

(I guess we'd have to `xstrdup()` the return values because the return value of `get_merge_branch_1()` needs to be `free()`d, but maybe we can avoid so much `malloc()/free()`ing.) Again, the function name should probably be changed to something clearer, maybe to `get_tracking_branch()`.

One thing that is not clear at all to me is whether

	git pull --rebase origin master next

would error out as expected, or simply rebase to `origin/master`.

+/**
+ * Sets fork_point to the point at which the current branch forked from its
+ * remote merge branch. Returns 0 on success, -1 on failure.
+ */
+static int get_rebase_fork_point(unsigned char fork_point[GIT_SHA1_RAWSZ],
+		const char *repo, const char *refspec)
+{
+	int ret;
+	struct branch *curr_branch;
+	char *remote_merge_branch;
+	struct argv_array args = ARGV_ARRAY_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (!(curr_branch = branch_get("HEAD")))
+		return -1;
+
+	if (refspec)
+		remote_merge_branch = get_merge_branch_2(repo, refspec);
+	else
+		remote_merge_branch = get_merge_branch_1(repo);
+
+	if (!remote_merge_branch)
+		return -1;

We should probably `return error(_"No tracking branch found for %s@, refspec ? refspec : "HEAD");` so that the user has a chance to understand that there has been a problem and how to solve it.

+	argv_array_pushl(&args, "merge-base", "--fork-point",
+			remote_merge_branch, curr_branch->name, NULL);
+	cp.argv = args.argv;

Let's just use `cp.args` directly...

+	cp.no_stdin = 1;
+	cp.no_stderr = 1;
+	cp.git_cmd = 1;
+
+	if ((ret = capture_command(&cp, &sb, GIT_SHA1_HEXSZ)))
+		goto cleanup;
+
+	if ((ret = get_sha1_hex(sb.buf, fork_point)))
+		goto cleanup;
+
+cleanup:
+	free(remote_merge_branch);
+	strbuf_release(&sb);
+	return ret ? -1 : 0;
+}
+
+[...]
+/**
+ * Given the current HEAD SHA1, the merge head returned from git-fetch and the
+ * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
+ * appropriate arguments and returns its exit status.
+ */
+static int run_rebase(unsigned char curr_head[GIT_SHA1_RAWSZ],
+		unsigned char merge_head[GIT_SHA1_RAWSZ],
+		unsigned char fork_point[GIT_SHA1_RAWSZ])
+{
+	int ret;
+	unsigned char oct_merge_base[GIT_SHA1_RAWSZ];
+	struct argv_array args = ARGV_ARRAY_INIT;
+
+	if (!get_octopus_merge_base(oct_merge_base, curr_head, merge_head,
fork_point))

It might be my mail program only that mangled the diff here. But it could also be that this line is a little long (by my count, it is 81 columns wide).

Otherwise, this looks really good to me.

Ciao,
Dscho