Re: [PATCH v5 3/3] test-stdlib: show that git-std-lib is independent

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2024, 22:24 UTC
Message-ID: <xmqqr0h4b0ic.fsf@gitster.g>
In-Reply-To: <20240222175033.1489723-4-calvinwan@google.com>

Calvin Wan <calvinwan@google.com> writes:

Show 12 quoted lines

> diff --git a/Makefile b/Makefile
> index d37ea9d34b..1d762ce13a 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -870,9 +870,7 @@ TEST_BUILTINS_OBJS += test-xml-encode.o
>  # Do not add more tests here unless they have extra dependencies. Add
>  # them in TEST_BUILTINS_OBJS above.
>  TEST_PROGRAMS_NEED_X += test-fake-ssh
> -TEST_PROGRAMS_NEED_X += test-tool
> -
> -TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
> +TEST_PROGRAMS_NEED_X += $(info tpnxnpg=$(NO_POSIX_GOODIES))test-tool

Is this version meant to be ready for reviewing? $(info) used like this does not look like a good fit for production code.

Show 13 quoted lines

> diff --git a/strbuf.h b/strbuf.h
> index e959caca87..f775416307 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -1,6 +1,8 @@
>  #ifndef STRBUF_H
>  #define STRBUF_H
>  
> +#include "git-compat-util.h"
> +
>  /*
>   * NOTE FOR STRBUF DEVELOPERS
>   *

The same comment about header inclusion I made on [1/3] applies here, too. I am open to hearing better ideas to handle system headers, but my preference is to allow any and all headers assume <git-compat-util.h> (or its moral equivalent that may be stripped down by moving non-essential things out) is already included, which in turn means those *.c files (like t/helper/test-stdlib.c we see below) would include <git-compat-util.h> (or its trimmed down version) as the first header, before including <strbuf.h>.

In any case, this change, if we were to make it (and I do not think we should), should be treated like the change to pager.h in [1/3], i.e. part of making the existing headers ready to be shared with the "stdlib" effort. It does not belong to this [3/3] step, where we are supposed to be demonstrating the use of "stdlib", which has become (minimally) usable with the steps before this one.

Show 11 quoted lines

> diff --git a/stubs/misc.c b/stubs/misc.c
> index 92da76fd46..8d80581e39 100644
> --- a/stubs/misc.c
> +++ b/stubs/misc.c
> @@ -9,6 +9,7 @@
>   * yet. To do that we need to start using dgettext() rather than
>   * gettext() in our code.
>   */
> +#include "gettext.h"
>  int git_gettext_enabled = 0;
>  #endif

This change should have happened before this [3/3] step, whose point is to demonstrate "stdlib" that has already been made (minimally) usable with steps before this one.

Show 31 quoted lines

> diff --git a/t/helper/.gitignore b/t/helper/.gitignore
> index 8c2ddcce95..5cec3b357f 100644
> --- a/t/helper/.gitignore
> +++ b/t/helper/.gitignore
> @@ -1,2 +1,3 @@
>  /test-tool
>  /test-fake-ssh
> +/test-stdlib
> diff --git a/t/helper/test-stdlib.c b/t/helper/test-stdlib.c
> new file mode 100644
> index 0000000000..460b472fb4
> --- /dev/null
> +++ b/t/helper/test-stdlib.c
> @@ -0,0 +1,266 @@
> +#include "git-compat-util.h"
> +#include "abspath.h"
> +#include "hex-ll.h"
> +#include "parse.h"
> +#include "strbuf.h"
> +#include "string-list.h"
> +
> +/*
> + * Calls all functions from git-std-lib
> + * Some inline/trivial functions are skipped
> + *
> + * NEEDSWORK: The purpose of this file is to show that an executable can be
> + * built with git-std-lib.a and git-stub-lib.a, and then executed. If there
> + * is another executable that demonstrates this (for example, a unit test that
> + * takes the form of an executable compiled with git-std-lib.a and git-stub-
> + * lib.a), this file can be removed.
> + */

Or alternatively, these "random list of function calls" can be turned into a more realistic test helpers in place. "stdlib" will hopefully gain more coverage of the features of low level helpers "git" binary proper uses, and I do not think it is far-fetched to migrate the "test-tool date" subcommands all to not link directly with "libgit.a" but with gitstdlib instead and the things should work, right? Right now, the "random list of function calls" do not do anything useful, but that does not have to be the case. It should offer us more value to us than "It links!" ;-).

Having said that, the most valuable part in this [3/3] step is how this t/helper/test-stdlib is linked, i.e. this part from the Makefile:

> +t/helper/test-stdlib$X: t/helper/test-stdlib.o GIT-LDFLAGS $(STD_LIB_FILE) $(STUB_LIB_FILE) $(GITLIBS)
> +	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
> +		$< $(STD_LIB_FILE) $(STUB_LIB_FILE) $(EXTLIBS)

where we have no $(LIB_FILE) (aka libgit.a). Especially if we can grow the capability in $(STD_LIB_FILE) without adding too much stuff to $(STUB_LIB_FILE), this is a major achievement. Very nice.