Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

configure http transport #586

Merged
merged 97 commits into from
Nov 17, 2022
Merged

configure http transport #586

merged 97 commits into from
Nov 17, 2022

Conversation

Byron
Copy link
Member

@Byron Byron commented Nov 8, 2022

We should support the most common configuration flags for the widespread http transport.

Tasks

  • analyse available http config keys
  • http::Options (shared by implementations) with most common configuration values.
  • Configure curl according to (simple) options
  • pass through the git user-agent to make it configurable
    • also add gitoxide.userAgent configuration for overrides.
  • Obtain 'simple' http options from configuration and pass them on
  • lenient mode for reading values
  • setup http proxy correctly (turn it into an actual URL like curl wants it)

We also select the most important few for implementation.
The most time-consuming part would be to implement overrides correctly
and validate it against the baseline of git, it's unclear how git
can be queried or made to leak certain information. Maybe by overriding
DNS mappings to let it go against an unknown local IP/port?
…but it clearly shows that the backend needs to be abstracted or else
we can't have general HTTP options as are implied by the way git works.
It's not needed anymore as the backend config itself is behind an
Arc<Mutex>
@Byron
Copy link
Member Author

Byron commented Nov 9, 2022

@paulyoung I have refactored the kind of Options passed to the reqwest backend to allow for common http related options (typically those behind http.* in the git-config). The only thing that changes is that the previous options are now in http::reqwest::Options, which should be assigned to the http::Options::backend field of these new common options.

This also means that one day one can configure reqwest with the common HTTP options to allow it to be a fully-fledged backend. It's worth noting that this might be a lot of work, as many of the http configuration options are indeed specific to curl so on-on-one mappings won't always be possible.

I hope that works for you, and signals a step towards one day making reqwest into a fully-fledged backend - there is demand to get rustls into the tree and avoid curl + openssl in the process.

@Byron Byron mentioned this pull request Nov 9, 2022
27 tasks
…uple()` returns specific key-value pair.

This is in preparation for allowing user-defined agent strings.
…rs more flexibly.

Value can now also be owned, which is useful if the value type is a
`Cow<'_, String>`.
…ies`.

That way it's easier to pass custom agent strings when invoking.
Note that this is already a breaking change earlier.
Doable by setting the `gitoxide.userAgent` variable.
This showed an issue with remote name handling as well which
needs fixing once remote names are affecting the configuration.
…a pending change. (#595)

As opposed to dropping the Transaction, this method allows to obtain all
edits that would have been applied.
…and fail as apparently probing really needs the right directory to work
as expected.
Or is there anything else?
Right now they only appear to be locks already taken, not the best UX
for sure, but a starting point.
…ating the iterator. (#595)

Previously, it would fail on first iteration, making it seem like there
is one reference even though it's just an error stating that the base
cannot be read.

This is clearly worse than making a metadata check on the filesystem,
no matter how unlikely the case.
…ase, correctly yield zero references. (#595)

Previously it reported an error, now it does not and instead performs no
iteration, which is more helpful to the user of the API I believe as
they won't randomly fail just because somebody deleted the `refs`
folder.
…ase-insensitie filesystems*. (#595)

The asterisk indicates that this only works if packed-refs are present
and these references are written straight to packed references without
ever trying to handle the otherwise conflicting loose reference files.

This is done by leveraging the fact that in presence of packed-refs
or a pending creation of packed-refs, there is no need to create
per-file locks as concurrent transactions also have to obtain the
packed-refs lock and fail (or wait) until it's done.
…ock is helt. (#595)

The logic is similar to what's done with updates.
… file.

The per-loose lock file isn't a requirement anymore as the packed-refs
lock can be used to enforce consistency, at least among
`gitoxide` powered tools.
Didn't actually check `git` does it similarly, but also couldn't
find any special behaviour related to clone and fetch ref updates.
#595)

Previously it was possible for symbolic refs to be deleted right after
they have been created or updated as they were included in the set of
refs that was assumed to be part of packed-refs, which isn't the case
for symbolic refs.
Previously it wouldn't set the correct packed-refs mode which would
create packed refs, but also leave loose refs.
V2 ref fetching (invoking a command) is indeed only available in V2
which in turn is only for fetch.

Maybe the solution here is rather to make ls-refs a general command,
which might lead to reorganzation. For now, it's OK to keep it there
I think knowing push is only V1 which is what this is for anyway.
…ck` as well.

Note that `fetch::handshake()` is still present, selecting the correct
service like before, but most of the `fetch::Ref` parsing is now handled
by `handshake::Ref` in it's more general location.

There is still `fetch::refs(…)` which just invokes a V2 ls-refs command
(without that being generalized) as `git-receive-pack` just supports
V1 at the moment, and making the `fetch::LsRefs` Command more general
would mean that every command has to be general, at is doesn't matter
anymore if it's used for Fetch or Push.

As long as git won't support V2 for pushes, which it never might,
there is no need to introduce this breakage even though it
should probably be done before going 1.0.
These are generally useful in all interactions, including push.
This represents much better on how it is actually used, which is for
validation of any Command invoked through the protocol independenty.
…refs(…)`.

This move also removes the `protocol` parameter as ls-refs is only available
in V2 and the caller is forced to know that after running a
`handshake(…)` anyway which yields the actual protocol used by the
server.

Note that the client can suggest the version to use when connecting,
which is specific to the transport at hand.
@Byron Byron merged commit 665b53e into main Nov 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants