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

Commit Message

Jeff King June 21, 2023, 8:55 a.m. UTC

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
>   (merged to 'next' on 2023-06-15 at 76ebce0b7a)
>  + fsmonitor-ll.h: split this header out of fsmonitor.h
>  + hash-ll, hashmap: move oidhash() to hash-ll
>  + object-store-ll.h: split this header out of object-store.h
>  + khash: name the structs that khash declares
>  + merge-ll: rename from ll-merge
>  + git-compat-util.h: remove unneccessary include of wildmatch.h
>  + builtin.h: remove unneccessary includes
>  + list-objects-filter-options.h: remove unneccessary include
>  + diff.h: remove unnecessary include of oidset.h
>  + repository: remove unnecessary include of path.h
>  + log-tree: replace include of revision.h with simple forward declaration
>  + cache.h: remove this no-longer-used header
>  + read-cache*.h: move declarations for read-cache.c functions from cache.h
>  + repository.h: move declaration of the_index from cache.h
>  + merge.h: move declarations for merge.c from cache.h
>  + diff.h: move declaration for global in diff.c from cache.h
>  + preload-index.h: move declarations for preload-index.c from elsewhere
>  + sparse-index.h: move declarations for sparse-index.c from cache.h
>  + name-hash.h: move declarations for name-hash.c from cache.h
>  + run-command.h: move declarations for run-command.c from cache.h
>  + statinfo: move stat_{data,validity} functions from cache/read-cache
>  + read-cache: move shared add/checkout/commit code
>  + add: modify add_files_to_cache() to avoid globals
>  + read-cache: move shared commit and ls-files code
>  + setup: adopt shared init-db & clone code
>  + init-db, clone: change unnecessary global into passed parameter
>  + init-db: remove unnecessary global variable
>  + init-db: document existing bug with core.bare in template config
>  + Merge branch 'en/header-split-cache-h-part-2' into en/header-split-cache-h-part-3
>  (this branch is used by js/cmake-wo-cache-h.)
> 
>  Originally merged to 'next' on 2023-06-13
> 
>  Header files cleanup.
> 
>  Will merge to 'master' together with its fix-up in js/cmake-wo-cache-h
>  source: <[e-mail]>

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

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:


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.

-Peff

Comments

Junio C Hamano June 21, 2023, 5:05 p.m. UTC | #1

Jeff King <[e-mail]> 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: <[e-mail]>
>
> 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'?

Jeff King June 21, 2023, 8:26 p.m. UTC | #2

On Wed, Jun 21, 2023 at 10:05:40AM -0700, Junio C Hamano wrote:

> > 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'?

Yeah. I guess the real build problem is actually in the merge of split-2
(it conflicted with a simultaneous topic, hence the fix coming in the
merge). So another option to address that here would be to amend the
4bd872e0ed (Merge branch 'en/header-split-cache-h-part-2' into
en/header-split-cache-h-part-3, 2023-05-08) to include that fixup.

As for the others, I'd consider:

  1. (optional) Drop the #ifndef at the very start of the series, before
     we touch anything, with the rationale that it is not doing anything
     and masks errors. I don't _think_ this can ever backfire, because
     we unconditionally set DEFAULT_GIT_TEMPLATE_DIR (unlike some other
     things like DEFAULT_PAGER, where the Makefile might leave it
     unset). But we can also leave this out, or do it as a separate
     topic, if we want to minimize changes / risk of screwing something
     up.

  2. Squash the Makefile fix into the "adopt shared init-db" patch
     (currently 0d652b238).

And that would leave the result fully bisectable. But if we prefer to
keep the history closer to reality, I can prepare the Makefile thing as
a patch on top.

-Peff

Junio C Hamano June 21, 2023, 10:03 p.m. UTC | #3

Jeff King <[e-mail]> writes:

> Yeah. I guess the real build problem is actually in the merge of split-2
> (it conflicted with a simultaneous topic, hence the fix coming in the
> merge). So another option to address that here would be to amend the
> 4bd872e0ed (Merge branch 'en/header-split-cache-h-part-2' into
> en/header-split-cache-h-part-3, 2023-05-08) to include that fixup.
>
> As for the others, I'd consider:
>
>   1. (optional) Drop the #ifndef at the very start of the series, before
>      we touch anything, with the rationale that it is not doing anything
>      and masks errors. I don't _think_ this can ever backfire, because
>      we unconditionally set DEFAULT_GIT_TEMPLATE_DIR (unlike some other
>      things like DEFAULT_PAGER, where the Makefile might leave it
>      unset). But we can also leave this out, or do it as a separate
>      topic, if we want to minimize changes / risk of screwing something
>      up.
>
>   2. Squash the Makefile fix into the "adopt shared init-db" patch
>      (currently 0d652b238).
>
> And that would leave the result fully bisectable. But if we prefer to
> keep the history closer to reality, I can prepare the Makefile thing as
> a patch on top.

I've done the messiest, I guess ;-)

 * revert merge of the part-3 topic and Dscho's cmake fix out of 'next'.

 * rebase part-3 on top of the more recent 'master'.

 * squash in the two hunks (including setup.c change) from you into
   the "setup: adopt shared init-db & clone code" step.

 * squash in Dscho's cmake fix into "cache.h: remove this
   no-longer-used header" step.

The result is not in 'next' yet until I hear something from those
who have been involved in the topic, including Elijah and Dscho.

Thanks.

Elijah Newren June 23, 2023, 6:33 a.m. UTC | #4

On Wed, Jun 21, 2023 at 3:03 PM Junio C Hamano <[e-mail]> wrote:
>
> Jeff King <[e-mail]> writes:
>
> > Yeah. I guess the real build problem is actually in the merge of split-2
> > (it conflicted with a simultaneous topic, hence the fix coming in the
> > merge). So another option to address that here would be to amend the
> > 4bd872e0ed (Merge branch 'en/header-split-cache-h-part-2' into
> > en/header-split-cache-h-part-3, 2023-05-08) to include that fixup.
> >
> > As for the others, I'd consider:
> >
> >   1. (optional) Drop the #ifndef at the very start of the series, before
> >      we touch anything, with the rationale that it is not doing anything
> >      and masks errors. I don't _think_ this can ever backfire, because
> >      we unconditionally set DEFAULT_GIT_TEMPLATE_DIR (unlike some other
> >      things like DEFAULT_PAGER, where the Makefile might leave it
> >      unset). But we can also leave this out, or do it as a separate
> >      topic, if we want to minimize changes / risk of screwing something
> >      up.
> >
> >   2. Squash the Makefile fix into the "adopt shared init-db" patch
> >      (currently 0d652b238).
> >
> > And that would leave the result fully bisectable. But if we prefer to
> > keep the history closer to reality, I can prepare the Makefile thing as
> > a patch on top.
>
> I've done the messiest, I guess ;-)
>
>  * revert merge of the part-3 topic and Dscho's cmake fix out of 'next'.
>
>  * rebase part-3 on top of the more recent 'master'.
>
>  * squash in the two hunks (including setup.c change) from you into
>    the "setup: adopt shared init-db & clone code" step.
>
>  * squash in Dscho's cmake fix into "cache.h: remove this
>    no-longer-used header" step.
>
> The result is not in 'next' yet until I hear something from those
> who have been involved in the topic, including Elijah and Dscho.

I did a range-diff to compare my original series to your newly rebased
one, and read through all the differences (including Dscho's and
Peff's suggested changes, as well as the various slight adjustments
due to rebasing).  I also rebuilt every patch in your rebase of the
series to ensure they all build, and ran all tests on a couple of the
patches to verify the pass (I didn't run all tests for each patch,
because I did that for my original series and the differences in the
range-diff suggested I only needed to spot check all the tests).

Anyway, your rebase of en/header-split-cache-h-part-3, including
Dscho's and Peff's changes, all look good to me.

Thanks everyone!

Junio C Hamano June 23, 2023, 4:38 p.m. UTC | #5

Elijah Newren <[e-mail]> writes:

> Anyway, your rebase of en/header-split-cache-h-part-3, including
> Dscho's and Peff's changes, all look good to me.
>
> Thanks everyone!

Thanks for confirming.  Let's merge it down to 'next' then.

Jeff King June 24, 2023, 1:25 a.m. UTC | #6

On Fri, Jun 23, 2023 at 09:38:07AM -0700, Junio C Hamano wrote:

> Elijah Newren <[e-mail]> writes:
> 
> > Anyway, your rebase of en/header-split-cache-h-part-3, including
> > Dscho's and Peff's changes, all look good to me.
> >
> > Thanks everyone!
> 
> Thanks for confirming.  Let's merge it down to 'next' then.

I did a range-diff between en/header-split-cache-h-part-3{1} and the
current tip, and the changes look good to me. It might have been worth
mentioning the #ifndef changes in the commit message for e8cf8ef507
(setup: adopt shared init-db & clone code, 2023-05-16). But I see you
merged to 'next', and I don't think it's worth going back to change it
again.

It's nice when we can get everything in there, but I find that it's not
uncommon to sometimes dig extra details out of the list archive if I
don't understand part of a change when doing archaeology.

-Peff

Patch

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