git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [RFC PATCH 3/7] t3705: add tests for `git add` in sparse checkouts

From
Junio C Hamano <gitster@pobox.com>
Date
Feb 17, 2021, 23:01 UTC
Message-ID
<xmqqpn0yi9ii.fsf@gitster.g>
In-Reply-To
<6e30f133e234ff1d3a29f36423cd3fdca58d8095.1613593946.git.matheus.bernardino@usp.br>
Matheus Tavares <matheus.bernardino@usp.br> writes:
Show 7 quoted lines
> We already have a couple tests for `add` with SKIP_WORKTREE entries in
> t7012, but these only cover the most basic scenarios. As we will be
> changing how `add` deals with sparse paths in the subsequent commits,
> let's move these two tests to their own file and add more test cases
> for different `add` options and situations. This also demonstrates two
> options that don't currently respect SKIP_WORKTREE entries: `--chmod`
> and `--renormalize`.
Nice.  It makes sense to describe what we want first, like this step..
Show 24 quoted lines
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>  t/t3705-add-sparse-checkout.sh   | 92 ++++++++++++++++++++++++++++++++
>  t/t7012-skip-worktree-writing.sh | 19 -------
>  2 files changed, 92 insertions(+), 19 deletions(-)
>  create mode 100755 t/t3705-add-sparse-checkout.sh
>
> diff --git a/t/t3705-add-sparse-checkout.sh b/t/t3705-add-sparse-checkout.sh
> new file mode 100755
> index 0000000000..5530e796b5
> --- /dev/null
> +++ b/t/t3705-add-sparse-checkout.sh
> @@ -0,0 +1,92 @@
> +#!/bin/sh
> +
> +test_description='git add in sparse checked out working trees'
> +
> +. ./test-lib.sh
> +
> +SPARSE_ENTRY_BLOB=""
> +
> +# Optionally take a string for the entry's contents
> +setup_sparse_entry()
> +{
Style.
	setup_sparse_entry () {
on a single line.
Show 5 quoted lines
> +	if test -f sparse_entry
> +	then
> +		rm sparse_entry
> +	fi &&
> +	git update-index --force-remove sparse_entry &&
Why not an unconditional removal on the working tree side?
	rm -f sparse_entry &&
	git update-index --force-remove sparse_entry &&
Are there cases where we may have sparse_entry directory here?
> +
> +	if test "$#" -eq 1
No need to quote $# (we know it is a number).
> +	then
> +		printf "$1" >sparse_entry

Make sure the test writers know that they are passing a string that will be interpreted as a printf format. Review the comment before the function and adjust it appropriately ("a string" is not what you want to tell them).

> +	else
> +		printf "" >sparse_entry
Just
		>sparse_entry
is sufficient, no?
Show 7 quoted lines
> +	fi &&
> +	git add sparse_entry &&
> +	git update-index --skip-worktree sparse_entry &&
> +	SPARSE_ENTRY_BLOB=$(git rev-parse :sparse_entry)
> +}
> +
> +test_sparse_entry_unchanged() {
Style.
	test_sparse_entry_unchanged () {
> +	echo "100644 $SPARSE_ENTRY_BLOB 0	sparse_entry" >expected &&
> +	git ls-files --stage sparse_entry >actual &&
> +	test_cmp expected actual

OK. So the expected pattern is to first "setup", do stuff that shouldn't affect the sparse entry in the index, and then call this to make sure?

> +}
> +test_expect_success "git add does not remove SKIP_WORKTREE entries" '

We use the term SKIP_WORKTREE and SPARSE interchangeably here. I wonder if it is easier to understand if we stick to one e.g. by saying "... does not remove 'sparse' entries" instead? I dunno.

> +	setup_sparse_entry &&
> +	rm sparse_entry &&
> +	git add sparse_entry &&
> +	test_sparse_entry_unchanged

Wow. OK. Makes a reader wonder what should happen when the two operations are replaced with "git rm sparse_entry"; let's read on.

Show 8 quoted lines
> +'
> +
> +test_expect_success "git add -A does not remove SKIP_WORKTREE entries" '
> +	setup_sparse_entry &&
> +	rm sparse_entry &&
> +	git add -A &&
> +	test_sparse_entry_unchanged
> +'

OK. As there is nothing other than sparse_entry in the working tree or in the index, the above two should be equivalent.

I wonder what should happen if the "add -A" gets replaced with "add ."; it should behave the same way, I think. Is it worth testing that case as well, or is it redundant?

Show 6 quoted lines
> +for opt in "" -f -u --ignore-removal
> +do
> +	if test -n "$opt"
> +	then
> +		opt=" $opt"
> +	fi

The above is cumulative, and as a consequence, "git add -u <path>" is not tested, but "git add -f -u <path>" is. Intended? How was the order of the options listed in "for opt in ..." chosen?

Show 20 quoted lines
> +	test_expect_success "git add$opt does not update SKIP_WORKTREE entries" '
> +		setup_sparse_entry &&
> +		echo modified >sparse_entry &&
> +		git add $opt sparse_entry &&
> +		test_sparse_entry_unchanged
> +	'
> +done
> +
> +test_expect_success 'git add --refresh does not update SKIP_WORKTREE entries' '
> +	setup_sparse_entry &&
> +	test-tool chmtime -60 sparse_entry &&
> +	git add --refresh sparse_entry &&
> +
> +	# We must unset the SKIP_WORKTREE bit, otherwise
> +	# git diff-files would skip examining the file
> +	git update-index --no-skip-worktree sparse_entry &&
> +
> +	echo sparse_entry >expected &&
> +	git diff-files --name-only sparse_entry >actual &&
> +	test_cmp actual expected

Hmph, I am not sure what we are testing at this point. One way to make the final diff-files step show sparse_entry would be for "git add --refresh" to be a no-op, in which case, the cached stat information in the index would be different in mtime from the path in the working tree. But "update-index --no-skip-worktree" may be buggy and further change or invalidate the cached stat information to cause diff-files to report that the path may be different.

Show 6 quoted lines
> +'
> +
> +test_expect_failure 'git add --chmod does not update SKIP_WORKTREE entries' '
> +	setup_sparse_entry &&
> +	git add --chmod=+x sparse_entry &&
> +	test_sparse_entry_unchanged

Hmph. Should we also check if sparse_entry in the filesystem also is not made executable, not just the entry in the index?

Show 8 quoted lines
> +'
> +
> +test_expect_failure 'git add --renormalize does not update SKIP_WORKTREE entries' '
> +	test_config core.autocrlf false &&
> +	setup_sparse_entry "LINEONE\r\nLINETWO\r\n" &&
> +	echo "sparse_entry text=auto" >.gitattributes &&
> +	git add --renormalize sparse_entry &&
> +	test_sparse_entry_unchanged
Makes sense.

What should "git diff sparse_entry" say at this point, I have to wonder?

> +'
> +
> +test_done
Thanks.
Previous: Matheus TavaresNext: Eric Sunshine
Message 19 of 42 in “rm: honor sparse checkout patterns”
  1. rm: honor sparse checkout patternsMatheus Tavares, Nov 12, 2020
  2. Elijah NewrenNov 12, 2020
  3. Derrick StoleeNov 13, 2020
  4. Matheus Tavares BernardinoNov 15, 2020
  5. Johannes SixtNov 15, 2020
  6. Matheus Tavares BernardinoNov 16, 2020
  7. Johannes SchindelinNov 23, 2020
  8. Matheus Tavares BernardinoNov 24, 2020
  9. Jeff HostetlerNov 16, 2020
  10. Elijah NewrenNov 17, 2020
  11. rm: honor sparse checkout patternsMatheus Tavares, Nov 16, 2020
  12. 0/7 add/rm: honor sparse checkout and warn on sparse pathsMatheus Tavares, Feb 17, 2021
  13. 1/7 add --chmod: don't update index when --dry-run is usedMatheus Tavares, Feb 17, 2021
  14. Junio C HamanoFeb 17, 2021
  15. Matheus TavaresFeb 18, 2021
  16. 2/7 add: include magic part of pathspec on --refresh errorMatheus Tavares, Feb 17, 2021
  17. Junio C HamanoFeb 17, 2021
  18. 3/7 t3705: add tests for `git add` in sparse checkoutsMatheus Tavares, Feb 17, 2021
  19. Junio C HamanoFeb 17, 2021
  20. Eric SunshineFeb 17, 2021
  21. Junio C HamanoFeb 17, 2021
  22. Matheus Tavares BernardinoFeb 18, 2021
  23. Matheus Tavares BernardinoFeb 18, 2021
  24. Matheus TavaresFeb 18, 2021
  25. Junio C HamanoFeb 18, 2021
  26. Junio C HamanoFeb 18, 2021
  27. Elijah NewrenFeb 22, 2021
  28. 4/7 add: make --chmod and --renormalize honor sparse checkoutsMatheus Tavares, Feb 17, 2021
  29. 5/7 pathspec: allow to ignore SKIP_WORKTREE entries on index matchingMatheus Tavares, Feb 17, 2021
  30. 6/7 add: warn when pathspec only matches SKIP_WORKTREE entriesMatheus Tavares, Feb 17, 2021
  31. Junio C HamanoFeb 19, 2021
  32. Matheus Tavares BernardinoFeb 19, 2021
  33. 7/7 rm: honor sparse checkout patternsMatheus Tavares, Feb 17, 2021
  34. Elijah NewrenFeb 22, 2021
  35. Junio C HamanoNov 16, 2020
  36. Elijah NewrenNov 17, 2020
  37. Elijah NewrenNov 20, 2020
  38. sparse-checkout questions and proposals [Was: Re: [PATCH] rm: honor sparse checkout patterns]Elijah Newren, Dec 31, 2020
  39. Derrick StoleeJan 4, 2021
  40. Elijah NewrenJan 6, 2021
  41. Derrick StoleeJan 7, 2021
  42. Elijah NewrenJan 7, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.