libgit2 / pygit2

Python bindings for libgit2
https://www.pygit2.org/
Other
1.58k stars 382 forks source link

Add support for push options #1282

Closed kempniu closed 3 months ago

kempniu commented 3 months ago

With libgit2/libgit2#6439 having been recently merged, this PR attempts adding support for push options to pygit2.

While the basic code change (d387941a309804ac7bc198be4d34e4086204258f) is fairly trivial, I had to jump through some other hoops to get it to work. Here is a summary of how this PR reached its current form:

  1. First, I need a libgit2 version that supports push options. Let's build it!
  2. Oh no, push options only work in libgit2's master branch, not in libgit 1.7.2, which is the latest version that pygit2 currently works with.
  3. Okay, let's hack around the current requirement for libgit2 1.7.x in pygit2's master branch. (These changes are not included in this PR.)
  4. This should be easy now, just turn a list of strings into a git_strarray and voilà.
  5. Oh no, StrArray is only able to allocate a new git_strarray structure, how do I assign a list of strings to an existing git_strarray that is a member of another structure (git_push_options)?
  6. Okay, let's add something reusable that can be used for this purpose.
  7. Done, now we can finally add support for push options :rocket:

So here we are:

There is one more pitfall: I don't have any bright ideas for testing push options using the test infrastructure currently existing in the pygit2 repository. Hook support for libgit2 has been in the making for a few years now (libgit2/libgit2#4620) and server-side hooks cannot be defined for a GitHub repository (and even if this was possible, the test would need to examine some sort of an artifact created by the server-side hook, as it is done in the relevant libgit2 tests), so the only option I can think of is firing up a local Git daemon for this purpose during pygit2 tests. It's all doable, of course, but I thought it is prudent to wait for initial feedback on this proposal before going any further.

I would be happy to rework any part of this PR - it just felt that the additions proposed here might be useful in other similar cases in the future and it is always easier to discuss specific proposals than high-level ideas.

Thank you in advance for taking a look!

Fixes #1126

jdavid commented 3 months ago

Regarding the tests, if it makes things easier, we may assume that the feature is tested in libgit2 and just test whether we are passing the arguments correctly... if it makes the tests simpler.

kempniu commented 3 months ago

As for tests, testing the arguments passed to libgit2 is still tricky because the structure we're interested in checking only ephemerally lives inside pygit2/remotes.py:push() and CFFI lib objects are read-only and cannot be patched...

The best I could come up with is 9fbf4070fb35cd38bd991aa9b49c88edea8ac173. It patches pygit2.callbacks.RemoteCallbacks, which is as deep as we can patch pure Python code in order to inspect to the relevant structures.

jdavid commented 3 months ago

Could you fix the "Lints" workflow, https://github.com/libgit2/pygit2/actions/runs/8422070812/job/23060689058 ?

(I think you're right, that .ptr makes more sense than .array)

kempniu commented 3 months ago

Could you fix the "Lints" workflow, https://github.com/libgit2/pygit2/actions/runs/8422070812/job/23060689058 ?

Sure, though that one only gets triggered for the arr.array approach, and...

(I think you're right, that .ptr makes more sense than .array)

...I treated this is a request to revert to the originally-proposed StrArray.ptr approach, so no extra change should now be required to make the ruff workflow pass.

I used this opportunity to fold the tests (9fbf4070fb35cd38bd991aa9b49c88edea8ac173) into the commit that implements the new feature (a41cbab5c7e04f2d55f4bbee852bceb9a4d5eaa1).

jdavid commented 3 months ago

Merged, thanks!

Sorry I was not clear earlier. I meant that the public interface of StrArray should only offer one way, either .array or .ptr, but not both (with a preference for .ptr). In the latest commit I've made .array private, renaming it to .__array, and updated callbacks.py and repository.py accordingly.