Variadic macros

[pks-t]

The macro git_parse_error is implemented in a variadic way so
that it's possible to pass printf-style parameters.
Unfortunately, variadic macros are not defined by C89 and thus we
cannot use that functionality. But as we have implemented
git_error_vset in the previous commit, we can now just use that
instead.

Convert git_parse_error to a variadic function and use
git_error_vset to fix the compliance violation.

---

Edit: fixed more locations to be compliant

[ethomson]

The C89 statement was - when written - meant to be twofold:

• the API for consumers is C89
• it compiles under MSVC

The latter isn’t really C89, but for older versions that’s the spec that it fully implemented, with some new stuff sprinkled in.

Those are the only two things that I care about as well.

It’s 2019, what’s the value in being “C89 compliant” within our (internal) code base and removing variadic macros?

[pks-t]

The other question is: what's the downside? It's not like the code is getting significantly more complicated, the conversion is quite trivial. And I can totally relate to every report of non-C89 code if we officially claim to be C89 compliant. Either we are, or we are not compliant. If somebody comes along with a C89-only compiler (and these do still exist in niche communities), then that person should be able to compile libgit2

[ethomson]

The other question is: what's the downside?

I can think of a few - first, the maintainability of this presents a challenge, unless we actually use a compiler that enforces C89 then we're bound to either add back variadic macros without noticing or other not-quite-C89 compliant things.

Second is the perf - I don't mind an additional function call during error handling, since that's not a hot path. But tracing is. Whenever we stick a bunch of git_trace statements into the library, it's to track down some problem in production that has so far eluded us. Tracing - done poorly - can destroy our perf, so we need to be very surgical with it and avoid any unnecessary work. inline is just a hint, IIRC, and I'd hate for the compiler to decide not to inline. I realize that this is unlikely, but it is a consideration.

But the biggest, IMO, is opportunity cost. I feel like we have a decent amount of work to do and only a little time to do it. Focusing on this means less time for us to focus on perf problems, API stabilization, bugfixing, etc.

So I think that we need to balance the goal of C89 compliance with everything else that we could be doing.

You mention nice compilers - and I am sympathetic to this - but it's also clear that nobody's tried to actually compile this in practice on a strictly C89 compiler. (The trace code, for example, has been unchanged for at least 5 years.)

My hunch is that if somebody tries to compile it with something wacky, that it will have bits and pieces of support for a variety of standards (like crappy old MSVC versions) and we'll get bug reports for those particular things that aren't supported.

Anyway, that's my thinking. I would prefer to update the websites / docs / etc to say that we have a "C89 API". C99 is twenty years old now, I don't think that we should feel obligated to not use it within the library itself.

[ethomson]

Just to follow up, I'm not totally opposed to this, but I do want to take the new tracing indirection through its paces before I'm 👍 on it.

[pks-t]

I can think of a few - first, the maintainability of this presents a challenge, unless we actually use a compiler that enforces C89 then we're bound to either add back variadic macros without noticing or other not-quite-C89 compliant things.

Yeah, it sucks that most compilers aren't strict by default if adding the equivalent of -std=c89. I thought about giving -Wpedantic a go, but I bet it will generate quite a wall of warnings.

Second is the perf - I don't mind an additional function call during error handling, since that's not a hot path. But tracing is. Whenever we stick a bunch of git_trace statements into the library, it's to track down some problem in production that has so far eluded us. Tracing - done poorly - can destroy our perf, so we need to be very surgical with it and avoid any unnecessary work. inline is just a hint, IIRC, and I'd hate for the compiler to decide not to inline. I realize that this is unlikely, but it is a consideration.

To be honest, I don't quite get the tracing functionality. What's it supposed to do? We don't use it anywhere in our code, and providing tracing functionality so that callers of libgit2 can trace their own code seems backwards to me.

But the biggest, IMO, is opportunity cost. I feel like we have a decent amount of work to do and only a little time to do it. Focusing on this means less time for us to focus on perf problems, API stabilization, bugfixing, etc.

I think this greatly overestimates the cost of maintaining C89 compliance. We historically didn't have a lot of things to fix in order to be compliant, so I'd argue that not too much time was wasted on this. Furthermore, by fixing existing compliance issues we're not really creating additional costs going forward.

Anyway, that's my thinking. I would prefer to update the websites / docs / etc to say that we have a "C89 API". C99 is twenty years old now, I don't think that we should feel obligated to not use it within the library itself.

I don't know. I would like to avoid using several C99 features, like inline variable declarations. If we lift the C99 restriction and change our compiler to use -std=c99, then it's only a matter of time until things like that sneak into our codebase. Furthermore, I'd argue that we would in fact waste more time like that if we have to manually tell PR authors to use C89 style for some things, where the compiler could've done that work for us right when the author was writing code.

Just to follow up, I'm not totally opposed to this, but I do want to take the new tracing indirection through its paces before I'm 👍 on it.

Fair enough. I'll see what I can do there, but it's really hard for me to grasp as there's actually no users in our codebase except for our tests. Could you maybe provide some more details on its intentions?

[ethomson]

To be honest, I don't quite get the tracing functionality. What's it supposed to do? We don't use it anywhere in our code, and providing tracing functionality so that callers of libgit2 can trace their own code seems backwards to me.

Tracing isn't there for people to trace their code, it's for them (and us) to be able to trace libgit2 itself and put the results in whatever storage format they're already using or comfortable with that they can run analytics on. With the git_trace mechanism there (and in the bindings) then when someone is investigating a problem in libgit2, they only need to go to that area (in libgit2) and add a few git_trace statements in to get a better understanding of the problem.

They can keep the trace points always registered in their libgit2 so that plumbing is already ready to go, even if there isn't any tracing to be done.

If we didn't have git_trace then these people would have to hack libgit2 up to be aware of their internal tracing infrastructure. This would be particularly annoying if they're in a managed language. So having it always ready to go is a big help.

It's true that we don't have any git_trace statements in the codebase itself. I sort of expected we would when I added this, but it hasn't seemed particularly useful to me.

But tracing has been useful: to give you a concrete examples, Azure Repos always builds libgit2 with tracing support. We register that (via LibGit2Sharp) to go to ETW logging and tracing which is a very efficient logging platform. But generally there are no git_trace statements. (I think.)

But we have added some when investigating a problem. Especially when we were onboarding projects into the pull request workflow, we decorated the merge code with trace statements to understand how things were or were not working and to understand perf issues.

The workflow that we often use is to set tracing to some high level to collect data for a very brief period of time and then turn it back off. Even very efficient tracing is a drag if you're collecting data en masse. So when we turn it off we want it to be as close to a no-op as absolutely possible.

I have a long-term goal of trying to increase pull request speed for the Windows repository, which will definitely involve some trace statements, but I haven't been able to attack that yet.

[pks-t]

Fair enough, thanks for your explanation!

[pks-t]

The ironic thing is: the only thing that understandably keeps you from accepting this is also the only real issue we have according to your statement, as it's the only place where we expose a non-C89 API :)

I'm trying to think of any way to implement this in a non-variadic way, but I cannot really think of any. We could at least force-inline it, but that's not a proper solution, either:

diff --git a/src/trace.h b/src/trace.h
index ed0fa0949..8c3afa7f3 100644
--- a/src/trace.h
+++ b/src/trace.h
@@ -23,27 +23,37 @@ extern struct git_trace_data git_trace__data;
 
 #define git_trace_level() (git_trace__data.level)
 
-GIT_INLINE(void) git_trace(git_trace_level_t level, const char *fmt, ...)
+GIT_INLINE(void) git_trace__write_fmt(git_trace_level_t level, const char *fmt, va_list ap);
 {
 	git_trace_cb callback = git_trace__data.callback;
 	git_buf message = GIT_BUF_INIT;
-	va_list ap;
 
-	if (git_trace__data.level < level || !git_trace__data.callback)
-		return;
-
-	va_start(ap, fmt);
 	git_buf_vprintf(&message, fmt, ap);
-	va_end(ap);
-
 	callback(level, git_buf_cstr(&message));
-
 	git_buf_dispose(&message);
 }
 
+#if defined(_MSC_VER)
+# define GIT_ALWAYS_INLINE(type) static __forceinline type
+#elif defined(__GNUC__)
+# define GIT_ALWAYS_INLINE(type) static inline __attribute__((__always_inline__)) type
 #else
+# define GIT_ALWAYS_INLINE(type) static type
+#endif
 
-GIT_INLINE(void) git_trace__null(
+GIT_ALWAYS_INLINE(void) git_trace(git_trace_level_t level, const char *fmt, ...)
+{
+	if (git_trace__data.level >= level && git_trace__data.callback != NULL) {
+		va_list ap;
+		va_start(ap, fmt);
+		git_trace__write_fmt(l, fmt, ap);
+		va_end(ap);
+	}
+}
+
+#else
+
+GIT_ALWAYSINLINE(void) git_trace__null(
 	git_trace_level_t level,
 	const char *fmt, ...)
 {

[ethomson]

The ironic thing is: the only thing that understandably keeps you from accepting this is also the only real issue we have according to your statement, as it's the only place where we expose a non-C89 API :)

It's not exposed to end-users, though, git_trace is only ever used internally. (The tracing setup is exposed, but not variadic.)

[pks-t]

Sorry if I'm missing your point. git_trace is exposed via "include/git2/trace.h", so I'd very much argue that it is exposed to end-users.

[ethomson]

But not the variadic bits - only git_trace_set, the levels enum and the callback definition is public.

[pks-t]

Gotcha, somehow I've missed that. So yeah, as I don't have any fix available, should I just drop the tracing part of this PR for now and at least get the other parts in?

[ethomson]

Oh jeez I missed this question. I'm +1 on doing this except for the tracing bits. As I mentioned, I'd like to have a play with the perf on changes there, and it's definitely a high priority for me. Unfortunately, that means ... a little while still? :/

[pks-t]

Rebased to fix conflicts. I've also dropped the tracing part from this PR for now.

[ethomson]

👍