From: Junio C Hamano <gitster@pobox.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org, newren@gmail.com, stolee@gmail.com
Subject: Re: [RFC PATCH 3/7] t3705: add tests for `git add` in sparse checkouts
Date: Wed, 17 Feb 2021 15:01:41 -0800 [thread overview]
Message-ID: <xmqqpn0yi9ii.fsf@gitster.g> (raw)
In-Reply-To: <6e30f133e234ff1d3a29f36423cd3fdca58d8095.1613593946.git.matheus.bernardino@usp.br> (Matheus Tavares's message of "Wed, 17 Feb 2021 18:02:26 -0300")
Matheus Tavares <matheus.bernardino@usp.br> writes:
> 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..
> 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.
> + 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?
> + 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.
> +'
> +
> +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?
> +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?
> + 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.
> +'
> +
> +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?
> +'
> +
> +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.
next prev parent reply other threads:[~2021-02-17 23:02 UTC|newest]
Thread overview: 93+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 21:01 [PATCH] rm: honor sparse checkout patterns Matheus Tavares
2020-11-12 23:54 ` Elijah Newren
2020-11-13 13:47 ` Derrick Stolee
2020-11-15 20:12 ` Matheus Tavares Bernardino
2020-11-15 21:42 ` Johannes Sixt
2020-11-16 12:37 ` Matheus Tavares Bernardino
2020-11-23 13:23 ` Johannes Schindelin
2020-11-24 2:48 ` Matheus Tavares Bernardino
2020-11-16 14:30 ` Jeff Hostetler
2020-11-17 4:53 ` Elijah Newren
2020-11-16 13:58 ` [PATCH v2] " Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 0/7] add/rm: honor sparse checkout and warn on sparse paths Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 1/7] add --chmod: don't update index when --dry-run is used Matheus Tavares
2021-02-17 21:45 ` Junio C Hamano
2021-02-18 1:33 ` Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 2/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-02-17 22:20 ` Junio C Hamano
2021-02-17 21:02 ` [RFC PATCH 3/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-02-17 23:01 ` Junio C Hamano [this message]
2021-02-17 23:22 ` Eric Sunshine
2021-02-17 23:34 ` Junio C Hamano
2021-02-18 3:11 ` Matheus Tavares Bernardino
2021-02-18 3:07 ` Matheus Tavares Bernardino
2021-02-18 14:38 ` Matheus Tavares
2021-02-18 19:05 ` Junio C Hamano
2021-02-18 19:02 ` Junio C Hamano
2021-02-22 18:53 ` Elijah Newren
2021-02-17 21:02 ` [RFC PATCH 4/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 5/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-02-17 21:02 ` [RFC PATCH 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries Matheus Tavares
2021-02-19 0:34 ` Junio C Hamano
2021-02-19 17:11 ` Matheus Tavares Bernardino
2021-02-17 21:02 ` [RFC PATCH 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-02-22 18:57 ` [RFC PATCH 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-02-24 4:05 ` [PATCH v2 " Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-02-24 5:15 ` Elijah Newren
2021-02-24 4:05 ` [PATCH v2 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-02-24 5:23 ` Elijah Newren
2021-02-24 4:05 ` [PATCH v2 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag Matheus Tavares
2021-02-24 4:05 ` [PATCH v2 6/7] add: warn when pathspec only matches SKIP_WORKTREE entries Matheus Tavares
2021-02-24 6:50 ` Elijah Newren
2021-02-24 15:33 ` Matheus Tavares
2021-03-04 15:23 ` Matheus Tavares
2021-03-04 17:21 ` Elijah Newren
2021-03-04 21:03 ` Junio C Hamano
2021-03-04 22:48 ` Elijah Newren
2021-03-04 21:26 ` Matheus Tavares Bernardino
2021-02-24 4:05 ` [PATCH v2 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-02-24 6:59 ` Elijah Newren
2021-02-24 7:05 ` [PATCH v2 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-03-12 22:47 ` [PATCH v3 " Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-03-23 20:00 ` Derrick Stolee
2021-03-12 22:47 ` [PATCH v3 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-03-12 22:47 ` [PATCH v3 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-03-12 22:48 ` [PATCH v3 5/7] refresh_index(): add REFRESH_DONT_MARK_SPARSE_MATCHES flag Matheus Tavares
2021-03-18 23:45 ` Junio C Hamano
2021-03-19 0:00 ` Junio C Hamano
2021-03-19 12:23 ` Matheus Tavares Bernardino
2021-03-19 16:05 ` Junio C Hamano
2021-03-30 18:51 ` Matheus Tavares Bernardino
2021-03-31 9:14 ` Elijah Newren
2021-03-12 22:48 ` [PATCH v3 6/7] add: warn when asked to update SKIP_WORKTREE entries Matheus Tavares
2021-03-12 22:48 ` [PATCH v3 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-03-21 23:03 ` Ævar Arnfjörð Bjarmason
2021-03-22 1:08 ` Matheus Tavares Bernardino
2021-03-23 20:47 ` Derrick Stolee
2021-03-13 7:07 ` [PATCH v3 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-04-08 20:41 ` [PATCH v4 " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 1/7] add: include magic part of pathspec on --refresh error Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 2/7] t3705: add tests for `git add` in sparse checkouts Matheus Tavares
2021-04-14 16:39 ` Derrick Stolee
2021-04-08 20:41 ` [PATCH v4 3/7] add: make --chmod and --renormalize honor " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 4/7] pathspec: allow to ignore SKIP_WORKTREE entries on index matching Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 5/7] refresh_index(): add flag to ignore SKIP_WORKTREE entries Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 6/7] add: warn when asked to update " Matheus Tavares
2021-04-08 20:41 ` [PATCH v4 7/7] rm: honor sparse checkout patterns Matheus Tavares
2021-04-14 16:36 ` [PATCH v4 0/7] add/rm: honor sparse checkout and warn on sparse paths Elijah Newren
2021-04-14 18:04 ` Matheus Tavares Bernardino
2021-04-16 21:33 ` Junio C Hamano
2021-04-16 23:17 ` Elijah Newren
2020-11-16 20:14 ` [PATCH] rm: honor sparse checkout patterns Junio C Hamano
2020-11-17 5:20 ` Elijah Newren
2020-11-20 17:06 ` Elijah Newren
2020-12-31 20:03 ` sparse-checkout questions and proposals [Was: Re: [PATCH] rm: honor sparse checkout patterns] Elijah Newren
2021-01-04 3:02 ` Derrick Stolee
2021-01-06 19:15 ` Elijah Newren
2021-01-07 12:53 ` Derrick Stolee
2021-01-07 17:36 ` Elijah Newren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqpn0yi9ii.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=matheus.bernardino@usp.br \
--cc=newren@gmail.com \
--cc=stolee@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.