[RFC PATCH 0/1] check-attr: integrate with sparse-index

[RFC PATCH 0/1] check-attr: integrate with sparse-index

[Shuqi Liang]

Integrate git check-attr with sparse-index.

Only add 'check-attr pathspec inside sparse definition' yet, just want to see 
if the added tests in this patch  on the right track.
Also, I have a question about git rev-parse. I read the documentation and wanted to add a test for it, 
but I noticed that Derrick already integrated it with sparse-index last year. 
Is there anything else I can do with git rev-parse to integrate with sparse-index, or should we just leave it be? 

Thanks!

Shuqi Liang (1):
  test check-attr pathspec inside sparse definition

 builtin/check-attr.c                     |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)


base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1
-- 
2.39.0

[RFC PATCH 1/1] check-attr: integrate with sparse-index

[Shuqi Liang]

Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>
---
 builtin/check-attr.c                     |  3 +++
 t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++
 2 files changed, 22 insertions(+)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 0fef10eb6b..f85b91ebba 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -112,6 +112,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
 
 	git_config(git_default_config, NULL);
 
+	prepare_repo_settings(the_repository);
+	the_repository->settings.command_requires_full_index = 0;
+	
 	argc = parse_options(argc, argv, prefix, check_attr_options,
 			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
 
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
index 801919009e..b28010aa5c 100755
--- a/t/t1092-sparse-checkout-compatibility.sh
+++ b/t/t1092-sparse-checkout-compatibility.sh
@@ -2055,4 +2055,23 @@ test_expect_success 'grep sparse directory within submodules' '
 	test_cmp actual expect
 '
 
+test_expect_success 'check-attr pathspec inside sparse definition' '
+	init_repos &&
+
+	run_on_all touch deep/test.c &&
+	echo "*.c diff=cpp -crlf myAttr" >>.gitattributes &&
+	run_on_all cp ../.gitattributes . &&
+	test_all_match git add .gitattributes &&
+	test_all_match git commit -m "add .gitattributes" &&
+	
+	run_on_all git reset --hard &&
+	test_all_match echo "deep/test.c" | git check-attr --stdin  -a &&
+
+	run_on_all git reset --hard &&
+	test_all_match git check-attr -a  deep/test.c &&
+
+	run_on_all git reset --hard &&
+	test_all_match git check-attr -a  --cached deep/test.c 
+'
+
 test_done
-- 
2.39.0

Re: [RFC PATCH 0/1] check-attr: integrate with sparse-index

[Victoria Dye]

Shuqi Liang wrote:
> Integrate git check-attr with sparse-index.

Since you're only looking for feedback on a test (one that theoretically
passes without any sparse index changes), the title should reflect that.
Something like "t1092: add a test for check-attr".

Also, the line wrapping of the following lines is a bit strange. Please make
sure to wrap to 72 characters, as it makes responding inline easier for
reviewers. For the sake of commenting here, I've rewrapped them.

> 
> Also, I have a question about git rev-parse. I read the documentation and
> wanted to add a test for it, but I noticed that Derrick already integrated
> it with sparse-index last year. Is there anything else I can do with git
> rev-parse to integrate with sparse-index, or should we just leave it be? 

Ah, sorry about that, I forgot to remove it. I don't think there's anything
else to do there, so we can leave it be.

I also think the list on the SoC Ideas 2023 [1] is somewhat out of order
(there are definitely some easier/almost trivial ones marked as "harder"
than some more involved ones) - I noticed last year, but never got around to
updating it. I'll try to submit a pull request to update the order and
remove 'rev-parse' sometime today or tomorrow. Sorry about that!

In the meantime, I think the easiest commands will probably be 'git
describe', 'git diff-files', and 'git diff-index' - feel free to switch to
one of those if you'd like. Of course, you're welcome to continue working on
'check-attr' - it'll be a little more complicated, but I don't think it'll
be as bad as, say, 'git mv' was (which needed substantial changes to its
sparse-checkout behavior before even starting the sparse index integration).

[1] https://git.github.io/SoC-2023-Ideas/

> 
> Thanks!
> 
> Shuqi Liang (1):
>   test check-attr pathspec inside sparse definition
> 
>  builtin/check-attr.c                     |  3 +++
>  t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> 
> base-commit: a38d39a4c50d1275833aba54c4dbdfce9e2e9ca1

Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

[Victoria Dye]

Shuqi Liang wrote:

Just a heads-up, I was CC'd only on the cover letter (and not this patch). 

> Signed-off-by: Shuqi Liang <cheskaqiqi@gmail.com>

Even in an RFC (or maybe *especially* in an RFC?), it's important to provide
some context around what you're doing in a patch/why you're doing it. It
seems like you provided that information in your cover letter [1], though,
so I think this "series" would be better off submitted as a single patch,
with the cover letter contents...

> ---

...right here! That is, below the '---' line but before the diff summary.

[1] https://lore.kernel.org/git/20230227050543.218294-1-cheskaqiqi@gmail.com/

>  builtin/check-attr.c                     |  3 +++
>  t/t1092-sparse-checkout-compatibility.sh | 19 +++++++++++++++++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/builtin/check-attr.c b/builtin/check-attr.c
> index 0fef10eb6b..f85b91ebba 100644
> --- a/builtin/check-attr.c
> +++ b/builtin/check-attr.c
> @@ -112,6 +112,9 @@ int cmd_check_attr(int argc, const char **argv, const char *prefix)
>  
>  	git_config(git_default_config, NULL);
>  
> +	prepare_repo_settings(the_repository);
> +	the_repository->settings.command_requires_full_index = 0;

The test below doesn't do anything special related to the sparse index, so
this change is unnecessary (and, as far as I can tell, will break in some
usage of 'git check-attr'). If you're only looking for feedback on testing,
it'd better to leave this out.

> +	
>  	argc = parse_options(argc, argv, prefix, check_attr_options,
>  			     check_attr_usage, PARSE_OPT_KEEP_DASHDASH);
>  
> diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh
> index 801919009e..b28010aa5c 100755
> --- a/t/t1092-sparse-checkout-compatibility.sh
> +++ b/t/t1092-sparse-checkout-compatibility.sh
> @@ -2055,4 +2055,23 @@ test_expect_success 'grep sparse directory within submodules' '
>  	test_cmp actual expect
>  '
>  
> +test_expect_success 'check-attr pathspec inside sparse definition' '
> +	init_repos &&
> +
> +	run_on_all touch deep/test.c &&
> +	echo "*.c diff=cpp -crlf myAttr" >>.gitattributes &&

Is there a specific reason you wanted to create a new file, rather than use
something in the existing structure (e.g. 'deep/a'?). If not, I'd recommend
using the existing file structure setup by the test in the 'setup' test at
the beginning of the file.

> +	run_on_all cp ../.gitattributes . &&
> +	test_all_match git add .gitattributes &&
> +	test_all_match git commit -m "add .gitattributes" &&
> +	
> +	run_on_all git reset --hard &&

Unless I'm missing something, there's nothing to 'reset' here? Same for the
other 'reset's you have below. If they're not needed, they should be
removed.

> +	test_all_match echo "deep/test.c" | git check-attr --stdin  -a &&

In addition to testing that all of them match, it would be helpful to see
*which* attributes are reported. The test 'ls-files' demonstrates one way of
doing that sort of test.

> +
> +	run_on_all git reset --hard &&
> +	test_all_match git check-attr -a  deep/test.c &&

Besides the things already noted in my earlier comments, these scenarios
seem reasonable.

> +
> +	run_on_all git reset --hard &&
> +	test_all_match git check-attr -a  --cached deep/test.c 

'deep/test.c' isn't in the index, so AFAICT this should return nothing.
While the case of an untracked file is interesting, I think that would be
*in addition to* a test on a file that exists in the index, e.g. 'deep/a'.

> +'> +
>  test_done

Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

[Derrick Stolee]

On 2/27/2023 12:18 PM, Victoria Dye wrote:
> Shuqi Liang wrote:

>> +	prepare_repo_settings(the_repository);
>> +	the_repository->settings.command_requires_full_index = 0;
> 
> The test below doesn't do anything special related to the sparse index, so
> this change is unnecessary (and, as far as I can tell, will break in some
> usage of 'git check-attr'). If you're only looking for feedback on testing,
> it'd better to leave this out.

This change is part of the performance improvements given by sparse index,
but the correctness test you've added only ensures that the end result is
correct, not fast.

It's possible that even with this change we hit an ensure_full_index() call
somewhere in the call stack. To test that the sparse-index stays sparse
throughout the process lifetime (when possible) create a test that uses the
ensure_not_expanded helper. There are several examples in t1092 to use as
a starting point.

Victoria is right that it is helpful to first establish test coverage of
the builtin for correctness reasons before making this change. It helps to
add tests for cases that would require expanding the sparse index, such as
checking the attributes for paths outside of the sparse-checkout cone.

Once the correctness tests are in place, you can then make this change to
the builtin and add the tests that check ensure_not_expanded, since the
change at that point is _only_ that we are allowing the builtin to
operate upon the sparse index without expanding it immediately after read.

A good example of this "final step" is [1], which updates the builtin for
'git diff' as well as adding _only_ the ensure_not_expanded tests.

[1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/
    [PATCH v6 6/7] diff: enable and test the sparse index

You can look at the surrounding patches for other ideas, but it should be
noted that both 'git diff' and 'git blame' in that series had previous
correctness tests in t1092 that only needed _edits_, instead of being
created from scratch.

Thanks,
-Stolee

Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

[Junio C Hamano]

Derrick Stolee <derrickstolee@github.com> writes:

> Victoria is right that it is helpful to first establish test coverage of
> the builtin for correctness reasons before making this change. It helps to
> add tests for cases that would require expanding the sparse index, such as
> checking the attributes for paths outside of the sparse-checkout cone.
>
> Once the correctness tests are in place, you can then make this change to
> the builtin and add the tests that check ensure_not_expanded, since the
> change at that point is _only_ that we are allowing the builtin to
> operate upon the sparse index without expanding it immediately after read.
>
> A good example of this "final step" is [1], which updates the builtin for
> 'git diff' as well as adding _only_ the ensure_not_expanded tests.

Thanks for a detailed help.

Re: [RFC PATCH 1/1] check-attr: integrate with sparse-index

[Shuqi Liang]

Hi  Victoria, Hi  Derrick

Thanks a lot for your valuable feedback!  They are really helpful!

These two days are very busy for me for preparing the midterm. After
1st  March, I'll start to address them in the next roll or start to
Investigation around 'git diff-files‘  as soon as possible .



-- 
Thanks & Regards,
Shuqi

On Mon, Feb 27, 2023 at 12:57 PM Derrick Stolee
<derrickstolee@github.com> wrote:
>
> On 2/27/2023 12:18 PM, Victoria Dye wrote:
> > Shuqi Liang wrote:
>
> >> +    prepare_repo_settings(the_repository);
> >> +    the_repository->settings.command_requires_full_index = 0;
> >
> > The test below doesn't do anything special related to the sparse index, so
> > this change is unnecessary (and, as far as I can tell, will break in some
> > usage of 'git check-attr'). If you're only looking for feedback on testing,
> > it'd better to leave this out.
>
> This change is part of the performance improvements given by sparse index,
> but the correctness test you've added only ensures that the end result is
> correct, not fast.
>
> It's possible that even with this change we hit an ensure_full_index() call
> somewhere in the call stack. To test that the sparse-index stays sparse
> throughout the process lifetime (when possible) create a test that uses the
> ensure_not_expanded helper. There are several examples in t1092 to use as
> a starting point.
>
> Victoria is right that it is helpful to first establish test coverage of
> the builtin for correctness reasons before making this change. It helps to
> add tests for cases that would require expanding the sparse index, such as
> checking the attributes for paths outside of the sparse-checkout cone.
>
> Once the correctness tests are in place, you can then make this change to
> the builtin and add the tests that check ensure_not_expanded, since the
> change at that point is _only_ that we are allowing the builtin to
> operate upon the sparse index without expanding it immediately after read.
>
> A good example of this "final step" is [1], which updates the builtin for
> 'git diff' as well as adding _only_ the ensure_not_expanded tests.
>
> [1] https://lore.kernel.org/git/897611682af64ba6bd0d2dfcfeae56cfe953c45e.1638806161.git.gitgitgadget@gmail.com/
>     [PATCH v6 6/7] diff: enable and test the sparse index
>
> You can look at the surrounding patches for other ideas, but it should be
> noted that both 'git diff' and 'git blame' in that series had previous
> correctness tests in t1092 that only needed _edits_, instead of being
> created from scratch.
>
> Thanks,
> -Stolee