Re: [PATCH v3 2/2] remote: find tracking branches for URL push destinations

[Junio C Hamano <gitster@pobox.com>]

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

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Git accepts a repository URL as branch.<name>.pushRemote and can push
> to it. This branch setting takes precedence over remote.pushDefault.
>
> A branch can be configured with a URL-valued pushRemote before any push
> occurs. If the remotes are later rearranged with "git remote rename" and
> "git remote add", the newly added remote may use that URL. The URL value
> is unaffected by the rename and continues to take precedence over
> remote.pushDefault. The URL and the remote then point to the same
> repository, but Git does not connect them for tracking. Pushing works,
> but @{push} cannot identify the remote's tracking branch. As a result,
> "git status" cannot show the push branch, and an up-to-date push can
> leave its tracking information stale.
>
> When exactly one configured remote uses the push destination URL, use
> that remote for push tracking. Continue to push to the URL so the
> configured remote's push settings do not change existing behavior. Keep
> the current behavior when no remote matches or multiple remotes match.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
> ---
>  Documentation/config/branch.adoc |   1 +
>  Documentation/revisions.adoc     |   3 +
>  remote.c                         |  34 ++++++++-
>  remote.h                         |   2 +
>  t/t5505-remote.sh                | 124 +++++++++++++++++++++++++++++++
>  transport.c                      |   5 +-
>  6 files changed, 167 insertions(+), 2 deletions(-)

Hmph, the changes since the previous round look a bit incoherent.

> diff --git a/Documentation/config/branch.adoc b/Documentation/config/branch.adoc
> index a4db9fa5c8..5a85fde8de 100644
> --- a/Documentation/config/branch.adoc
> +++ b/Documentation/config/branch.adoc
> @@ -55,6 +55,7 @@ This option defaults to `never`.
>  	repository), you would want to set `remote.pushDefault` to
>  	specify the remote to push to for all branches, and use this
>  	option to override it for a specific branch.
> +	The value may be the name of a configured remote or a repository URL.
>  
>  `branch.<name>.merge`::
>  	Defines, together with `branch.<name>.remote`, the upstream branch
> diff --git a/Documentation/revisions.adoc b/Documentation/revisions.adoc
> index 6ea6c7cead..78f96fe8b0 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 destination is a URL and exactly one configured remote has
> +  that URL among its `remote.<name>.url` values, '@\{push}' reports that
> +  remote's remote-tracking branch.

We claim we use remote.<name>.url here.

>  Here's an example to make it more clear:
>  +
> diff --git a/remote.c b/remote.c
> index 0dc36956c3..4a29669443 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1887,13 +1887,45 @@ 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)
> +{
> +	const struct strvec *push_urls;
> +	struct remote *first_match = NULL;
> +	struct remote_state *remote_state = repo->remote_state;
> +	const char *check_url;
> +
> +	if (remote->origin != REMOTE_UNCONFIGURED)
> +		return remote;
> +
> +	push_urls = push_url_of_remote(remote);
> +	if (push_urls->nr != 1)
> +		return remote;
> +	check_url = push_urls->v[0];

But we correctly pay attention to both .url and .pushurl, giving
precedence to the latter.

> +	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, check_url))
> +			continue;
> +		if (first_match)
> +			return remote;
> +		first_match = candidate;
> +	}
> +
> +	return first_match ? first_match : remote;
> +}

> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index 6f5e86dede..983aff6552 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="file://$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,108 @@ 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 matching 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
> +'

But the test does not seem to exercise remote.<name>.pushURL
anywhere.

> +test_expect_success 'pushInsteadOf URL pushRemote is trackable' '
> +	setup_url_pushremote &&
> +	(
> +		cd client &&
> +		git remote rename origin upstream &&
> +		git remote add -f origin "$fork_url" &&
> +		git config "url.$fork_url.pushInsteadOf" fork: &&
> +		git config branch.topic.pushRemote fork:
> +	) &&

Testing insteadof is a nice touch, though.

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

So is a test that checks non-unique case where the machinery should
not kick in.

Thanks.