Git Mailing List Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] remote.c: fix handling of push:remote_ref
@ 2020-02-28 17:24 Damien Robert
  2020-02-28 18:23 ` Jeff King
  0 siblings, 1 reply; 24+ messages in thread
From: Damien Robert @ 2020-02-28 17:24 UTC (permalink / raw)
  To: git, Jeff King; +Cc: 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)


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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-02-28 17:24 [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
@ 2020-02-28 18:23 ` Jeff King
  2020-03-01 22:05   ` Damien Robert
  0 siblings, 1 reply; 24+ messages in thread
From: Jeff King @ 2020-02-28 18:23 UTC (permalink / raw)
  To: Damien Robert; +Cc: J Wyman, git, Damien Robert

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

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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-02-28 18:23 ` Jeff King
@ 2020-03-01 22:05   ` Damien Robert
  2020-03-02 13:32     ` Jeff King
  2020-03-02 13:48     ` Jeff King
  0 siblings, 2 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-01 22:05 UTC (permalink / raw)
  To: Jeff King; +Cc: J Wyman, git

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

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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-03-01 22:05   ` Damien Robert
@ 2020-03-02 13:32     ` Jeff King
  2020-03-03 16:12       ` [PATCH v2 0/2] Damien Robert
  2020-03-03 16:16       ` [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
  2020-03-02 13:48     ` Jeff King
  1 sibling, 2 replies; 24+ messages in thread
From: Jeff King @ 2020-03-02 13:32 UTC (permalink / raw)
  To: Damien Robert; +Cc: git

[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


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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-03-01 22:05   ` Damien Robert
  2020-03-02 13:32     ` Jeff King
@ 2020-03-02 13:48     ` Jeff King
  2020-03-03 16:25       ` Damien Robert
  1 sibling, 1 reply; 24+ messages in thread
From: Jeff King @ 2020-03-02 13:48 UTC (permalink / raw)
  To: Damien Robert; +Cc: git

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

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

* [PATCH v2 0/2]
  2020-03-02 13:32     ` Jeff King
@ 2020-03-03 16:12       ` Damien Robert
  2020-03-03 16:12         ` [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch() Damien Robert
  2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
  2020-03-03 16:16       ` [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
  1 sibling, 2 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:12 UTC (permalink / raw)
  To: git, Jeff King; +Cc: 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)


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

* [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
  2020-03-03 16:12       ` [PATCH v2 0/2] Damien Robert
@ 2020-03-03 16:12         ` Damien Robert
  2020-03-03 17:51           ` Junio C Hamano
  2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
  1 sibling, 1 reply; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:12 UTC (permalink / raw)
  To: git, Jeff King; +Cc: 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)


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

* [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 16:12       ` [PATCH v2 0/2] Damien Robert
  2020-03-03 16:12         ` [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch() Damien Robert
@ 2020-03-03 16:12         ` Damien Robert
  2020-03-03 16:29           ` Damien Robert
                             ` (2 more replies)
  1 sibling, 3 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:12 UTC (permalink / raw)
  To: git, Jeff King; +Cc: 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)


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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-03-02 13:32     ` Jeff King
  2020-03-03 16:12       ` [PATCH v2 0/2] Damien Robert
@ 2020-03-03 16:16       ` Damien Robert
  1 sibling, 0 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:16 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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?

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

* Re: [PATCH 1/1] remote.c: fix handling of push:remote_ref
  2020-03-02 13:48     ` Jeff King
@ 2020-03-03 16:25       ` Damien Robert
  0 siblings, 0 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:25 UTC (permalink / raw)
  To: Jeff King; +Cc: git

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.

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

* Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
@ 2020-03-03 16:29           ` Damien Robert
  2020-03-03 18:29             ` Junio C Hamano
  2020-03-03 18:21           ` Junio C Hamano
  2020-03-12 16:45           ` [PATCH v3 1/1] " Damien Robert
  2 siblings, 1 reply; 24+ messages in thread
From: Damien Robert @ 2020-03-03 16:29 UTC (permalink / raw)
  To: git, Jeff King

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

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

* Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
  2020-03-03 16:12         ` [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch() Damien Robert
@ 2020-03-03 17:51           ` Junio C Hamano
  2020-03-03 21:11             ` Jeff King
  0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-03-03 17:51 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Jeff King, Damien Robert

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?

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

* Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
  2020-03-03 16:29           ` Damien Robert
@ 2020-03-03 18:21           ` Junio C Hamano
  2020-03-03 22:24             ` Damien Robert
  2020-03-12 16:45           ` [PATCH v3 1/1] " Damien Robert
  2 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-03-03 18:21 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Jeff King, Damien Robert

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?


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

* Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 16:29           ` Damien Robert
@ 2020-03-03 18:29             ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-03-03 18:29 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Jeff King

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.


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

* Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
  2020-03-03 17:51           ` Junio C Hamano
@ 2020-03-03 21:11             ` Jeff King
  2020-03-03 22:22               ` Junio C Hamano
  0 siblings, 1 reply; 24+ messages in thread
From: Jeff King @ 2020-03-03 21:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Damien Robert, git, Damien Robert

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

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

* Re: [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch()
  2020-03-03 21:11             ` Jeff King
@ 2020-03-03 22:22               ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-03-03 22:22 UTC (permalink / raw)
  To: Jeff King; +Cc: Damien Robert, git, Damien Robert

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.

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

* Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 18:21           ` Junio C Hamano
@ 2020-03-03 22:24             ` Damien Robert
  2020-03-03 22:48               ` Junio C Hamano
  0 siblings, 1 reply; 24+ messages in thread
From: Damien Robert @ 2020-03-03 22:24 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

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

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

* Re: [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref)
  2020-03-03 22:24             ` Damien Robert
@ 2020-03-03 22:48               ` Junio C Hamano
  0 siblings, 0 replies; 24+ messages in thread
From: Junio C Hamano @ 2020-03-03 22:48 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Jeff King

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.

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

* [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
  2020-03-03 16:29           ` Damien Robert
  2020-03-03 18:21           ` Junio C Hamano
@ 2020-03-12 16:45           ` " Damien Robert
  2020-03-25 22:16             ` Damien Robert
  2020-03-28 13:15             ` Jeff King
  2 siblings, 2 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-12 16:45 UTC (permalink / raw)
  To: git, Jeff King, Junio C Hamano; +Cc: 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)


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

* Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-12 16:45           ` [PATCH v3 1/1] " Damien Robert
@ 2020-03-25 22:16             ` Damien Robert
  2020-03-27 22:08               ` Junio C Hamano
  2020-03-28 13:15             ` Jeff King
  1 sibling, 1 reply; 24+ messages in thread
From: Damien Robert @ 2020-03-25 22:16 UTC (permalink / raw)
  To: git, Jeff King, Junio C Hamano

Hi Junio,

IMHO this patch should be good to cook.

Thanks!

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

* Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-25 22:16             ` Damien Robert
@ 2020-03-27 22:08               ` Junio C Hamano
  2020-03-28 22:25                 ` Damien Robert
  0 siblings, 1 reply; 24+ messages in thread
From: Junio C Hamano @ 2020-03-27 22:08 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Jeff King

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.



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

* Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-12 16:45           ` [PATCH v3 1/1] " Damien Robert
  2020-03-25 22:16             ` Damien Robert
@ 2020-03-28 13:15             ` Jeff King
  2020-03-28 13:31               ` Jeff King
  1 sibling, 1 reply; 24+ messages in thread
From: Jeff King @ 2020-03-28 13:15 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Junio C Hamano, Damien Robert

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

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

* Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-28 13:15             ` Jeff King
@ 2020-03-28 13:31               ` Jeff King
  0 siblings, 0 replies; 24+ messages in thread
From: Jeff King @ 2020-03-28 13:31 UTC (permalink / raw)
  To: Damien Robert; +Cc: git, Junio C Hamano, Damien Robert

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

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

* Re: [PATCH v3 1/1] remote.c: fix handling of %(push:remoteref)
  2020-03-27 22:08               ` Junio C Hamano
@ 2020-03-28 22:25                 ` Damien Robert
  0 siblings, 0 replies; 24+ messages in thread
From: Damien Robert @ 2020-03-28 22:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King

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

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

end of thread, back to index

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 17:24 [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
2020-02-28 18:23 ` Jeff King
2020-03-01 22:05   ` Damien Robert
2020-03-02 13:32     ` Jeff King
2020-03-03 16:12       ` [PATCH v2 0/2] Damien Robert
2020-03-03 16:12         ` [PATCH v2 1/2] remote: drop "explicit" parameter from remote_ref_for_branch() Damien Robert
2020-03-03 17:51           ` Junio C Hamano
2020-03-03 21:11             ` Jeff King
2020-03-03 22:22               ` Junio C Hamano
2020-03-03 16:12         ` [PATCH v2 2/2] remote.c: fix handling of %(push:remoteref) Damien Robert
2020-03-03 16:29           ` Damien Robert
2020-03-03 18:29             ` Junio C Hamano
2020-03-03 18:21           ` Junio C Hamano
2020-03-03 22:24             ` Damien Robert
2020-03-03 22:48               ` Junio C Hamano
2020-03-12 16:45           ` [PATCH v3 1/1] " Damien Robert
2020-03-25 22:16             ` Damien Robert
2020-03-27 22:08               ` Junio C Hamano
2020-03-28 22:25                 ` Damien Robert
2020-03-28 13:15             ` Jeff King
2020-03-28 13:31               ` Jeff King
2020-03-03 16:16       ` [PATCH 1/1] remote.c: fix handling of push:remote_ref Damien Robert
2020-03-02 13:48     ` Jeff King
2020-03-03 16:25       ` Damien Robert

Git Mailing List Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/git/0 git/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 git git/ https://lore.kernel.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.git


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git