Re: [PATCH] object-file: disallow adding submodules of different hash algo

> -- >8 --
> Subject: [PATCH] read-cache: drop submodule check from add_to_cache()
>
> 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>
> ---
>  object-file.c  | 2 +-
>  read-cache.c   | 3 ---
>  t/t3700-add.sh | 1 +
>  3 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/object-file.c b/object-file.c
> index 8c43c52ed0..a7438b6205 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -1662,7 +1662,7 @@ int index_path(struct index_state *istate, struct object_id *oid,
>  		break;
>  	case S_IFDIR:
>  		if (repo_resolve_gitlink_ref(istate->repo, path, "HEAD", oid))
> -			return -1;
> +			return error(_("'%s' does not have a commit checked out"), path);
>  		if (&hash_algos[oid->algo] != istate->repo->hash_algo)
>  			return error(_("cannot add a submodule of a different hash algorithm"));
>  		break;
> diff --git a/read-cache.c b/read-cache.c
> index 032480d0c7..990d4ead0d 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -706,7 +706,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  	int add_option = (ADD_CACHE_OK_TO_ADD|ADD_CACHE_OK_TO_REPLACE|
>  			  (intent_only ? ADD_CACHE_NEW_ONLY : 0));
>  	unsigned hash_flags = pretend ? 0 : INDEX_WRITE_OBJECT;
> -	struct object_id oid;
>  
>  	if (flags & ADD_CACHE_RENORMALIZE)
>  		hash_flags |= INDEX_RENORMALIZE;
> @@ -716,8 +715,6 @@ int add_to_index(struct index_state *istate, const char *path, struct stat *st,
>  
>  	namelen = strlen(path);
>  	if (S_ISDIR(st_mode)) {
> -		if (repo_resolve_gitlink_ref(the_repository, path, "HEAD", &oid) < 0)
> -			return error(_("'%s' does not have a commit checked out"), path);
>  		while (namelen && path[namelen-1] == '/')
>  			namelen--;
>  	}
> diff --git a/t/t3700-add.sh b/t/t3700-add.sh
> index b075eb9b11..d8cc0e4c66 100755
> --- a/t/t3700-add.sh
> +++ b/t/t3700-add.sh
> @@ -388,6 +388,7 @@ test_expect_success 'error on a repository with no commits' '
>  	test_must_fail git add empty >actual 2>&1 &&
>  	cat >expect <<-EOF &&
>  	error: '"'empty/'"' does not have a commit checked out
> +	error: unable to index file '"'empty/'"'
>  	fatal: adding files failed
>  	EOF
>  	test_cmp expect actual