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:
quoted hunk ↗ jump to hunk
@@ -506,11 +547,185 @@ static int run_merge(void)
return ret;
}
+/**
+ * Returns the merge branch for the current branch. Returns NULL if repo is not
+ * a valid remote, HEAD does not point to a branch, repo is not the branch's
+ * configured remote or the branch does not have any configured merge branch.
+ */
+static char *get_merge_branch_1(const char *repo)
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.
quoted hunk ↗ jump to hunk
+{
+ 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`.
quoted hunk ↗ jump to hunk
+/**
+ * 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.
quoted hunk ↗ jump to hunk
+ 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