Re: [PATCH v2 2/6] push: move code to setup_push_simple()

[Junio C Hamano <gitster@pobox.com>]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> In order to avoid doing unnecessary things and simplify it in further
> patches.
>
> The code is copied exactly as-is; no functional changes.

The title says "move" and I expected to see corresponding deletions,
but it seems that this (possibly temporarily) duplicates what these
two functions do, without removing them, which (possibly
temporarily) risks the two to drift apart.  So, the resulting code
from this step will do nothing wrong (it's just two function's
bodies inlined in duplicated form), but depending on how the code
evolves, it might turn out to be a good change or a bad change---we
cannot judge by this step alone.

From a quick scanning of the remainder of the series, it seems that
3/6 and 4/6 improve the copied code without changing the behaviour,
and at the end these two functions remain, so we have duplicated
logic for these two functions and improvements only live in one of
the copies (namely, in the setup_push_simple())?  Would that be a
problem, or it is too much work to do better, I wonder?

Let's keep reading.

> Reviewed-by: Elijah Newren <newren@gmail.com>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/push.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index 7045e4ef0c..d173c39283 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -225,10 +225,38 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
>  
>  static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
>  {
> -	if (triangular)
> -		setup_push_current(remote, branch);
> -	else
> -		setup_push_upstream(remote, branch, triangular, 1);
> +	if (triangular) {
> +		if (!branch)
> +			die(_(message_detached_head_die), remote->name);
> +		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
> +	} else {
> +		if (!branch)
> +			die(_(message_detached_head_die), remote->name);
> +		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
> +			die(_("The current branch %s has no upstream branch.\n"
> +			    "To push the current branch and set the remote as upstream, use\n"
> +			    "\n"
> +			    "    git push --set-upstream %s %s\n"),
> +			    branch->name,
> +			    remote->name,
> +			    branch->name);
> +		if (branch->merge_nr != 1)
> +			die(_("The current branch %s has multiple upstream branches, "
> +			    "refusing to push."), branch->name);
> +		if (triangular)
> +			die(_("You are pushing to remote '%s', which is not the upstream of\n"
> +			      "your current branch '%s', without telling me what to push\n"
> +			      "to update which remote branch."),
> +			    remote->name, branch->name);
> +
> +		if (1) {
> +			/* Additional safety */
> +			if (strcmp(branch->refname, branch->merge[0]->src))
> +				die_push_simple(branch, remote);
> +		}
> +
> +		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
> +	}
>  }
>  
>  static int is_workflow_triangular(struct remote *remote)