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:
> @@ -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.
> +{
> + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html