insteadOf and git-request-pull output

insteadOf and git-request-pull output

[Konstantin Ryabitsev]

Hi, all:

Looks like setting url.insteadOf rules alters the output of
git-request-pull. I'm not sure that's the intended use of insteadOf,
which is supposed to replace URLs for local use, not to expose them
publicly (but I may be wrong). E.g.:

$ git request-pull HEAD^ git://foo.example.com/example | grep example
  git://foo.example.com/example

$ git config url.ssh://bar.insteadOf git://foo

$ git request-pull HEAD^ git://foo.example.com/example | grep example
  ssh://bar.example.com/example

I think that if we use the "principle of least surprise," insteadOf
rules shouldn't be applied for git-request-pull URLs.

Best,
-K

Re: insteadOf and git-request-pull output

[Ævar Arnfjörð Bjarmason]

On Thu, Nov 15 2018, Konstantin Ryabitsev wrote:

> Hi, all:
>
> Looks like setting url.insteadOf rules alters the output of
> git-request-pull. I'm not sure that's the intended use of insteadOf,
> which is supposed to replace URLs for local use, not to expose them
> publicly (but I may be wrong). E.g.:
>
> $ git request-pull HEAD^ git://foo.example.com/example | grep example
>   git://foo.example.com/example
>
> $ git config url.ssh://bar.insteadOf git://foo
>
> $ git request-pull HEAD^ git://foo.example.com/example | grep example
>   ssh://bar.example.com/example
>
> I think that if we use the "principle of least surprise," insteadOf
> rules shouldn't be applied for git-request-pull URLs.

I haven't used request-pull so I don't have much of an opinion on this,
but do you think the same applies to 'git remote get-url <remote>'?

I.e. should it also show the original unmunged URL, or the munged one as
it does now?

Re: insteadOf and git-request-pull output

[Konstantin Ryabitsev]

On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > I think that if we use the "principle of least surprise," insteadOf
> > rules shouldn't be applied for git-request-pull URLs.
> 
> I haven't used request-pull so I don't have much of an opinion on this,
> but do you think the same applies to 'git remote get-url <remote>'?
> 
> I.e. should it also show the original unmunged URL, or the munged one as
> it does now?

I don't know, maybe both? As opposed to git-request-pull, this is not
exposing the insteadOf URL to someone other than the person who set it
up, so even if it does return the munged URL, it wouldn't be unexpected.

-K

Re: insteadOf and git-request-pull output

[Junio C Hamano]

Konstantin Ryabitsev <konstantin@linuxfoundation.org> writes:

> On Thu, Nov 15, 2018 at 07:54:32PM +0100, Ævar Arnfjörð Bjarmason wrote:
>> > I think that if we use the "principle of least surprise," insteadOf
>> > rules shouldn't be applied for git-request-pull URLs.
>> 
>> I haven't used request-pull so I don't have much of an opinion on this,
>> but do you think the same applies to 'git remote get-url <remote>'?
>> 
>> I.e. should it also show the original unmunged URL, or the munged one as
>> it does now?
>
> I don't know, maybe both? As opposed to git-request-pull, this is not
> exposing the insteadOf URL to someone other than the person who set it
> up, so even if it does return the munged URL, it wouldn't be unexpected.

Yeah, I think the local "git remote" should show the rewritten URL
(because that information is not available otherwise) and it is OK
to optionally show the original.  Also, I think it would be nice if
request-pull gave external-facing names.  After all, insteadOf
address is for your own use (e.g. maybe pointing at corporate
mirror), and not for the intended recipient of request-pull.

In short, I think I agree with things you are saying in this
exchange.

Re: insteadOf and git-request-pull output

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On Thu, Nov 15, 2018 at 01:28:26PM -0500, Konstantin Ryabitsev wrote:
> Hi, all:
> 
> Looks like setting url.insteadOf rules alters the output of
> git-request-pull. I'm not sure that's the intended use of insteadOf,
> which is supposed to replace URLs for local use, not to expose them
> publicly (but I may be wrong). E.g.:
> 
> $ git request-pull HEAD^ git://foo.example.com/example | grep example
>   git://foo.example.com/example
> 
> $ git config url.ssh://bar.insteadOf git://foo
> 
> $ git request-pull HEAD^ git://foo.example.com/example | grep example
>   ssh://bar.example.com/example
> 
> I think that if we use the "principle of least surprise," insteadOf
> rules shouldn't be applied for git-request-pull URLs.

I'd like to point out a different use that may change your view.  I have
an insteadOf alias, gh:, that points to GitHub.  Performing the rewrite
is definitely the right thing to do, since other users may not have my
alias available.

I agree that in your case, a rewrite seems less appropriate, but I think
we should only skip the rewrite if the value already matches a valid
URL.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: insteadOf and git-request-pull output

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> $ git request-pull HEAD^ git://foo.example.com/example | grep example
>>   ssh://bar.example.com/example
>> 
>> I think that if we use the "principle of least surprise," insteadOf
>> rules shouldn't be applied for git-request-pull URLs.
>
> I'd like to point out a different use that may change your view.  I have
> an insteadOf alias, gh:, that points to GitHub.  Performing the rewrite
> is definitely the right thing to do, since other users may not have my
> alias available.
>
> I agree that in your case, a rewrite seems less appropriate, but I think
> we should only skip the rewrite if the value already matches a valid
> URL.

It would be tricky to define what a valid URL is, though.  Do we
need some way to say "this is a private URL that should not be
given preference when composing a request-pull message"?  E.g.

    [url "git://git.dev/"]
            insteadOf = https://git.dev/

    [url "https://github.com/"]
            insteadOf = gh:
	    private

The former does not mark https://git.dev/ a private one, so a
"request-pull https://git.dev/$thing" would show the original
"https://git.dev/$thing" without rewriting.  The latter marks gh: a
private one so "request-pull gh:$thing" would be rewritten before
exposed to the public as "https://github.com/$thing"

Or something like that?

Re: insteadOf and git-request-pull output

[Jeff King]

On Sat, Nov 17, 2018 at 04:46:22PM +0900, Junio C Hamano wrote:

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> >> $ git request-pull HEAD^ git://foo.example.com/example | grep example
> >>   ssh://bar.example.com/example
> >> 
> >> I think that if we use the "principle of least surprise," insteadOf
> >> rules shouldn't be applied for git-request-pull URLs.
> >
> > I'd like to point out a different use that may change your view.  I have
> > an insteadOf alias, gh:, that points to GitHub.  Performing the rewrite
> > is definitely the right thing to do, since other users may not have my
> > alias available.
> >
> > I agree that in your case, a rewrite seems less appropriate, but I think
> > we should only skip the rewrite if the value already matches a valid
> > URL.
> 
> It would be tricky to define what a valid URL is, though.  Do we
> need some way to say "this is a private URL that should not be
> given preference when composing a request-pull message"?  E.g.
> 
>     [url "git://git.dev/"]
>             insteadOf = https://git.dev/
> 
>     [url "https://github.com/"]
>             insteadOf = gh:
> 	    private
> 
> The former does not mark https://git.dev/ a private one, so a
> "request-pull https://git.dev/$thing" would show the original
> "https://git.dev/$thing" without rewriting.  The latter marks gh: a
> private one so "request-pull gh:$thing" would be rewritten before
> exposed to the public as "https://github.com/$thing"
> 
> Or something like that?

One funny thing about this is that the "private" config key is
url.https://github.com.private. But that's the public URL!

It makes sense if you think of it as "this rewrite is private". And that
would probably serve for most people's needs, though it gets funny when
you have multiple conversions:

  [url "https://github.com/"]
  insteadOf = gh:
  insteadOf = git://github.com

you may want to share that you are rewriting one of those, but not the
other.

I suspect it would be less confusing if the rewrite were inverted, like:

  [url "gh:"]
  rewriteTo = https://github.com
  rewritePrivate

  [url "git://github.com"]
  rewriteTo = https://github.com

where the mapping of sections to rewrite rules must be one-to-one, not
one-to-many (and you can see that the flip side is that we have to
repeat ourselves).

I hate to introduce two ways of doing the same thing, but maybe it is
simple enough to explain that url.X.insteadOf=Y is identical to
url.Y.rewriteTo=X. I do think people have gotten confused by the
ordering of insteadOf over the years, so this would let them specify it
in whichever way makes the most sense to them.

-Peff

Re: insteadOf and git-request-pull output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I suspect it would be less confusing if the rewrite were inverted, like:
>
>   [url "gh:"]
>   rewriteTo = https://github.com
>   rewritePrivate
>
>   [url "git://github.com"]
>   rewriteTo = https://github.com
>
> where the mapping of sections to rewrite rules must be one-to-one, not
> one-to-many (and you can see that the flip side is that we have to
> repeat ourselves).
>
> I hate to introduce two ways of doing the same thing, but maybe it is
> simple enough to explain that url.X.insteadOf=Y is identical to
> url.Y.rewriteTo=X. I do think people have gotten confused by the
> ordering of insteadOf over the years, so this would let them specify it
> in whichever way makes the most sense to them.

I do admit that the current "insteadOf" was too cute a way to
configure this feature and I often have to read it aloud twice
before convincing myself I got X and Y right.  It would have been
cleaner if the definition were in the opposite direction, exactly
because you can rewrite a single thing into just one way, but we do
want to support that many things mapping to the same, and the
approach to use "url.Y.rewriteTo=X" does express it better.

Re: insteadOf and git-request-pull output

[Jeff King]

On Sat, Nov 17, 2018 at 11:07:32PM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I suspect it would be less confusing if the rewrite were inverted, like:
> >
> >   [url "gh:"]
> >   rewriteTo = https://github.com
> >   rewritePrivate
> >
> >   [url "git://github.com"]
> >   rewriteTo = https://github.com
> >
> > where the mapping of sections to rewrite rules must be one-to-one, not
> > one-to-many (and you can see that the flip side is that we have to
> > repeat ourselves).
> >
> > I hate to introduce two ways of doing the same thing, but maybe it is
> > simple enough to explain that url.X.insteadOf=Y is identical to
> > url.Y.rewriteTo=X. I do think people have gotten confused by the
> > ordering of insteadOf over the years, so this would let them specify it
> > in whichever way makes the most sense to them.
> 
> I do admit that the current "insteadOf" was too cute a way to
> configure this feature and I often have to read it aloud twice
> before convincing myself I got X and Y right.  It would have been
> cleaner if the definition were in the opposite direction, exactly
> because you can rewrite a single thing into just one way, but we do
> want to support that many things mapping to the same, and the
> approach to use "url.Y.rewriteTo=X" does express it better.

In case anybody is interested in picking this up, the code is actually
quite simple (the underlying data structure remains the same; we're just
inverting the order in the config). It would need documentation and
tests, and probably a matching pushRewriteTo.

    diff --git a/remote.c b/remote.c
    index 28c7260ed1..26b1a7b119 100644
    --- a/remote.c
    +++ b/remote.c
    @@ -345,6 +345,11 @@ static int handle_config(const char *key, const char *value, void *cb)
     				return config_error_nonbool(key);
     			rewrite = make_rewrite(&rewrites_push, name, namelen);
     			add_instead_of(rewrite, xstrdup(value));
    +		} else if (!strcmp(subkey, "rewriteto")) {
    +			if (!value)
    +				return config_error_nonbool(key);
    +			rewrite = make_rewrite(&rewrites, value, strlen(value));
    +			add_instead_of(rewrite, xmemdupz(name, namelen));
     		}
     	}
     

However, I did notice the cleanup below as part of writing that. It may
be worth applying independently.

-- >8 --
Subject: [PATCH] remote: check config validity before creating rewrite struct

When parsing url.foo.insteadOf, we call make_rewrite() and only then
check to make sure the config value is a string (and return an error if
it isn't). This isn't quite a leak, because the struct we allocate is
part of a global array, but it does leave a funny half-finished struct.

In practice, it doesn't make much difference because we exit soon after
due to the config error anyway. But let's flip the order here to avoid
leaving a trap for somebody in the future.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/remote.c b/remote.c
index b850f2feb3..28c7260ed1 100644
--- a/remote.c
+++ b/remote.c
@@ -336,14 +336,14 @@ static int handle_config(const char *key, const char *value, void *cb)
 		if (!name)
 			return 0;
 		if (!strcmp(subkey, "insteadof")) {
-			rewrite = make_rewrite(&rewrites, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
+			rewrite = make_rewrite(&rewrites, name, namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		} else if (!strcmp(subkey, "pushinsteadof")) {
-			rewrite = make_rewrite(&rewrites_push, name, namelen);
 			if (!value)
 				return config_error_nonbool(key);
+			rewrite = make_rewrite(&rewrites_push, name, namelen);
 			add_instead_of(rewrite, xstrdup(value));
 		}
 	}
-- 
2.20.0.rc1.703.g93fba25b62