Re: [RFC PATCH v6 0/2] diff-files: integrate with sparse index

[Victoria Dye <vdye@github.com>]

Shuqi Liang wrote:
> Did not fix the logic of spare-checkout yet. Leave the spare diff-files 
> with pathspec outside sparse definition as 'test_expect_failure' now.
> but will fix soon.
> 
> Changes since v5:
> 
> 1. Add space after "definition."
> 
> 2. Add test case for a pathspec with wildcards or other "magic"
> 
> 3. Before messing with modified files on disk, add a "baseline" 
> of correct behavior when a pathspec points to out-of-cone files.
> 
> 4. Write the contents of an
> existing file inside that sparse directory to disk manually
> 
> 5. Use 'test_all_match' rather than 'test_sparse_match'. 
> wouldn't need the additional 'test_must_be_empty' checks.
> 
> 
> Shuqi Liang (2):
>   t1092: add tests for `git diff-files`
>   diff-files: integrate with sparse index
> 
>  builtin/diff-files.c                     |  8 +++
>  t/perf/p2000-sparse-operations.sh        |  2 +
>  t/t1092-sparse-checkout-compatibility.sh | 73 ++++++++++++++++++++++++
>  3 files changed, 83 insertions(+)

[forgot to Reply-All - Shuqi, sorry for the duplicate email!]

In future iterations, please also include a range-diff in your version
iterations. The description of changes is useful, but the range-diff
provides a much more detailed and comprehensive summary of the changes
(making it exceptionally helpful for reviewers). I *think* you can just add
the '--range-diff <previous iteration>' option to 'git format-patch' (see
MyFirstContribution [1] for more detailed instructions).

[1] https://git-scm.com/docs/MyFirstContribution#v2-git-send-email

For anyone that's interested, here's the range-diff vs. v5:

1:  fb9ec0901c ! 1:  14bbcf41e0 t1092: add tests for `git diff-files`
    @@ Commit message
         the sparse-checkout cone.
     
         In order to have staged changes outside of
    -    the sparse-checkout cone, create a 'newdirectory/testfile' and
    -    add it to the index, while leaving it outside of
    -    the sparse-checkout definition.Test 'newdirectory/testfile'
    -    being present on-disk without modifications, then change content inside
    -    'newdirectory/testfile' in order to test 'newdirectory/testfile'
    -    being present on-disk with modifications.
    +    the sparse-checkout cone, make a directory called 'folder1' and
    +    copy `a` into 'folder1/a'. 'folder1/a' is identical to `a` in the base
    +    commit. These make 'folder1/a' in the index, while leaving it outside of
    +    the sparse-checkout definition. Test 'folder1/a'being present on-disk
    +    without modifications, then change content inside 'folder1/a' in order
    +    to test 'folder1/a' being present on-disk with modifications.
     
         Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
     
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'grep sparse direc
     +
     +	test_all_match git diff-files &&
     +
    -+	test_all_match git diff-files deep/a 
    ++	test_all_match git diff-files deep/a && 
     +
    ++	# test wildcard
    ++	test_all_match git diff-files deep/*
     +'
     +
    -+test_expect_success 'diff-files with pathspec outside sparse definition' '
    ++test_expect_failure 'diff-files with pathspec outside sparse definition' '
     +	init_repos &&
     +
    ++	test_sparse_match test_must_fail git diff-files folder2/a &&
    ++
     +	write_script edit-contents <<-\EOF &&
     +	echo text >>"$1"
     +	EOF
     +
    -+	# add file to the index but outside of cone
    -+	run_on_sparse mkdir newdirectory &&
    -+	run_on_sparse ../edit-contents newdirectory/testfile &&
    -+	test_sparse_match git add --sparse newdirectory/testfile &&
    ++	# Add file to the index but outside of cone for sparse-checkout cases.
    ++	# Add file to the index without sparse-checkout cases to ensure all have 
    ++	# same output.
    ++	run_on_all mkdir folder1 &&
    ++	run_on_all cp a folder1/a &&
     +
     +	# file present on-disk without modifications
    -+	test_sparse_match git diff-files &&
    -+	test_must_be_empty sparse-checkout-out &&
    -+	test_must_be_empty sparse-checkout-err &&
    -+	test_sparse_match git diff-files newdirectory/testfile &&
    -+	test_must_be_empty sparse-checkout-out &&
    -+	test_must_be_empty sparse-checkout-err &&
    ++	test_all_match git diff-files &&
    ++	test_all_match git diff-files folder1/a &&
     +
     +	# file present on-disk with modifications
    -+	FN=newdirectory/testfile &&
    -+	OID=$(git -C sparse-checkout hash-object $FN) &&
    -+	ZERO=$(test_oid zero) &&
    -+	echo ":100644 100644 $OID $ZERO M	$FN" >expect &&
    -+
    -+	run_on_sparse ../edit-contents newdirectory/testfile &&
    -+	test_sparse_match git diff-files &&
    -+	test_cmp expect sparse-checkout-out &&
    -+	test_sparse_match git diff-files newdirectory/testfile &&
    -+	test_cmp expect sparse-checkout-out
    ++	run_on_all ../edit-contents folder1/a &&
    ++	test_all_match git diff-files &&
    ++	test_all_match git diff-files folder1/a
     +'
     +
      test_done
2:  04e24f7db5 ! 2:  734cd24f0c diff-files: integrate with sparse index
    @@ Metadata
      ## Commit message ##
         diff-files: integrate with sparse index
     
    +    Originally, diff-files a pathspec that is out-of-cone in a sparse-index
    +    environment, Git dies with "pathspec '<x>' did not match any files",
    +    mainly because it does not expand the index so nothing is matched.
    +    Expand the index when the <pathspec> needs an expanded index, i.e. the
    +    <pathspec> contains wildcard that may need a full-index or the
    +    <pathspec> is simply outside of sparse-checkout definition.
    +
         Remove full index requirement for `git diff-files`
    -    and test to ensure the index is not expanded in `git diff-files`.
    +    and add test to ensure the index only expanded when necessary
    +    in `git diff-files`.
     
         The `p2000` tests demonstrate a ~96% execution time reduction for 'git
         diff-files' and a ~97% execution time reduction for 'git diff-files'
    @@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char
      	repo_init_revisions(the_repository, &rev, prefix);
      	rev.abbrev = 0;
      
    +@@ builtin/diff-files.c: int cmd_diff_files(int argc, const char **argv, const char *prefix)
    + 		result = -1;
    + 		goto cleanup;
    + 	}
    ++
    ++	if (pathspec_needs_expanded_index(the_repository->index, &rev.diffopt.pathspec))
    ++		ensure_full_index(the_repository->index);
    ++		
    + 	result = run_diff_files(&rev, options);
    + 	result = diff_result_code(&rev.diffopt, result);
    + cleanup:
     
      ## t/perf/p2000-sparse-operations.sh ##
     @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
    @@ t/perf/p2000-sparse-operations.sh: test_perf_on_all git checkout-index -f --all
      test_done
     
      ## t/t1092-sparse-checkout-compatibility.sh ##
    -@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with pathspec outside sparse definition' '
    - 	test_cmp expect sparse-checkout-out
    +@@ t/t1092-sparse-checkout-compatibility.sh: test_expect_failure 'diff-files with pathspec outside sparse definition' '
    + 	test_all_match git diff-files folder1/a
      '
      
    ++test_expect_success 'diff-files pathspec expands index when necessary' '
    ++	init_repos &&
    ++
    ++	write_script edit-contents <<-\EOF &&
    ++	echo text >>"$1"
    ++	EOF
    ++
    ++	run_on_all ../edit-contents deep/a &&
    ++	
    ++	# pathspec that should expand index
    ++	! ensure_not_expanded diff-files "*/a" &&
    ++	test_must_be_empty sparse-index-err &&
    ++
    ++	! ensure_not_expanded diff-files "**a" &&
    ++	test_must_be_empty sparse-index-err
    ++'
    ++
     +test_expect_success 'sparse index is not expanded: diff-files' '
     +	init_repos &&
     +
    @@ t/t1092-sparse-checkout-compatibility.sh: test_expect_success 'diff-files with p
     +
     +	run_on_all ../edit-contents deep/a &&
     +
    -+	ensure_not_expanded diff-files  &&
    -+	ensure_not_expanded diff-files deep/a 
    ++	ensure_not_expanded diff-files &&
    ++	ensure_not_expanded diff-files deep/a &&
    ++	ensure_not_expanded diff-files deep/*
     +'
     +
      test_done

> 
> 
> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1