Re: [PATCH v3 5/6] git-std-lib: introduce git standard library
From: Phillip Wood <hidden>
Date: 2023-09-11 21:38:36
Hi Calvin On 08/09/2023 18:44, Calvin Wan wrote:
quoted hunk ↗ jump to hunk
+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].
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+### 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.
quoted hunk ↗ jump to hunk
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]
quoted hunk ↗ jump to hunk
@@ -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
quoted hunk ↗ jump to hunk
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 beingdiff --git a/stubs/pager.c b/stubs/pager.c
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
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/ (local) [2] https://lore.kernel.org/git/CAFySSZBMng9nEdCkuT5+fc6rfFgaFfU2E0NP3=jUQC1yRcUE6Q@mail.gmail.com/ (local)