Re: [PATCH v2 2/2] read-cache: drop submodule check from add_to_cache()

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> From: Jeff King <peff@peff.net>
>
> In add_to_cache(), we treat any directories as submodules, and complain
> if we can't resolve their HEAD. This call to resolve_gitlink_ref() was
> added by f937bc2f86 (add: error appropriately on repository with no
> commits, 2019-04-09), with the goal of improving the error message for
> empty repositories.
>
> But we already resolve the submodule HEAD in index_path(), which is
> where we find the actual oid we're going to use. Resolving it again here
> introduces some downsides:
>
>   1. It's more work, since we have to open up the submodule repository's
>      files twice.
>
>   2. There are call paths that get to index_path() without going through
>      add_to_cache(). For instance, we'd want a similar informative
>      message if "git diff empty" finds that it can't resolve the
>      submodule's HEAD. (In theory we can also get there through
>      update-index, but AFAICT it refuses to consider directories as
>      submodules at all, and just complains about them).
>
>   3. The resolution in index_path() catches more errors that we don't
>      handle here. In particular, it will validate that the object format
>      for the submodule matches that of the superproject. This isn't a
>      bug, since our call in add_to_cache() throws away the oid it gets
>      without looking at it. But it certainly caused confusion for me
>      when looking at where the object-format check should go.
>
> So instead of resolving the submodule HEAD in add_to_cache(), let's just
> teach the call in index_path() to actually produce an error message
> (which it already does for other cases). That's probably what f937bc2f86
> should have done in the first place, and it gives us a single point of
> resolution when adding a submodule to the index.
>
> The resulting output is slightly more verbose, as we propagate the error
> up the call stack, but I think that's OK (and again, matches many other
> errors we get when indexing fails).
>
> I've left the text of the error message as-is, though it is perhaps
> overly specific.  There are many reasons that resolving the submodule
> HEAD might fail, though outside of corruption or system errors it is
> probably most likely that the submodule HEAD is simply on an unborn
> branch.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

A tangent.

You can (!) place your own sign-off after Peff's, as you would want
to certify

c. The contribution was provided directly to me by some other
   person who certified (a), (b) or (c) and I have not modified
   it.

of the DCO, but it seems that it is optional (which I did not know
about---the explanation in SubmittingPatches stops at "Indeed you
are encouraged to do so" without making it a requirement).