git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Matheus Tavares Bernardino <matheus.bernardino@usp.br>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git <git@vger.kernel.org>, Elijah Newren <newren@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <stolee@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 11/13] mv: refuse to move sparse paths
Date: Fri, 27 Aug 2021 18:20:27 -0300	[thread overview]
Message-ID: <CAHd-oW5-f2Kb5DR-UTfu1qB1fm63oHf62WYsbGd5ajZueOWHtA@mail.gmail.com> (raw)
In-Reply-To: <d31c641180645ee4059aab9230841ad90f9244fe.1629842085.git.gitgitgadget@gmail.com>

On Tue, Aug 24, 2021 at 6:54 PM Derrick Stolee via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> diff --git a/builtin/mv.c b/builtin/mv.c
> index c2f96c8e895..b58fd4ce5ba 100644
> --- a/builtin/mv.c
> +++ b/builtin/mv.c
> @@ -176,10 +177,22 @@ int cmd_mv(int argc, const char **argv, const char *prefix)
>                 const char *src = source[i], *dst = destination[i];
>                 int length, src_is_dir;
>                 const char *bad = NULL;
> +               int skip_sparse = 0;
>
>                 if (show_only)
>                         printf(_("Checking rename of '%s' to '%s'\n"), src, dst);
>
> +               if (!path_in_sparse_checkout(src, &the_index)) {

`git mv` can only move/rename tracked paths, but since we check
whether `src` is sparse before checking if it is in the index, the
user will get the sparse error message instead. This is OK, but the
advice might be misleading, as it says they can use `--sparse` if they
really want to move the file, but repeating the command with
`--sparse` will now fail for another reason. I wonder if we should
check whether `src` is tracked before checking if it is sparse, or if
that is not really an issue we should bother with.

> +                       string_list_append(&only_match_skip_worktree, src);
> +                       skip_sparse = 1;
> +               }
> +               if (!path_in_sparse_checkout(dst, &the_index)) {
> +                       string_list_append(&only_match_skip_worktree, dst);
> +                       skip_sparse = 1;
> +               }
> +               if (skip_sparse)
> +                       continue;
> +
>                 length = strlen(src);
>                 if (lstat(src, &st) < 0)
>                         bad = _("bad source");
>
> diff --git a/t/t7002-mv-sparse-checkout.sh b/t/t7002-mv-sparse-checkout.sh
> new file mode 100755
> index 00000000000..5397c6d07bd
> --- /dev/null
> +++ b/t/t7002-mv-sparse-checkout.sh
> @@ -0,0 +1,99 @@
> +#!/bin/sh
> +
> +test_description='git mv in sparse working trees'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup' "
> +       mkdir -p sub/dir sub/dir2 &&
> +       touch a b c sub/d sub/dir/e sub/dir2/e &&
> +       git add -A &&
> +       git commit -m files &&
> +
> +       cat >sparse_error_header <<-EOF &&
> +       The following pathspecs didn't match any eligible path, but they do match index
> +       entries outside the current sparse checkout:
> +       EOF
> +
> +       cat >sparse_hint <<-EOF
> +       hint: Disable or modify the sparsity rules if you intend to update such entries.
> +       hint: Disable this message with \"git config advice.updateSparsePath false\"
> +       EOF
> +"
> +
> +test_expect_success 'mv refuses to move sparse-to-sparse' '
> +       rm -f e &&

At first glance, it confused me a bit that we are removing `e` when
the setup didn't create it. But then I realized the test itself might
create `e` if `git mv` succeeds in moving the `b` file. Could perhaps
this and the others `rm -f e` be a `test_when_finished`, to make it
clearer that it is a cleanup?

> +       git reset --hard &&
> +       git sparse-checkout set a &&
> +       touch b &&
> +       test_must_fail git mv b e 2>stderr &&

Here we try to move a "tracked sparse path" to an "untracked sparse
path". Do we also want to test with a tracked to tracked operation?
(Although the code path will be the same, of course.)

> +       cat sparse_error_header >expect &&
> +       echo b >>expect &&
> +       echo e >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'
> +
> +test_expect_success 'mv refuses to move sparse-to-sparse, ignores failure' '
> +       rm -f e &&
> +       git reset --hard &&
> +       git sparse-checkout set a &&
> +       touch b &&
> +       git mv -k b e 2>stderr &&

Maybe also check that `b` is still there, and `e` is missing?

> +       cat sparse_error_header >expect &&
> +       echo b >>expect &&
> +       echo e >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'
> +
> +test_expect_success 'mv refuses to move non-sparse-to-sparse' '
> +       rm -f e &&
> +       git reset --hard &&
> +       git sparse-checkout set a &&
> +       test_must_fail git mv a e 2>stderr &&
> +       cat sparse_error_header >expect &&
> +       echo e >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'

OK.

> +test_expect_success 'mv refuses to move sparse-to-non-sparse' '
> +       rm -f e &&
> +       git reset --hard &&
> +       git sparse-checkout set a e &&
> +       touch b &&
> +       test_must_fail git mv b e 2>stderr &&
> +       cat sparse_error_header >expect &&
> +       echo b >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'

OK.

> +test_expect_success 'recursive mv refuses to move (possible) sparse' '
> +       rm -f e &&
> +       git reset --hard &&
> +       # Without cone mode, "sub" and "sub2" do not match
> +       git sparse-checkout set sub/dir sub2/dir &&
> +       test_must_fail git mv sub sub2 2>stderr &&
> +       cat sparse_error_header >expect &&
> +       echo sub >>expect &&
> +       echo sub2 >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'
> +
> +test_expect_success 'recursive mv refuses to move sparse' '
> +       git reset --hard &&
> +       # Use cone mode so "sub/" matches the sparse-checkout patterns
> +       git sparse-checkout init --cone &&
> +       git sparse-checkout set sub/dir sub2/dir &&
> +       test_must_fail git mv sub sub2 2>stderr &&
> +       cat sparse_error_header >expect &&
> +       echo sub/dir2/e >>expect &&
> +       echo sub2/dir2/e >>expect &&
> +       cat sparse_hint >>expect &&
> +       test_cmp expect stderr
> +'
> +

Ah, these last two are very interesting cases!

  reply	other threads:[~2021-08-27 21:20 UTC|newest]

Thread overview: 116+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 21:54 [PATCH 00/13] [RFC] Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior Derrick Stolee via GitGitGadget
2021-08-24 21:54 ` [PATCH 01/13] t1092: behavior for adding sparse files Derrick Stolee via GitGitGadget
2021-08-24 21:54 ` [PATCH 02/13] dir: extract directory-matching logic Derrick Stolee via GitGitGadget
2021-08-24 21:54 ` [PATCH 03/13] dir: select directories correctly Derrick Stolee via GitGitGadget
2021-09-24  7:44   ` René Scharfe
2021-08-24 21:54 ` [PATCH 04/13] dir: fix pattern matching on dirs Derrick Stolee via GitGitGadget
2021-08-24 21:54 ` [PATCH 05/13] add: fail when adding an untracked sparse file Derrick Stolee via GitGitGadget
2021-08-27 21:06   ` Matheus Tavares Bernardino
2021-08-27 22:50     ` Matheus Tavares Bernardino
2021-09-08 17:54     ` Derrick Stolee
2021-08-24 21:54 ` [PATCH 06/13] add: skip paths that are outside sparse-checkout cone Derrick Stolee via GitGitGadget
2021-08-27 21:13   ` Matheus Tavares
2021-09-08 19:46     ` Derrick Stolee
2021-09-08 20:02       ` Derrick Stolee
2021-09-08 21:06     ` Derrick Stolee
2021-08-24 21:54 ` [PATCH 07/13] add: implement the --sparse option Derrick Stolee via GitGitGadget
2021-08-27 21:14   ` Matheus Tavares Bernardino
2021-08-24 21:54 ` [PATCH 08/13] add: prevent adding sparse conflict files Derrick Stolee via GitGitGadget
2021-08-27 21:16   ` Matheus Tavares Bernardino
2021-08-24 21:54 ` [PATCH 09/13] rm: add --sparse option Derrick Stolee via GitGitGadget
2021-08-27 21:17   ` Matheus Tavares Bernardino
2021-09-08 18:04     ` Derrick Stolee
2021-08-24 21:54 ` [PATCH 10/13] rm: skip sparse paths with missing SKIP_WORKTREE Derrick Stolee via GitGitGadget
2021-08-27 21:18   ` Matheus Tavares Bernardino
2021-08-24 21:54 ` [PATCH 11/13] mv: refuse to move sparse paths Derrick Stolee via GitGitGadget
2021-08-27 21:20   ` Matheus Tavares Bernardino [this message]
2021-08-27 23:44     ` Matheus Tavares Bernardino
2021-09-08 18:41     ` Derrick Stolee
2021-08-24 21:54 ` [PATCH 12/13] mv: add '--sparse' option to ignore sparse-checkout Derrick Stolee via GitGitGadget
2021-08-28 14:18   ` Matheus Tavares Bernardino
2021-08-24 21:54 ` [PATCH 13/13] advice: update message to suggest '--sparse' Derrick Stolee via GitGitGadget
2021-09-12 13:23 ` [PATCH v2 00/14] Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 01/14] t3705: test that 'sparse_entry' is unstaged Derrick Stolee via GitGitGadget
2021-09-15  5:22     ` Elijah Newren
2021-09-15 16:17       ` Derrick Stolee
2021-09-15 16:32     ` Matheus Tavares
2021-09-15 16:42       ` Derrick Stolee
2021-09-12 13:23   ` [PATCH v2 02/14] t1092: behavior for adding sparse files Derrick Stolee via GitGitGadget
2021-09-12 22:17     ` Ævar Arnfjörð Bjarmason
2021-09-13 15:02       ` Derrick Stolee
2021-09-12 13:23   ` [PATCH v2 03/14] dir: extract directory-matching logic Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 04/14] dir: select directories correctly Derrick Stolee via GitGitGadget
2021-09-12 22:21     ` Ævar Arnfjörð Bjarmason
2021-09-15 14:41       ` Derrick Stolee
2021-09-15 14:54     ` Elijah Newren
2021-09-15 16:43       ` Derrick Stolee
2021-09-12 13:23   ` [PATCH v2 05/14] dir: fix pattern matching on dirs Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 06/14] add: fail when adding an untracked sparse file Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 07/14] add: skip tracked paths outside sparse-checkout cone Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 08/14] add: implement the --sparse option Derrick Stolee via GitGitGadget
2021-09-15 16:59     ` Elijah Newren
2021-09-20 15:45       ` Derrick Stolee
2021-09-12 13:23   ` [PATCH v2 09/14] add: update --chmod to skip sparse paths Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 10/14] add: update --renormalize " Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 11/14] rm: add --sparse option Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 12/14] rm: skip sparse paths with missing SKIP_WORKTREE Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 13/14] mv: refuse to move sparse paths Derrick Stolee via GitGitGadget
2021-09-12 13:23   ` [PATCH v2 14/14] advice: update message to suggest '--sparse' Derrick Stolee via GitGitGadget
2021-09-12 21:58     ` Ævar Arnfjörð Bjarmason
2021-09-15 16:54       ` Derrick Stolee
2021-09-15 20:18   ` [PATCH v2 00/14] Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior Elijah Newren
2021-09-20 17:45   ` [PATCH v3 " Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 01/14] t3705: test that 'sparse_entry' is unstaged Derrick Stolee via GitGitGadget
2021-09-22 22:52       ` Junio C Hamano
2021-09-20 17:45     ` [PATCH v3 02/14] t1092: behavior for adding sparse files Derrick Stolee via GitGitGadget
2021-09-22 23:06       ` Junio C Hamano
2021-09-23 13:37         ` Derrick Stolee
2021-09-20 17:45     ` [PATCH v3 03/14] dir: extract directory-matching logic Derrick Stolee via GitGitGadget
2021-09-22 23:13       ` Junio C Hamano
2021-09-23 13:39         ` Derrick Stolee
2021-09-23 13:42           ` Derrick Stolee
2021-09-23 18:23             ` Junio C Hamano
2021-09-24 13:29               ` Derrick Stolee
2021-09-20 17:45     ` [PATCH v3 04/14] dir: select directories correctly Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 05/14] dir: fix pattern matching on dirs Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 06/14] add: fail when adding an untracked sparse file Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 07/14] add: skip tracked paths outside sparse-checkout cone Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 08/14] add: implement the --sparse option Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 09/14] add: update --chmod to skip sparse paths Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 10/14] add: update --renormalize " Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 11/14] rm: add --sparse option Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 12/14] rm: skip sparse paths with missing SKIP_WORKTREE Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 13/14] mv: refuse to move sparse paths Derrick Stolee via GitGitGadget
2021-09-20 17:45     ` [PATCH v3 14/14] advice: update message to suggest '--sparse' Derrick Stolee via GitGitGadget
2021-09-24  6:08     ` [PATCH v3 00/14] Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior Elijah Newren
2021-09-24 15:39     ` [PATCH v4 00/13] Sparse-checkout: modify 'git add', 'git rm', and 'git mv' behavior Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 01/13] t3705: test that 'sparse_entry' is unstaged Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 02/13] t1092: behavior for adding sparse files Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 03/13] dir: select directories correctly Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 04/13] dir: fix pattern matching on dirs Derrick Stolee via GitGitGadget
2021-11-02  0:15         ` Glen Choo
2021-11-02  0:34           ` Junio C Hamano
2021-11-02 13:42             ` Derrick Stolee
2021-11-02 14:50               ` Derrick Stolee
2021-11-02 15:33                 ` Ævar Arnfjörð Bjarmason
2021-11-03 14:40                   ` Derrick Stolee
2021-11-03 17:14                     ` Junio C Hamano
2021-09-24 15:39       ` [PATCH v4 05/13] add: fail when adding an untracked sparse file Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 06/13] add: skip tracked paths outside sparse-checkout cone Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 07/13] add: implement the --sparse option Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 08/13] add: update --chmod to skip sparse paths Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 09/13] add: update --renormalize " Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 10/13] rm: add --sparse option Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 11/13] rm: skip sparse paths with missing SKIP_WORKTREE Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 12/13] mv: refuse to move sparse paths Derrick Stolee via GitGitGadget
2021-09-24 15:39       ` [PATCH v4 13/13] advice: update message to suggest '--sparse' Derrick Stolee via GitGitGadget
2021-09-27 15:51       ` [PATCH v4 00/13] Sparse-checkout: modify 'git add', 'git rm', and 'git mv' behavior Elijah Newren
2021-09-27 20:51         ` Junio C Hamano
2021-10-18 21:28   ` [PATCH v2 00/14] Sparse-checkout: modify 'git add', 'git rm', and 'git add' behavior Sean Christopherson
2021-10-19 12:29     ` Derrick Stolee
2021-10-19 16:50       ` Sean Christopherson
2021-10-20 13:28         ` Junio C Hamano
2021-10-20 14:28           ` Sean Christopherson
2021-10-22  2:28     ` [RFC PATCH] add|rm|mv: fix bug that prevent the update of non-sparse Matheus Tavares
2021-10-22  4:03       ` Matheus Tavares
2021-10-25 16:40       ` Derrick Stolee

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAHd-oW5-f2Kb5DR-UTfu1qB1fm63oHf62WYsbGd5ajZueOWHtA@mail.gmail.com \
    --to=matheus.bernardino@usp.br \
    --cc=derrickstolee@github.com \
    --cc=dstolee@microsoft.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).