Calvin Wan wrote: > As a library boundary, wrapper.c should not directly log trace2 > statistics, but instead provide those statistics upon > request. Therefore, move the trace2 logging code to trace2.[ch.]. This > also allows wrapper.c to not be dependent on trace2.h and repository.h. > ... > diff --git a/trace2.h b/trace2.h > index 4ced30c0db..689e9a4027 100644 > --- a/trace2.h > +++ b/trace2.h > @@ -581,4 +581,9 @@ void trace2_collect_process_info(enum trace2_process_info_reason reason); > > const char *trace2_session_id(void); > > +/* > + * Writes out trace statistics for fsync > + */ > +void trace_git_fsync_stats(void); > + This function does not belong in 'trace2.h', IMO. The purpose of that file is to contain the generic API for Trace2 (e.g., 'trace2_printf()', 'trace2_region_(enter|exit)'), whereas this function is effectively a wrapper around a specific invocation of that API. You note in the commit message that "wrapper.c should not directly log trace2 statistics" with the reasoning of "[it's] a library boundary," but I suspect the unstated underlying reason is "because it tracks 'count_fsync_*' in static variables." This case would be better handled, then, by replacing the usage in 'wrapper.c' with a new Trace2 counter (API introduced in [1]). That keeps this usage consistent with the API already established for Trace2, rather than starting an unsustainable trend of creating ad-hoc, per-metric wrappers in 'trace2.[c|h]'. An added note re: the commit message - it's extremely important that functions _anywhere in Git_ are able to use the Trace2 API directly. A developer could reasonably want to measure performance, keep track of an interesting metric, log when a region is entered in the larger trace, capture error information, etc. for any function, regardless of where in falls in the internal library organization. To that end, I think either the commit message should be rephrased to remove that statement (if the issue is really "we're using a static variable and we want to avoid that"), or the libification effort should be updated to accommodate use of Trace2 anywhere in Git. [1] https://lore.kernel.org/git/pull.1373.v4.git.1666618868.gitgitgadget@xxxxxxxxx/
