libgit2 1.4.0 changed the public API of "git_buf" and renamed the attribute "asize" to "reserved". For details, see https://github.com/libgit2/libgit2/pull/6078/files. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/1165
-- Commit Summary --
* GitChangeBar: Support libgit2 1.4.x
-- File Changes --
M git-changebar/src/gcb-plugin.c (9)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/1165.patch https://github.com/geany/geany-plugins/pull/1165.diff
@eht16 commented on this pull request.
@@ -49,6 +49,11 @@
# define git_buf_dispose git_buf_free # define git_error_last giterr_last #endif +#if ! defined (LIBGIT2_VER_MINOR) || ( (LIBGIT2_VER_MAJOR == 1) && (LIBGIT2_VER_MINOR < 4) ) +# define gcb_git_buf_reserved asize
Since `asize` and `reserved` are rather generic words, I introduced a prefixed version to avoid accidental name clashes.
Is there a better way?
Tested against libgit2 1.1.0 and 1.4.0.
Unfortunately, we'll get new issues with future releases of libgit2 because `git_blob_filtered_content` and `git_buf_grow` are already deprecated. Though replacing those would require more work and probably some refactoring.
@eht16 pushed 1 commit.
108ef15553663e3db49343cbc4e4664c521cb96b GitChangeBar: Support libgit2 1.4.x
If nobody objects, I would like to merge this soon as it breaks building on/for Windows, too.
@elextr commented on this pull request.
@@ -49,6 +49,11 @@
# define git_buf_dispose git_buf_free # define git_error_last giterr_last #endif +#if ! defined (LIBGIT2_VER_MINOR) || ( (LIBGIT2_VER_MAJOR == 1) && (LIBGIT2_VER_MINOR < 4) ) +# define gcb_git_buf_reserved asize
Only putting `#if` around each use (there are only two AFAICT so it would be not too intrusive, but lots of trailing space removal noise might be hiding more) but this is fine here.
@eht16 commented on this pull request.
@@ -49,6 +49,11 @@
# define git_buf_dispose git_buf_free # define git_error_last giterr_last #endif +#if ! defined (LIBGIT2_VER_MINOR) || ( (LIBGIT2_VER_MAJOR == 1) && (LIBGIT2_VER_MINOR < 4) ) +# define gcb_git_buf_reserved asize
I ruled out `if`ing the two occurrences pretty early. Since there are already compability macros in place, it seemed appropriate to follow this pattern here.
Regarding the whitespace changes: should I remove them from this PR? For reviewing, you can also hide them in the "Files" tab here: 
@elextr commented on this pull request.
@@ -49,6 +49,11 @@
# define git_buf_dispose git_buf_free # define git_error_last giterr_last #endif +#if ! defined (LIBGIT2_VER_MINOR) || ( (LIBGIT2_VER_MAJOR == 1) && (LIBGIT2_VER_MINOR < 4) ) +# define gcb_git_buf_reserved asize
To be clear, my comment was not an objection, I was just noting the only __other__ way I can think of ... not a __better__ way.
Regarding the whitespace, if it was Geany I would recommend a separate PR, or at least a separate commit, but since this is a plugin, if the maintainer doesn't object, its fine.
@eht16 pushed 1 commit.
8e2ee31baa0da2aa8c6407a86a2595c83ffacd6e GitChangeBar: Support libgit2 1.4.x
I removed the whitespace changes, so it is clean now.
Well _thats_ a whole lot more reviewable :smile:
I am wondering how safe testing `reserved` for zero and assigning to it are?
AFAICT "probably" ok, otherwise there needs to be a runtime check as well as the compile time check.
I'm not sure about the relevance of runtime checks. At least, the changes in this PR won't make it worse than the other compatibility macros before, I'd say.
At least, the changes in this PR won't make it worse than the other compatibility macros before, I'd say.
I didn't think it had any compatibility checks before, thats why it refused to compile with a version of libgit2 that had `reserved` not `asize`.
I probably have not explained my concerns well enough.
I am not an expert on libgit, but IIUC the change in libgit is not simply that the field name was changed, but that the field is now unused. But this patch still tests and assigns to the unused field. But whatever value it is testing for will not be set and whatever value it is trying to pass are ignored by libgit because the field is unused.
It seems to me that if the test is still needed or the value is still supposed to be passed to libgit then we should be testing something else, or passing the value somewhere else, not using `reserved` as if it is still `asize`.
Good point.
I think it could be ok, `git_buf_grow` is called only once in the code directly and in all cases I've tested the `asize` field was non-zero, i.e. the `git_buf` instance was allocated by libgit2 and so the custom code in `gcb_git_buf_grow` was never used.
However, either I'm wrong or there was a previous use case where this was necessary. As long as I don't know this, it's hard to say what's correct. Interestingly, newer versions of libgit2 (I think it was since 0.23 or so) even do not allow using `git_buf_grow` with a custom allocated buffer and raise an error instead. So if the plugin would use a custom allocated buffer, we probably would see errors (or at least warnings in the logs). Or I misunderstood the code.
Anyway, I can't spend more time on this.
Maybe someone else wants (@b4n?).
@elextr is right that the logic wouldn't make sense if we just started using the `reserved` field instead -- although in most cases it would still actually work -- as the code gives it a very definite meaning.
Interestingly, newer versions of libgit2 (I think it was since 0.23 or so) even do not allow using git_buf_grow with a custom allocated buffer and raise an error instead. So if the plugin would use a custom allocated buffer, we probably would see errors (or at least warnings in the logs). Or I misunderstood the code.
Indeed, that probably just shows that despite the documentation `git_blob_filtered_content()` never actually returns an internal buffer. And looking at the code, 1.4 doesn't seem to ever be able to do so (in a mostly reliable way ATM).
---
Anyway, I created #1178 which should solve it, although it still makes an assumption on what the API actually does no matter what the docs suggest (yet, the docs have had a history of not being up-to-date on developer's mindset), which unfortunately is the only way to avoid unnecessary copies in the common cases.
This shows once more that libgit2 is not a stable API and thus a little tiresome to use, but well.
Closed #1165.
@b4n commented on this pull request.
@@ -49,6 +49,11 @@
# define git_buf_dispose git_buf_free # define git_error_last giterr_last #endif +#if ! defined (LIBGIT2_VER_MINOR) || ( (LIBGIT2_VER_MAJOR == 1) && (LIBGIT2_VER_MINOR < 4) ) +# define gcb_git_buf_reserved asize
I appreciate the effort on not riddling the code with `#if`s, but the point of the compatibility code is to use new APIs and map it to old one where needed, so that the rest of the code is not using the deprecated/removed names. Introducing a virtual name is not really doing the same, and kind of makes this awkward IMO, so here I'd rather have `#if`s carefully placed in the code, potentially with helper functions.
github-comments@lists.geany.org