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

From: Damien Robert <damien.olivier.robert@gmail.com>
Date: Apr 18, 2020, 17:36 UTC
Message-ID: <20200418173651.djzriazxj5kbo6ax@doriath>
In-Reply-To: <xmqq8sitvgtd.fsf@gitster.c.googlers.com>

From Junio C Hamano, Fri 17 Apr 2020 at 16:22:06 (-0700) :

Show 6 quoted lines

> Ah, of course ;-) The code in builtin/push.c rely on being able to
> pass NULL as the name and rely on current branch getting used; you
> have to pass the name of the ref you are trying to format %(push)
> for, so you would trigger add_url_alias(), which says as a fallback
> that URL for "origin" is "origin" and makes ret->url non-NULL (hence
> it no longer is !valid_remote() and gets returned).

Indeed, if 'origin' is implicit and does not exists, then remote_get(NULL) returns NULL, while remote_get(remote_for_branch(current_branch, NULL)) returns the 'origin' remote

Likewise pushremote_get(NULL) will differ from remote_get(pushremote_for_branch(current_branch, NULL)) if 'origin' is the implicit push remote and does not exists.

In particular, there is currently no way to check with remote_get("origin") if an implicit 'origin' really exists or not.

What we would need to get the same results is an extra parameter to remote_get_1 which tells it if the given name was obtained explicitly or implicitly.

Now this induces some bugs in the ref-filter machinery. For instance, for %(upstream:remotename) and %(push:remotename), the code prints `remote_for_branch(branch, &explicit)` and `pushremote_for_branch(branch, &explicit)` respectively only if they are explicit.

But this is not quite correct, if we think of %(upstream:remotename) as to "which remote would a bare `git fetch` contact when on this branch?"; then 'origin' should be printed, provided it really exists.

> Geez.  This is tricky.
> > But I think that means that my fixup is actually wrong when a pushRemote is
> > set without a remote while 'origin' do exist.

So here is a test done about whether a triangular setup is detected, when a
branch has a pushRemote but no remote and
1) pushRemote=foobar, origin does not exists
2) pushRemote=foobar, origin does exists
3) pushRemote=origin, origin does not exists
4) pushRemote=origin, origin exists
in the following cases:
A) `git push` B) "%(push)" with the code currently in next 
C) "%(push)" with the fixup I sent D) what I would have expected

  A   B   C   D
1 no  yes no  yes
2 yes yes no  yes
3 no  no  no  no
4 no  no  no  no

Assuming D is indeed the correct way to go, there are two ways to fix
is_workflow_triangular in push.c (not tested): one is to replace
	return (fetch_remote && fetch_remote != remote);
by
	return (!fetch_remote || fetch_remote != remote);
Indeed in this case we know that the push remote exists, so if the
fetch_remote does not exists, we know we are in a triangular workflow.

Another way would be to call 
  fetch_remotename=remote_for_branch(current_branch, &explicit);
and compare it with remote->name.

Going back to my 'fixup', it is clearly wrong (in the opposite direction of what is currently in next!). But assuming that 'D' is what we want for triangular workflows, the patch in next is actually correct and it is `git push` who is wrong :)

About the patch currently in next, apart from this situation which is tricky to fix as you observed, one thing I may have changed if you had not commited it already is to change static int is_workflow_triangular(struct branch *branch) into static int is_workflow_triangular(struct branch *branch, struct remote *push_remote) since all the callers already have the push_remote, this would prevent us from recomputing it. But this can be a separate fix, if you think this does not warrant a revert of the merge.

PS: While I am on the subject of bugs in ref-filter.c, for exhaustivity,
in push.c `setup_push_upstream` checks that
	(branch->merge_nr != 1)
but this is not done for %(push)