From 533e0325abcf51d6b3ff689dfacf8fa95e3b6da1 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:12 -0500 Subject: [PATCH 01/13] push: create new get_upstream_ref() helper This code is duplicated among multiple functions. No functional changes. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 29fea70ff1..e3e792c69c 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -185,29 +185,37 @@ static const char message_detached_head_die[] = "\n" " git push %s HEAD:\n"); -static void setup_push_upstream(struct remote *remote, struct branch *branch, - int same_remote) +static const char *get_upstream_ref(struct branch *branch, const char *remote_name) { - 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, + remote_name, branch->name); if (branch->merge_nr != 1) die(_("The current branch %s has multiple upstream branches, " "refusing to push."), branch->name); + + return branch->merge[0]->src; +} + +static void setup_push_upstream(struct remote *remote, struct branch *branch, + int same_remote) +{ + const char *upstream_ref; + if (!branch) + die(_(message_detached_head_die), remote->name); + upstream_ref = get_upstream_ref(branch, remote->name); if (!same_remote) 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); - refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src); + refspec_appendf(&rs, "%s:%s", branch->refname, upstream_ref); } static void setup_push_current(struct remote *remote, struct branch *branch) @@ -223,20 +231,12 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int die(_(message_detached_head_die), remote->name); if (same_remote) { - 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); + const char *upstream_ref; + + upstream_ref = get_upstream_ref(branch, remote->name); /* Additional safety */ - if (strcmp(branch->refname, branch->merge[0]->src)) + if (strcmp(branch->refname, upstream_ref)) die_push_simple(branch, remote); } refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); From 72739680fc912c6e8dedaf06af4969f8e52ffb4d Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:13 -0500 Subject: [PATCH 02/13] push: return immediately in trivial switch case There's no need to break when nothing else will be executed. Will help next patches. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index e3e792c69c..0aa1d0f07d 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -257,25 +257,25 @@ static void setup_default_push_refspecs(struct remote *remote) default: case PUSH_DEFAULT_MATCHING: refspec_append(&rs, ":"); - break; + return; case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: setup_push_simple(remote, branch, same_remote); - break; + return; case PUSH_DEFAULT_UPSTREAM: setup_push_upstream(remote, branch, same_remote); - break; + return; case PUSH_DEFAULT_CURRENT: setup_push_current(remote, branch); - break; + return; case PUSH_DEFAULT_NOTHING: die(_("You didn't specify any refspecs to push, and " "push.default is \"nothing\".")); - break; + return; } } From 04159fba42b61ffa954dfb1fa13df1862c210ad8 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:14 -0500 Subject: [PATCH 03/13] push: split switch cases We want all the cases that don't do anything with a branch first, and then the rest. That way we will be able to get the branch and die if there's a problem in the parent function, instead of inside the function of each mode. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 0aa1d0f07d..f64b7100f0 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -254,11 +254,20 @@ static void setup_default_push_refspecs(struct remote *remote) int same_remote = is_same_remote(remote); switch (push_default) { - default: case PUSH_DEFAULT_MATCHING: refspec_append(&rs, ":"); return; + case PUSH_DEFAULT_NOTHING: + die(_("You didn't specify any refspecs to push, and " + "push.default is \"nothing\".")); + return; + default: + break; + } + + switch (push_default) { + default: case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: setup_push_simple(remote, branch, same_remote); @@ -271,11 +280,6 @@ static void setup_default_push_refspecs(struct remote *remote) case PUSH_DEFAULT_CURRENT: setup_push_current(remote, branch); return; - - case PUSH_DEFAULT_NOTHING: - die(_("You didn't specify any refspecs to push, and " - "push.default is \"nothing\".")); - return; } } From cc16f95d212d8e0bba07ddfb82aaf416dc4df1c2 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:15 -0500 Subject: [PATCH 04/13] push: factor out null branch check No need to do it in every single function. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index f64b7100f0..8fcbd2878d 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -206,8 +206,6 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, int same_remote) { const char *upstream_ref; - if (!branch) - die(_(message_detached_head_die), remote->name); upstream_ref = get_upstream_ref(branch, remote->name); if (!same_remote) die(_("You are pushing to remote '%s', which is not the upstream of\n" @@ -220,16 +218,11 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, static void setup_push_current(struct remote *remote, struct branch *branch) { - if (!branch) - die(_(message_detached_head_die), remote->name); refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); } static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote) { - if (!branch) - die(_(message_detached_head_die), remote->name); - if (same_remote) { const char *upstream_ref; @@ -266,6 +259,9 @@ static void setup_default_push_refspecs(struct remote *remote) break; } + if (!branch) + die(_(message_detached_head_die), remote->name); + switch (push_default) { default: case PUSH_DEFAULT_UNSPECIFIED: From 65c63a005434306e7c064eb463ff1f61de56d38e Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:16 -0500 Subject: [PATCH 05/13] push: only get the branch when needed Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index 8fcbd2878d..d9f9d20f39 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -243,7 +243,7 @@ static int is_same_remote(struct remote *remote) static void setup_default_push_refspecs(struct remote *remote) { - struct branch *branch = branch_get(NULL); + struct branch *branch; int same_remote = is_same_remote(remote); switch (push_default) { @@ -259,6 +259,7 @@ static void setup_default_push_refspecs(struct remote *remote) break; } + branch = branch_get(NULL); if (!branch) die(_(message_detached_head_die), remote->name); From 00458dc5f191f52ef965ca7185b739e8c800b7cb Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:17 -0500 Subject: [PATCH 06/13] push: make setup_push_* return the dst All of the setup_push_* functions are appending a refspec. Do this only once on the parent function. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index d9f9d20f39..933b1cc6c0 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -202,8 +202,8 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na return branch->merge[0]->src; } -static void setup_push_upstream(struct remote *remote, struct branch *branch, - int same_remote) +static const char *setup_push_upstream(struct remote *remote, struct branch *branch, + int same_remote) { const char *upstream_ref; upstream_ref = get_upstream_ref(branch, remote->name); @@ -212,16 +212,15 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch, "your current branch '%s', without telling me what to push\n" "to update which remote branch."), remote->name, branch->name); - - refspec_appendf(&rs, "%s:%s", branch->refname, upstream_ref); + return upstream_ref; } -static void setup_push_current(struct remote *remote, struct branch *branch) +static const char *setup_push_current(struct remote *remote, struct branch *branch) { - refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); + return branch->refname; } -static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote) +static const char *setup_push_simple(struct remote *remote, struct branch *branch, int same_remote) { if (same_remote) { const char *upstream_ref; @@ -232,7 +231,7 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int if (strcmp(branch->refname, upstream_ref)) die_push_simple(branch, remote); } - refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname); + return branch->refname; } static int is_same_remote(struct remote *remote) @@ -245,6 +244,7 @@ static void setup_default_push_refspecs(struct remote *remote) { struct branch *branch; int same_remote = is_same_remote(remote); + const char *dst; switch (push_default) { case PUSH_DEFAULT_MATCHING: @@ -267,17 +267,19 @@ static void setup_default_push_refspecs(struct remote *remote) default: case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: - setup_push_simple(remote, branch, same_remote); - return; + dst = setup_push_simple(remote, branch, same_remote); + break; case PUSH_DEFAULT_UPSTREAM: - setup_push_upstream(remote, branch, same_remote); - return; + dst = setup_push_upstream(remote, branch, same_remote); + break; case PUSH_DEFAULT_CURRENT: - setup_push_current(remote, branch); - return; + dst = setup_push_current(remote, branch); + break; } + + refspec_appendf(&rs, "%s:%s", branch->refname, dst); } static const char message_advice_pull_before_push[] = From d371a9ef4cfc4610989b0a3d71c79cbe19e0073a Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:18 -0500 Subject: [PATCH 07/13] push: trivial simplifications Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 933b1cc6c0..43c039a2e3 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -205,14 +205,12 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na static const char *setup_push_upstream(struct remote *remote, struct branch *branch, int same_remote) { - const char *upstream_ref; - upstream_ref = get_upstream_ref(branch, remote->name); if (!same_remote) 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); - return upstream_ref; + return get_upstream_ref(branch, remote->name); } static const char *setup_push_current(struct remote *remote, struct branch *branch) @@ -222,15 +220,9 @@ static const char *setup_push_current(struct remote *remote, struct branch *bran static const char *setup_push_simple(struct remote *remote, struct branch *branch, int same_remote) { - if (same_remote) { - const char *upstream_ref; - - upstream_ref = get_upstream_ref(branch, remote->name); - - /* Additional safety */ - if (strcmp(branch->refname, upstream_ref)) + if (same_remote) + if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) die_push_simple(branch, remote); - } return branch->refname; } From 0add899baf5a9ee478659821327db719ce74454a Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:19 -0500 Subject: [PATCH 08/13] push: get rid of all the setup_push_* functions Their code is much simpler now and can move into the parent function. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 38 +++++++++++--------------------------- 1 file changed, 11 insertions(+), 27 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index 43c039a2e3..da406fc890 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -202,30 +202,6 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na return branch->merge[0]->src; } -static const char *setup_push_upstream(struct remote *remote, struct branch *branch, - int same_remote) -{ - if (!same_remote) - 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); - return get_upstream_ref(branch, remote->name); -} - -static const char *setup_push_current(struct remote *remote, struct branch *branch) -{ - return branch->refname; -} - -static const char *setup_push_simple(struct remote *remote, struct branch *branch, int same_remote) -{ - if (same_remote) - if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) - die_push_simple(branch, remote); - return branch->refname; -} - static int is_same_remote(struct remote *remote) { struct remote *fetch_remote = remote_get(NULL); @@ -259,15 +235,23 @@ static void setup_default_push_refspecs(struct remote *remote) default: case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: - dst = setup_push_simple(remote, branch, same_remote); + if (same_remote) + if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) + die_push_simple(branch, remote); + dst = branch->refname; break; case PUSH_DEFAULT_UPSTREAM: - dst = setup_push_upstream(remote, branch, same_remote); + if (!same_remote) + 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); + dst = get_upstream_ref(branch, remote->name); break; case PUSH_DEFAULT_CURRENT: - dst = setup_push_current(remote, branch); + dst = branch->refname; break; } From 1f934725f7597366c981b72be6597124e2c21a77 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:20 -0500 Subject: [PATCH 09/13] push: factor out the typical case Only override dst on the odd case. This allows a preemptive break on the `simple` case. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index da406fc890..b5e951bf59 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -231,14 +231,16 @@ static void setup_default_push_refspecs(struct remote *remote) if (!branch) die(_(message_detached_head_die), remote->name); + dst = branch->refname; + switch (push_default) { default: case PUSH_DEFAULT_UNSPECIFIED: case PUSH_DEFAULT_SIMPLE: - if (same_remote) - if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) - die_push_simple(branch, remote); - dst = branch->refname; + if (!same_remote) + break; + if (strcmp(branch->refname, get_upstream_ref(branch, remote->name))) + die_push_simple(branch, remote); break; case PUSH_DEFAULT_UPSTREAM: @@ -251,7 +253,6 @@ static void setup_default_push_refspecs(struct remote *remote) break; case PUSH_DEFAULT_CURRENT: - dst = branch->refname; break; } From 1afd78fb5c29b8ad95de072bb8a0edacc69db61a Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:21 -0500 Subject: [PATCH 10/13] push: remove redundant check If fetch_remote is NULL (i.e. the branch remote is invalid), then it can't possibly be same as remote, which can't be NULL. The check is redundant, and so is the extra variable. Also, fix the Yoda condition: we want to check if remote is the same as the branch remote, not the other way around. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index b5e951bf59..aa22d6a8e5 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -204,8 +204,7 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na static int is_same_remote(struct remote *remote) { - struct remote *fetch_remote = remote_get(NULL); - return (!fetch_remote || fetch_remote == remote); + return remote == remote_get(NULL); } static void setup_default_push_refspecs(struct remote *remote) From c5b09cf771ea3c340335aeff501ebec342f78aa4 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:22 -0500 Subject: [PATCH 11/13] push: remove trivial function It's a single line that is used in a single place, and the variable has the same name as the function. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/builtin/push.c b/builtin/push.c index aa22d6a8e5..a873f8da92 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -202,15 +202,10 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na return branch->merge[0]->src; } -static int is_same_remote(struct remote *remote) -{ - return remote == remote_get(NULL); -} - static void setup_default_push_refspecs(struct remote *remote) { struct branch *branch; - int same_remote = is_same_remote(remote); + int same_remote = remote == remote_get(NULL); const char *dst; switch (push_default) { From e0c91cffde8477ababb5163180f08e29da54db3e Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:23 -0500 Subject: [PATCH 12/13] push: only check same_remote when needed Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index a873f8da92..f3916c66d1 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -205,8 +205,8 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na static void setup_default_push_refspecs(struct remote *remote) { struct branch *branch; - int same_remote = remote == remote_get(NULL); const char *dst; + int same_remote; switch (push_default) { case PUSH_DEFAULT_MATCHING: @@ -226,6 +226,7 @@ static void setup_default_push_refspecs(struct remote *remote) die(_(message_detached_head_die), remote->name); dst = branch->refname; + same_remote = remote == remote_get(NULL); switch (push_default) { default: From 7088ce71918a0f9fde2ceb421d4b332888a202c7 Mon Sep 17 00:00:00 2001 From: Felipe Contreras Date: Mon, 31 May 2021 14:51:24 -0500 Subject: [PATCH 13/13] push: don't get a full remote object All we need to know is that their names are the same. Additionally this might be easier to parse for some since remote_for_branch is more descriptive than remote_get(NULL). Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- builtin/push.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/push.c b/builtin/push.c index f3916c66d1..e8b10a9b7e 100644 --- a/builtin/push.c +++ b/builtin/push.c @@ -226,7 +226,7 @@ static void setup_default_push_refspecs(struct remote *remote) die(_(message_detached_head_die), remote->name); dst = branch->refname; - same_remote = remote == remote_get(NULL); + same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL)); switch (push_default) { default: