patchgit-submodule.sh: try harder to fetch a submodule

This is the logical continuum of fb43e31f2b4 (submodule: try harder to fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as some assumptions were not correct.

assumes that the fetch_in_submodule only fails when the serverside does not support fetching by sha1.

There are other failures, why such a fetch may fail, such as
    fatal: Couldn't find remote ref HEAD
which can happen if the remote side doesn't advertise HEAD. Not advertising
HEAD is allowed by the protocol spec and would happen, if HEAD points at a
ref, that this user cannot see (due to ACLs for example).

So do try even harder for a submodule by ignoring the exit code of the first fetch and rather relying on the following is_tip_reachable to see if we try fetching again.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 1 -
 1 file changed, 1 deletion(-)

Hi,

Stefan Beller wrote:

Interesting.

I think what would help most is an example set of commands I can use to reproduce this (bonus points if in the form of a test).

I'm having some trouble with the formatting here. Is the part preceded by '>' a quote, and if so a quote from what?

nit: it can be useful to have a blank line before and after such example output to help both my eyes and tools like "git log --format='%w(100)%b'" to understand the formatting.

A more typical example would be if the ref simply doesn't exist (i.e., is a branch yet to be born).

Is keeping the '||' at the end of this line intended?

Should this error message be changed?

Thanks, Jonathan

On Fri, May 11, 2018 at 4:28 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

I tried coming up with a test in git-core that produces a remote similar to Gerrit, and let me tell you, it's not Git that is weird here. ;)

The quote is from fb43e31f2b4.

Why would you use this formatting?

Oh, I checked that but not for the submodule case, let me check that.

yes, as we only want to run the code below if there was some error.

I don't think so?

Jonathan Nieder <jrnieder@gmail.com> writes:

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. And if this failed clone were a full-repository clone that tried to grab even HEAD, then it is likely that we got the tip we need to populate the submodule's working tree (or the remote is useless for that in the first place). So the "allow to try even harder" is probably a good direction to go in.

Good question.  It used to be

	guard1 || action1 || die
	guard2 || action2 || die

Even after a successful exit from "action1", the code used to try the second attempt, and the patch is removing the first die, making the whole thing into

	guard1 || action1 ||
	guard2 || action2 || die

which suggests a grave regression, doesn't it? "action1" (a whole repository fetch) may not pull down the needed commit even the fetch operation itself may succeed, in which case "guard2" notices that the tip is still not here and "action2" (an exact SHA-1 fetch) tries to pull down the exact thing as the last resort.

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.

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

ok.

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

This is the logical continuum of fb43e31f2b4 (submodule: try harder to fetch needed sha1 by direct fetching sha1, 2016-02-23) and fixes it as some assumptions were not correct.

The commit states:

There are other failures, why such a fetch may fail, such as
    fatal: Couldn't find remote ref HEAD
which can happen if the remote side doesn't advertise HEAD and we do not
have a local fetch refspec.

Not advertising HEAD is allowed by the protocol spec and would happen, if HEAD points at an unborn branch for example.

Not having a local fetch refspec can happen when submodules are fetched shallowly, as then git-clone doesn't setup a fetch refspec.

So do try even harder for a submodule by ignoring the exit code of the first fetch and rather relying on the following is_tip_reachable to see if we try fetching again.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 git-submodule.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Stefan Beller wrote:

Thanks, makes sense.

I'm fearing that there are more than two.

[...]

I can reproduce the issue without submodules and without --local, as follows:

	git init --bare empty.git
	git init --bare almost-empty.git
	git -C ~/src/git push $(pwd)/almost-empty HEAD:refs/heads/upstream

	git clone --single-branch file://$(pwd)/empty.git
	git clone --single-branch file://$(pwd)/almost-empty.git

	git -C almost-empty.git branch -D upstream

	git -C empty fetch
	git -C almost-empty fetch

Expected result: Both fetches succeed.

Actual result: First fetch succeeds, second produces "fatal: Couldn't find remote ref HEAD".

Note that empty.git and almost-empty.git are basically identical. The difference instead lies in the clones' .git/config files: