sparse-checkout: list directories in cone mode #500
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
/submit |
|
Submitted as [email protected] |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -28,7 +28,7 @@ THE FUTURE. | |||
| COMMANDS | |||
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
"Derrick Stolee via GitGitGadget" <[email protected]> writes:
> From: Derrick Stolee <[email protected]>
> Subject: Re: [PATCH 1/1] sparse-checkout: list folders in cone mode
s/folder/directory/ everywhere as the rest of Git.
> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
> command taks a list of folders as input, then creates an ordered
"takes"
> list of sparse-checkout patterns such that those folders are
> recursively included and all sibling blobs along the parent folders
In this sentence, what does a "blob" really mean? Do you mean a
filesystem entity, that is not a folder, that is immediately
contained in the "parent folder" (in other words, regular files
and symbolic links)?
How would this interact with a submodule by the way?
> are also included. Listing the patterns is less user-friendly than the
> folders themselves.
>
> In cone mode, and as long as the patterns match the expected cone-mode
> pattern types, change the output of 'git sparse-checkout list' to only
> show the folders that created the patterns.
> ...
> +In the cone mode case, the `git sparse-checkout list` subcommand will list the
> +folders that define the recursive patterns. For the example sparse-checkout file
> +above, the output is as follows:
> +
> +--------------------------
> +$ git sparse-checkout list
> +A/B/C
> +--------------------------
> +
Sounds like a worthwhile usability improvement.
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/26/2019 4:17 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email protected]> writes:
>
>> From: Derrick Stolee <[email protected]>
>
>> Subject: Re: [PATCH 1/1] sparse-checkout: list folders in cone mode
> s/folder/directory/ everywhere as the rest of Git.
>
>> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
>> command taks a list of folders as input, then creates an ordered
>
> "takes"
Good catch.
>> list of sparse-checkout patterns such that those folders are
>> recursively included and all sibling blobs along the parent folders
>
> In this sentence, what does a "blob" really mean? Do you mean a
> filesystem entity, that is not a folder, that is immediately
> contained in the "parent folder" (in other words, regular files
> and symbolic links)?
You're right, I'm using strange wording here. How about "sibling
entries"?
> How would this interact with a submodule by the way?
I just checked with the Git repo by running:
git submodule init
git submodule update
git sparse-checkout init --cone
The working directory then contains all blobs at root AND the
sha1collisiondetection submodule. Interesting that the sparse-
checkout feature ignores submodules. That seems like the best
approach since the user can already enlist in a subset of the
submodules.
>> are also included. Listing the patterns is less user-friendly than the
>> folders themselves.
>>
>> In cone mode, and as long as the patterns match the expected cone-mode
>> pattern types, change the output of 'git sparse-checkout list' to only
>> show the folders that created the patterns.
>> ...
>> +In the cone mode case, the `git sparse-checkout list` subcommand will list the
>> +folders that define the recursive patterns. For the example sparse-checkout file
>> +above, the output is as follows:
>> +
>> +--------------------------
>> +$ git sparse-checkout list
>> +A/B/C
>> +--------------------------
>> +
>
> Sounds like a worthwhile usability improvement.
Thanks,
-Stolee
derrickstolee
force-pushed
the
sparse-checkout-list
branch
from
07be7b8 to
d6f4f40
Compare
|
/submit |
|
Submitted as [email protected] |
| @@ -28,7 +28,7 @@ THE FUTURE. | |||
| COMMANDS | |||
There was a problem hiding this comment.
On the Git mailing list, Eric Sunshine wrote (reply to this):
On Fri, Dec 27, 2019 at 1:48 PM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> @@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
> +test_expect_success 'interaction with submodules' '
> + ...
> + cat >expect <<-EOF &&
> + a
> + folder1
> + modules
> + EOF
You would normally use \-EOF rather than -EOF to make it clear that no
interpolation is needed/expected within the here-doc body. However,
this script is already full of -EOF when \-EOF ought to be used, so
being consistent with existing tests may override an objection.
Likewise, please note for future reference that the usual way
here-docs are formatted in Git test scripts is to indent the body of
the here-doc to the same level as the command which opens it. That is:
cat >expect <<\-EOF &&
a
folder1
modules
EOF
But, again, this script is already full of these malformatted
here-docs, so maintaining consistency with the existing test in the
script is probably okay.
(Of course, a preparatory patch fixing these here-doc issues would be
welcome, but might also be outside the scope of this submission. And,
fixing these very minor issues is quite low-priority, so I wouldn't
expect or demand it.)
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 12/27/2019 3:20 PM, Eric Sunshine wrote:
> On Fri, Dec 27, 2019 at 1:48 PM Derrick Stolee via GitGitGadget
> <[email protected]> wrote:
>> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
>> @@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
>> +test_expect_success 'interaction with submodules' '
>> + ...
>> + cat >expect <<-EOF &&
>> + a
>> + folder1
>> + modules
>> + EOF
>
> You would normally use \-EOF rather than -EOF to make it clear that no
> interpolation is needed/expected within the here-doc body. However,
> this script is already full of -EOF when \-EOF ought to be used, so
> being consistent with existing tests may override an objection.
>
> Likewise, please note for future reference that the usual way
> here-docs are formatted in Git test scripts is to indent the body of
> the here-doc to the same level as the command which opens it. That is:
Thanks for pointing out the difference, except...
> cat >expect <<\-EOF &&
This should be <<-\EOF
> a
> folder1
> modules
> EOF
>
> But, again, this script is already full of these malformatted
> here-docs, so maintaining consistency with the existing test in the
> script is probably okay.
Hm. Having these lines have the same tabbing hurts my eyes (it is
harder to see where the contents end, much like if we didn't tab
inside a subshell or an if block).
This is also a place where we are inconsistent, and it's not just
my fault for writing the test script in my own style. Here are a
few scripts that tab the same way as here:
t0008-ignore.sh
t4124-apply-ws-rule.sh
t9400-diff-highlight.sh
These are definitely the minority. I just mention them so anyone
who does a cleanup of this whitespace inconsistency takes the time
to look for all examples.
For now, I'll fix the here-doc interpolation issue for this test,
but keep the whitespace matching the rest of the test script. I'll
add this concern to my next series.
Thanks,
-Stolee
| @@ -28,7 +28,7 @@ THE FUTURE. | |||
| COMMANDS | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set'
> command takes a list of folders as input, then creates an ordered
s/folders/directories/
> list of sparse-checkout patterns such that those folders are
s/folders/directories/
> recursively included and all sibling entries along the parent folders
again here.
> are also included. Listing the patterns is less user-friendly than the
> folders themselves.
and here.
>
> In cone mode, and as long as the patterns match the expected cone-mode
> pattern types, change the output of 'git sparse-checkout list' to only
> show the folders that created the patterns.
and here.
>
> With this change, the following piped commands would not change the
> working directory:
>
> git sparse-checkout list | git sparse-checkout set --stdin
>
> The only time this would not work is if core.sparseCheckoutCone is
> true, but the sparse-checkout file contains patterns that do not
> match the expected pattern types for cone mode.
>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> Documentation/git-sparse-checkout.txt | 11 ++++++++++-
> builtin/sparse-checkout.c | 21 +++++++++++++++++++++
> t/t1091-sparse-checkout-builtin.sh | 11 +++++++++++
> 3 files changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index 9c3c66cc37..dcca9ee826 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -28,7 +28,7 @@ THE FUTURE.
> COMMANDS
> --------
> 'list'::
> - Provide a list of the contents in the sparse-checkout file.
> + Describe the patterns in the sparse-checkout file.
>
> 'init'::
> Enable the `core.sparseCheckout` setting. If the
> @@ -150,6 +150,15 @@ expecting patterns of these types. Git will warn if the patterns do not match.
> If the patterns do match the expected format, then Git will use faster hash-
> based algorithms to compute inclusion in the sparse-checkout.
>
> +In the cone mode case, the `git sparse-checkout list` subcommand will list the
> +folders that define the recursive patterns. For the example sparse-checkout file
and here.
> +above, the output is as follows:
> +
> +--------------------------
> +$ git sparse-checkout list
> +A/B/C
> +--------------------------
> +
> If `core.ignoreCase=true`, then the pattern-matching algorithm will use a
> case-insensitive check. This corrects for case mismatched filenames in the
> 'git sparse-checkout set' command to reflect the expected cone in the working
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index 5d62f7a66d..b3bed891cb 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -53,6 +53,8 @@ static int sparse_checkout_list(int argc, const char **argv)
>
> memset(&pl, 0, sizeof(pl));
>
> + pl.use_cone_patterns = core_sparse_checkout_cone;
> +
> sparse_filename = get_sparse_checkout_filename();
> res = add_patterns_from_file_to_list(sparse_filename, "", 0, &pl, NULL);
> free(sparse_filename);
> @@ -62,6 +64,25 @@ static int sparse_checkout_list(int argc, const char **argv)
> return 0;
> }
>
> + if (pl.use_cone_patterns) {
> + int i;
> + struct pattern_entry *pe;
> + struct hashmap_iter iter;
> + struct string_list sl = STRING_LIST_INIT_DUP;
> +
> + hashmap_for_each_entry(&pl.recursive_hashmap, &iter, pe, ent) {
> + /* pe->pattern starts with "/", skip it */
> + string_list_insert(&sl, pe->pattern + 1);
> + }
> +
> + string_list_sort(&sl);
> +
> + for (i = 0; i < sl.nr; i++)
> + printf("%s\n", sl.items[i].string);
> +
> + return 0;
> + }
> +
> write_patterns_to_file(stdout, &pl);
> clear_pattern_list(&pl);
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 6f7e2d0c9e..37f6d8fa90 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -246,6 +246,17 @@ test_expect_success 'cone mode: init and set' '
> test_cmp expect dir
> '
>
> +test_expect_success 'cone mode: list' '
> + cat >expect <<-EOF &&
> + folder1
> + folder2
> + EOF
> + git -C repo sparse-checkout set --stdin <expect &&
> + git -C repo sparse-checkout list >actual 2>err &&
> + test_must_be_empty err &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'cone mode: set with nested folders' '
There's another one here but that's just the context area, so it'd be
a separate patch in a separate cleanup series...
> git -C repo sparse-checkout set deep deep/deeper1/deepest 2>err &&
> test_line_count = 0 err &&
> --
The rest looks good
| @@ -28,7 +28,7 @@ THE FUTURE. | |||
| COMMANDS | |||
There was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
<[email protected]> wrote:
>
> From: Derrick Stolee <[email protected]>
>
> Junio asked what the behavior is between the sparse-checkout feature
> and the submodule feature.
Does this first sentence help future readers? It is what spurred you
to write the documentation, but it seems like something that could
just be left out.
> The sparse-checkout builtin has not changed
> the way these features interact, but we may as well document it in
> the builtin docs.
>
> Using 'git submodule (init|deinit)' a user can select a subset of
> submodules to populate. This behaves very similar to the sparse-checkout
> feature, but those directories contain their own .git directory
> including an object database and ref space. To have the sparse-checkout
> file also determine if those files should exist would easily cause
> problems. Therefore, keeping these features independent in this way
> is the best way forward.
>
> Also create a test that demonstrates this behavior to make sure
> it doesn't change as the sparse-checkout feature evolves.
>
> Reported-by: Junio C Hamano <[email protected]>
> Signed-off-by: Derrick Stolee <[email protected]>
> ---
> Documentation/git-sparse-checkout.txt | 10 ++++++++++
> t/t1091-sparse-checkout-builtin.sh | 28 +++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/Documentation/git-sparse-checkout.txt b/Documentation/git-sparse-checkout.txt
> index dcca9ee826..2b7aaa0310 100644
> --- a/Documentation/git-sparse-checkout.txt
> +++ b/Documentation/git-sparse-checkout.txt
> @@ -164,6 +164,16 @@ case-insensitive check. This corrects for case mismatched filenames in the
> 'git sparse-checkout set' command to reflect the expected cone in the working
> directory.
>
> +
> +SUBMODULES
> +----------
> +
> +If your repository contains one or more submodules, then those submodules will
> +appear based on which you initialized with the `git submodule` command. If
> +your sparse-checkout patterns exclude an initialized submodule, then that
> +submodule will still appear in your working directory.
> +
> +
> SEE ALSO
> --------
>
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 37f6d8fa90..5572beeeca 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -340,4 +340,32 @@ test_expect_success 'cone mode: set with core.ignoreCase=true' '
> test_cmp expect dir
> '
>
> +test_expect_success 'interaction with submodules' '
> + git clone repo super &&
> + (
> + cd super &&
> + mkdir modules &&
> + git submodule add ../repo modules/child &&
> + git add . &&
> + git commit -m "add submodule" &&
> + git sparse-checkout init --cone &&
> + git sparse-checkout set folder1
> + ) &&
> + list_files super >dir &&
> + cat >expect <<-EOF &&
> + a
> + folder1
> + modules
> + EOF
> + test_cmp expect dir &&
> + list_files super/modules/child >dir &&
> + cat >expect <<-EOF &&
> + a
> + deep
> + folder1
> + folder2
> + EOF
> + test_cmp expect dir
> +'
> +
> test_done
> --
I read over the rest, and not being a submodule user I'm not sure what
I'd expect. But it certainly seems reasonable to document how these
features interact and that you haven't made any modifications in the
area.
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Elijah Newren <[email protected]> writes:
> On Fri, Dec 27, 2019 at 10:47 AM Derrick Stolee via GitGitGadget
> <[email protected]> wrote:
>>
>> From: Derrick Stolee <[email protected]>
>>
>> Junio asked what the behavior is between the sparse-checkout feature
>> and the submodule feature.
>
> Does this first sentence help future readers? It is what spurred you
> to write the documentation, but it seems like something that could
> just be left out.
I tend to agree. It can safely go below the three-dash lines.
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
This branch is now known as |
|
This patch series was integrated into pu via git@2976db4. |
When core.sparseCheckoutCone is enabled, the 'git sparse-checkout set' command takes a list of directories as input, then creates an ordered list of sparse-checkout patterns such that those directories are recursively included and all sibling entries along the parent directories are also included. Listing the patterns is less user-friendly than the directories themselves. In cone mode, and as long as the patterns match the expected cone-mode pattern types, change the output of 'git sparse-checkout list' to only show the directories that created the patterns. With this change, the following piped commands would not change the working directory: git sparse-checkout list | git sparse-checkout set --stdin The only time this would not work is if core.sparseCheckoutCone is true, but the sparse-checkout file contains patterns that do not match the expected pattern types for cone mode. Signed-off-by: Derrick Stolee <[email protected]>
Using 'git submodule (init|deinit)' a user can select a subset of submodules to populate. This behaves very similar to the sparse-checkout feature, but those directories contain their own .git directory including an object database and ref space. To have the sparse-checkout file also determine if those files should exist would easily cause problems. Therefore, keeping these features independent in this way is the best way forward. Also create a test that demonstrates this behavior to make sure it doesn't change as the sparse-checkout feature evolves. Reported-by: Junio C Hamano <[email protected]> Signed-off-by: Derrick Stolee <[email protected]>
derrickstolee
force-pushed
the
sparse-checkout-list
branch
from
331bb7d to
74bbd0f
Compare
|
/submit |
|
Submitted as [email protected] |
|
On the Git mailing list, Elijah Newren wrote (reply to this): |
|
This patch series was integrated into pu via git@1703e8f. |
|
This patch series was integrated into pu via git@7f199ec. |
|
This patch series was integrated into pu via git@1a41514. |
|
This patch series was integrated into pu via git@337ac6e. |
|
This patch series was integrated into next via git@e1a1746. |
|
This patch series was integrated into pu via git@c20d4fd. |
|
This patch series was integrated into next via git@c20d4fd. |
|
This patch series was integrated into master via git@c20d4fd. |
|
Closed via c20d4fd. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
When in cone mode, "git sparse-checkout set" takes a list of folders and constructs an ordered list of patterns for the sparse-checkout file. The "git sparse-checkout list" subcommand outputs the contents of the sparse-checkout file in a very basic way.
This patch changes the behavior of "git sparse-checkout list" when core.sparseCheckoutCone=true. It will output the folders that were used in "git sparse-checkout set" to create the patterns, instead of the patterns themselves.
I believe this was requested in the initial review, but I cannot find that message now.
I was going to include this as part of a longer follow-up series, but I think this may be worth considering for the 2.25.0 release. Hence, it is included by itself.
Update in V2:
Fixed typos/word choice in commit message.
Added a second commit including clarification on interactions with submodules.
Thanks,
-Stolee
Cc: [email protected], [email protected], [email protected]