remote: add callback to resolve URLs before connecting by eaigner · Pull Request #5056 · libgit2/libgit2

eaigner · 2019-04-21T18:49:23Z

I tried to move the original PR to another branch on my fork, and Github auto-closed my PR.
This is a new one replacing #5028.
Sorry for the dupe.

Since libssh2 doesn't read host configuration from the config file,
this callback can be used to hand over URL resolving to the client
without touching the SSH implementation itself.

Example:

Remote origin has URL

ssh://myhost/~/repo.git

This would obviously fail with the current libgit2 implementation,
given the following SSH config:

Host myhost
    User gituser
    HostName my.very.long.host.with.subdomain
    Port 1234

The URL callback allows this to be rewritten to e.g.

gituser@my.very.long.host.with.subdomain:1234/~/repo.git

by the client without much hassle.

Since libssh2 doesn't read host configuration from the config file, this callback can be used to hand over URL resolving to the client without touching the SSH implementation itself.

pks-t

Thanks for your amendments! One more thing, otherwise I'm good

eaigner · 2019-04-26T10:52:20Z

somehow the build system does not automatically build at the moment

pks-t · 2019-04-26T10:52:46Z

/rebuild

libgit2-azure-pipelines · 2019-04-26T10:52:55Z

Okay, @pks-t, I started to rebuild this pull request as build #1839.

pks-t · 2019-04-26T10:53:26Z

Thanks for alerting, I didn't notice

pks-t · 2019-04-26T10:54:22Z

It looks like CI is building, but there's no status checks in our PRs anymore. @ethomson, got any clue why that is?

ethomson · 2019-04-26T12:57:18Z

It looks like CI is building, but there's no status checks in our PRs anymore. @ethomson, got any clue why that is?

Let me find out.

pks-t · 2019-04-29T07:29:27Z

/rebuild

libgit2-azure-pipelines · 2019-04-29T07:29:37Z

Okay, @pks-t, I started to rebuild this pull request as build #1847.

pks-t · 2019-04-29T07:29:49Z

Trying another rebuild, as I saw other PRs do have checks again.

ethomson · 2019-04-29T09:27:54Z

I don't 100% understand the checks architecture between GitHub and Azure Pipelines, but I think that there's only one-shot to get this hooked up. I think that GitHub notifies Azure Pipelines at PR creation and Azure Pipelines then sets itself up as a check. I think that one of those two deliveries failed, and so now this PR will forever not have checks from Azure Pipelines. Unfortunately that means we'll have to click through to the build manually.

I'll raise an issue to see if we can investigate.

eaigner · 2019-04-29T09:38:30Z

Well https://dev.azure.com/libgit2/libgit2/_build/results?buildId=1847 says all checks have passed. Shouldn't that suffice?

ethomson · 2019-04-29T09:50:09Z

Shouldn't that suffice?

Absolutely.

@tiennou what are you thinking here?

tiennou · 2019-04-29T10:15:33Z

Weeell, on one hand I have some concern about adding a "backend" (this word ^^)-specific function that's more or less a workaround, eg. if I were to add libssh to the party*, I'd had to handle this customization point on top of its .ssh/config support (does we do ours before, after ?). Hence my technical answer would be to NAK, invoking layering violations, and the need for upstreaming.

On the other hand, I understand where this is coming from, the libssh2 project has limited bandwidth at the moment, and it really is a big chunk of actual missing functionality. Because of all this I'm reluctant to shot it down…

* which I'm debating doing, just so I can finally stop worrying about baking libssh2-specific things in our 1.0 interface 😉.

ethomson · 2019-04-29T10:32:34Z

Indeed the impetus for this functionality came from the libssh2 limitation, however this does feel generally useful. It's basically programmatic insteadOf without having to deal with the configuration.

tiennou

Hmm, okay, I think if we make it explicitly clear that this might not be the actual, final URL passed to the transport, I'm satisfied from a technical standpoint (ie. a libssh backend is by design allowed to re-substitute its own URL). This is the complexity I'm concerned about: this feels like a can of worm to me, like substituting the URL once, causing the lower layer to not recognize it anymore… Maybe I'm just too paranoid though 🤣.

I've reviewed this more closely, I think there's still a potential allocator mismatch lurking, but apart for that, if that's something we want and the specification is a little more fleshed out, I'm fine.

ethomson · 2019-04-30T12:52:58Z

I'm happy with this git_buf approach. Callers can set it if they wish using git_buf_set, at which point we do a copy, so we control both the allocator and the free function.

I'm heads-down on the day job though so I'm very scattered, and I'd like to hold off just to make sure that I haven't missed anything before we 🚢 this.

ethomson · 2019-04-30T13:06:41Z

Sorry, to clarify, hold off for another core contributor to 👍 this in case I'm missing something.

eaigner mentioned this pull request Apr 21, 2019

remote: add callback to resolve URLs before connecting #5028

Closed

remote: add callback to resolve URLs before connecting

b7df959

Since libssh2 doesn't read host configuration from the config file, this callback can be used to hand over URL resolving to the client without touching the SSH implementation itself.

pks-t requested changes Apr 26, 2019

View reviewed changes

Erik Aigner added 2 commits April 26, 2019 11:26

param order and passthrough

9b36fcc

url is always non NULL at this point

cef8ce3

pks-t reviewed Apr 26, 2019

View reviewed changes

Comment thread tests/network/remote/remotes.c Outdated

Comment thread src/remote.c Outdated

Erik Aigner added 2 commits April 26, 2019 12:21

use GIT_UNUSED instead of pragma

d93dbb7

return zero instead of GIT_OK

47972bf

pks-t approved these changes Apr 26, 2019

View reviewed changes

tiennou reviewed Apr 29, 2019

View reviewed changes

Comment thread tests/network/remote/remotes.c Outdated

Comment thread tests/network/remote/remotes.c Outdated

Erik Aigner added 6 commits April 29, 2019 15:19

fix static test method names

32fa407

use POSIX strcmp and strdup

a363b7a

pass git_buf instead of char pointer to callback

e7487a3

change wording

99da9a9

make sure buffer is clear before passing to callback

9cb502a

explain second buf_clear

ef66d73

Erik Aigner added 2 commits April 29, 2019 16:51

use url.ptr in test compare

2f4da56

use url.ptr in test_network_remote_remotes__parsing

67e93ab

tiennou mentioned this pull request Apr 29, 2019

Remote URL last-chance resolution #5062

Merged

eaigner closed this Apr 30, 2019

Conversation

eaigner commented Apr 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eaigner commented Apr 26, 2019

Uh oh!

pks-t commented Apr 26, 2019

Uh oh!

libgit2-azure-pipelines bot commented Apr 26, 2019

Uh oh!

pks-t commented Apr 26, 2019

Uh oh!

pks-t commented Apr 26, 2019

Uh oh!

ethomson commented Apr 26, 2019

Uh oh!

pks-t commented Apr 29, 2019

Uh oh!

libgit2-azure-pipelines bot commented Apr 29, 2019

Uh oh!

pks-t commented Apr 29, 2019

Uh oh!

ethomson commented Apr 29, 2019

Uh oh!

eaigner commented Apr 29, 2019

Uh oh!

ethomson commented Apr 29, 2019

Uh oh!

tiennou commented Apr 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented Apr 29, 2019

Uh oh!

tiennou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ethomson commented Apr 30, 2019

Uh oh!

ethomson commented Apr 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tiennou commented Apr 29, 2019 •

edited

Loading