git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
* [PATCH] rm: honor sparse checkout patterns
@ 2020-11-12 21:01 Matheus Tavares
  2020-11-12 23:54 ` Elijah Newren
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Matheus Tavares @ 2020-11-12 21:01 UTC (permalink / raw)
  To: git; +Cc: stolee, newren

Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
operation to the paths that match both the command line pathspecs and
the repository's sparsity patterns. This better matches the expectations
of users with sparse-checkout definitions, while still allowing them
to optionally enable the old behavior with 'sparse.restrictCmds=false'
or the global '--no-restrict-to-sparse-paths' option.

Suggested-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

This is based on mt/grep-sparse-checkout.
Original feature request: https://github.com/gitgitgadget/git/issues/786

 Documentation/config/sparse.txt  |  3 ++-
 Documentation/git-rm.txt         |  9 +++++++++
 builtin/rm.c                     |  7 ++++++-
 t/t3600-rm.sh                    | 22 ++++++++++++++++++++++
 t/t7011-skip-worktree-reading.sh |  5 -----
 5 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt
index 494761526e..79d7d173e9 100644
--- a/Documentation/config/sparse.txt
+++ b/Documentation/config/sparse.txt
@@ -12,7 +12,8 @@ When this option is true (default), some git commands may limit their behavior
 to the paths specified by the sparsity patterns, or to the intersection of
 those paths and any (like `*.c`) that the user might also specify on the
 command line. When false, the affected commands will work on full trees,
-ignoring the sparsity patterns. For now, only git-grep honors this setting.
+ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
+setting.
 +
 Note: commands which export, integrity check, or create history will always
 operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.),
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index ab750367fd..25dda8ff35 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -25,6 +25,15 @@ When `--cached` is given, the staged content has to
 match either the tip of the branch or the file on disk,
 allowing the file to be removed from just the index.
 
+CONFIGURATION
+-------------
+
+sparse.restrictCmds::
+	By default, git-rm only matches and removes paths within the
+	sparse-checkout patterns. This behavior can be changed with the
+	`sparse.restrictCmds` setting or the global
+	`--no-restrict-to-sparse-paths` option. For more details, see the
+	full `sparse.restrictCmds` definition in linkgit:git-config[1].
 
 OPTIONS
 -------
diff --git a/builtin/rm.c b/builtin/rm.c
index 4858631e0f..e1fe71c321 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -14,6 +14,7 @@
 #include "string-list.h"
 #include "submodule.h"
 #include "pathspec.h"
+#include "sparse-checkout.h"
 
 static const char * const builtin_rm_usage[] = {
 	N_("git rm [<options>] [--] <file>..."),
@@ -254,7 +255,7 @@ static struct option builtin_rm_options[] = {
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	struct lock_file lock_file = LOCK_INIT;
-	int i;
+	int i, sparse_paths_only;
 	struct pathspec pathspec;
 	char *seen;
 
@@ -293,8 +294,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	seen = xcalloc(pathspec.nr, 1);
 
+	sparse_paths_only = restrict_to_sparse_paths(the_repository);
+
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
+		if (sparse_paths_only && ce_skip_worktree(ce))
+			continue;
 		if (!ce_path_match(&the_index, ce, &pathspec, seen))
 			continue;
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index efec8d13b6..7bf55b42eb 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -892,4 +892,26 @@ test_expect_success 'rm empty string should fail' '
 	test_must_fail git rm -rf ""
 '
 
+test_expect_success 'rm should respect --[no]-restrict-to-sparse-paths' '
+	git init sparse-repo &&
+	(
+		cd sparse-repo &&
+		touch a b c &&
+		git add -A &&
+		git commit -m files &&
+		git sparse-checkout set "/a" &&
+
+		# By default, it should not rm paths outside the sparse-checkout
+		test_must_fail git rm b 2>stderr &&
+		test_i18ngrep "fatal: pathspec .b. did not match any files" stderr &&
+
+		# But it should rm them with --no-restrict-to-sparse-paths
+		git --no-restrict-to-sparse-paths rm b &&
+
+		# And also with sparse.restrictCmds=false
+		git reset &&
+		git -c sparse.restrictCmds=false rm b
+	)
+'
+
 test_done
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 26852586ac..1761a2b1b9 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -132,11 +132,6 @@ test_expect_success 'diff-files does not examine skip-worktree dirty entries' '
 	test -z "$(git diff-files -- one)"
 '
 
-test_expect_success 'git-rm succeeds on skip-worktree absent entries' '
-	setup_absent &&
-	git rm 1
-'
-
 test_expect_success 'commit on skip-worktree absent entries' '
 	git reset &&
 	setup_absent &&
-- 
2.28.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  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-16 13:58 ` [PATCH v2] " Matheus Tavares
  2020-11-16 20:14 ` [PATCH] " Junio C Hamano
  2 siblings, 1 reply; 14+ messages in thread
From: Elijah Newren @ 2020-11-12 23:54 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: Git Mailing List, Derrick Stolee

Hi,

On Thu, Nov 12, 2020 at 1:02 PM Matheus Tavares
<matheus.bernardino@usp.br> wrote:
>
> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> operation to the paths that match both the command line pathspecs and
> the repository's sparsity patterns. This better matches the expectations
> of users with sparse-checkout definitions, while still allowing them
> to optionally enable the old behavior with 'sparse.restrictCmds=false'
> or the global '--no-restrict-to-sparse-paths' option.

(For Stolee:) Did this arise when a user specified a directory to
delete, and a (possibly small) part of that directory was in the
sparse checkout while other portions of it were outside?

I can easily see users thinking they are dealing with just the files
relevant to them, and expecting the directory deletion to only affect
that relevant subset, so this seems like a great idea.  We'd just want
to make sure we have a good error message if they explicitly list a
single path outside the sparse checkout.

> Suggested-by: Derrick Stolee <stolee@gmail.com>
> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
> ---
>
> This is based on mt/grep-sparse-checkout.
> Original feature request: https://github.com/gitgitgadget/git/issues/786
>
>  Documentation/config/sparse.txt  |  3 ++-
>  Documentation/git-rm.txt         |  9 +++++++++
>  builtin/rm.c                     |  7 ++++++-
>  t/t3600-rm.sh                    | 22 ++++++++++++++++++++++
>  t/t7011-skip-worktree-reading.sh |  5 -----
>  5 files changed, 39 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt
> index 494761526e..79d7d173e9 100644
> --- a/Documentation/config/sparse.txt
> +++ b/Documentation/config/sparse.txt
> @@ -12,7 +12,8 @@ When this option is true (default), some git commands may limit their behavior
>  to the paths specified by the sparsity patterns, or to the intersection of
>  those paths and any (like `*.c`) that the user might also specify on the
>  command line. When false, the affected commands will work on full trees,
> -ignoring the sparsity patterns. For now, only git-grep honors this setting.
> +ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
> +setting.
>  +
>  Note: commands which export, integrity check, or create history will always
>  operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.),
> diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
> index ab750367fd..25dda8ff35 100644
> --- a/Documentation/git-rm.txt
> +++ b/Documentation/git-rm.txt
> @@ -25,6 +25,15 @@ When `--cached` is given, the staged content has to
>  match either the tip of the branch or the file on disk,
>  allowing the file to be removed from just the index.
>
> +CONFIGURATION
> +-------------
> +
> +sparse.restrictCmds::
> +       By default, git-rm only matches and removes paths within the
> +       sparse-checkout patterns. This behavior can be changed with the
> +       `sparse.restrictCmds` setting or the global
> +       `--no-restrict-to-sparse-paths` option. For more details, see the
> +       full `sparse.restrictCmds` definition in linkgit:git-config[1].

Hmm, I wonder what people will think who are reading through the
manual and have never used sparse-checkout.  This seems prone to
confusion for them.  Maybe instead we could word this as:

When sparse-checkouts are in use, by default git-rm will only match
and remove paths within the sparse-checkout patterns...

>
>  OPTIONS
>  -------
> diff --git a/builtin/rm.c b/builtin/rm.c
> index 4858631e0f..e1fe71c321 100644
> --- a/builtin/rm.c
> +++ b/builtin/rm.c
> @@ -14,6 +14,7 @@
>  #include "string-list.h"
>  #include "submodule.h"
>  #include "pathspec.h"
> +#include "sparse-checkout.h"
>
>  static const char * const builtin_rm_usage[] = {
>         N_("git rm [<options>] [--] <file>..."),
> @@ -254,7 +255,7 @@ static struct option builtin_rm_options[] = {
>  int cmd_rm(int argc, const char **argv, const char *prefix)
>  {
>         struct lock_file lock_file = LOCK_INIT;
> -       int i;
> +       int i, sparse_paths_only;
>         struct pathspec pathspec;
>         char *seen;
>
> @@ -293,8 +294,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>
>         seen = xcalloc(pathspec.nr, 1);
>
> +       sparse_paths_only = restrict_to_sparse_paths(the_repository);
> +
>         for (i = 0; i < active_nr; i++) {
>                 const struct cache_entry *ce = active_cache[i];
> +               if (sparse_paths_only && ce_skip_worktree(ce))
> +                       continue;
>                 if (!ce_path_match(&the_index, ce, &pathspec, seen))
>                         continue;
>                 ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
> diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
> index efec8d13b6..7bf55b42eb 100755
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -892,4 +892,26 @@ test_expect_success 'rm empty string should fail' '
>         test_must_fail git rm -rf ""
>  '
>
> +test_expect_success 'rm should respect --[no]-restrict-to-sparse-paths' '
> +       git init sparse-repo &&
> +       (
> +               cd sparse-repo &&
> +               touch a b c &&
> +               git add -A &&
> +               git commit -m files &&
> +               git sparse-checkout set "/a" &&
> +
> +               # By default, it should not rm paths outside the sparse-checkout
> +               test_must_fail git rm b 2>stderr &&
> +               test_i18ngrep "fatal: pathspec .b. did not match any files" stderr &&

Ah, this answers my question about whether the user gets an error
message when they explicitly call out a single path outside the sparse
checkout.  I'm curious if we want to be slightly more verbose on the
error message when sparse-checkouts are in effect.  In particular, if
no paths match the sparsity patterns, but some paths would have
matched the pathspec ignoring the sparsity patterns, then perhaps the
error message should include a reference to the
--no-restrict-to-sparse-paths flag.

> +
> +               # But it should rm them with --no-restrict-to-sparse-paths
> +               git --no-restrict-to-sparse-paths rm b &&
> +
> +               # And also with sparse.restrictCmds=false
> +               git reset &&
> +               git -c sparse.restrictCmds=false rm b
> +       )
> +'
> +
>  test_done

Do we also want to include a testcase where the user specifies a
directory and part of that directory is within the sparsity paths and
part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
sub' ?

> diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
> index 26852586ac..1761a2b1b9 100755
> --- a/t/t7011-skip-worktree-reading.sh
> +++ b/t/t7011-skip-worktree-reading.sh
> @@ -132,11 +132,6 @@ test_expect_success 'diff-files does not examine skip-worktree dirty entries' '
>         test -z "$(git diff-files -- one)"
>  '
>
> -test_expect_success 'git-rm succeeds on skip-worktree absent entries' '
> -       setup_absent &&
> -       git rm 1
> -'
> -
>  test_expect_success 'commit on skip-worktree absent entries' '
>         git reset &&
>         setup_absent &&
> --
> 2.28.0

Sweet, nice and simple.  Thanks for sending this in; I think it'll be very nice.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-12 23:54 ` Elijah Newren
@ 2020-11-13 13:47   ` Derrick Stolee
  2020-11-15 20:12     ` Matheus Tavares Bernardino
  2020-11-16 14:30     ` Jeff Hostetler
  0 siblings, 2 replies; 14+ messages in thread
From: Derrick Stolee @ 2020-11-13 13:47 UTC (permalink / raw)
  To: Elijah Newren, Matheus Tavares; +Cc: Git Mailing List

On 11/12/2020 6:54 PM, Elijah Newren wrote:
> Hi,
> 
> On Thu, Nov 12, 2020 at 1:02 PM Matheus Tavares
> <matheus.bernardino@usp.br> wrote:
>>
>> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
>> operation to the paths that match both the command line pathspecs and
>> the repository's sparsity patterns. This better matches the expectations
>> of users with sparse-checkout definitions, while still allowing them
>> to optionally enable the old behavior with 'sparse.restrictCmds=false'
>> or the global '--no-restrict-to-sparse-paths' option.
> 
> (For Stolee:) Did this arise when a user specified a directory to
> delete, and a (possibly small) part of that directory was in the
> sparse checkout while other portions of it were outside?

The user who suggested this used a command like 'git rm */*.csprojx' to
remove all paths with that file extension, but then realized that they
were deleting all of those files from the entire repo, not just the
current sparse-checkout.

> I can easily see users thinking they are dealing with just the files
> relevant to them, and expecting the directory deletion to only affect
> that relevant subset, so this seems like a great idea.  We'd just want
> to make sure we have a good error message if they explicitly list a
> single path outside the sparse checkout.

We should definitely consider how to make this more usable for users
who operate within a sparse-checkout but try to modify files outside
the sparse-checkout.

Is there a warning message such as "the supplied pathspec doesn't
match any known file" that we could extend to recommend possibly
disabling the sparse.restrictCmds config? (I see that you identify
one below.)

>> +CONFIGURATION
>> +-------------
>> +
>> +sparse.restrictCmds::
>> +       By default, git-rm only matches and removes paths within the
>> +       sparse-checkout patterns. This behavior can be changed with the
>> +       `sparse.restrictCmds` setting or the global
>> +       `--no-restrict-to-sparse-paths` option. For more details, see the
>> +       full `sparse.restrictCmds` definition in linkgit:git-config[1].
> 
> Hmm, I wonder what people will think who are reading through the
> manual and have never used sparse-checkout.  This seems prone to
> confusion for them.  Maybe instead we could word this as:
> 
> When sparse-checkouts are in use, by default git-rm will only match
> and remove paths within the sparse-checkout patterns...

A preface such as "When using sparse-checkouts..." can help users
ignore these config settings if they are unfamiliar with the
concept.
>> @@ -293,8 +294,12 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>
>>         seen = xcalloc(pathspec.nr, 1);
>>
>> +       sparse_paths_only = restrict_to_sparse_paths(the_repository);
>> +
>>         for (i = 0; i < active_nr; i++) {
>>                 const struct cache_entry *ce = active_cache[i];
>> +               if (sparse_paths_only && ce_skip_worktree(ce))
>> +                       continue;
>>                 if (!ce_path_match(&the_index, ce, &pathspec, seen))
>>                         continue;
>>                 ALLOC_GROW(list.entry, list.nr + 1, list.alloc);

This seems like an incredibly simple implementation! Excellent.

>> +test_expect_success 'rm should respect --[no]-restrict-to-sparse-paths' '
>> +       git init sparse-repo &&
>> +       (
>> +               cd sparse-repo &&
>> +               touch a b c &&
>> +               git add -A &&
>> +               git commit -m files &&
>> +               git sparse-checkout set "/a" &&
>> +
>> +               # By default, it should not rm paths outside the sparse-checkout
>> +               test_must_fail git rm b 2>stderr &&
>> +               test_i18ngrep "fatal: pathspec .b. did not match any files" stderr &&
> 
> Ah, this answers my question about whether the user gets an error
> message when they explicitly call out a single path outside the sparse
> checkout.  I'm curious if we want to be slightly more verbose on the
> error message when sparse-checkouts are in effect.  In particular, if
> no paths match the sparsity patterns, but some paths would have
> matched the pathspec ignoring the sparsity patterns, then perhaps the
> error message should include a reference to the
> --no-restrict-to-sparse-paths flag.

The error message could be modified similar to below:

if (!seen[i]) {
	if (!ignore_unmatch) {
		die(_("pathspec '%s' did not match any files%s"),
			original,
			sparse_paths_only
				? _("; disable sparse.restrictCmds if you intend to edit outside the current sparse-checkout definition")
				: "");
	}
}

>> +
>> +               # But it should rm them with --no-restrict-to-sparse-paths
>> +               git --no-restrict-to-sparse-paths rm b &&
>> +
>> +               # And also with sparse.restrictCmds=false
>> +               git reset &&
>> +               git -c sparse.restrictCmds=false rm b
>> +       )
>> +'
>> +
>>  test_done
> 
> Do we also want to include a testcase where the user specifies a
> directory and part of that directory is within the sparsity paths and
> part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
> sub' ?

That is definitely an interesting case. I'm not sure the current
implementation will do the "right" thing here. Definitely worth
testing, and it might require a more complicated implementation.

>> diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
>> index 26852586ac..1761a2b1b9 100755
>> --- a/t/t7011-skip-worktree-reading.sh
>> +++ b/t/t7011-skip-worktree-reading.sh
>> @@ -132,11 +132,6 @@ test_expect_success 'diff-files does not examine skip-worktree dirty entries' '
>>         test -z "$(git diff-files -- one)"
>>  '
>>
>> -test_expect_success 'git-rm succeeds on skip-worktree absent entries' '
>> -       setup_absent &&
>> -       git rm 1
>> -'
>> -

Instead of deleting this case, perhaps we should just use "-c sparse.restrictCmds=false"
in the 'git rm' command, so we are still testing this case?

Thanks again! I appreciate that you jumped on this suggestion.

-Stolee


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  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 14:30     ` Jeff Hostetler
  1 sibling, 1 reply; 14+ messages in thread
From: Matheus Tavares Bernardino @ 2020-11-15 20:12 UTC (permalink / raw)
  To: Derrick Stolee; +Cc: Elijah Newren, Git Mailing List

Hi, Stolee and Elijah

Thank you both for the comments. I'll try to send v2 soon.

On Fri, Nov 13, 2020 at 10:47 AM Derrick Stolee <stolee@gmail.com> wrote:
>
> On 11/12/2020 6:54 PM, Elijah Newren wrote:
> >
> > Do we also want to include a testcase where the user specifies a
> > directory and part of that directory is within the sparsity paths and
> > part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
> > sub' ?
>
> That is definitely an interesting case.

I've added the test [1], but it's failing on Windows and I'm not quite
sure why. The trash dir artifact shows that `git sparse-checkout set
/sub/dir` produced the following path on the sparse-checkout file:
"D:/a/git/git/git-sdk-64-minimal/sub/dir".

If I change the setup cmd to `git sparse-checkout set sub/dir` (i.e.
without the root slash), it works as expected. Could this be a bug, or
am I missing something?

[1]: https://github.com/matheustavares/git/commit/656bffa1793ce86b638d7ad1da2452103ce8b14b#diff-69312bb98fb0cf46e6906e3384c11529f3f04713d331a85d67fc77a3e43944f9R919

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-15 20:12     ` Matheus Tavares Bernardino
@ 2020-11-15 21:42       ` Johannes Sixt
  2020-11-16 12:37         ` Matheus Tavares Bernardino
  0 siblings, 1 reply; 14+ messages in thread
From: Johannes Sixt @ 2020-11-15 21:42 UTC (permalink / raw)
  To: Matheus Tavares Bernardino, Derrick Stolee
  Cc: Elijah Newren, Git Mailing List

Am 15.11.20 um 21:12 schrieb Matheus Tavares Bernardino:
> Thank you both for the comments. I'll try to send v2 soon.
> 
> On Fri, Nov 13, 2020 at 10:47 AM Derrick Stolee <stolee@gmail.com> wrote:
>>
>> On 11/12/2020 6:54 PM, Elijah Newren wrote:
>>>
>>> Do we also want to include a testcase where the user specifies a
>>> directory and part of that directory is within the sparsity paths and
>>> part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
>>> sub' ?
>>
>> That is definitely an interesting case.
> 
> I've added the test [1], but it's failing on Windows and I'm not quite
> sure why. The trash dir artifact shows that `git sparse-checkout set
> /sub/dir` produced the following path on the sparse-checkout file:
> "D:/a/git/git/git-sdk-64-minimal/sub/dir".

If 'git sparse-checkout' is run from a bash command line, I would not be 
surprised if the absolute path is munched in the way that you observe, 
provided that D:/a/git/git/git-sdk-64-minimal is where your MinGW 
subsystem is located. I that the case?

> If I change the setup cmd to `git sparse-checkout set sub/dir` (i.e.
> without the root slash), it works as expected. Could this be a bug, or
> am I missing something?

Not a bug, unless tell us that D:/a/git/git/git-sdk-64-minimal is a 
completely random path in your system or does not even exist.

-- Hannes

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-15 21:42       ` Johannes Sixt
@ 2020-11-16 12:37         ` Matheus Tavares Bernardino
  2020-11-23 13:23           ` Johannes Schindelin
  0 siblings, 1 reply; 14+ messages in thread
From: Matheus Tavares Bernardino @ 2020-11-16 12:37 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Derrick Stolee, Elijah Newren, Git Mailing List

On Sun, Nov 15, 2020 at 6:42 PM Johannes Sixt <j6t@kdbg.org> wrote:
>
> Am 15.11.20 um 21:12 schrieb Matheus Tavares Bernardino:
> > Thank you both for the comments. I'll try to send v2 soon.
> >
> > On Fri, Nov 13, 2020 at 10:47 AM Derrick Stolee <stolee@gmail.com> wrote:
> >>
> >> On 11/12/2020 6:54 PM, Elijah Newren wrote:
> >>>
> >>> Do we also want to include a testcase where the user specifies a
> >>> directory and part of that directory is within the sparsity paths and
> >>> part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
> >>> sub' ?
> >>
> >> That is definitely an interesting case.
> >
> > I've added the test [1], but it's failing on Windows and I'm not quite
> > sure why. The trash dir artifact shows that `git sparse-checkout set
> > /sub/dir` produced the following path on the sparse-checkout file:
> > "D:/a/git/git/git-sdk-64-minimal/sub/dir".
>
> If 'git sparse-checkout' is run from a bash command line, I would not be
> surprised if the absolute path is munched in the way that you observe,
> provided that D:/a/git/git/git-sdk-64-minimal is where your MinGW
> subsystem is located. I that the case?

Yeah, that must be it, thanks. I didn't run the command myself as I'm
not on Windows, but D:/a/git/git/git-sdk-64-minimal must be the path
where MinGW was installed by our GitHub Actions script, then. I'll use
"sub/dir" without the root slash in t3600 to avoid the conversion.
Thanks again!

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2] rm: honor sparse checkout patterns
  2020-11-12 21:01 [PATCH] rm: honor sparse checkout patterns Matheus Tavares
  2020-11-12 23:54 ` Elijah Newren
@ 2020-11-16 13:58 ` Matheus Tavares
  2020-11-16 20:14 ` [PATCH] " Junio C Hamano
  2 siblings, 0 replies; 14+ messages in thread
From: Matheus Tavares @ 2020-11-16 13:58 UTC (permalink / raw)
  To: git; +Cc: stolee, newren

Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
operation to the paths that match both the command line pathspecs and
the repository's sparsity patterns. This better matches the expectations
of users with sparse-checkout definitions, while still allowing them
to optionally enable the old behavior with 'sparse.restrictCmds=false'
or the global '--no-restrict-to-sparse-paths' option.

Suggested-by: Derrick Stolee <stolee@gmail.com>
Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>
---

Based on mt/grep-sparse-checkout.

Changes since v1:
- Reworded git-rm docs to avoid confusion for those who never used
  sparse-checkout.
- Included an advice about disabling sparse.restrictCmds when the
  given pathspec doesn't match any files and sparse-checkout is enabled.
- Added test for `git rm -r` removing a dir that is only partially
  included in the sparse-checkout.
- Adjusted test in t7011 to use `-c sparse.restrictCmds=false`, instead
  of removing it.

 Documentation/config/sparse.txt  |  3 +-
 Documentation/git-rm.txt         | 10 +++++++
 builtin/rm.c                     | 24 ++++++++++------
 t/t3600-rm.sh                    | 47 ++++++++++++++++++++++++++++++++
 t/t7011-skip-worktree-reading.sh |  4 +--
 5 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/Documentation/config/sparse.txt b/Documentation/config/sparse.txt
index 494761526e..79d7d173e9 100644
--- a/Documentation/config/sparse.txt
+++ b/Documentation/config/sparse.txt
@@ -12,7 +12,8 @@ When this option is true (default), some git commands may limit their behavior
 to the paths specified by the sparsity patterns, or to the intersection of
 those paths and any (like `*.c`) that the user might also specify on the
 command line. When false, the affected commands will work on full trees,
-ignoring the sparsity patterns. For now, only git-grep honors this setting.
+ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
+setting.
 +
 Note: commands which export, integrity check, or create history will always
 operate on full trees (e.g. fast-export, format-patch, fsck, commit, etc.),
diff --git a/Documentation/git-rm.txt b/Documentation/git-rm.txt
index ab750367fd..33bec8c249 100644
--- a/Documentation/git-rm.txt
+++ b/Documentation/git-rm.txt
@@ -25,6 +25,16 @@ When `--cached` is given, the staged content has to
 match either the tip of the branch or the file on disk,
 allowing the file to be removed from just the index.
 
+CONFIGURATION
+-------------
+
+sparse.restrictCmds::
+	When sparse-checkouts are in use, by default git-rm will only
+	match and remove paths within the sparse-checkout patterns.
+	This behavior can be changed with the `sparse.restrictCmds`
+	setting or the global `--no-restrict-to-sparse-paths` option.
+	For more details, see the full `sparse.restrictCmds` definition
+	in linkgit:git-config[1].
 
 OPTIONS
 -------
diff --git a/builtin/rm.c b/builtin/rm.c
index 4858631e0f..90f6bb4cae 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -14,6 +14,7 @@
 #include "string-list.h"
 #include "submodule.h"
 #include "pathspec.h"
+#include "sparse-checkout.h"
 
 static const char * const builtin_rm_usage[] = {
 	N_("git rm [<options>] [--] <file>..."),
@@ -254,7 +255,7 @@ static struct option builtin_rm_options[] = {
 int cmd_rm(int argc, const char **argv, const char *prefix)
 {
 	struct lock_file lock_file = LOCK_INIT;
-	int i;
+	int i, sparse_paths_only;
 	struct pathspec pathspec;
 	char *seen;
 
@@ -293,8 +294,13 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 
 	seen = xcalloc(pathspec.nr, 1);
 
+	sparse_paths_only = core_apply_sparse_checkout &&
+			    restrict_to_sparse_paths(the_repository);
+
 	for (i = 0; i < active_nr; i++) {
 		const struct cache_entry *ce = active_cache[i];
+		if (sparse_paths_only && ce_skip_worktree(ce))
+			continue;
 		if (!ce_path_match(&the_index, ce, &pathspec, seen))
 			continue;
 		ALLOC_GROW(list.entry, list.nr + 1, list.alloc);
@@ -310,14 +316,16 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
 		int seen_any = 0;
 		for (i = 0; i < pathspec.nr; i++) {
 			original = pathspec.items[i].original;
-			if (!seen[i]) {
-				if (!ignore_unmatch) {
-					die(_("pathspec '%s' did not match any files"),
-					    original);
-				}
-			}
-			else {
+			if (seen[i]) {
 				seen_any = 1;
+			} else if (!ignore_unmatch) {
+				const char *sparse_config_advice =
+					_("; disable sparse.restrictCmds if you intend to edit"
+					  " outside the current sparse-checkout definition");
+
+				die(_("pathspec '%s' did not match any files%s"),
+				    original,
+				    sparse_paths_only ? sparse_config_advice : "");
 			}
 			if (!recursive && seen[i] == MATCHED_RECURSIVELY)
 				die(_("not removing '%s' recursively without -r"),
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index efec8d13b6..25cd7187fa 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -892,4 +892,51 @@ test_expect_success 'rm empty string should fail' '
 	test_must_fail git rm -rf ""
 '
 
+test_expect_success 'rm should respect --[no]-restrict-to-sparse-paths' '
+	git init sparse-repo &&
+	(
+		cd sparse-repo &&
+		touch a b c &&
+		git add -A &&
+		git commit -m files &&
+		git sparse-checkout set "/a" &&
+
+		# By default, it should not rm paths outside the sparse-checkout
+		test_must_fail git rm b 2>stderr &&
+		test_i18ngrep "fatal: pathspec .b. did not match any files" stderr &&
+		test_i18ngrep "disable sparse.restrictCmds if you intend to edit outside" stderr &&
+
+		# But it should rm them with --no-restrict-to-sparse-paths
+		git --no-restrict-to-sparse-paths rm b &&
+
+		# And also with sparse.restrictCmds=false
+		git reset &&
+		git -c sparse.restrictCmds=false rm b
+	)
+'
+
+test_expect_success 'recursive rm should respect --[no]-restrict-to-sparse-paths' '
+	git init sparse-repo-2 &&
+	(
+		cd sparse-repo-2 &&
+		mkdir -p sub/dir &&
+		touch sub/f1 sub/dir/f2 &&
+		git add -A &&
+		git commit -m files &&
+		git sparse-checkout set "sub/dir" &&
+
+		git rm -r sub &&
+		echo "D  sub/dir/f2" >expected &&
+		git status --porcelain -uno >actual &&
+		test_cmp expected actual &&
+
+		git reset &&
+		git --no-restrict-to-sparse-paths rm -r sub &&
+		echo "D  sub/dir/f2" >expected-no-restrict &&
+		echo "D  sub/f1"     >>expected-no-restrict &&
+		git status --porcelain -uno >actual-no-restrict &&
+		test_cmp expected-no-restrict actual-no-restrict
+	)
+'
+
 test_done
diff --git a/t/t7011-skip-worktree-reading.sh b/t/t7011-skip-worktree-reading.sh
index 26852586ac..08ede90e14 100755
--- a/t/t7011-skip-worktree-reading.sh
+++ b/t/t7011-skip-worktree-reading.sh
@@ -132,9 +132,9 @@ test_expect_success 'diff-files does not examine skip-worktree dirty entries' '
 	test -z "$(git diff-files -- one)"
 '
 
-test_expect_success 'git-rm succeeds on skip-worktree absent entries' '
+test_expect_success 'git-rm succeeds on skip-worktree absent entries when sparse.restrictCmds=false' '
 	setup_absent &&
-	git rm 1
+	git -c sparse.restrictCmds=false rm 1
 '
 
 test_expect_success 'commit on skip-worktree absent entries' '
-- 
2.28.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-13 13:47   ` Derrick Stolee
  2020-11-15 20:12     ` Matheus Tavares Bernardino
@ 2020-11-16 14:30     ` Jeff Hostetler
  2020-11-17  4:53       ` Elijah Newren
  1 sibling, 1 reply; 14+ messages in thread
From: Jeff Hostetler @ 2020-11-16 14:30 UTC (permalink / raw)
  To: Derrick Stolee, Elijah Newren, Matheus Tavares; +Cc: Git Mailing List



On 11/13/20 8:47 AM, Derrick Stolee wrote:
> On 11/12/2020 6:54 PM, Elijah Newren wrote:
>> Hi,
>>
>> On Thu, Nov 12, 2020 at 1:02 PM Matheus Tavares
>> <matheus.bernardino@usp.br> wrote:
>>>
>>> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
>>> operation to the paths that match both the command line pathspecs and
>>> the repository's sparsity patterns. This better matches the expectations
>>> of users with sparse-checkout definitions, while still allowing them
>>> to optionally enable the old behavior with 'sparse.restrictCmds=false'
>>> or the global '--no-restrict-to-sparse-paths' option.
>>
>> (For Stolee:) Did this arise when a user specified a directory to
>> delete, and a (possibly small) part of that directory was in the
>> sparse checkout while other portions of it were outside?
> 
> The user who suggested this used a command like 'git rm */*.csprojx' to
> remove all paths with that file extension, but then realized that they
> were deleting all of those files from the entire repo, not just the
> current sparse-checkout.

Aren't the wildcards expanded by the shell before the command
line is given to Git?  So the Git command should only receive
command line args that actually match existing files, right??

Jeff

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-12 21:01 [PATCH] rm: honor sparse checkout patterns Matheus Tavares
  2020-11-12 23:54 ` Elijah Newren
  2020-11-16 13:58 ` [PATCH v2] " Matheus Tavares
@ 2020-11-16 20:14 ` Junio C Hamano
  2020-11-17  5:20   ` Elijah Newren
  2 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2020-11-16 20:14 UTC (permalink / raw)
  To: Matheus Tavares; +Cc: git, stolee, newren

Matheus Tavares <matheus.bernardino@usp.br> writes:

> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> operation to the paths that match both the command line pathspecs and
> the repository's sparsity patterns.

> This better matches the expectations
> of users with sparse-checkout definitions, while still allowing them
> to optionally enable the old behavior with 'sparse.restrictCmds=false'
> or the global '--no-restrict-to-sparse-paths' option.

Hmph.  Is "rm" the only oddball that ignores the sparse setting?

>  to the paths specified by the sparsity patterns, or to the intersection of
>  those paths and any (like `*.c`) that the user might also specify on the
>  command line. When false, the affected commands will work on full trees,
> -ignoring the sparsity patterns. For now, only git-grep honors this setting.
> +ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
> +setting.

I am not sure if this is a good direction to go---can we make an
inventory of all commands that affect working tree files and see
which ones need the same treatment before going forward with just
"grep" and "rm"?  Documenting the decision on the ones that will not
get the same treatment may also be a good idea.  What I am aiming
for is to prevent users from having to know in which versions of Git
they can rely on the sparsity patterns with what commands, and doing
things piecemeal like these two topics would be a road to confusion.

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-16 14:30     ` Jeff Hostetler
@ 2020-11-17  4:53       ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2020-11-17  4:53 UTC (permalink / raw)
  To: Jeff Hostetler; +Cc: Derrick Stolee, Matheus Tavares, Git Mailing List

On Mon, Nov 16, 2020 at 6:30 AM Jeff Hostetler <git@jeffhostetler.com> wrote:
>
> On 11/13/20 8:47 AM, Derrick Stolee wrote:
> > On 11/12/2020 6:54 PM, Elijah Newren wrote:
> >> Hi,
> >>
> >> On Thu, Nov 12, 2020 at 1:02 PM Matheus Tavares
> >> <matheus.bernardino@usp.br> wrote:
> >>>
> >>> Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> >>> operation to the paths that match both the command line pathspecs and
> >>> the repository's sparsity patterns. This better matches the expectations
> >>> of users with sparse-checkout definitions, while still allowing them
> >>> to optionally enable the old behavior with 'sparse.restrictCmds=false'
> >>> or the global '--no-restrict-to-sparse-paths' option.
> >>
> >> (For Stolee:) Did this arise when a user specified a directory to
> >> delete, and a (possibly small) part of that directory was in the
> >> sparse checkout while other portions of it were outside?
> >
> > The user who suggested this used a command like 'git rm */*.csprojx' to
> > remove all paths with that file extension, but then realized that they
> > were deleting all of those files from the entire repo, not just the
> > current sparse-checkout.
>
> Aren't the wildcards expanded by the shell before the command
> line is given to Git?  So the Git command should only receive
> command line args that actually match existing files, right??

Good point.  I suspect, though, that the issue may still be a problem
if the user were to quote the wildcards; that may have been what
happened and the reporting of the case just lost them somewhere along
the way.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-16 20:14 ` [PATCH] " Junio C Hamano
@ 2020-11-17  5:20   ` Elijah Newren
  2020-11-20 17:06     ` Elijah Newren
  0 siblings, 1 reply; 14+ messages in thread
From: Elijah Newren @ 2020-11-17  5:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matheus Tavares, Git Mailing List, Derrick Stolee

Hi,

On Mon, Nov 16, 2020 at 12:14 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Matheus Tavares <matheus.bernardino@usp.br> writes:
>
> > Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> > operation to the paths that match both the command line pathspecs and
> > the repository's sparsity patterns.
>
> > This better matches the expectations
> > of users with sparse-checkout definitions, while still allowing them
> > to optionally enable the old behavior with 'sparse.restrictCmds=false'
> > or the global '--no-restrict-to-sparse-paths' option.
>
> Hmph.  Is "rm" the only oddball that ignores the sparse setting?

This might make you much less happy, but in general none of the
commands pay attention to the setting; I think a line or two in
merge-recursive.c is the only part of the codebase outside of
unpack_trees() that pays any attention to it at all.  This was noted
as a problem in the initial review of the sparse-checkout series at
[1], and was the biggest factor behind me requesting the following
being added to the manpage for sparse-checkout[2]:

THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
THE FUTURE.

However, multiple groups were using sparse checkouts anyway, via
manually editing .git/info/sparse-checkout and running `git read-tree
-mu HEAD`, and adding various wrappers around it, and Derrick and I
thought there was value in getting _something_ out there to smooth it
out a little bit.  I'd still say it's pretty rough around the
edges...but useful nonetheless.

[1] https://lore.kernel.org/git/CABPp-BHJeuEHBDkf93m9sfSZ4rZB7+eFejiAXOsjLEUu5eT5FA@mail.gmail.com/
[2] https://lore.kernel.org/git/CABPp-BEryfaeYhuUsiDTaYdRKpK6GRi7hgZ5XSTVkoHVkx2qQA@mail.gmail.com/

> >  to the paths specified by the sparsity patterns, or to the intersection of
> >  those paths and any (like `*.c`) that the user might also specify on the
> >  command line. When false, the affected commands will work on full trees,
> > -ignoring the sparsity patterns. For now, only git-grep honors this setting.
> > +ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
> > +setting.
>
> I am not sure if this is a good direction to go---can we make an
> inventory of all commands that affect working tree files and see
> which ones need the same treatment before going forward with just
> "grep" and "rm"?  Documenting the decision on the ones that will not
> get the same treatment may also be a good idea.  What I am aiming
> for is to prevent users from having to know in which versions of Git
> they can rely on the sparsity patterns with what commands, and doing
> things piecemeal like these two topics would be a road to confusion.

It's not just commands which affect the working tree that need to be
inventoried and adjusted.  We've made lists of commands in the past:

[3] https://lore.kernel.org/git/CABPp-BEbNCYk0pCuEDQ_ViB2=varJPBsVODxNvJs0EVRyBqjBg@mail.gmail.com/
[4] https://lore.kernel.org/git/xmqqy2y3ejwe.fsf@gitster-ct.c.googlers.com/

But the working-directory related ones are perhaps more problematic.
One additoinal example: I just got a report today that "git stash
apply" dies with a fatal error and the working directory in some
intermediate state when trying to apply a stash when the working
directory has a different set of sparsity paths than when the stash
was created.  (Granted, an error makes sense, but this was throwing
untranslated error messages, meaning they weren't in codepaths that
were meant to be triggered.)  This case may not be an apples to apples
comparison, but the testcase did involve adding new files before
stashing, so the stash apply would have been trying to remove files.
Anyway, I'll send more details on that issue in a separate thread
after I've had some time to dig into it.


Anyway, I'm not sure this helps, because I'm basically saying things
are kind of messy, and we're fixing as we go rather than having a full
implementation and all the fixes.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-17  5:20   ` Elijah Newren
@ 2020-11-20 17:06     ` Elijah Newren
  0 siblings, 0 replies; 14+ messages in thread
From: Elijah Newren @ 2020-11-20 17:06 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matheus Tavares, Git Mailing List, Derrick Stolee

Hi,

On Mon, Nov 16, 2020 at 9:20 PM Elijah Newren <newren@gmail.com> wrote:
>
> On Mon, Nov 16, 2020 at 12:14 PM Junio C Hamano <gitster@pobox.com> wrote:
> >
> > Matheus Tavares <matheus.bernardino@usp.br> writes:
> >
> > > Make git-rm honor the 'sparse.restrictCmds' setting, by restricting its
> > > operation to the paths that match both the command line pathspecs and
> > > the repository's sparsity patterns.
> >
> > > This better matches the expectations
> > > of users with sparse-checkout definitions, while still allowing them
> > > to optionally enable the old behavior with 'sparse.restrictCmds=false'
> > > or the global '--no-restrict-to-sparse-paths' option.
> >
> > Hmph.  Is "rm" the only oddball that ignores the sparse setting?
>
> This might make you much less happy, but in general none of the
> commands pay attention to the setting; I think a line or two in

This isn't quite right; as noted at the just submitted [1], there are
three different classes of ways that existing commands at least
partially pay attention to the setting.

[1] https://lore.kernel.org/git/5143cba7047d25137b3d7f8c7811a875c1931aee.1605891222.git.gitgitgadget@gmail.com/

> merge-recursive.c is the only part of the codebase outside of
> unpack_trees() that pays any attention to it at all.  This was noted
> as a problem in the initial review of the sparse-checkout series at
> [1], and was the biggest factor behind me requesting the following
> being added to the manpage for sparse-checkout[2]:
>
> THIS COMMAND IS EXPERIMENTAL. ITS BEHAVIOR, AND THE BEHAVIOR OF OTHER
> COMMANDS IN THE PRESENCE OF SPARSE-CHECKOUTS, WILL LIKELY CHANGE IN
> THE FUTURE.

The fact that commands have only somewhat paid attention to this
setting is still a problem, though.  In fact, it was apparently a
known problem as far back as 2009 just from looking at the short list
of TODOs at the end of that file.

> > >  to the paths specified by the sparsity patterns, or to the intersection of
> > >  those paths and any (like `*.c`) that the user might also specify on the
> > >  command line. When false, the affected commands will work on full trees,
> > > -ignoring the sparsity patterns. For now, only git-grep honors this setting.
> > > +ignoring the sparsity patterns. For now, only git-grep and git-rm honor this
> > > +setting.
> >
> > I am not sure if this is a good direction to go---can we make an
> > inventory of all commands that affect working tree files and see
> > which ones need the same treatment before going forward with just
> > "grep" and "rm"?  Documenting the decision on the ones that will not
> > get the same treatment may also be a good idea.  What I am aiming
> > for is to prevent users from having to know in which versions of Git
> > they can rely on the sparsity patterns with what commands, and doing
> > things piecemeal like these two topics would be a road to confusion.
>
> It's not just commands which affect the working tree that need to be
> inventoried and adjusted.  We've made lists of commands in the past:
>
> [3] https://lore.kernel.org/git/CABPp-BEbNCYk0pCuEDQ_ViB2=varJPBsVODxNvJs0EVRyBqjBg@mail.gmail.com/
> [4] https://lore.kernel.org/git/xmqqy2y3ejwe.fsf@gitster-ct.c.googlers.com/

So, I think there are a few other commands that need to be modified
the same way rm is here by Matheus, a longer list of commands than
what I previously linked to for other modifications, some warnings and
error messages that need to be cleaned up, and a fair amount of
additional testing needed.  I also think we need to revisit the flag
names for --restrict-to-sparse-paths and
--no-restrict-to-sparse-paths; some feedback I'm getting suggest they
might be more frequently used than I originally suspected and thus we
might want shorter names.  (--sparse and --dense?)  So we probably
want to wait off on both mt/grep-sparse-checkout and
mt/rm-sparse-checkout (sorry Matheus) and maybe my recently submitted
stash changes (though those don't have an exposed
--[no]-restrict-to-sparse-paths flag and are modelled on existing
merge behavior) until we have a bigger plan in place.

But I only dug into it a bit while working on the stash apply bug; I'm
going to dig more (probably just after Thanksgiving) and perhaps make
a Documentation/technical/ file of some sort to propose more plans
here.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-16 12:37         ` Matheus Tavares Bernardino
@ 2020-11-23 13:23           ` Johannes Schindelin
  2020-11-24  2:48             ` Matheus Tavares Bernardino
  0 siblings, 1 reply; 14+ messages in thread
From: Johannes Schindelin @ 2020-11-23 13:23 UTC (permalink / raw)
  To: Matheus Tavares Bernardino
  Cc: Johannes Sixt, Derrick Stolee, Elijah Newren, Git Mailing List

Hi Matheus,

On Mon, 16 Nov 2020, Matheus Tavares Bernardino wrote:

> On Sun, Nov 15, 2020 at 6:42 PM Johannes Sixt <j6t@kdbg.org> wrote:
> >
> > Am 15.11.20 um 21:12 schrieb Matheus Tavares Bernardino:
> > > Thank you both for the comments. I'll try to send v2 soon.
> > >
> > > On Fri, Nov 13, 2020 at 10:47 AM Derrick Stolee <stolee@gmail.com> wrote:
> > >>
> > >> On 11/12/2020 6:54 PM, Elijah Newren wrote:
> > >>>
> > >>> Do we also want to include a testcase where the user specifies a
> > >>> directory and part of that directory is within the sparsity paths and
> > >>> part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
> > >>> sub' ?
> > >>
> > >> That is definitely an interesting case.
> > >
> > > I've added the test [1], but it's failing on Windows and I'm not quite
> > > sure why. The trash dir artifact shows that `git sparse-checkout set
> > > /sub/dir` produced the following path on the sparse-checkout file:
> > > "D:/a/git/git/git-sdk-64-minimal/sub/dir".
> >
> > If 'git sparse-checkout' is run from a bash command line, I would not be
> > surprised if the absolute path is munched in the way that you observe,
> > provided that D:/a/git/git/git-sdk-64-minimal is where your MinGW
> > subsystem is located. I that the case?
>
> Yeah, that must be it, thanks. I didn't run the command myself as I'm
> not on Windows, but D:/a/git/git/git-sdk-64-minimal must be the path
> where MinGW was installed by our GitHub Actions script, then. I'll use
> "sub/dir" without the root slash in t3600 to avoid the conversion.
> Thanks again!

In the `windows-test` job, the construct `$(pwd)` will give you the
Windows form (`D:/a/git/git/git-sdk-64-minimal`) whereas the `$PWD` form
will give you the Unix-y form (`/`). What form to use depends on the
context (if the absolute path comes from a shell script, the Unix-y form,
if the absolute path comes from `git.exe` itself, the Windows form).

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH] rm: honor sparse checkout patterns
  2020-11-23 13:23           ` Johannes Schindelin
@ 2020-11-24  2:48             ` Matheus Tavares Bernardino
  0 siblings, 0 replies; 14+ messages in thread
From: Matheus Tavares Bernardino @ 2020-11-24  2:48 UTC (permalink / raw)
  To: Johannes Schindelin
  Cc: Johannes Sixt, Derrick Stolee, Elijah Newren, Git Mailing List

On Mon, Nov 23, 2020 at 10:23 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Matheus,
>
> On Mon, 16 Nov 2020, Matheus Tavares Bernardino wrote:
>
> > On Sun, Nov 15, 2020 at 6:42 PM Johannes Sixt <j6t@kdbg.org> wrote:
> > >
> > > Am 15.11.20 um 21:12 schrieb Matheus Tavares Bernardino:
> > > > Thank you both for the comments. I'll try to send v2 soon.
> > > >
> > > > On Fri, Nov 13, 2020 at 10:47 AM Derrick Stolee <stolee@gmail.com> wrote:
> > > >>
> > > >> On 11/12/2020 6:54 PM, Elijah Newren wrote:
> > > >>>
> > > >>> Do we also want to include a testcase where the user specifies a
> > > >>> directory and part of that directory is within the sparsity paths and
> > > >>> part is out?  E.g.  'git sparse-checkout set /sub/dir && git rm -r
> > > >>> sub' ?
> > > >>
> > > >> That is definitely an interesting case.
> > > >
> > > > I've added the test [1], but it's failing on Windows and I'm not quite
> > > > sure why. The trash dir artifact shows that `git sparse-checkout set
> > > > /sub/dir` produced the following path on the sparse-checkout file:
> > > > "D:/a/git/git/git-sdk-64-minimal/sub/dir".
> > >
> > > If 'git sparse-checkout' is run from a bash command line, I would not be
> > > surprised if the absolute path is munched in the way that you observe,
> > > provided that D:/a/git/git/git-sdk-64-minimal is where your MinGW
> > > subsystem is located. I that the case?
> >
> > Yeah, that must be it, thanks. I didn't run the command myself as I'm
> > not on Windows, but D:/a/git/git/git-sdk-64-minimal must be the path
> > where MinGW was installed by our GitHub Actions script, then. I'll use
> > "sub/dir" without the root slash in t3600 to avoid the conversion.
> > Thanks again!
>
> In the `windows-test` job, the construct `$(pwd)` will give you the
> Windows form (`D:/a/git/git/git-sdk-64-minimal`) whereas the `$PWD` form
> will give you the Unix-y form (`/`). What form to use depends on the
> context (if the absolute path comes from a shell script, the Unix-y form,
> if the absolute path comes from `git.exe` itself, the Windows form).

Got it, thanks for the explanation!

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-11-24  2:51 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2020-11-16 20:14 ` [PATCH] " Junio C Hamano
2020-11-17  5:20   ` Elijah Newren
2020-11-20 17:06     ` Elijah Newren

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

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

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git