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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Sep 15, 2023, 18:39 UTC
Message-ID: <20230915183927.1597414-1-jonathantanmy@google.com>
In-Reply-To: <20230908174443.1027716-5-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 12 quoted lines

> diff --git a/Makefile b/Makefile
> index 9226c719a0..0a2d1ae3cc 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -669,6 +669,7 @@ FUZZ_PROGRAMS =
>  GIT_OBJS =
>  LIB_OBJS =
>  SCALAR_OBJS =
> +STUB_OBJS =
>  OBJECTS =
>  OTHER_PROGRAMS =
>  PROGRAM_OBJS =

I don't think stubs should be compiled into git-std-lib.a - I would expect a consumer of this library to be able to specify their own implementations if needed (e.g. their own trace2).

Show 22 quoted lines

> @@ -956,6 +957,7 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
>  
>  LIB_H = $(FOUND_H_SOURCES)
>  
> +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

This means that LIB_OBJS (in this patch, used both by git-std-lib and as part of compiling the regular Git binary) can differ based on the GIT_STD_LIB variable. It does seem that we cannot avoid GIT_STD_LIB for now, because the git-std-lib can only be compiled without GETTEXT (so we need a variable to make sure that none of these .o files are compiled with GETTEXT), but we should still minimize the changes between compiling with GIT_STD_LIB and without it, at least to minimize future work. Could we have two separate lists? So, leave LIB_OBJS alone and make a new STD_LIB_OBJS.

Show 13 quoted lines

> 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 think we still want to keep the idea of "the code should still be good even if we have no use for git-std-lib" as much as possible, so could we stub lstat_cache_aware_rmdir() instead? We could have a new git-compat- util-stub.c (or whatever we want to call it).

Show 12 quoted lines

> @@ -966,9 +966,11 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);
>  #endif
>  
>  #ifdef NO_PTHREADS
> +#ifdef GIT_STD_LIB
>  #define atexit git_atexit
>  int git_atexit(void (*handler)(void));
>  #endif
> +#endif
>  
>  static inline size_t st_add(size_t a, size_t b)
>  {

Same for git_atexit().

Show 18 quoted lines

> @@ -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().
>   */
> +
>  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

And for common_exit().

As for cmd_main(), that seems to be a convenience so that we can link
common_main.o with various other files (e.g. http-backend.c). I think
the right thing to do is to define a new cmd-main.h that declares only
cmd_main(), and then have only the files that need it (common_main.c and
all the files that define cmd_main()) include it. This cleanup patch can
be done before this patch. I think this is a good change that we would
want even without libification.