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

Add support for push options #1282

Merged
merged 3 commits into from
Mar 27, 2024
Merged

Add support for push options #1282

merged 3 commits into from
Mar 27, 2024

Conversation

kempniu
Copy link
Contributor

@kempniu kempniu commented Mar 18, 2024

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

While the basic code change (d387941) 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 🚀

So here we are:

  • ed3a6b2 proposes a slight change in the way StrArray is used, to make the cases where a pointer to an allocated git_strarray structure is passed to a libgit2 function slightly more readable (IMHO, of course),
  • 2c8fc47 adds a new method to StrArray that enables assigning a list of strings to an existing git_strarray structure (I deliberately avoided naming the method copy_to() because FFI scoping rules still need to be adhered to),
  • d387941 actually adds support for push options, using the StrArray.assign_to() method added by the previous commit.

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

Copy link
Member

@jdavid jdavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rebase over master? It has already been upgraded to v1.8

pygit2/utils.py Show resolved Hide resolved
@jdavid
Copy link
Member

jdavid commented Mar 24, 2024

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
Copy link
Contributor Author

kempniu commented Mar 25, 2024

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 9fbf407. 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
Copy link
Member

jdavid commented Mar 26, 2024

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)

To emphasize that the StrArray() context manager returns a pointer,
implement its ptr() property and use it whenever calling a C function
that takes a pointer to a git_strarray structure as a parameter.
Enable the StrArray() context manager to be used for assigning a list of
strings to a pre-existing git_strarray structure.  This is useful when
some other structure contains a git_strarray (rather than a pointer to
it).
Add a new keyword argument to Remote.push() that is a counterpart of
`git push --push-option=<string>`.

Fixes #1126
@kempniu
Copy link
Contributor Author

kempniu commented Mar 27, 2024

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 (9fbf407) into the commit that implements the new feature (a41cbab).

@jdavid jdavid merged commit 34b6875 into libgit2:master Mar 27, 2024
@jdavid
Copy link
Member

jdavid commented Mar 27, 2024

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.

@kempniu kempniu deleted the add-support-for-push-options branch March 27, 2024 09:42
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 19, 2024
# 1.15.0 (2024-05-18)

- Many deprecated features have been removed, see below

- Upgrade to libgit2 v1.8.1

- New `push_options` optional argument in `Repository.push(...)`
  [#1282](libgit2/pygit2#1282)

- New support comparison of `Oid` with text string

- Fix `CheckoutNotify.IGNORED`
  [#1288](libgit2/pygit2#1288)

- Use default error handler when decoding/encoding paths
  [#537](libgit2/pygit2#537)

- Remove setuptools runtime dependency
  [#1281](libgit2/pygit2#1281)

- Coding style with ruff
  [#1280](libgit2/pygit2#1280)

- Add wheels for ppc64le
  [#1279](libgit2/pygit2#1279)

- Fix tests on EPEL8 builds for s390x
  [#1283](libgit2/pygit2#1283)

Deprecations:

- Deprecate `IndexEntry.hex`, use `str(IndexEntry.id)`

Breaking changes:

- Remove deprecated `oid.hex`, use `str(oid)`
- Remove deprecated `object.hex`, use `str(object.id)`
- Remove deprecated `object.oid`, use `object.id`

- Remove deprecated `Repository.add_submodule(...)`, use `Repository.submodules.add(...)`
- Remove deprecated `Repository.lookup_submodule(...)`, use `Repository.submodules[...]`
- Remove deprecated `Repository.init_submodules(...)`, use `Repository.submodules.init(...)`
- Remove deprecated `Repository.update_submodule(...)`, use `Repository.submodules.update(...)`

- Remove deprecated constants `GIT_OBJ_XXX`, use `ObjectType`
- Remove deprecated constants `GIT_REVPARSE_XXX`, use `RevSpecFlag`
- Remove deprecated constants `GIT_REF_XXX`, use `ReferenceType`
- Remove deprecated `ReferenceType.OID`, use instead `ReferenceType.DIRECT`
- Remove deprecated `ReferenceType.LISTALL`, use instead `ReferenceType.ALL`

- Remove deprecated support for passing dicts to repository\'s `merge(...)`,
  `merge_commits(...)` and `merge_trees(...)`. Instead pass `MergeFlag` for `flags`, and
  `MergeFileFlag` for `file_flags`.

- Remove deprecated support for passing a string for the favor argument to repository\'s
  `merge(...)`, `merge_commits(...)` and `merge_trees(...)`. Instead pass `MergeFavor`.
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.

Push options support within pygit2
2 participants