mbox series

[RFC,v2,0/7] Introduce Git Standard Library

Message ID 20230810163346.274132-1-calvinwan@google.com (mailing list archive)
Headers show
Series Introduce Git Standard Library | expand

Message

Calvin Wan Aug. 10, 2023, 4:33 p.m. UTC
Original cover letter:
https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/

In the initial RFC, I had a patch that removed the trace2 dependency
from usage.c so that git-std-lib.a would not have dependencies outside
of git-std-lib.a files. Consequently this meant that tracing would not
be possible in git-std-lib.a files for other developers of Git, and it
is not a good idea for the libification effort to close the door on
tracing in certain files for future development (thanks Victoria for
pointing this out). That patch has been removed and instead I introduce
stubbed out versions of repository.[ch] and trace2.[ch] that are swapped
in during compilation time (I'm no Makefile expert so any advice on how
on I could do this better would be much appreciated). These stubbed out
files contain no implementations and therefore do not have any
additional dependencies, allowing git-std-lib.a to compile with only the
stubs as additional dependencies. This also has the added benefit of
removing `#ifdef GIT_STD_LIB` macros in C files for specific library
compilation rules. Libification shouldn't pollute C files with these
macros. The boundaries for git-std-lib.a have also been updated to
contain these stubbed out files.

I have also made some additional changes to the Makefile to piggy back
off of our existing build rules for .c/.o targets and their
dependencies. As I learn more about Makefiles, I am continuing to look
for ways to improve these rules. Eventually I would like to be able to
have a set of rules that future libraries can emulate and is scalable
in the sense of not creating additional toil for developers that are not
interested in libification.

Calvin Wan (7):
  hex-ll: split out functionality from hex
  object: move function to object.c
  config: correct bad boolean env value error message
  parse: create new library for parsing strings and env values
  date: push pager.h dependency up
  git-std-lib: introduce git standard library
  git-std-lib: add test file to call git-std-lib.a functions

 Documentation/technical/git-std-lib.txt | 186 ++++++++++++++++++
 Makefile                                |  64 ++++++-
 attr.c                                  |   2 +-
 builtin/blame.c                         |   2 +-
 builtin/log.c                           |   2 +-
 color.c                                 |   2 +-
 config.c                                | 173 +----------------
 config.h                                |  14 +-
 date.c                                  |   5 +-
 date.h                                  |   2 +-
 git-compat-util.h                       |   7 +-
 hex-ll.c                                |  49 +++++
 hex-ll.h                                |  27 +++
 hex.c                                   |  47 -----
 hex.h                                   |  24 +--
 mailinfo.c                              |   2 +-
 object.c                                |   5 +
 object.h                                |   6 +
 pack-objects.c                          |   2 +-
 pack-revindex.c                         |   2 +-
 parse-options.c                         |   3 +-
 parse.c                                 | 182 ++++++++++++++++++
 parse.h                                 |  20 ++
 pathspec.c                              |   2 +-
 preload-index.c                         |   2 +-
 progress.c                              |   2 +-
 prompt.c                                |   2 +-
 rebase.c                                |   2 +-
 ref-filter.c                            |   3 +-
 revision.c                              |   3 +-
 strbuf.c                                |   2 +-
 stubs/repository.c                      |   4 +
 stubs/repository.h                      |   8 +
 stubs/trace2.c                          |  22 +++
 stubs/trace2.h                          |  69 +++++++
 symlinks.c                              |   2 +
 t/Makefile                              |   4 +
 t/helper/test-date.c                    |   3 +-
 t/helper/test-env-helper.c              |   2 +-
 t/stdlib-test.c                         | 239 ++++++++++++++++++++++++
 unpack-trees.c                          |   2 +-
 url.c                                   |   2 +-
 urlmatch.c                              |   2 +-
 wrapper.c                               |   8 +-
 wrapper.h                               |   5 -
 write-or-die.c                          |   2 +-
 46 files changed, 925 insertions(+), 295 deletions(-)
 create mode 100644 Documentation/technical/git-std-lib.txt
 create mode 100644 hex-ll.c
 create mode 100644 hex-ll.h
 create mode 100644 parse.c
 create mode 100644 parse.h
 create mode 100644 stubs/repository.c
 create mode 100644 stubs/repository.h
 create mode 100644 stubs/trace2.c
 create mode 100644 stubs/trace2.h
 create mode 100644 t/stdlib-test.c

Range-diff against v1:
1:  f7abe7a239 < -:  ---------- trace2: log fsync stats in trace2 rather than wrapper
2:  c302ae0052 = 1:  78634bc406 hex-ll: split out functionality from hex
3:  74e8e35ae2 ! 2:  21ec1d276e object: move function to object.c
    @@ wrapper.c
      #include "config.h"
      #include "gettext.h"
     -#include "object.h"
    + #include "repository.h"
      #include "strbuf.h"
    - 
    - static intmax_t count_fsync_writeout_only;
    + #include "trace2.h"
     @@ wrapper.c: int rmdir_or_warn(const char *file)
      	return warn_if_unremovable("rmdir", file, rmdir(file));
      }
4:  419c702633 = 3:  41dcf8107c config: correct bad boolean env value error message
5:  a325002438 ! 4:  3e800a41c4 parse: create new library for parsing strings and env values
    @@ wrapper.c
     -#include "config.h"
     +#include "parse.h"
      #include "gettext.h"
    + #include "repository.h"
      #include "strbuf.h"
    - 
     
      ## write-or-die.c ##
     @@
6:  475190310a < -:  ---------- pager: remove pager_in_use()
-:  ---------- > 5:  7a4a088bc3 date: push pager.h dependency up
7:  d7f4d4a137 ! 6:  c9002734d0 git-std-lib: introduce git standard library
    @@ Documentation/technical/git-std-lib.txt (new)
     +easily included in git-std-lib.a, which as a root dependency means that
     +higher level libraries do not have to worry about compatibility files in
     +compat/. The rest of the functions defined in git-compat-util.h are
    -+implemented in top level files and, in this patch set, are hidden behind
    ++implemented in top level files and are hidden behind
     +an #ifdef if their implementation is not in git-std-lib.a.
     +
     +Rationale summary
    @@ Documentation/technical/git-std-lib.txt (new)
     + - low-level git/* files with functions defined in git-compat-util.h
     +   (ctype.c)
     + - compat/*
    ++ - stubbed out dependencies in stubs/ (stubs/repository.c, stubs/trace2.c)
     +
     +There are other files that might fit this definition, but that does not
     +mean it should belong in git-std-lib.a. Those files should start as
     +their own separate library since any file added to git-std-lib.a loses
     +its flexibility of being easily swappable.
     +
    ++Wrapper.c and usage.c have dependencies on repository and trace2 that are
    ++possible to remove at the cost of sacrificing the ability for standard Git
    ++to be able to trace functions in those files and other files in git-std-lib.a.
    ++In order for git-std-lib.a to compile with those dependencies, stubbed out
    ++versions of those files are implemented and swapped in during compilation time.
    ++
     +Files inside of Git Standard Library
     +================
     +
    @@ Documentation/technical/git-std-lib.txt (new)
     +usage.c
     +utf8.c
     +wrapper.c
    ++stubs/repository.c
    ++stubs/trace2.c
     +relevant compat/ files
     +
     +Pitfalls
     +================
     +
    -+In patch 7, I use #ifdef GIT_STD_LIB to both stub out code and hide
    -+certain function headers. As other parts of Git are libified, if we
    -+have to use more ifdefs for each different library, then the codebase
    -+will become uglier and harder to understand. 
    -+
     +There are a small amount of files under compat/* that have dependencies
     +not inside of git-std-lib.a. While those functions are not called on
     +Linux, other OSes might call those problematic functions. I don't see
    @@ Documentation/technical/git-std-lib.txt (new)
      \ No newline at end of file
     
      ## Makefile ##
    +@@ Makefile: FUZZ_PROGRAMS =
    + GIT_OBJS =
    + LIB_OBJS =
    + SCALAR_OBJS =
    ++STUB_OBJS =
    + OBJECTS =
    + OTHER_PROGRAMS =
    + PROGRAM_OBJS =
    +@@ Makefile: 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
    +@@ Makefile: 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
    ++
    ++ifdef STUB_REPOSITORY
    ++STUB_OBJS += stubs/repository.o
    ++endif
    ++
    ++ifdef STUB_TRACE2
    ++STUB_OBJS += stubs/trace2.o
    ++endif
    ++
    ++LIB_OBJS += $(STUB_OBJS)
    ++endif
    + 
    + BUILTIN_OBJS += builtin/add.o
    + BUILTIN_OBJS += builtin/am.o
     @@ Makefile: ifdef FSMONITOR_OS_SETTINGS
      	COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o
      endif
    @@ Makefile: $(FUZZ_PROGRAMS): all
     +### Libified Git rules
     +
     +# git-std-lib
    -+# `make git-std-lib GIT_STD_LIB=YesPlease`
    ++# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
     +STD_LIB = git-std-lib.a
     +
    -+GIT_STD_LIB_OBJS += abspath.o
    -+GIT_STD_LIB_OBJS += ctype.o
    -+GIT_STD_LIB_OBJS += date.o
    -+GIT_STD_LIB_OBJS += hex-ll.o
    -+GIT_STD_LIB_OBJS += parse.o
    -+GIT_STD_LIB_OBJS += strbuf.o
    -+GIT_STD_LIB_OBJS += usage.o
    -+GIT_STD_LIB_OBJS += utf8.o
    -+GIT_STD_LIB_OBJS += wrapper.o
    -+
    -+$(STD_LIB): $(GIT_STD_LIB_OBJS) $(COMPAT_OBJS)
    ++$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
     +	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
     +
    -+git-std-lib: $(STD_LIB)
    ++TEMP_HEADERS = temp_headers/
    ++
    ++git-std-lib:
    ++# Move headers to temporary folder and replace them with stubbed headers.
    ++# After building, move headers and stubbed headers back.
    ++ifneq ($(STUB_OBJS),)
    ++	mkdir -p $(TEMP_HEADERS); \
    ++	for d in $(STUB_OBJS); do \
    ++		BASE=$${d%.*}; \
    ++		mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
    ++		mv $${BASE}.h $${BASE##*/}.h; \
    ++	done; \
    ++	$(MAKE) $(STD_LIB); \
    ++	for d in $(STUB_OBJS); do \
    ++		BASE=$${d%.*}; \
    ++		mv $${BASE##*/}.h $${BASE}.h; \
    ++		mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
    ++	done; \
    ++	rm -rf temp_headers
    ++else
    ++	$(MAKE) $(STD_LIB)
    ++endif
     
      ## git-compat-util.h ##
     @@ git-compat-util.h: static inline int noop_core_config(const char *var UNUSED,
    @@ git-compat-util.h: int git_access(const char *path, int mode);
      /*
       * You can mark a stack variable with UNLEAK(var) to avoid it being
     
    + ## stubs/repository.c (new) ##
    +@@
    ++#include "git-compat-util.h"
    ++#include "repository.h"
    ++
    ++struct repository *the_repository;
    +
    + ## stubs/repository.h (new) ##
    +@@
    ++#ifndef REPOSITORY_H
    ++#define REPOSITORY_H
    ++
    ++struct repository { int stub; };
    ++
    ++extern struct repository *the_repository;
    ++
    ++#endif /* REPOSITORY_H */
    +
    + ## stubs/trace2.c (new) ##
    +@@
    ++#include "git-compat-util.h"
    ++#include "trace2.h"
    ++
    ++void trace2_region_enter_fl(const char *file, int line, const char *category,
    ++			    const char *label, const struct repository *repo, ...) { }
    ++void trace2_region_leave_fl(const char *file, int line, const char *category,
    ++			    const char *label, const struct repository *repo, ...) { }
    ++void trace2_data_string_fl(const char *file, int line, const char *category,
    ++			   const struct repository *repo, const char *key,
    ++			   const char *value) { }
    ++void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names) { }
    ++void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
    ++			    va_list ap) { }
    ++void trace2_cmd_name_fl(const char *file, int line, const char *name) { }
    ++void trace2_thread_start_fl(const char *file, int line,
    ++			    const char *thread_base_name) { }
    ++void trace2_thread_exit_fl(const char *file, int line) { }
    ++void trace2_data_intmax_fl(const char *file, int line, const char *category,
    ++			   const struct repository *repo, const char *key,
    ++			   intmax_t value) { }
    ++int trace2_is_enabled(void) { return 0; }
    ++void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
    +
    + ## stubs/trace2.h (new) ##
    +@@
    ++#ifndef TRACE2_H
    ++#define TRACE2_H
    ++
    ++struct child_process { int stub; };
    ++struct repository;
    ++struct json_writer { int stub; };
    ++
    ++void trace2_region_enter_fl(const char *file, int line, const char *category,
    ++			    const char *label, const struct repository *repo, ...);
    ++
    ++#define trace2_region_enter(category, label, repo) \
    ++	trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
    ++
    ++void trace2_region_leave_fl(const char *file, int line, const char *category,
    ++			    const char *label, const struct repository *repo, ...);
    ++
    ++#define trace2_region_leave(category, label, repo) \
    ++	trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
    ++
    ++void trace2_data_string_fl(const char *file, int line, const char *category,
    ++			   const struct repository *repo, const char *key,
    ++			   const char *value);
    ++
    ++#define trace2_data_string(category, repo, key, value)                       \
    ++	trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
    ++			      (value))
    ++
    ++void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
    ++
    ++#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
    ++
    ++void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
    ++			    va_list ap);
    ++
    ++#define trace2_cmd_error_va(fmt, ap) \
    ++	trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
    ++
    ++
    ++void trace2_cmd_name_fl(const char *file, int line, const char *name);
    ++
    ++#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
    ++
    ++void trace2_thread_start_fl(const char *file, int line,
    ++			    const char *thread_base_name);
    ++
    ++#define trace2_thread_start(thread_base_name) \
    ++	trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
    ++
    ++void trace2_thread_exit_fl(const char *file, int line);
    ++
    ++#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
    ++
    ++void trace2_data_intmax_fl(const char *file, int line, const char *category,
    ++			   const struct repository *repo, const char *key,
    ++			   intmax_t value);
    ++
    ++#define trace2_data_intmax(category, repo, key, value)                       \
    ++	trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
    ++			      (value))
    ++
    ++enum trace2_process_info_reason {
    ++	TRACE2_PROCESS_INFO_STARTUP,
    ++	TRACE2_PROCESS_INFO_EXIT,
    ++};
    ++int trace2_is_enabled(void);
    ++void trace2_collect_process_info(enum trace2_process_info_reason reason);
    ++
    ++#endif /* TRACE2_H */
    ++
    +
      ## symlinks.c ##
     @@ symlinks.c: void invalidate_lstat_cache(void)
      	reset_lstat_cache(&default_cache);
    @@ symlinks.c: int lstat_cache_aware_rmdir(const char *path)
      	return ret;
      }
     +#endif
    -
    - ## usage.c ##
    -@@
    -  */
    - #include "git-compat-util.h"
    - #include "gettext.h"
    -+
    -+#ifdef GIT_STD_LIB
    -+#undef trace2_cmd_name
    -+#undef trace2_cmd_error_va
    -+#define trace2_cmd_name(x) 
    -+#define trace2_cmd_error_va(x, y)
    -+#else
    - #include "trace2.h"
    -+#endif
    - 
    - static void vreportf(const char *prefix, const char *err, va_list params)
    - {
8:  cb96e67774 ! 7:  0bead8f980 git-std-lib: add test file to call git-std-lib.a functions
    @@ t/stdlib-test.c (new)
     +	strbuf_splice(sb, 0, 1, "foo", 3);
     +	strbuf_insert(sb, 0, "foo", 3);
     +	// strbuf_vinsertf() called by strbuf_insertf
    -+	strbuf_insertf(sb, 0, "%s", "foo"); 
    ++	strbuf_insertf(sb, 0, "%s", "foo");
     +	strbuf_remove(sb, 0, 1);
     +	strbuf_add(sb, "foo", 3);
     +	strbuf_addbuf(sb, sb2);
    @@ t/stdlib-test.c (new)
     +	unlink(path);
     +	read_in_full(fd, &sb, 1);
     +	write_in_full(fd, &sb, 1);
    -+	pread_in_full(fd, &sb, 1, 0);	
    ++	pread_in_full(fd, &sb, 1, 0);
     +}
     +
     +int main() {
    @@ t/stdlib-test.c (new)
     +	fprintf(stderr, "all git-std-lib functions finished calling\n");
     +	return 0;
     +}
    - \ No newline at end of file

Comments

Glen Choo Aug. 10, 2023, 10:05 p.m. UTC | #1
Calvin Wan <calvinwan@google.com> writes:

> Calvin Wan (7):
>   hex-ll: split out functionality from hex
>   object: move function to object.c
>   config: correct bad boolean env value error message
>   parse: create new library for parsing strings and env values
>   date: push pager.h dependency up
>   git-std-lib: introduce git standard library
>   git-std-lib: add test file to call git-std-lib.a functions

This doesn't seem to apply to 'master'. Do you have a base commit that
reviewers could apply the patches to?
Phillip Wood Aug. 15, 2023, 9:20 a.m. UTC | #2
On 10/08/2023 23:05, Glen Choo wrote:
> Calvin Wan <calvinwan@google.com> writes:
> 
>> Calvin Wan (7):
>>    hex-ll: split out functionality from hex
>>    object: move function to object.c
>>    config: correct bad boolean env value error message
>>    parse: create new library for parsing strings and env values
>>    date: push pager.h dependency up
>>    git-std-lib: introduce git standard library
>>    git-std-lib: add test file to call git-std-lib.a functions
> 
> This doesn't seem to apply to 'master'. Do you have a base commit that
> reviewers could apply the patches to?

I don't know what they are based on, but I did manage to apply them to 
master by using "am -3" and resolving the conflicts. The result is at 
https://github.com/phillipwood/git/tree/cw/git-std-lib/rfc-v2 if anyone 
is interested.

Best Wishes

Phillip
Phillip Wood Aug. 15, 2023, 9:41 a.m. UTC | #3
Hi Calvin

On 10/08/2023 17:33, Calvin Wan wrote:
> Original cover letter:
> https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
> 
> In the initial RFC, I had a patch that removed the trace2 dependency
> from usage.c so that git-std-lib.a would not have dependencies outside
> of git-std-lib.a files. Consequently this meant that tracing would not
> be possible in git-std-lib.a files for other developers of Git, and it
> is not a good idea for the libification effort to close the door on
> tracing in certain files for future development (thanks Victoria for
> pointing this out). That patch has been removed and instead I introduce
> stubbed out versions of repository.[ch] and trace2.[ch] that are swapped
> in during compilation time (I'm no Makefile expert so any advice on how
> on I could do this better would be much appreciated). These stubbed out
> files contain no implementations and therefore do not have any
> additional dependencies, allowing git-std-lib.a to compile with only the
> stubs as additional dependencies.

I think stubbing out trace2 is a sensible approach. I don't think we
need separate headers when using the stub though, or a stub for
repository.c as we don't call any of the functions declared in that
header. I've appended a patch that shows a simplified stub. It also
removes the recursive make call as it no-longer needs to juggle the
header files.

> This also has the added benefit of
> removing `#ifdef GIT_STD_LIB` macros in C files for specific library
> compilation rules. Libification shouldn't pollute C files with these
> macros. The boundaries for git-std-lib.a have also been updated to
> contain these stubbed out files.

Do you have any plans to support building with gettext support so we
can use git-std-lib.a as a dependency of libgit.a?
  
> I have also made some additional changes to the Makefile to piggy back
> off of our existing build rules for .c/.o targets and their
> dependencies. As I learn more about Makefiles, I am continuing to look
> for ways to improve these rules. Eventually I would like to be able to
> have a set of rules that future libraries can emulate and is scalable
> in the sense of not creating additional toil for developers that are not
> interested in libification.

I'm not sure reusing LIB_OBJS for different targets is a good idea.
Once libgit.a starts to depend on git-std-lib.a we'll want to build them
both with a single make invocation without resorting to recursive make
calls. I think we could perhaps make a template function to create the
compilation rules for each library - see the end of
https://wingolog.org/archives/2023/08/08/a-negative-result

Best Wishes

Phillip

---- >8 -----
 From 194403e42f116cc3c6ed8eb8b03d6933b24067e4 Mon Sep 17 00:00:00 2001
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Date: Sat, 12 Aug 2023 17:27:23 +0100
Subject: [PATCH] git-std-lib: simplify sub implementation

The code in std-lib does not depend directly on the functions declared
in repository.h and so it does not need to provide stub
implementations of the functions declared in repository.h. There is a
transitive dependency on `struct repository` from the functions
declared in trace2.h but the stub implementation of those functions
can simply define its own stub for struct repository. There is also no
need to use different headers when compiling against the stub
implementation of trace2.

This means we can simplify the stub implementation by removing
stubs/{repository.[ch],trace2.h} and simplify the Makefile by removing
the code that replaces header files when compiling against the trace2
stub. git-std-lib.a can now be built by running

   make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease

There is one other small fixup in this commit:

  - `wrapper.c` includes `repository.h` but does not use any of the
    declarations.

Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
  Makefile           | 29 +-------------------
  stubs/repository.c |  4 ---
  stubs/repository.h |  8 ------
  stubs/trace2.c     |  5 ++++
  stubs/trace2.h     | 68 ----------------------------------------------
  wrapper.c          |  1 -
  6 files changed, 6 insertions(+), 109 deletions(-)
  delete mode 100644 stubs/repository.c
  delete mode 100644 stubs/repository.h
  delete mode 100644 stubs/trace2.h

diff --git a/Makefile b/Makefile
index a821d73c9d0..8eff4021025 100644
--- a/Makefile
+++ b/Makefile
@@ -1209,10 +1209,6 @@ LIB_OBJS += usage.o
  LIB_OBJS += utf8.o
  LIB_OBJS += wrapper.o
  
-ifdef STUB_REPOSITORY
-STUB_OBJS += stubs/repository.o
-endif
-
  ifdef STUB_TRACE2
  STUB_OBJS += stubs/trace2.o
  endif
@@ -3866,31 +3862,8 @@ fuzz-all: $(FUZZ_PROGRAMS)
  ### Libified Git rules
  
  # git-std-lib
-# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
+# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease`
  STD_LIB = git-std-lib.a
  
  $(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
  	$(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
-
-TEMP_HEADERS = temp_headers/
-
-git-std-lib:
-# Move headers to temporary folder and replace them with stubbed headers.
-# After building, move headers and stubbed headers back.
-ifneq ($(STUB_OBJS),)
-	mkdir -p $(TEMP_HEADERS); \
-	for d in $(STUB_OBJS); do \
-		BASE=$${d%.*}; \
-		mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
-		mv $${BASE}.h $${BASE##*/}.h; \
-	done; \
-	$(MAKE) $(STD_LIB); \
-	for d in $(STUB_OBJS); do \
-		BASE=$${d%.*}; \
-		mv $${BASE##*/}.h $${BASE}.h; \
-		mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
-	done; \
-	rm -rf temp_headers
-else
-	$(MAKE) $(STD_LIB)
-endif
diff --git a/stubs/repository.c b/stubs/repository.c
deleted file mode 100644
index f81520d083a..00000000000
--- a/stubs/repository.c
+++ /dev/null
@@ -1,4 +0,0 @@
-#include "git-compat-util.h"
-#include "repository.h"
-
-struct repository *the_repository;
diff --git a/stubs/repository.h b/stubs/repository.h
deleted file mode 100644
index 18262d748e5..00000000000
--- a/stubs/repository.h
+++ /dev/null
@@ -1,8 +0,0 @@
-#ifndef REPOSITORY_H
-#define REPOSITORY_H
-
-struct repository { int stub; };
-
-extern struct repository *the_repository;
-
-#endif /* REPOSITORY_H */
diff --git a/stubs/trace2.c b/stubs/trace2.c
index efc3f9c1f39..7d894822288 100644
--- a/stubs/trace2.c
+++ b/stubs/trace2.c
@@ -1,6 +1,10 @@
  #include "git-compat-util.h"
  #include "trace2.h"
  
+struct child_process { int stub; };
+struct repository { int stub; };
+struct json_writer { int stub; };
+
  void trace2_region_enter_fl(const char *file, int line, const char *category,
  			    const char *label, const struct repository *repo, ...) { }
  void trace2_region_leave_fl(const char *file, int line, const char *category,
@@ -19,4 +23,5 @@ void trace2_data_intmax_fl(const char *file, int line, const char *category,
  			   const struct repository *repo, const char *key,
  			   intmax_t value) { }
  int trace2_is_enabled(void) { return 0; }
+void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
  void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
diff --git a/stubs/trace2.h b/stubs/trace2.h
deleted file mode 100644
index 836a14797cc..00000000000
--- a/stubs/trace2.h
+++ /dev/null
@@ -1,68 +0,0 @@
-#ifndef TRACE2_H
-#define TRACE2_H
-
-struct child_process { int stub; };
-struct repository;
-struct json_writer { int stub; };
-
-void trace2_region_enter_fl(const char *file, int line, const char *category,
-			    const char *label, const struct repository *repo, ...);
-
-#define trace2_region_enter(category, label, repo) \
-	trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
-
-void trace2_region_leave_fl(const char *file, int line, const char *category,
-			    const char *label, const struct repository *repo, ...);
-
-#define trace2_region_leave(category, label, repo) \
-	trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
-
-void trace2_data_string_fl(const char *file, int line, const char *category,
-			   const struct repository *repo, const char *key,
-			   const char *value);
-
-#define trace2_data_string(category, repo, key, value)                       \
-	trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
-			      (value))
-
-void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
-
-#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
-
-void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
-			    va_list ap);
-
-#define trace2_cmd_error_va(fmt, ap) \
-	trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
-
-
-void trace2_cmd_name_fl(const char *file, int line, const char *name);
-
-#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
-
-void trace2_thread_start_fl(const char *file, int line,
-			    const char *thread_base_name);
-
-#define trace2_thread_start(thread_base_name) \
-	trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
-
-void trace2_thread_exit_fl(const char *file, int line);
-
-#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
-
-void trace2_data_intmax_fl(const char *file, int line, const char *category,
-			   const struct repository *repo, const char *key,
-			   intmax_t value);
-
-#define trace2_data_intmax(category, repo, key, value)                       \
-	trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
-			      (value))
-
-enum trace2_process_info_reason {
-	TRACE2_PROCESS_INFO_STARTUP,
-	TRACE2_PROCESS_INFO_EXIT,
-};
-int trace2_is_enabled(void);
-void trace2_collect_process_info(enum trace2_process_info_reason reason);
-
-#endif /* TRACE2_H */
diff --git a/wrapper.c b/wrapper.c
index 9eae4a8b3a0..e6facc5ff0c 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -5,7 +5,6 @@
  #include "abspath.h"
  #include "parse.h"
  #include "gettext.h"
-#include "repository.h"
  #include "strbuf.h"
  #include "trace2.h"
Calvin Wan Aug. 16, 2023, 5:17 p.m. UTC | #4
Thanks for resolving the conflicts on master. I should've rebased
before sending out this v2 since it's built off of 2.41 with some of
my other patch cleanup series.
Junio C Hamano Aug. 16, 2023, 9:19 p.m. UTC | #5
Calvin Wan <calvinwan@google.com> writes:

> Thanks for resolving the conflicts on master. I should've rebased
> before sending out this v2 since it's built off of 2.41 with some of
> my other patch cleanup series.

I think the freeze period before the release would be a good time to
rebuild on an updated base to prepare v3 for posting.

Thanks.
Calvin Wan Sept. 8, 2023, 5:41 p.m. UTC | #6
Original cover letter:
https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/

I have taken this series out of RFC since there weren't any significant
concerns with the overall concept and design of this series. This reroll
incorporates some smaller changes such as dropping the "push pager
dependency" patch in favor of stubbing it out. The main change this
reroll cleans up the Makefile rules and stubs, as suggested by
Phillip Wood (appreciate the help on this one)!

This series has been rebased onto 1fc548b2d6a: The sixth batch

Originally this series was built on other patches that have since been
merged, which is why the range-diff is shown removing many of them.

Calvin Wan (6):
  hex-ll: split out functionality from hex
  wrapper: remove dependency to Git-specific internal file
  config: correct bad boolean env value error message
  parse: create new library for parsing strings and env values
  git-std-lib: introduce git standard library
  git-std-lib: add test file to call git-std-lib.a functions

 Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++
 Makefile                                |  41 ++++-
 attr.c                                  |   2 +-
 color.c                                 |   2 +-
 config.c                                | 173 +-----------------
 config.h                                |  14 +-
 entry.c                                 |   5 +
 entry.h                                 |   6 +
 git-compat-util.h                       |   7 +-
 hex-ll.c                                |  49 +++++
 hex-ll.h                                |  27 +++
 hex.c                                   |  47 -----
 hex.h                                   |  24 +--
 mailinfo.c                              |   2 +-
 pack-objects.c                          |   2 +-
 pack-revindex.c                         |   2 +-
 parse-options.c                         |   3 +-
 parse.c                                 | 182 +++++++++++++++++++
 parse.h                                 |  20 ++
 pathspec.c                              |   2 +-
 preload-index.c                         |   2 +-
 progress.c                              |   2 +-
 prompt.c                                |   2 +-
 rebase.c                                |   2 +-
 strbuf.c                                |   2 +-
 stubs/pager.c                           |   6 +
 stubs/pager.h                           |   6 +
 stubs/trace2.c                          |  27 +++
 symlinks.c                              |   2 +
 t/Makefile                              |   4 +
 t/helper/test-env-helper.c              |   2 +-
 t/stdlib-test.c                         | 231 ++++++++++++++++++++++++
 unpack-trees.c                          |   2 +-
 url.c                                   |   2 +-
 urlmatch.c                              |   2 +-
 wrapper.c                               |   9 +-
 wrapper.h                               |   5 -
 write-or-die.c                          |   2 +-
 38 files changed, 824 insertions(+), 287 deletions(-)
 create mode 100644 Documentation/technical/git-std-lib.txt
 create mode 100644 hex-ll.c
 create mode 100644 hex-ll.h
 create mode 100644 parse.c
 create mode 100644 parse.h
 create mode 100644 stubs/pager.c
 create mode 100644 stubs/pager.h
 create mode 100644 stubs/trace2.c
 create mode 100644 t/stdlib-test.c

Range-diff against v2:
 1:  121788f263 <  -:  ---------- strbuf: clarify API boundary
 2:  5e91404ecd <  -:  ---------- strbuf: clarify dependency
 3:  5c05f40181 <  -:  ---------- abspath: move related functions to abspath
 4:  e1addc77e5 <  -:  ---------- credential-store: move related functions to credential-store file
 5:  62e8c42f59 <  -:  ---------- object-name: move related functions to object-name
 6:  0abba57acb <  -:  ---------- path: move related function to path
 7:  d33267a390 <  -:  ---------- strbuf: remove global variable
 8:  665d2c2089 <  -:  ---------- init-db: document existing bug with core.bare in template config
 9:  68d0a8ff16 <  -:  ---------- init-db: remove unnecessary global variable
10:  8c8ec85507 <  -:  ---------- init-db, clone: change unnecessary global into passed parameter
11:  d555e2b365 <  -:  ---------- setup: adopt shared init-db & clone code
12:  689a7bc8aa <  -:  ---------- read-cache: move shared commit and ls-files code
13:  392f8e75b7 <  -:  ---------- add: modify add_files_to_cache() to avoid globals
14:  49ce237013 <  -:  ---------- read-cache: move shared add/checkout/commit code
15:  c5d8370d40 <  -:  ---------- statinfo: move stat_{data,validity} functions from cache/read-cache
16:  90a72b6f86 <  -:  ---------- run-command.h: move declarations for run-command.c from cache.h
17:  f27516c780 <  -:  ---------- name-hash.h: move declarations for name-hash.c from cache.h
18:  895c38a050 <  -:  ---------- sparse-index.h: move declarations for sparse-index.c from cache.h
19:  8678d4ad20 <  -:  ---------- preload-index.h: move declarations for preload-index.c from elsewhere
20:  4a463abaae <  -:  ---------- diff.h: move declaration for global in diff.c from cache.h
21:  3440e762c7 <  -:  ---------- merge.h: move declarations for merge.c from cache.h
22:  e70853e398 <  -:  ---------- repository.h: move declaration of the_index from cache.h
23:  ccd2014d73 <  -:  ---------- read-cache*.h: move declarations for read-cache.c functions from cache.h
24:  d3a482afa9 <  -:  ---------- cache.h: remove this no-longer-used header
25:  eaa087f446 <  -:  ---------- log-tree: replace include of revision.h with simple forward declaration
26:  5d2b0a9c75 <  -:  ---------- repository: remove unnecessary include of path.h
27:  250f83014e <  -:  ---------- diff.h: remove unnecessary include of oidset.h
28:  d0f9913958 <  -:  ---------- list-objects-filter-options.h: remove unneccessary include
29:  03a2b2a515 <  -:  ---------- builtin.h: remove unneccessary includes
30:  15edc22d00 <  -:  ---------- git-compat-util.h: remove unneccessary include of wildmatch.h
31:  e4e1bec8bd <  -:  ---------- merge-ll: rename from ll-merge
32:  9185495fd0 <  -:  ---------- khash: name the structs that khash declares
33:  15fb05e453 <  -:  ---------- object-store-ll.h: split this header out of object-store.h
34:  2608fe4b23 <  -:  ---------- hash-ll, hashmap: move oidhash() to hash-ll
35:  5e8dc5b574 <  -:  ---------- fsmonitor-ll.h: split this header out of fsmonitor.h
36:  37d32fc3fd <  -:  ---------- git-compat-util: move strbuf.c funcs to its header
37:  6ed19d5fe2 <  -:  ---------- git-compat-util: move wrapper.c funcs to its header
38:  555d1b8942 <  -:  ---------- sane-ctype.h: create header for sane-ctype macros
39:  72d591e282 <  -:  ---------- kwset: move translation table from ctype
40:  5d1dc2a118 <  -:  ---------- common.h: move non-compat specific macros and functions
41:  33e07e552e <  -:  ---------- git-compat-util: move usage.c funcs to its header
42:  417a8aa733 <  -:  ---------- treewide: remove unnecessary includes for wrapper.h
43:  65e35d00c1 <  -:  ---------- common: move alloc macros to common.h
44:  78634bc406 !  1:  2f99eb2ca4 hex-ll: split out functionality from hex
    @@ hex.h
     +#include "hex-ll.h"
      
      /*
    -  * Try to read a SHA1 in hexadecimal format from the 40 characters
    -@@ hex.h: int get_oid_hex(const char *hex, struct object_id *sha1);
    +  * Try to read a hash (specified by the_hash_algo) in hexadecimal
    +@@ hex.h: int get_oid_hex(const char *hex, struct object_id *oid);
      /* Like get_oid_hex, but for an arbitrary hash algorithm. */
      int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
      
45:  21ec1d276e !  2:  7b2d123628 object: move function to object.c
    @@ Metadata
     Author: Calvin Wan <calvinwan@google.com>
     
      ## Commit message ##
    -    object: move function to object.c
    +    wrapper: remove dependency to Git-specific internal file
     
    -    While remove_or_warn() is a simple ternary operator to call two other
    -    wrapper functions, it creates an unnecessary dependency to object.h in
    -    wrapper.c. Therefore move the function to object.[ch] where the concept
    -    of GITLINKs is first defined.
    +    In order for wrapper.c to be built independently as part of a smaller
    +    library, it cannot have dependencies to other Git specific
    +    internals. remove_or_warn() creates an unnecessary dependency to
    +    object.h in wrapper.c. Therefore move the function to entry.[ch] which
    +    performs changes on the worktree based on the Git-specific file modes in
    +    the index.
     
    - ## object.c ##
    -@@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
    - 	FREE_AND_NULL(o->object_state);
    - 	FREE_AND_NULL(o->shallow_stat);
    + ## entry.c ##
    +@@ entry.c: void unlink_entry(const struct cache_entry *ce, const char *super_prefix)
    + 		return;
    + 	schedule_dir_for_removal(ce->name, ce_namelen(ce));
      }
     +
     +int remove_or_warn(unsigned int mode, const char *file)
    @@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
     +	return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
     +}
     
    - ## object.h ##
    -@@ object.h: void clear_object_flags(unsigned flags);
    -  */
    - void repo_clear_commit_marks(struct repository *r, unsigned int flags);
    + ## entry.h ##
    +@@ entry.h: int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st)
    + void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
    + 			   struct stat *st);
      
     +/*
     + * Calls the correct function out of {unlink,rmdir}_or_warn based on
    @@ object.h: void clear_object_flags(unsigned flags);
     + */
     +int remove_or_warn(unsigned int mode, const char *path);
     +
    - #endif /* OBJECT_H */
    + #endif /* ENTRY_H */
     
      ## wrapper.c ##
     @@
46:  41dcf8107c =  3:  b37beb206a config: correct bad boolean env value error message
47:  3e800a41c4 !  4:  3a827cf45c parse: create new library for parsing strings and env values
    @@ Commit message
         config.c, there are other files that only need parsing functionality and
         not config functionality. By separating out string and environment value
         parsing from config, those files can instead be dependent on parse,
    -    which has a much smaller dependency chain than config.
    +    which has a much smaller dependency chain than config. This ultimately
    +    allows us to inclue parse.[ch] in an independent library since it
    +    doesn't have dependencies to Git-specific internals unlike in
    +    config.[ch].
     
         Move general string and env parsing functions from config.[ch] to
         parse.[ch].
    @@ config.c: static int git_parse_source(struct config_source *cs, config_fn_t fn,
     -	return 1;
     -}
     -
    - static int reader_config_name(struct config_reader *reader, const char **out);
    - static int reader_origin_type(struct config_reader *reader,
    - 			      enum config_origin_type *type);
    -@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value)
    + NORETURN
    + static void die_bad_number(const char *name, const char *value,
    + 			   const struct key_value_info *kvi)
    +@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value,
      	return ret;
      }
      
    @@ config.c: static enum fsync_component parse_fsync_components(const char *var, co
     -	return -1;
     -}
     -
    - int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
    + int git_config_bool_or_int(const char *name, const char *value,
    + 			   const struct key_value_info *kvi, int *is_bool)
      {
    - 	int v = git_parse_maybe_bool_text(value);
     @@ config.c: void git_global_config(char **user_out, char **xdg_out)
      	*xdg_out = xdg_config;
      }
    @@ config.c: void git_global_config(char **user_out, char **xdg_out)
     
      ## config.h ##
     @@
    - 
      #include "hashmap.h"
      #include "string-list.h"
    + #include "repository.h"
     -
     +#include "parse.h"
      
48:  7a4a088bc3 <  -:  ---------- date: push pager.h dependency up
49:  c9002734d0 !  5:  f8e4ac50a0 git-std-lib: introduce git standard library
    @@ Documentation/technical/git-std-lib.txt (new)
     +Rationale behind Git Standard Library
     +================
     +
    -+The rationale behind Git Standard Library essentially is the result of
    -+two observations within the Git codebase: every file includes
    -+git-compat-util.h which defines functions in a couple of different
    -+files, and wrapper.c + usage.c have difficult-to-separate circular
    -+dependencies with each other and other files.
    ++The rationale behind what's in and what's not in the Git Standard
    ++Library essentially is the result of two observations within the Git
    ++codebase: every file includes git-compat-util.h which defines functions
    ++in a couple of different files, and wrapper.c + usage.c have
    ++difficult-to-separate circular dependencies with each other and other
    ++files.
     +
     +Ubiquity of git-compat-util.h and circular dependencies
     +========
    @@ Documentation/technical/git-std-lib.txt (new)
     + - low-level git/* files with functions defined in git-compat-util.h
     +   (ctype.c)
     + - compat/*
    -+ - stubbed out dependencies in stubs/ (stubs/repository.c, stubs/trace2.c)
    ++ - stubbed out dependencies in stubs/ (stubs/pager.c, stubs/trace2.c)
     +
     +There are other files that might fit this definition, but that does not
     +mean it should belong in git-std-lib.a. Those files should start as
     +their own separate library since any file added to git-std-lib.a loses
     +its flexibility of being easily swappable.
     +
    -+Wrapper.c and usage.c have dependencies on repository and trace2 that are
    ++Wrapper.c and usage.c have dependencies on pager and trace2 that are
     +possible to remove at the cost of sacrificing the ability for standard Git
     +to be able to trace functions in those files and other files in git-std-lib.a.
     +In order for git-std-lib.a to compile with those dependencies, stubbed out
    @@ Documentation/technical/git-std-lib.txt (new)
     +usage.c
     +utf8.c
     +wrapper.c
    -+stubs/repository.c
    -+stubs/trace2.c
     +relevant compat/ files
     +
    ++When these files are compiled together with the following files (or
    ++user-provided files that provide the same functions), they form a
    ++complete library:
    ++stubs/pager.c
    ++stubs/trace2.c
    ++
     +Pitfalls
     +================
     +
    @@ Makefile: LIB_OBJS += write-or-die.o
     +LIB_OBJS += utf8.o
     +LIB_OBJS += wrapper.o
     +
    -+ifdef STUB_REPOSITORY
    -+STUB_OBJS += stubs/repository.o
    -+endif
    -+
     +ifdef STUB_TRACE2
     +STUB_OBJS += stubs/trace2.o
     +endif
     +
    ++ifdef STUB_PAGER
    ++STUB_OBJS += stubs/pager.o
    ++endif
    ++
     +LIB_OBJS += $(STUB_OBJS)
     +endif
      
    @@ Makefile: ifdef FSMONITOR_OS_SETTINGS
      NO_TCLTK = NoThanks
      endif
     @@ Makefile: clean: profile-clean coverage-clean cocciclean
    - 	$(RM) po/git.pot po/git-core.pot
      	$(RM) git.res
      	$(RM) $(OBJECTS)
    + 	$(RM) headless-git.o
     -	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
     +	$(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB) $(STD_LIB_FILE)
      	$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
    @@ Makefile: $(FUZZ_PROGRAMS): all
     +### Libified Git rules
     +
     +# git-std-lib
    -+# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
    ++# `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) $@ $^
    -+
    -+TEMP_HEADERS = temp_headers/
    -+
    -+git-std-lib:
    -+# Move headers to temporary folder and replace them with stubbed headers.
    -+# After building, move headers and stubbed headers back.
    -+ifneq ($(STUB_OBJS),)
    -+	mkdir -p $(TEMP_HEADERS); \
    -+	for d in $(STUB_OBJS); do \
    -+		BASE=$${d%.*}; \
    -+		mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
    -+		mv $${BASE}.h $${BASE##*/}.h; \
    -+	done; \
    -+	$(MAKE) $(STD_LIB); \
    -+	for d in $(STUB_OBJS); do \
    -+		BASE=$${d%.*}; \
    -+		mv $${BASE##*/}.h $${BASE}.h; \
    -+		mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
    -+	done; \
    -+	rm -rf temp_headers
    -+else
    -+	$(MAKE) $(STD_LIB)
    -+endif
     
      ## git-compat-util.h ##
     @@ git-compat-util.h: static inline int noop_core_config(const char *var UNUSED,
    @@ git-compat-util.h: const char *inet_ntop(int af, const void *src, char *dst, siz
      #endif
     +#endif
      
    - /*
    -  * Limit size of IO chunks, because huge chunks only cause pain.  OS X
    -@@ git-compat-util.h: int git_access(const char *path, int mode);
    - # endif
    - #endif
    + static inline size_t st_add(size_t a, size_t b)
    + {
    +@@ git-compat-util.h: static inline int is_missing_file_error(int errno_)
    + 	return (errno_ == ENOENT || errno_ == ENOTDIR);
    + }
      
     +#ifndef GIT_STD_LIB
      int cmd_main(int, const char **);
    @@ git-compat-util.h: int git_access(const char *path, int mode);
      /*
       * You can mark a stack variable with UNLEAK(var) to avoid it being
     
    - ## stubs/repository.c (new) ##
    + ## stubs/pager.c (new) ##
     @@
    -+#include "git-compat-util.h"
    -+#include "repository.h"
    ++#include "pager.h"
     +
    -+struct repository *the_repository;
    ++int pager_in_use(void)
    ++{
    ++	return 0;
    ++}
     
    - ## stubs/repository.h (new) ##
    + ## stubs/pager.h (new) ##
     @@
    -+#ifndef REPOSITORY_H
    -+#define REPOSITORY_H
    ++#ifndef PAGER_H
    ++#define PAGER_H
     +
    -+struct repository { int stub; };
    ++int pager_in_use(void);
     +
    -+extern struct repository *the_repository;
    -+
    -+#endif /* REPOSITORY_H */
    ++#endif /* PAGER_H */
     
      ## stubs/trace2.c (new) ##
     @@
     +#include "git-compat-util.h"
     +#include "trace2.h"
     +
    ++struct child_process { int stub; };
    ++struct repository { int stub; };
    ++struct json_writer { int stub; };
    ++
     +void trace2_region_enter_fl(const char *file, int line, const char *category,
     +			    const char *label, const struct repository *repo, ...) { }
     +void trace2_region_leave_fl(const char *file, int line, const char *category,
    @@ stubs/trace2.c (new)
     +			   const struct repository *repo, const char *key,
     +			   intmax_t value) { }
     +int trace2_is_enabled(void) { return 0; }
    ++void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
     +void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
     
    - ## stubs/trace2.h (new) ##
    -@@
    -+#ifndef TRACE2_H
    -+#define TRACE2_H
    -+
    -+struct child_process { int stub; };
    -+struct repository;
    -+struct json_writer { int stub; };
    -+
    -+void trace2_region_enter_fl(const char *file, int line, const char *category,
    -+			    const char *label, const struct repository *repo, ...);
    -+
    -+#define trace2_region_enter(category, label, repo) \
    -+	trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
    -+
    -+void trace2_region_leave_fl(const char *file, int line, const char *category,
    -+			    const char *label, const struct repository *repo, ...);
    -+
    -+#define trace2_region_leave(category, label, repo) \
    -+	trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
    -+
    -+void trace2_data_string_fl(const char *file, int line, const char *category,
    -+			   const struct repository *repo, const char *key,
    -+			   const char *value);
    -+
    -+#define trace2_data_string(category, repo, key, value)                       \
    -+	trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
    -+			      (value))
    -+
    -+void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
    -+
    -+#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
    -+
    -+void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
    -+			    va_list ap);
    -+
    -+#define trace2_cmd_error_va(fmt, ap) \
    -+	trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
    -+
    -+
    -+void trace2_cmd_name_fl(const char *file, int line, const char *name);
    -+
    -+#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
    -+
    -+void trace2_thread_start_fl(const char *file, int line,
    -+			    const char *thread_base_name);
    -+
    -+#define trace2_thread_start(thread_base_name) \
    -+	trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
    -+
    -+void trace2_thread_exit_fl(const char *file, int line);
    -+
    -+#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
    -+
    -+void trace2_data_intmax_fl(const char *file, int line, const char *category,
    -+			   const struct repository *repo, const char *key,
    -+			   intmax_t value);
    -+
    -+#define trace2_data_intmax(category, repo, key, value)                       \
    -+	trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
    -+			      (value))
    -+
    -+enum trace2_process_info_reason {
    -+	TRACE2_PROCESS_INFO_STARTUP,
    -+	TRACE2_PROCESS_INFO_EXIT,
    -+};
    -+int trace2_is_enabled(void);
    -+void trace2_collect_process_info(enum trace2_process_info_reason reason);
    -+
    -+#endif /* TRACE2_H */
    -+
    -
      ## symlinks.c ##
     @@ symlinks.c: void invalidate_lstat_cache(void)
      	reset_lstat_cache(&default_cache);
    @@ symlinks.c: int lstat_cache_aware_rmdir(const char *path)
      	return ret;
      }
     +#endif
    +
    + ## wrapper.c ##
    +@@
    + #include "abspath.h"
    + #include "parse.h"
    + #include "gettext.h"
    +-#include "repository.h"
    + #include "strbuf.h"
    + #include "trace2.h"
    + 
50:  0bead8f980 !  6:  7840e1830a git-std-lib: add test file to call git-std-lib.a functions
    @@ t/stdlib-test.c (new)
     +	struct strbuf sb3 = STRBUF_INIT;
     +	struct string_list list = STRING_LIST_INIT_NODUP;
     +	char *buf = "foo";
    -+	struct strbuf_expand_dict_entry dict[] = {
    -+		{ "foo", NULL, },
    -+		{ "bar", NULL, },
    -+	};
     +	int fd = open("/dev/null", O_RDONLY);
     +
     +	fprintf(stderr, "calling strbuf functions\n");
    @@ t/stdlib-test.c (new)
     +	strbuf_add_commented_lines(sb, "foo", 3, '#');
     +	strbuf_commented_addf(sb, '#', "%s", "foo");
     +	// strbuf_vaddf() called by strbuf_addf()
    -+	strbuf_expand(sb, "%s", strbuf_expand_literal_cb, NULL);
    -+	strbuf_expand(sb, "%s", strbuf_expand_dict_cb, &dict);
    -+	// strbuf_expand_literal_cb() called by strbuf_expand()
    -+	// strbuf_expand_dict_cb() called by strbuf_expand()
     +	strbuf_addbuf_percentquote(sb, &sb3);
     +	strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
     +	strbuf_fread(sb, 0, stdin);
Junio C Hamano Sept. 8, 2023, 8:36 p.m. UTC | #7
Calvin Wan <calvinwan@google.com> writes:

> I have taken this series out of RFC since there weren't any significant
> concerns with the overall concept and design of this series. This reroll
> incorporates some smaller changes such as dropping the "push pager
> dependency" patch in favor of stubbing it out. The main change this
> reroll cleans up the Makefile rules and stubs, as suggested by
> Phillip Wood (appreciate the help on this one)!

What is your plan for the "config-parse" stuff?  The "create new library"
step in this series seem to aim for the same goal in a different ways.

> This series has been rebased onto 1fc548b2d6a: The sixth batch
>
> Originally this series was built on other patches that have since been
> merged, which is why the range-diff is shown removing many of them.

Good.  Previous rounds did not really attract much interest from the
public if I recall correctly.  Let's see how well this round fares.

>  Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++

It is interesting to see that there is no "std.*lib\.c" in the set
of source files, or "std.*lib\.a" target in the Makefile.
Junio C Hamano Sept. 8, 2023, 9:30 p.m. UTC | #8
Junio C Hamano <gitster@pobox.com> writes:

> Calvin Wan <calvinwan@google.com> writes:
>
>> I have taken this series out of RFC since there weren't any significant
>> concerns with the overall concept and design of this series. This reroll
>> incorporates some smaller changes such as dropping the "push pager
>> dependency" patch in favor of stubbing it out. The main change this
>> reroll cleans up the Makefile rules and stubs, as suggested by
>> Phillip Wood (appreciate the help on this one)!
>
> What is your plan for the "config-parse" stuff?  The "create new library"
> step in this series seem to aim for the same goal in a different ways.

Actually, this one is far less ambitious in touching "config"
subsystem, in that it only deals with parsing strings as values.
The other one knows how a config file is laid out, what key the
value we are about to read is expected for, etc., and it will
benefit by having the "parse" code separated out by this series, but
they are more or less orthogonal.

Queued.  Thanks.