Glen Choo <chooglen@xxxxxxxxxx> writes: > >> + # Use test_cmp manually because verify_fetch_result does not > >> + # consider submodule2. All the repos should be fetched, but only > >> + # submodule2 should be read from a commit > >> + cat <<-EOF > expect.err.combined && > >> + From $pwd/. > >> + OLD_HEAD..$super_head super -> origin/super > >> + OLD_HEAD..$super_sub2_only_head super-sub2-only -> origin/super-sub2-only > >> + Fetching submodule submodule > >> + From $pwd/submodule > >> + OLD_HEAD..$sub_head sub -> origin/sub > >> + Fetching submodule submodule/subdir/deepsubmodule > >> + From $pwd/deepsubmodule > >> + OLD_HEAD..$deep_head deep -> origin/deep > >> + Fetching submodule submodule2 at commit $super_sub2_only_head > >> + From $pwd/submodule2 > >> + OLD_HEAD..$sub2_head sub2 -> origin/sub2 > >> + EOF > >> + sed -E "s/[0-9a-f]+\.\./OLD_HEAD\.\./" actual.err >actual.err.cmp && > >> + test_cmp expect.err.combined actual.err.cmp > >> +' > > > > Could verify_fetch_result be modified to consider the new submodule > > instead? > > Since submodule2 is on the end of the file, I could modify > verify_fetch_result() to concatenate extra text on the end. But if it > weren't in the middle, we'd need to insert arbitrary text in the middle > of the file. > > I can't think of a good way to do this without compromising test > readability, so I'll just do concatenation for now. Looking at it, I think you can do it by adding a section that verifies the "Fetching submodule submodule2" part if the file is present (so, no change in behavior in the rest of the tests since they don't write this file) and also modifying check_super to allow specification of the sub2 part (or making a new function for this). > > What's the error message printed to the user here? (Just from reading > > the code, I would have expected this to succeed, with the submodule > > fetch being from same-name-1's submodule since we're fetching submodules > > by name, but apparently that is not the case.) > > Yeah, I think this might trip up some readers. The message is: > > From ../same-name-2 > b7ebb59..944b5ac master -> same-name-2/master > Fetching submodule submodule > fatal: git upload-pack: not our ref 7ff6874077503acb9d0a52e280aaed9748276319 > fatal: remote error: upload-pack: not our ref 7ff6874077503acb9d0a52e280aaed9748276319 > Errors during submodule fetch: > submodule > > Which, I believe, comes from how we fetch commits by oid: > > static int get_next_submodule(struct child_process *cp, struct strbuf *err, > void *data, void **task_cb) > [...] > oid_array_for_each_unique(task->commits, > append_oid_to_argv, &cp->args); > > When the following is true: > > - the submodule is found in the index > - we are fetching submodules unconditionally (--recurse-submodules=yes") > - no superproject commit "changes" the submodule > > task->commits is empty, and we just fetch the from the submodule's > remote by name. But as long as any superproject commit "changes" the > submodule, we try to fetch by oid, which, as this test demonstrates, may > fail. Ah, so we try to fetch an OID from a submodule given by a fetched commit, which is different from the submodule the client already has locally. This might be a sign that we need to store more information about the submodule so that we can print a clearer message. I haven't looked into this deeply, but this might be possible by putting more information in the util of changed_submodule_names, and when we have already seen that submodule, to add more information to the util instead of skipping it.
