Re: What's cooking in git.git (Apr 2020, #01; Wed, 15)

[Damien Robert <damien.olivier.robert@gmail.com>]

From Junio C Hamano, Thu 16 Apr 2020 at 16:34:22 (-0700) :
> > Here were the two reasons for the RFC of this patch e3165570dfca690ea1a71518799153f6350777ae
> > ...
> > This means that I get the fallback of 'origin' if no remote is specified.
> > So if I set a pushRemote="foobar" but no remote, then remote.c will
> > consider we are in a triangular workflow but git push will not.
 
> OK, so in short, what is queued in 'next' is quite borked X-<.  I
> don't mind reverting the merge then.

Yes sorry, I never meant for this patch version to be queued up, hence its rfc
status :/

The reason I sent it as is, as outlined by my cover letter, was that I
found it quite surprising that a pushRemote without remote was not
considered by 'git push' as a triangular workflow. Technically a triangular
workflow is when the push remote is different from the fetch remote, and we
could argue when there is no fetch remote it is indeed the case (I know
that was what I was expecting). So I was wondering if we should not change
this logic in git push instead.

I'll let you decide if you prefer reverting this merge, or applying the
following fixup on top of next instead.

--- 8< ---
From 2f9268e2fb6ee280137fb928180882619eb9c3e5 Mon Sep 17 00:00:00 2001
From: Damien Robert <damien.olivier.robert+git@gmail.com>
Date: Fri, 17 Apr 2020 14:41:02 +0200
Subject: [PATCH 1/1] remote.c: fix detection of triangular workflow

When a branch has a pushRemote but no remote, then git push does not
consider this as a triangular workflow.

In remote.c, since is_workflow_triangular does not check for the *explicit
value, it considers that such a branch is in a triangular workflow
(except when 'pushRemote=origin' since the default non explicit value of
fetch_remote is 'origin').

Fix that by checking the values of explicit in remote_for_branch and
pushremote_for_branch, and add tests.

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

diff --git a/remote.c b/remote.c
index 7c99469598..18a190198a 100644
--- a/remote.c
+++ b/remote.c
@@ -1636,9 +1636,12 @@ static const char *tracking_for_push_dest(struct remote *remote,
 
 static int is_workflow_triangular(struct branch *branch)
 {
-	struct remote *fetch_remote = remote_get(remote_for_branch(branch, NULL));
-	struct remote *push_remote = remote_get(pushremote_for_branch(branch, NULL));
-	return (fetch_remote && push_remote && fetch_remote != push_remote);
+	int explicit;
+	struct remote *fetch_remote = remote_get(remote_for_branch(branch, &explicit));
+	if (!explicit || !fetch_remote)
+		return 0;
+	struct remote *push_remote = remote_get(pushremote_for_branch(branch, &explicit));
+	return (explicit && push_remote && fetch_remote != push_remote);
 }
 
 /**
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 8e59ab2567..4c01d4406f 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -945,6 +945,38 @@ test_expect_success '%(push) and %(push:remoteref)' '
 			--format="%(push:remotename),%(push:remoteref),%(push)" \
 			refs/heads/master)" &&
 		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		actual="$(git -c push.default=nothing for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		git config --unset branch.master.remote &&
+		git config --unset branch.master.merge &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		git config branch.master.merge refs/heads/master &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		git config branch.master.merge refs/heads/other &&
+		actual="$(git -c push.default=simple for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		actual="$(git -c push.default=upstream for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,, = "$actual" &&
+		actual="$(git -c push.default=current for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
+		actual="$(git -c push.default=matching for-each-ref \
+			--format="%(push:remotename),%(push:remoteref),%(push)" \
+			refs/heads/master)" &&
+		test to,refs/heads/master,refs/remotes/to/master = "$actual" &&
 		actual="$(git -c push.default=nothing for-each-ref \
 			--format="%(push:remotename),%(push:remoteref),%(push)" \
 			refs/heads/master)" &&
-- 
Patched on top of v2.26.1-301-g55bc3eb7cb (git version 2.26.0)