Re: [PATCH v3 01/10] submodule--helper: add is_active command

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 14, 2017, 17:40 UTC
Message-ID: <xmqqh92vkb8d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170313214341.172676-2-bmwill@google.com>

Brandon Williams <bmwill@google.com> writes:

> There are a lot of places where an explicit check for
> submodule."<name>".url is done to see if a submodule exists.  In order
> to centralize this check introduce a helper which can be used to query
> if a submodule is active or not.

    "Right now, is_submodule_initialized(), the underlying
    implementation of "submodule--helper is-active", checks exactly
    the same thing.  When submodule.<name>.url is set to a non-empty
    string, then it is initialized.  We'd want to change this
    definition in later steps of this series and having the checks
    centralized is a necessary preparatory step."

or something like that need to be tacked at the end to make it known that 02-04/10 are meant as pure code clean-up without improvement in behaviour.

"Tacking at the end" will make the whole thing awkward to read, and that is because the flow of logic is inverted. The first sentence, "there are a lot of places...", is trying to state that there is a problem worthy of fixing, but it does so without saying why it is a problem in the first place. After step 04/10 of this series, we would still be able to say "There are a log of places where an explicit call to "submodule--helper is-active" is done to see if a submodule exists." Is that still a problem? The answer depends on what motivated this change, and it is better to make the motivation known in advance.

Here is my attempt to rewrite the whole thing.

    The definition of which submodules are of interest by the user
    is tied to the configuration submodule.<name>.url; when it is
    set to a non-empty string, it is of interest.  We'd want to be
    able to later change this definition, but there are many places
    that explicitly check this condition in the scripted Porcelain.

    Introduce the "is-active" subcommand to "submodule--helper", so
    that the exact definition of what submodule is of interest can
    be centrally defined (and changed in later steps).  In a few
    patches that follow, this helper is used to replace the explicit
    checks of the configuration variable in scripts.

You are adding the "is-active" subcommand, and the implementation detail to do so is by using a new is_active() function, so the patch title needs a bit of tweaking, too.

	Subject: submodule--helper: add is-active subcommand

Show 35 quoted lines

> diff --git a/t/t7413-submodule-is-active.sh b/t/t7413-submodule-is-active.sh
> new file mode 100755
> index 000000000..f18e0c925
> --- /dev/null
> +++ b/t/t7413-submodule-is-active.sh
> @@ -0,0 +1,31 @@
> +#!/bin/sh
> +
> +test_description='Test submodule--helper is-active
> +
> +This test verifies that `git submodue--helper is-active` correclty identifies
> +submodules which are "active" and interesting to the user.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' '
> +	git init sub &&
> +	test_commit -C sub initial &&
> +	git init super &&
> +	test_commit -C super initial &&
> +	git -C super submodule add ../sub sub1 &&
> +	git -C super submodule add ../sub sub2 &&
> +	git -C super commit -a -m "add 2 submodules at sub{1,2}"
> +'
> +
> +test_expect_success 'is-active works with urls' '
> +	git -C super submodule--helper is-active sub1 &&
> +	git -C super submodule--helper is-active sub2 &&
> +
> +	git -C super config --unset submodule.sub1.URL &&
> +	test_must_fail git -C super submodule--helper is-active sub1 &&
> +	git -C super config submodule.sub1.URL ../sub &&
> +	git -C super submodule--helper is-active sub1
> +'

Looks sensible.

Thanks.