Re: [PATCH 3/4] object-name: reject trees found in the index

[Josh Steadmon <steadmon@google.com>]

On 2022.04.07 16:37, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> 
> The get_oid_with_context_1() method is used when parsing revision
> arguments. One particular case is to take a ":<path>" string and search
> the index for the given path.
> 
> In the case of a sparse index, this might find a sparse directory entry,
> in which case the contained object is a tree. In the case of a full
> index, this search within the index would fail.

Another case where my naive understanding of sparse-indexes caused a lot
of confusion for me. I don't know if we want to add reminders everywhere
in this series, but there should probably be at least one commit message
in the series that points out that sparse indexes are not subsets of
full indexes.


> In order to maintain identical return state as in a full index, inspect
> the discovered cache entry to see if it is a sparse directory and reject
> it. This requires being careful around the only_to_die option to be sure
> we die only at the correct time.
> 
> This changes the behavior of 'git show :<sparse-dir>', but does not
> bring it entirely into alignment with a full index case. It specifically
> hits the wrong error message within diagnose_invalid_index_path(). That
> error message will be corrected in a future change.
> 
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  object-name.c                            | 19 ++++++++++++++++++-
>  t/t1092-sparse-checkout-compatibility.sh | 11 ++---------
>  2 files changed, 20 insertions(+), 10 deletions(-)
> 
> diff --git a/object-name.c b/object-name.c
> index f0e327f91f5..2dc5d2549b8 100644
> --- a/object-name.c
> +++ b/object-name.c
> @@ -1881,6 +1881,20 @@ static char *resolve_relative_path(struct repository *r, const char *rel)
>  			   rel);
>  }
>  
> +static int reject_tree_in_index(struct repository *repo,
> +				int only_to_die,
> +				const struct cache_entry *ce,
> +				int stage,
> +				const char *prefix,
> +				const char *cp)
> +{
> +	if (!S_ISSPARSEDIR(ce->ce_mode))
> +		return 0;
> +	if (only_to_die)
> +		diagnose_invalid_index_path(repo, stage, prefix, cp);
> +	return -1;
> +}
> +
>  static enum get_oid_result get_oid_with_context_1(struct repository *repo,
>  				  const char *name,
>  				  unsigned flags,
> @@ -1955,9 +1969,12 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,
>  			    memcmp(ce->name, cp, namelen))
>  				break;
>  			if (ce_stage(ce) == stage) {
> +				free(new_path);
> +				if (reject_tree_in_index(repo, only_to_die, ce,
> +							 stage, prefix, cp))
> +					return -1;
>  				oidcpy(oid, &ce->oid);
>  				oc->mode = ce->ce_mode;
> -				free(new_path);
>  				return 0;
>  			}
>  			pos++;
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index f6a14e08b81..9d32361110d 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -1164,15 +1164,8 @@ test_expect_success 'show (cached blobs/trees)' '
>  	test_must_fail git -C full-checkout show :folder1/ &&
>  	test_must_fail git -C sparse-checkout show :folder1/ &&
>  
> -	git -C sparse-index show :folder1/ >actual &&
> -	git -C full-checkout show HEAD:folder1 >expect &&
> -
> -	# The output of "git show" includes the way we referenced the
> -	# objects, so strip that out.
> -	test_line_count = 4 actual &&
> -	tail -n 2 actual >actual-trunc &&
> -	tail -n 2 expect >expect-trunc &&
> -	test_cmp expect-trunc actual-trunc
> +	test_must_fail git -C sparse-index show :folder1/ 2>err &&
> +	grep "is in the index, but not at stage 0" err
>  '

It might be worth a note that we're demonstrating the current behavior
here, but this is not the desired end-state. In other words, explicitly
note that this is the "wrong error message" referred to in the commit
message.


>  
>  test_expect_success 'submodule handling' '
> -- 
> gitgitgadget
>