Re: [PATCH v3 02/10] submodule status: use submodule--helper is-active

From: Stefan Beller <sbeller@google.com>
Date: Mar 14, 2017, 18:16 UTC
Message-ID: <CAGZ79kZ6noz7RGbmNwg=NhuWYH=KbNPYUWL7We9YjbHn6uf2Jg@mail.gmail.com>
In-Reply-To: <xmqqd1djkay4.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 14, 2017 at 10:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 39 quoted lines

> Brandon Williams <bmwill@google.com> writes:
>
>> Signed-off-by: Brandon Williams <bmwill@google.com>
>> ---
>>  git-submodule.sh | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 136e26a2c..ab233712d 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -1010,14 +1010,13 @@ cmd_status()
>>       do
>>               die_if_unmatched "$mode" "$sha1"
>>               name=$(git submodule--helper name "$sm_path") || exit
>> -             url=$(git config submodule."$name".url)
>>               displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
>>               if test "$stage" = U
>>               then
>>                       say "U$sha1 $displaypath"
>>                       continue
>>               fi
>> -             if test -z "$url" ||
>> +             if ! git submodule--helper is-active "$sm_path" ||
>>               {
>>                       ! test -d "$sm_path"/.git &&
>>                       ! test -f "$sm_path"/.git
>
> The $name is no longer used after this step in cmd_status function,
> as the sole purpose of learning the name from the path was so that
> we can ask if the submodule has .URL defined and the query is done
> by name, not path.
>
> This actually raises a question.
>
> Shouldn't "submodule--helper is-active" ask about submodule while
> identifying the submodule in question by name?  Or do all (or most
> of) the callers start from path and ask is-active on them so that it
> is handier to let them ask by path?

A similar observation can be made, when looking at submodule_from_name and submodule_from_path (both defined in submodule-config.h)

submodule_from_name has only one real world use case in code, which I suspect is even a bug.

That line was last touched with 851e18c3859a (submodule: use new config API for worktree configurations). but the original mechanism comes from 7dce19d374 (fetch/pull: Add the --recurse-submodules option).

There we take the path as name and if a real name exists, the name is overwritten with the real name, i.e.

    name = name_for_path(path) ? name_for_path(path) : path;

which IMHO is overly accepting and we should just die in case of !name_for_path.

So to answer your original question, I think the codebase currently thinks by_path is handier, the name is a mere internal field in "struct submodule", useful for looking up its git dir.

Thanks, Stefan