[v2,8/9] git-sparse-checkout.txt: mark non-cone mode as deprecated

Commit Message

Elijah Newren March 12, 2022, 3:11 a.m. UTC

From: Elijah Newren <[e-mail]>

While we have no current plans to actually remove --no-cone mode, we
think users would be better off not using it.  Update the documentation
accordingly, including explaining why we think non-cone mode is
problematic for users.

Signed-off-by: Elijah Newren <[e-mail]>
---
 Documentation/git-sparse-checkout.txt | 86 +++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 4 deletions(-)

Comments

Junio C Hamano March 14, 2022, 9:13 p.m. UTC | #1

"Elijah Newren via GitGitGadget" <[e-mail]> writes:

> +the default mode, but we do not recommend using it.  It does not work
> +with the `--sparse-index` option, and will likely be incompatible with
> +other new features as they are added.

If we explicitly say we have no intention to remove, while saying
that "non-cone" mode is a second class citizen, the users would be
confused. Does this mixed attitude only apply to non-cone mode, or
is it true for any other fringe features that we do not care about
in Git?

This does not build confidence with the Git software suite.  If we
are removing, then this attitude is OK, but if we are keeping it,
then we should promise that we never add features only to "cone".

> +INTERNALS -- NON-CONE PROBLEMS
> +------------------------------
> +
> +The `$GIT_DIR/info/sparse-checkout` file populated by the `set` and
> +`add` subcommands is defined to be a bunch of patterns (one per line)
> +using the same syntax as `.gitignore` files.  In cone mode, these
> +patterns are restricted to matching directories (and users only ever
> +need supply or see directory names), while in non-cone mode any
> +gitignore-style pattern is permitted.  Using the full gitignore-style
> +patterns in non-cone mode has a number of shortcomings:
> +
> +  * Fundamentally, it makes various worktree-updating processes (pull,
> +    merge, rebase, switch, reset, checkout, etc.) require O(N*M) pattern
> +    matches, where N is the number of patterns and M is the number of
> +    paths in the index.  This scales poorly.
> +
> +  * Avoiding the scaling issue has to be done via limiting the number
> +    of patterns via specifying leading directory name or glob.

These are valid.

> +  * Passing globs on the command line is error-prone as users may
> +    forget to quote the glob, causing the shell to expand it into all
> +    matching files and pass them all individually along to
> +    sparse-checkout set/add.  This both exacerbates the scaling
> +    problem, and hardcodes the list of selected files to those which
> +    were present at the time the initial set/add subcommand was run
> +    (and thus ignoring other files matching the same glob which come
> +    into the working tree after switching branches or pulling down
> +    updates).

That's end-user mistakes that is common to other things like
pathspecs, i.e. "git grep -- \*.c" vs "git grep -- *.c".  It hardly
deserves to spend more than 5 lines shouting.

> +  * It uses "ignore"/"exclude" syntax for selecting what to "include",
> +    which periodically causes confusion.

Well, .gitignore uses the syntax to select the paths to include or
exclude from a set, and then the set specifies what is left out of
the index.  I am not sure if that is the source of confusion.

> +  * It introduces inconsistencies in the Git command line, since other
> +    commands use pathspecs, but sparse-checkout (in non-cone mode) uses
> +    gitignore patterns.

But "cone" mode does not take pathspec, does it?  ":(icase)/A/B/C"
for example?  I do not see this as a credible argument against it.

> +  * It has edge cases where the "right" behavior is unclear.  Two examples:
> +
> +    First, two users are in a subdirectory, and the first runs
> +       git sparse-checkout set '/toplevel-dir/*.c'
> +    while the second runs
> +       git sparse-checkout set relative-dir

Are we aware which mode of sparse-checkout they are in?  Do we give
a mechanism for users to ask "we have 'non-cone' sparse checkout in
the sparse-checkout file.  we want to use the more efficient and
recommended 'cone' mode.  Please convert it", or even if there is
not an automated conversion mechansim, can the user manually do so
(which I suspect is not fundamentally possible---the cone mode is
inherently less expressive)?

If not, then we should expect these two users to _know_ (or at least
check before they run "set" subcommand) which mode is in use.

So, with that expectation,

> +    Should those arguments be transliterated into
> +       current/subdirectory/toplevel-dir/*.c
> +    and
> +       current/subdirectory/relative-dir

If we are using non-cone mode, then both would be recorded as-is.

If we are using cone mode, we may reject the one that ends with "*.c"
based on the two traits it has, i.e. (1) there is no such directory
in the working tree nor the index nor the HEAD, and (2) it has an
unusual character (i.e. glob) '*' to be in a real path.

Elijah Newren April 22, 2022, 2:31 a.m. UTC | #2

On Mon, Mar 14, 2022 at 2:14 PM Junio C Hamano <[e-mail]> wrote:
>
> "Elijah Newren via GitGitGadget" <[e-mail]> writes:
>
> > +the default mode, but we do not recommend using it.  It does not work
> > +with the `--sparse-index` option, and will likely be incompatible with
> > +other new features as they are added.
>
> If we explicitly say we have no intention to remove, while saying
> that "non-cone" mode is a second class citizen, the users would be
> confused.

I don't see why they'd be confused.  It seems common to me in software
development to state that a less preferred option will continue to be
supported with its existing feature set, while some other option is
both recommended and will gain new features.

> Does this mixed attitude only apply to non-cone mode, or
> is it true for any other fringe features that we do not care about
> in Git?
>
> This does not build confidence with the Git software suite.  If we
> are removing, then this attitude is OK, but if we are keeping it,
> then we should promise that we never add features only to "cone".

The sparse-index feature only works with "cone" mode and adding
support for non-cone mode would be infeasible.  So we've already
crossed that line.

I'd also like to make partial clones and sparse-checkouts work better
together (something you also suggested in the past), and improve
support for disconnected development.  In particular, having a partial
clone that starts with the history of files within the sparse-cone
instead of an empty set of blobs seems like a reasonable way to do
that.  However, having a partial clone that starts with the history of
files matching a non-cone mode set of paths seems intractable to me; I
don't see how that could reasonably be implemented server side and
would expect it to be rejected even if implemented.  So there's at
least one example in my mind where we would want to cross that line
again.

Although, if you really are saying these are the only two acceptable
options to you, then I'll start pushing to remove non-cone mode;
that'd be much preferable to removing the sparse-index feature and
hamstringing future development.

> > +INTERNALS -- NON-CONE PROBLEMS
> > +------------------------------
> > +
> > +The `$GIT_DIR/info/sparse-checkout` file populated by the `set` and
> > +`add` subcommands is defined to be a bunch of patterns (one per line)
> > +using the same syntax as `.gitignore` files.  In cone mode, these
> > +patterns are restricted to matching directories (and users only ever
> > +need supply or see directory names), while in non-cone mode any
> > +gitignore-style pattern is permitted.  Using the full gitignore-style
> > +patterns in non-cone mode has a number of shortcomings:
> > +
> > +  * Fundamentally, it makes various worktree-updating processes (pull,
> > +    merge, rebase, switch, reset, checkout, etc.) require O(N*M) pattern
> > +    matches, where N is the number of patterns and M is the number of
> > +    paths in the index.  This scales poorly.
> > +
> > +  * Avoiding the scaling issue has to be done via limiting the number
> > +    of patterns via specifying leading directory name or glob.
>
> These are valid.
>
> > +  * Passing globs on the command line is error-prone as users may
> > +    forget to quote the glob, causing the shell to expand it into all
> > +    matching files and pass them all individually along to
> > +    sparse-checkout set/add.  This both exacerbates the scaling
> > +    problem, and hardcodes the list of selected files to those which
> > +    were present at the time the initial set/add subcommand was run
> > +    (and thus ignoring other files matching the same glob which come
> > +    into the working tree after switching branches or pulling down
> > +    updates).
>
> That's end-user mistakes that is common to other things like
> pathspecs, i.e. "git grep -- \*.c" vs "git grep -- *.c".  It hardly
> deserves to spend more than 5 lines shouting.

Fair point.

I do think there's still a small qualitative difference, though, in
regards to timing and the ability of the user to discover their
mistake.  With sparse-checkout, the mistake gets recorded and affects
later commands.  In particular, it's possible that at the time the
command is run, the shell completion of the glob catches all currently
existing relevant files, so the user doesn't notice the mistake.  Once
the user switches branches or rebases or merges or whatever, then the
past expansion of the glob finally bites the user.  In contrast, if
the grep/log/status command is wrong, it's noticeable from the
immediate output the user gets and they can simply re-run with the
corrected command.

> > +  * It uses "ignore"/"exclude" syntax for selecting what to "include",
> > +    which periodically causes confusion.
>
> Well, .gitignore uses the syntax to select the paths to include or
> exclude from a set, and then the set specifies what is left out of
> the index.  I am not sure if that is the source of confusion.

Perhaps I should reword this point:

  * .gitignore files use the gitignore pattern syntax to specify
patterns that are *excluded* (unless the pattern is negated) from the
index.
  * non-cone mode sparse-checkout uses the same syntax to specify
patterns with the opposite meaning -- they are *included* in the tree
(unless the pattern is negated).

If there were two copies of the pattern specification, or if the
documentation carefully pointed out matching/non-matching instead of
exclude/include everywhere, then this perhaps wouldn't be a problem.
However, the documentation for these patterns is almost completely
.gitignore-specific and written talking about exclusion (with
inclusion exceptions).

> > +  * It introduces inconsistencies in the Git command line, since other
> > +    commands use pathspecs, but sparse-checkout (in non-cone mode) uses
> > +    gitignore patterns.
>
> But "cone" mode does not take pathspec, does it?  ":(icase)/A/B/C"
> for example?  I do not see this as a credible argument against it.

My objection is that it's weird having two different forms of "special
path pattern matching" in the git command line.  Every git subcommand
that has such a thing uses pathspecs, except for non-cone mode
sparse-checkout.

I don't see why any subcommand necessarily needs to have "special path
pattern matching", I'm just saying that *if* some subcommand does have
such a thing, it'd be nice to use the same kind of special path
pattern matching that all other git commands do.

> > +  * It has edge cases where the "right" behavior is unclear.  Two examples:
> > +
> > +    First, two users are in a subdirectory, and the first runs
> > +       git sparse-checkout set '/toplevel-dir/*.c'
> > +    while the second runs
> > +       git sparse-checkout set relative-dir
>
> Are we aware which mode of sparse-checkout they are in?  Do we give
> a mechanism for users to ask "we have 'non-cone' sparse checkout in
> the sparse-checkout file.  we want to use the more efficient and
> recommended 'cone' mode.  Please convert it", or even if there is
> not an automated conversion mechansim, can the user manually do so
> (which I suspect is not fundamentally possible---the cone mode is
> inherently less expressive)?
>
> If not, then we should expect these two users to _know_ (or at least
> check before they run "set" subcommand) which mode is in use.
>
> So, with that expectation,
>
> > +    Should those arguments be transliterated into
> > +       current/subdirectory/toplevel-dir/*.c
> > +    and
> > +       current/subdirectory/relative-dir
>
> If we are using non-cone mode, then both would be recorded as-is.

Recording '/toplevel-dir/*.c' and 'relative-dir' as-is sounds great to
me, but doesn't it run contrary to what you previously pushed for
tab-completion?  Previously, you said that in non-cone mode, tab
completion should complete on files and directories (with no "relative
to the toplevel directory" qualification that I could see), which does
not seem to work well with what you've specified here.

Patch

diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
index ae6ea8b48b0..aaf3ae63853 100644
--- a/Documentation/git-sparse-checkout.txt
+++ b/Documentation/git-sparse-checkout.txt
@@ -71,10 +71,13 @@  and `--cone` needed to be specified or `core.sparseCheckoutCone` needed
 to be enabled.
 +
 When `--no-cone` is passed, the input list is considered a list of
-patterns.  This mode is harder to use and less performant, and is thus
-not recommended.  See the "Sparse Checkout" section of
-linkgit:git-read-tree[1] and the "Pattern Set" sections below for more
-details.
+patterns.  This mode is harder to use, and unless you can keep the
+number of patterns small, its design also scales poorly.  It used to be
+the default mode, but we do not recommend using it.  It does not work
+with the `--sparse-index` option, and will likely be incompatible with
+other new features as they are added.  See the "Non-cone Problems"
+section below and the "Sparse Checkout" section of
+linkgit:git-read-tree[1] for more details.
 +
 Use the `--[no-]sparse-index` option to use a sparse index (the
 default is to not use it).  A sparse index reduces the size of the
@@ -191,6 +194,81 @@  directory, it updates the skip-worktree bits in the index based
 on this file. The files matching the patterns in the file will
 appear in the working directory, and the rest will not.
 
+INTERNALS -- NON-CONE PROBLEMS
+------------------------------
+
+The `$GIT_DIR/info/sparse-checkout` file populated by the `set` and
+`add` subcommands is defined to be a bunch of patterns (one per line)
+using the same syntax as `.gitignore` files.  In cone mode, these
+patterns are restricted to matching directories (and users only ever
+need supply or see directory names), while in non-cone mode any
+gitignore-style pattern is permitted.  Using the full gitignore-style
+patterns in non-cone mode has a number of shortcomings:
+
+  * Fundamentally, it makes various worktree-updating processes (pull,
+    merge, rebase, switch, reset, checkout, etc.) require O(N*M) pattern
+    matches, where N is the number of patterns and M is the number of
+    paths in the index.  This scales poorly.
+
+  * Avoiding the scaling issue has to be done via limiting the number
+    of patterns via specifying leading directory name or glob.
+
+  * Passing globs on the command line is error-prone as users may
+    forget to quote the glob, causing the shell to expand it into all
+    matching files and pass them all individually along to
+    sparse-checkout set/add.  This both exacerbates the scaling
+    problem, and hardcodes the list of selected files to those which
+    were present at the time the initial set/add subcommand was run
+    (and thus ignoring other files matching the same glob which come
+    into the working tree after switching branches or pulling down
+    updates).
+
+  * It uses "ignore"/"exclude" syntax for selecting what to "include",
+    which periodically causes confusion.
+
+  * It introduces inconsistencies in the Git command line, since other
+    commands use pathspecs, but sparse-checkout (in non-cone mode) uses
+    gitignore patterns.
+
+  * It has edge cases where the "right" behavior is unclear.  Two examples:
+
+    First, two users are in a subdirectory, and the first runs
+       git sparse-checkout set '/toplevel-dir/*.c'
+    while the second runs
+       git sparse-checkout set relative-dir
+    Should those arguments be transliterated into
+       current/subdirectory/toplevel-dir/*.c
+    and
+       current/subdirectory/relative-dir
+    before inserting into the sparse-checkout file?  The user who typed
+    the first command is probably aware that arguments to set/add are
+    supposed to be patterns in non-cone mode, and probably would not be
+    happy with such a transliteration.  However, many gitignore-style
+    patterns are just paths, which might be what the user who typed the
+    second command was thinking, and they'd be upset if their argument
+    wasn't transliterated.
+
+    Second, what should bash-completion complete on for set/add commands
+    for non-cone users?  If it suggests paths, is it exacerbating the
+    problem above?  Also, if it suggests paths, what if the user has a
+    file or directory that begins with either a '!' or '#' or has a '*',
+    '\', '?', '[', or ']' in its name?  And if it suggests paths, will
+    it complete "/pro" to "/proc" (in the root filesytem) rather than to
+    "/progress.txt" in the current directory?  (Note that users are
+    likely to want to start paths with a leading '/' in non-cone mode,
+    for the same reason that .gitignore files often have one.)
+    Completing on files or directories might give nasty surprises in
+    all these cases.
+
+  * The excessive flexibility made other extensions essentially
+    impractical.  `--sparse-index` may not have been feasible in
+    non-cone mode, but even if it was, it would have been far more work
+    to implement and may have been too slow in practice.  Some ideas for
+    adding coupling between partial clones and sparse checkouts are only
+    practical with a more restricted set of paths.
+
+For all these reasons, non-cone mode is deprecated.  Please switch to
+using cone mode.
 
 INTERNALS -- CONE PATTERN SET
 -----------------------------