Deprecation: export the deprecated functions properly - #4979

This is good, but not enough to fix git_buf_free(): you need to also include deprecated.h in buffer.h.
Ideally every single entry from deprecated.h should be checked, which should not prove that difficult (adapt paths as needed):

$ sed -n 's/^.*GIT_EXTERN([^)]*) \([a-zA-Z0-9_]*\).*$/\1/p' include/git2/deprecated.h 
git_buf_free
giterr_last
giterr_clear
giterr_set_str
giterr_set_oom

From what I see, only giterr_* and git_buf_free need special attention, and so once you added the include in buffer.h we should be all set.

Also to check it's indeed properly included in the resulting library (amend paths as needed)

$ sed -n 's/^.*GIT_EXTERN([^)]*) \([a-zA-Z0-9_]*\).*$/\1/p' include/git2/deprecated.h | while read f; do objdump -T /usr/lib/libgit2.so | grep -qF $f || echo "$f is missing" >&2; done

Adding this WFM indeed:

diff --git a/src/buffer.c b/src/buffer.c
index 51fb48a45..61bd30471 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -10,6 +10,8 @@
 #include "buf_text.h"
 #include <ctype.h>
 
+#include <git2/deprecated.h>
+
 /* Used as default value for git_buf->ptr so that people can always
  * assume ptr is non-NULL and zero terminated even for new git_bufs.
  */

Thanks for the catch @b4n. I've moved the inclusion of deprecated.h into common.h, which all source files should include to ensure that we don't have to include deprecated.h piecemeal in a way that we could forget (like I did earlier).

I've also added some tests to ensure that we get the declarations for the deprecated functions (via common.h) when GIT_DEPRECATE_HARD is unset. Although this isn't bulletproof, this should help ensure that we handle deprecation rather responsibly. We'll get warnings about declarations if we use these deprecated functions without properly declaring them:

/Users/ethomson/libgit2/tests/core/deprecate.c:31:2: warning:
      implicit declaration of function 'giterr_set_oom'
      [-Wimplicit-function-declaration]
        giterr_set_oom();
        ^

(These warnings will be errors in the CI since we build with -Werror there.)

Obviously there's still possibilities here for a developer to introduce a new deprecated function that does not end up in this test case but this is a good start. Long term we could add another nightly build that produces a normally deprecated version of the library and ensures that the symbols are actually present and properly exported, but I think that this is a good step for now.

Ha ha, thanks MSVC and its precompiled headers. 😡

OK, I'm dropping the test. I thought it would work out well enough, but precompiled headers on MSVC are proving challenging. (Since GIT_DEPRECATE_HARD is defined, it can be brought in the precompiled header, which is later implicitly included to speed up compilation.) Thus a test trying to #undef it won't work? And I can't pass /Y- to a single file in cmake, even with SET_SOURCE_FILE_PROPERTIES?

When the MSVC builds have GIT_DEPRECATE_HARD defined, they don't see the declaration. Which means they think that giterr_last returns an int not a pointer, so they're treating it like a 32 bit. Which means they're looking at the wrong pointer value. Oops!

Anyway, I don't know how to fix this offhand and as the disclaimer above points out, these tests are Not Really Perfect anyway, so I'm just going to drop them. I confirmed manually that we export the git_buf and giterr functions, and we won't be doing any more deprecation in the v0.28 branch.

So I want to get this merged and 0.28.1 released so that anybody else picking up the release won't see this breakage. We can plan a better test strategy for vnext. (Building a version of the library without any deprecation and ensuring that we can link to it would be my preference.)