Re: [PATCH v2 2/6] push: move code to setup_push_simple()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- May 31, 2021, 05:04 UTC
- Message-ID
- <xmqqczt7xzm4.fsf@gitster.g>
- In-Reply-To
- <20210529071115.1908310-3-felipe.contreras@gmail.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.
Show 53 quoted lines
> 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)