sparse checkout: fix a few bugs and check argument validity for set/add #1118

newren · 2022-01-18T19:15:20Z

== Maintainer notes ==

Note: There is a small textual and small semantic conflict with ds/sparse-checkout-requires-per-worktree-config in seen. I included the diff with the correct resolution near the end of this cover letter. If you'd prefer I rebased on top of ds/sparse-chckout-requires-per-worktree-config, let me know.

== Overview ==

This series continues attempts to make sparse-checkouts more user friendly. A quick overview:

Patches 1-2 fix existing bugs from en/sparse-checkout-set (i.e. in v2.35.0)
Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/), in cone mode. Since we'll get nasty surprises whether we use or don't use "prefix" for non-cone mode, simply throw an error if set/add subcommands of sparse-checkout are run from a subdirectory.
Patches 4-5 check positional arguments to set/add and provide errors/warnings for very likely mistakes. It also adds a --skip-checks flag for overridding in case you have a very unusual situation.

== Update history ==

Changes since v4:

have --skip-checks enable running from a subdirectory in non-cone mode
make sure new die() messages are marked for translation (and using single quotes instead of double)

Changes since v3:

Use strpbrk() instead of multiple strchr(), fix commit message relative to backslashes.

Changes since v2:

Dropped patch 5
Added Stolee's Reviewed-by

Changes since v1:

Dropped the commit changing cone-mode to default (patch 7, which will be split into multiple patches and submitted as a separate series)
Removed the RFC label
Decided to error out when running set/add with paths from a subdirectory in non-cone mode, and added tests
Changed the warning for non-cone mode with individual files to point out that the user is likely trying to select an individual file, but should likely add a leading slash to ensure that is what happens
Fixed typos, removed unnecessary condition checks

== Conflict resolution ==

Patch to resolve textual and semantic conflict with ds/sparse-checkout-requires-per-worktree-config:

diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
remerge CONFLICT (content): Merge conflict in t/t1091-sparse-checkout-builtin.sh
index 3c6adeb885..3a95d2996d 100755
--- a/t/t1091-sparse-checkout-builtin.sh
+++ b/t/t1091-sparse-checkout-builtin.sh
@@ -275,24 +275,8 @@ test_expect_success 'sparse-index enabled and disabled' '
 	diff -u sparse full | tail -n +3 >actual &&
 	test_cmp expect actual &&
 
-<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< 286c22e5ec (sparse-checkout: reject arguments in cone-mode that look like patterns)
 	git -C repo config --list >config &&
-	! grep index.sparse config
-|||||||||||||||||||||||||||||||| 89bece5c8c
-		diff -u sparse full | tail -n +3 >actual &&
-		test_cmp expect actual &&
-
-		git -C repo config --list >config &&
-		! grep index.sparse config
-	)
-================================
-		diff -u sparse full | tail -n +3 >actual &&
-		test_cmp expect actual &&
-
-		git -C repo config --list >config &&
-		test_cmp_config -C repo false index.sparse
-	)
->>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> 3ce1138272 (config: make git_configset_get_string_tmp() private)
+	test_cmp_config -C repo false index.sparse
 '
 
 test_expect_success 'cone mode: init and set' '
@@ -532,6 +516,7 @@ test_expect_success 'reapply can handle config options' '
 	cat >expect <<-\EOF &&
 	core.sparsecheckout=true
 	core.sparsecheckoutcone=true
+	index.sparse=false
 	EOF
 	test_cmp expect actual &&
 
@@ -539,6 +524,8 @@ test_expect_success 'reapply can handle config options' '
 	git -C repo config --worktree --list >actual &&
 	cat >expect <<-\EOF &&
 	core.sparsecheckout=true
+	core.sparsecheckoutcone=false
+	index.sparse=false
 	EOF
 	test_cmp expect actual &&

== CCs ==

cc: Victoria Dye vdye@github.com
cc: Derrick Stolee stolee@gmail.com
cc: Lessley Dennington lessleydennington@gmail.com
cc: Derrick Stolee derrickstolee@github.com
cc: Elijah Newren newren@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com

newren · 2022-02-13T00:39:05Z

/submit

gitgitgadget · 2022-02-13T00:40:15Z

Submitted as pull.1118.git.1644712797.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1118/newren/sparse-checkout-default-and-arg-validity-v1

To fetch this version to local tag pr-1118/newren/sparse-checkout-default-and-arg-validity-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1118/newren/sparse-checkout-default-and-arg-validity-v1

builtin/sparse-checkout.c

gitgitgadget · 2022-02-14T15:47:18Z

User Derrick Stolee <derrickstolee@github.com> has been added to the cc: list.

builtin/sparse-checkout.c

Documentation/git-sparse-checkout.txt

gitgitgadget · 2022-02-14T16:23:00Z

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> Note (reason for RFC): this is RFC primarily because of dependencies (you
> may not want to pick this up yet, Junio), though there is also a question of
> whether to split patch 7 into two steps -- one for now and one we take in
> some future release. In particular, the first step could be to have
> sparse-checkout error out if neither --no-cone nor --cone are specified and
> then change the default to be --cone in some future release. I don't think
> splitting it into two steps is needed given (a) users who are unaware of the
> change will still get useful error messages telling them that directories
> are expected due to patches 4-6 of this series, and (b) the huge
> "EXPERIMENTAL" warning and explicit note about likely behavioral changes in
> git-sparse-checkout.txt serves as warning about the changes. However, the
> two step approach is an alternative.

I support this change. This will also require an update to the 'git clone'
documentation around the '--sparse' option, as I imagine we are going to
be changing behavior there. (If not, then we should do that as part of the
deprecation.)

> Note 2 (dependencies): this depends on en/present-despite-skipped (which
> depends on vd/sparse-clean-etc) and on
> ds/sparse-checkout-requires-per-worktree-config, because of otherwise heavy
> text conflicts in patch 7 to git-sparse-checkout.txt. Given that neither of
> those have merged to next yet, it may be premature to pick up this series.

Yes, hopefully things will start to settle down a little, especially since
vd/sparse-clean-etc is due to merge any day now.

> This series continues attempts to make sparse-checkouts more user friendly.
> A quick overview:
> 
>  * Patches 1-2 fix existing bugs from en/sparse-checkout-set
>  * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
>    https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
>    at least in cone mode. In non-cone mode it is not clear if patch 3 is a
>    "fix" or a "break" (see the "NON-CONE PROBLEMS" section of the manual
>    added in patch 7, and
>    https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
>    where Stolee suggested it might be incorrect).
>  * Patches 4-6 check positional arguments to set/add and provide
>    errors/warnings for very likely mistakes. It also adds a --skip-checks
>    flag for overridding in case you have a very unusual situation.

I took a close look at these patches and mostly have minor typo fixes. There
was one behavior issue: I don't think you should warn for file paths in non-
cone-mode. Being able to select a single file in a directory full of large
files is one of the main reasons to use non-cone-mode, in my experience.

It might be worth adding some documentation about how to reorganize a repo
to fit cone mode patterns, but that's not necessary.

>  * Patch 7 makes cone mode the default, and makes large updates to the
>    documentation both to explain why we changed the default, and to simplify
>    the documentation since users can just use directories and ignore the
>    intricacies of gitignore-style patterns and how they relate to sparse
>    checkouts.

I'm a fan of the end-result of this patch. I responded with some specific
comments and a suggestion for splitting it into a series of four patches.

Your first 6 patches are likely to be noncontroversial and could merge
more quickly than the deprecation. I think it would be good to get the
full deprecation under full review as soon as possible so we can give the
community a long window to comment on it.

We can also consider if we need a release or two where this behavior
change is announced, but not actually done. I'm not sure if that is
necessary. Making '--no-cone' required might stir up some noise that
indicate how much of an impact the change would make.

Thanks,
-Stolee

builtin/sparse-checkout.c

gitgitgadget · 2022-02-15T05:14:45Z

On the Git mailing list, Elijah Newren wrote (reply to this):

On Mon, Feb 14, 2022 at 8:19 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> On 2/12/2022 7:39 PM, Elijah Newren via GitGitGadget wrote:
> > Note (reason for RFC): this is RFC primarily because of dependencies (you
> > may not want to pick this up yet, Junio), though there is also a question of
> > whether to split patch 7 into two steps -- one for now and one we take in
> > some future release. In particular, the first step could be to have
> > sparse-checkout error out if neither --no-cone nor --cone are specified and
> > then change the default to be --cone in some future release. I don't think
> > splitting it into two steps is needed given (a) users who are unaware of the
> > change will still get useful error messages telling them that directories
> > are expected due to patches 4-6 of this series, and (b) the huge
> > "EXPERIMENTAL" warning and explicit note about likely behavioral changes in
> > git-sparse-checkout.txt serves as warning about the changes. However, the
> > two step approach is an alternative.
>
> I support this change. This will also require an update to the 'git clone'
> documentation around the '--sparse' option, as I imagine we are going to
> be changing behavior there. (If not, then we should do that as part of the
> deprecation.)

Why would that be needed?  The documentation does not specify anything
about cone vs. non-cone mode, only that the initial working tree will
only have files from the toplevel directory present.  So, the
documentation is correct without any needed changes.

> > Note 2 (dependencies): this depends on en/present-despite-skipped (which
> > depends on vd/sparse-clean-etc) and on
> > ds/sparse-checkout-requires-per-worktree-config, because of otherwise heavy
> > text conflicts in patch 7 to git-sparse-checkout.txt. Given that neither of
> > those have merged to next yet, it may be premature to pick up this series.
>
> Yes, hopefully things will start to settle down a little, especially since
> vd/sparse-clean-etc is due to merge any day now.

Yeah, and en/present-despite-skipped addressed all reviewer comments a
full month ago, so I'd like to believe it's ready to merge down soon
too.

> > This series continues attempts to make sparse-checkouts more user friendly.
> > A quick overview:
> >
> >  * Patches 1-2 fix existing bugs from en/sparse-checkout-set
> >  * Patch 3 fixes sparse-checkout-from-subdirectories-ignores-"prefix" (see
> >    https://lore.kernel.org/git/29f0410e-6dfa-2e86-394d-b1fb735e7608@gmail.com/),
> >    at least in cone mode. In non-cone mode it is not clear if patch 3 is a
> >    "fix" or a "break" (see the "NON-CONE PROBLEMS" section of the manual
> >    added in patch 7, and
> >    https://lore.kernel.org/git/e1934710-e228-adc4-d37c-f706883bd27c@gmail.com/
> >    where Stolee suggested it might be incorrect).
> >  * Patches 4-6 check positional arguments to set/add and provide
> >    errors/warnings for very likely mistakes. It also adds a --skip-checks
> >    flag for overridding in case you have a very unusual situation.
>
> I took a close look at these patches and mostly have minor typo fixes. There
> was one behavior issue: I don't think you should warn for file paths in non-
> cone-mode. Being able to select a single file in a directory full of large
> files is one of the main reasons to use non-cone-mode, in my experience.

In which case shouldn't we still show a warning when users specify a
path rather than a pattern, since the former risks selecting more than
one file?  (Adding a leading slash should be recommended for such a
case, right?)

> It might be worth adding some documentation about how to reorganize a repo
> to fit cone mode patterns, but that's not necessary.
>
> >  * Patch 7 makes cone mode the default, and makes large updates to the
> >    documentation both to explain why we changed the default, and to simplify
> >    the documentation since users can just use directories and ignore the
> >    intricacies of gitignore-style patterns and how they relate to sparse
> >    checkouts.
>
> I'm a fan of the end-result of this patch. I responded with some specific
> comments and a suggestion for splitting it into a series of four patches.
>
> Your first 6 patches are likely to be noncontroversial and could merge
> more quickly than the deprecation.

Will do.

> I think it would be good to get the
> full deprecation under full review as soon as possible so we can give the
> community a long window to comment on it.
>
> We can also consider if we need a release or two where this behavior
> change is announced, but not actually done. I'm not sure if that is
> necessary. Making '--no-cone' required might stir up some noise that
> indicate how much of an impact the change would make.

We can discuss this more later, but I think it's worthwhile to
consider what happens even if folks didn't read the BIG warning about
behavioral changes in the git-sparse-checkout.txt manual, and didn't
know about the default change, and tried to use it anyway.  If they
specify something other than a directory, then they'd get an error
message due to the first six patches of this series -- at which point
they can look to the manual and decide to add --no-cone to their
command.  In other words, it basically does the same thing that we'd
do if we decided to have an interim period with an error when neither
--cone or --no-cone were specified, other than the error message
perhaps being slightly different.  What if the user does specify
directories and doesn't know about the default mode change?  Well,
that's where the two modes overlap and things work fine (with only
minor differences in behavior, such as better performance, and files
from leading directories being included), so the user would be able to
continue with their work.  So, I'm not sure that an interim period
where we error out when neither --cone or --no-cone are specified is
going to buy us much of anything.  And besides, we do have that super
big scary warning in the manual.  Anyway, I'll bring this all up again
when I resubmit the final patch broken up into a separate series.

gitgitgadget · 2022-02-15T05:14:48Z

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

newren · 2022-02-15T06:22:54Z

/submit

gitgitgadget · 2022-02-15T06:23:33Z

Error: git fetch https://github.com/gitgitgadget/git -- +refs/notes/gitgitgadget:refs/notes/gitgitgadget refs/heads/maint:refs/remotes/upstream/maint refs/heads/seen:refs/remotes/upstream/seen refs/heads/master:refs/remotes/upstream/master refs/heads/next:refs/remotes/upstream/next +refs/tags/pr-1118/newren/sparse-checkout-default-and-arg-validity-v1:refs/tags/pr-1118/newren/sparse-checkout-default-and-arg-validity-v1 +refs/pull/1118/head:refs/pull/1118/head +refs/pull/1118/merge:refs/pull/1118/merge failed: 1,
From https://github.com/gitgitgadget/git
! [rejected] seen -> upstream/seen (non-fast-forward)
83b2b27..7e23e61 next -> upstream/next

e30119b...286c22e refs/pull/1118/head -> refs/pull/1118/head (forced update)
6f29d0f...e84a5c8 refs/pull/1118/merge -> refs/pull/1118/merge (forced update)

newren · 2022-02-15T07:38:42Z

/submit

gitgitgadget · 2022-02-15T07:39:16Z

Error: git fetch https://github.com/gitgitgadget/git -- +refs/notes/gitgitgadget:refs/notes/gitgitgadget refs/heads/maint:refs/remotes/upstream/maint refs/heads/seen:refs/remotes/upstream/seen refs/heads/master:refs/remotes/upstream/master refs/heads/next:refs/remotes/upstream/next +refs/tags/pr-1118/newren/sparse-checkout-default-and-arg-validity-v1:refs/tags/pr-1118/newren/sparse-checkout-default-and-arg-validity-v1 +refs/pull/1118/head:refs/pull/1118/head +refs/pull/1118/merge:refs/pull/1118/merge failed: 1,
From https://github.com/gitgitgadget/git
! [rejected] seen -> upstream/seen (non-fast-forward)

newren · 2022-02-15T08:06:50Z

@dscho: Any idea why it's failing to submit? (Is it upset with the fact that I rebased and modified the target branch?)

dscho · 2022-02-15T08:23:46Z

Huh. Let me have a look.

dscho · 2022-02-15T08:24:58Z

Oh, I think I know what it is... a recent refactoring seems to have introduced a bug.

In 2c0a435 (extensible: introduce product-config feature, 2022-02-06), we refactored the code to make the names of upstream branches configurable. By mistake, we omitted the `+` that forces updates to the corresponding remote-tracking branches in case of non-fast-forwards. But we do have to expect that e.g. `seen` (and occasionally, `next`) need to be force-fetched. Noticed in gitgitgadget/git#1118 (comment) Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

The set and add subcommands accept multiple positional arguments. The meaning of these arguments differs slightly in the two modes: Cone mode only accepts directories. If given a file, it would previously treat it as a directory, causing not just the file itself to be included but all sibling files as well -- likely against users' expectations. Throw an error if the specified path is a file in the index. Provide a --skip-checks argument to allow users to override (e.g. for the case when the given path IS a directory on another branch). Non-cone mode accepts general gitignore patterns. There are many reasons to avoid this mode, but one possible reason to use it instead of cone mode: to be able to select individual files within a directory. However, if a file is passed to set/add in non-cone mode, you won't be selecting a single file, you'll be selecting a file with the same name in any directory. Thus users will likely want to prefix any paths they specify with a leading '/' character; warn users if the patterns they specify exactly name a file because it means they are likely missing such a leading slash. Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com>

In sparse-checkout add/set under cone mode, the arguments passed are supposed to be directories rather than gitignore-style patterns. However, given the amount of effort spent in the manual discussing patterns, it is easy for users to assume they need to pass patterns such as /foo/* or !/bar/*/ or perhaps they really do ignore the directory rule and specify a random gitignore-style pattern like *.c To help catch such mistakes, throw an error if any of the positional arguments: * starts with any of '/!' * contains any of '*?[]' Inform users they can pass --skip-checks if they have a directory that really does have such special characters in its name. (We exclude '\' because of sparse-checkout's special handling of backslashes; see the MINGW test in t1091.46.) Reviewed-by: Derrick Stolee <derrickstolee@github.com> Signed-off-by: Elijah Newren <newren@gmail.com>

gitgitgadget · 2022-02-18T22:18:46Z

This patch series was integrated into seen via git@3991b9d.

newren · 2022-02-19T16:43:57Z

/submit

gitgitgadget · 2022-02-19T16:45:10Z

Submitted as pull.1118.v5.git.1645289086.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1118/newren/sparse-checkout-default-and-arg-validity-v5

To fetch this version to local tag pr-1118/newren/sparse-checkout-default-and-arg-validity-v5:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1118/newren/sparse-checkout-default-and-arg-validity-v5

gitgitgadget · 2022-02-20T19:46:52Z

On the Git mailing list, Derrick Stolee wrote (reply to this):

On 2/19/2022 11:44 AM, Elijah Newren via GitGitGadget wrote:

> == Update history ==
> 
> Changes since v4:
> 
>  * have --skip-checks enable running from a subdirectory in non-cone mode
>  * make sure new die() messages are marked for translation (and using single
>    quotes instead of double)
> 
> Changes since v3:
> 
>  * Use strpbrk() instead of multiple strchr(), fix commit message relative
>    to backslashes.
> 
> Changes since v2:
> 
>  * Dropped patch 5
>  * Added Stolee's Reviewed-by

I looked at the range-diffs for these iterations and found the changes
to be acceptable. This version (still) looks good to go.

Thanks,
-Stolee

gitgitgadget · 2022-02-21T00:35:45Z

This patch series was integrated into seen via git@20bf310.

gitgitgadget · 2022-02-22T07:49:31Z

This patch series was integrated into seen via git@41dd20f.

gitgitgadget · 2022-02-24T01:42:07Z

This patch series was integrated into seen via git@105bb4d.

gitgitgadget · 2022-02-24T03:44:31Z

There was a status update in the "Cooking" section about the branch en/sparse-checkout-fixes on the Git mailing list:

Further polishing of "git sparse-checkout".

Will merge to 'next'.
source: <pull.1118.v5.git.1645289086.gitgitgadget@gmail.com>

gitgitgadget · 2022-02-26T00:32:53Z

This patch series was integrated into seen via git@bdf35d8.

gitgitgadget · 2022-02-26T02:48:27Z

There was a status update in the "Cooking" section about the branch en/sparse-checkout-fixes on the Git mailing list:

Further polishing of "git sparse-checkout".

Will merge to 'next'.
source: <pull.1118.v5.git.1645289086.gitgitgadget@gmail.com>

gitgitgadget · 2022-02-26T06:25:40Z

This patch series was integrated into seen via git@5fa832e.

gitgitgadget · 2022-03-01T00:15:26Z

This patch series was integrated into seen via git@6c95082.

gitgitgadget · 2022-03-01T00:15:28Z

This patch series was integrated into next via git@7307431.

gitgitgadget · 2022-03-01T13:46:30Z

There was a status update in the "Cooking" section about the branch en/sparse-checkout-fixes on the Git mailing list:

Further polishing of "git sparse-checkout".

Will merge to 'master'.
source: <pull.1118.v5.git.1645289086.gitgitgadget@gmail.com>

gitgitgadget · 2022-03-01T22:05:43Z

This patch series was integrated into seen via git@7b4ce0c.

gitgitgadget · 2022-03-02T07:56:54Z

This patch series was integrated into seen via git@dac150e.

gitgitgadget · 2022-03-04T00:00:54Z

This patch series was integrated into seen via git@b4bee80.

gitgitgadget · 2022-03-04T04:33:23Z

There was a status update in the "Cooking" section about the branch en/sparse-checkout-fixes on the Git mailing list:

Further polishing of "git sparse-checkout".

Will merge to 'master'.
source: <pull.1118.v5.git.1645289086.gitgitgadget@gmail.com>

gitgitgadget · 2022-03-04T22:52:16Z

This patch series was integrated into seen via git@8f9c673.

gitgitgadget · 2022-03-07T22:51:00Z

This patch series was integrated into seen via git@9671764.

gitgitgadget · 2022-03-07T22:51:01Z

This patch series was integrated into master via git@9671764.

gitgitgadget · 2022-03-07T22:51:02Z

This patch series was integrated into next via git@9671764.

gitgitgadget · 2022-03-07T22:51:05Z

Closed via 9671764.

newren force-pushed the sparse-checkout-default-and-arg-validity branch 5 times, most recently from 16baea5 to 888cfe5 Compare January 19, 2022 01:50

newren force-pushed the sparse-checkout-default-and-arg-validity branch from 888cfe5 to 31d62da Compare February 12, 2022 21:27

newren changed the title ~~sparse checkout: make --cone mode the default, and check add/set argument validity~~ RFC: sparse checkout: make --cone mode the default, and check add/set argument validity Feb 12, 2022

newren force-pushed the sparse-checkout-default-and-arg-validity branch from 31d62da to e30119b Compare February 12, 2022 23:16