git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: Elijah Newren <newren@gmail.com>
Cc: Lessley Dennington via GitGitGadget <gitgitgadget@gmail.com>,
	Git Mailing List <git@vger.kernel.org>,
	Derrick Stolee <stolee@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	johannes.schindelin@gmail.com,
	Lessley Dennington <lessleydennington@gmail.com>
Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
Date: Tue, 18 Jan 2022 23:22:34 +0100	[thread overview]
Message-ID: <20220118222234.GB2632@szeder.dev> (raw)
In-Reply-To: <CABPp-BGR+UrW5ej-8+XXHPkjMfFgURycd9rWC+2awUvYcr_PXA@mail.gmail.com>

On Sat, Jan 15, 2022 at 05:03:24PM -0800, Elijah Newren wrote:
> On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
> > > Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
> >
> > None of these patches touch sparse-checkout, but only the completion
> > script and its tests.  Therefore "completion:" would be a better
> > matching area prefix.
> 
> Thanks for the detailed feedback and guidance in your review.  Very
> helpful.  I'll omit quoting most of it here, but I do want to comment
> on the point about directories.
> 
> ...
> > > 4. A list of directories (but not files) is provided when users enter git
> > > sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
> >
> > Why limit completion only to directories?  Both of those subcommands
> > accept files,
> 
> Discussed in part at [1], but let me give a more detailed answer.

It was a semi-rhetorical question.  Whether the reasons for expluding
files are sound or not, it should be convincingly justified in the
commit message.

> Both of these commands accept not only directories and files, but also
> nearly arbitrary input as far as I can tell.  (In cone-mode, it'll
> accept anything so long as it doesn't look like a relative path that
> tries to reach above the toplevel directory with '../' sequences.  In
> non-cone mode, I think it accepts completely arbitrary input).  If our
> guide is merely what the command swallows, then we should forgo
> completion for these subcommands, because it's not possible to
> enumerate all possible completions.  I don't think that's a useful
> guide or starting point, so we instead need to discuss what are
> reasonable completions.
> 
> cone-mode works exclusively on directories.  So, in that mode,
> directories are what we want to complete on.  (And if a file is
> specified, cone-mode will treat it as a directory and add expressions
> for including all the files under that "directory", which might be
> confusing.  sparse-checkout doesn't verify it is a diretory, because
> it *might* name a directory in a different branch, including one not
> yet downloaded.  But "might name a directory on another branch" is no
> reason to suggest picking that pathname with completion.)
> 
> In non-cone mode, arbitrary expressions are valid and will be treated
> as gitignore-style expressions.  That again leaves us with either not
> providing completions, or choosing a subset of possible inputs that
> are reasonable suggestions for users.  I prefer the latter, and in
> particular I feel that directories are reasonable suggestions.  In
> contrast, I don't think providing files is helpful, because it
> reinforces the design flaw of non-cone mode.  Non-cone mode has
> quadratic performance baked into its design, and since
> sparse-checkouts are about performance, non-cone mode kind of defeats
> the purpose of the command.

Or about disk space.  Which, because of the potentially significantly
reduced number of files in the work tree can bring along significant
performance benefits, even with quadratic behavior.

>  (In addition to other problems[2].)  So,
> I think non-cone mode should be deprecated and excised.  Patches
> elsewhere are moving in the direction of deprecation already[3], and
> we've already discussed multiple steps we'll likely take soon
> continuing in that direction.  In the meantime, providing just
> directories for completion seems like a good direction to me.
> 
> [1] https://lore.kernel.org/git/CABPp-BG=wr81CPtW1M12xFN_0dyS8mAZjM6o=77LA20Zge8Xng@mail.gmail.com/
> [2] https://lore.kernel.org/git/CABPp-BF=-1aZd=nFHF6spo7Ksa7f7Wb7ervCt0QvtNitMY=ZBA@mail.gmail.com/
> [3] https://lore.kernel.org/git/0af00779128e594aff0ee4ec5378addeac8e88a2.1642175983.git.gitgitgadget@gmail.com/
> ("This mode is harder to use and less performant, and is thus not
> recommended.")
> 
> > and I think 'git sparse-checkout set README.md' is a
> > perfectly reasonable command.
> 
> Reasonable in what sense?  That it makes it (vastly) easier to
> implement the completion and sparse-checkout set|add will swallow it,
> or that it's something that should actually be recommended for users
> doing sparse-checkouts?  While the former certainly holds, I don't
> think the latter does.

I used the following command to create a sparse-checkout from
linux.git to build and play with 'perf':

  git sparse-checkout set tools/perf/ tools/scripts/ tools/build/ tools/include/ tools/arch/x86/ tools/lib/ /.gitignore /.gitattributes

Including the top-level '.gitignore' and '.gitattributes' was
important, becase those ignore object files and specify userdiff.
Now, I wouldn't mind having other files present in the top-level
directory, because there are only a handful of files there.  However,
being able to specify just those two files to 'git sparse-checkout'
was great, because I didn't even have to think about what wildcard
pattern to use, and what negative pattern to use to exclude anything
that might have been included recursively.

I don't remember having any performance issues with it, on the
contrary, I do remember that Git suddenly became much faster that in
the full worktree.

So I'm fairly convinced that specifying files to sparse-checkout is a
feature that can make users' life easier.  It certainly made my life
easier.


On a related note: I just noticed the leading slashes in '/.gitignore'
and '/.gitattributes'.  __git_complete_index_file() is not ready for
that, I'm afraid; but I don't think the proposed patches could deal
with that, either (but I didn't actually try).

It would be great if completion could cope with patterns starting with
'/' and/or '!'.



  parent reply	other threads:[~2022-01-18 22:22 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-30 13:43   ` Derrick Stolee
2021-12-30 16:19     ` Lessley Dennington
2021-12-30 17:43       ` Derrick Stolee
2021-12-31 19:27         ` Elijah Newren
2022-01-04 19:19           ` Lessley Dennington
2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-30 13:50   ` Derrick Stolee
2021-12-30 16:24     ` Lessley Dennington
2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-31 20:03     ` Elijah Newren
2021-12-31 22:20       ` Junio C Hamano
2021-12-31 22:25         ` Elijah Newren
2022-01-04 19:25         ` Lessley Dennington
2022-01-04 20:25           ` Elijah Newren
2022-01-05 14:05             ` Lessley Dennington
2022-01-04 19:24       ` Lessley Dennington
2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-31 22:52     ` Elijah Newren
2022-01-04 19:41       ` Lessley Dennington
2022-01-04 20:42         ` Elijah Newren
2022-01-05 20:20           ` Lessley Dennington
2022-01-05 22:55             ` Elijah Newren
2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2022-01-15  9:57       ` SZEDER Gábor
2022-01-16  1:03         ` Elijah Newren
2022-01-16 22:13           ` Junio C Hamano
2022-01-17 18:14             ` Elijah Newren
2022-01-17 19:40               ` Junio C Hamano
2022-01-18 17:56                 ` Lessley Dennington
2022-01-22  1:07                   ` Lessley Dennington
2022-01-22  1:08                     ` Lessley Dennington
2022-01-22  2:11                       ` Lessley Dennington
2022-01-18 21:02               ` SZEDER Gábor
2022-01-18 21:43                 ` Elijah Newren
2022-01-18 17:59           ` Lessley Dennington
2022-01-18 22:22           ` SZEDER Gábor [this message]
2022-01-18 23:30             ` Elijah Newren
2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
2022-01-12 23:43       ` Lessley Dennington
2022-01-13  0:00         ` Junio C Hamano
2022-01-13  0:38         ` Elijah Newren
2022-01-13 19:02           ` Lessley Dennington
2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
2022-01-11 17:17       ` Lessley Dennington
2022-01-11 19:45         ` Taylor Blau
2022-01-12 18:35           ` Lessley Dennington
2022-01-27 21:21     ` [PATCH v4 0/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-01-27 21:21       ` [PATCH v4 1/3] completion: add sparse-checkout tests Lessley Dennington via GitGitGadget
2022-01-28  0:08         ` Elijah Newren
2022-01-28  1:56           ` Junio C Hamano
2022-01-28  2:04             ` Elijah Newren
2022-01-27 21:21       ` [PATCH v4 2/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-01-28  1:21         ` Elijah Newren
2022-01-31 20:03           ` Lessley Dennington
2022-01-31 21:37             ` Elijah Newren
2022-01-27 21:21       ` [PATCH v4 3/3] completion: ensure cone mode completion with multiple <TAB>s Lessley Dennington via GitGitGadget
2022-01-28  1:53         ` Elijah Newren
2022-02-03 20:44       ` [PATCH v5 0/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-02-03 20:44         ` [PATCH v5 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-03 23:52           ` Elijah Newren
2022-02-04  0:34             ` Lessley Dennington
2022-02-03 20:44         ` [PATCH v5 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-03 20:44         ` [PATCH v5 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-02-03 23:58           ` Elijah Newren
2022-02-04  0:37             ` Lessley Dennington
2022-02-04  4:25             ` Ævar Arnfjörð Bjarmason
2022-02-04 21:55           ` Junio C Hamano
2022-02-03 21:48         ` [PATCH v5 0/3] completion: sparse-checkout updates Junio C Hamano
2022-02-03 22:17           ` Lessley Dennington
2022-02-03 23:28             ` Junio C Hamano
2022-02-03 23:59               ` Lessley Dennington
2022-02-04  2:43                 ` Lessley Dennington
2022-02-04  3:28                   ` Lessley Dennington
2022-02-04  4:21                   ` Ævar Arnfjörð Bjarmason
2022-02-04  3:26         ` [PATCH v6 " Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-02-04  6:05           ` [PATCH v6 0/3] completion: sparse-checkout updates Elijah Newren
2022-02-04 17:04             ` Junio C Hamano
2022-02-04 17:55               ` Elijah Newren
2022-02-04 19:54                 ` Junio C Hamano
2022-02-04 20:01                   ` Elijah Newren
2022-02-04 21:47                     ` Junio C Hamano
2022-02-07 17:31           ` [PATCH v7 " Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-04-06  9:42               ` Adam Dinwoodie
2022-02-08  4:16             ` [PATCH v7 0/3] completion: sparse-checkout updates Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

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

  git send-email \
    --in-reply-to=20220118222234.GB2632@szeder.dev \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmail.com \
    --cc=lessleydennington@gmail.com \
    --cc=newren@gmail.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

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