Re: [PATCH v3 5/6] git-std-lib: introduce git standard library

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Calvin

On 08/09/2023 18:44, Calvin Wan wrote:
> +ifndef GIT_STD_LIB
>   LIB_OBJS += abspath.o
>   LIB_OBJS += add-interactive.o
>   LIB_OBJS += add-patch.o
> @@ -1196,6 +1198,27 @@ LIB_OBJS += write-or-die.o
>   LIB_OBJS += ws.o
>   LIB_OBJS += wt-status.o
>   LIB_OBJS += xdiff-interface.o
> +else ifdef GIT_STD_LIB
> +LIB_OBJS += abspath.o
> +LIB_OBJS += ctype.o
> +LIB_OBJS += date.o
> +LIB_OBJS += hex-ll.o
> +LIB_OBJS += parse.o
> +LIB_OBJS += strbuf.o
> +LIB_OBJS += usage.o
> +LIB_OBJS += utf8.o
> +LIB_OBJS += wrapper.o

It is still not clear to me how re-using LIB_OBJS like this is 
compatible with building libgit.a and git-stb-lib.a in a single make 
process c.f. [1].

> +ifdef GIT_STD_LIB
> +	BASIC_CFLAGS += -DGIT_STD_LIB
> +	BASIC_CFLAGS += -DNO_GETTEXT

As I've said before [2] I think that being able to built git-std-lib.a 
with gettext support is a prerequisite for using it to build git (just 
like trace2 support is). If we cannot build git using git-std-lib then 
the latter is likely to bit rot and so I don't think git-std-lib should 
be merged until there is a demonstration of building git using it.


> +### Libified Git rules
> +
> +# git-std-lib
> +# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease STUB_PAGER=YesPlease`
> +STD_LIB = git-std-lib.a
> +
> +$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
> +	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^

This is much nicer that the previous version.

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 3e7a59b5ff..14bf71c530 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -455,8 +455,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 thought we'd agreed that this represents a change in behavior that 
should be fixed c.f. [2]

> @@ -1462,14 +1464,17 @@ static inline int is_missing_file_error(int errno_)
>   	return (errno_ == ENOENT || errno_ == ENOTDIR);
>   }
>   
> +#ifndef GIT_STD_LIB
>   int cmd_main(int, const char **);
>   
>   /*
>    * Intercept all calls to exit() and route them to trace2 to
>    * optionally emit a message before calling the real exit().
>    */
> +

Nit: this blank line seems unnecessary

>   int common_exit(const char *file, int line, int code);
>   #define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
> +#endif
>   
>   /*
>    * You can mark a stack variable with UNLEAK(var) to avoid it being
> diff --git a/stubs/pager.c b/stubs/pager.c

> diff --git a/stubs/pager.h b/stubs/pager.h
> new file mode 100644
> index 0000000000..b797910881
> --- /dev/null
> +++ b/stubs/pager.h
> @@ -0,0 +1,6 @@
> +#ifndef PAGER_H
> +#define PAGER_H
> +
> +int pager_in_use(void);
> +
> +#endif /* PAGER_H */

Is this file actually used for anything? pager_in_use() is already 
declared in pager.h in the project root directory.

> diff --git a/wrapper.c b/wrapper.c
> index 7da15a56da..eeac3741cf 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -5,7 +5,6 @@
>   #include "abspath.h"
>   #include "parse.h"
>   #include "gettext.h"
> -#include "repository.h"

It is probably worth splitting this change out with a commit message 
explaining why the include is unneeded.

This is looking good, it would be really nice to see a demonstration of 
building git using git-std-lib (with gettext support) in the next iteration.

Best Wishes

Phillip


[1] 
https://lore.kernel.org/git/a0f04bd7-3a1e-b303-fd52-eee2af4d38b3@gmail.com/
[2] 
https://lore.kernel.org/git/CAFySSZBMng9nEdCkuT5+fc6rfFgaFfU2E0NP3=jUQC1yRcUE6Q@mail.gmail.com/