Re: [PATCH v2 15/19] pull: teach git pull about --rebase

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2015, 01:56 UTC
Message-ID: <xmqqk2vcuy10.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1433314143-4478-16-git-send-email-pyokagan@gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 20 quoted lines

> +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?

Show 28 quoted lines

> +/**
> + * 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?

Show 8 quoted lines

> +/**
> + * 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.

Show 10 quoted lines

> +/**
> + * 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.

Show 11 quoted lines

> +/**
> + * 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).