v2, 6 partspush: hedge code of default=simple

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

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

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

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

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

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

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

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

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

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

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

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

>> 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]?

> 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

> 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]
> 
> That seems to be a different issue. Related, but not quite the same.

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

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

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

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

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

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

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

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

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

> 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;
>  	}
>  }

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

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

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

> 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;
>  	}

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

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

> 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

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

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