attr: enable attr pathspec magic for git-add and git-stash

Allow users to limit or exclude files based on file attributes
during git-add and git-stash.

For example, the chromium project would like to use

    $ git add . ':(exclude,attr:submodule)'

as submodules are managed by an external tool, forbidding end users
to record changes with "git add".  Allowing "git add" to often
records changes that users do not want in their commits.

This commit 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, we fix a bug in prefix_magic() where attr values were
unintentionally removed.  This was triggerable when parse_pathspec()
is called with PATHSPEC_PREFIX_ORIGIN as a flag, which was the case
for git-stash (Bug originally filed here [*])

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.

[Reference]
 * https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)

Signed-off-by: Joanna Wang <jojwang@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This commit is contained in:
Joanna Wang 2023-11-03 16:34:48 +00:00 committed by Junio C Hamano
parent 692be87cbb
commit 1164c7232e
4 changed files with 139 additions and 18 deletions

View file

@ -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 * Check the "pathspec '%s' did not match any files" block
* below before enabling new magic. * below before enabling new magic.
*/ */
parse_pathspec(&pathspec, PATHSPEC_ATTR, parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_FULL | PATHSPEC_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH, PATHSPEC_SYMLINK_LEADING_PATH,
prefix, argv); prefix, argv);
@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
if (pathspec.nr) if (pathspec.nr)
die(_("'%s' and pathspec arguments cannot be used together"), "--pathspec-from-file"); 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_PREFER_FULL |
PATHSPEC_SYMLINK_LEADING_PATH, PATHSPEC_SYMLINK_LEADING_PATH,
prefix, pathspec_from_file, pathspec_file_nul); 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_LITERAL |
PATHSPEC_GLOB | PATHSPEC_GLOB |
PATHSPEC_ICASE | PATHSPEC_ICASE |
PATHSPEC_EXCLUDE); PATHSPEC_EXCLUDE |
PATHSPEC_ATTR);
for (i = 0; i < pathspec.nr; i++) { for (i = 0; i < pathspec.nr; i++) {
const char *path = pathspec.items[i].match; const char *path = pathspec.items[i].match;

3
dir.c
View file

@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,
PATHSPEC_LITERAL | PATHSPEC_LITERAL |
PATHSPEC_GLOB | PATHSPEC_GLOB |
PATHSPEC_ICASE | PATHSPEC_ICASE |
PATHSPEC_EXCLUDE); PATHSPEC_EXCLUDE |
PATHSPEC_ATTR);
for (i = 0; i < pathspec->nr; i++) { for (i = 0; i < pathspec->nr; i++) {
const struct pathspec_item *item = &pathspec->items[i]; const struct pathspec_item *item = &pathspec->items[i];

View file

@ -109,16 +109,37 @@ static struct pathspec_magic {
{ PATHSPEC_ATTR, '\0', "attr" }, { 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; /* No magic was found in element, just add prefix magic */
strbuf_addstr(sb, ":("); if (!magic) {
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) strbuf_addf(sb, ":(prefix:%d)", prefixlen);
if (magic & pathspec_magic[i].bit) { return;
if (sb->buf[sb->len - 1] != '(') }
strbuf_addch(sb, ',');
strbuf_addstr(sb, pathspec_magic[i].name); /*
* At this point, we know 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) {
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); strbuf_addf(sb, ",prefix:%d)", prefixlen);
} }
@ -493,7 +514,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
struct strbuf sb = STRBUF_INIT; struct strbuf sb = STRBUF_INIT;
/* Preserve the actual prefix length of each pattern */ /* 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); strbuf_addstr(&sb, match);
item->original = strbuf_detach(&sb, NULL); item->original = strbuf_detach(&sb, NULL);

View file

@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '
fileSetLabel label fileSetLabel label
fileValue label=foo fileValue label=foo
fileWrongLabel label☺ fileWrongLabel label☺
newFileA* labelA
newFileB* labelB
EOF EOF
echo fileSetLabel label1 >sub/.gitattributes && echo fileSetLabel label1 >sub/.gitattributes &&
git add .gitattributes sub/.gitattributes && git add .gitattributes sub/.gitattributes &&
git commit -m "add attributes" 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' ' test_expect_success 'check specific set attr' '
cat <<-\EOF >expect && cat <<-\EOF >expect &&
fileSetLabel fileSetLabel
@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '
test_expect_success 'check unspecified attr' ' test_expect_success 'check unspecified attr' '
cat <<-\EOF >expect && cat <<-\EOF >expect &&
.gitattributes .gitattributes
.gitignore
fileA fileA
fileAB fileAB
fileAC fileAC
@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '
test_expect_success 'check unspecified attr (2)' ' test_expect_success 'check unspecified attr (2)' '
cat <<-\EOF >expect && cat <<-\EOF >expect &&
HEAD:.gitattributes HEAD:.gitattributes
HEAD:.gitignore
HEAD:fileA HEAD:fileA
HEAD:fileAB HEAD:fileAB
HEAD:fileAC HEAD:fileAC
@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '
test_expect_success 'check multiple unspecified attr' ' test_expect_success 'check multiple unspecified attr' '
cat <<-\EOF >expect && cat <<-\EOF >expect &&
.gitattributes .gitattributes
.gitignore
fileC fileC
fileNoLabel fileNoLabel
fileWrongLabel fileWrongLabel
@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
test_i18ngrep "Only one" actual 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 # 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 # 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, # attr magic. This test does not advocate check-ignore to stay that way.
# though, but git-add is convenient as it has its own internal pathspec # When you teach the command to grok the pathspec, you need to find
# parsing. # another command to replace it for the test.
test_must_fail git add ":(attr:labelB)" 2>actual && test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
test_i18ngrep "magic not supported" 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_expect_success 'abort on giving invalid label on the command line' '
test_must_fail git ls-files . ":(attr:☺)" test_must_fail git ls-files . ":(attr:☺)"
' '