Re: bug in en/header-split-cache-h-part-3, was Re: What's cooking in git.git (Jun 2023, #05; Tue, 20)

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

Jeff King <peff@peff.net> writes:

> On Tue, Jun 20, 2023 at 05:04:42PM -0700, Junio C Hamano wrote:
>
>> * en/header-split-cache-h-part-3 (2023-05-16) 29 commits
>>  ...
>>  Will merge to 'master' together with its fix-up in js/cmake-wo-cache-h
>>  source: <pull.1525.v3.git.1684218848.gitgitgadget@gmail.com>
>
> I think this series has a bug with finding the correct templates dir. If
> I check out 76ebce0b7a (Merge branch 'en/header-split-cache-h-part-3'
> into next, 2023-06-15) and run:
>
>   make prefix=/tmp/foo install && /tmp/foo/bin/git init /tmp/bar
>
> I get:
>
>   warning: templates not found in /usr/share/git-core/templates
>
> Whereas if I go to 76ebce0b7a^, that warning does not appear (and
> presumably the templates come from /tmp/foo/share, but I didn't check).

Ouch.

> Our tests can't notice because they always specify the in-repo template
> dir directly in the bin-wrappers script (and other users might not even
> get the warning if they have another git installed in /usr; it would
> just silently use the wrong template).
>
>   Side note: I'm using the merge along 'next' there because much to my
>   surprise, the tip of en/header-split-cache-h-part-3 does not build by
>   itself! It needs the evil merge result from ccd12a3d6c (Merge branch
>   'en/header-split-cache-h-part-2', 2023-05-09). I wonder if it got
>   applied on the wrong base. It does work when merged to 'next' (and
>   should with 'master' as well), but it hurts bisectability.
>
> After rebasing on 'master' to make it buildable on its own, I bisected
> my make/init command above, which shows the template problem appearing
> in 3f85c72fad (setup: adopt shared init-db & clone code, 2023-05-16).
>
> I guess the problem is the movement of the code from init-db.c to
> setup.c, and we'd want something like this:
>
> diff --git a/Makefile b/Makefile
> index e440728c24..b09c8165d0 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2742,8 +2742,8 @@ exec-cmd.sp exec-cmd.s exec-cmd.o: EXTRA_CPPFLAGS = \
>  	'-DBINDIR="$(bindir_relative_SQ)"' \
>  	'-DFALLBACK_RUNTIME_PREFIX="$(prefix_SQ)"'
>  
> -builtin/init-db.sp builtin/init-db.s builtin/init-db.o: GIT-PREFIX
> -builtin/init-db.sp builtin/init-db.s builtin/init-db.o: EXTRA_CPPFLAGS = \
> +setup.sp setup.s setup.o: GIT-PREFIX
> +setup.sp setup.s setup.o: EXTRA_CPPFLAGS = \
>  	-DDEFAULT_GIT_TEMPLATE_DIR='"$(template_dir_SQ)"'
>  
>  config.sp config.s config.o: GIT-PREFIX
>
>
> It does make me wonder if we should also just do this:
>
> diff --git a/setup.c b/setup.c
> index f8e1296766..6e7282e680 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -1718,10 +1718,6 @@ int daemonize(void)
>  #endif
>  }
>  
> -#ifndef DEFAULT_GIT_TEMPLATE_DIR
> -#define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
> -#endif
> -
>  #ifdef NO_TRUSTABLE_FILEMODE
>  #define TEST_FILEMODE 0
>  #else
>
> Since the Makefile always provides that value, having a baked-in
> fallback does not make much sense. And in this case it masked a real bug
> which should have been a compilation error, and instead silently used
> the wrong value.
>
> So I think we'd at least want to fix the Makefile before graduating this
> topic any further. But IMHO it would also be worth adjusting the topic's
> start point so that we don't have a big chunk of commits which fail to
> build in the final history.

Hmph, meaning (1) revert the merge of the topic to 'next', (2)
rebase the topic on top of the current 'master', instead of 4bd872e0,
which was a merge of the prerequisite series into then-current
master, (3) apply the Makefile (plus setup.c) fix, and then (4)
merge the result back to 'next'?