From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 28 Jun 2023 13:27:04 GMT
Subject: Re: [RFC PATCH 7/8] git-std-lib: introduce git standard library
Message-ID: <d77f99df-f50a-39fa-a7c5-1bb958f196fb@gmail.com>
In-Reply-To: <20230627195251.1973421-8-calvinwan@google.com>

Hi Calvin

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"

> 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

> 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

Thanks for posting this RFC. I've only really given it a quick glance 
but on the whole it seems to make sense.

Best Wishes

Phillip