Re: [PATCH] git-submodule.sh: try harder to fetch a submodule

From: Stefan Beller <sbeller@google.com>
Date: May 15, 2018, 19:07 UTC
Message-ID: <CAGZ79ka_8GmL9j9mTNLkbqN3xNkfCfedzs2st-tH8jMjQ2A4DQ@mail.gmail.com>
In-Reply-To: <xmqqmux5g2pa.fsf@gitster-ct.c.googlers.com>

On Fri, May 11, 2018 at 5:03 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 6 quoted lines

>> A more typical example would be if the ref simply doesn't exist (i.e.,
>> is a branch yet to be born).
>
> Indeed this is interesting.  At first glance I thought this was
> about underlying "git clone" failing to grab things from a
> repository with unborn HEAD, but that part works perfectly OK.

ok.

Show 8 quoted lines

> So it probably should be more like
>
>         guard1 || action1 || warn
>         guard2 || action2 || die
>
> so that no matter what the outcome of the action1 is, the second set
> gets executed.
>

I'll resend it with a warning (using say()).

I think we have 2 bugs and this is merely fixing the second bug.

The first bug:
We had a call chain as
  git clone --recurse-submodules=<path spec>
    -> git submodule update --init --recursive $(pathspec)
      -> git submodule--helper update-clone # will clone
        -> git submodule helper clone
          -> git clone --no-checkout --separate-git-dir ...

The call to the "git clone" produces an interesting submodule state:

  $ git init confused-head
  $ (cd confused-head && git branch test \
        $(git commit-tree $(git write-tree) -m test))
  $ git clone --no-checkout  --depth=1 \
        --separate-git-dir=test.git confused-head/.git test
Cloning into 'test'...
warning: --depth is ignored in local clones; use file:// instead.
done.

  $ git -C test.git config remote.origin.fetch
  $ echo $?
1

(A) Despite the warning of --depth having no impact, the
  omission thereof changes the repository state.
(B) There is no remote.origin.fetch configuration, which
  is weird. See builtin/clone.c:830, that states for this case:

    /*
     * otherwise, the next "git fetch" will
     * simply fetch from HEAD without updating
     * any remote-tracking branch, which is what
     * we want.
     */

I disagree as the next fetch will be confused (HEAD is not advertised on the next ls-remote)

The patch that is under discussion here is merely papering over the effect of having no fetch spec, by allowing the second fetch (fetching the sha1 directly) to run, which ignores the configuration as a refspec is given.

However it is still a bug, as such repositories are out there, which is why I said there are 2 bugs initially. It's just that the first bug enables the second bug.

Thanks, Stefan