Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sparse index:
checkout-index
#424Sparse index:
checkout-index
#424Changes from all commits
7d45ff5
cf3e9f4
2e96d80
50367bf
61c3288
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little odd. From what I understand, we generally use the
--sparse
flag to enable sparse behavior, but here we seem to be circumventing it. We may want to consider something more like--ignore-sparse
. Let me know if I'm missing something, though!There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check out gitgitgadget#1018 which restricts how Git operates outside of the sparse-checkout. The
--sparse
option allows overriding these protections. It's really new.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ldennington I definitely see where you're coming from (it does seem somewhat backwards that
--sparse
would expand the index/do things outside the sparse checkout). I did base it on @derrickstolee's linked PR (+ls-trees
), and I think this line conveys it best:The option here is pretty much treating
--sparse
as "does things in parts of the repository that should be sparse", rather than ignoring those parts of the repository. @derrickstolee please correct me if I'm using it wrong, though - I'm mostly looking for this to be consistent with the other already-implemented options (and offer a way to usecheckout-index --all
without always expanding the full index).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdye: I think you have the right approach. We should make Git ignore files outside of the sparse-checkout cone as much as possible, and allow users to override that behavior by specifying
--sparse
. We do not expect users to have a legitimate reason to care about files outside of the sparse-checkout, but Git needs to care unless we make these changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why shouldn't these be combined? What if I want to poke at a file outside of my sparse-checkout? It certainly seems like
git checkout-index --sparse -- deep/deeper2/a
would be a good way to get that file on-disk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't include
--sparse
for individual files is because, as it is right now, individually-specified files will always expand the index if that specified file is not already in the index - I wasn't really sure what the "right" way to handle it was, though. On one hand, not requiring a--sparse
flag tocheckout-index
individual files (even those outside the checkout definition) is the most consistent with existing behavior. On the other hand, it is kind of weird that we need a special--sparse
option for files in sparse directories for--all
, but not when you list out individual files.What do you think would be better (especially based on how you've handled similar situations in other commands)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a plumbing command, let's be cautious and avoid the behavior break by requiring
--sparse
for individual files. (This is a change of opinion from my earlier comment.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to make both implementations say "file is outside the sparse-checkout definition"?
I wonder if using
if (!path_in_sparse_checkout(name, &the_index))
somewhere would help here.It's perhaps a bit tricky how this loop is structured, since it is doing a prefix match on a range of index entries.
In this test script, what happens with this example?
Note that
deep/deeper2/
is a sparse directory within thedeep/
directory, but we would still want the other entries to succeed, silently ignoring thedeep/deeper2/
entry. This would only be an error if the user rangit checkout-index -f -- deep/deeper2/
, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just comparing the names, not performing a prefix match (the first condition in this check makes sure the names are the same length):
The result is that, from your example,
git checkout-index -f -- deep/
(or any path that doesn't refer to a file entry in the index) returnsgit checkout-index: <name> is not in the cache
. It seems like this command expects exact names of cache entries (except for the subcommands using a pathspec, but they handle things differently anyway), in which case<name> is not in the cache
makes some amount of sense? Agreed that it's misleading, though - would that be something that should be clarified in the command doc, or in this message itself?As for checking whether something is in the sparse checkout, this doesn't actually block checking out files outside the sparse definition (you can successfully run
git checkout-index -- folder1/a
without any other repo setup). The issue is that sparse directories do appear as standalone cache entries, so without theS_ISSPARSEDIR
check the entry would be passed along tocheckout_entry
(which then fails because it doesn't handle directories). Since there's no supportcheckout-index
-ing directories otherwise, this probably needs to be blocked rather than worked around. Maybe the message should be the same as whenever anyone tries tocheckout-index
a directory in general? I.e., "lie" and say<name> is not in the cache
, or some other error message that's the same between them?