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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Mar 14, 2017 at 10:46 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Brandon Williams <bmwill@xxxxxxxxxx> writes:
>
>> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
>> ---
>>  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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]