Re: [PATCH] push: Alias pushurl from push rewrites

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 27, 2013, 23:08 UTC
Message-ID: <7vfvzgjv9d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130327174259.373bafe1@hoelz.ro>

>> Subject: Re: [PATCH] push: Alias pushurl from push rewrites

Please increment [PATCH v$n] for a new round, so that we can tell which one is the latest.

Rob Hoelz <rob@hoelz.ro> writes:

> git push currently doesn't consider pushInsteadOf when
> using pushurl; this test tests that.

This patch is no longer "this test", and I thought you are changing the behaviour so that the command takes it into account.

Show 12 quoted lines

> If you use pushurl with an alias that has a pushInsteadOf configuration
> value, Git does not take advantage of it.  For example:
>
> [url "git://github.com/"]
>     insteadOf = github:
> [url "git://github.com/myuser/"]
>     insteadOf = mygithub:
> [url "git@github.com:myuser/"]
>     pushInsteadOf = mygithub:
> [remote "origin"]
>     url     = github:organization/project
>     pushurl = mygithub:project

Perhaps indent the above a bit to make it more readable?

But more importantly, isn't this a variant of what we discussed in a separate thread about "triangular workflow", where you pull from one place (org/project) and push to another (project)? I thought the conclusion from the discussion was that using url/pushurl to call two logically diffent repositories with the same name is wrong. For one thing, the "pretend as if we immediately fetched" update of remote tracking branches will go out of sync, so the above is a broken configuration, with or without pushInsteadOf.

Show 6 quoted lines

> With the above configuration, the following occurs:
>
> $ git push origin master
> fatal: remote error:
>   You can't push to git://github.com/myuser/project.git
>   Use git@github.com:myuser/project.git

Yup, that is a documented behaviour.

Show 6 quoted lines

> So you can see that pushurl is being followed (it's not attempting to
> push to git://github.com/organization/project), but insteadOf values are
> being used as opposed to pushInsteadOf values for expanding the pushurl
> alias.
>
> This commit fixes that.

Saying "fixes" before justifying why such a patch that changes a documented behaviour is a good idea is a bit weak, to say the least.

Care to justify with a non-triangular example, where origin is associated to logically the same repository?

That is, currently you can do either:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = github:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        pushInsteadOf = github:

     ; The repository
     [remote "origin"]
        url = github:project

or:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = githubf:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        insteadOf = githubp:

     ; The repository
     [remote "origin"]
        url = githubf:project
        pushUrl = githubp:project

You would need to make a convincing argument to justify why allowing:

     ; Fetch anonymously
     [url "git://github.com/me/"]
        insteadOf = github:

     ; Pushing needs to go over ssh
     [url "git@github.com:me/"]
        pushInsteadOf = github:

     ; The repository
     [remote "origin"]
        url = github:project
        pushUrl = github:project

is a good idea.

I also suspect there could be people who rely on the documented behaviour; they can manually rewrite their pushURL to what insteadOf setting has been rewriting to work it around, but to them, this change may be a needless regression. I do not offhand how severe a regression it is, though.