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

NULL (#f) not allowed for git_diff_options #4

Open
kengruven opened this issue Jul 17, 2023 · 4 comments
Open

NULL (#f) not allowed for git_diff_options #4

kengruven opened this issue Jul 17, 2023 · 4 comments

Comments

@kengruven
Copy link

kengruven commented Jul 17, 2023

The libgit2 docs for git_diff_options say "passing NULL for the options structure will give the defaults", and the libgit2 sample code shows this in practice.

With these Racket bindings, though, several functions (like git_diff_tree_to_tree) use _git_diff_opts-pointer rather than _git_diff_opts-pointer/null, so passing #f here results in an error:

git_diff_opts->C: argument is not non-null `git_diff_opts' pointer

The scrbl docs even say that this parameter has type [opts (or/c git_diff_opts? #f)].

@kengruven
Copy link
Author

I tried working around this from my program, and got stuck at every attempt:

  • There's a C macro GIT_DIFF_OPTIONS_INIT which has a value for git_diff_options (but not the defaults, it seems?), but this constant isn't wrapped by libgit2-racket so you can't use that
  • There's a C function git_diff_options_init which creates a default git_diff_options for you, but it requires a starting C struct that you wish to initialize
  • The libgit2-racket bindings has a define-cstruct _git_diff_opts but it misses the oid_type field from the middle of the struct, so you can't create your own from scratch, either

Based on the upstream docs, I think the most low-level way to create a non-null "default" value would be something like:

(make-git_diff_opts GIT_DIFF_OPTS_VERSION 'GIT_DIFF_NORMAL 'GIT_SUBMODULE_IGNORE_UNSPECIFIED
                    (make-git_strarray '()) #f #f #f 3 0 0 7 (* 512 1024 1024) "a" "b")

but this fails with "the expected number of arguments does not match the given number" because oid_type is missing from the mapping.

@LiberalArtist
Copy link
Member

Thanks for the report!

With these Racket bindings, though, several functions (like git_diff_tree_to_tree) use _git_diff_opts-pointer rather than _git_diff_opts-pointer/null, so passing #f here results in an error:

There's a C function git_diff_options_init which creates a default git_diff_options for you, but it requires a starting C struct that you wish to initialize

These are definitely our bug and should be fixed.

  • There's a C macro GIT_DIFF_OPTIONS_INIT which has a value for git_diff_options (but not the defaults, it seems?), but this constant isn't wrapped by libgit2-racket so you can't use that

  • There's a C function git_diff_options_init which creates a default git_diff_options for you, but it requires a starting C struct that you wish to initialize

We can't really use C macros like GIT_DIFF_OPTIONS_INIT because they don't exist at runtime for dlsym: IIRC, that's why upstream added functions like git_diff_options_init. But they also make these very friendly guarantees about NULL being the default, so we don't really need all of those functions in Racket. In other words, something very much like your:

(make-git_diff_opts GIT_DIFF_OPTS_VERSION 'GIT_DIFF_NORMAL 'GIT_SUBMODULE_IGNORE_UNSPECIFIED
                    (make-git_strarray '()) #f #f #f 3 0 0 7 (* 512 1024 1024) "a" "b")

ought to work, but for bugs (though maybe we shouldn't expose GIT_DIFF_OPTS_VERSION to clients, since the only thing they could do with it is cause errors).

This is also crying out for a convenience function with optional keyword arguments.

@kengruven
Copy link
Author

We can't really use C macros like GIT_DIFF_OPTIONS_INIT because they don't exist at runtime

That's true, but the Racket bindings already say things like (define GIT_DIFF_HUNK_HEADER_SIZE 128) to mimic other C macro constants. That big ( make-git_diff_opts ...) expression took me 15 minutes to figure out, so (at least in the case where one needed a custom git_diff_opts value) I think it could be useful to have available in Racket.

That said, I don't think I need any non-default diff options for my program. Allowing null (and letting libgit2 use its default value) would work just fine for me.

@kengruven
Copy link
Author

I've never used Racket FFI before, so I have no idea what I'm doing. Does this change make sense?

Still left to do, to completely support git_diff_options:

  • add oid_type field to define-cstruct _git_diff_opts
  • add git_oid_t enum (how does #ifdef work here?)
  • maybe define an GIT_DIFF_OPTIONS_INIT with default values

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

No branches or pull requests

2 participants