[PATCH 1/1] remote.c: fix handling of push:remote_ref

[PATCH 1/1] remote.c: fix handling of push:remote_ref

[Damien Robert]

To get the meaning of push:remoteref, ref-filter.c calls
remote_ref_for_branch.

However remote_ref_for_branch only handles the case of a specified refspec.
The other cases treated by branch_get_push_1 are the mirror case,
PUSH_DEFAULT_{NOTHING,MATCHING,CURRENT,UPSTREAM,UNSPECIFIED,SIMPLE}.

In all these cases, either there is no push remote, or the remote_ref is
branch->refname. So we can handle all these cases by returning
branch->refname, provided that remote is not empty.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 remote.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/remote.c b/remote.c
index 593ce297ed..75e42b1e36 100644
--- a/remote.c
+++ b/remote.c
@@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
 					*explicit = 1;
 				return dst;
 			}
+			else if (remote) {
+				if (explicit)
+					*explicit = 1;
+				return branch->refname;
+			}
 		}
 	}
 	if (explicit)
-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Jeff King]

On Fri, Feb 28, 2020 at 06:24:55PM +0100, Damien Robert wrote:

> To get the meaning of push:remoteref, ref-filter.c calls
> remote_ref_for_branch.
> 
> However remote_ref_for_branch only handles the case of a specified refspec.
> The other cases treated by branch_get_push_1 are the mirror case,
> PUSH_DEFAULT_{NOTHING,MATCHING,CURRENT,UPSTREAM,UNSPECIFIED,SIMPLE}.

Just to back up a minute to the user-visible problem, it's that:

  git config push.default matching
  git for-each-ref --format='%(push)'
  git for-each-ref --format='%(push:remoteref)'

prints a useful tracking ref for the first for-each-ref, but an empty
string for the second. That feature (and remote_ref_for_branch) come
from 9700fae5ee (for-each-ref: let upstream/push report the remote ref
name, 2017-11-07). Author cc'd for guidance.

I wonder if %(upstream:remoteref) has similar problems, but I suppose
not (it doesn't have this implicit config, so we'd always either have a
remote ref or we'd have no upstream at all).

> In all these cases, either there is no push remote, or the remote_ref is
> branch->refname. So we can handle all these cases by returning
> branch->refname, provided that remote is not empty.

In the case of "upstream", the names could be different, couldn't they?

If I do this:

  git init parent
  git -C parent commit --allow-empty -m foo
  
  git clone parent child
  cd child
  git branch --track mybranch origin/master
  git config push.default upstream
  git for-each-ref \
    --format='push=%(push), remoteref=%(push:remoteref)' \
    refs/heads/mybranch

the current code gives no remoteref value, which seems wrong. But with
your patch I'd get "refs/heads/mybranch", which is also wrong.

I think you're right that all of the other cases would always use the
same refname on the remote.

>  remote.c | 5 +++++
>  1 file changed, 5 insertions(+)

We'd want some test coverage to make sure this doesn't regress. There
are already some tests covering this feature in t6300. And indeed, your
patch causes them to fail when checking a "simple" push case (but I
think I'd argue the current expected value there is wrong). That should
be expanded to cover the "upstream" case, too, once we figure out how to
get it right.

> diff --git a/remote.c b/remote.c
> index 593ce297ed..75e42b1e36 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
>  					*explicit = 1;
>  				return dst;
>  			}
> +			else if (remote) {
> +				if (explicit)
> +					*explicit = 1;
> +				return branch->refname;
> +			}

Saying "*explicit = 1" here seems weird. Isn't the whole point that
these modes _aren't_ explicit?

It looks like our only caller will ignore our return value unless we say
"explicit", though. I have to wonder what the point of that flag is,
versus just returning NULL when we don't have anything to return.

-Peff

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Damien Robert]

First I apologize for sending the patch too soon, I forgot to put the RFC
flag, this is not a complete patch, and I do intend to fix the tests. I was
aware of this issue since my patch about push:track, but since I don't use
push:remoteref this was a low priority to fix. Last friday I was sick and
could not work, so I took the opportunity to scratch this itch.

From Jeff King, Fri 28 Feb 2020 at 13:23:49 (-0500) :
> Just to back up a minute to the user-visible problem, it's that:
>   git config push.default matching
>   git for-each-ref --format='%(push)'
>   git for-each-ref --format='%(push:remoteref)'
> prints a useful tracking ref for the first for-each-ref, but an empty
> string for the second. That feature (and remote_ref_for_branch) come
> from 9700fae5ee (for-each-ref: let upstream/push report the remote ref
> name, 2017-11-07). Author cc'd for guidance.

Yes exactly.
 
> I wonder if %(upstream:remoteref) has similar problems, but I suppose
> not (it doesn't have this implicit config, so we'd always either have a
> remote ref or we'd have no upstream at all).

And the code is different. upstream:remoteref uses branch->merge_name[0].
This is due to the fact that the branch struct stores different things for
merge branches than for push branches (which hurt my sense of symmetry :)).

> > In all these cases, either there is no push remote, or the remote_ref is
> > branch->refname. So we can handle all these cases by returning
> > branch->refname, provided that remote is not empty.
> In the case of "upstream", the names could be different, couldn't they?

Yes of course. Moreover there is also the case of 'nothing' where we should
not return the branch name (so what we should test for in the other cases
is not the existence of `remote` but of `branch->push_remote_ref`.)

> We'd want some test coverage to make sure this doesn't regress. There
> are already some tests covering this feature in t6300. And indeed, your
> patch causes them to fail when checking a "simple" push case (but I
> think I'd argue the current expected value there is wrong). That should
> be expanded to cover the "upstream" case, too, once we figure out how to
> get it right.

Yes, I'll do both simple and upstream for tests I think.

> > diff --git a/remote.c b/remote.c
> > index 593ce297ed..75e42b1e36 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -538,6 +538,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
> >  					*explicit = 1;
> >  				return dst;
> >  			}
> > +			else if (remote) {
> > +				if (explicit)
> > +					*explicit = 1;
> > +				return branch->refname;
> > +			}
> 
> Saying "*explicit = 1" here seems weird. Isn't the whole point that
> these modes _aren't_ explicit?

Well pushremote_for_branch also set explicit=1 if only remote.pushDefault
is set, so I followed suit.

> It looks like our only caller will ignore our return value unless we say
> "explicit", though. I have to wonder what the point of that flag is,
> versus just returning NULL when we don't have anything to return.

I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the
situation is handled by RR_REMOTE_REF, where explicit is not used at all.
So we could remove it.


So it remains the problem of handling the 'upstream' case.
The ideal solution would be to not duplicate branch_get_push_1.
In most of the case, this function finds `dst` which is exactly the
push:remoteref we are looking for. 

Then branch_get_push_1 uses
		ret = tracking_for_push_dest(remote, dst, err);
which simply calls
	ret = apply_refspecs(&remote->fetch, dst);

The only different case is
	case PUSH_DEFAULT_UPSTREAM:
		return branch_get_upstream(branch, err);
which returns
	branch->merge[0]->dst

So we could almost write an auxiliary function that returns push:remoteref
and use it both in remote_ref_for_branch and branch_get_push_1 (via a
further call to tracking_for_push_dest) except for the 'upstream' case
which is subtly different.

In the 'upstream' case, the auxiliary function would return
branch->merge_name[0]. So the question is: can
tracking_for_push_dest(branch->merge_name[0]) be different from
branch->merge[0]->dst?

Now branch->merge is set in `set_merge`, where it is constructed via
		if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
			     &oid, &ref) == 1)
And I don't understand dwim_ref enough to know if there could be
differences in our setting from tracking_for_push_dest in corner cases.


Another solution could be as follow: we already store `push` in
`branch->push_tracking_ref`. So the question is: can we always easily convert
something like refs/remotes/origin/branch_name to refs/heads/branch_name
(ie essentially reverse ètracking_for_push_dest`), or are there corner cases?


Otherwise a simple but not elegant solution would be to copy paste the
code of branch_get_push_1 to remote_ref_for_branch, simply removing the
calls to `tracking_for_push_dest` and using remote->branch_name[0] rather
than remote->branch[0]->dst for the upstream case.



-- 
Damien Robert
http://www.normalesup.org/~robert/pro

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Jeff King]

[dropping J from cc, since my earlier email bounced]

On Sun, Mar 01, 2020 at 11:05:31PM +0100, Damien Robert wrote:

> > Saying "*explicit = 1" here seems weird. Isn't the whole point that
> > these modes _aren't_ explicit?
> 
> Well pushremote_for_branch also set explicit=1 if only remote.pushDefault
> is set, so I followed suit.

Yeah, I think the useless "explicit" was a mistake back when the
function was added. See the patch below.

> > It looks like our only caller will ignore our return value unless we say
> > "explicit", though. I have to wonder what the point of that flag is,
> > versus just returning NULL when we don't have anything to return.
> 
> I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the
> situation is handled by RR_REMOTE_REF, where explicit is not used at all.
> So we could remove it.

We do look at it, but it's pointless to do so:

  $ git grep -hn -C4 remote_ref_for_branch origin:ref-filter.c
  1461-	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
  1462-		int explicit;
  1463-		const char *merge;
  1464-
  1465:		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
  1466-					      &explicit);
  1467-		*s = xstrdup(explicit ? merge : "");
  1468-	} else
  1469-		BUG("unhandled RR_* enum");

I think we probably ought to do this as a preparatory patch in your
series.

-- >8 --
Subject: remote: drop "explicit" parameter from remote_ref_for_branch()

Commit 9700fae5ee (for-each-ref: let upstream/push report the remote ref
name, 2017-11-07) added a remote_ref_for_branch() helper, which is
modeled after remote_for_branch(). This includes providing an "explicit"
out-parameter that tells the caller whether the remote was configured by
the user, or whether we picked a default name like "origin".

But unlike remote names, there's no default case for the remote branch
name. In any case where we don't set "explicit", we'd just an empty
string anyway. Let's instead return NULL in this case, letting us
simplify the function interface.

Signed-off-by: Jeff King <peff@peff.net>
---
 ref-filter.c |  6 ++----
 remote.c     | 11 ++---------
 remote.h     |  3 +--
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6867e33648..9837700732 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1459,12 +1459,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			remote_for_branch(branch, &explicit);
 		*s = xstrdup(explicit ? remote : "");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
-		int explicit;
 		const char *merge;
 
-		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
-					      &explicit);
-		*s = xstrdup(explicit ? merge : "");
+		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push);
+		*s = xstrdup(merge ? merge : "");
 	} else
 		BUG("unhandled RR_* enum");
 }
diff --git a/remote.c b/remote.c
index 593ce297ed..c43196ec06 100644
--- a/remote.c
+++ b/remote.c
@@ -516,14 +516,11 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push,
-				  int *explicit)
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
 {
 	if (branch) {
 		if (!for_push) {
 			if (branch->merge_nr) {
-				if (explicit)
-					*explicit = 1;
 				return branch->merge_name[0];
 			}
 		} else {
@@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
 						  branch->refname))) {
-				if (explicit)
-					*explicit = 1;
 				return dst;
 			}
 		}
 	}
-	if (explicit)
-		*explicit = 0;
-	return "";
+	return NULL;
 }
 
 static struct remote *remote_get_1(const char *name,
diff --git a/remote.h b/remote.h
index b134cc21be..11d8719b58 100644
--- a/remote.h
+++ b/remote.h
@@ -261,8 +261,7 @@ struct branch {
 struct branch *branch_get(const char *name);
 const char *remote_for_branch(struct branch *branch, int *explicit);
 const char *pushremote_for_branch(struct branch *branch, int *explicit);
-const char *remote_ref_for_branch(struct branch *branch, int for_push,
-				  int *explicit);
+const char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
-- 
2.25.1.947.ga5bc3d07fe

[PATCH v2 0/2]

[Damien Robert]

Here is the version 2. I incorporated Jeff's preliminary patch, and handled
all push.default cases and added tests for them.

Damien Robert (1):
  remote.c: fix handling of %(push:remoteref)

Jeff King (1):
  remote: drop "explicit" parameter from remote_ref_for_branch()

 ref-filter.c            |   6 +--
 remote.c                | 113 +++++++++++++++++++++++++++++-----------
 remote.h                |   3 +-
 t/t6300-for-each-ref.sh |  29 ++++++++++-
 4 files changed, 115 insertions(+), 36 deletions(-)

-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

[PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()

[Damien Robert]

From: Jeff King <peff@peff.net>

Commit 9700fae5ee (for-each-ref: let upstream/push report the remote ref
name, 2017-11-07) added a remote_ref_for_branch() helper, which is
modeled after remote_for_branch(). This includes providing an "explicit"
out-parameter that tells the caller whether the remote was configured by
the user, or whether we picked a default name like "origin".

But unlike remote names, there's no default case for the remote branch
name. In any case where we don't set "explicit", we'd just an empty
string anyway. Let's instead return NULL in this case, letting us
simplify the function interface.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 ref-filter.c |  6 ++----
 remote.c     | 11 ++---------
 remote.h     |  3 +--
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index 6867e33648..9837700732 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1459,12 +1459,10 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			remote_for_branch(branch, &explicit);
 		*s = xstrdup(explicit ? remote : "");
 	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
-		int explicit;
 		const char *merge;
 
-		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
-					      &explicit);
-		*s = xstrdup(explicit ? merge : "");
+		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push);
+		*s = xstrdup(merge ? merge : "");
 	} else
 		BUG("unhandled RR_* enum");
 }
diff --git a/remote.c b/remote.c
index 593ce297ed..c43196ec06 100644
--- a/remote.c
+++ b/remote.c
@@ -516,14 +516,11 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push,
-				  int *explicit)
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
 {
 	if (branch) {
 		if (!for_push) {
 			if (branch->merge_nr) {
-				if (explicit)
-					*explicit = 1;
 				return branch->merge_name[0];
 			}
 		} else {
@@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
 			if (remote && remote->push.nr &&
 			    (dst = apply_refspecs(&remote->push,
 						  branch->refname))) {
-				if (explicit)
-					*explicit = 1;
 				return dst;
 			}
 		}
 	}
-	if (explicit)
-		*explicit = 0;
-	return "";
+	return NULL;
 }
 
 static struct remote *remote_get_1(const char *name,
diff --git a/remote.h b/remote.h
index b134cc21be..11d8719b58 100644
--- a/remote.h
+++ b/remote.h
@@ -261,8 +261,7 @@ struct branch {
 struct branch *branch_get(const char *name);
 const char *remote_for_branch(struct branch *branch, int *explicit);
 const char *pushremote_for_branch(struct branch *branch, int *explicit);
-const char *remote_ref_for_branch(struct branch *branch, int for_push,
-				  int *explicit);
+const char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

> From: Jeff King <peff@peff.net>
>
> Commit 9700fae5ee (for-each-ref: let upstream/push report the remote ref
> name, 2017-11-07) added a remote_ref_for_branch() helper, which is
> modeled after remote_for_branch(). This includes providing an "explicit"
> out-parameter that tells the caller whether the remote was configured by
> the user, or whether we picked a default name like "origin".
>
> But unlike remote names, there's no default case for the remote branch
> name.

Up to this point, it is well written and easy to read.  I think
"there is no case where a default name for the remoate branch is
used" would be even more easy to read.

In any case, if there is no case that default name, I understand
that explicit is always set to 1?

> In any case where we don't set "explicit", we'd just an empty
> string anyway.

Sorry, but I cannot parse this.  But earlier, you established that
there is no case that a default is used, so is there any case where
we don't set "explicit"?  I don't get it.

> Let's instead return NULL in this case, letting us
> simplify the function interface.

>  	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
> -		int explicit;
>  		const char *merge;
>  
> -		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
> -					      &explicit);
> -		*s = xstrdup(explicit ? merge : "");
> +		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push);
> +		*s = xstrdup(merge ? merge : "");

Mental note: as long as explicit is set to true when the function
returns a non-null value, this change will be a no-op as far as the
result is concerned, which is a good thing.

> diff --git a/remote.c b/remote.c
> index 593ce297ed..c43196ec06 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -516,14 +516,11 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
>  	return remote_for_branch(branch, explicit);
>  }
>  
> -const char *remote_ref_for_branch(struct branch *branch, int for_push,
> -				  int *explicit)
> +const char *remote_ref_for_branch(struct branch *branch, int for_push)
>  {
>  	if (branch) {
>  		if (!for_push) {
>  			if (branch->merge_nr) {
> -				if (explicit)
> -					*explicit = 1;
>  				return branch->merge_name[0];

This case returns a non-NULL pointer, and used to set explicit to
true.

>  			}
>  		} else {
> @@ -534,15 +531,11 @@ const char *remote_ref_for_branch(struct branch *branch, int for_push,
>  			if (remote && remote->push.nr &&
>  			    (dst = apply_refspecs(&remote->push,
>  						  branch->refname))) {
> -				if (explicit)
> -					*explicit = 1;
>  				return dst;

So did this.

>  			}
>  		}
>  	}
> -	if (explicit)
> -		*explicit = 0;
> -	return "";
> +	return NULL;

And this one used to return 0 in explicit and returned an empty
string.  The only caller ignored that empty string so returning NULL
in the new code does not make a difference, which also is a good
thing.

So the code looks correctly done.  I just don't get the feeling that
the proposed log message explains the change clearly to readers.

After reading the code through, I think "there's no default case"
was what caused my confusion.

    But unlike remote names, there is no default name when the user
    didn't configure one.  The only way the "explicit" parameter is
    used by the caller is to use the value returned from the helper
    when it is set, and use an empty string otherwise, ignoring the
    returned value from the helper.

    Let's drop the "explicit" out-parameter, and return NULL when
    the returned value from the helper should be ignored, to
    simplify the function interface.

perhaps?

Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()

[Jeff King]

On Tue, Mar 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:

> > But unlike remote names, there's no default case for the remote branch
> > name.
> 
> Up to this point, it is well written and easy to read.  I think
> "there is no case where a default name for the remoate branch is
> used" would be even more easy to read.
> 
> In any case, if there is no case that default name, I understand
> that explicit is always set to 1?
> 
> > In any case where we don't set "explicit", we'd just an empty
> > string anyway.
> 
> Sorry, but I cannot parse this.  But earlier, you established that
> there is no case that a default is used, so is there any case where
> we don't set "explicit"?  I don't get it.

Maybe more clear:

For remote names, we will always return one of two things:

  - a remote name based on user config, in which case explicit=1

  - the default string "origin", in which case explicit=0

For remote branches, we will return either:

  - the remote branch name from config, in which case explicit=1

  - the empty string, in which case explicit=0

But nobody ever looks at that empty string. For the second case, we
could just as well return NULL. At which point we don't need an explicit
flag at all, as the caller can just check for NULL.

(written before reading what you wrote below)

> After reading the code through, I think "there's no default case"
> was what caused my confusion.
> 
>     But unlike remote names, there is no default name when the user
>     didn't configure one.  The only way the "explicit" parameter is
>     used by the caller is to use the value returned from the helper
>     when it is set, and use an empty string otherwise, ignoring the
>     returned value from the helper.
> 
>     Let's drop the "explicit" out-parameter, and return NULL when
>     the returned value from the helper should be ignored, to
>     simplify the function interface.

Yes, that looks fine to me.

-Peff

Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 03, 2020 at 09:51:07AM -0800, Junio C Hamano wrote:
>
>> > But unlike remote names, there's no default case for the remote branch
>> > name.
>> 
>> Up to this point, it is well written and easy to read.  I think
>> "there is no case where a default name for the remoate branch is
>> used" would be even more easy to read.
>> 
>> In any case, if there is no case that default name, I understand
>> that explicit is always set to 1?
>> 
>> > In any case where we don't set "explicit", we'd just an empty
>> > string anyway.
>> 
>> Sorry, but I cannot parse this.  But earlier, you established that
>> there is no case that a default is used, so is there any case where
>> we don't set "explicit"?  I don't get it.
>
> Maybe more clear:
> ...
> Yes, that looks fine to me.

Thanks for a clarification.

[PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Looking at the value of %(push:remoteref) only handles the case when an
explicit push refspec is passed. But it does not handle the fallback
cases of looking at the configuration value of `push.default`.

In particular, doing something like

    git config push.default current
    git for-each-ref --format='%(push)'
    git for-each-ref --format='%(push:remoteref)'

prints a useful tracking ref for the first for-each-ref, but an empty
string for the second.

Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
let upstream/push report the remote ref name) is to get exactly which
branch `git push` will push to, even in the fallback cases, fix this.

To get the meaning of %(push:remoteref), `ref-filter.c` calls
`remote_ref_for_branch`. We simply add a new static helper function,
`branch_get_push_remoteref` that follows the logic of
`branch_get_push_1`, and call it from `remote_ref_for_branch`.

We also update t/6300-for-each-ref.sh to handle all `push.default`
strategies. This involves testing `push.default=simple` twice, once
where there is a matching upstream branch and once when there is none.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 remote.c                | 106 +++++++++++++++++++++++++++++++---------
 t/t6300-for-each-ref.sh |  29 ++++++++++-
 2 files changed, 112 insertions(+), 23 deletions(-)

diff --git a/remote.c b/remote.c
index c43196ec06..b3ce992d01 100644
--- a/remote.c
+++ b/remote.c
@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
-{
-	if (branch) {
-		if (!for_push) {
-			if (branch->merge_nr) {
-				return branch->merge_name[0];
-			}
-		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
-
-			if (remote && remote->push.nr &&
-			    (dst = apply_refspecs(&remote->push,
-						  branch->refname))) {
-				return dst;
-			}
-		}
-	}
-	return NULL;
-}
-
 static struct remote *remote_get_1(const char *name,
 				   const char *(*get_default)(struct branch *, int *))
 {
@@ -1656,6 +1634,76 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+/**
+ * Return the local name of the remote tracking branch, as in
+ * %(push:remoteref), that corresponds to the ref we would push to given a
+ * bare `git push` while `branch` is checked out.
+ * See also branch_get_push_1 below.
+ */
+static const char *branch_get_push_remoteref(struct branch *branch)
+{
+	struct remote *remote;
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		return NULL;
+
+	if (remote->push.nr) {
+		char *dst;
+
+		dst = apply_refspecs(&remote->push, branch->refname);
+		if (!dst)
+			return NULL;
+
+		return dst;
+	}
+
+	if (remote->mirror)
+		return branch->refname;
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		return NULL;
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return branch->refname;
+
+	case PUSH_DEFAULT_UPSTREAM:
+		{
+			if (!branch || !branch->merge ||
+			    !branch->merge[0] || !branch->merge[0]->dst)
+			return NULL;
+
+			return branch->merge[0]->src;
+		}
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up, *cur;
+
+			up = branch_get_upstream(branch, NULL);
+			if (!up)
+				return NULL;
+			cur = tracking_for_push_dest(remote, branch->refname, NULL);
+			if (!cur)
+				return NULL;
+			if (strcmp(cur, up))
+				return NULL;
+
+			return branch->refname;
+		}
+	}
+
+	BUG("unhandled push situation");
+}
+
+/**
+ * Return the tracking branch, as in %(push), that corresponds to the ref we
+ * would push to given a bare `git push` while `branch` is checked out.
+ * See also branch_get_push_remoteref above.
+ */
 static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
@@ -1735,6 +1783,20 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
+{
+	if (branch) {
+		if (!for_push) {
+			if (branch->merge_nr) {
+				return branch->merge_name[0];
+			}
+		} else {
+			return branch_get_push_remoteref(branch);
+		}
+	}
+	return NULL;
+}
+
 /*
  * Create and return a list of (struct ref) consisting of copies of
  * each remote_ref that matches refspec.  refspec must be a pattern.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9c910ce746..60e21834fd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '
 		actual="$(git for-each-ref \
 			--format="%(push:remotename),%(push:remoteref)" \
 			refs/heads/push-simple)" &&
-		test from, = "$actual"
+		test from, = "$actual" &&
+		git config branch.push-simple.remote from &&
+		git config branch.push-simple.merge refs/heads/master &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/master = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		git config branch.push-simple.merge refs/heads/push-simple &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual"
 	)
 '
 
-- 
Patched on top of v2.25.1-377-g2d2118b814 (git version 2.25.1)

Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

I have some remarks/questions:

From Damien Robert, Tue 03 Mar 2020 at 17:12:23 (+0100) :
> +	if (remote->push.nr) {
> +		char *dst;
> +		dst = apply_refspecs(&remote->push, branch->refname);
> +		if (!dst)
> +			return NULL;
> +		return dst;
> +	}

Should I simply `return apply_refspecs(&remote->push, branch->refname);`
here, or is it a good form to always check for a NULL return value even if
we do nothing with it?

> +	case PUSH_DEFAULT_MATCHING:
> +	case PUSH_DEFAULT_CURRENT:
> +		return branch->refname;

Here I follow the logic of branch_get_push1, but the case of
push.default=matching is not quite correct, because we never check
that we have a matching remote branch. On the other hand we cannot check
this until we contact the remote, so I don't know how we could get around
that.


-- 
Damien Robert
http://www.normalesup.org/~robert/pro

Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

>> +	case PUSH_DEFAULT_MATCHING:
>> +	case PUSH_DEFAULT_CURRENT:
>> +		return branch->refname;
>
> Here I follow the logic of branch_get_push1, but the case of
> push.default=matching is not quite correct, because we never check
> that we have a matching remote branch. On the other hand we cannot check
> this until we contact the remote, so I don't know how we could get around
> that.

Quite honestly, I do not think that is a problem that needs to be
solved; there is no workable definition.

Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

> Looking at the value of %(push:remoteref) only handles the case when an
> explicit push refspec is passed. But it does not handle the fallback
> cases of looking at the configuration value of `push.default`.
>
> In particular, doing something like
>
>     git config push.default current
>     git for-each-ref --format='%(push)'
>     git for-each-ref --format='%(push:remoteref)'
>
> prints a useful tracking ref for the first for-each-ref, but an empty
> string for the second.
>
> Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
> let upstream/push report the remote ref name) is to get exactly which
> branch `git push` will push to, even in the fallback cases, fix this.
>
> To get the meaning of %(push:remoteref), `ref-filter.c` calls
> `remote_ref_for_branch`. We simply add a new static helper function,
> `branch_get_push_remoteref` that follows the logic of
> `branch_get_push_1`, and call it from `remote_ref_for_branch`.
>
> We also update t/6300-for-each-ref.sh to handle all `push.default`
> strategies. This involves testing `push.default=simple` twice, once
> where there is a matching upstream branch and once when there is none.
>
> Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
> ---
>  remote.c                | 106 +++++++++++++++++++++++++++++++---------
>  t/t6300-for-each-ref.sh |  29 ++++++++++-
>  2 files changed, 112 insertions(+), 23 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index c43196ec06..b3ce992d01 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
>  	return remote_for_branch(branch, explicit);
>  }
>  
> -const char *remote_ref_for_branch(struct branch *branch, int for_push)
> -{
> -	if (branch) {
> -		if (!for_push) {
> -			if (branch->merge_nr) {
> -				return branch->merge_name[0];
> -			}
> -		} else {
> -			const char *dst, *remote_name =
> -				pushremote_for_branch(branch, NULL);
> -			struct remote *remote = remote_get(remote_name);
> -
> -			if (remote && remote->push.nr &&
> -			    (dst = apply_refspecs(&remote->push,
> -						  branch->refname))) {
> -				return dst;
> -			}
> -		}
> -	}
> -	return NULL;
> -}

Mental note: this function was moved down, the main part of the
logic extracted to a new branch_get_push_remoteref() helper, which
in addition got extended.

> @@ -1656,6 +1634,76 @@ static const char *tracking_for_push_dest(struct remote *remote,
>  	return ret;
>  }
>  
> +/**
> + * Return the local name of the remote tracking branch, as in
> + * %(push:remoteref), that corresponds to the ref we would push to given a
> + * bare `git push` while `branch` is checked out.
> + * See also branch_get_push_1 below.
> + */
> +static const char *branch_get_push_remoteref(struct branch *branch)
> +{
> +	struct remote *remote;
> +
> +	remote = remote_get(pushremote_for_branch(branch, NULL));
> +	if (!remote)
> +		return NULL;
> +
> +	if (remote->push.nr) {
> +		char *dst;
> +
> +		dst = apply_refspecs(&remote->push, branch->refname);
> +		if (!dst)
> +			return NULL;
> +
> +		return dst;
> +	}

That's a fairly expensive way to write

	if (remote->push.nr)
		return apply_refspecs(&remote->push, branch->refname);

one-liner.

In any case, up to this point, the code does exactly the same thing
as the original (i.e. when remote.<remotename>.push is set and
covers the current branch, use that to figure out which way we are
pushing).

> +	if (remote->mirror)
> +		return branch->refname;

If mirroring, we push to the same name, OK.

> +	switch (push_default) {
> +	case PUSH_DEFAULT_NOTHING:
> +		return NULL;
> +
> +	case PUSH_DEFAULT_MATCHING:
> +	case PUSH_DEFAULT_CURRENT:
> +		return branch->refname;

These three cases are straight-forward, I think.

> +	case PUSH_DEFAULT_UPSTREAM:
> +		{
> +			if (!branch || !branch->merge ||
> +			    !branch->merge[0] || !branch->merge[0]->dst)
> +			return NULL;
> +
> +			return branch->merge[0]->src;
> +		}

This is strangely indented and somewhat unreadable.  Why isn't this
more like:

	case PUSH_DEFAULT_UPSTREAM:
		if (branch && branch->merge && branch->merge[0] &&
		    branch->merge[0]->dst)
			return branch->merge[0]->src;
		break;

and have "return NULL" after the switch() statement before we leave
the function?

> +
> +	case PUSH_DEFAULT_UNSPECIFIED:
> +	case PUSH_DEFAULT_SIMPLE:
> +		{
> +			const char *up, *cur;
> +
> +			up = branch_get_upstream(branch, NULL);
> +			if (!up)
> +				return NULL;
> +			cur = tracking_for_push_dest(remote, branch->refname, NULL);
> +			if (!cur)
> +				return NULL;
> +			if (strcmp(cur, up))
> +				return NULL;

This is probably not all that performance critical, so

			up = branch_get_upstream(branch, NULL);
			current = tracking_for_push_dest(remote, branch->refname, NULL);
			if (!up || !current || strcmp(current, up))
				return NULL;

might be easier to follow.

> +			return branch->refname;

> +		}
> +	}
> +
> +	BUG("unhandled push situation");

This is better done / easier to read inside switch() as default:
clause.

By the way, I have a bit higher-level question.  

All of the above logic that decides what should happen in "git push"
MUST have existing code we already use to implement "git push", no?

Why do we need to reinvent it here, instead of reusing the existing
code?  Is it because the interface into the functions that implement
the existing logic is very different from what this function wants?

Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

From Junio C Hamano, Tue 03 Mar 2020 at 10:21:31 (-0800) :
> Mental note: this function was moved down, the main part of the
> logic extracted to a new branch_get_push_remoteref() helper, which
> in addition got extended.

Exactly.

> That's a fairly expensive way to write
> 	if (remote->push.nr)
> 		return apply_refspecs(&remote->push, branch->refname);
> one-liner.

You anticipated the question I asked afterwards :)

> > +	case PUSH_DEFAULT_UPSTREAM:
> > +		{
> > +			if (!branch || !branch->merge ||
> > +			    !branch->merge[0] || !branch->merge[0]->dst)
> > +			return NULL;
> > +
> > +			return branch->merge[0]->src;
> > +		}
> 
> This is strangely indented and somewhat unreadable.  Why isn't this
> more like:

Sorry I missed the indentation for the return NULL.
 
> 	case PUSH_DEFAULT_UPSTREAM:
> 		if (branch && branch->merge && branch->merge[0] &&
> 		    branch->merge[0]->dst)
> 			return branch->merge[0]->src;
> 		break;
> 
> and have "return NULL" after the switch() statement before we leave
> the function?

We could, I agree this is more readable. If we return NULL after the
switch, we need to put the
> > +	BUG("unhandled push situation")
in a default clause a you say.

The reason I wrote the code as above is to be as close as possible to
`branch_get_push_1`. If we make the changes you suggest, I'll probably need
a preliminary patch to change `branch_get_push_1` accordingly.

> > +	case PUSH_DEFAULT_UNSPECIFIED:
> > +	case PUSH_DEFAULT_SIMPLE:
> > +		{
> > +			const char *up, *cur;
> > +
> > +			up = branch_get_upstream(branch, NULL);
> > +			if (!up)
> > +				return NULL;
> > +			cur = tracking_for_push_dest(remote, branch->refname, NULL);
> > +			if (!cur)
> > +				return NULL;
> > +			if (strcmp(cur, up))
> > +				return NULL;
> 
> This is probably not all that performance critical, so
> 			up = branch_get_upstream(branch, NULL);
> 			current = tracking_for_push_dest(remote, branch->refname, NULL);
> 			if (!up || !current || strcmp(current, up))
> 				return NULL;
> might be easier to follow.

I don't mind but likewise in this case we should probably change
branch_get_push_1 too.

> By the way, I have a bit higher-level question.  
> 
> All of the above logic that decides what should happen in "git push"
> MUST have existing code we already use to implement "git push", no?

Yes.

> Why do we need to reinvent it here, instead of reusing the existing
> code?  Is it because the interface into the functions that implement
> the existing logic is very different from what this function wants?

Mostly yes. The logic of git push is to massage the refspecs directly, for
instance:
	case PUSH_DEFAULT_MATCHING:
		refspec_append(&rs, ":");
	case PUSH_DEFAULT_CURRENT:
		...
		strbuf_addf(&refspec, "%s:%s", branch->refname, branch->refname);
	case PUSH_DEFAULT_UPSTREAM:
		...
		strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src);

And the error messages are also not the same, and to give a good error
message we need to parse the different cases.

It may be possible to refactorize all this, but not in an obvious way and
it would be a lot more work than this patch series.

-- 
Damien Robert
http://www.normalesup.org/~robert/pro

Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

>> By the way, I have a bit higher-level question.  
>> 
>> All of the above logic that decides what should happen in "git push"
>> MUST have existing code we already use to implement "git push", no?
>
> Yes.
>
>> Why do we need to reinvent it here, instead of reusing the existing
>> code?  Is it because the interface into the functions that implement
>> the existing logic is very different from what this function wants?
>
> Mostly yes. The logic of git push is to massage the refspecs directly, for
> instance:
> 	case PUSH_DEFAULT_MATCHING:
> 		refspec_append(&rs, ":");
> 	case PUSH_DEFAULT_CURRENT:
> 		...
> 		strbuf_addf(&refspec, "%s:%s", branch->refname, branch->refname);
> 	case PUSH_DEFAULT_UPSTREAM:
> 		...
> 		strbuf_addf(&refspec, "%s:%s", branch->refname, branch->merge[0]->src);
>
> And the error messages are also not the same, and to give a good error
> message we need to parse the different cases.
>
> It may be possible to refactorize all this, but not in an obvious way and
> it would be a lot more work than this patch series.

Yeah, in light of the analysis I agree it makes sense to take the
approach of these two patches, at least for now.

Thanks.

[PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Looking at the value of %(push:remoteref) only handles the case when an
explicit push refspec is passed. But it does not handle the fallback
cases of looking at the configuration value of `push.default`.

In particular, doing something like

    git config push.default current
    git for-each-ref --format='%(push)'
    git for-each-ref --format='%(push:remoteref)'

prints a useful tracking ref for the first for-each-ref, but an empty
string for the second.

Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
let upstream/push report the remote ref name) is to get exactly which
branch `git push` will push to, even in the fallback cases, fix this.

To get the meaning of %(push:remoteref), `ref-filter.c` calls
`remote_ref_for_branch`. We simply add a new static helper function,
`branch_get_push_remoteref` that follows the logic of
`branch_get_push_1`, and call it from `remote_ref_for_branch`.

We also update t/6300-for-each-ref.sh to handle all `push.default`
strategies. This involves testing `push.default=simple` twice, once
where there is a matching upstream branch and once when there is none.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---

I ended up following most of Junio's suggestion, except having a
    default: BUG(...)
and returning NULL at the end of the case.

I prefer to return explicitly in each case statement rather than use break
to fallback at the end of the case.

I said I would also update branch_get_push1 to be as similar as possible to
branch_get_push_remoteref, but because of the error handling of the latter,
it would makes the syntax a bit weird, so I did not touch it.

I am still a bit annoyed that I cannot call branch_get_push_remoteref from
branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can
wait and we will need to work with the code duplication meanwhile.

 remote.c                | 94 +++++++++++++++++++++++++++++++----------
 t/t6300-for-each-ref.sh | 29 ++++++++++++-
 2 files changed, 100 insertions(+), 23 deletions(-)

diff --git a/remote.c b/remote.c
index c43196ec06..352ea240cd 100644
--- a/remote.c
+++ b/remote.c
@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
-{
-	if (branch) {
-		if (!for_push) {
-			if (branch->merge_nr) {
-				return branch->merge_name[0];
-			}
-		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
-
-			if (remote && remote->push.nr &&
-			    (dst = apply_refspecs(&remote->push,
-						  branch->refname))) {
-				return dst;
-			}
-		}
-	}
-	return NULL;
-}
-
 static struct remote *remote_get_1(const char *name,
 				   const char *(*get_default)(struct branch *, int *))
 {
@@ -1656,6 +1634,64 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+/**
+ * Return the local name of the remote tracking branch, as in
+ * %(push:remoteref), that corresponds to the ref we would push to given a
+ * bare `git push` while `branch` is checked out.
+ * See also branch_get_push_1 below.
+ */
+static const char *branch_get_push_remoteref(struct branch *branch)
+{
+	struct remote *remote;
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		return NULL;
+
+	if (remote->push.nr) {
+		return apply_refspecs(&remote->push, branch->refname);
+	}
+
+	if (remote->mirror)
+		return branch->refname;
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		return NULL;
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return branch->refname;
+
+	case PUSH_DEFAULT_UPSTREAM:
+		if (branch && branch->merge && branch->merge[0] &&
+		    branch->merge[0]->dst)
+			return branch->merge[0]->src;
+		else
+			return NULL;
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up, *cur;
+
+			up = branch_get_upstream(branch, NULL);
+			cur = tracking_for_push_dest(remote, branch->refname, NULL);
+			if (up && cur && !strcmp(cur, up))
+				return branch->refname;
+			else
+				return NULL;
+
+		}
+	}
+	BUG("unhandled push situation");
+}
+
+/**
+ * Return the tracking branch, as in %(push), that corresponds to the ref we
+ * would push to given a bare `git push` while `branch` is checked out.
+ * See also branch_get_push_remoteref above.
+ */
 static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
@@ -1735,6 +1771,20 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
+{
+	if (branch) {
+		if (!for_push) {
+			if (branch->merge_nr) {
+				return branch->merge_name[0];
+			}
+		} else {
+			return branch_get_push_remoteref(branch);
+		}
+	}
+	return NULL;
+}
+
 /*
  * Create and return a list of (struct ref) consisting of copies of
  * each remote_ref that matches refspec.  refspec must be a pattern.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9c910ce746..60e21834fd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '
 		actual="$(git for-each-ref \
 			--format="%(push:remotename),%(push:remoteref)" \
 			refs/heads/push-simple)" &&
-		test from, = "$actual"
+		test from, = "$actual" &&
+		git config branch.push-simple.remote from &&
+		git config branch.push-simple.merge refs/heads/master &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/master = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		git config branch.push-simple.merge refs/heads/push-simple &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual"
 	)
 '
 
-- 
Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Hi Junio,

IMHO this patch should be good to cook.

Thanks!

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

> IMHO this patch should be good to cook.

Would love to queue it but I haven't had a time to look at it.

Thanks.

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

From Junio C Hamano, Fri 27 Mar 2020 at 15:08:14 (-0700) :
> Damien Robert <damien.olivier.robert@gmail.com> writes:
> > IMHO this patch should be good to cook.
> Would love to queue it but I haven't had a time to look at it.

Sure, no worries, I was just wondering if this was on your todo list or if
the last patch version fell through the cracks (I have no idea how you
manage to keep up with the traffic of this ml).

Your message managed to invoke Jeff who suggests some improvements to the
test, so I'll reroll anyway :)

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Jeff King]

On Thu, Mar 12, 2020 at 05:45:58PM +0100, Damien Robert wrote:

> Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
> let upstream/push report the remote ref name) is to get exactly which
> branch `git push` will push to, even in the fallback cases, fix this.
> 
> To get the meaning of %(push:remoteref), `ref-filter.c` calls
> `remote_ref_for_branch`. We simply add a new static helper function,
> `branch_get_push_remoteref` that follows the logic of
> `branch_get_push_1`, and call it from `remote_ref_for_branch`.

I looked at this again with fresh eyes, and I think it's a pretty
practical fix all around. I noticed one memory leak, but it's actually
there already. :-/

> I ended up following most of Junio's suggestion, except having a
>     default: BUG(...)
> and returning NULL at the end of the case.
>
> I prefer to return explicitly in each case statement rather than use break
> to fallback at the end of the case.

What you have looks reasonable to me (and would hopefully get us a
compiler warning if new push modes are added).

> I said I would also update branch_get_push1 to be as similar as possible to
> branch_get_push_remoteref, but because of the error handling of the latter,
> it would makes the syntax a bit weird, so I did not touch it.
>
> I am still a bit annoyed that I cannot call branch_get_push_remoteref from
> branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can
> wait and we will need to work with the code duplication meanwhile.

I looked into this, too, and have a working patch. It does get a little
awkward, though, and I'm happy to just take your patch for now as the
practical thing.

> -const char *remote_ref_for_branch(struct branch *branch, int for_push)
> [...]
> -			const char *dst, *remote_name =
> -				pushremote_for_branch(branch, NULL);
> -			struct remote *remote = remote_get(remote_name);
> -
> -			if (remote && remote->push.nr &&
> -			    (dst = apply_refspecs(&remote->push,
> -						  branch->refname))) {
> -				return dst;
> -			}

This is the leak in the old code. apply_refspecs() returns a newly
allocated buffer, but our caller would never know to free it because we
return a const pointer.

And we have the same problem in the new code:

> +static const char *branch_get_push_remoteref(struct branch *branch)
> [...]
> +	if (remote->push.nr) {
> +		return apply_refspecs(&remote->push, branch->refname);
> +	}

But we can't return a "char *", because all of the other return values
point to long-lived strings that the caller won't own. One way to solve
it would be to xstrdup() all of those. I'm not thrilled about that,
though; for-each-ref already does way more allocations-per-ref than I'd
like.

A better solution would be for this function to write the result into a
strbuf. For one-off calls that's no worse than allocating a string to
return, and for repeated calls like for-each-ref, it could reuse the
same allocated strbuf over and over.

Since this leak existed before your patch, I'm inclined to treat it as a
separate topic and not have it hold up this fix.

> +static const char *branch_get_push_remoteref(struct branch *branch)
> [...]

All the logic here makes sense to me.

> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh

The tests makes sense to me, though I found a few nits to pick:

> index 9c910ce746..60e21834fd 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '
>  		actual="$(git for-each-ref \
>  			--format="%(push:remotename),%(push:remoteref)" \
>  			refs/heads/push-simple)" &&
> -		test from, = "$actual"
> +		test from, = "$actual" &&

The existing tests just assume taht push.default=simple. Since we're now
testing everything, should this be "git -c push.default=simple" to be
more explicit?

Likewise, is it worth labeling all of the simple cases with a comment
(or possibly putting them in separate tests, though I guess some of the
setup is shared)?  This one expects blank because there's no configured
upstream.

> +		git config branch.push-simple.remote from &&
> +		git config branch.push-simple.merge refs/heads/master &&
> +		actual="$(git for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from, = "$actual" &&

This one expects blank because the upstream and local names don't match.

> +		actual="$(git -c push.default=upstream for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from,refs/heads/master = "$actual" &&

This one has a real configured upstream (and relies on the
branch.*.merge config set above). OK.

It's a little funny that the branch is still called push-simple. :)

> +		actual="$(git -c push.default=current for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from,refs/heads/push-simple = "$actual" &&

Same name on the other side. OK.

> +		actual="$(git -c push.default=matching for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from,refs/heads/push-simple = "$actual" &&

Ditto for matching, which I think is the only sensible output.

> +		actual="$(git -c push.default=nothing for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from, = "$actual" &&

Nothing for nothing. Makes sense.

> +		git config branch.push-simple.merge refs/heads/push-simple &&
> +		actual="$(git for-each-ref \
> +			--format="%(push:remotename),%(push:remoteref)" \
> +			refs/heads/push-simple)" &&
> +		test from,refs/heads/push-simple = "$actual"

And this is a real simple that actually shows something. It would make
more sense to me with the other "simple" tests, but I guess _not_ having
the upstream set to the same name is important for the quality of the
"current" and "upstream" tests.

Maybe we could do this test first, before setting branch.*.merge to
refs/heads/master?

-Peff

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Jeff King]

On Sat, Mar 28, 2020 at 09:15:53AM -0400, Jeff King wrote:

> > I said I would also update branch_get_push1 to be as similar as possible to
> > branch_get_push_remoteref, but because of the error handling of the latter,
> > it would makes the syntax a bit weird, so I did not touch it.
> >
> > I am still a bit annoyed that I cannot call branch_get_push_remoteref from
> > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can
> > wait and we will need to work with the code duplication meanwhile.
> 
> I looked into this, too, and have a working patch. It does get a little
> awkward, though, and I'm happy to just take your patch for now as the
> practical thing.

Here's what I came up with (against master, but I stole a few bits from
your patch to connect it to remote_ref_for_branch and test it). I'll
quote bits of it to comment inline, and you can find the complete patch
at the bottom.

> @@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch,
>  }
>  
>  __attribute__((format (printf,2,3)))
> -static const char *error_buf(struct strbuf *err, const char *fmt, ...)
> +static void *error_buf(struct strbuf *err, const char *fmt, ...)
>  {
>  	if (err) {
>  		va_list ap;

This loosens up error_buf() to make it usable for functions that aren't
returning a string. Which we use for...

> @@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...)
>  	return NULL;
>  }
>  
> -const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> +struct refspec_item *branch_get_upstream_refspec(struct branch *branch,
> +						 struct strbuf *err)
>  {
>  	if (!branch)
>  		return error_buf(err, _("HEAD does not point to a branch"));
> @@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
>  				 _("upstream branch '%s' not stored as a remote-tracking branch"),
>  				 branch->merge[0]->src);
>  
> -	return branch->merge[0]->dst;
> +	return branch->merge[0];
> +}
> +
> +const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
> +{
> +	struct refspec_item *ret = branch_get_upstream_refspec(branch, err);
> +	if (ret)
> +		return ret->dst;
> +	return NULL;
>  }
>  

We can't use branch_get_upstream() to get the remote side, since it
returns branch->merge[0]->dst, and we'd want branch->merge[0]->src. So
this factors out a helper that returns both sides, and
branch_get_upstream() can pick out "dst".

> @@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote,
>  	return ret;
>  }
>  
> -static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
> +static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err)
>  {
>  	struct remote *remote;
>  

Here I was able to just convert push_1 into push_remoteref, since it has
all of the error-handling we want (and the error_buf() helper makes it
safe to pass a NULL and just ignore the errors if a caller wants to).

And then push_1 essentially becomes:

  const char *remoteref = branch_get_push_remoteref(branch, err);
  return tracking_for_push_dest(remote, remoteref, err);

> @@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
>  				 branch->name);
>  
>  	if (remote->push.nr) {
> -		char *dst;
> -		const char *ret;
> -
> -		dst = apply_refspecs(&remote->push, branch->refname);
> +		char *dst = apply_refspecs(&remote->push, branch->refname);
>  		if (!dst)
>  			return error_buf(err,
>  					 _("push refspecs for '%s' do not include '%s'"),
>  					 remote->name, branch->name);
>  
> -		ret = tracking_for_push_dest(remote, dst, err);
> -		free(dst);
> -		return ret;
> +		return dst;
>  	}

We're really just dropping the tracking_for_push_dest() here, since we
want the remote side. This matches what your patch did, except that we
have the error handling.

By the way, this is how I noticed the memory leak. And this patch does
make it worse because we used to get the cleanup of "dst" right, but now
it will be split across two functions and we won't free it.

For that matter, tracking_for_push_dest() also returns an allocated
string as a "const char *", so it's a leak, too. Maybe the strbuf plan
is worth pursuing. :)

>  	if (remote->mirror)
> -		return tracking_for_push_dest(remote, branch->refname, err);
> +		return branch->refname;

Again, just dropping the tracking conversion, and it matches your patch.

>  	switch (push_default) {
>  	case PUSH_DEFAULT_NOTHING:
>  		return error_buf(err, _("push has no destination (push.default is 'nothing')"));

We don't need to touch anything here, because both the remote and local
sides are NULL. :)

>  
>  	case PUSH_DEFAULT_MATCHING:
>  	case PUSH_DEFAULT_CURRENT:
> -		return tracking_for_push_dest(remote, branch->refname, err);
> +		return branch->refname;

Again, just dropping tracking.

>  	case PUSH_DEFAULT_UPSTREAM:
> -		return branch_get_upstream(branch, err);
> +		{
> +			struct refspec_item *ret =
> +				branch_get_upstream_refspec(branch, err);
> +			if (ret)
> +				return ret->src;
> +			return NULL;
> +		}

Here we have to use the new helper to pull out the "src" side. This
unfortunately means that to get the tracking ref, we'll re-apply
tracking_for_push_dest(), even though we could have just gotten the
actual value we wanted from ret->dst (without a memory leak!).

> @@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
>  			if (strcmp(cur, up))
>  				return error_buf(err,
>  						 _("cannot resolve 'simple' push to a single destination"));
> -			return cur;
> +			return branch->refname;
>  		}

And here again we're less efficient. We already checked the tracking
here to make sure we have the same name on both sides. But when we
return, we'll still apply tracking_for_push_dest(), which will just give
us the same name again (but the caller doesn't know that).

> @@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)
>  	if (!branch)
>  		return error_buf(err, _("HEAD does not point to a branch"));
>  
> -	if (!branch->push_tracking_ref)
> -		branch->push_tracking_ref = branch_get_push_1(branch, err);
> +	if (!branch->push_tracking_ref) {
> +		const char *remoteref = branch_get_push_remoteref(branch, err);
> +		if (remoteref) {
> +			/*
> +			 * ugh, we have to find remote again; should there be a
> +			 * master function which returns both remote and remoteref?
> +			 */
> +			struct remote *remote =
> +				remote_get(pushremote_for_branch(branch, NULL));
> +			branch->push_tracking_ref =
> +				tracking_for_push_dest(remote, remoteref, err);
> +		}
> +	}
>  	return branch->push_tracking_ref;

And here I just dropped push_1 entirely and did it inline. The extra
remote is ugly though. I guess we could return it as an out-parameter.

So it does work, but it's kind of awkward. And even if we solved the
leaking problem by using a strbuf, that would make it doubly awkward,
because the code above would need an _extra_ strbuf to store the
branch_get_push_remoteref() result, and only to then convert it via
tracking_for_push_dest().

Full patch is below if you want to try it out or hack on it further.

---
diff --git a/remote.c b/remote.c
index c43196ec06..22144c96b5 100644
--- a/remote.c
+++ b/remote.c
@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
-{
-	if (branch) {
-		if (!for_push) {
-			if (branch->merge_nr) {
-				return branch->merge_name[0];
-			}
-		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
-
-			if (remote && remote->push.nr &&
-			    (dst = apply_refspecs(&remote->push,
-						  branch->refname))) {
-				return dst;
-			}
-		}
-	}
-	return NULL;
-}
-
 static struct remote *remote_get_1(const char *name,
 				   const char *(*get_default)(struct branch *, int *))
 {
@@ -1604,7 +1582,7 @@ int branch_merge_matches(struct branch *branch,
 }
 
 __attribute__((format (printf,2,3)))
-static const char *error_buf(struct strbuf *err, const char *fmt, ...)
+static void *error_buf(struct strbuf *err, const char *fmt, ...)
 {
 	if (err) {
 		va_list ap;
@@ -1615,7 +1593,8 @@ static const char *error_buf(struct strbuf *err, const char *fmt, ...)
 	return NULL;
 }
 
-const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
+struct refspec_item *branch_get_upstream_refspec(struct branch *branch,
+						 struct strbuf *err)
 {
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
@@ -1639,7 +1618,15 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 				 _("upstream branch '%s' not stored as a remote-tracking branch"),
 				 branch->merge[0]->src);
 
-	return branch->merge[0]->dst;
+	return branch->merge[0];
+}
+
+const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
+{
+	struct refspec_item *ret = branch_get_upstream_refspec(branch, err);
+	if (ret)
+		return ret->dst;
+	return NULL;
 }
 
 static const char *tracking_for_push_dest(struct remote *remote,
@@ -1656,7 +1643,7 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
-static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
+static const char *branch_get_push_remoteref(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
 
@@ -1667,33 +1654,34 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 				 branch->name);
 
 	if (remote->push.nr) {
-		char *dst;
-		const char *ret;
-
-		dst = apply_refspecs(&remote->push, branch->refname);
+		char *dst = apply_refspecs(&remote->push, branch->refname);
 		if (!dst)
 			return error_buf(err,
 					 _("push refspecs for '%s' do not include '%s'"),
 					 remote->name, branch->name);
 
-		ret = tracking_for_push_dest(remote, dst, err);
-		free(dst);
-		return ret;
+		return dst;
 	}
 
 	if (remote->mirror)
-		return tracking_for_push_dest(remote, branch->refname, err);
+		return branch->refname;
 
 	switch (push_default) {
 	case PUSH_DEFAULT_NOTHING:
 		return error_buf(err, _("push has no destination (push.default is 'nothing')"));
 
 	case PUSH_DEFAULT_MATCHING:
 	case PUSH_DEFAULT_CURRENT:
-		return tracking_for_push_dest(remote, branch->refname, err);
+		return branch->refname;
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
+		{
+			struct refspec_item *ret =
+				branch_get_upstream_refspec(branch, err);
+			if (ret)
+				return ret->src;
+			return NULL;
+		}
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
@@ -1709,7 +1697,7 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 			if (strcmp(cur, up))
 				return error_buf(err,
 						 _("cannot resolve 'simple' push to a single destination"));
-			return cur;
+			return branch->refname;
 		}
 	}
 
@@ -1721,8 +1709,19 @@ const char *branch_get_push(struct branch *branch, struct strbuf *err)
 	if (!branch)
 		return error_buf(err, _("HEAD does not point to a branch"));
 
-	if (!branch->push_tracking_ref)
-		branch->push_tracking_ref = branch_get_push_1(branch, err);
+	if (!branch->push_tracking_ref) {
+		const char *remoteref = branch_get_push_remoteref(branch, err);
+		if (remoteref) {
+			/*
+			 * ugh, we have to find remote again; should there be a
+			 * master function which returns both remote and remoteref?
+			 */
+			struct remote *remote =
+				remote_get(pushremote_for_branch(branch, NULL));
+			branch->push_tracking_ref =
+				tracking_for_push_dest(remote, remoteref, err);
+		}
+	}
 	return branch->push_tracking_ref;
 }
 
@@ -1735,6 +1734,20 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
+{
+	if (branch) {
+		if (!for_push) {
+			if (branch->merge_nr) {
+				return branch->merge_name[0];
+			}
+		} else {
+			return branch_get_push_remoteref(branch, NULL);
+		}
+	}
+	return NULL;
+}
+
 /*
  * Create and return a list of (struct ref) consisting of copies of
  * each remote_ref that matches refspec.  refspec must be a pattern.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 9c910ce746..60e21834fd 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -874,7 +874,34 @@ test_expect_success ':remotename and :remoteref' '
 		actual="$(git for-each-ref \
 			--format="%(push:remotename),%(push:remoteref)" \
 			refs/heads/push-simple)" &&
-		test from, = "$actual"
+		test from, = "$actual" &&
+		git config branch.push-simple.remote from &&
+		git config branch.push-simple.merge refs/heads/master &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/master = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from, = "$actual" &&
+		git config branch.push-simple.merge refs/heads/push-simple &&
+		actual="$(git for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/push-simple)" &&
+		test from,refs/heads/push-simple = "$actual"
 	)
 '
 

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

From Jeff King, Sat 28 Mar 2020 at 09:31:34 (-0400) :
> > > I am still a bit annoyed that I cannot call branch_get_push_remoteref from
> > > branch_get_push1 because of the PUSH_DEFAULT_UPSTREAM case, but this can
> > > wait and we will need to work with the code duplication meanwhile.

> > I looked into this, too, and have a working patch. It does get a little
> > awkward, though, and I'm happy to just take your patch for now as the
> > practical thing.

Hi Jeff,

I looked up at your patch again, because the code duplication gets more
annoying the more new corner cases I have to handle to get the push ref
correct in all cases (cf my cover letter to v6).

This implements what I was suggesting in
https://public-inbox.org/git/20200301220531.iuokzzdb5gruslrn@doriath/

Essentially in branch_get_push you call:

        remote = remote_get(pushremote_for_branch(branch, NULL));
        tracking_for_push_dest(remote, branch_get_push_remoteref(branch),

And as I pointed out, this is currently exactly what branch_get_push_1
does, except in the
PUSH_DEFAULT_UPSTREAM where it returns branch->merge[0]->dst.

But branch->merge is set up in `set_merge`, where we have:

                ret->merge[i]->src = xstrdup(ret->merge_name[i]);
                if (!remote_find_tracking(remote, ret->merge[i]) ||
                    strcmp(ret->remote_name, "."))
                continue;
                if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
                             &oid, &ref) == 1)
                        ret->merge[i]->dst = ref;

So in particular, when the remote is local, the current code path calls
dwim_ref. (I have no idea who set up ret->merge[i]->dst if the remote is
not local...)

So my question was: can dwim_ref(branch->merge[0]->src) be different from
tracking_for_push_dest(branch->merge[0]->src)?

So I admit I don't understand everything dwim_ref does, but there is at
least one case where the answer is yes: if we have a dangling symref,
dwim_ref which calls expand_ref in refs.c will detect it. So in the current
code, %(push) would show nothing, while with your patch it would show the
dangling symref.

Obviously we cannot allow a regression for this very common case ;)

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

From Jeff King, Sat 28 Mar 2020 at 09:15:53 (-0400) :

> The tests makes sense to me, though I found a few nits to pick:
[...]

So I updated the tests as follow. I put them in another test, as you
suggested, this is indeed much clearer.

test_expect_success ':push:remoteref' '
	git init remote-tests &&
	(
		cd remote-tests &&
		test_commit initial &&
		git remote add from fifth.coffee:blub &&
		git config branch.master.remote from &&
		actual="$(git -c push.default=simple for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from, = "$actual" &&
		git config branch.master.merge refs/heads/master &&
		actual="$(git -c push.default=simple for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from,refs/heads/master = "$actual" &&
		git config branch.master.merge refs/heads/other &&
		actual="$(git -c push.default=simple for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from, = "$actual" &&
		actual="$(git -c push.default=upstream for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from,refs/heads/other = "$actual" &&
		actual="$(git -c push.default=current for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from,refs/heads/master = "$actual" &&
		actual="$(git -c push.default=matching for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from,refs/heads/master = "$actual" &&
		actual="$(git -c push.default=nothing for-each-ref \
			--format="%(push:remotename),%(push:remoteref)" \
			refs/heads/master)" &&
		test from, = "$actual"
	)
'

And the test works with my patch.

So I decided for completude to also test with
		git config branch.master.pushRemote to
to test with a triangular workflow, and I found several (already existing)
bugs.

Heres what happen with such a triangular workflow when we do a `git push`:
- with push.default=simple, we have
	case PUSH_DEFAULT_SIMPLE:
		if (triangular)
			setup_push_current(remote, branch);
		else
			setup_push_upstream(remote, branch, triangular, 1);
		break;
  so the current branch is always pushed.
- with push.default=upstream, we have
	case PUSH_DEFAULT_UPSTREAM:
		setup_push_upstream(remote, branch, triangular, 0);
		break;
  which then gives
  	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."),

By the way this matches what the documentation says.

However here is the result of
git -c push.default=$value for-each-ref --format="%(push:remotename),%(push:remoteref),%(push)" refs/heads/master
for $value=
- simple: to,,
- upstream: to,refs/heads/other,refs/remotes/from/other

Note that without my patch the %(push:remoteref) values would always be empty,
but my patch does not touch %(push).

So in both branch_get_push_1 and branch_get_push_remoteref I should first
detect if we have a triangular workflow, and update the logic of the code
accordingly.

Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)

[Jeff King]

On Mon, Apr 06, 2020 at 06:04:39PM +0200, Damien Robert wrote:

> Heres what happen with such a triangular workflow when we do a `git push`:
> - with push.default=simple, we have
> 	case PUSH_DEFAULT_SIMPLE:
> 		if (triangular)
> 			setup_push_current(remote, branch);
> 		else
> 			setup_push_upstream(remote, branch, triangular, 1);
> 		break;
>   so the current branch is always pushed.

Yeah, otherwise every push to a remote other than origin would require a
refspec. I think with respect to for-each-ref, this "triangular" case
would only kick in if you define remote.pushDefault (since you can't
specify a remote on the command-line).

Though hmm. I guess maybe it could kick in if the upstream of the branch
is on a non-default remote? For push, that would work because
remote_get() will look at the current branch. But of course in
for-each-ref, we're asking speculatively about other branches.

So I think if we want to support this triangular logic in for-each-ref,
we need to have a more careful definition than what's in push.c's
is_workflow_triangular(). I.e., it would probably make sense to consider
it from the position of "if we were on this branch, what would it push".

And ditto for @{push}, I guess.

> - with push.default=upstream, we have
> 	case PUSH_DEFAULT_UPSTREAM:
> 		setup_push_upstream(remote, branch, triangular, 0);
> 		break;
>   which then gives
>   	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."),
> 
> By the way this matches what the documentation says.

Yeah. I think in the triangular case (at least as defined in push.c)
we'd always be pushing to the non-upstream, so this die() makes sense.

In for-each-ref, I guess we'd hit this case with remote.pushDefault
again. Without that, we'd always be pushing to the upstream anyway.

> However here is the result of
> git -c push.default=$value for-each-ref --format="%(push:remotename),%(push:remoteref),%(push)" refs/heads/master
> for $value=
> - simple: to,,
> - upstream: to,refs/heads/other,refs/remotes/from/other
> 
> Note that without my patch the %(push:remoteref) values would always be empty,
> but my patch does not touch %(push).
> 
> So in both branch_get_push_1 and branch_get_push_remoteref I should first
> detect if we have a triangular workflow, and update the logic of the code
> accordingly.

Yes, I agree this could be improved. I'm OK leaving that as a separate
fix to your current remoteref work, though.

-Peff

[RFC PATCH v4 0/2] %(push) and %(push:remoteref) bug fixes

[Damien Robert]

This fix several bugs in for-each-ref for %(push) and %(push:remoteref), as
explained in the commit messages.

Note that there are still several bugs:
- the memory leak mentioned by Jeff in
  https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/

- in my patch, to detect if the workflow is triangular, I use:

static int is_workflow_triangular(struct branch *branch)
{
	struct remote *fetch_remote = remote_get(remote_for_branch(branch, NULL));
	struct remote *push_remote = remote_get(pushremote_for_branch(branch, NULL));
	return (fetch_remote && push_remote && fetch_remote != push_remote);
}

But remote_get will always fallback to 'origin'. So this means that if we
set up a pushRemote="foobar" and no 'remote', the workflow is detected as
triangular.

Whereas in `git push`, this workflow will not be detected as triangular.

=> So I can check that by looking at *explicit, but I actually have a
question about what constitutes a triangular workflow, hence the RFC.

Furthermore, the upstream (and simple in non triangular workflow) case of
%(push) and (push:remoteref) are essentially via `branch_get_upstream`, which
is also used for %(upstream):

	branch && branch->merge && branch->merge[0] &&
		    	    branch->merge[0]->dst)

but `git push` does different checks:

	if (!branch->merge_nr || !branch->merge || !branch->remote_name)
		die(_("The current branch %s has no upstream branch.\n"...
	if (branch->merge_nr != 1)
		die(_("The current branch %s has multiple upstream branches, "
		    "refusing to push."), branch->name);

in particular git push fails if merge_nr !=1 or if branch has no remote,
whereas %(push) will still indicates a push branch (assuming I fix
is_workflow_triangular).

So I'll need to add a `branch_get_push` with these checks instead.

So I first send this patch as an RFC, and I'll see how to proceed
afterwards to handle these remaining corner cases.
Luckily, having a pushRemote but no remote, or several merge in the branch
config are probably not too common.

=> So one question I have first is about the case when we do have a
branch.pushRemote but not a branch.remote.
Should this still be considered a triangular workflow?

According to git-push, no:

	static int is_workflow_triangular(struct remote *remote)
	{
		struct remote *fetch_remote = remote_get(NULL);
		return (fetch_remote && fetch_remote != remote);
	}

but I would argue that we should.

This would change nothing for push.default=upstream, since currently we
check that `branch` has a remote_name in `setup_push_upstream` so it fails
anyway even if the workflow is not explicitly triangular, but this would
make push.default=simple behave as current, exactly as when branch.remote
is different from branch.pushRemote (and I would argue that no
branch.remote is a particular case of this situation).


PS: the first patch has no tests because I add them in the second patch, it
is more convenient to add them at once and test both patches.

PPS: v4 and v5 are intermediate versions I made but did not send to the ML.


Damien Robert (2):
  remote.c: fix %(push) for triangular workflows
  remote.c: fix handling of %(push:remoteref)

 remote.c                | 139 ++++++++++++++++++++++++++++++----------
 t/t6300-for-each-ref.sh |  81 +++++++++++++++++++++--
 2 files changed, 180 insertions(+), 40 deletions(-)

-- 
Patched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)

[PATCH v6 1/2] remote.c: fix %(push) for triangular workflows

[Damien Robert]

The behaviour of `git push` when push.default is simple or upstream
changes in a triangular workflow, but this was not taken into account by
%(push). Update the code to detect triangular workflows and fix this.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 remote.c | 42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

diff --git a/remote.c b/remote.c
index c43196ec06..3750a2bcc1 100644
--- a/remote.c
+++ b/remote.c
@@ -1656,6 +1656,18 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+static int is_workflow_triangular(struct branch *branch)
+{
+	struct remote *fetch_remote = remote_get(remote_for_branch(branch, NULL));
+	struct remote *push_remote = remote_get(pushremote_for_branch(branch, NULL));
+	return (fetch_remote && push_remote && fetch_remote != push_remote);
+}
+
+/**
+ * Return the tracking branch, as in %(push), that corresponds to the ref we
+ * would push to given a bare `git push` while `branch` is checked out.
+ * See also branch_get_push_remoteref above.
+ */
 static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
@@ -1693,23 +1705,31 @@ static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 		return tracking_for_push_dest(remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
-		return branch_get_upstream(branch, err);
+		if (is_workflow_triangular(branch))
+			return error_buf(err, _("push has no destination (push.default is 'upstream' and we are in a triangular workflow)"));
+		else
+			return branch_get_upstream(branch, err);
 
 	case PUSH_DEFAULT_UNSPECIFIED:
 	case PUSH_DEFAULT_SIMPLE:
 		{
 			const char *up, *cur;
 
-			up = branch_get_upstream(branch, err);
-			if (!up)
-				return NULL;
-			cur = tracking_for_push_dest(remote, branch->refname, err);
-			if (!cur)
-				return NULL;
-			if (strcmp(cur, up))
-				return error_buf(err,
-						 _("cannot resolve 'simple' push to a single destination"));
-			return cur;
+			if (is_workflow_triangular(branch)) {
+				return tracking_for_push_dest(remote, branch->refname, err);
+			}
+			else {
+				up = branch_get_upstream(branch, err);
+				if (!up)
+					return NULL;
+				cur = tracking_for_push_dest(remote, branch->refname, err);
+				if (!cur)
+					return NULL;
+				if (strcmp(cur, up))
+					return error_buf(err,
+							 _("cannot resolve 'simple' push to a single destination"));
+				return cur;
+			}
 		}
 	}
 
-- 
Patched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)

[PATCH v6 2/2] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Looking at the value of %(push:remoteref) only handles the case when an
explicit push refspec is passed. But it does not handle the fallback
cases of looking at the configuration value of `push.default`.

In particular, doing something like

    git config push.default current
    git for-each-ref --format='%(push)'
    git for-each-ref --format='%(push:remoteref)'

prints a useful tracking ref for the first for-each-ref, but an empty
string for the second.

Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
let upstream/push report the remote ref name) is to get exactly which
branch `git push` will push to, even in the fallback cases, fix this.

To get the meaning of %(push:remoteref), `ref-filter.c` calls
`remote_ref_for_branch`. We simply add a new static helper function,
`branch_get_push_remoteref` that follows the logic of
`branch_get_push_1`, and call it from `remote_ref_for_branch`.

We also update t/6300-for-each-ref.sh to handle all `push.default`
strategies. This involves testing `push.default=simple` twice, once
where there is a matching upstream branch and once when there is none.

Finally we also test for triangular workflows.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 remote.c                | 97 +++++++++++++++++++++++++++++++----------
 t/t6300-for-each-ref.sh | 81 +++++++++++++++++++++++++++++++---
 2 files changed, 149 insertions(+), 29 deletions(-)

diff --git a/remote.c b/remote.c
index 3750a2bcc1..2b7f8a3af5 100644
--- a/remote.c
+++ b/remote.c
@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
-{
-	if (branch) {
-		if (!for_push) {
-			if (branch->merge_nr) {
-				return branch->merge_name[0];
-			}
-		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
-
-			if (remote && remote->push.nr &&
-			    (dst = apply_refspecs(&remote->push,
-						  branch->refname))) {
-				return dst;
-			}
-		}
-	}
-	return NULL;
-}
-
 static struct remote *remote_get_1(const char *name,
 				   const char *(*get_default)(struct branch *, int *))
 {
@@ -1663,6 +1641,67 @@ static int is_workflow_triangular(struct branch *branch)
 	return (fetch_remote && push_remote && fetch_remote != push_remote);
 }
 
+/**
+ * Return the local name of the remote tracking branch, as in
+ * %(push:remoteref), that corresponds to the ref we would push to given a
+ * bare `git push` while `branch` is checked out.
+ * See also branch_get_push_1 below.
+ */
+static const char *branch_get_push_remoteref(struct branch *branch)
+{
+	struct remote *remote;
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		return NULL;
+
+	if (remote->push.nr) {
+		return apply_refspecs(&remote->push, branch->refname);
+	}
+
+	if (remote->mirror)
+		return branch->refname;
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		return NULL;
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return branch->refname;
+
+	case PUSH_DEFAULT_UPSTREAM:
+		if (is_workflow_triangular(branch))
+		    return NULL;
+		else {
+			if (branch && branch->merge && branch->merge[0] &&
+		    	    branch->merge[0]->dst)
+				return branch->merge[0]->src;
+			else
+				return NULL;
+		}
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up, *cur;
+
+			if (is_workflow_triangular(branch))
+				return branch->refname;
+			else {
+				up = branch_get_upstream(branch, NULL);
+				cur = tracking_for_push_dest(remote, branch->refname, NULL);
+				if (up && cur && !strcmp(cur, up))
+					return branch->refname;
+				else
+					return NULL;
+			}
+
+		}
+	}
+	BUG("unhandled push situation");
+}
+
 /**
  * Return the tracking branch, as in %(push), that corresponds to the ref we
  * would push to given a bare `git push` while `branch` is checked out.
@@ -1755,6 +1794,20 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
+{
+	if (branch) {
+		if (!for_push) {
+			if (branch->merge_nr) {
+				return branch->merge_name[0];
+			}
+		} else {
+			return branch_get_push_remoteref(branch);
+		}
+	}
+	return NULL;
+}
+
 /*
  * Create and return a list of (struct ref) consisting of copies of
  * each remote_ref that matches refspec.  refspec must be a pattern.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index b3c1092338..8e59ab2567 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -875,13 +875,80 @@ test_expect_success ':remotename and :remoteref' '
 			git for-each-ref --format="${pair%=*}" \
 				refs/heads/master >actual &&
 			test_cmp expect actual
-		done &&
-		git branch push-simple &&
-		git config branch.push-simple.pushRemote from &&
-		actual="$(git for-each-ref \
-			--format="%(push:remotename),%(push:remoteref)" \
-			refs/heads/push-simple)" &&
-		test from, = "$actual"
+		done
+	)
+'
+
+test_expect_success '%(push) and %(push:remoteref)' '
+	git init pushremote-tests &&
+	(
+		cd pushremote-tests &&
+		test_commit initial &&
+		git remote add from fifth.coffee:blub &&
+		git config branch.master.remote from &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,, = "$actual" &&
+		git config branch.master.merge refs/heads/master &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master,refs/remotes/from/master = "$actual" &&
+		git config branch.master.merge refs/heads/other &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,refs/heads/other,refs/remotes/from/other = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master,refs/remotes/from/master = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master,refs/remotes/from/master = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test from,, = "$actual" &&
+		git remote add to southridge.audio:repo &&
+		git config branch.master.pushRemote to &&
+		git config --unset branch.master.merge &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		git config branch.master.merge refs/heads/master &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		git config branch.master.merge refs/heads/other &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual"
 	)
 '
 
-- 
Patched on top of v2.26.0-106-g9fadedd637 (git version 2.26.0)

[PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Looking at the value of %(push:remoteref) only handles the case when an
explicit push refspec is passed. But it does not handle the fallback
cases of looking at the configuration value of `push.default`.

In particular, doing something like

    git config push.default current
    git for-each-ref --format='%(push)'
    git for-each-ref --format='%(push:remoteref)'

prints a useful tracking ref for the first for-each-ref, but an empty
string for the second.

Since the intention of %(push:remoteref), from 9700fae5ee (for-each-ref:
let upstream/push report the remote ref name) is to get exactly which
branch `git push` will push to, even in the fallback cases, fix this.

To get the meaning of %(push:remoteref), `ref-filter.c` calls
`remote_ref_for_branch`. We simply add a new static helper function,
`branch_get_push_remoteref` that follows the logic of
`branch_get_push_1`, and call it from `remote_ref_for_branch`.

We also update t/6300-for-each-ref.sh to handle all `push.default`
strategies. This involves testing `push.default=simple` twice, once
where there is a matching upstream branch and once when there is none.

Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com>
---
 remote.c                | 94 +++++++++++++++++++++++++++++++----------
 t/t6300-for-each-ref.sh | 44 ++++++++++++++++---
 2 files changed, 111 insertions(+), 27 deletions(-)

diff --git a/remote.c b/remote.c
index c43196ec06..352ea240cd 100644
--- a/remote.c
+++ b/remote.c
@@ -516,28 +516,6 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)
 	return remote_for_branch(branch, explicit);
 }
 
-const char *remote_ref_for_branch(struct branch *branch, int for_push)
-{
-	if (branch) {
-		if (!for_push) {
-			if (branch->merge_nr) {
-				return branch->merge_name[0];
-			}
-		} else {
-			const char *dst, *remote_name =
-				pushremote_for_branch(branch, NULL);
-			struct remote *remote = remote_get(remote_name);
-
-			if (remote && remote->push.nr &&
-			    (dst = apply_refspecs(&remote->push,
-						  branch->refname))) {
-				return dst;
-			}
-		}
-	}
-	return NULL;
-}
-
 static struct remote *remote_get_1(const char *name,
 				   const char *(*get_default)(struct branch *, int *))
 {
@@ -1656,6 +1634,64 @@ static const char *tracking_for_push_dest(struct remote *remote,
 	return ret;
 }
 
+/**
+ * Return the local name of the remote tracking branch, as in
+ * %(push:remoteref), that corresponds to the ref we would push to given a
+ * bare `git push` while `branch` is checked out.
+ * See also branch_get_push_1 below.
+ */
+static const char *branch_get_push_remoteref(struct branch *branch)
+{
+	struct remote *remote;
+
+	remote = remote_get(pushremote_for_branch(branch, NULL));
+	if (!remote)
+		return NULL;
+
+	if (remote->push.nr) {
+		return apply_refspecs(&remote->push, branch->refname);
+	}
+
+	if (remote->mirror)
+		return branch->refname;
+
+	switch (push_default) {
+	case PUSH_DEFAULT_NOTHING:
+		return NULL;
+
+	case PUSH_DEFAULT_MATCHING:
+	case PUSH_DEFAULT_CURRENT:
+		return branch->refname;
+
+	case PUSH_DEFAULT_UPSTREAM:
+		if (branch && branch->merge && branch->merge[0] &&
+		    branch->merge[0]->dst)
+			return branch->merge[0]->src;
+		else
+			return NULL;
+
+	case PUSH_DEFAULT_UNSPECIFIED:
+	case PUSH_DEFAULT_SIMPLE:
+		{
+			const char *up, *cur;
+
+			up = branch_get_upstream(branch, NULL);
+			cur = tracking_for_push_dest(remote, branch->refname, NULL);
+			if (up && cur && !strcmp(cur, up))
+				return branch->refname;
+			else
+				return NULL;
+
+		}
+	}
+	BUG("unhandled push situation");
+}
+
+/**
+ * Return the tracking branch, as in %(push), that corresponds to the ref we
+ * would push to given a bare `git push` while `branch` is checked out.
+ * See also branch_get_push_remoteref above.
+ */
 static const char *branch_get_push_1(struct branch *branch, struct strbuf *err)
 {
 	struct remote *remote;
@@ -1735,6 +1771,20 @@ static int ignore_symref_update(const char *refname)
 	return (flag & REF_ISSYMREF);
 }
 
+const char *remote_ref_for_branch(struct branch *branch, int for_push)
+{
+	if (branch) {
+		if (!for_push) {
+			if (branch->merge_nr) {
+				return branch->merge_name[0];
+			}
+		} else {
+			return branch_get_push_remoteref(branch);
+		}
+	}
+	return NULL;
+}
+
 /*
  * Create and return a list of (struct ref) consisting of copies of
  * each remote_ref that matches refspec.  refspec must be a pattern.
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index b3c1092338..c89fd66083 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -875,12 +875,46 @@ test_expect_success ':remotename and :remoteref' '
 			git for-each-ref --format="${pair%=*}" \
 				refs/heads/master >actual &&
 			test_cmp expect actual
-		done &&
-		git branch push-simple &&
-		git config branch.push-simple.pushRemote from &&
-		actual="$(git for-each-ref \
+		done
+	)
+'
+
+test_expect_success ':push:remoteref' '
+	git init push-tests &&
+	(
+		cd push-tests &&
+		test_commit initial &&
+		git remote add from fifth.coffee:blub &&
+		git config branch.master.remote from &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from, = "$actual" &&
+		git config branch.master.merge refs/heads/master &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master = "$actual" &&
+		git config branch.master.merge refs/heads/other &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from,refs/heads/other = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref)" \
+			refs/heads/master)" &&
+		test from,refs/heads/master = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
 			--format="%(push:remotename),%(push:remoteref)" \
-			refs/heads/push-simple)" &&
+			refs/heads/master)" &&
 		test from, = "$actual"
 	)
 '
-- 
Patched on top of v2.26.1-107-gefe3874640 (git version 2.26.0)

Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

So this patch does not solve:
- the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/
- the non correct result in a triangular workflow mentioned in
  https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/
  (aka v6, with an incomplete fix)
- the code duplication between branch_get_push_remoteref and remote_get_1
  (see my answer to Jeff's code mentioned above as to why his approach
  would give a micro regression).

Luckily I have figured how to solve 2) and 3). Unfortunately I haven't have
time to work on this since two weeks, so I have just sent the original bug
fix first so at least this one gets fixed.

This is exactly as v4, except I moved the tests around to make them easier
to grok.

Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

> So this patch does not solve:
> - the memory leak mentioned by Jeff in https://public-inbox.org/git/20200328131553.GA643242@coredump.intra.peff.net/
> - the non correct result in a triangular workflow mentioned in
>   https://public-inbox.org/git/20200406175648.25737-1-damien.olivier.robert+git@gmail.com/
>   (aka v6, with an incomplete fix)
> - the code duplication between branch_get_push_remoteref and remote_get_1
>   (see my answer to Jeff's code mentioned above as to why his approach
>   would give a micro regression).
>
> Luckily I have figured how to solve 2) and 3). Unfortunately I haven't have
> time to work on this since two weeks, so I have just sent the original bug
> fix first so at least this one gets fixed.
>
> This is exactly as v4, except I moved the tests around to make them easier
> to grok.

Anything new on this topic?  No rush, but I'd hate to see a
basically good topic to be left in the stalled state too long.

Thanks.

Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)

[Damien Robert]

Hi Junio,

From Junio C Hamano, Thu 03 Sep 2020 at 15:01:10 (-0700) :
> Anything new on this topic?

Unfortunately no, work keep stacking up faster than I can unstack it...

> No rush, but I'd hate to see a basically good topic to be left in the stalled state too long.

Hum, what about migrating the version that was in next to master? I am not
fond of it because the series is not perfect and I am not satisfied with a
patch series that is not as good as I would like it to be. So that was why
I was arguing against merging it back then.

On the other hand it does correct existing bugs, and the bugs it leaves
remaining (apart from the memory leak) happens only in exotic cases. So I
would not want my sense of perfection to prevent this series from graduating
too long.

And unfortunately I cannot give you an ETA for a fully satisfying series as
I envision it.

So I guess it is your call. If you think the version that was in next is
good enough to graduate, I can send one last reroll with a commit message
explaining the remaining kinks, and iron them out later.

Or more precisely, we can:
- only use this patch (v8) without the triangular workflow fixes
- use this patch (v8) + the triangular workflow fixes from https://public-inbox.org/git/20200406175648.25737-2-damien.olivier.robert+git@gmail.com/

The 'bug' that remains is that it detects a triangular setup, when
1) a branch has a pushRemote but no remote and
2) pushRemote=foobar and origin does not exists
while 'git push' treat this as a non triangular workflow.

IMHO 'git push' is wrong here and in my ideal perfect series it would be
fixed there, but maybe meanwhile we can live with this small discrepancy.

What do you think?

-- 
Damien Robert
http://www.normalesup.org/~robert/pro

Re: [PATCH v8 1/1] remote.c: fix handling of %(push:remoteref)

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> writes:

> Hum, what about migrating the version that was in next to master? I am not
> fond of it because the series is not perfect and I am not satisfied with a
> patch series that is not as good as I would like it to be. So that was why
> I was arguing against merging it back then.
>
> On the other hand it does correct existing bugs, and the bugs it leaves
> remaining (apart from the memory leak) happens only in exotic cases. So I
> would not want my sense of perfection to prevent this series from graduating
> too long.
>
> And unfortunately I cannot give you an ETA for a fully satisfying series as
> I envision it.

That's OK---that is what "no rush" means.

We can throw the one bug it fixes together with the "bugs it leaves"
into the same category, i.e. happens only in narrow cases.  We now
know that you won't be actively working on the topic right now,
perhaps others can pick up where you left off and perhaps you can
help reviewing such a follow-up work ;-)

Thanks.

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Damien Robert]

From Jeff King, Mon 02 Mar 2020 at 08:32:17 (-0500) :
> > I think you looked at the RR_REMOTE_NAME (ref-filter.c:1455), here the
> > situation is handled by RR_REMOTE_REF, where explicit is not used at all.
> > So we could remove it.
> 
> We do look at it, but it's pointless to do so:

Oh sorry, I don't know how I missed this line while I saw it above.
> 
>   $ git grep -hn -C4 remote_ref_for_branch origin:ref-filter.c
>   1461-	} else if (atom->u.remote_ref.option == RR_REMOTE_REF) {
>   1462-		int explicit;
>   1463-		const char *merge;
>   1464-
>   1465:		merge = remote_ref_for_branch(branch, atom->u.remote_ref.push,
>   1466-					      &explicit);
>   1467-		*s = xstrdup(explicit ? merge : "");
>   1468-	} else
>   1469-		BUG("unhandled RR_* enum");
> 
> I think we probably ought to do this as a preparatory patch in your
> series.

I wonder about the case of RR_REMOTE_NAME to.
We always have explicit=1, except if we fallback all the way to 'origin',
via pushremote_for_branch and then remote_for_branch. But 'origin' even
through it is implicit, is still the name of the remote we fetch/push to by
default. So should not %(push), %(upstream) still show origin in this case?

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Jeff King]

On Sun, Mar 01, 2020 at 11:05:31PM +0100, Damien Robert wrote:

> So it remains the problem of handling the 'upstream' case.
> The ideal solution would be to not duplicate branch_get_push_1.

Yeah, that would be nice (though at least if it's all contained in
remote.c, we can live with some duplication). There's already some
duplication in the way remote_ref_for_branch() applies remote refspecs.

And I think all of this may be duplicated with git-push itself (which
would also be nice to get rid of, but last time I looked into it was
hard to refactor it to do so).

> In most of the case, this function finds `dst` which is exactly the
> push:remoteref we are looking for.
> 
> Then branch_get_push_1 uses
> 		ret = tracking_for_push_dest(remote, dst, err);
> which simply calls
> 	ret = apply_refspecs(&remote->fetch, dst);

Right, there we already have the remote name, and are applying the fetch
refspecs to know what our tracking branch would be. So in
remote_ref_for_branch(), we'd just not apply those.

> The only different case is
> 	case PUSH_DEFAULT_UPSTREAM:
> 		return branch_get_upstream(branch, err);
> which returns
> 	branch->merge[0]->dst

We also have PUSH_DEFAULT_NOTHING, for which obviously we'd return
nothing (NULL or an empty string).

Likewise for SIMPLE, we probably need to check that the upstream has a
matching name (and return nothing if not).

> So we could almost write an auxiliary function that returns push:remoteref
> and use it both in remote_ref_for_branch and branch_get_push_1 (via a
> further call to tracking_for_push_dest) except for the 'upstream' case
> which is subtly different.

Yes, that makes sense.

> In the 'upstream' case, the auxiliary function would return
> branch->merge_name[0]. So the question is: can
> tracking_for_push_dest(branch->merge_name[0]) be different from
> branch->merge[0]->dst?

Those will both return tracking refs. I think you just want
merge[0]->src for the upstream case.

And yes, the two can be different. It's the same case as when the
upstream branch has a different name than the current branch.

> Another solution could be as follow: we already store `push` in
> `branch->push_tracking_ref`. So the question is: can we always easily convert
> something like refs/remotes/origin/branch_name to refs/heads/branch_name
> (ie essentially reverse ètracking_for_push_dest`), or are there corner cases?

This would basically be reverse-applying the fetch refspec. In theory
it should be possible, but there are cases where somebody has
overlapping refspecs. But at any rate, I think it's better to just get
the pre-mapped values (i.e., avoid calling tracking_for_push_dest() in
the first place).

> Otherwise a simple but not elegant solution would be to copy paste the
> code of branch_get_push_1 to remote_ref_for_branch, simply removing the
> calls to `tracking_for_push_dest` and using remote->branch_name[0] rather
> than remote->branch[0]->dst for the upstream case.

Yeah, I think that's going to be the easiest. It would be nice to avoid
repeating that switch(), but frankly I think the boilerplate you'll end
up with trying to handle the two cases may be worse than just repeating
it. It may be worth adding a comment to each function to mention the
other, and that any changes need to match.

-Peff

Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref

[Damien Robert]

From Jeff King, Mon 02 Mar 2020 at 08:48:42 (-0500) :
> And I think all of this may be duplicated with git-push itself (which
> would also be nice to get rid of, but last time I looked into it was
> hard to refactor it to do so).

I had a quick look at git-push but the duplication does not seems too bad.

> > In the 'upstream' case, the auxiliary function would return
> > branch->merge_name[0]. So the question is: can
> > tracking_for_push_dest(branch->merge_name[0]) be different from
> > branch->merge[0]->dst?

> Those will both return tracking refs. I think you just want
> merge[0]->src for the upstream case.
> And yes, the two can be different. It's the same case as when the
> upstream branch has a different name than the current branch.

I meant, now that we have branch_get_push_remoteref, can we replace
the body of branch_get_push_1 by
	remote = remote_get(pushremote_for_branch(branch, NULL));
	ret = tracking_for_push_dest(remote, branch_get_push_remoteref(branch), err);
(we would need to add error handling in branch_get_push_remoteref but that
is easy)

Currently that is exactly what branch_get_push_1 does, except in the
PUSH_DEFAULT_UPSTREAM where it returns branch->merge[0]->dst.
But branch->merge is set up in `set_merge`, where we have:
		ret->merge[i]->src = xstrdup(ret->merge_name[i]);
		...
		if (dwim_ref(ret->merge_name[i], strlen(ret->merge_name[i]),
			     &oid, &ref) == 1)
			ret->merge[i]->dst = ref;
So my question was: can dwim_ref(branch->merge[0]->src) be different from
tracking_for_push_dest(branch->merge[0]->src)?

> Yeah, I think that's going to be the easiest. It would be nice to avoid
> repeating that switch(), but frankly I think the boilerplate you'll end
> up with trying to handle the two cases may be worse than just repeating
> it.

That's what I went with. We can always refactorise branch_get_push_1 to use
branch_get_push_remoteref afterwards.

> It may be worth adding a comment to each function to mention the
> other, and that any changes need to match.

I tried to add a comment, but I don't know if it is helpful enough.