[PATCH 0/2] remote: resolve url push tracking

[PATCH 0/2] remote: resolve url push tracking

[Harald Nordgren via GitGitGadget]

After renaming remotes, git status may stop showing the push branch even
though Git still pushes to the right URL, use the remote with the same URL
to find it.

Harald Nordgren (2):
  remote: pass repository to push tracking helper
  remote: resolve URL-valued push tracking remotes

 Documentation/revisions.adoc |   3 +
 remote.c                     |  36 ++++++++++--
 remote.h                     |   2 +
 t/t5505-remote.sh            | 104 +++++++++++++++++++++++++++++++++++
 transport.c                  |   5 +-
 5 files changed, 144 insertions(+), 6 deletions(-)


base-commit: 41365c2a9ba347870b80881c0d67454edd22fd49
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2358%2FHaraldNordgren%2Fremote-resolve-url-push-tracking-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2358/HaraldNordgren/remote-resolve-url-push-tracking-v1
Pull-Request: https://github.com/git/git/pull/2358
-- 
gitgitgadget

[PATCH 1/2] remote: pass repository to push tracking helper

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

The push tracking helper currently only needs the push remote. However,
resolving a URL-valued remote requires access to the repository's list
of configured remotes.

Pass the repository through the existing callers and mark the parameter
as unused for now. This prepares the helper for that lookup without
changing its behavior.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 remote.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/remote.c b/remote.c
index e6c52c850c..89d0f9e2d8 100644
--- a/remote.c
+++ b/remote.c
@@ -1887,7 +1887,8 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 	return branch->merge[0]->dst;
 }
 
-static char *tracking_for_push_dest(struct remote *remote,
+static char *tracking_for_push_dest(struct repository *repo UNUSED,
+				    struct remote *remote,
 				    const char *refname,
 				    struct strbuf *err)
 {
@@ -1925,13 +1926,13 @@ static char *branch_get_push_1(struct repository *repo,
 					 _("push refspecs for '%s' do not include '%s'"),
 					 remote->name, branch->name);
 
-		ret = tracking_for_push_dest(remote, dst, err);
+		ret = tracking_for_push_dest(repo, remote, dst, err);
 		free(dst);
 		return ret;
 	}
 
 	if (remote->mirror)
-		return tracking_for_push_dest(remote, branch->refname, err);
+		return tracking_for_push_dest(repo, remote, branch->refname, err);
 
 	switch (push_default) {
 	case PUSH_DEFAULT_NOTHING:
@@ -1939,7 +1940,7 @@ static char *branch_get_push_1(struct repository *repo,
 
 	case PUSH_DEFAULT_MATCHING:
 	case PUSH_DEFAULT_CURRENT:
-		return tracking_for_push_dest(remote, branch->refname, err);
+		return tracking_for_push_dest(repo, remote, branch->refname, err);
 
 	case PUSH_DEFAULT_UPSTREAM:
 		return xstrdup_or_null(branch_get_upstream(branch, err));
@@ -1953,7 +1954,7 @@ static char *branch_get_push_1(struct repository *repo,
 			up = branch_get_upstream(branch, err);
 			if (!up)
 				return NULL;
-			cur = tracking_for_push_dest(remote, branch->refname, err);
+			cur = tracking_for_push_dest(repo, remote, branch->refname, err);
 			if (!cur)
 				return NULL;
 			if (strcmp(cur, up)) {
-- 
gitgitgadget

[PATCH 2/2] remote: resolve URL-valued push tracking remotes

[Harald Nordgren via GitGitGadget]

From: Harald Nordgren <haraldnordgren@gmail.com>

A branch may name its push destination with a URL instead of a
configured remote. This is useful in fork workflows, where the original
remote is renamed to "upstream", the fork is added as "origin", and an
existing branch.<name>.pushRemote continues to contain the fork URL.

Git can still push through the anonymous remote created for that URL.
However, the anonymous remote has no fetch refspec. Git therefore cannot
resolve @{push} to origin/<branch> or update that remote-tracking branch
after a push. The push can succeed, or report that everything is up to
date, while status continues to compare against a stale tracking ref or
cannot show the push branch at all.

A uniquely matching configured remote already provides the missing
mapping. Use its fetch refspec when resolving the push tracking branch
and when updating tracking refs after a push. This changes neither the
push destination nor configuration. Keep the existing behavior when no
remote matches or multiple remotes share the URL, since either case is
ambiguous.

Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
---
 Documentation/revisions.adoc |   3 +
 remote.c                     |  27 ++++++++-
 remote.h                     |   2 +
 t/t5505-remote.sh            | 104 +++++++++++++++++++++++++++++++++++
 transport.c                  |   5 +-
 5 files changed, 139 insertions(+), 2 deletions(-)

diff --git a/Documentation/revisions.adoc b/Documentation/revisions.adoc
index 6ea6c7cead..b691691c8c 100644
--- a/Documentation/revisions.adoc
+++ b/Documentation/revisions.adoc
@@ -127,6 +127,9 @@ some output processing may assume ref names in UTF-8.
   `git push` were run while `branchname` was checked out (or the current
   `HEAD` if no branchname is specified). Like for '@\{upstream\}', we report
   the remote-tracking branch that corresponds to that branch at the remote.
+  If the push remote is specified as a URL, the fetch refspec of a uniquely
+  matching configured remote is used to find and update the remote-tracking
+  branch.
 +
 Here's an example to make it more clear:
 +
diff --git a/remote.c b/remote.c
index 89d0f9e2d8..03908dfe8d 100644
--- a/remote.c
+++ b/remote.c
@@ -1887,13 +1887,38 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
 	return branch->merge[0]->dst;
 }
 
-static char *tracking_for_push_dest(struct repository *repo UNUSED,
+struct remote *repo_remote_for_push_tracking(struct repository *repo,
+					     struct remote *remote)
+{
+	struct remote *first_match = NULL;
+	struct remote_state *remote_state = repo->remote_state;
+
+	if (remote->origin != REMOTE_UNCONFIGURED || remote->url.nr != 1)
+		return remote;
+
+	for (int i = 0; i < remote_state->remotes_nr; i++) {
+		struct remote *candidate = remote_state->remotes[i];
+
+		if (!candidate || candidate == remote ||
+		    !remote_is_configured(candidate, 0) ||
+		    !remote_has_url(candidate, remote->url.v[0]))
+			continue;
+		if (first_match)
+			return remote;
+		first_match = candidate;
+	}
+
+	return first_match ? first_match : remote;
+}
+
+static char *tracking_for_push_dest(struct repository *repo,
 				    struct remote *remote,
 				    const char *refname,
 				    struct strbuf *err)
 {
 	char *ret;
 
+	remote = repo_remote_for_push_tracking(repo, remote);
 	ret = apply_refspecs(&remote->fetch, refname);
 	if (!ret)
 		return error_buf(err,
diff --git a/remote.h b/remote.h
index 72a54d84ad..cca02033b9 100644
--- a/remote.h
+++ b/remote.h
@@ -345,6 +345,8 @@ char *remote_ref_for_branch(struct branch *branch, int for_push);
 
 const char *repo_default_remote(struct repository *repo);
 const char *repo_remote_from_url(struct repository *repo, const char *url);
+struct remote *repo_remote_for_push_tracking(struct repository *repo,
+					     struct remote *remote);
 
 /* returns true if the given branch has merge configuration given. */
 int branch_has_merge_config(struct branch *branch);
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index e592c0bcde..e16b3f320a 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -24,6 +24,28 @@ setup_repository () {
 	)
 }
 
+setup_url_pushremote () {
+	rm -rf fork.git client &&
+	git clone --bare one fork.git &&
+	git clone one client &&
+	fork_url="$TRASH_DIRECTORY/fork.git" &&
+	(
+		cd client &&
+		git checkout -b topic --track origin/main &&
+		git commit --allow-empty -m topic-change &&
+		git config push.default current &&
+		git config status.compareBranches "@{upstream} @{push}" &&
+		git config branch.topic.pushRemote "$fork_url" &&
+		git push
+	)
+}
+
+check_status () {
+	git -C client status >actual &&
+	cat >expected &&
+	test_cmp expected actual
+}
+
 tokens_match () {
 	echo "$1" | tr ' ' '\012' | sort | sed -e '/^$/d' >expect &&
 	echo "$2" | tr ' ' '\012' | sort | sed -e '/^$/d' >actual &&
@@ -1018,6 +1040,88 @@ test_expect_success 'rename a remote renames repo remote.pushDefault but keeps g
 	)
 '
 
+test_expect_success 'URL-valued pushRemote without matching remote is not trackable' '
+	setup_url_pushremote &&
+
+	check_status <<-EOF
+	On branch topic
+	Your branch is ahead of ${SQ}origin/main${SQ} by 1 commit.
+	  (use "git push" to publish your local commits)
+
+	nothing to commit, working tree clean
+	EOF
+'
+
+test_expect_success 'adding fork remote makes URL-valued pushRemote trackable' '
+	setup_url_pushremote &&
+
+	(
+		cd client &&
+		git remote rename origin upstream &&
+		git remote add -f origin "$fork_url"
+	) &&
+
+	check_status <<-EOF
+	On branch topic
+	Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
+
+	Your branch is up to date with ${SQ}origin/topic${SQ}.
+
+	nothing to commit, working tree clean
+	EOF
+'
+
+test_expect_success 'up-to-date URL push refreshes stale tracking branch' '
+	setup_url_pushremote &&
+	(
+		cd client &&
+		git remote rename origin upstream &&
+		git remote add -f origin "$fork_url" &&
+		git commit --allow-empty -m another-topic-change &&
+		git -C ../fork.git fetch ../client topic:topic
+	) &&
+
+	check_status <<-EOF &&
+	On branch topic
+	Your branch is ahead of ${SQ}upstream/main${SQ} by 2 commits.
+
+	Your branch is ahead of ${SQ}origin/topic${SQ} by 1 commit.
+	  (use "git push" to publish your local commits)
+
+	nothing to commit, working tree clean
+	EOF
+
+	git -C client push >actual 2>&1 &&
+	test_grep "Everything up-to-date" actual &&
+
+	check_status <<-EOF
+	On branch topic
+	Your branch is ahead of ${SQ}upstream/main${SQ} by 2 commits.
+
+	Your branch is up to date with ${SQ}origin/topic${SQ}.
+
+	nothing to commit, working tree clean
+	EOF
+'
+
+test_expect_success 'duplicate remote URL leaves URL-valued pushRemote ambiguous' '
+	setup_url_pushremote &&
+	(
+		cd client &&
+		git remote rename origin upstream &&
+		git remote add -f origin "$fork_url" &&
+		git remote add duplicate "$fork_url"
+	) &&
+
+	check_status <<-EOF
+	On branch topic
+	Your branch is ahead of ${SQ}upstream/main${SQ} by 1 commit.
+	  (use "git push" to publish your local commits)
+
+	nothing to commit, working tree clean
+	EOF
+'
+
 test_expect_success 'rename handles remote without fetch refspec' '
 	git clone --bare one no-refspec.git &&
 	# confirm assumption that bare clone does not create refspec
diff --git a/transport.c b/transport.c
index fc144f0aed..30a4ab2cd5 100644
--- a/transport.c
+++ b/transport.c
@@ -1553,8 +1553,11 @@ int transport_push(struct repository *r,
 	if (!(flags & (TRANSPORT_PUSH_DRY_RUN |
 		       TRANSPORT_RECURSE_SUBMODULES_ONLY))) {
 		struct ref *ref;
+		struct remote *tracking_remote = repo_remote_for_push_tracking(
+			r, transport->remote);
+
 		for (ref = remote_refs; ref; ref = ref->next)
-			transport_update_tracking_ref(transport->remote, ref, verbose);
+			transport_update_tracking_ref(tracking_remote, ref, verbose);
 	}
 
 	if (porcelain && !push_ret)
-- 
gitgitgadget

Re: [PATCH 1/2] remote: pass repository to push tracking helper

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> The push tracking helper currently only needs the push remote. However,
> resolving a URL-valued remote requires access to the repository's list
> of configured remotes.

It is unclear to me what 'resolving a URL-valued remote' means.
Could you describe what you are trying to achieve, without relying
on unexplained terms like 'to resolve' and 'URL-valued remote',
which seem to carry specialized meanings in this context?

Thanks.

> Pass the repository through the existing callers and mark the parameter
> as unused for now. This prepares the helper for that lookup without
> changing its behavior.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>  remote.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/remote.c b/remote.c
> index e6c52c850c..89d0f9e2d8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1887,7 +1887,8 @@ const char *branch_get_upstream(struct branch *branch, struct strbuf *err)
>  	return branch->merge[0]->dst;
>  }
>  
> -static char *tracking_for_push_dest(struct remote *remote,
> +static char *tracking_for_push_dest(struct repository *repo UNUSED,
> +				    struct remote *remote,
>  				    const char *refname,
>  				    struct strbuf *err)
>  {
> @@ -1925,13 +1926,13 @@ static char *branch_get_push_1(struct repository *repo,
>  					 _("push refspecs for '%s' do not include '%s'"),
>  					 remote->name, branch->name);
>  
> -		ret = tracking_for_push_dest(remote, dst, err);
> +		ret = tracking_for_push_dest(repo, remote, dst, err);
>  		free(dst);
>  		return ret;
>  	}
>  
>  	if (remote->mirror)
> -		return tracking_for_push_dest(remote, branch->refname, err);
> +		return tracking_for_push_dest(repo, remote, branch->refname, err);
>  
>  	switch (push_default) {
>  	case PUSH_DEFAULT_NOTHING:
> @@ -1939,7 +1940,7 @@ static char *branch_get_push_1(struct repository *repo,
>  
>  	case PUSH_DEFAULT_MATCHING:
>  	case PUSH_DEFAULT_CURRENT:
> -		return tracking_for_push_dest(remote, branch->refname, err);
> +		return tracking_for_push_dest(repo, remote, branch->refname, err);
>  
>  	case PUSH_DEFAULT_UPSTREAM:
>  		return xstrdup_or_null(branch_get_upstream(branch, err));
> @@ -1953,7 +1954,7 @@ static char *branch_get_push_1(struct repository *repo,
>  			up = branch_get_upstream(branch, err);
>  			if (!up)
>  				return NULL;
> -			cur = tracking_for_push_dest(remote, branch->refname, err);
> +			cur = tracking_for_push_dest(repo, remote, branch->refname, err);
>  			if (!cur)
>  				return NULL;
>  			if (strcmp(cur, up)) {

Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes

[Junio C Hamano]

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> A branch may name its push destination with a URL instead of a
> configured remote. This is useful in fork workflows, where the original
> remote is renamed to "upstream", the fork is added as "origin", and an
> existing branch.<name>.pushRemote continues to contain the fork URL.
>
> Git can still push through the anonymous remote created for that URL.
> However, the anonymous remote has no fetch refspec. Git therefore cannot
> resolve @{push} to origin/<branch> or update that remote-tracking branch
> after a push. The push can succeed, or report that everything is up to
> date, while status continues to compare against a stale tracking ref or
> cannot show the push branch at all.

Let me try to think aloud, rephrasing the explanation with a
slightly more concrete illustration, to see whether I understand
what you are trying to achieve.

The current system allows you to set:

     [branch "mytopic"]
        pushRemote = https://hosting.site/users/me/mine.git/
     [remote "notlinked"]
        url = https://hosting.site/users/me/mine.git/
        push = refs/heads/mytopic
        fetch = refs/heads/*:refs/remotes/notlinked/*

but when on the 'mytopic' branch, @{push} cannot determine which
branch at the remote repository to update, so it cannot map it back
to our remote-tracking branch ('refs/remotes/notlinked/mytopic' in
the above illustration).

A question.  Do we currently accept a string that is not a remote
name as the value for 'branch.<name>.pushRemote' by design?

The 'git config --help' output explains that:

 - 'branch.<name>.pushRemote' overrides 'branch.<name>.remote' and
   'remote.pushDefault'; and

 - 'branch.<name>.remote' and 'remote.pushDefault' tell 'git fetch'
   and 'git push' which remote to work with.

It therefore seems clear that setting a string that is not a remote
name (such as a URL) as the value for these three variables is a
misconfiguration in the current system.

I am not saying that it should stay that way forever.  But please
re-read your first sentence and tell me whether it is clear that the
patch extends the current system with a new feature.  It was far
from clear to me and caused significant confusion.  Writing it like
this:

    Under the current system, a branch cannot name its push
    destination using a URL.  If we were to extend the system
    to allow this, such and such benefits would become
    possible.

would have been far less confusing.

If that is what you are doing, that is.

> A uniquely matching configured remote already provides the missing
> mapping.

A very good consideration.  It was the first thing that came to my
mind while I was thinking aloud, constructing an illustration with
'notlinked', wondering "what if there is another remote, with the
same URL, but different 'push' configuration?".

> Use its fetch refspec when resolving the push tracking branch
> and when updating tracking refs after a push.

Is this not needless, and is mentioning it not confusing?  If I
understand correctly, what the change entails is:

 * If the value of 'branch.<name>.pushRemote' (call it X) is 'not' a
   remote name, try to see whether there is a unique remote that
   has either (1) a 'pushurl' whose value matches X, or (2) no
   'pushurl' but a 'url' whose value matches X.  If no such remote
   exists, simply abort and refuse to proceed.

 * If there is such a remote, pretend that the value of
   'branch.<name>.pushRemote' were the name of that remote, and do
   everything else as usual.

And mapping the current branch name to its push destination via
'remote.<name>.push' to find the name of the destination branch at
the remote, and then mapping it back to our remote-tracking branch
using 'remote.<name>.fetch', is not something new that this topic
needs to update, no?

Thanks.  Once I understand what you are trying to achieve, I will
offer further comments on the implementation, as I find this topic
potentially quite interesting.

Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes

[Harald Nordgren]

Thanks for your continued support on all my topics!

Yes, I should clarify in the commit message what the actual motivation
is, which is for me to handle remote renames in a smoother way, since
'gh' renmames remotes when forking a repo which is messing with
@{push} and compareBranches for 'git status'.


Harald

Re: [PATCH 2/2] remote: resolve URL-valued push tracking remotes

[Junio C Hamano]

Harald Nordgren <haraldnordgren@gmail.com> writes:

> Thanks for your continued support on all my topics!
>
> Yes, I should clarify in the commit message what the actual motivation
> is, which is for me to handle remote renames in a smoother way, since
> 'gh' renmames remotes when forking a repo which is messing with
> @{push} and compareBranches for 'git status'.


Yeah, it would be a good thing to do in an updated version.

Thanks.