Show 325 quoted lines
> This lets users limit files or exclude files based on file
> attributes during git-add and git-stash.
> For example, the chromium project would like to use this like
> "git add --all ':(exclude,attr:submodule)'", as submodules are managed in a
> unique way and often results in submodule changes that users do not want in
> their commits.
>
> This does not change any attr magic implementation. It is only adding
> attr as an allowed pathspec in git-add and git-stash, which was previously
> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).
> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.
> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,
> which was the case for git-stash.
> (But originally filed here:
> https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)
>
> Furthermore, while other commands hit this code path it did not result in unexpected
> behavior because this bug only impacts the pathspec->items->original field which is
> NOT used to filter paths. However, git-stash does use pathspec->items->original when
> building args used to call other git commands.
> (See add_pathspecs() usage and implementation in stash.c)
>
> It is possible that when the attr pathspec feature was first added in
> b0db704652 (pathspec: allow querying for attributes, 2017-03-13),
> "PATHSPEC_ATTR" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.
>
> Later, to get a more user-friendly error message when attr was used with git-add,
> PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()
> 84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).
> However, this user-friendly error message was never added for git-stash.
>
> Signed-off-by: Joanna Wang <jojwang@google.com>
>
> ---
>
> > At this point in the code, is it guaranteed that element[0] is ':'
> > and never a NUL? Also is it guaranteed that element has ')'
> > somewhere later if element[1] is '('?
> No sorry, we can only assume this if there was element magic detected
> by parse_element_magic(). So if element magic is not 0, we know the body
> is in the expected form (either long or short).
> I have added comments and a check for magic to guard against this.
>
> > I wonder if this existing bug caused by
> > failing to copy the value of "attr:" is triggerable from a codepath
> > that already allows PATHSPEC_ATTR magic.
> While there are other commands that go through the prefix_magic path (like `git-add -i`),
> AFAICT they are not actually impacted by the bug. The bug only impacts the value of
> pathspec->items->original which is not used during pathspec matching. But git-stash
> uses `original` to pass in as args to other commands it calls. I've included this
> detail in the description above.
>
> > but as the third parameter strbuf_add() takes is of type size_t, it would not
> > hurt to define "len" as the same type as well.
> Thanks for spotting. fixed.
>
> builtin/add.c | 7 ++-
> dir.c | 3 +-
> pathspec.c | 36 ++++++++---
> t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--
> 4 files changed, 136 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 5126d2ede3..d46e4d10e9 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> * Check the "pathspec '%s' did not match any files" block
> * below before enabling new magic.
> */
> - parse_pathspec(&pathspec, PATHSPEC_ATTR,
> + parse_pathspec(&pathspec, 0,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, argv);
> @@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> if (pathspec.nr)
> die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file");
>
> - parse_pathspec_file(&pathspec, PATHSPEC_ATTR,
> + parse_pathspec_file(&pathspec, 0,
> PATHSPEC_PREFER_FULL |
> PATHSPEC_SYMLINK_LEADING_PATH,
> prefix, pathspec_from_file, pathspec_file_nul);
> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
>
> for (i = 0; i < pathspec.nr; i++) {
> const char *path = pathspec.items[i].match;
> diff --git a/dir.c b/dir.c
> index 16fdb03f2a..4d1cd039be 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
> PATHSPEC_LITERAL |
> PATHSPEC_GLOB |
> PATHSPEC_ICASE |
> - PATHSPEC_EXCLUDE);
> + PATHSPEC_EXCLUDE |
> + PATHSPEC_ATTR);
>
> for (i = 0; i < pathspec->nr; i++) {
> const struct pathspec_item *item = &pathspec->items[i];
> diff --git a/pathspec.c b/pathspec.c
> index bb1efe1f39..2f8721cc15 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -109,16 +109,34 @@ static struct pathspec_magic {
> { PATHSPEC_ATTR, '\0', "attr" },
> };
>
> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)
> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)
> {
> - int i;
> - strbuf_addstr(sb, ":(");
> - for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)
> - if (magic & pathspec_magic[i].bit) {
> - if (sb->buf[sb->len - 1] != '(')
> - strbuf_addch(sb, ',');
> - strbuf_addstr(sb, pathspec_magic[i].name);
> + /* No magic was found in element, just add prefix magic */
> + if (magic == 0) {
> + strbuf_addf(sb, ":(prefix:%d)", prefixlen);
> + return;
> + }
> +
> + /*
> + * At this point we known that parse_element_magic() was able to extract some pathspec
> + * magic from element. So we know element is correctly formatted in either shorthand
> + * or longhand form
> + */
> + if (element[1] != '(') {
> + /* Process an element in shorthand form (e.g. ":!/<match>") */
> + strbuf_addstr(sb, ":(");
> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
> + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\0')) {
> + if (sb->buf[sb->len - 1] != '(')
> + strbuf_addch(sb, ',');
> + strbuf_addstr(sb, pathspec_magic[i].name);
> + }
> }
> + } else {
> + /* For the longhand form, we copy everything up to the final ')' */
> + size_t len = strchr(element, ')') - element;
> + strbuf_add(sb, element, len);
> + }
> strbuf_addf(sb, ",prefix:%d)", prefixlen);
> }
>
> @@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
> struct strbuf sb = STRBUF_INIT;
>
> /* Preserve the actual prefix length of each pattern */
> - prefix_magic(&sb, prefixlen, element_magic);
> + prefix_magic(&sb, prefixlen, element_magic, elt);
>
> strbuf_addstr(&sb, match);
> item->original = strbuf_detach(&sb, NULL);
> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
> index f70c395e75..e46fa176ed 100755
> --- a/t/t6135-pathspec-with-attrs.sh
> +++ b/t/t6135-pathspec-with-attrs.sh
> @@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
> fileSetLabel label
> fileValue label=foo
> fileWrongLabel label☺
> + newFileA* labelA
> + newFileB* labelB
> EOF
> echo fileSetLabel label1 >sub/.gitattributes &&
> git add .gitattributes sub/.gitattributes &&
> git commit -m "add attributes"
> '
>
> +test_expect_success 'setup .gitignore' '
> + cat <<-\EOF >.gitignore &&
> + actual
> + expect
> + pathspec_file
> + EOF
> + git add .gitignore &&
> + git commit -m "add gitignore"
> +'
> +
> test_expect_success 'check specific set attr' '
> cat <<-\EOF >expect &&
> fileSetLabel
> @@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
> test_expect_success 'check unspecified attr' '
> cat <<-\EOF >expect &&
> .gitattributes
> + .gitignore
> fileA
> fileAB
> fileAC
> @@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
> test_expect_success 'check unspecified attr (2)' '
> cat <<-\EOF >expect &&
> HEAD:.gitattributes
> + HEAD:.gitignore
> HEAD:fileA
> HEAD:fileAB
> HEAD:fileAC
> @@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
> test_expect_success 'check multiple unspecified attr' '
> cat <<-\EOF >expect &&
> .gitattributes
> + .gitignore
> fileC
> fileNoLabel
> fileWrongLabel
> @@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
> test_i18ngrep "Only one" actual
> '
>
> -test_expect_success 'fail if attr magic is used places not implemented' '
> +test_expect_success 'fail if attr magic is used in places not implemented' '
> # The main purpose of this test is to check that we actually fail
> # when you attempt to use attr magic in commands that do not implement
> - # attr magic. This test does not advocate git-add to stay that way,
> - # though, but git-add is convenient as it has its own internal pathspec
> - # parsing.
> - test_must_fail git add ":(attr:labelB)" 2>actual &&
> + # attr magic. This test does not advocate check-ignore to stay that way.
> + # When you teach the command to grok the pathspec, you need to find
> + # another command to replace it for the test.
> + test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
> test_i18ngrep "magic not supported" actual
> '
>
> +test_expect_success 'check that attr magic works for git stash push' '
> + cat <<-\EOF >expect &&
> + A sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git stash push --include-untracked -- ":(exclude,attr:labelB)" &&
> + git stash show --include-untracked --name-status >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --all' '
> + cat <<-\EOF >expect &&
> + sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git add --all ":(exclude,attr:labelB)" &&
> + git diff --name-only --cached >actual &&
> + git restore -W -S . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add -u' '
> + cat <<-\EOF >expect &&
> + sub/fileA
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + >sub/fileA &&
> + >sub/fileB &&
> + git add -u ":(exclude,attr:labelB)" &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . && rm sub/new* &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add <path>' '
> + cat <<-\EOF >expect &&
> + fileA
> + fileB
> + sub/fileA
> + EOF
> + >fileA &&
> + >fileB &&
> + >sub/fileA &&
> + >sub/fileB &&
> + git add ":(exclude,attr:labelB)sub/*" &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git -add .' '
> + cat <<-\EOF >expect &&
> + sub/fileA
> + EOF
> + >fileA &&
> + >fileB &&
> + >sub/fileA &&
> + >sub/fileB &&
> + cd sub &&
> + git add . ":(exclude,attr:labelB)" &&
> + cd .. &&
> + git diff --name-only --cached >actual &&
> + git restore -S -W . &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'check that attr magic works for git add --pathspec-from-file' '
> + cat <<-\EOF >pathspec_file &&
> + :(exclude,attr:labelB)
> + EOF
> + cat <<-\EOF >expect &&
> + sub/newFileA-foo
> + EOF
> + >sub/newFileA-foo &&
> + >sub/newFileB-foo &&
> + git add --all --pathspec-from-file=pathspec_file &&
> + git diff --name-only --cached >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.42.0.869.gea05f2083d-goog
>