[v2,1/5] t1011: add testcase demonstrating accidental loss of user modifications

Commit Message

Elijah Newren Jan. 14, 2022, 3:59 p.m. UTC

From: Elijah Newren <[e-mail]>

If a user has a file with local modifications that is not marked as
SKIP_WORKTREE, but the sparsity patterns are such that it should be
marked that way, and the user then invokes a command like

   * git checkout -q HEAD^

or

   * git read-tree -mu HEAD^

Then the file will be deleted along with all the users' modifications.
Add a testcase demonstrating this problem.

Note: This bug only triggers if something other than 'HEAD' is given;
if the commands above had specified 'HEAD', then the users' file would
be left alone.

Signed-off-by: Elijah Newren <[e-mail]>
---
 t/t1011-read-tree-sparse-checkout.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Ævar Arnfjörð Bjarmason Feb. 16, 2022, 8:51 a.m. UTC | #1

On Fri, Jan 14 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
> [...]
> +	test_path_is_file init.t &&
> +	grep -q dirty init.t
> [...]
> +	test_path_is_file init.t &&
> +	grep -q dirty init.t

Maybe I'm missing something, but can these two just be:

    grep dirty init.t

I.e. won't grep report errors appropriately here, e.g.:
    
    $ grep foo t
    grep: t: Is a directory
    $ grep foo x
    grep: x: No such file or directory

The only prior art I could find was the same pattern in your c449947a79d
(directory rename detection: files/directories in the way of some
renames, 2018-04-19).

It's probably good to lose the "-q" too, unless this output is way too
verbose without it. In any case the errors wouldn't be affected.

Elijah Newren Feb. 16, 2022, 4:02 p.m. UTC | #2

On Wed, Feb 16, 2022 at 12:53 AM Ævar Arnfjörð Bjarmason
<[e-mail]> wrote:
>
> On Fri, Jan 14 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <[e-mail]>
> > [...]
> > +     test_path_is_file init.t &&
> > +     grep -q dirty init.t
> > [...]
> > +     test_path_is_file init.t &&
> > +     grep -q dirty init.t
>
> Maybe I'm missing something, but can these two just be:
>
>     grep dirty init.t
>
> I.e. won't grep report errors appropriately here, e.g.:
>
>     $ grep foo t
>     grep: t: Is a directory
>     $ grep foo x
>     grep: x: No such file or directory
>
> The only prior art I could find was the same pattern in your c449947a79d
> (directory rename detection: files/directories in the way of some
> renames, 2018-04-19).
>
> It's probably good to lose the "-q" too, unless this output is way too
> verbose without it. In any case the errors wouldn't be affected.

Fair point.  I believe my original test during development was just
test_path_is_file, then I came back later and decided I should check
the contents weren't lost.  A test_cmp might be a better choice to
replace both of these, because we don't really want to check that it
just has "dirty" somewhere in the file, but that it's been left alone
and only has those contents.

Patch

diff --git a/t/t1011-read-tree-sparse-checkout.sh b/t/t1011-read-tree-sparse-checkout.sh
index 24092c09a95..1b2395b8a82 100755
--- a/t/t1011-read-tree-sparse-checkout.sh
+++ b/t/t1011-read-tree-sparse-checkout.sh
@@ -187,6 +187,27 @@  test_expect_success 'read-tree updates worktree, absent case' '
 	test ! -f init.t
 '
 
+test_expect_success 'read-tree will not throw away dirty changes, non-sparse' '
+	echo "/*" >.git/info/sparse-checkout &&
+	read_tree_u_must_succeed -m -u HEAD &&
+
+	echo dirty >init.t &&
+	read_tree_u_must_fail -m -u HEAD^ &&
+	test_path_is_file init.t &&
+	grep -q dirty init.t
+'
+
+test_expect_failure 'read-tree will not throw away dirty changes, sparse' '
+	echo "/*" >.git/info/sparse-checkout &&
+	read_tree_u_must_succeed -m -u HEAD &&
+
+	echo dirty >init.t &&
+	echo sub/added >.git/info/sparse-checkout &&
+	read_tree_u_must_fail -m -u HEAD^ &&
+	test_path_is_file init.t &&
+	grep -q dirty init.t
+'
+
 test_expect_success 'read-tree updates worktree, dirty case' '
 	echo sub/added >.git/info/sparse-checkout &&
 	git checkout -f top &&