git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/7] Unconvolutize push.default=simple
@ 2021-05-31 19:32 Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 1/7] push: rename !triangular to same_remote Felipe Contreras
                   ` (7 more replies)
  0 siblings, 8 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

Tired of jumping through hoops trying to understand what the "simple"
mode does, I decided to reorganize it up for good so it's crystal
clear.

There are no functional changes.

Basically the simple mode pushes the current branch with the same name
on the remote.

Except... when there's no upstream branch configured with the same name.

Now the code and the documentation are clear.

The difference from v2 is that now triangular is renamed to same_remote in the very first patch.
That way the rest of both series are easier to digest.

Unfortunately the first patch is a little noisy, and the same with the rangediff. That might be a
small price to pay to make the rest easier.

The result of this series is in short this function:

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) {
		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);

		/* Additional safety */
		if (strcmp(branch->refname, branch->merge[0]->src))
			die_push_simple(branch, remote);
	}
	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
}

Felipe Contreras (7):
  push: rename !triangular to same_remote
  push: hedge code of default=simple
  push: copy code to setup_push_simple()
  push: reorganize setup_push_simple()
  push: simplify setup_push_simple()
  push: remove unused code in setup_push_upstream()
  doc: push: explain default=simple correctly

 Documentation/config/push.txt | 13 +++++-----
 builtin/push.c                | 48 +++++++++++++++++++++++------------
 2 files changed, 38 insertions(+), 23 deletions(-)

Range-diff against v2:
1:  f1f42bda32 < -:  ---------- push: hedge code of default=simple
-:  ---------- > 1:  e8efe0d844 push: rename !triangular to same_remote
-:  ---------- > 2:  f84aa09a93 push: hedge code of default=simple
2:  bb6d810011 ! 3:  7d04af5b2c push: move code to setup_push_simple()
    @@ Metadata
     Author: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## Commit message ##
    -    push: move code to setup_push_simple()
    +    push: copy code to setup_push_simple()
     
         In order to avoid doing unnecessary things and simplify it in further
    -    patches.
    +    patches. In particular moving the additional name safety out of
    +    setup_push_upstream() and into setup_push_simple() and thus making both
    +    more straightforward.
     
         The code is copied exactly as-is; no functional changes.
     
    @@ Commit message
      ## builtin/push.c ##
     @@ builtin/push.c: static void setup_push_current(struct remote *remote, struct branch *branch)
      
    - static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    + static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
      {
    --	if (triangular)
    +-	if (!same_remote)
     -		setup_push_current(remote, branch);
     -	else
    --		setup_push_upstream(remote, branch, triangular, 1);
    -+	if (triangular) {
    +-		setup_push_upstream(remote, branch, same_remote, 1);
    ++	if (!same_remote) {
     +		if (!branch)
     +			die(_(message_detached_head_die), remote->name);
     +		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
    @@ builtin/push.c: static void setup_push_current(struct remote *remote, struct bra
     +		if (branch->merge_nr != 1)
     +			die(_("The current branch %s has multiple upstream branches, "
     +			    "refusing to push."), branch->name);
    -+		if (triangular)
    ++		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."),
    @@ builtin/push.c: static void setup_push_current(struct remote *remote, struct bra
     +	}
      }
      
    - static int is_workflow_triangular(struct remote *remote)
    + static int is_same_remote(struct remote *remote)
3:  d66a442fba ! 4:  5e0e09bc7a push: reorganize setup_push_simple()
    @@ Commit message
         push: reorganize setup_push_simple()
     
         Simply move the code around and remove dead code. In particular the
    -    'trivial' conditional is a no-op since that part of the code is the
    -    !trivial leg of the conditional beforehand.
    +    '!same_remote' conditional is a no-op since that part of the code is the
    +    same_remote leg of the conditional beforehand.
     
         No functional changes.
     
    @@ Commit message
      ## builtin/push.c ##
     @@ builtin/push.c: static void setup_push_current(struct remote *remote, struct branch *branch)
      
    - static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    + static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
      {
     +	const char *dst;
     +
     +	if (!branch)
     +		die(_(message_detached_head_die), remote->name);
     +
    - 	if (triangular) {
    + 	if (!same_remote) {
     -		if (!branch)
     -			die(_(message_detached_head_die), remote->name);
     -		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
    @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct bran
      		if (branch->merge_nr != 1)
      			die(_("The current branch %s has multiple upstream branches, "
      			    "refusing to push."), branch->name);
    --		if (triangular)
    +-		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."),
    @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct bran
     +	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
      }
      
    - static int is_workflow_triangular(struct remote *remote)
    + static int is_same_remote(struct remote *remote)
4:  eaae6a826a ! 5:  723d95b572 push: simplify setup_push_simple()
    @@ Metadata
      ## Commit message ##
         push: simplify setup_push_simple()
     
    -    There's a safety check to make sure branch->refname is not different
    +    There's a safety check to make sure branch->refname isn't different
         from branch->merge[0]->src, otherwise we die().
     
         Therefore we always push to branch->refname.
    @@ Commit message
      ## builtin/push.c ##
     @@ builtin/push.c: static void setup_push_current(struct remote *remote, struct branch *branch)
      
    - static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    + static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
      {
     -	const char *dst;
     -
      	if (!branch)
      		die(_(message_detached_head_die), remote->name);
      
    --	if (triangular) {
    +-	if (!same_remote) {
     -		dst = branch->refname;
     -	} else {
    -+	if (!triangular) {
    ++	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"
    @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct bran
     +	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
      }
      
    - static int is_workflow_triangular(struct remote *remote)
    + static int is_same_remote(struct remote *remote)
5:  8d9ae5e552 ! 6:  8ffaacd0db push: remove unused code in setup_push_upstream()
    @@ builtin/push.c: static const char message_detached_head_die[] =
      	   "    git push %s HEAD:<name-of-remote-branch>\n");
      
      static void setup_push_upstream(struct remote *remote, struct branch *branch,
    --				int triangular, int simple)
    -+				int triangular)
    +-				int same_remote, int simple)
    ++				int same_remote)
      {
      	if (!branch)
      		die(_(message_detached_head_die), remote->name);
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      		break;
      
      	case PUSH_DEFAULT_UPSTREAM:
    --		setup_push_upstream(remote, branch, triangular, 0);
    -+		setup_push_upstream(remote, branch, triangular);
    +-		setup_push_upstream(remote, branch, same_remote, 0);
    ++		setup_push_upstream(remote, branch, same_remote);
      		break;
      
      	case PUSH_DEFAULT_CURRENT:
6:  b35bdf14dc = 7:  3cc20c876f doc: push: explain default=simple correctly
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 1/7] push: rename !triangular to same_remote
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 2/7] push: hedge code of default=simple Felipe Contreras
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

The typical case is what git was designed for: distributed remotes.

It's only the atypical case--fetching and pushing to the same
remote--that we need to keep an eye on.

No functional changes.

Liked-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 194967ed79..06406353ce 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -186,7 +186,7 @@ static const char message_detached_head_die[] =
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
 static void setup_push_upstream(struct remote *remote, struct branch *branch,
-				int triangular, int simple)
+				int same_remote, int simple)
 {
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
@@ -201,7 +201,7 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 	if (branch->merge_nr != 1)
 		die(_("The current branch %s has multiple upstream branches, "
 		    "refusing to push."), branch->name);
-	if (triangular)
+	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."),
@@ -223,16 +223,16 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
-static int is_workflow_triangular(struct remote *remote)
+static int is_same_remote(struct remote *remote)
 {
 	struct remote *fetch_remote = remote_get(NULL);
-	return (fetch_remote && fetch_remote != remote);
+	return (!fetch_remote || fetch_remote == remote);
 }
 
 static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch = branch_get(NULL);
-	int triangular = is_workflow_triangular(remote);
+	int same_remote = is_same_remote(remote);
 
 	switch (push_default) {
 	default:
@@ -242,14 +242,14 @@ static void setup_default_push_refspecs(struct remote *remote)
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		if (triangular)
+		if (!same_remote)
 			setup_push_current(remote, branch);
 		else
-			setup_push_upstream(remote, branch, triangular, 1);
+			setup_push_upstream(remote, branch, same_remote, 1);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, branch, triangular, 0);
+		setup_push_upstream(remote, branch, same_remote, 0);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 2/7] push: hedge code of default=simple
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 1/7] push: rename !triangular to same_remote Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 3/7] push: copy code to setup_push_simple() Felipe Contreras
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

`simple` is the most important mode so move the relevant code to its own
function to make it easier to see what it's doing.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 06406353ce..48c38fe25a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -223,6 +223,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
+static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
+{
+	if (!same_remote)
+		setup_push_current(remote, branch);
+	else
+		setup_push_upstream(remote, branch, same_remote, 1);
+}
+
 static int is_same_remote(struct remote *remote)
 {
 	struct remote *fetch_remote = remote_get(NULL);
@@ -242,10 +250,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		if (!same_remote)
-			setup_push_current(remote, branch);
-		else
-			setup_push_upstream(remote, branch, same_remote, 1);
+		setup_push_simple(remote, branch, same_remote);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 3/7] push: copy code to setup_push_simple()
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 1/7] push: rename !triangular to same_remote Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 2/7] push: hedge code of default=simple Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 4/7] push: reorganize setup_push_simple() Felipe Contreras
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

In order to avoid doing unnecessary things and simplify it in further
patches. In particular moving the additional name safety out of
setup_push_upstream() and into setup_push_simple() and thus making both
more straightforward.

The code is copied exactly as-is; no functional changes.

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 48c38fe25a..6a620a90e3 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 same_remote)
 {
-	if (!same_remote)
-		setup_push_current(remote, branch);
-	else
-		setup_push_upstream(remote, branch, same_remote, 1);
+	if (!same_remote) {
+		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 (!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);
+
+		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_same_remote(struct remote *remote)
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 4/7] push: reorganize setup_push_simple()
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
                   ` (2 preceding siblings ...)
  2021-05-31 19:32 ` [PATCH v3 3/7] push: copy code to setup_push_simple() Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 5/7] push: simplify setup_push_simple() Felipe Contreras
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

Simply move the code around and remove dead code. In particular the
'!same_remote' conditional is a no-op since that part of the code is the
same_remote leg of the conditional beforehand.

No functional changes.

Suggestions-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 29 ++++++++++++-----------------
 1 file changed, 12 insertions(+), 17 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 6a620a90e3..972d8e1cfd 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,13 +225,14 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
 {
+	const char *dst;
+
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+
 	if (!same_remote) {
-		if (!branch)
-			die(_(message_detached_head_die), remote->name);
-		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
+		dst = 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"
@@ -243,20 +244,14 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 		if (branch->merge_nr != 1)
 			die(_("The current branch %s has multiple upstream branches, "
 			    "refusing to push."), branch->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);
-
-		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);
+		/* Additional safety */
+		if (strcmp(branch->refname, branch->merge[0]->src))
+			die_push_simple(branch, remote);
+
+		dst = branch->merge[0]->src;
 	}
+	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
 }
 
 static int is_same_remote(struct remote *remote)
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 5/7] push: simplify setup_push_simple()
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
                   ` (3 preceding siblings ...)
  2021-05-31 19:32 ` [PATCH v3 4/7] push: reorganize setup_push_simple() Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 6/7] push: remove unused code in setup_push_upstream() Felipe Contreras
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

There's a safety check to make sure branch->refname isn't different
from branch->merge[0]->src, otherwise we die().

Therefore we always push to branch->refname.

Suggestions-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 972d8e1cfd..e37c751268 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,14 +225,10 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
 {
-	const char *dst;
-
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 
-	if (!same_remote) {
-		dst = branch->refname;
-	} else {
+	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"
@@ -248,10 +244,8 @@ static void setup_push_simple(struct remote *remote, struct branch *branch, int
 		/* Additional safety */
 		if (strcmp(branch->refname, branch->merge[0]->src))
 			die_push_simple(branch, remote);
-
-		dst = branch->merge[0]->src;
 	}
-	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
+	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
 }
 
 static int is_same_remote(struct remote *remote)
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 6/7] push: remove unused code in setup_push_upstream()
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
                   ` (4 preceding siblings ...)
  2021-05-31 19:32 ` [PATCH v3 5/7] push: simplify setup_push_simple() Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:32 ` [PATCH v3 7/7] doc: push: explain default=simple correctly Felipe Contreras
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

Now it's not used for the simple mode.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/push.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index e37c751268..29fea70ff1 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -186,7 +186,7 @@ static const char message_detached_head_die[] =
 	   "    git push %s HEAD:<name-of-remote-branch>\n");
 
 static void setup_push_upstream(struct remote *remote, struct branch *branch,
-				int same_remote, int simple)
+				int same_remote)
 {
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
@@ -207,12 +207,6 @@ static void setup_push_upstream(struct remote *remote, struct branch *branch,
 		      "to update which remote branch."),
 		    remote->name, branch->name);
 
-	if (simple) {
-		/* 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);
 }
 
@@ -271,7 +265,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, branch, same_remote, 0);
+		setup_push_upstream(remote, branch, same_remote);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v3 7/7] doc: push: explain default=simple correctly
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
                   ` (5 preceding siblings ...)
  2021-05-31 19:32 ` [PATCH v3 6/7] push: remove unused code in setup_push_upstream() Felipe Contreras
@ 2021-05-31 19:32 ` Felipe Contreras
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
  7 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:32 UTC (permalink / raw)
  To: git
  Cc: Elijah Newren, Ævar Arnfjörð Bjarmason, Jeff King,
	Junio C Hamano, Felipe Contreras

Now that the code has been simplified and it's clear what it's
actually doing, update the documentation to reflect that.

Namely; the simple mode only barfs when working on a centralized
workflow, and there's no configured upstream branch with the same name.

Cc: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/config/push.txt | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/push.txt b/Documentation/config/push.txt
index f2667b2689..632033638c 100644
--- a/Documentation/config/push.txt
+++ b/Documentation/config/push.txt
@@ -24,15 +24,14 @@ push.default::
 
 * `tracking` - This is a deprecated synonym for `upstream`.
 
-* `simple` - in centralized workflow, work like `upstream` with an
-  added safety to refuse to push if the upstream branch's name is
-  different from the local one.
+* `simple` - pushes the current branch with the same name on the remote.
 +
-When pushing to a remote that is different from the remote you normally
-pull from, work as `current`.  This is the safest option and is suited
-for beginners.
+If you are working on a centralized workflow (pushing to the same repository you
+pull from, which is typically `origin`), then you need to configure an upstream
+branch with the same name.
 +
-This mode has become the default in Git 2.0.
+This mode is the default since Git 2.0, and is the safest option suited for
+beginners.
 
 * `matching` - push all branches having the same name on both ends.
   This makes the repository you are pushing to remember the set of
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 00/13] push: revamp push.default
  2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
                   ` (6 preceding siblings ...)
  2021-05-31 19:32 ` [PATCH v3 7/7] doc: push: explain default=simple correctly Felipe Contreras
@ 2021-05-31 19:51 ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 01/13] push: create new get_upstream_ref() helper Felipe Contreras
                     ` (13 more replies)
  7 siblings, 14 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

This patch series is the new second part of [1] and it's mostly a ton of
cleanups.

In theory there should be no functional changes.

Most of the changes since v1 are due to the fact that
s/!triangular/same_remote/ was done early in the previous series. Other
than that there's a few changes to the code and commit messages
suggested by Junio C Hamano.

The end result is almost identical to v1, only the way we get there
changes (plus there's an extra cosmetic break).

There's too many changes to list them all, it's much easier to see the
end result:

static const char *get_upstream_ref(struct branch *branch, const char *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);

	return branch->merge[0]->src;
}

static void setup_default_push_refspecs(struct remote *remote)
{
	struct branch *branch;
	const char *dst;
	int same_remote;

	switch (push_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;
	}

	branch = branch_get(NULL);
	if (!branch)
		die(_(message_detached_head_die), remote->name);

	dst = branch->refname;
	same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));

	switch (push_default) {
	default:
	case PUSH_DEFAULT_UNSPECIFIED:
	case PUSH_DEFAULT_SIMPLE:
		if (!same_remote)
			break;
		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
			die_push_simple(branch, remote);
		break;

	case PUSH_DEFAULT_UPSTREAM:
		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:
		break;
	}

	refspec_appendf(&rs, "%s:%s", branch->refname, dst);
}

[1] https://lore.kernel.org/git/20210531193237.216726-1-felipe.contreras@gmail.com

Felipe Contreras (13):
  push: create new get_upstream_ref() helper
  push: return immediately in trivial switch case
  push: split switch cases
  push: factor out null branch check
  push: only get the branch when needed
  push: make setup_push_* return the dst
  push: trivial simplifications
  push: get rid of all the setup_push_* functions
  push: factor out the typical case
  push: remove redundant check
  push: remove trivial function
  push: only check same_remote when needed
  push: don't get a full remote object

 builtin/push.c | 93 ++++++++++++++++++--------------------------------
 1 file changed, 34 insertions(+), 59 deletions(-)

Range-diff against v1:
 1:  9d9d800b11 !  1:  675528cf7a push: create new get_upstream_ref() helper
    @@ builtin/push.c: static const char message_detached_head_die[] =
      	   "    git push %s HEAD:<name-of-remote-branch>\n");
      
     -static void setup_push_upstream(struct remote *remote, struct branch *branch,
    --				int triangular)
    +-				int same_remote)
     +static const char *get_upstream_ref(struct branch *branch, const char *remote_name)
      {
     -	if (!branch)
    @@ builtin/push.c: static const char message_detached_head_die[] =
     +}
     +
     +static void setup_push_upstream(struct remote *remote, struct branch *branch,
    -+				int triangular)
    ++				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 (triangular)
    + 	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."),
    @@ builtin/push.c: static const char message_detached_head_die[] =
     @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct branch *branch, int
      		die(_(message_detached_head_die), remote->name);
      
    - 	if (!triangular) {
    + 	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"
 2:  9d24821512 !  2:  dcbe8c53b5 push: return immediately in trivial switch case
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      
      	case PUSH_DEFAULT_UNSPECIFIED:
      	case PUSH_DEFAULT_SIMPLE:
    - 		setup_push_simple(remote, branch, triangular);
    + 		setup_push_simple(remote, branch, same_remote);
     -		break;
     +		return;
      
      	case PUSH_DEFAULT_UPSTREAM:
    - 		setup_push_upstream(remote, branch, triangular);
    + 		setup_push_upstream(remote, branch, same_remote);
     -		break;
     +		return;
      
 3:  160b8bee93 !  3:  55b227151f push: reorder switch cases
    @@ Metadata
     Author: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## Commit message ##
    -    push: reorder switch cases
    +    push: split switch cases
     
         We want all the cases that don't do anything with a branch first, and
    -    then the rest.
    -
    -    Will help further patches.
    +    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 <felipe.contreras@gmail.com>
     
      ## builtin/push.c ##
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
    - 	int triangular = is_workflow_triangular(remote);
    + 	int same_remote = is_same_remote(remote);
      
      	switch (push_default) {
     -	default:
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
     +		    "push.default is \"nothing\"."));
     +		return;
     +	default:
    ++		break;
     +	}
     +
     +	switch (push_default) {
     +	default:
      	case PUSH_DEFAULT_UNSPECIFIED:
      	case PUSH_DEFAULT_SIMPLE:
    - 		setup_push_simple(remote, branch, triangular);
    + 		setup_push_simple(remote, branch, same_remote);
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      	case PUSH_DEFAULT_CURRENT:
      		setup_push_current(remote, branch);
 4:  2b299e2e5a !  4:  4ea0ee4631 push: factor out null branch check
    @@ Commit message
     
      ## builtin/push.c ##
     @@ builtin/push.c: static void setup_push_upstream(struct remote *remote, struct branch *branch,
    - 				int triangular)
    + 				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 (triangular)
    + 	if (!same_remote)
      		die(_("You are pushing to remote '%s', which is not the upstream of\n"
     @@ builtin/push.c: static void setup_push_upstream(struct remote *remote, struct branch *branch,
      
    @@ builtin/push.c: static void setup_push_upstream(struct remote *remote, struct br
      	refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
      }
      
    - static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    + static void setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
      {
     -	if (!branch)
     -		die(_(message_detached_head_die), remote->name);
     -
    - 	if (!triangular) {
    + 	if (same_remote) {
      		const char *upstream_ref;
      
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
    - 	default:
    + 		break;
      	}
      
     +	if (!branch)
 5:  4a721c99f1 !  5:  ae3d0dfdfe push: only get the branch when needed
    @@ Commit message
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## builtin/push.c ##
    -@@ builtin/push.c: static int is_workflow_triangular(struct remote *remote)
    +@@ builtin/push.c: 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 triangular = is_workflow_triangular(remote);
    + 	int same_remote = is_same_remote(remote);
      
      	switch (push_default) {
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
    - 	default:
    + 		break;
      	}
      
     +	branch = branch_get(NULL);
 6:  30d5c43c28 !  6:  9d9a9ebfbe push: make setup_push_* return the dst
    @@ Commit message
         push: make setup_push_* return the dst
     
         All of the setup_push_* functions are appending a refspec. Do this only
    -    once in the parent function.
    +    once on the parent function.
     
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
    @@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const
      }
      
     -static void setup_push_upstream(struct remote *remote, struct branch *branch,
    --				int triangular)
    +-				int same_remote)
     +static const char *setup_push_upstream(struct remote *remote, struct branch *branch,
    -+	int triangular)
    ++	int same_remote)
      {
      	const char *upstream_ref;
      	upstream_ref = get_upstream_ref(branch, remote->name);
    @@ builtin/push.c: static void setup_push_upstream(struct remote *remote, struct br
     +	return branch->refname;
      }
      
    --static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    -+static const char *setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    +-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 (!triangular) {
    + 	if (same_remote) {
      		const char *upstream_ref;
     @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct branch *branch, int
      		if (strcmp(branch->refname, upstream_ref))
    @@ builtin/push.c: static void setup_push_simple(struct remote *remote, struct bran
     +	return branch->refname;
      }
      
    - static int is_workflow_triangular(struct remote *remote)
    + static int is_same_remote(struct remote *remote)
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      {
      	struct branch *branch;
    - 	int triangular = is_workflow_triangular(remote);
    + 	int same_remote = is_same_remote(remote);
     +	const char *dst;
      
      	switch (push_default) {
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      	default:
      	case PUSH_DEFAULT_UNSPECIFIED:
      	case PUSH_DEFAULT_SIMPLE:
    --		setup_push_simple(remote, branch, triangular);
    +-		setup_push_simple(remote, branch, same_remote);
     -		return;
    -+		dst = setup_push_simple(remote, branch, triangular);
    ++		dst = setup_push_simple(remote, branch, same_remote);
     +		break;
      
      	case PUSH_DEFAULT_UPSTREAM:
    --		setup_push_upstream(remote, branch, triangular);
    +-		setup_push_upstream(remote, branch, same_remote);
     -		return;
    -+		dst = setup_push_upstream(remote, branch, triangular);
    ++		dst = setup_push_upstream(remote, branch, same_remote);
     +		break;
      
      	case PUSH_DEFAULT_CURRENT:
 7:  88cd2572a3 !  7:  f96581291a push: trivial simplifications
    @@ Commit message
      ## builtin/push.c ##
     @@ builtin/push.c: 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 triangular)
    + 	int same_remote)
      {
     -	const char *upstream_ref;
     -	upstream_ref = get_upstream_ref(branch, remote->name);
    - 	if (triangular)
    + 	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."),
    @@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const
      static const char *setup_push_current(struct remote *remote, struct branch *branch)
     @@ builtin/push.c: 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 triangular)
    + static const char *setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
      {
    --	if (!triangular) {
    +-	if (same_remote) {
     -		const char *upstream_ref;
     -
     -		upstream_ref = get_upstream_ref(branch, remote->name);
     -
     -		/* Additional safety */
     -		if (strcmp(branch->refname, upstream_ref))
    -+	if (!triangular)
    ++	if (same_remote)
     +		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
      			die_push_simple(branch, remote);
     -	}
 8:  e31eba87d8 !  8:  d0cedd5c81 push: get rid of all the setup_push_* functions
    @@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const
      }
      
     -static const char *setup_push_upstream(struct remote *remote, struct branch *branch,
    --	int triangular)
    +-	int same_remote)
     -{
    --	if (triangular)
    +-	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."),
    @@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const
     -	return branch->refname;
     -}
     -
    --static const char *setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
    +-static const char *setup_push_simple(struct remote *remote, struct branch *branch, int same_remote)
     -{
    --	if (!triangular)
    +-	if (same_remote)
     -		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
     -			die_push_simple(branch, remote);
     -	return branch->refname;
     -}
     -
    - static int is_workflow_triangular(struct remote *remote)
    + static int is_same_remote(struct remote *remote)
      {
      	struct remote *fetch_remote = remote_get(NULL);
     @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      	default:
      	case PUSH_DEFAULT_UNSPECIFIED:
      	case PUSH_DEFAULT_SIMPLE:
    --		dst = setup_push_simple(remote, branch, triangular);
    -+		if (!triangular)
    +-		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, triangular);
    -+		if (triangular)
    +-		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."),
 9:  d5f60ad791 !  9:  47bbad5a47 push: factor out the typical case
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      	default:
      	case PUSH_DEFAULT_UNSPECIFIED:
      	case PUSH_DEFAULT_SIMPLE:
    --		if (!triangular)
    +-		if (same_remote)
     -			if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
     -				die_push_simple(branch, remote);
     -		dst = branch->refname;
    -+		if (triangular)
    ++		if (!same_remote)
     +			break;
     +		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
     +			die_push_simple(branch, remote);
10:  e1945bb451 <  -:  ---------- push: remove redundant check
11:  93f8b38364 <  -:  ---------- push: fix Yoda condition
12:  cdf961d231 <  -:  ---------- push: remove trivial function
 -:  ---------- > 10:  6a8e30bf38 push: remove redundant check
 -:  ---------- > 11:  976d7b9f3a push: remove trivial function
13:  e5e55c00e7 ! 12:  d9df139855 push: only get triangular when needed
    @@ Metadata
     Author: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## Commit message ##
    -    push: only get triangular when needed
    +    push: only check same_remote when needed
     
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
    @@ builtin/push.c: static const char *get_upstream_ref(struct branch *branch, const
      static void setup_default_push_refspecs(struct remote *remote)
      {
      	struct branch *branch;
    --	int triangular = remote != remote_get(NULL);
    +-	int same_remote = remote == remote_get(NULL);
      	const char *dst;
    -+	int triangular;
    ++	int same_remote;
      
      	switch (push_default) {
      	case PUSH_DEFAULT_MATCHING:
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      		die(_(message_detached_head_die), remote->name);
      
      	dst = branch->refname;
    -+	triangular = remote != remote_get(NULL);
    ++	same_remote = remote == remote_get(NULL);
      
      	switch (push_default) {
      	default:
14:  2fd84a4312 ! 13:  ffc52d649c push: don't get a full remote object
    @@ Metadata
      ## Commit message ##
         push: don't get a full remote object
     
    -    All we need to know is that their names are different.
    +    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 <felipe.contreras@gmail.com>
     
    @@ builtin/push.c: static void setup_default_push_refspecs(struct remote *remote)
      		die(_(message_detached_head_die), remote->name);
      
      	dst = branch->refname;
    --	triangular = remote != remote_get(NULL);
    -+	triangular = strcmp(remote->name, remote_for_branch(branch, NULL));
    +-	same_remote = remote == remote_get(NULL);
    ++	same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
      
      	switch (push_default) {
      	default:
15:  2a203239e4 <  -:  ---------- push: rename !triangular to same_remote
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 01/13] push: create new get_upstream_ref() helper
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 02/13] push: return immediately in trivial switch case Felipe Contreras
                     ` (12 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

This code is duplicated among multiple functions.

No functional changes.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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:<name-of-remote-branch>\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);
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 02/13] push: return immediately in trivial switch case
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 01/13] push: create new get_upstream_ref() helper Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 03/13] push: split switch cases Felipe Contreras
                     ` (11 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

There's no need to break when nothing else will be executed.

Will help next patches.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
 	}
 }
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 03/13] push: split switch cases
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 01/13] push: create new get_upstream_ref() helper Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 02/13] push: return immediately in trivial switch case Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 04/13] push: factor out null branch check Felipe Contreras
                     ` (10 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

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 <felipe.contreras@gmail.com>
---
 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;
 	}
 }
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 04/13] push: factor out null branch check
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (2 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 03/13] push: split switch cases Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 05/13] push: only get the branch when needed Felipe Contreras
                     ` (9 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

No need to do it in every single function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 05/13] push: only get the branch when needed
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (3 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 04/13] push: factor out null branch check Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 06/13] push: make setup_push_* return the dst Felipe Contreras
                     ` (8 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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);
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 06/13] push: make setup_push_* return the dst
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (4 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 05/13] push: only get the branch when needed Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 07/13] push: trivial simplifications Felipe Contreras
                     ` (7 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

All of the setup_push_* functions are appending a refspec. Do this only
once on the parent function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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[] =
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 07/13] push: trivial simplifications
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (5 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 06/13] push: make setup_push_* return the dst Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 08/13] push: get rid of all the setup_push_* functions Felipe Contreras
                     ` (6 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
 }
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 08/13] push: get rid of all the setup_push_* functions
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (6 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 07/13] push: trivial simplifications Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 09/13] push: factor out the typical case Felipe Contreras
                     ` (5 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

Their code is much simpler now and can move into the parent function.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
 	}
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 09/13] push: factor out the typical case
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (7 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 08/13] push: get rid of all the setup_push_* functions Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 10/13] push: remove redundant check Felipe Contreras
                     ` (4 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

Only override dst on the odd case.

This allows a preemptive break on the `simple` case.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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;
 	}
 
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 10/13] push: remove redundant check
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (8 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 09/13] push: factor out the typical case Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 11/13] push: remove trivial function Felipe Contreras
                     ` (3 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

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 <felipe.contreras@gmail.com>
---
 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)
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 11/13] push: remove trivial function
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (9 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 10/13] push: remove redundant check Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 12/13] push: only check same_remote when needed Felipe Contreras
                     ` (2 subsequent siblings)
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

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 <felipe.contreras@gmail.com>
---
 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) {
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 12/13] push: only check same_remote when needed
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (10 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 11/13] push: remove trivial function Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-05-31 19:51   ` [PATCH v2 13/13] push: don't get a full remote object Felipe Contreras
  2021-06-02  1:16   ` [PATCH v2 00/13] push: revamp push.default Junio C Hamano
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 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:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 13/13] push: don't get a full remote object
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (11 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 12/13] push: only check same_remote when needed Felipe Contreras
@ 2021-05-31 19:51   ` Felipe Contreras
  2021-06-02  1:16   ` [PATCH v2 00/13] push: revamp push.default Junio C Hamano
  13 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-05-31 19:51 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Elijah Newren, Brandon Williams,
	René Scharfe, Felipe Contreras

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 <felipe.contreras@gmail.com>
---
 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:
-- 
2.32.0.rc0


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 00/13] push: revamp push.default
  2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
                     ` (12 preceding siblings ...)
  2021-05-31 19:51   ` [PATCH v2 13/13] push: don't get a full remote object Felipe Contreras
@ 2021-06-02  1:16   ` Junio C Hamano
  2021-06-02  4:05     ` Felipe Contreras
  13 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2021-06-02  1:16 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: git, Elijah Newren, Brandon Williams, René Scharfe

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

> The end result is almost identical to v1, only the way we get there
> changes (plus there's an extra cosmetic break).

Good.  The endpoint matches exactly to what was queued, with the
SQUASH??? fix for the "break" thing, which is necessary to make
"make sparse" happy (so it is not just "cosmetic").

IOW, this series allows us to drop the following.

commit c1964311c404afaadffd57d14d769a4051281c2b
Author: Junio C Hamano <gitster@pobox.com>
Date:   Mon May 31 12:41:06 2021 +0900

    SQUASH??? fix compilation breakage
    
    make sparse found these:
    
    builtin/push.c:221:9: error: Expected ; at end of statement
    builtin/push.c:221:9: error: got }

diff --git a/builtin/push.c b/builtin/push.c
index f1ac531252..e8b10a9b7e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -218,6 +218,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		    "push.default is \"nothing\"."));
 		return;
 	default:
+		break;
 	}
 
 	branch = branch_get(NULL);

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 00/13] push: revamp push.default
  2021-06-02  1:16   ` [PATCH v2 00/13] push: revamp push.default Junio C Hamano
@ 2021-06-02  4:05     ` Felipe Contreras
  0 siblings, 0 replies; 24+ messages in thread
From: Felipe Contreras @ 2021-06-02  4:05 UTC (permalink / raw)
  To: Junio C Hamano, Felipe Contreras
  Cc: git, Elijah Newren, Brandon Williams, René Scharfe

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > The end result is almost identical to v1, only the way we get there
> > changes (plus there's an extra cosmetic break).
> 
> Good.  The endpoint matches exactly to what was queued, with the
> SQUASH??? fix for the "break" thing, which is necessary to make
> "make sparse" happy (so it is not just "cosmetic").
> 
> IOW, this series allows us to drop the following.

Indeed.

> commit c1964311c404afaadffd57d14d769a4051281c2b
> Author: Junio C Hamano <gitster@pobox.com>
> Date:   Mon May 31 12:41:06 2021 +0900
> 
>     SQUASH??? fix compilation breakage
>     
>     make sparse found these:
>     
>     builtin/push.c:221:9: error: Expected ; at end of statement
>     builtin/push.c:221:9: error: got }

Is it really a "compilation breakage"?

Documentation/SubmittingPatches doesn't mention sparse. I do remember
setting up sparse when I worked with Linux in the past, but it's not
precisely a standard thing. At least in Arch Linux it's not even part of
the main packages [1].

Cheers.

[1] https://aur.archlinux.org/packages/sparse/

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2021-06-02  4:05 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-31 19:32 [PATCH v3 0/7] Unconvolutize push.default=simple Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 1/7] push: rename !triangular to same_remote Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 2/7] push: hedge code of default=simple Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 3/7] push: copy code to setup_push_simple() Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 4/7] push: reorganize setup_push_simple() Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 5/7] push: simplify setup_push_simple() Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 6/7] push: remove unused code in setup_push_upstream() Felipe Contreras
2021-05-31 19:32 ` [PATCH v3 7/7] doc: push: explain default=simple correctly Felipe Contreras
2021-05-31 19:51 ` [PATCH v2 00/13] push: revamp push.default Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 01/13] push: create new get_upstream_ref() helper Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 02/13] push: return immediately in trivial switch case Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 03/13] push: split switch cases Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 04/13] push: factor out null branch check Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 05/13] push: only get the branch when needed Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 06/13] push: make setup_push_* return the dst Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 07/13] push: trivial simplifications Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 08/13] push: get rid of all the setup_push_* functions Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 09/13] push: factor out the typical case Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 10/13] push: remove redundant check Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 11/13] push: remove trivial function Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 12/13] push: only check same_remote when needed Felipe Contreras
2021-05-31 19:51   ` [PATCH v2 13/13] push: don't get a full remote object Felipe Contreras
2021-06-02  1:16   ` [PATCH v2 00/13] push: revamp push.default Junio C Hamano
2021-06-02  4:05     ` Felipe Contreras

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).