8000 Implement push options on push by russell · Pull Request #6439 · libgit2/libgit2 · GitHub
[go: up one dir, main page]

Skip to content

Implement push options on push #6439

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? 8000 Sign in to your account

Merged
merged 5 commits into from
Feb 7, 2024
Merged

Implement push options on push #6439

merged 5 commits into from
Feb 7, 2024

Conversation

russell
Copy link
Contributor
@russell russell commented Dec 1, 2022

This PR implements passing push options on push if the server supports the capability. Push options are sent after the initial handshake https://github.com/git/git/blob/master/Documentation/gitprotocol-pack.txt#L563

I implemented this because SourceHut uses push-options to skip CI, but after looking at #5335 I see that GitLab has the same feature. Gitea implemented a small amount of functionality using push-options, but it's not as critical for it.

Unfortunately I wasn't aware of PR #6281 but I did find it before I implemented any tests, so I have rebased and improved the tests suite from that PR.

This PR adds a field to the push_options struct called push_options, this might seem a bit confusing, but push-options are the name of the options in the Git CLI. Though the name could be shortened to options if that was clearer.

The test suite can only be run on focal or newer Ubuntu's. This is because older versions of the git CLI doesn't support receiving push options (or doesn't advertise the capability). When testing libgit2 against focal, I found that SSH doesn't work, it looks like an incompatibility between libssh2 and openssh, with the logs up to debug3 I was able to spot some differences between the ssh client an libssh2, but i really don't understand ssh well enough to know why the connection is rejected. I tried upgrading libssh2 to 1.10.0, but that didn't help.

fixes #5335

russell and others added 2 commits November 27, 2022 00:17
Push options are an optional capability of a git server.  If they are
listed in the servers capabilities, when the client lists push-options
in it's list of capabilities, then the client sends it's list of push
options followed by a flush-pkt.

So, If we have any declared push options, then we will list it as a
client capability, and send the options.

If the request contains push options but the server has no push
options capability, then error.
We found that the best way to test push options was to receive them via a git hooks script, and output them to a location that the CI tests could read to check and see if the push options were interpreted by git correctly.

Co-Authored-By: pireads <pireads@gmail.com>
Co-Authored-By: lotdeef <lcfisch2@asu.edu>
Co-Authored-By: PSI497 <497.psi.497@gmail.com>
@russell
Copy link
Contributor Author
russell commented Dec 1, 2022

Also, push options for local push can't be supported until hooks are implemented for local repos, i didn't look into this too much. It should be added separately i think to prevent this PR growing too much

@boretrk
Copy link
Contributor
boretrk commented Dec 1, 2022

push options for local push can't be supported until hooks are implemented for local repos

If you look into hooks it could be a good idea to check the security issue mentioned in #6400 to see if it applies.

Since we use `git_push_options` as the options structure to our
`git_push` command, much like we do everywhere else, "push_options"
becomes ambiguous. "remote_options" isn't much better for us. Call them
"remote_push_options", which is still quite bad, and not particularly
insightful for end users, but at least something that we can discuss
unambiguously.
Keep the push options tests more constrained to our CI environment;
writing files within the test sandbox instead of outside of it. Provide
the path to the output file in the test data. In addition, add the
repository to the test resources instead of recreating the hooks every
time.
@ethomson
Copy link
Member
ethomson commented Feb 6, 2024

Thank you so much for pulling this together. This is great work by you and the other contributors. I apologize that it took so long to address this PR, but I love the simplifications that you made.

I merged main into this, since the PR had accumulated some conflicts during the time that I spent not reviewing it. I also made some minor tweaks — I changed the name to remote_push_options which... is not great, but since we had push_options already, I felt like it helped disambiguate. I also changed the CI test behavior a little bit so that we could keep mutable data contained within the sandbox.

Thanks again - I think that this is a nice feature and I appreciate you and everybody else who went above and beyond to add integration tests. 🙏

@ethomson ethomson merged commit 8535fdb into libgit2:main Feb 7, 2024
bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 2, 2024
bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 2, 2024
@ethomson ethomson mentioned this pull request Mar 9, 2024
bnjmnt4n added a commit to bnjmnt4n/git2-rs that referenced this pull request Mar 22, 2024
github-merge-queue bot pushed a commit to rust-lang/git2-rs that referenced this pull request May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support push options
4 participants
0