[PATCH v2 0/6] Unconvolutize push.default=simple

[PATCH v2 0/6] Unconvolutize push.default=simple

[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.

This is basically the same as v1, except I removed a bunch of patches which are now part of a
different series. Also, I updated some commit messages with suggestions from Elijah Newren.

The result of this series is in short this function:

static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
{
	if (!branch)
		die(_(message_detached_head_die), remote->name);

	if (!triangular) {
		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 (6):
  push: hedge code of default=simple
  push: move 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                | 40 ++++++++++++++++++++++++-----------
 2 files changed, 34 insertions(+), 19 deletions(-)

Range-diff against v1:
 1:  2856711eb3 !  1:  f1f42bda32 push: hedge code of default=simple
    @@ Commit message
         `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 ##
 2:  33acb09e82 !  2:  bb6d810011 push: move code to setup_push_simple()
    @@ Commit message
     
         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 ##
 3:  de1b621b7e !  3:  d66a442fba push: reorganize setup_push_simple()
    @@ Metadata
      ## Commit message ##
         push: reorganize setup_push_simple()
     
    -    Simply move the code around.
    +    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.
     
         No functional changes.
     
    +    Suggestions-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## builtin/push.c ##
 4:  83efcad143 !  4:  eaae6a826a push: simplify setup_push_simple()
    @@ Metadata
      ## Commit message ##
         push: simplify setup_push_simple()
     
    -    branch->refname can never be different from branch->merge[0]->src.
    +    There's a safety check to make sure branch->refname is not 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 ##
 5:  d7489e9545 =  5:  8d9ae5e552 push: remove unused code in setup_push_upstream()
 6:  e693341403 <  -:  ---------- push: merge current and simple
 7:  830a57c867 <  -:  ---------- push: remove redundant check
 8:  d2dded5998 <  -:  ---------- push: fix Yoda condition
 9:  7528738091 <  -:  ---------- push: remove trivial function
10:  1ae0546df6 <  -:  ---------- push: flip !triangular for centralized
11:  3acd42e385 !  6:  b35bdf14dc doc: push: explain default=simple correctly
    @@ Metadata
      ## Commit message ##
         doc: push: explain default=simple correctly
     
    -    Now that the code has been unconvolutized and it's clear what it's
    +    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
-- 
2.32.0.rc0

[PATCH v2 1/6] push: hedge code of default=simple

[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 194967ed79..7045e4ef0c 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 triangular)
+{
+	if (triangular)
+		setup_push_current(remote, branch);
+	else
+		setup_push_upstream(remote, branch, triangular, 1);
+}
+
 static int is_workflow_triangular(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 (triangular)
-			setup_push_current(remote, branch);
-		else
-			setup_push_upstream(remote, branch, triangular, 1);
+		setup_push_simple(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-- 
2.32.0.rc0

Re: [PATCH v2 1/6] push: hedge code of default=simple

[Junio C Hamano]

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

> `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.

The change and the above description makes sense to me.

I didn't, and still don't, understand the use of verb "to hedge" in
the title, though.  Apparently it isn't "to evade the risk of
commitment", "to protect oneself finantially", and of course not "to
plant, form, or trim a hedge".

> Reviewed-by: Elijah Newren <newren@gmail.com>

I trust Elijah would complain and/or clarify if this footer is
inappropriate (I didn't see an explicit "You can have my
Reviewed-by", but only "this looks good to me", and didn't know
what he meant).

I do like the change of the phrasing from triangular to same-remote
at the end of the extended series, by the way.  It makes the code
simpler to read and much easier to reason about, and it would be
nice to have it even before this step ;-)


> 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 194967ed79..7045e4ef0c 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 triangular)
> +{
> +	if (triangular)
> +		setup_push_current(remote, branch);
> +	else
> +		setup_push_upstream(remote, branch, triangular, 1);
> +}
> +
>  static int is_workflow_triangular(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 (triangular)
> -			setup_push_current(remote, branch);
> -		else
> -			setup_push_upstream(remote, branch, triangular, 1);
> +		setup_push_simple(remote, branch, triangular);
>  		break;
>  
>  	case PUSH_DEFAULT_UPSTREAM:

Re: [PATCH v2 1/6] push: hedge code of default=simple

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > `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.
> 
> The change and the above description makes sense to me.
> 
> I didn't, and still don't, understand the use of verb "to hedge" in
> the title, though.

> Apparently it isn't "to evade the risk of
> commitment", "to protect oneself finantially", and of course not "to
> plant, form, or trim a hedge".

Those appear to be the intransitive verb definitions of Merriam-Webster
[1], there's an object in the sentence (code), so it's the transitive
ones that are applicable:

 * to enclose or protect with or as if with a dense row of shrubs or low
   trees: ENCIRCLE
 * to confine so as to prevent freedom of movement or action

Some synonyms: block, border, cage, confine, coop, corral, edge, fence,
restrict, ring, surround.

I think it fits.

> > Reviewed-by: Elijah Newren <newren@gmail.com>
> 
> I trust Elijah would complain and/or clarify if this footer is
> inappropriate (I didn't see an explicit "You can have my
> Reviewed-by", but only "this looks good to me", and didn't know
> what he meant).

But he did review the entire series. So I think it's safe to say he
reviewed this patch.

> I do like the change of the phrasing from triangular to same-remote
> at the end of the extended series, by the way.  It makes the code
> simpler to read and much easier to reason about, and it would be
> nice to have it even before this step ;-)

All right, I'll keep in mind for the next round.

Cheers.

[1] https://www.merriam-webster.com/dictionary/hedge

-- 
Felipe Contreras

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

[Felipe Contreras]

In order to avoid doing unnecessary things and simplify it in further
patches.

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 7045e4ef0c..d173c39283 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,10 +225,38 @@ static void setup_push_current(struct remote *remote, struct branch *branch)
 
 static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
-	if (triangular)
-		setup_push_current(remote, branch);
-	else
-		setup_push_upstream(remote, branch, triangular, 1);
+	if (triangular) {
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
+		refspec_appendf(&rs, "%s:%s", branch->refname, branch->refname);
+	} else {
+		if (!branch)
+			die(_(message_detached_head_die), remote->name);
+		if (!branch->merge_nr || !branch->merge || !branch->remote_name)
+			die(_("The current branch %s has no upstream branch.\n"
+			    "To push the current branch and set the remote as upstream, use\n"
+			    "\n"
+			    "    git push --set-upstream %s %s\n"),
+			    branch->name,
+			    remote->name,
+			    branch->name);
+		if (branch->merge_nr != 1)
+			die(_("The current branch %s has multiple upstream branches, "
+			    "refusing to push."), branch->name);
+		if (triangular)
+			die(_("You are pushing to remote '%s', which is not the upstream of\n"
+			      "your current branch '%s', without telling me what to push\n"
+			      "to update which remote branch."),
+			    remote->name, branch->name);
+
+		if (1) {
+			/* Additional safety */
+			if (strcmp(branch->refname, branch->merge[0]->src))
+				die_push_simple(branch, remote);
+		}
+
+		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
+	}
 }
 
 static int is_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

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

[Junio C Hamano]

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

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

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

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

Let's keep reading.

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

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

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > In order to avoid doing unnecessary things and simplify it in further
> > patches.
> >
> > The code is copied exactly as-is; no functional changes.
> 
> The title says "move" and I expected to see corresponding deletions,
> but it seems that this (possibly temporarily) duplicates what these
> two functions do, without removing them, which (possibly
> temporarily) risks the two to drift apart.

OK. Copy then.

> From a quick scanning of the remainder of the series, it seems that
> 3/6 and 4/6 improve the copied code without changing the behaviour,
> and at the end these two functions remain, so we have duplicated
> logic for these two functions and improvements only live in one of
> the copies (namely, in the setup_push_simple())?

Not quite: setup_push_upstream() was simplified too, in 5/6.

Maybe I can mention that part of the code of the `simple` mode lives in
setup_push_upstream() and by copying the code to the right function both
can be simplified.

-- 
Felipe Contreras

[PATCH v2 3/6] push: reorganize setup_push_simple()

[Felipe Contreras]

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.

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 d173c39283..9c807ed707 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 triangular)
 {
+	const char *dst;
+
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+
 	if (triangular) {
-		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 (triangular)
-			die(_("You are pushing to remote '%s', which is not the upstream of\n"
-			      "your current branch '%s', without telling me what to push\n"
-			      "to update which remote branch."),
-			    remote->name, branch->name);
-
-		if (1) {
-			/* Additional safety */
-			if (strcmp(branch->refname, branch->merge[0]->src))
-				die_push_simple(branch, remote);
-		}
 
-		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
+		/* 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_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

Re: [PATCH v2 3/6] push: reorganize setup_push_simple()

[Junio C Hamano]

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

> 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.
>
> 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(-)

Much cleaner.  Nice.

> diff --git a/builtin/push.c b/builtin/push.c
> index d173c39283..9c807ed707 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 triangular)
>  {
> +	const char *dst;
> +
> +	if (!branch)
> +		die(_(message_detached_head_die), remote->name);
> +
>  	if (triangular) {
> -		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 (triangular)
> -			die(_("You are pushing to remote '%s', which is not the upstream of\n"
> -			      "your current branch '%s', without telling me what to push\n"
> -			      "to update which remote branch."),
> -			    remote->name, branch->name);
> -
> -		if (1) {
> -			/* Additional safety */
> -			if (strcmp(branch->refname, branch->merge[0]->src))
> -				die_push_simple(branch, remote);
> -		}
>  
> -		refspec_appendf(&rs, "%s:%s", branch->refname, branch->merge[0]->src);
> +		/* 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_workflow_triangular(struct remote *remote)

[PATCH v2 4/6] push: simplify setup_push_simple()

[Felipe Contreras]

There's a safety check to make sure branch->refname is not 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 9c807ed707..73fe083682 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 triangular)
 {
-	const char *dst;
-
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 
-	if (triangular) {
-		dst = branch->refname;
-	} else {
+	if (!triangular) {
 		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_workflow_triangular(struct remote *remote)
-- 
2.32.0.rc0

Re: [PATCH v2 4/6] push: simplify setup_push_simple()

[Junio C Hamano]

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

> There's a safety check to make sure branch->refname is not 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(-)

Simpler and nicer.  Good.


> diff --git a/builtin/push.c b/builtin/push.c
> index 9c807ed707..73fe083682 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 triangular)
>  {
> -	const char *dst;
> -
>  	if (!branch)
>  		die(_(message_detached_head_die), remote->name);
>  
> -	if (triangular) {
> -		dst = branch->refname;
> -	} else {
> +	if (!triangular) {
>  		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_workflow_triangular(struct remote *remote)

[PATCH v2 5/6] push: remove unused code in setup_push_upstream()

[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 73fe083682..0961826269 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 triangular)
 {
 	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, triangular, 0);
+		setup_push_upstream(remote, branch, triangular);
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-- 
2.32.0.rc0

[PATCH v2 6/6] doc: push: explain default=simple correctly

[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

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Mathias Kunter]

Am 29.05.21 um 09:11 schrieb 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.
> 
> [...]
> 
> +* `simple` - pushes the current branch with the same name on the remote.
> +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.

I'd like to remark that I personally find the following description of 
`push.default=simple`, taken from the git push man page [1], easier to 
understand:

> The current branch is pushed to the corresponding upstream branch, but
> as a safety measure, the push is aborted if the upstream branch does not
> have the same name as the local one.

[1] https://git-scm.com/docs/git-push#_description

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Mathias Kunter wrote:
> Am 29.05.21 um 09:11 schrieb 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.
> > 
> > [...]
> > 
> > +* `simple` - pushes the current branch with the same name on the remote.
> > +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.
> 
> I'd like to remark that I personally find the following description of 
> `push.default=simple`, taken from the git push man page [1], easier to 
> understand:
> 
> > The current branch is pushed to the corresponding upstream branch, but
> > as a safety measure, the push is aborted if the upstream branch does not
> > have the same name as the local one.

Except that isn't accurate.

  git clone $url
  git checkout -b fix-1
  # do commits
  git push

Does that push the current branch to the corresponding upstream branch?

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Mathias Kunter]

Am 30.05.21 um 21:09 schrieb Felipe Contreras:
>>> The current branch is pushed to the corresponding upstream branch, but
>>> as a safety measure, the push is aborted if the upstream branch does not
>>> have the same name as the local one.
> 
> Except that isn't accurate.
> 
>    git clone $url
>    git checkout -b fix-1
>    # do commits
>    git push
> 
> Does that push the current branch to the corresponding upstream branch?

I see. Then maybe reword to something like this:

> The current branch is pushed to a branch with the same name on the
> remote, but as a safety measure, the push is aborted if a corresponding
> upstream branch does not have the same name as the local one.

In your above example, I'm in centralized workflow, but I can still push 
the fix-1 branch to origin without having to configure an upstream 
branch for it. So this seems to contradict with the currently proposed 
wording:

> If you are working on a centralized workflow, then you need to configure
> an upstream branch with the same name.

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Mathias Kunter wrote:
> Am 30.05.21 um 21:09 schrieb Felipe Contreras:
> >>> The current branch is pushed to the corresponding upstream branch, but
> >>> as a safety measure, the push is aborted if the upstream branch does not
> >>> have the same name as the local one.
> > 
> > Except that isn't accurate.
> > 
> >    git clone $url
> >    git checkout -b fix-1
> >    # do commits
> >    git push
> > 
> > Does that push the current branch to the corresponding upstream branch?
> 
> I see. Then maybe reword to something like this:
> 
> > The current branch is pushed to a branch with the same name on the
> > remote, but as a safety measure, the push is aborted if a corresponding
> > upstream branch does not have the same name as the local one.
> 
> In your above example, I'm in centralized workflow, but I can still push 
> the fix-1 branch to origin without having to configure an upstream 
> branch for it.

No, you can't:

  % git push
  fatal: The current branch fix-1 has no upstream branch.
  To push the current branch and set the remote as upstream, use

      git push --set-upstream origin fix-1

Isn't that problem the one you originally described [1]?

The behavior doesn't change if you don't specify the remote:
`git push` == `git push origin`.

> So this seems to contradict with the currently proposed 
> wording:
> 
> > If you are working on a centralized workflow, then you need to configure
> > an upstream branch with the same name.

My wording is correct, and that's precisely the problem.

Maybe you are thinking this patch series implements the fix I proposed:
it doesn't.

The two patch series merely reorganizes the code to make it simpler and
easier to understand. No functionality changes.

Cheers.

[1] https://lore.kernel.org/git/065528bf-b496-83d3-767d-2a78e50a9edf@gmail.com

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Mathias Kunter]

Am 31.05.21 um 17:14 schrieb Felipe Contreras:
>> In your above example, I'm in centralized workflow, but I can still push
>> the fix-1 branch to origin without having to configure an upstream
>> branch for it.
> 
> No, you can't:
> 
>    % git push
>    fatal: The current branch fix-1 has no upstream branch.
>    To push the current branch and set the remote as upstream, use
> 
>        git push --set-upstream origin fix-1
> 
> Isn't that problem the one you originally described [1]?

It is.

> Maybe you are thinking this patch series implements the fix I proposed:
> it doesn't.

Yes, I thought so. Sorry for the confusion. When I asked "will your 
provided patch fix these failing push commands" you answered "It's not 
really a patch (yet), but yeah: it will". So that's why I thought so.

My point simply is: For the sake of easiness of use, I think the 
following should work with the default settings of git:

    git clone $url
    git checkout -b fix-1
    # do commits
    git push           # should push to origin/fix-1
    # others push to origin/fix-1 (but no local changes)
    git pull           # should pull from origin/fix-1

Will this be implemented?

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Mathias Kunter wrote:
> Am 31.05.21 um 17:14 schrieb Felipe Contreras:
> >> In your above example, I'm in centralized workflow, but I can still push
> >> the fix-1 branch to origin without having to configure an upstream
> >> branch for it.
> > 
> > No, you can't:
> > 
> >    % git push
> >    fatal: The current branch fix-1 has no upstream branch.
> >    To push the current branch and set the remote as upstream, use
> > 
> >        git push --set-upstream origin fix-1
> > 
> > Isn't that problem the one you originally described [1]?
> 
> It is.
> 
> > Maybe you are thinking this patch series implements the fix I proposed:
> > it doesn't.
> 
> Yes, I thought so. Sorry for the confusion. When I asked "will your 
> provided patch fix these failing push commands" you answered "It's not 
> really a patch (yet), but yeah: it will". So that's why I thought so.

I have not yet sent the patch to change the current behavior.

First I sent a bunch of patches to reorganize the code so it's more
clear what it's actually doing.

Once those patches are merged, then I will probably send the one to
change the behavior.

I will include you in the Cc list when I do so.

> My point simply is: For the sake of easiness of use, I think the 
> following should work with the default settings of git:
> 
>     git clone $url
>     git checkout -b fix-1
>     # do commits
>     git push           # should push to origin/fix-1
>     # others push to origin/fix-1 (but no local changes)
>     git pull           # should pull from origin/fix-1

I agree.

> Will this be implemented?

It's hard do say, but it probably won't.

All my suggestions for improvement are mostly disregarded, and
considering *nobody* has said a word about whether or not the current
behavior makes sense, I don't think there are very good chances of
changing it.

Consider my last mail "git push default doesn't make sense" [1]: nobody
replied.

But maybe that's my fault, because even though I broke the In-Reply-To
in order to start a new thread, it seems mail handling software still
uses References (incorrectly IMO).

So maybe if I start a new thread completely fresh people will notice,
and reply.

I will do so with a patch once I'm done with the reorganization.


In my experience you need to convince either Junio Hamano, or Jeff King
for any change in behavior to happen, and until they do comment on this
one it's fair to say it won't happen.

Cheers.

[1] https://lore.kernel.org/git/60b15cd2c4136_2183bc20893@natae.notmuch/

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Mathias Kunter]

Am 31.05.21 um 19:31 schrieb Felipe Contreras:
> Once those patches are merged, then I will probably send the one to
> change the behavior.
> 
> I will include you in the Cc list when I do so.

Thank you very much.

> In my experience you need to convince either Junio Hamano, or Jeff King
> for any change in behavior to happen, and until they do comment on this
> one it's fair to say it won't happen.

I assume they are on this mailing list?

I'd say it should be quite an argument if the related StackOverflow 
question dealing with this exact issue is one of the top-voted git 
questions of all time. [1]

It seems that millions of developers (judging by the number of views of 
that question) wonder what they need to do so that "git pull and git 
push will work immediately" on a newly created local branch.

If that would "just work" out of the box and with the default settings 
of git, without having to read up the solution in StackOverflow first, 
then it would certainly be an improvement for a huge number of people. 
So, to all whom it may concern, I think a behavioral change is advisable 
here.

Thank you.

[1] https://stackoverflow.com/q/2765421

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Mathias Kunter wrote:
> Am 31.05.21 um 19:31 schrieb Felipe Contreras:
> > Once those patches are merged, then I will probably send the one to
> > change the behavior.
> > 
> > I will include you in the Cc list when I do so.
> 
> Thank you very much.
> 
> > In my experience you need to convince either Junio Hamano, or Jeff King
> > for any change in behavior to happen, and until they do comment on this
> > one it's fair to say it won't happen.
> 
> I assume they are on this mailing list?

Of course, and they are also on the Cc list of this mail.

> I'd say it should be quite an argument if the related StackOverflow 
> question dealing with this exact issue is one of the top-voted git 
> questions of all time. [1]

That seems to be a different issue. Related, but not quite the same.

That question is answered at the time the user tries to make a push:

  fatal: The current branch fix-1 has no upstream branch.
  To push the current branch and set the remote as upstream, use

      git push --set-upstream origin fix-1

That's literally the top answer in SO.

> It seems that millions of developers (judging by the number of views of 
> that question) wonder what they need to do so that "git pull and git 
> push will work immediately" on a newly created local branch.

Yes, probably many people do wonder that, but not necessarily all of
them (at least of the ones that participatd in that SO question).

> If that would "just work" out of the box and with the default settings 
> of git, without having to read up the solution in StackOverflow first, 
> then it would certainly be an improvement for a huge number of people. 

I agree, but I have already tried to improve the interface in a number
of ways to no avail.

Inertia is a powerful force.

We'll see how this one goes.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Mathias Kunter]

Am 31.05.21 um 23:15 schrieb Felipe Contreras:
>> I'd say it should be quite an argument if the related StackOverflow
>> question dealing with this exact issue is one of the top-voted git
>> questions of all time. [1]
> 
> That seems to be a different issue. Related, but not quite the same.

Well, what the OP actually wanted to know there is "how can I use git 
pull and git push on a new local branch". The OP knows it can be done by 
configuring an upstream branch, and therefore he also specifically asks 
about the details of how to do that.

> We'll see how this one goes.

Thank you for your efforts.

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Junio C Hamano]

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

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

I suspect it would be simpler to read and easier to understand to
bring the parethesized part front, e.g.

    If you are pushing to the same repository you pull from (which
    is typically `origin`), then you need to ...

as it would avoid "the project is not centralized, but I push to my
own repository and pull from it---what should I do?" questions.

>  +
> +This mode is the default since Git 2.0, and is the safest option suited for
> +beginners.

Nice.

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > +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.
> 
> I suspect it would be simpler to read and easier to understand to
> bring the parethesized part front, e.g.
> 
>     If you are pushing to the same repository you pull from (which
>     is typically `origin`), then you need to ...
> 
> as it would avoid "the project is not centralized, but I push to my
> own repository and pull from it---what should I do?" questions.

The top of `push.default says:

  Different values are well-suited for specific workflows; for instance,
  in a purely central workflow (i.e. the fetch source is equal to the
  push destination), `upstream` is probably what you want.

We already brought up the central workflow, I think it's fine to reuse
the concept below.

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Junio C Hamano]

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

>> I suspect it would be simpler to read and easier to understand to
>> bring the parethesized part front, e.g.
>> 
>>     If you are pushing to the same repository you pull from (which
>>     is typically `origin`), then you need to ...
>> 
>> as it would avoid "the project is not centralized, but I push to my
>> own repository and pull from it---what should I do?" questions.
>
> The top of `push.default says:
>
>   Different values are well-suited for specific workflows; for instance,
>   in a purely central workflow (i.e. the fetch source is equal to the
>   push destination), `upstream` is probably what you want.
>
> We already brought up the central workflow, I think it's fine to reuse
> the concept below.

Oh, thanks for finding another instance to be corrected.  Even in
that sentence, the more important point is that upstream would be
appropriate if you push to the same place as you fetch from, and
we do not have to say "purely central" at all.

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> >> I suspect it would be simpler to read and easier to understand to
> >> bring the parethesized part front, e.g.
> >> 
> >>     If you are pushing to the same repository you pull from (which
> >>     is typically `origin`), then you need to ...
> >> 
> >> as it would avoid "the project is not centralized, but I push to my
> >> own repository and pull from it---what should I do?" questions.
> >
> > The top of `push.default says:
> >
> >   Different values are well-suited for specific workflows; for instance,
> >   in a purely central workflow (i.e. the fetch source is equal to the
> >   push destination), `upstream` is probably what you want.
> >
> > We already brought up the central workflow, I think it's fine to reuse
> > the concept below.
> 
> Oh, thanks for finding another instance to be corrected.

In fact there's many:

  This mode only makes sense if you are pushing to the same repository
  you would normally pull from (i.e. central workflow).

> Even in that sentence, the more important point is that upstream would
> be appropriate if you push to the same place as you fetch from, and we
> do not have to say "purely central" at all.

Actually, I forgot the main reason I decided to rename centralized to
same_repo: they are not the same thing.

You can have a decentralized workflow where you have multiple
repositories configured, but every branch pulls and pushes to the same
repository (to them, not other branches):

  hotfix <-> dayjob/production
  cleanups <-> upstream/master
  experiment-1 <-> personal/experiment-1

So it's more like:

  centralized = ~decentralized
  triangular = ~two-way

A centralized workflow consists of a single repository where branches
are typically two-way, but not necessarily.

A decentralized workflow consists of multiple repositories where
branches are typically triangular, but not necessarily.

So the triangularity is per branch, not per repository, and same_repo
means a two-way branch, could be a centralized or decentralized
workflow.

Therefore your proposal:

  If you are pushing to the same repository you pull from (which is
  typically `origin`), then you need to ...

is actually better, I just had to remind myself that centralized and
same repo are not the same thing.


Of course it would help to have a place in Documentation/ where
trianguar and centralized are explained.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Philip Oakley]

On 01/06/2021 13:12, Felipe Contreras wrote:
> So it's more like:
>
>   centralized = ~decentralized
>   triangular = ~two-way
>
> A centralized workflow consists of a single repository where branches
> are typically two-way, but not necessarily.
>
> A decentralized workflow consists of multiple repositories where
> branches are typically triangular, but not necessarily.
>
> So the triangularity is per branch, not per repository, and same_repo
> means a two-way branch, could be a centralized or decentralized
> workflow.
My personal viewpoint is that triangular flow happens when you cannot
push to the repo you consider as upstream.

Rather you typically have a publish/backup repo instead (semi-public,
semi-private - few are interested ;-).

That (can't push one way around the triangle) part of the flow is
separate from the distinction between patch flows and merge (Pull)
request flows.

E.g. My personal Git repo can be triangular with both git.git and
git-for-windows, plus a few (what I view as) fetch-only repos from other
collaborators/maintainers beyond the triangular 'golden' upstream repo.

I often consider GitHub as a centraliser, but I don't think it's what is
being considered above.

--
A thought did come to mind that a Git serve/repo (typically bare) should
be able to offer a 'refs/users/*' space (c.f. refs/remotes used by
individual users) that allows a type of 'centralised' operation (almost
as if all the users used a common alternates repo). Users could only
push to their own /user refs, but could pull from the main refs/heads,
and their own refs/users/ space.

This would give flexibility to smaller corporate central operations to
offer 'triangular flow' where each dev would feel like they have their
own 'push' repo, when in reality it's really personalised branches. As
usual the authentication of user names being handed off elsewhere;-). It
could avoid some of the --alternate management aspects.

It's a thought..

--

Philip

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Philip Oakley wrote:
> On 01/06/2021 13:12, Felipe Contreras wrote:
> > So it's more like:
> >
> >   centralized = ~decentralized
> >   triangular = ~two-way
> >
> > A centralized workflow consists of a single repository where branches
> > are typically two-way, but not necessarily.
> >
> > A decentralized workflow consists of multiple repositories where
> > branches are typically triangular, but not necessarily.
> >
> > So the triangularity is per branch, not per repository, and same_repo
> > means a two-way branch, could be a centralized or decentralized
> > workflow.

> My personal viewpoint is that triangular flow happens when you cannot
> push to the repo you consider as upstream.

It's not about permissions. Even if I had permissions to push to git.git,
I wouldn't do so. I do have permission to push to some public projects, but I
instead send patches/pull requests like everyone else.

It's more about ownership. In my personal repositories I can push
whatever I want, but on shared repositories I have to be more careful.

> Rather you typically have a publish/backup repo instead (semi-public,
> semi-private - few are interested ;-).
> 
> That (can't push one way around the triangle) part of the flow is
> separate from the distinction between patch flows and merge (Pull)
> request flows.

I think it's not separate, that is the thing that makes a triangular
flow triangular: the flow of patches goes through a different repository,
and then they get picked and merged into the upstream one.

> E.g. My personal Git repo can be triangular with both git.git and
> git-for-windows, plus a few (what I view as) fetch-only repos from other
> collaborators/maintainers beyond the triangular 'golden' upstream repo.
> 
> I often consider GitHub as a centraliser, but I don't think it's what is
> being considered above.

GitHub is all about pull requests, you fork a repository, you push your
branch into that personal fork, and then you request a pull from
upstream.

That's triangular.

> --
> A thought did come to mind that a Git serve/repo (typically bare) should
> be able to offer a 'refs/users/*' space (c.f. refs/remotes used by
> individual users) that allows a type of 'centralised' operation (almost
> as if all the users used a common alternates repo). Users could only
> push to their own /user refs, but could pull from the main refs/heads,
> and their own refs/users/ space.
> 
> This would give flexibility to smaller corporate central operations to
> offer 'triangular flow' where each dev would feel like they have their
> own 'push' repo, when in reality it's really personalised branches. As
> usual the authentication of user names being handed off elsewhere;-). It
> could avoid some of the --alternate management aspects.
> 
> It's a thought..

Yeah, and interesting thought. But it demonstrates what I said above:
you can have a central repository, and yet have triangular branches:

 feature-1 <=  origin/master
            => origin/felipec/feature-1

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Philip Oakley]

On 01/06/2021 17:35, Felipe Contreras wrote:
> Philip Oakley wrote:
>> On 01/06/2021 13:12, Felipe Contreras wrote:
>>> So it's more like:
>>>
>>>   centralized = ~decentralized
>>>   triangular = ~two-way
>>>
>>> A centralized workflow consists of a single repository where branches
>>> are typically two-way, but not necessarily.
>>>
>>> A decentralized workflow consists of multiple repositories where
>>> branches are typically triangular, but not necessarily.
>>>
>>> So the triangularity is per branch, not per repository, and same_repo
>>> means a two-way branch, could be a centralized or decentralized
>>> workflow.
>> My personal viewpoint is that triangular flow happens when you cannot
>> push to the repo you consider as upstream.
> It's not about permissions. Even if I had permissions to push to git.git,
> I wouldn't do so. I do have permission to push to some public projects, but I
> instead send patches/pull requests like everyone else.

I had it that if you don't have permissions then you definitely need to
use a Triangular flow. Hence how I was presenting the view.
>
> It's more about ownership. In my personal repositories I can push
> whatever I want, but on shared repositories I have to be more careful.

True, there are social choices that are equivalent to a type of
'permission'.
>
>> Rather you typically have a publish/backup repo instead (semi-public,
>> semi-private - few are interested ;-).
>>
>> That (can't push one way around the triangle) part of the flow is
>> separate from the distinction between patch flows and merge (Pull)
>> request flows.
> I think it's not separate, that is the thing that makes a triangular
> flow triangular: the flow of patches goes through a different repository,
> and then they get picked and merged into the upstream one.
Pull requests can do the same thing, just as in Git-for Windows..

>
>> E.g. My personal Git repo can be triangular with both git.git and
>> git-for-windows, plus a few (what I view as) fetch-only repos from other
>> collaborators/maintainers beyond the triangular 'golden' upstream repo.
>>
>> I often consider GitHub as a centraliser, but I don't think it's what is
>> being considered above.
> GitHub is all about pull requests, you fork a repository, you push your
> branch into that personal fork, and then you request a pull from
> upstream.
>
> That's triangular.

Yes.
>
>> --
>> A thought did come to mind that a Git serve/repo (typically bare) should
>> be able to offer a 'refs/users/*' space (c.f. refs/remotes used by
>> individual users) that allows a type of 'centralised' operation (almost
>> as if all the users used a common alternates repo). Users could only
>> push to their own /user refs, but could pull from the main refs/heads,
>> and their own refs/users/ space.
>>
>> This would give flexibility to smaller corporate central operations to
>> offer 'triangular flow' where each dev would feel like they have their
>> own 'push' repo, when in reality it's really personalised branches. As
>> usual the authentication of user names being handed off elsewhere;-). It
>> could avoid some of the --alternate management aspects.
>>
>> It's a thought..
> Yeah, and interesting thought. But it demonstrates what I said above:
> you can have a central repository, and yet have triangular branches:

I see triangular being about repos, rather than branches. The suggestion
about was, essentially, about managing multiple user forks on the same
server without using alternates etc. It's not fully thought through ;-)

>
>  feature-1 <=  origin/master
>             => origin/felipec/feature-1
>
> Cheers.
>
I expect to be off-line for a week or so.

Philip

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Felipe Contreras]

Philip Oakley wrote:
> On 01/06/2021 17:35, Felipe Contreras wrote:
> > Philip Oakley wrote:
> >> On 01/06/2021 13:12, Felipe Contreras wrote:
> >>> So it's more like:
> >>>
> >>>   centralized = ~decentralized
> >>>   triangular = ~two-way
> >>>
> >>> A centralized workflow consists of a single repository where branches
> >>> are typically two-way, but not necessarily.
> >>>
> >>> A decentralized workflow consists of multiple repositories where
> >>> branches are typically triangular, but not necessarily.
> >>>
> >>> So the triangularity is per branch, not per repository, and same_repo
> >>> means a two-way branch, could be a centralized or decentralized
> >>> workflow.
> >> My personal viewpoint is that triangular flow happens when you cannot
> >> push to the repo you consider as upstream.
> > It's not about permissions. Even if I had permissions to push to git.git,
> > I wouldn't do so. I do have permission to push to some public projects, but I
> > instead send patches/pull requests like everyone else.
> 
> I had it that if you don't have permissions then you definitely need to
> use a Triangular flow. Hence how I was presenting the view.

If you don't have permissions you have no option but a triangular flow.

If you are in a triangular flow that doesn't necessarily mean you don't
have permissions.

> >> A thought did come to mind that a Git serve/repo (typically bare) should
> >> be able to offer a 'refs/users/*' space (c.f. refs/remotes used by
> >> individual users) that allows a type of 'centralised' operation (almost
> >> as if all the users used a common alternates repo). Users could only
> >> push to their own /user refs, but could pull from the main refs/heads,
> >> and their own refs/users/ space.
> >>
> >> This would give flexibility to smaller corporate central operations to
> >> offer 'triangular flow' where each dev would feel like they have their
> >> own 'push' repo, when in reality it's really personalised branches. As
> >> usual the authentication of user names being handed off elsewhere;-). It
> >> could avoid some of the --alternate management aspects.
> >>
> >> It's a thought..
> > Yeah, and interesting thought. But it demonstrates what I said above:
> > you can have a central repository, and yet have triangular branches:
> 
> I see triangular being about repos, rather than branches.

If you have a feature-1 branch that fetches from origin, rebases onto
origin/master, but pushes to origin/feature-1...

Does that qualify as triangular?

-- 
Felipe Contreras

Re: [PATCH v2 6/6] doc: push: explain default=simple correctly

[Junio C Hamano]

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

> Therefore your proposal:
>
>   If you are pushing to the same repository you pull from (which is
>   typically `origin`), then you need to ...
>
> is actually better, I just had to remind myself that centralized and
> same repo are not the same thing.

Yes, that is exactly why I brought up

>> as it would avoid "the project is not centralized, but I push to my
>> own repository and pull from it---what should I do?" questions.

as a reason why we want to phrase it that way, so we are on the same
page ;-)

[PATCH 00/15] push: revamp push.default

[Felipe Contreras]

This patch series is the new second part of [1].

It borrows a few patches from v1, but it's mostly a ton of cleanups.

In theory there should be no functional changes.

There's too many changes to list them all, it's much easier to see the 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:
	}

	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/20210529071115.1908310-1-felipe.contreras@gmail.com/

Felipe Contreras (15):
  push: create new get_upstream_ref() helper
  push: return immediately in trivial switch case
  push: reorder 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: fix Yoda condition
  push: remove trivial function
  push: only get triangular when needed
  push: don't get a full remote object
  push: rename !triangular to same_remote

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

-- 
2.32.0.rc0

[PATCH 01/15] push: create new get_upstream_ref() helper

[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 0961826269..39a271bb27 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 triangular)
+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 triangular)
+{
+	const char *upstream_ref;
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+	upstream_ref = get_upstream_ref(branch, remote->name);
 	if (triangular)
 		die(_("You are pushing to remote '%s', which is not the upstream of\n"
 		      "your current branch '%s', without telling me what to push\n"
 		      "to update which remote branch."),
 		    remote->name, branch->name);
 
-	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 (!triangular) {
-		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

[PATCH 02/15] push: return immediately in trivial switch case

[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 39a271bb27..f4e919450d 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, triangular);
-		break;
+		return;
 
 	case PUSH_DEFAULT_UPSTREAM:
 		setup_push_upstream(remote, branch, triangular);
-		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

[PATCH 03/15] push: reorder switch cases

[Felipe Contreras]

We want all the cases that don't do anything with a branch first, and
then the rest.

Will help further patches.

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

diff --git a/builtin/push.c b/builtin/push.c
index f4e919450d..c19321bb9d 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -254,11 +254,19 @@ static void setup_default_push_refspecs(struct remote *remote)
 	int triangular = is_workflow_triangular(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:
+	}
+
+	switch (push_default) {
+	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		setup_push_simple(remote, branch, triangular);
@@ -271,11 +279,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

Re: [PATCH 03/15] push: reorder switch cases

[Junio C Hamano]

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

> We want all the cases that don't do anything with a branch first, and
> then the rest.
>
> Will help further patches.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/push.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/push.c b/builtin/push.c
> index f4e919450d..c19321bb9d 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -254,11 +254,19 @@ static void setup_default_push_refspecs(struct remote *remote)
>  	int triangular = is_workflow_triangular(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:
> +	}
> +
> +	switch (push_default) {
> +	default:

This is not quite "reorder" but split into two.  It is not yet clear
how it helps, but hopefully we'll find out why splitting the switch
into two switches is a good idea soon in a later step, so the title
needs updating to sell that aspect of the change (unless it is a
pointless change made by mistake that the originally-single switch
got split into two, but that is unlikely the case ;-).

>  	case PUSH_DEFAULT_UNSPECIFIED:
>  	case PUSH_DEFAULT_SIMPLE:
>  		setup_push_simple(remote, branch, triangular);
> @@ -271,11 +279,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;
>  	}
>  }

Re: [PATCH 03/15] push: reorder switch cases

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > We want all the cases that don't do anything with a branch first, and
> > then the rest.
> >
> > Will help further patches.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >  builtin/push.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/builtin/push.c b/builtin/push.c
> > index f4e919450d..c19321bb9d 100644
> > --- a/builtin/push.c
> > +++ b/builtin/push.c
> > @@ -254,11 +254,19 @@ static void setup_default_push_refspecs(struct remote *remote)
> >  	int triangular = is_workflow_triangular(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:
> > +	}
> > +
> > +	switch (push_default) {
> > +	default:
> 
> This is not quite "reorder" but split into two.

OK.

> It is not yet clear
> how it helps, but hopefully we'll find out why splitting the switch
> into two switches is a good idea soon in a later step, so the title
> needs updating to sell that aspect of the change

How about: By splitting the two kinds of modes we can return sooner in
the case the mode doesn't need to do anything else, and for the rest
that reuse quite a bit of code it can be set up after the first switch.

-- 
Felipe Contreras

[PATCH 04/15] push: factor out null branch check

[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 c19321bb9d..a2abacf64d 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 triangular)
 {
 	const char *upstream_ref;
-	if (!branch)
-		die(_(message_detached_head_die), remote->name);
 	upstream_ref = get_upstream_ref(branch, remote->name);
 	if (triangular)
 		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 triangular)
 {
-	if (!branch)
-		die(_(message_detached_head_die), remote->name);
-
 	if (!triangular) {
 		const char *upstream_ref;
 
@@ -265,6 +258,9 @@ static void setup_default_push_refspecs(struct remote *remote)
 	default:
 	}
 
+	if (!branch)
+		die(_(message_detached_head_die), remote->name);
+
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
-- 
2.32.0.rc0

[PATCH 05/15] push: only get the branch when needed

[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 a2abacf64d..4b3a14278a 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -243,7 +243,7 @@ static int is_workflow_triangular(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);
 
 	switch (push_default) {
@@ -258,6 +258,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 	default:
 	}
 
+	branch = branch_get(NULL);
 	if (!branch)
 		die(_(message_detached_head_die), remote->name);
 
-- 
2.32.0.rc0

Re: [PATCH 05/15] push: only get the branch when needed

[Junio C Hamano]

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

> 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 a2abacf64d..4b3a14278a 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -243,7 +243,7 @@ static int is_workflow_triangular(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);
>  
>  	switch (push_default) {
> @@ -258,6 +258,7 @@ static void setup_default_push_refspecs(struct remote *remote)
>  	default:

Not a fault of this step, but please make it a habit to have
"break;" here.  case label with absolutely no body just looks
strange and is distracting to the eyes.

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

This step is the true justification for the splitting of a single
switch into two switches done in [03/15].  Makes quite a lot of
sense.

Re: [PATCH 05/15] push: only get the branch when needed

[Felipe Contreras]

Junio C Hamano wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
> 
> > 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 a2abacf64d..4b3a14278a 100644
> > --- a/builtin/push.c
> > +++ b/builtin/push.c
> > @@ -243,7 +243,7 @@ static int is_workflow_triangular(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);
> >  
> >  	switch (push_default) {
> > @@ -258,6 +258,7 @@ static void setup_default_push_refspecs(struct remote *remote)
> >  	default:
> 
> Not a fault of this step, but please make it a habit to have
> "break;" here.  case label with absolutely no body just looks
> strange and is distracting to the eyes.

All right.

> >  	}
> >  
> > +	branch = branch_get(NULL);
> >  	if (!branch)
> >  		die(_(message_detached_head_die), remote->name);
> 
> This step is the true justification for the splitting of a single
> switch into two switches done in [03/15].  Makes quite a lot of
> sense.

It is one, not the only one. But yeah, the most readily apparent.

-- 
Felipe Contreras

[PATCH 06/15] push: make setup_push_* return the dst

[Felipe Contreras]

All of the setup_push_* functions are appending a refspec. Do this only
once in 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 4b3a14278a..23b1908a5c 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 triangular)
+static const char *setup_push_upstream(struct remote *remote, struct branch *branch,
+	int triangular)
 {
 	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 triangular)
+static const char *setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
 {
 	if (!triangular) {
 		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_workflow_triangular(struct remote *remote)
@@ -245,6 +244,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch;
 	int triangular = is_workflow_triangular(remote);
+	const char *dst;
 
 	switch (push_default) {
 	case PUSH_DEFAULT_MATCHING:
@@ -266,17 +266,19 @@ static void setup_default_push_refspecs(struct remote *remote)
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		setup_push_simple(remote, branch, triangular);
-		return;
+		dst = setup_push_simple(remote, branch, triangular);
+		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		setup_push_upstream(remote, branch, triangular);
-		return;
+		dst = setup_push_upstream(remote, branch, triangular);
+		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

[PATCH 07/15] push: trivial simplifications

[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 23b1908a5c..21968abf6e 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 triangular)
 {
-	const char *upstream_ref;
-	upstream_ref = get_upstream_ref(branch, remote->name);
 	if (triangular)
 		die(_("You are pushing to remote '%s', which is not the upstream of\n"
 		      "your current branch '%s', without telling me what to push\n"
 		      "to update which remote branch."),
 		    remote->name, branch->name);
-	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 triangular)
 {
-	if (!triangular) {
-		const char *upstream_ref;
-
-		upstream_ref = get_upstream_ref(branch, remote->name);
-
-		/* Additional safety */
-		if (strcmp(branch->refname, upstream_ref))
+	if (!triangular)
+		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
 			die_push_simple(branch, remote);
-	}
 	return branch->refname;
 }
 
-- 
2.32.0.rc0

[PATCH 08/15] push: get rid of all the setup_push_* functions

[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 21968abf6e..dbb4f78e61 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 triangular)
-{
-	if (triangular)
-		die(_("You are pushing to remote '%s', which is not the upstream of\n"
-		      "your current branch '%s', without telling me what to push\n"
-		      "to update which remote branch."),
-		    remote->name, branch->name);
-	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 triangular)
-{
-	if (!triangular)
-		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)
 {
 	struct remote *fetch_remote = remote_get(NULL);
@@ -258,15 +234,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, triangular);
+		if (!triangular)
+			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)
+			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

Re: [PATCH 08/15] push: get rid of all the setup_push_* functions

[Junio C Hamano]

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

> 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(-)

Finally, this resolves the risk of deliberately duplicated code of
diverging, which was made in the earlier series.  Without seeing
this step, it has been dubious if the earlier change was a good one,
but with this step, it is a very clear win.

Nicely done.

> diff --git a/builtin/push.c b/builtin/push.c
> index 21968abf6e..dbb4f78e61 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 triangular)
> -{
> -	if (triangular)
> -		die(_("You are pushing to remote '%s', which is not the upstream of\n"
> -		      "your current branch '%s', without telling me what to push\n"
> -		      "to update which remote branch."),
> -		    remote->name, branch->name);
> -	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 triangular)
> -{
> -	if (!triangular)
> -		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)
>  {
>  	struct remote *fetch_remote = remote_get(NULL);
> @@ -258,15 +234,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, triangular);
> +		if (!triangular)
> +			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)
> +			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;
>  	}

[PATCH 09/15] push: factor out the typical case

[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 dbb4f78e61..94cb2eda63 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -230,14 +230,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 (!triangular)
-			if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
-				die_push_simple(branch, remote);
-		dst = branch->refname;
+		if (triangular)
+			break;
+		if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
+			die_push_simple(branch, remote);
 		break;
 
 	case PUSH_DEFAULT_UPSTREAM:
@@ -250,7 +252,6 @@ static void setup_default_push_refspecs(struct remote *remote)
 		break;
 
 	case PUSH_DEFAULT_CURRENT:
-		dst = branch->refname;
 		break;
 	}
 
-- 
2.32.0.rc0

[PATCH 10/15] push: remove redundant check

[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.

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 94cb2eda63..7485522807 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_workflow_triangular(struct remote *remote)
 {
-	struct remote *fetch_remote = remote_get(NULL);
-	return (fetch_remote && fetch_remote != remote);
+	return remote_get(NULL) != remote;
 }
 
 static void setup_default_push_refspecs(struct remote *remote)
-- 
2.32.0.rc0

[PATCH 11/15] push: fix Yoda condition

[Felipe Contreras]

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7485522807..468ccc1067 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -204,7 +204,7 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na
 
 static int is_workflow_triangular(struct remote *remote)
 {
-	return remote_get(NULL) != remote;
+	return remote != remote_get(NULL);
 }
 
 static void setup_default_push_refspecs(struct remote *remote)
-- 
2.32.0.rc0

Re: [PATCH 11/15] push: fix Yoda condition

[Junio C Hamano]

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

> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

This is 10/15's making, and squashing them together will make both
the entire series (because doing so means that the reviewers need to
see one fewer step) and 10/15 (because the end result of the conversion
is more "logical") easier to review (and later read in "git log -p").

>
> diff --git a/builtin/push.c b/builtin/push.c
> index 7485522807..468ccc1067 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -204,7 +204,7 @@ static const char *get_upstream_ref(struct branch *branch, const char *remote_na
>  
>  static int is_workflow_triangular(struct remote *remote)
>  {
> -	return remote_get(NULL) != remote;
> +	return remote != remote_get(NULL);
>  }
>  
>  static void setup_default_push_refspecs(struct remote *remote)

[PATCH 12/15] push: remove trivial function

[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 468ccc1067..c220f70795 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_workflow_triangular(struct remote *remote)
-{
-	return remote != remote_get(NULL);
-}
-
 static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch;
-	int triangular = is_workflow_triangular(remote);
+	int triangular = remote != remote_get(NULL);
 	const char *dst;
 
 	switch (push_default) {
-- 
2.32.0.rc0

[PATCH 13/15] push: only get triangular when needed

[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 c220f70795..818ca4c5c9 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 triangular = remote != remote_get(NULL);
 	const char *dst;
+	int triangular;
 
 	switch (push_default) {
 	case PUSH_DEFAULT_MATCHING:
@@ -225,6 +225,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 		die(_(message_detached_head_die), remote->name);
 
 	dst = branch->refname;
+	triangular = remote != remote_get(NULL);
 
 	switch (push_default) {
 	default:
-- 
2.32.0.rc0

Re: [PATCH 13/15] push: only get triangular when needed

[Junio C Hamano]

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

> 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 c220f70795..818ca4c5c9 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 triangular = remote != remote_get(NULL);
>  	const char *dst;
> +	int triangular;
>  
>  	switch (push_default) {
>  	case PUSH_DEFAULT_MATCHING:
> @@ -225,6 +225,7 @@ static void setup_default_push_refspecs(struct remote *remote)
>  		die(_(message_detached_head_die), remote->name);
>  
>  	dst = branch->refname;
> +	triangular = remote != remote_get(NULL);
>  
>  	switch (push_default) {
>  	default:

This is another change that got helped by an earlier split.
Good.

[PATCH 14/15] push: don't get a full remote object

[Felipe Contreras]

All we need to know is that their names are different.

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 818ca4c5c9..2f30a97b97 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -225,7 +225,7 @@ 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));
 
 	switch (push_default) {
 	default:
-- 
2.32.0.rc0

[PATCH 15/15] push: rename !triangular to same_remote

[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.

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

diff --git a/builtin/push.c b/builtin/push.c
index 2f30a97b97..f1ac531252 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -206,7 +206,7 @@ static void setup_default_push_refspecs(struct remote *remote)
 {
 	struct branch *branch;
 	const char *dst;
-	int triangular;
+	int same_remote;
 
 	switch (push_default) {
 	case PUSH_DEFAULT_MATCHING:
@@ -225,20 +225,20 @@ static void setup_default_push_refspecs(struct remote *remote)
 		die(_(message_detached_head_die), remote->name);
 
 	dst = branch->refname;
-	triangular = strcmp(remote->name, remote_for_branch(branch, NULL));
+	same_remote = !strcmp(remote->name, remote_for_branch(branch, NULL));
 
 	switch (push_default) {
 	default:
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
-		if (triangular)
+		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 (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."),
-- 
2.32.0.rc0

Re: [PATCH 15/15] push: rename !triangular to same_remote

[Junio C Hamano]

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

> 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.

Yup.  Avoiding the phrase "centralized" and using "same_remote"
makes quite a lot of sense, too.

Overall the end-result of the series is quite pleasant read, even
though some intermediate states in the middle risked readers to
worry about "are we going in the right direction?"

Well done.

Re: [PATCH v2 0/6] Unconvolutize push.default=simple

[Philip Oakley]

On 29/05/2021 08:11, Felipe Contreras wrote:
> 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.

Not your problem, but I do note, as a general point, that we don't
explain the different variants of Triangular workflow anywhere [just two
mentions in gitrevisions.txt] (e.g. patch flow versus server-side
merges, etc.). 


Philip
>
> This is basically the same as v1, except I removed a bunch of patches which are now part of a
> different series. Also, I updated some commit messages with suggestions from Elijah Newren.
>
> The result of this series is in short this function:
>
> static void setup_push_simple(struct remote *remote, struct branch *branch, int triangular)
> {
> 	if (!branch)
> 		die(_(message_detached_head_die), remote->name);
>
> 	if (!triangular) {
> 		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 (6):
>   push: hedge code of default=simple
>   push: move 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                | 40 ++++++++++++++++++++++++-----------
>  2 files changed, 34 insertions(+), 19 deletions(-)
>
> Range-diff against v1:
>  1:  2856711eb3 !  1:  f1f42bda32 push: hedge code of default=simple
>     @@ Commit message
>          `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 ##
>  2:  33acb09e82 !  2:  bb6d810011 push: move code to setup_push_simple()
>     @@ Commit message
>      
>          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 ##
>  3:  de1b621b7e !  3:  d66a442fba push: reorganize setup_push_simple()
>     @@ Metadata
>       ## Commit message ##
>          push: reorganize setup_push_simple()
>      
>     -    Simply move the code around.
>     +    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.
>      
>          No functional changes.
>      
>     +    Suggestions-by: Elijah Newren <newren@gmail.com>
>          Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>      
>       ## builtin/push.c ##
>  4:  83efcad143 !  4:  eaae6a826a push: simplify setup_push_simple()
>     @@ Metadata
>       ## Commit message ##
>          push: simplify setup_push_simple()
>      
>     -    branch->refname can never be different from branch->merge[0]->src.
>     +    There's a safety check to make sure branch->refname is not 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 ##
>  5:  d7489e9545 =  5:  8d9ae5e552 push: remove unused code in setup_push_upstream()
>  6:  e693341403 <  -:  ---------- push: merge current and simple
>  7:  830a57c867 <  -:  ---------- push: remove redundant check
>  8:  d2dded5998 <  -:  ---------- push: fix Yoda condition
>  9:  7528738091 <  -:  ---------- push: remove trivial function
> 10:  1ae0546df6 <  -:  ---------- push: flip !triangular for centralized
> 11:  3acd42e385 !  6:  b35bdf14dc doc: push: explain default=simple correctly
>     @@ Metadata
>       ## Commit message ##
>          doc: push: explain default=simple correctly
>      
>     -    Now that the code has been unconvolutized and it's clear what it's
>     +    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

Re: [PATCH v2 0/6] Unconvolutize push.default=simple

[Felipe Contreras]

Philip Oakley wrote:
> On 29/05/2021 08:11, Felipe Contreras wrote:
> > 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.
> 
> Not your problem, but I do note, as a general point, that we don't
> explain the different variants of Triangular workflow anywhere [just two
> mentions in gitrevisions.txt] (e.g. patch flow versus server-side
> merges, etc.). 

All my documentation improvements get stuck in tar, so somebody else
would need to do that.

If and when they do, it will become clear the big pit in functionality
there is.

-- 
Felipe Contreras

Re: [PATCH v2 0/6] Unconvolutize push.default=simple

[Ævar Arnfjörð Bjarmason]

On Sat, May 29 2021, Felipe Contreras wrote:

> 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.

I'd find the end-state even more readable if this "triangular" wasn't
passed as a parameter but we just did the top-level dispatch based on
that, i.e. "simple" is really internally SIMPLE_SIMPLE and
SIMPLE_TRIANGULAR, why not dispatch on that? Our internal enum doesn't
need to 1=1 map to the config setting.

Part of that's that I prefer reading code in the current "master" which
is if -> die -> most of the rest of function, v.s. your end-state of if
-> stuff -> else -> most of the function being indented to that "else".

Re: [PATCH v2 0/6] Unconvolutize push.default=simple

[Felipe Contreras]

Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, May 29 2021, Felipe Contreras wrote:
> 
> > 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.
> 
> I'd find the end-state even more readable if this "triangular" wasn't
> passed as a parameter but we just did the top-level dispatch based on
> that, i.e. "simple" is really internally SIMPLE_SIMPLE and
> SIMPLE_TRIANGULAR, why not dispatch on that? Our internal enum doesn't
> need to 1=1 map to the config setting.

Yes, but where are you going to make sure you are in SIMPLE, and not in
CURRENT? Surely you are not suggesting to modify
git_default_push_config(), which is pretty straightforward. So we would
need yet another enum.

I don't think there's a need for that, as you can see on the patch
series on top of this, there's not even a need to dispatch anything.

> Part of that's that I prefer reading code in the current "master" which
> is if -> die -> most of the rest of function, v.s. your end-state of if
> -> stuff -> else -> most of the function being indented to that "else".

I prefer that as well, but unfortunately in this case we need to do
something after the checks to die(). I could add a goto, but for just
one conditional seems like it's not worth it.

Either way this is a temporary thing, because in the next patch series
the code becomes pretty quickly:

  if (!triangular && strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
          die_push_simple(branch, remote);

And then:

  if (triangular)
    break;
  if (strcmp(branch->refname, get_upstream_ref(branch, remote->name)))
    die_push_simple(branch, remote);

Cheers.

-- 
Felipe Contreras