[PATCHv8 1/5] string list: improve comment

> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  string-list.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/string-list.h b/string-list.h
> index d3809a1..465a1f0 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_
>   * list->strdup_strings must be set, as new memory needs to be
>   * allocated to hold the substrings.  If maxsplit is non-negative,
>   * then split at most maxsplit times.  Return the number of substrings
> - * appended to list.
> + * appended to list. The list may be non-empty already.

> Stefan Beller <sbeller@google.com> writes:
>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  string-list.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/string-list.h b/string-list.h
>> index d3809a1..465a1f0 100644
>> --- a/string-list.h
>> +++ b/string-list.h
>> @@ -106,7 +106,7 @@ void unsorted_string_list_delete_item(struct string_list *list, int i, int free_
>>   * list->strdup_strings must be set, as new memory needs to be
>>   * allocated to hold the substrings.  If maxsplit is non-negative,
>>   * then split at most maxsplit times.  Return the number of substrings
>> - * appended to list.
>> + * appended to list. The list may be non-empty already.
>
> I personally find that the original comment is clear enough, though.
>
> When somebody says "resulting elements of the split are appended to
> the list" without saying either:
>
>   a. "the list MUST be empty in the beginning", or
>   b. "the list will be emptied first before the split result are appended",
>
> wouldn't it be natural to take it as "you can append them to any
> list, be it empty or not, and they are _appended_, not replaced"?

>
> So while this is not incorrect per-se, I am not sure if it adds much
> value.  If somebody needs this additional clarification, because the
> original did not say a. above, she would certainly need more
> clarification because the original did not say b. above, either.
>
> "The list may be non-empty already", but she would keep wondering if
> the existing contents would be discarded before the result gets
> appended to it.
>
> You may say "No, there won't be such a confusion, because we say
> 'append'; empty and then append is 'replace'".  But then the same
> logic would say "There cannot be a requirement for the list to be
> empty in the first place, because we say 'append'".
>
> So...

> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index cafc284..aa9f220 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -384,6 +384,23 @@ full pathname may have special meaning:
>  +
>  Glob magic is incompatible with literal magic.
>  
> +attr;;
> +	Additionally to matching the pathspec, the path must have the
> +	attribute as specified. The syntax for specifying the required
> +	attributes is "`attr: [mode] <attribute name> [=value]`"
> ++
> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and
> +you can query each attribute for certain states. The "`[mode]`" is a special
> +character to indicate which attribute states are looked for. The following
> +modes are available:
> +
> + - an empty "`[mode]`" matches if the attribute is set
> + - "`-`" the attribute must be unset
> + - "`!`" the attribute must be unspecified
> + - an empty "`[mode]`" combined with "`[=value]`" matches if the attribute has
> +   the given value.
> ++

> +static int match_attrs(const char *name, int namelen,
> +		       const struct pathspec_item *item)
> +{
> +	int i;
> +
> +	git_check_attr_counted(name, namelen, item->attr_check);
> +	for (i = 0; i < item->attr_match_nr; i++) {
> +		const char *value;
> +		int matched;
> +		enum attr_match_mode match_mode;
> +
> +		value = item->attr_check->check[i].value;
> +		match_mode = item->attr_match[i].match_mode;
> +
> +		if (ATTR_TRUE(value))
> +			matched = match_mode == MATCH_SET;
> +		else if (ATTR_FALSE(value))
> +			matched = match_mode == MATCH_UNSET;
> +		else if (ATTR_UNSET(value))
> +			matched = match_mode == MATCH_UNSPECIFIED;

> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)
> +{
> +	struct string_list_item *si;
> +	struct string_list list = STRING_LIST_INIT_DUP;
> +
> +
> +	if (!value || !strlen(value))
> +		die(_("attr spec must not be empty"));
> +
> +	string_list_split(&list, value, ' ', -1);
> +	string_list_remove_empty_items(&list, 0);
> +
> +	if (!item->attr_check)
> +		item->attr_check = git_attr_check_alloc();
> +	else
> +		die(_("Only one 'attr:' specification is allowed."));
> +
> +	ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);
> +
> +	for_each_string_list_item(si, &list) {
> +		size_t attr_len;
> +
> +		int j = item->attr_match_nr++;
> +		const char *attr = si->string;
> +		struct attr_match *am = &item->attr_match[j];
> +
> +		if (attr[0] == '!')
> +			am->match_mode = MATCH_UNSPECIFIED;
> +		else if (attr[0] == '-')
> +			am->match_mode = MATCH_UNSET;
> +		else
> +			am->match_mode = MATCH_SET;
> +
> +		if (am->match_mode != MATCH_SET)
> +			/* skip first character */
> +			attr++;
> +		attr_len = strcspn(attr, "=");
> +		if (attr[attr_len] == '=') {
> +			am->match_mode = MATCH_VALUE;
> +			am->value = xstrdup(&attr[attr_len + 1]);
> +			if (strchr(am->value, '\\'))
> +				die(_("attr spec values must not contain backslashes"));
> +		} else
> +			am->value = NULL;
> +

> +		if (!attr_name_valid(attr, attr_len)) {
> +			struct strbuf sb = STRBUF_INIT;
> +			am->match_mode = INVALID_ATTR;
> +			invalid_attr_name_message(&sb, attr, attr_len);
> +			die(_("invalid attribute in '%s': '%s'"), value, sb.buf);
> +		}
> +
> +		am->attr = git_attr_counted(attr, attr_len);

> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
> new file mode 100755
> index 0000000..c0d8cda
> --- /dev/null
> +++ b/t/t6134-pathspec-with-labels.sh
> @@ -0,0 +1,166 @@
> +#!/bin/sh
> +
> +test_description='test labels in pathspecs'
> +. ./test-lib.sh
> +
> +test_expect_success 'setup a tree' '
> +	mkdir sub &&
> +	for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do
> +		: >$p &&
> +		git add $p &&
> +		: >sub/$p
> +		git add sub/$p
> +	done &&
> +	git commit -m $p &&
> +	git ls-files >actual &&
> +	cat <<EOF >expect &&
> +fileA
> +fileAB
> +fileAC
> +fileB
> +fileBC
> +fileC
> +fileNoLabel
> +fileSetLabel
> +fileUnsetLabel
> +fileValue
> +fileWrongLabel
> +sub/fileA
> +sub/fileAB
> +sub/fileAC
> +sub/fileB
> +sub/fileBC
> +sub/fileC
> +sub/fileNoLabel
> +sub/fileSetLabel
> +sub/fileUnsetLabel
> +sub/fileValue
> +sub/fileWrongLabel
> +EOF

> +fileWrongLabel label☺
> +EOF
> +	git add .gitattributes &&
> +	git commit -m "add attributes"
> +'
> +
> +sq="'"

> +
> +test_expect_success 'check specific set attr' '
> +	cat <<EOF >expect &&
> +fileSetLabel
> +sub/fileSetLabel
> +EOF
> +	git ls-files ":(attr:label)" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check specific unset attr' '
> +	cat <<EOF >expect &&
> +fileUnsetLabel
> +sub/fileUnsetLabel
> +EOF
> +	git ls-files ":(attr:-label)" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check specific value attr' '
> +	cat <<EOF >expect &&
> +fileValue
> +sub/fileValue
> +EOF
> +	git ls-files ":(attr:label=foo)" >actual &&
> +	test_cmp expect actual &&
> +	git ls-files ":(attr:label=bar)" >actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_expect_success 'check unspecified attr' '
> +	cat <<EOF >expect &&
> +.gitattributes
> +...
> +EOF
> +	git ls-files :\(attr:\!label\) >actual &&

> +test_expect_success 'check multiple unspecified attr' '
> +	cat <<EOF >expect &&
> +.gitattributes
> +fileC
> +fileNoLabel
> +fileWrongLabel
> +sub/fileC
> +sub/fileNoLabel
> +sub/fileWrongLabel
> +EOF
> +	git ls-files :\(attr:\!labelB\ \!labelA\ \!label\) >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check label with more labels but excluded path' '
> +	cat <<EOF >expect &&
> +fileAB
> +fileB
> +fileBC
> +EOF
> +	git ls-files ":(attr:labelB)" ":(exclude)sub/" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'check label excluding other labels' '
> +	cat <<EOF >expect &&
> +fileAB
> +fileB
> +fileBC
> +sub/fileAB
> +sub/fileB
> +EOF
> +	git ls-files ":(attr:labelB)" ":(exclude,attr:labelC)sub/" >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'abort on giving invalid label on the command line' '
> +	test_must_fail git ls-files . ":(attr:☺)" 2>actual &&
> +	test_i18ngrep "fatal" actual
> +'
> +
> +test_done

> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
>> index cafc284..aa9f220 100644
>> --- a/Documentation/glossary-content.txt
>> +++ b/Documentation/glossary-content.txt
>> @@ -384,6 +384,23 @@ full pathname may have special meaning:
>>  +
>>  Glob magic is incompatible with literal magic.
>>
>> +attr;;
>> +     Additionally to matching the pathspec, the path must have the
>> +     attribute as specified. The syntax for specifying the required
>> +     attributes is "`attr: [mode] <attribute name> [=value]`"
>> ++
>> +Attributes can have 4 states (Set, Unset, Set to a value, unspecified) and
>> +you can query each attribute for certain states. The "`[mode]`" is a special
>> +character to indicate which attribute states are looked for. The following
>> +modes are available:
>> +
>> + - an empty "`[mode]`" matches if the attribute is set
>> + - "`-`" the attribute must be unset
>> + - "`!`" the attribute must be unspecified
>> + - an empty "`[mode]`" combined with "`[=value]`" matches if the attribute has
>> +   the given value.
>> ++
>
> As an initial design, I find this much more agreeable than the
> previous rounds.  I however find the phrasing of the above harder
> than necessary to understand, for a few reasons.
>
>  * Mixed use of "X matches if ..." and "... must be Y" makes it
>    unclear if they are talking about different kind of things, or
>    the same kind of things in merely different ways.
>
>  * It does not make it clear "=value" is only meaningful when [mode]
>    is empty.
>
> Perhaps dropping the '[mode]' thing altogether and instead saying
>
>         After `attr:` comes a space separated list of "attribute
>         requirements", all of which must be met in order for the
>         path to be considered a match; this is in addition to the
>         usual non-magic pathspec pattern matching.
>
>         Each of the attribute requirements for the path takes one of
>         these forms:
>
>         - "`ATTR`" requires that the attribute `ATTR` must be set.
>
>         - "`-ATTR`" requires that the attribute `ATTR` must be unset.
>
>         - "`ATTR=VALUE`" requires that the attribute `ATTR` must be
>           set to the string `VALUE`.
>
>         - "`!ATTR`" requires that the attribute `ATTR` must be
>           unspecified.
>
> would make the resulting text easier to read?

>
>> +static int match_attrs(const char *name, int namelen,
>> +                    const struct pathspec_item *item)
>> +{
>> +     int i;
>> +
>> +     git_check_attr_counted(name, namelen, item->attr_check);
>> +     for (i = 0; i < item->attr_match_nr; i++) {
>> +             const char *value;
>> +             int matched;
>> +             enum attr_match_mode match_mode;
>> +
>> +             value = item->attr_check->check[i].value;
>> +             match_mode = item->attr_match[i].match_mode;
>> +
>> +             if (ATTR_TRUE(value))
>> +                     matched = match_mode == MATCH_SET;
>> +             else if (ATTR_FALSE(value))
>> +                     matched = match_mode == MATCH_UNSET;
>> +             else if (ATTR_UNSET(value))
>> +                     matched = match_mode == MATCH_UNSPECIFIED;
>
> readability nit:
>
>         matched = (match_mode == MATCH_WHATEVER);
>
> would be easier to view

>
>> +             else
>> +                     matched = (match_mode == MATCH_VALUE &&
>> +                                !strcmp(item->attr_match[i].value, value));
>
> and would match the last case above better.
>
>> +static void parse_pathspec_attr_match(struct pathspec_item *item, const char *value)
>> +{
>> +     struct string_list_item *si;
>> +     struct string_list list = STRING_LIST_INIT_DUP;
>> +
>> +
>> +     if (!value || !strlen(value))
>> +             die(_("attr spec must not be empty"));
>> +
>> +     string_list_split(&list, value, ' ', -1);
>> +     string_list_remove_empty_items(&list, 0);
>> +
>> +     if (!item->attr_check)
>> +             item->attr_check = git_attr_check_alloc();
>> +     else
>> +             die(_("Only one 'attr:' specification is allowed."));
>> +
>> +     ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, item->attr_match_alloc);
>> +
>> +     for_each_string_list_item(si, &list) {
>> +             size_t attr_len;
>> +
>> +             int j = item->attr_match_nr++;
>> +             const char *attr = si->string;
>> +             struct attr_match *am = &item->attr_match[j];
>> +
>> +             if (attr[0] == '!')
>> +                     am->match_mode = MATCH_UNSPECIFIED;
>> +             else if (attr[0] == '-')
>> +                     am->match_mode = MATCH_UNSET;
>> +             else
>> +                     am->match_mode = MATCH_SET;
>> +
>> +             if (am->match_mode != MATCH_SET)
>> +                     /* skip first character */
>> +                     attr++;
>> +             attr_len = strcspn(attr, "=");
>> +             if (attr[attr_len] == '=') {
>> +                     am->match_mode = MATCH_VALUE;
>> +                     am->value = xstrdup(&attr[attr_len + 1]);
>> +                     if (strchr(am->value, '\\'))
>> +                             die(_("attr spec values must not contain backslashes"));
>> +             } else
>> +                     am->value = NULL;
>> +
>
> Let me try a variant to see if we can improve it (thinking aloud).
>
>         switch (*attr) {
>         case '!':
>                 am->match_mode = MATCH_UNSPECIFIED;
>                 attr++;
>                 break;
>         case '-':
>                 am->match_mode = MATCH_UNSET;
>                 attr++;
>                 break;
>         default:
>                 attr_len = strcspn(attr, "=");
>                 if (attr[attr_len] != '=')
>                         am->match_mode = MATCH_SET;
>                 else {
>                         am->match_mode = MATCH_VALUE;
>                         am->value = xstrdup(...);
>                 }
>                 break;
>         }
>
> Using switch/case does not save line counts at all but it may still
> make the result easier to follow.  More importantly, it would help
> you catch "!ATTR=VAR" as an "invalid attribute name 'ATTR=VAR' was
> requested" error by arranging the code to make sure that scanning
> for '=' would not happen in !/- case (in other words, while thiking
> aloud with an alternative way to write the same thing, I spotted a
> bug in the original ;-).

>
>> +             if (!attr_name_valid(attr, attr_len)) {
>> +                     struct strbuf sb = STRBUF_INIT;
>> +                     am->match_mode = INVALID_ATTR;
>> +                     invalid_attr_name_message(&sb, attr, attr_len);
>> +                     die(_("invalid attribute in '%s': '%s'"), value, sb.buf);
>> +             }
>> +
>> +             am->attr = git_attr_counted(attr, attr_len);
>
> I'd do this the other order, i.e.
>
>         am->attr = git_attr_counted(...);
>         if (!am->attr) {
>                 ...
>                 die(...);
>         }
>
> It is wasteful to call attr_name_valid() yourself before seeing an
> error from git_attr_counted().
>
>> diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
>> new file mode 100755
>> index 0000000..c0d8cda
>> --- /dev/null
>> +++ b/t/t6134-pathspec-with-labels.sh
>> @@ -0,0 +1,166 @@
>> +#!/bin/sh
>> +
>> +test_description='test labels in pathspecs'
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup a tree' '
>> +     mkdir sub &&
>> +     for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do
>> +             : >$p &&
>> +             git add $p &&
>> +             : >sub/$p
>> +             git add sub/$p
>> +     done &&
>> +     git commit -m $p &&
>> +     git ls-files >actual &&
>> +     cat <<EOF >expect &&
>> +fileA
>> +fileAB
>> +fileAC
>> +fileB
>> +fileBC
>> +fileC
>> +fileNoLabel
>> +fileSetLabel
>> +fileUnsetLabel
>> +fileValue
>> +fileWrongLabel
>> +sub/fileA
>> +sub/fileAB
>> +sub/fileAC
>> +sub/fileB
>> +sub/fileBC
>> +sub/fileC
>> +sub/fileNoLabel
>> +sub/fileSetLabel
>> +sub/fileUnsetLabel
>> +sub/fileValue
>> +sub/fileWrongLabel
>> +EOF
>
>         cat <<-\EOF >expect &&
>         fileA
>         ...
>         EOF
>
> to indent the whole thing?

>> +sq="'"
>
> I do not think you use this $sq variable below, but I may have
> overlooked its use.
>

>> +     git ls-files :\(attr:\!label\) >actual &&
>
> Why not the more normal-looking ":(attr:!label)"?  I do not think
> history substitution would trigger in an script.
>
> And no, "I may cut and paste into my interactive shell while
> debugging" is not a good reason to make the end-result (i.e. script)
> less readable.

> $ grep -r "cat" |grep "<<-"|wc -l
> 915
> $ grep -r "cat" |grep "<<"|grep -v "<<-"| wc -l
> 1329
>
> I was undecided what the prevailing style is, some did indent,
> others did not.

> +test_expect_success 'setup a tree' '
> +	mkdir sub &&
> +	for p in fileA fileB fileC fileAB fileAC fileBC fileNoLabel fileUnsetLabel fileSetLabel fileValue fileWrongLabel; do
> +		: >$p &&
> +		git add $p &&
> +		: >sub/$p
> +		git add sub/$p
> +	done &&
> +	git commit -m $p &&

> +	git ls-files >actual &&
> +	cat <<EOF >expect &&
> +fileA
> +fileAB
> +fileAC
> +fileB
> +fileBC
> +fileC
> +fileNoLabel
> +fileSetLabel
> +fileUnsetLabel
> +fileValue
> +fileWrongLabel
> +sub/fileA
> +sub/fileAB
> +sub/fileAC
> +sub/fileB
> +sub/fileBC
> +sub/fileC
> +sub/fileNoLabel
> +sub/fileSetLabel
> +sub/fileUnsetLabel
> +sub/fileValue
> +sub/fileWrongLabel
> +EOF
> +	test_cmp expect actual
> +'

> (B) requires some thought though. Here is my vision:
>
>     1) Allow pathspecs for sparse checkout.
>
>       I wonder if we just add support for that in .git/info-sparse-checkout
>       or if we add a new file that is for pathspecs only, or we have a config
>       option whether sparse-checkout follows pathspecs or gitignore patterns
>
>     2) Teach `git clone` a new option `--sparse-checkout <pathspec>`
>       When that option is set the pathspec is written into the new file from
>       (1) and core.sparsecheckout is set to true
>
>     3) Advertise to do a `git clone --sparse-checkout
> :(attr:default_submodules)`
>
> Going this way we would help making submodules not different but integrate more
> into other concepts of Git. As a downside this would require touching
> sparse checkout which may be more time consuming than just adding a
> `git clone --init-submodules-by-label` which stores the labels and only upddates
> those submodules.
>
> Or are there other ideas how to go from here?

> Stefan Beller <sbeller@google.com> writes:
>
>> (B) requires some thought though. Here is my vision:
>>
>>     1) Allow pathspecs for sparse checkout.
>>
>>       I wonder if we just add support for that in .git/info-sparse-checkout
>>       or if we add a new file that is for pathspecs only, or we have a config
>>       option whether sparse-checkout follows pathspecs or gitignore patterns
>>
>>     2) Teach `git clone` a new option `--sparse-checkout <pathspec>`
>>       When that option is set the pathspec is written into the new file from
>>       (1) and core.sparsecheckout is set to true
>>
>>     3) Advertise to do a `git clone --sparse-checkout
>> :(attr:default_submodules)`
>>
>> Going this way we would help making submodules not different but integrate more
>> into other concepts of Git. As a downside this would require touching
>> sparse checkout which may be more time consuming than just adding a
>> `git clone --init-submodules-by-label` which stores the labels and only upddates
>> those submodules.
>>
>> Or are there other ideas how to go from here?
>
> My take is to pretend sparse checkout does not exist at all and then
> go from there ;-)  It is a poorly designed and implemented "concept"
> that we do not want to spread around.
>
> You were going to add defaultGroup and teach 'clone' (and other
> commands) about it, and have clone find submodules in that Group,
> right?

> Right. But upon finding the new name for clone, I wondered why
> this has to be submodule specific. The attr pathspecs are also working
> with any other files. So if you don't use submodules, I think it would be
> pretty cool to have a
>
>     git clone --sparse-checkout=Documentation/ ...

> Stefan Beller <sbeller@google.com> writes:
>
>> Right. But upon finding the new name for clone, I wondered why
>> this has to be submodule specific. The attr pathspecs are also working
>> with any other files. So if you don't use submodules, I think it would be
>> pretty cool to have a
>>
>>     git clone --sparse-checkout=Documentation/ ...
>
> It would be cool, but arent' "sparse" and the various existing
> status "submodule" has very different things?

>
>  - A submodule can be uninitialized, in which case you do get an empty
>    directory but you do not see .git in it.
>
>  - A path can be excluded by the sparse checkout mechanism, in which
>    case you do not get _anything_ in the filesystem.

>
> So "git clone --sparse-exclude=Documentation/" that does not waste
> diskspace for Documentation/ directory may be an interesting thing
> to have, and "git clone --sparse-exclude=submodule-dir/" that does
> not even create submodule-dir/ directory may also be, but the latter
> is quite different from a submodule that is not initialied in a
> superproject that does not use any "sparse" mechanism.
>
> Besides, I think (improved) submodule mechanism would be a good way
> forward for scalability, and "sparse" hack is not (primarily because
> it still populates the index fully with 5 million entries even when
> your attention is narrowed only to a handful of directories with
> 2000 leaf entries; this misdesign requires other ugly hacks to be
> piled on, like untracked cache and split index).
>
> I do not think we want "submodule" to be tied to and dependent on
> the latter.

> Using these pathspec attrs are a good example for why we would
> want to make it more generic. Imagine a future, where more attributes
> can be codified into pathspecs and this is one of the possibilities:
>
>     git clone --sparse=":(exclude,size>5MB,binary)
>
> which would not checkout files that are large binary files. We could
> also extend the protocol (v2 with the capabilities in client speaks first)
> to transmit such a requirement to the server.

> On Fri, May 20, 2016 at 4:25 AM, Stefan Beller <sbeller@google.com> wrote:
>>> My take is to pretend sparse checkout does not exist at all and then
>>> go from there ;-)
>
> Hehe.. shameless plug, narrow checkout [1] should be its great
> successor where everything is done right (famous last words). Maybe I
> can convince Stefan to finish that off then I'll finally bring narrow
> clone!

>
> [1] http://article.gmane.org/gmane.comp.version-control.git/289112
>
>> Using these pathspec attrs are a good example for why we would
>> want to make it more generic. Imagine a future, where more attributes
>> can be codified into pathspecs and this is one of the possibilities:
>>
>>     git clone --sparse=":(exclude,size>5MB,binary)
>>
>> which would not checkout files that are large binary files. We could
>> also extend the protocol (v2 with the capabilities in client speaks first)
>> to transmit such a requirement to the server.
>
> I think you need narrow clone there ;-) It's the first step to have a
> clone with missing directories. I think then we can extend it further
> to exclude (large) files.

>
>> Why is sparseness considered bad?
>
> It does not scale well when the worktree gets bigger. It can be slow
> (but this is just a technical problem I haven't spent time on fixing).
> And it does not enable narrow clone (not with lots of hacks, I think I
> did just that in some early iterations).
>
>> If I wanted to just do the submodule only thing, this would be a smaller
>> series, I would guess.
>
> No no no. Do more. Start with narrow checkout. I'll help ;-)

> On Sun, May 22, 2016 at 4:45 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Fri, May 20, 2016 at 4:25 AM, Stefan Beller <sbeller@google.com> wrote:
>>>> My take is to pretend sparse checkout does not exist at all and then
>>>> go from there ;-)
>>
>> Hehe.. shameless plug, narrow checkout [1] should be its great
>> successor where everything is done right (famous last words). Maybe I
>> can convince Stefan to finish that off then I'll finally bring narrow
>> clone!
>
> I started reviewing that (see separate email).
>
> So conceptually the narrow checkout is "sparse checkout just for
> directories" IIUC.

>>> Using these pathspec attrs are a good example for why we would
>>> want to make it more generic. Imagine a future, where more attributes
>>> can be codified into pathspecs and this is one of the possibilities:
>>>
>>>     git clone --sparse=":(exclude,size>5MB,binary)
>>>
>>> which would not checkout files that are large binary files. We could
>>> also extend the protocol (v2 with the capabilities in client speaks first)
>>> to transmit such a requirement to the server.
>>
>> I think you need narrow clone there ;-) It's the first step to have a
>> clone with missing directories. I think then we can extend it further
>> to exclude (large) files.
>
> But if we only exclude some files we are not having a binary
> decision for a directory, so narrow checkout doesn't work here?
>
>     git clone --sparse=":(exclude,size>5MB,binary)
>
> would maybe lead to have
>   dir/path.c #included
>   dir/binary-asset.img # excluded
>
> so it is more a sparse thing than a narrow thing?

>>> Why is sparseness considered bad?
>>
>> It does not scale well when the worktree gets bigger. It can be slow
>> (but this is just a technical problem I haven't spent time on fixing).
>> And it does not enable narrow clone (not with lots of hacks, I think I
>> did just that in some early iterations).
>>
>>> If I wanted to just do the submodule only thing, this would be a smaller
>>> series, I would guess.
>>
>> No no no. Do more. Start with narrow checkout. I'll help ;-)
>
> Thanks for the encouragement!
> How is the interface going to look like for the narrow checkout/clone ?
> If the UI is supposed to be very similar, we can merge the two concepts
> and make it one thing.
>
> But if the UI is different enough, we may want to keep it separate as
> it is not as confusing to the user as we'd think?

> What I imagine UI-wise, see
> http://thread.gmane.org/gmane.comp.version-control.git/295221
>
>
> Thanks,
> Stefan
>
>> --
>> Duy