Re: [PATCH v2 03/11] submodule deinit: use most reliable url

From: Stefan Beller <sbeller@google.com>
Date: Mar 9, 2017, 01:56 UTC
Message-ID: <CAGZ79kZG+Y+AtYYxeE5hmsNxfwmNp1h7aKAem=JD3yiKB6STPw@mail.gmail.com>
In-Reply-To: <20170309012345.180702-4-bmwill@google.com>

On Wed, Mar 8, 2017 at 5:23 PM, Brandon Williams <bmwill@google.com> wrote:

> The user could have configured the submodule to have a different URL
> from the one in the superproject's config.  To account for this read
> what the submodule has configured for remote.origin.url and use that
> instead.

When reading this commit message, I first thought this is an unrelated bug fix. However it is not unrelated. In a later patch you introduce a mode where submodule.<name>.url may not exist in .git/config any more, (there may be a .existence flag instead?) such that url="", which is obviously a bad UI:

    Submodule '$name' (<empty string>) unregistered for path '$displaypath'"

Like the first patch of this series, we could have a helper function
"git submodule--helper url <name>" that figures out how to get the URL:
1) Look into that GIT_DIR
2) if non-existent check .git/config for the URL or
3) fall back to .gitmodules?

Instead of having such a helper, we directly look into that first option hoping it exists, however it doesn't have to:

  git submodule init <ps>
  # no command in between
  git submodule deinit <ps>
  # submodule was not cloned yet, but we still test positive for
    > # Remove the .git/config entries (unless the user already did it)
    > if test -n "$(git config --get-regexp submodule."$name\.")"

I am not sure if there is an easy way out here.

Thinking about the name for such a url helper lookup, I wonder if we want to have

    git submodule--helper show-url <name>

as potentially we end up having this issue for a lot of different things (e.g. submodule.<name>.shallow = (true,false), or in case the submodule is cloned you can give the actual depth as an integral number), so maybe we'd want to introduce one layer of indirection

    git submodule--helper ask-property \
       (is-active/URL/depth/size/..) <name>

So with that said, I wonder if we also want to ease up:

    GIT_DIR="$(git rev-parse --git-path modules/$name

to be

    GIT_DIR=$(git submodule--helper show-git-dir $name)

Show 5 quoted lines

>                 then
>                         # Remove the whole section so we have a clean state when
>                         # the user later decides to init this submodule again
> -                       url=$(git config submodule."$name".url)
> +                       url=$(GIT_DIR="$(git rev-parse --git-path modules/$name)" git config remote.origin.url)

In the submodule helper we have get_default_remote(), which we do not have in shell (which we seemed to have in shell?), so maybe it's worth not using origin here, although I guess it is rare that the original remote is named other than origin.

Show 6 quoted lines

>                         git config --remove-section submodule."$name" 2>/dev/null &&
>                         say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
>                 fi
> --
> 2.12.0.246.ga2ecc84866-goog
>

Thanks, Stefan