Re: [PATCH v3 3/9] submodule--helper: don't use global --super-prefix in "absorbgitdirs"

From: Glen Choo <chooglen@google.com>
Date: Nov 22, 2022, 19:53 UTC
Message-ID: <kl6lfseaepfu.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <patch-v3-3.9-6e10a47c60a-20221119T122853Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 10 quoted lines

> The "fsmonitor--daemon" test adjusted here was added in [3]. To assert
> that it didn't run into the "--super-prefix" message it was asserting
> the output it didn't have. Let's instead assert the full output that
> we *do* have, which we can do here as this is based on a change[4] to
> make it predictable (until [4] it contained absolute paths).
>
> We could also remove the test entirely (as [5] did), but even though
> the initial reason for having it is gone we're still getting some
> marginal benefit from testing the "fsmonitor" and "submodule
> absorbgitdirs" interaction, so let's keep it.

I'm a bit ambivalent on this, especially since we're testing "submodule absorbgitdirs" output in "fsmonitor" tests, but as you say, there is some benefit in testing the interaction. I think we'll have to change the test though...

Show 12 quoted lines

> The change here to have either a NULL or non-"" string as a
> "super_prefix" instead of the previous arrangement of "" or non-"" is
> somewhat arbitrary. We could also decide to never have to check for
> NULL.
>
> As we'll be changing the rest of the "git --super-prefix" users to the
> same pattern, leaving them all consistent makes sense. Why not pick ""
> over NULL? Because that's how the "prefix" works[6], and having
> "prefix" and "super_prefix" work the same way will be less
> confusing. That "prefix" picked NULL instead of "" is itself
> arbitrary, but as it's easy to make this small bit of our overall API
> consistent, let's go with that.

Okay, I find consistency with "prefix" convincing, and I do hope that we use "" someday :)

Show 12 quoted lines

> index 4abc74db2bb..31526937d95 100755
> --- a/t/t7527-builtin-fsmonitor.sh
> +++ b/t/t7527-builtin-fsmonitor.sh
> @@ -866,30 +866,11 @@ test_expect_success 'submodule always visited' '
>  # the submodule, and someone does a `git submodule absorbgitdirs`
>  # in the super, Git will recursively invoke `git submodule--helper`
>  # to do the work and this may try to read the index.  This will
> -# try to start the daemon in the submodule *and* pass (either
> -# directly or via inheritance) the `--super-prefix` arg to the
> -# `git fsmonitor--daemon start` command inside the submodule.
> -# This causes a warning because fsmonitor--daemon does take that
> -# global arg (see the table in git.c)

We've removed mentions of the "--super-prefix", as we should, since fsmonitor doesn't need to care about "--super-prefix" any more...

Show 15 quoted lines

> @@ -904,10 +885,12 @@ test_expect_success "stray submodule super-prefix warning" '
>  
>  	test_path_is_dir super/dir_1/dir_2/sub/.git &&
>  
> -	GIT_TRACE2_EVENT="$PWD/super-sub.trace" \
> -		git -C super submodule absorbgitdirs &&
> -
> -	! have_t2_error_event super-sub.trace
> +	cat >expect <<-\EOF &&
> +	Migrating git directory of '\''dir_1/dir_2/sub'\'' from '\''dir_1/dir_2/sub/.git'\'' to '\''.git/modules/dir_1/dir_2/sub'\''
> +	EOF
> +	git -C super submodule absorbgitdirs >out 2>actual &&
> +	test_cmp expect actual &&
> +	test_must_be_empty out
>  '

So let's adjust the test to match what we're testing. It's not the --super-prefix any more, it's the interaction between "submodule absorbgitdirs" and "fsmonitor--daemon", so perhaps something like:

  diff --git a/t/t7527-builtin-fsmonitor.sh b/t/t7527-builtin-fsmonitor.sh
  index 31526937d9..9f947341f5 100755
  --- a/t/t7527-builtin-fsmonitor.sh
  +++ b/t/t7527-builtin-fsmonitor.sh
  @@ -868,7 +868,7 @@ test_expect_success 'submodule always visited' '
  # to do the work and this may try to read the index.  This will
  # try to start the daemon in the submodule.

  -test_expect_success "stray submodule super-prefix warning" '
  +test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
          test_when_finished "rm -rf super; \
                              rm -rf sub" &&

  @@ -888,9 +888,14 @@ test_expect_success "stray submodule super-prefix warning" '
          cat >expect <<-\EOF &&
          Migrating git directory of '\''dir_1/dir_2/sub'\'' from '\''dir_1/dir_2/sub/.git'\'' to '\''.git/modules/dir_1/dir_2/sub'\''
          EOF
  -       git -C super submodule absorbgitdirs >out 2>actual &&
  +       GIT_TRACE2_EVENT="$PWD/.git/trace_absorbgitdirs" \
  +               git -C super submodule absorbgitdirs >out 2>actual &&
          test_cmp expect actual &&
  -       test_must_be_empty out
  +       test_must_be_empty out &&
  +
  +       # Confirm that the trace2 log contains a record of the
  +       # daemon starting.
  +       test_subcommand git fsmonitor--daemon start <.git/trace_absorbgitdirs
  '

  # On a case-insensitive file system, confirm that the daemon