Re: [PATCH v2 15/19] pull: teach git pull about --rebase
flat view
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:05:14
Paul Tan [off-list ref] writes:
quoted hunk
+enum rebase_type { + REBASE_INVALID = -1, + REBASE_FALSE = 0, + REBASE_TRUE, + REBASE_PRESERVE +}; + +/** + * Parses the value of --rebase, branch.*.rebase or pull.rebase. If value is a + * false value, returns REBASE_FALSE. If value is a true value, returns + * REBASE_TRUE. If value is "preserve", returns REBASE_PRESERVE. Otherwise, + * returns -1 to signify an invalid value. + */ +static enum rebase_type parse_config_rebase(const char *value) +{ + int v = git_config_maybe_bool("pull.rebase", value); + if (!v) + return REBASE_FALSE; + else if (v >= 0) + return REBASE_TRUE;
It is somewhat misleading to say "v >= 0" when you already use !v to signal something else. Perhaps "else if (v > 0)" is better?
quoted hunk
+/** + * Returns remote's upstream branch for the current branch. If remote is NULL, + * the current branch's configured default remote is used. Returns NULL if + * `remote` does not name a valid remote, HEAD does not point to a branch, + * remote is not the branch's configured remote or the branch does not have any + * configured upstream branch. + */ +static char *get_upstream_branch(const char *remote) +{ + struct remote *rm; + struct branch *curr_branch; + + rm = remote_get(remote); + if (!rm) + return NULL; + + curr_branch = branch_get("HEAD"); + if (!curr_branch) + return NULL; + + if (curr_branch->remote != rm) + return NULL; + + if (!curr_branch->merge_nr) + return NULL; + + return xstrdup(curr_branch->merge[0]->dst); +}
Hmph, it is somewhat surprising that we do not have such a helper
already. Wouldn't we need this logic to implement $branch@{upstream}
syntax?
quoted hunk
+/** + * Derives the remote tracking branch from the remote and refspec. + * + * FIXME: The current implementation assumes the default mapping of + * refs/heads/<branch_name> to refs/remotes/<remote_name>/<branch_name>. + */ +static char *get_tracking_branch(const char *remote, const char *refspec) +{
This does smell like an incomplete reimplementation of what get_fetch_map() knows how to do.
quoted hunk
+/** + * Given the repo and refspecs, sets fork_point to the point at which the + * current branch forked from its remote tracking 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) +{ +... +}
This function looks OK (the two get_*_branch() helpers it uses I am not sure about though). Same comment on "fork_point[]" parameter's type applies here, though. While I do not mind if you used "struct object_id" to represent these object names, if you are sticking to the traditional "unsigned char [20]", then these should be "unsigned char *" to be consistent with others.
quoted hunk
+/** + * Sets merge_base to the octopus merge base of curr_head, merge_head and + * fork_point. Returns 0 if a merge base is found, 1 otherwise. + */ +static int get_octopus_merge_base(unsigned char merge_base[GIT_SHA1_HEXSZ], + unsigned char curr_head[GIT_SHA1_RAWSZ], + unsigned char merge_head[GIT_SHA1_RAWSZ], + unsigned char fork_point[GIT_SHA1_RAWSZ]) +{ +... +}
OK (and everything after this point looks good).