Remote URL last-chance resolution by tiennou · Pull Request #5062 · libgit2/libgit2

tiennou · 2019-04-29T23:09:09Z

This is #5056, in 3 commits:

add a helper to upgrade options structures if needed
update the git_remote_callback users to the autoupgrader
add a callback to git_remote_callbacks to change the URL before the connection is made.

tiennou · 2019-05-01T14:20:22Z

Rebased, with the unneeded ➰ allocator removed. Most of the code from #5056 has thus been restored.

If that's even possible, it would be nice to have this one in quickly: any new contribution that touches one of our structs is going to have to do something like this (and some are pesky). The sooner this happens, the easier it gets for contributors. Maybe. That's how I felt when I noticed the change in #5056.

I've tried to take into account some of the ideas from the other backward-compatibility PR (git_refdb_backend?), but this one is simpler, thus I don't think it's sufficient I'll try to revisit the other one, though, in case it's not). It's missing documentation which I'd like to add before this goes in, but a first review round seemed like a good idea.

ethomson · 2019-05-01T15:08:55Z

I don't understand why you're incrementing the struct version here? You're adding a field, that's not an API breaking change?

ethomson · 2019-05-01T15:12:39Z

+ *
+ * You need to be careful not to overflow url_resolved. If it's too small,
+ * returning a positive value equal to the size required will cause the
+ * resolution to be tried again with a buffer of the requested size.


Is this comment accurate? I don't see this behavior.

Oops, a leftover from the old way, before I re-read buffer.h. Please disregard.

tiennou · 2019-05-01T16:39:16Z

It's binary-breaking because if I get a old v1 struct without the member and try to access it as a new, we'll read some stack data. Hence the bump. I don't think there's a way around, is it?

ethomson · 2019-05-01T17:08:04Z

Yes, this breaks ABI but not API. We've never tried to be ABI-consistent.

It's a noble goal that we should pursue but let's drop those two commits for now and get this merged so we can 🚢. We do need a strategy for coping with API breaking struct changes, which it seems like this aligns to as well. So let's split those two commits out into a new PR. I suspect that there's going to be a few subtle nuances to struct compatibility that we should spend some time to 🤔 about.

tiennou · 2019-05-01T17:53:02Z

Rebased, I've removed the updater and will refile as needed.

ethomson · 2019-05-19T11:10:33Z

+	 * Resolve URL before connecting to remote.
+	 * The returned URL will be used to connect to the remote instead.
+	 */
+	git_resolve_url_cb resolve_url;


I'd like this to be git_url_resolve_cb, since that sounds more like something that acts on (resolves) an object (url) instead of something that urls a resolve.

I'm otherwise 👍 on this, so I'm happy to make this change and merge it though to get this unblocked if you're busy.

I think it sounds weird in any order, so I renamed. A few weeks later, I have the feeling it could be provided as some kind of "proxying" internal transport, but maybe that's too much (that would make this use case go through the already existing custom transport callback just above this one).

pks-t

I'm happy with this PR, thanks! @ethomson, you good with this, too?

pks-t · 2019-05-21T08:10:21Z

+	GIT_UNUSED(url_resolved);
+	GIT_UNUSED(url);
+	GIT_UNUSED(direction);
+	GIT_UNUSED(payload);


Sometimes I wish we'd be allowed to use variadic macros. Would make such unused blocks at least a tad less awful

I've recently learnt that there's an __attribute__((unused)) that does the same thing, also available as __unused (for nicer declarations). These can go on parameters, and GCC and Clang looks to be fine with it. Just my 2¢.

Yeah, I've used them in my own projects, too. I'm not sure whether I like those more, though.

eaigner · 2019-05-21T08:23:42Z

ping. merge?

ethomson · 2019-05-21T08:57:03Z

Yep, this looks good to me. @tiennou can you remove that unused assignment to status and we can 🚢

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.

ethomson · 2019-05-21T12:33:47Z

Thanks @eaigner and @tiennou!

tiennou force-pushed the fix/ssh-url-substitution branch 2 times, most recently from b8e1b76 to bcac837 Compare April 30, 2019 11:58

tiennou mentioned this pull request Apr 30, 2019

remote: add callback to resolve URLs before connecting #5056

Closed

tiennou force-pushed the fix/ssh-url-substitution branch from bcac837 to f154cd3 Compare May 1, 2019 13:06

tiennou changed the title ~~remote: add callback to resolve URLs before connecting~~ Option struct updater & remote URL last-chance resolution May 1, 2019

ethomson reviewed May 1, 2019

View reviewed changes

Comment thread src/remote.c Outdated

tiennou force-pushed the fix/ssh-url-substitution branch from f154cd3 to 12d9940 Compare May 1, 2019 17:50

tiennou changed the title ~~Option struct updater & remote URL last-chance resolution~~ Remote URL last-chance resolution May 1, 2019

pks-t reviewed May 2, 2019

View reviewed changes

Comment thread src/remote.c

Comment thread src/remote.c Outdated

Comment thread src/remote.c Outdated

tiennou force-pushed the fix/ssh-url-substitution branch from 12d9940 to c7bcb8b Compare May 2, 2019 09:30

pks-t approved these changes May 2, 2019

View reviewed changes

Comment thread src/remote.c Outdated

tiennou force-pushed the fix/ssh-url-substitution branch from c7bcb8b to 33e42d4 Compare May 2, 2019 11:01

ethomson reviewed May 19, 2019

View reviewed changes

tiennou force-pushed the fix/ssh-url-substitution branch from 33e42d4 to daa7883 Compare May 20, 2019 08:19

pks-t approved these changes May 21, 2019

View reviewed changes

tiennou force-pushed the fix/ssh-url-substitution branch from daa7883 to f7d646a Compare May 21, 2019 11:31

remote: add callback to resolve URLs before connecting

59647e1

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.

tiennou force-pushed the fix/ssh-url-substitution branch from f7d646a to 59647e1 Compare May 21, 2019 12:11

ethomson merged commit 954f535 into libgit2:master May 21, 2019

tiennou deleted the fix/ssh-url-substitution branch May 21, 2019 12:48

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open

Conversation

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

Uh oh!

Uh oh!

tiennou commented May 1, 2019

Uh oh!

ethomson commented May 1, 2019

Uh oh!

ethomson May 1, 2019

Choose a reason for hiding this comment

Uh oh!

tiennou May 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tiennou commented May 1, 2019

Uh oh!

ethomson commented May 1, 2019

Uh oh!

tiennou commented May 1, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ethomson May 19, 2019

Choose a reason for hiding this comment

Uh oh!

tiennou May 20, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pks-t May 21, 2019

Choose a reason for hiding this comment

Uh oh!

tiennou May 21, 2019

Choose a reason for hiding this comment

Uh oh!

pks-t May 21, 2019

Choose a reason for hiding this comment

Uh oh!

eaigner commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ethomson commented May 21, 2019

Uh oh!

ethomson commented May 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tiennou commented Apr 29, 2019 •

edited

Loading

eaigner commented May 21, 2019 •

edited

Loading