From: Junio C Hamano <gitster@pobox•com>
To: Mike Hommey <mh@glandium•org>
Cc: git@vger•kernel.org, tboegi@web•de
Subject: Re: [PATCH v6 2/9] connect: only match the host with core.gitProxy
Date: Fri, 20 May 2016 14:48:28 -0700 [thread overview]
Message-ID: <xmqqbn40cser.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20160517013554.22578-3-mh@glandium.org> (Mike Hommey's message of "Tue, 17 May 2016 10:35:47 +0900")
Mike Hommey <mh@glandium•org> writes:
> Currently, core.gitProxy doesn't actually match purely on domain names
> as documented: it also matches ports.
>
> So a core.gitProxy value like "script for kernel.org" doesn't make the
> script called for an url like git://kernel.org:port/path, while it is
> called for git://kernel.org/path.
Let's recap. When you are accessing "git://kernel.org:9418/path"
- "script1 for kernel.org" does not match, which may be
counter-intuitive;
- "script2 for kernel.org:9418" matches and gets used.
Isn't that what the current code does? If so, I suspect that
allowing the first one to match may be an improvement with slight
backward incompatibility that we agreed that we do not mind.
Note for those other than Mike: The reason why we do not
care about the breakage of the backward compatibility is
because it would only matter when you configured both
"script1 for kernel.org" and "script2 for kernel.org:9418",
so that URL can be used to differenciate which proxy script
to use. In such a configuration, we used to call script2,
but "fixing" it would make it call script1. Given that the
proxy script takes the port number as its parameter, this
can be worked around easily by introducing a new script0
that switches between script1 and script2 based on the port
number parameter. IOW, we accept the compatibility breakage
because adjusting is easy.
We of course need to warn the user if we do this. That is
- If the URL we access has :<port>, and we apply <host>-only
core.gitProxy entry to it, and
- If the user has core.gitProxy entry for <host>:<port>,
we definitely need to warn so that the user is told that the "easy
adjustment" is necessary.
Thinking aloud a bit more, if the user has "script1 for kernel.org"
without "script2 for kernel.org:9418", and relied on the fact that
having :9418 that is not necessary (as it is the default port) in
the URL lets her bypass the proxy script, the user also needs to be
told that we have changed the rule of the game and her configuration
needs to be updated. So the above condition to warn would need to
be loosened, i.e. "if the URL being accessed has :<port>, and if we
apply gitProxy specified with only <host>" we need to warn.
A proper transition plan is necessary; if the long term ideal is to
use "script1 for kernel.org" for "git://kernel.org:9418/path", we do
not want to keep giving this warning forever.
And if we need a transition plan _anyway_, we probably should have a
period during which those who have been relying on the fact that
"script2 for kernel.org:9418" was a supported way to specify proxy
for "git://kernel.org:9418/path" would get a warning but will still
use "script2 for kernel.org:9418" as a valid core.gitProxy
specification.
> This per-port behavior seems like an oversight rather than a deliberate
> choice, so, make git://kernel.org:port/path call the gitProxy script in
> the case described above.
So, even if we agree that per-port behaviour is not something we
would use if we were building the system without any existing users
today, I do not think we want "git now fails with an error" at all.
It goes against the approach Git takes to give smooth transtion to
users when we must break backward compatibility.
> However, if people have been relying on this behavior, git now fails
> with an error message inviting for a configuration change.
next prev parent reply other threads:[~2016-05-20 21:48 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-17 1:35 [PATCH v6 0/9] connect: various cleanups Mike Hommey
2016-05-17 1:35 ` [PATCH v6 1/9] connect: call get_host_and_port() earlier Mike Hommey
2016-05-20 20:58 ` Junio C Hamano
2016-05-20 22:14 ` Mike Hommey
2016-05-20 22:20 ` Junio C Hamano
2016-05-20 22:28 ` Mike Hommey
2016-05-17 1:35 ` [PATCH v6 2/9] connect: only match the host with core.gitProxy Mike Hommey
2016-05-20 21:48 ` Junio C Hamano [this message]
2016-05-20 22:30 ` Mike Hommey
2016-05-20 22:56 ` Junio C Hamano
2016-05-17 1:35 ` [PATCH v6 3/9] connect: fill the host header in the git protocol with the host and port variables Mike Hommey
2016-05-17 1:35 ` [PATCH v6 4/9] connect: make parse_connect_url() return separated host and port Mike Hommey
2016-05-17 1:35 ` [PATCH v6 5/9] connect: group CONNECT_DIAG_URL handling code Mike Hommey
2016-05-17 1:35 ` [PATCH v6 6/9] connect: make parse_connect_url() return the user part of the url as a separate value Mike Hommey
2016-05-17 1:35 ` [PATCH v6 7/9] connect: change the --diag-url output to separate user and host Mike Hommey
2016-05-17 1:35 ` [PATCH v6 8/9] connect: actively reject git:// urls with a user part Mike Hommey
2016-05-17 1:35 ` [PATCH v6 9/9] connect: move ssh command line preparation to a separate function Mike Hommey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqbn40cser.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=mh@glandium$(echo .)org \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox