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

[Brandon Williams <bmwill@google.com>]

On 03/08, Stefan Beller wrote:
> 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:

So the later patch doesn't actually omit submodule.<name>.url but we
could end up with this if you init and then deinit without actually
cloning the submodule.  So maybe the best bet is to drop this patch from
the series and we can include one like it in a follow up which uses a
helper function that you suggest.

> 
>     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)
> 
> >                 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.
> 
> >                         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

-- 
Brandon Williams