Re: [RFC PATCH 7/8] git-std-lib: introduce git standard library

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 28, 2023, 13:27 UTC
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"

Show 11 quoted lines

> 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

Show 13 quoted lines

> 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