> On 27/06/2023 20:52, Calvin Wan wrote: > > The Git Standard Library intends to serve as the foundational library > > and root dependency that other libraries in Git will be built off of. > > That is to say, suppose we have libraries X and Y; a user that wants to > > use X and Y would need to include X, Y, and this Git Standard Library. > > I think having a library of commonly used functions and structures is a > good idea. While I appreciate that we don't want to include everything > I'm surprised to see it does not include things like "hashmap.c" and > "string-list.c" that will be required by the config library as well as > other code in "libgit.a". I don't think we want "libgitconfig.a" and > "libgit.a" to both contain a copy of "hashmap.o" and "string-list.o" I chose not to include hashmap and string-list in git-std-lib.a in the first pass since they can exist as libraries built on top of git-std-lib.a. There is no harm starting off with more libraries than fewer besides having something like the config library be dependent on lib-hashmap.a, lib-string-list.a, and git-std-lib.a rather than only git-std-lib.a. They can always be added into git-std-lib.a in the future. That being said, I do find it extremely unlikely that someone would want to swap out the implementation for hashmap or string-list so it is also very reasonable to include them into git-std-lib.a > > > diff --git a/Makefile b/Makefile > > index e9ad9f9ef1..255bd10b82 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -2162,6 +2162,11 @@ ifdef FSMONITOR_OS_SETTINGS > > COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o > > endif > > > > +ifdef GIT_STD_LIB > > + BASIC_CFLAGS += -DGIT_STD_LIB > > + BASIC_CFLAGS += -DNO_GETTEXT > > I can see other projects may want to build git-std-lib without gettext > support but if we're going to use git-std-lib within git it needs to be > able to be built with that support. The same goes for the trace > functions that you are redefining in usage.h Taking a closer look at gettext.[ch], I believe I can also include it into git-std-lib.a with a couple of minor changes. I'm currently thinking about how the trace functions should interact with git-std-lib.a since Victoria had similar comments on patch 1. I'll reply to that thread when I come up with an answer. > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index 481dac22b0..75aa9b263e 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -396,8 +396,8 @@ static inline int noop_core_config(const char *var UNUSED, > > #define platform_core_config noop_core_config > > #endif > > > > +#if !defined(__MINGW32__) && !defined(_MSC_VER) && !defined(GIT_STD_LIB) > > int lstat_cache_aware_rmdir(const char *path); > > -#if !defined(__MINGW32__) && !defined(_MSC_VER) > > #define rmdir lstat_cache_aware_rmdir > > #endif > > I'm not sure why the existing condition is being moved here Ah I see that this changes behavior for callers of lstat_cache_aware_rmdir if those conditions aren't satisfied. I should've added an extra #if for GIT_STD_LIB instead of adding it to the end of the current check and moving it. Thanks for spotting this.
