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

[Junio C Hamano <gitster@pobox.com>]

Calvin Wan <calvinwan@google.com> writes:

> 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.

> 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.

> 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.

> 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.