{"thread":{"id":"60314","subject":"[PATCH 1/1] Allow attr magic for git-add, git-stash.","startedAt":"2023-10-05T23:28:39Z","lastAt":"2023-11-09T23:27:06Z","messageCount":19,"participants":["Joanna Wang","Junio C Hamano","Ruben Safir","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"482731","messageId":"20231005232423.1834298-1-jojwang@google.com","threadId":"60314","inReplyTo":null,"subject":"[PATCH 1/1] Allow attr magic for git-add, git-stash.","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-10-05T23:23:28Z","receivedAt":"2023-10-05T23:28:39Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit added/stashed files or exclude files based on file\nattributes. For example, the chromium project would like to use\nthis like \"git add --all ':(exclude,attr:submodule)'\", as submodules\nare managed in a unique way and often results in submodule changes\nthat users do not want in their commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add/stash, which was previously\nblocked (for stash/add) by GUARD_PATHSPEC and (for add only) a pathspec\nmask in parse_pathspec()).\n\nWith this patch, attr is supported for the two commands. It is possible\nthat when the attr pathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13), \n\"PATHSPEC_ATTR\" was just unintentionally left out of a few\nGUARD_PATHSPEC() invocations. Later, to get a more user-friendly error\nmessage when attr was used with git-add, PATHSPEC_ATTR was added as a\nmask to git-add's invocation of parse_pathspec() in\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\n\nAny bugs this may trigger will be fixed in follow-up patches.\n\nSigned-off-by: Joanna Wang \n---\n\nI have tested this thoroughly with different flags, non-root directories,\nand other magic pathspecs, but may be unaware of some edge cases.\n\nThe GUARD_PATHSPEC() in dir.c is a potential part of the call stack of\nfill_directory(), which is called by add, stash, grep, ls-files,\nsparse-checkout, and status.\nHowever, this patch only seems to add new behavior to git-add and git-stash.\ngrep, ls-files, and status already support attr pathspecs and running some of\nthese commands with the debugger confirms this GUARD_PATHSPEC() is never reached.\nBut again I am not aware of all edge cases. For sparse-checkout I'm not sure\nif/how pathspec can be used, but either way did not see new behavior.\n\nFor context, attr magic support for git-add is part of work discussed here:\nhttps://lore.kernel.org/git/xmqqfs2yjgvr.fsf@gitster.g/T/#t\n\n builtin/add.c | 5 +++--\n dir.c | 8 +-------\n t/t6135-pathspec-with-attrs.sh | 34 ++++++++++++++++++++++++++--------\n 3 files changed, 30 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c27254a5cd..ef0b8d55fd 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 8486e4d56f..55c9607b1a 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2173,13 +2173,7 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \tif (!pathspec || !pathspec->nr)\n \t\treturn 0;\n \n-\tGUARD_PATHSPEC(pathspec,\n-\t\t PATHSPEC_FROMTOP |\n-\t\t PATHSPEC_MAXDEPTH |\n-\t\t PATHSPEC_LITERAL |\n-\t\t PATHSPEC_GLOB |\n-\t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\tGUARD_PATHSPEC(pathspec, PATHSPEC_ALL_MAGIC);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e11d8cb119 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,23 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +161,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +187,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +213,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,14 +253,18 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n-\t# The main purpose of this test is to check that we actually fail\n-\t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n-\ttest_i18ngrep \"magic not supported\" actual\n+test_expect_success 'check that attr magic works for git-add' '\n+\t# attr magic was previously blocked for git-add. With attr magic\n+\t# enabled for git-add, add a basic test to make sure it works as\n+\t# expected and add more tests if more bugs are discovered.\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\ttouch sub/newFileA-foo &&\n+\ttouch sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'abort on giving invalid label on the command line' '\n-- \n2.42.0.582.g8ccd20d70d-goog\n\n"},{"id":"482734","messageId":"xmqqedi8bhf1.fsf@gitster.g","threadId":"60314","inReplyTo":"20231005232423.1834298-1-jojwang@google.com","subject":"Re: [PATCH 1/1] Allow attr magic for git-add, git-stash.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-06T02:42:26Z","receivedAt":"2023-10-06T02:42:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n> This lets users limit added/stashed files or exclude files based on file\n> attributes. For example, the chromium project would like to use\n> this like \"git add --all ':(exclude,attr:submodule)'\", as submodules\n> are managed in a unique way and often results in submodule changes\n> that users do not want in their commits.\n> ...\n\nEverything else before the following line was written quite well\n(perhaps except for the commit title on the Subject: header). Very\npleasant to see in the first iteration of a new contributor.\n\n> Any bugs this may trigger will be fixed in follow-up patches.\n\nThis is rather a poor statement to make, as it hints that there are\nknown breakages this change will reveal that you are not telling us,\nalthough I suspect that it is not the case.\n\nBut in case there are such existing-but-not-revealed bugs, the above\nis not how the world works around here. Instead, any existing code\nwhose bug has been hidden behind the GUARD_PATHSPEC() and known to\nbe revealed with this change should be fixed with N preliminary\nclean-up patches, and then finally this patch would become the last\n[N+1/N+1] step of such a N+1 patch series.\n\nIf no bugs are known to be revealed by applying this code change,\nthen we should just drop the above comment. It is common to see a\npatch that changes behaviour without intending to break anything has\nunintended and unfortunate side effects, and we cannot really avoid\nit. The fallouts from such a change will be fixed after they are\ndiscovered, and that is not something we need or want to repeat at\nthe end of every commit message ;-)\n\n> I have tested this thoroughly with different flags, non-root directories,\n> and other magic pathspecs, but may be unaware of some edge cases.\n\nThese are good things to add to the new test this patch adds. Your\none time testing will not protect the current code that works for\nthese undocumented cases from future breakages, but when made into\na part of the test suite, it will.\n\n> diff --git a/builtin/add.c b/builtin/add.c\n> index c27254a5cd..ef0b8d55fd 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t * Check the \"pathspec '%s' did not match any files\" block\n> \t * below before enabling new magic.\n> \t */\n> -\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n> +\tparse_pathspec(&pathspec, 0,\n> \t\t PATHSPEC_PREFER_FULL |\n> \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n> \t\t prefix, argv);\n> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\t\t PATHSPEC_LITERAL |\n> \t\t\t PATHSPEC_GLOB |\n> \t\t\t PATHSPEC_ICASE |\n> -\t\t\t PATHSPEC_EXCLUDE);\n> +\t\t\t PATHSPEC_EXCLUDE |\n> +\t\t\t PATHSPEC_ATTR);\n\nBoth hunks look quite as expected. Looking good.\n\n> diff --git a/dir.c b/dir.c\n> index 8486e4d56f..55c9607b1a 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2173,13 +2173,7 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n> \tif (!pathspec || !pathspec->nr)\n> \t\treturn 0;\n> \n> -\tGUARD_PATHSPEC(pathspec,\n> -\t\t PATHSPEC_FROMTOP |\n> -\t\t PATHSPEC_MAXDEPTH |\n> -\t\t PATHSPEC_LITERAL |\n> -\t\t PATHSPEC_GLOB |\n> -\t\t PATHSPEC_ICASE |\n> -\t\t PATHSPEC_EXCLUDE);\n> +\tGUARD_PATHSPEC(pathspec, PATHSPEC_ALL_MAGIC);\n\nHmph, is it a good idea in general to use ALL_MAGIC in guard? The\nprogrammer who wrote this not just promises that everything in the\ncurrent set of PATHSPEC_FOO bits are supported in this codepath\nright now, but also says any *new* PATHSPEC_FOO magic will be\nautomatically supported. How does the updated code guarantee it?\n\n> @@ -239,14 +253,18 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n> \ttest_i18ngrep \"Only one\" actual\n> '\n> \n> -test_expect_success 'fail if attr magic is used places not implemented' '\n> -\t# The main purpose of this test is to check that we actually fail\n> -\t# when you attempt to use attr magic in commands that do not implement\n> -\t# attr magic. This test does not advocate git-add to stay that way,\n> -\t# though, but git-add is convenient as it has its own internal pathspec\n> -\t# parsing.\n> -\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n> -\ttest_i18ngrep \"magic not supported\" actual\n\nAs the comment says, this test is primarily about making sure that\nparse_pathspec() that limits the allowed pathspec magic and\nGUARD_PATHSPEC() that forbids certain magic are working as expected.\n\nIt is strongly preferrable, instead of butchering this test that\nguards these two mechanisms from being broken, to find a command\nthat still has some restriction on the magic it allows, and use it\nto make sure they still trigger and give \"magic not supported\" error\nmessage. IOW, do not remove the above test, but find something\nother than \"add\" that is suitable to still follow the original\nintention of the test.\n\nIt is, of course, very much welcome to add a new test below to\nprotect the new feature. IIRC, \"add --all\" and \"add -u\" used\nsomewhat different codepaths to find and update the changed paths.\n\nDon't we need a test each for both new paths (which \"--all\" is about\nand these two new files created in the test are) and also tracked\npaths (which \"-u\" would try to enumerate and update the index with)?\nFor that matter, wouldn't \"add . ':(exclude)...'\" be also something\nwe need to check? Or do these three all use pretty-much the same\ncodepath under the hood?\n\n> +test_expect_success 'check that attr magic works for git-add' '\n> +\t# attr magic was previously blocked for git-add. With attr magic\n> +\t# enabled for git-add, add a basic test to make sure it works as\n> +\t# expected and add more tests if more bugs are discovered.\n\nThese three lines are unwanted.\n\n\"was X, now Y\" and \"do Z now\" may belong to the log message where\nprevious state and new state are captured in the commit. But not in\nthe tracked contents. 6 months down the road, when reading t6135 in\norder to further polish the tests, nobody cares if \"add\" did not\nsupport the attr magic in the past. The title of the test already\nsays the test is about attr magic used in pathspec fed to \"git add\",\nso \"add a basic test\" is unnecessary to say.\n\n> +\tcat <<-\\EOF >expect &&\n> +\tsub/newFileA-foo\n> +\tEOF\n> +\ttouch sub/newFileA-foo &&\n> +\ttouch sub/newFileB-foo &&\n\nUnless it matters that these files have recent timestamps, do not\nuse \"touch\", merely to ensure presence of a file. We often use a\nsimple redirection\n\n\t>sub/newFileA-foo &&\n\nfor such purpose (I thought we had it somewhere in the coding\nguidelines document, but if not, we should add it).\n\n> +\tgit add --all \":(exclude,attr:labelB)\" &&\n> +\tgit diff --name-only --cached >actual &&\n> +\ttest_cmp expect actual\n> '\n\nSo we have two new paths (I do not offhand know if there are\nexisting paths that are already tracked), and one is with label and\nthe other is without. We tell \"add\" to grab all paths except for\nthe paths with the label and see that we do not see the one with the\nlabel. OK.\n\nThanks.\n"},{"id":"482744","messageId":"xmqqttr3adgg.fsf@gitster.g","threadId":"60314","inReplyTo":"xmqqedi8bhf1.fsf@gitster.g","subject":"Re: [PATCH 1/1] Allow attr magic for git-add, git-stash.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-06T17:05:35Z","receivedAt":"2023-10-06T17:05:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> -test_expect_success 'fail if attr magic is used places not implemented' '\n>> -\t# The main purpose of this test is to check that we actually fail\n>> -\t# when you attempt to use attr magic in commands that do not implement\n>> -\t# attr magic. This test does not advocate git-add to stay that way,\n>> -\t# though, but git-add is convenient as it has its own internal pathspec\n>> -\t# parsing.\n>> -\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n>> -\ttest_i18ngrep \"magic not supported\" actual\n> ...\n> IOW, do not remove the above test, but find something\n> other than \"add\" that is suitable to still follow the original\n> intention of the test.\n\nWe can do something like the attached, instead of discarding the\nprotection from future breakage. You're welcome to find another\ncommand and magic that are less likely to be implemented than\ncheck-ignore with attr magic and replace them, of course. I just\neyeballed the hits from\n\n $ git grep -A2 parse_pathspec\\(\n\nand picked one that has pathspec magic restriction (i.e., the second\nparameter is not zero) and appears near the surface (i.e., the one\nin blame.c for example is a bad target, because it is to ensure that\na pathspec that is internally created out of an end-user provided\npathname is treated only literally, and is impossible to pass a\npathspec with offending magic to that codepath from the outside) to\nmake it easier to pass offending magic from the command line.\n\nA tangent (to the topic of testing, but relevant to the whole\npatch). I notice 'stash' is mentioned on the topic, but I do not\nsee changes to the codepath that is specific to 'stash', and changes\nto the tests do not demonstrate existing breakage. The lack of code\nchanges probably is because something shared, which is pretected\nwith magic guard lifted by the patch, is called from 'stash' as well\nas 'add', or something? Whatever the reason is, it would be better\nto document it in the proposed log message, and have a test to make\nsure that the command works with the attr magic.\n\nAlternatively, leave the mention of 'stash' out of the patch, if it\nis not affected, perhaps?\n\nIn any case, here is a replacement for the removed test.\n\n\ndiff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..b450cb5b3a 100755\n--- c/t/t6135-pathspec-with-attrs.sh\n+++ w/t/t6135-pathspec-with-attrs.sh\n@@ -242,10 +242,10 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n test_expect_success 'fail if attr magic is used places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another commnad to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n"},{"id":"482779","messageId":"20231007002811.2337315-1-jojwang@google.com","threadId":"60314","inReplyTo":"xmqqttr3adgg.fsf@gitster.g","subject":"[PATCH 1/1] add: Enable attr pathspec magic for git-add.","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-10-07T00:28:10Z","receivedAt":"2023-10-07T00:28:22Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit added files or exclude files based on file\nattributes. For example, the chromium project would like to use\nthis like \"git add --all ':(exclude,attr:submodule)'\", as submodules\nare managed in a unique way and often results in submodule changes\nthat users do not want in their commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\n\nWith this patch, attr is supported. It is possible that when the attr\npathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13), \n\"PATHSPEC_ATTR\" was just unintentionally left out of a few\nGUARD_PATHSPEC() invocations. Later, to get a more user-friendly error\nmessage when attr was used with git-add, PATHSPEC_ATTR was added as a\nmask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\n\ngit-stash which goes through the same GUARD_PATHSPEC(), currently\ndoes not work with attr. So a PATHSPEC_ATTR mask has been added\nto its parse_pathspec and parse_pathspec_file()\n\nSigned-off-by: Joanna Wang \n---\n\n> (perhaps except for the commit title on the Subject: header).\nAdded 'add:' and specified it is 'patchpsec' magic, but lmk if i'm still missing something.\n\n> This is rather a poor statement to make, as it hints that there are\n> known breakages this change will reveal that you are not telling us,\n>although I suspect that it is not the case.\nYes, sorry I did not mean to make this hint. I was trying to express that\nif bugs did come up, we would not abandon this effort and expect someone\nelse to fix it. I have removed this line. I realized I also don't want\nto say we will forever be on the hook for fixing every related bug that comes up.\n\n> These are good things to add to the new test this patch adds.\nTests added.\n\n>Hmph, is it a good idea in general to use ALL_MAGIC in guard?\nMy original reasoning was using ALL_MAGIC would prevent new magic pathspecs\nfrom being unintentionally left out of commands and if a new magic does not\nwork with a command, whoever adds it should change ALL_MAGIC back to listing\nthem individually in the same patch. But yes, listing them individually seems\nsafer. Better to accidentally leave magic out that can be added with intention\nlater rather than make it easy to accidentally leave unsupported magic in.\nI switched it back to listing individually.\n\n>It is strongly preferrable, instead of butchering this test that\n>guards these two mechanisms from being broken, to find a command\n>that still has some restriction on the magic it allows, and use it\n>to make sure they still trigger and give \"magic not supported\" error\n>message.\nI added the test back with git-stash, which, after more testing i discovered it\nactually did not fully work with attr. More info below.\n\n>Unless it matters that these files have recent timestamps, do not\n>use \"touch\", merely to ensure presence of a file. We often use a\n>simple redirection.\nDone\n\n>A tangent (to the topic of testing, but relevant to the whole\n>patch). I notice 'stash' is mentioned on the topic, but I do not\n>see changes to the codepath that is specific to 'stash', and changes\n>to the tests do not demonstrate existing breakage. The lack of code\n>changes probably is because something shared, which is pretected\n>with magic guard lifted by the patch, is called from 'stash' as well\n>as 'add', or something?\nYes, the previous patch enabled attr with git-stash which was only blocked\nat the shared dir.c GUARD_PATHSPEC level. I thought attr was working for git-stash\nbut I was wrong after a few more manual tests.\nSo I added PATHSPEC_ATTR as a mask to stash's parse_pathspec and parse_pathspec_file.\n\n builtin/add.c | 7 ++-\n builtin/stash.c | 4 +-\n dir.c | 13 ++--\n t/t6135-pathspec-with-attrs.sh | 109 +++++++++++++++++++++++++++++++--\n 4 files changed, 117 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c27254a5cd..2de83964a3 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1ad496985a..9c77d3e4e4 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1760,7 +1760,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\tparse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n+\tparse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n \t\t prefix, argv);\n \n \tif (pathspec_from_file) {\n@@ -1773,7 +1773,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t\tif (ps.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&ps, 0,\n+\t\tparse_pathspec_file(&ps, PATHSPEC_ATTR,\n \t\t\t\t PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n \t} else if (pathspec_file_nul) {\ndiff --git a/dir.c b/dir.c\nindex 8486e4d56f..9bf9b53ca5 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2174,12 +2174,13 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\treturn 0;\n \n \tGUARD_PATHSPEC(pathspec,\n-\t\t PATHSPEC_FROMTOP |\n-\t\t PATHSPEC_MAXDEPTH |\n-\t\t PATHSPEC_LITERAL |\n-\t\t PATHSPEC_GLOB |\n-\t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+ PATHSPEC_FROMTOP |\n+ PATHSPEC_MAXDEPTH |\n+ PATHSPEC_LITERAL |\n+ PATHSPEC_GLOB |\n+ PATHSPEC_ICASE |\n+ PATHSPEC_EXCLUDE |\n+ PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..531b4f4d5e 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,100 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate stash push to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another commnad to replace it for the test.\n+\ttest_must_fail git stash push \":(attr:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"magic not supported\" actual\n+'\n+\n+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '\n+\t# This is like the test above but for attr magic pass in via --pathspec-from-file.\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(attr:labelB)\n+\tEOF\n+\ttest_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git' '\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.609.gbb76f46606-goog\n\n"},{"id":"482780","messageId":"20231007005042.2997590-1-jojwang@google.com","threadId":"60314","inReplyTo":"20231007002811.2337315-1-jojwang@google.com","subject":"Re:[PATCH 1/1] add: Enable attr pathspec magic for git-add.","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-10-07T00:50:42Z","receivedAt":"2023-10-07T00:50:57Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":">+test_expect_success 'check that attr magic works for git' '\n>+'\nSorry, i forgot to take this out. I can fix it in the next pathset after\nthe next round of review.\n\n\n"},{"id":"482789","messageId":"xmqqbkda4ubp.fsf@gitster.g","threadId":"60314","inReplyTo":"20231007002811.2337315-1-jojwang@google.com","subject":"Re: [PATCH 1/1] add: Enable attr pathspec magic for git-add.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-07T10:10:02Z","receivedAt":"2023-10-07T10:10:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n> Added 'add:' and specified it is 'patchpsec' magic, but lmk if i'm still missing something.\n\nIf you run\n\n $ git shortlog --no-merges v2.42.0..\n\nor something on your branch to see how well its subject blends with\nothers, you'd start wanting to (1) downcase \"Enable\", and (2) omit\nthe full-stop after the title, to make it look similar to others.\n\n> diff --git a/builtin/stash.c b/builtin/stash.c\n> index 1ad496985a..9c77d3e4e4 100644\n> --- a/builtin/stash.c\n> +++ b/builtin/stash.c\n> @@ -1760,7 +1760,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n> \t\t}\n> \t}\n> \n> -\tparse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n> +\tparse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n> \t\t prefix, argv);\n\nThis becomes necessary because \"stash\" later goes through other\nplaces this patch touches (e.g., dir.c::exclude_matches_pathspec()?)\nthat happened to be serving as a guard for another code \"stash\" has\nthat is not prepared to handle attribute magic? Do you know what\nexactly is not ready, so that perhaps others can help figuring out\nhow to make it ready for the attr magic?\n\n> diff --git a/dir.c b/dir.c\n> index 8486e4d56f..9bf9b53ca5 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2174,12 +2174,13 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n> \t\treturn 0;\n> \n> \tGUARD_PATHSPEC(pathspec,\n> -\t\t PATHSPEC_FROMTOP |\n> -\t\t PATHSPEC_MAXDEPTH |\n> -\t\t PATHSPEC_LITERAL |\n> -\t\t PATHSPEC_GLOB |\n> -\t\t PATHSPEC_ICASE |\n> -\t\t PATHSPEC_EXCLUDE);\n> + PATHSPEC_FROMTOP |\n> + PATHSPEC_MAXDEPTH |\n> + PATHSPEC_LITERAL |\n> + PATHSPEC_GLOB |\n> + PATHSPEC_ICASE |\n> + PATHSPEC_EXCLUDE |\n> + PATHSPEC_ATTR);\n\nWhy this reindent?\n\n> @@ -239,16 +254,100 @@ test_expect_success 'fail on multiple attr specifie\n> \ttest_i18ngrep \"Only one\" actual\n> '\n> \n> -test_expect_success 'fail if attr magic is used places not implemented' '\n> +test_expect_success 'fail if attr magic is used in places not implemented' '\n> \t# The main purpose of this test is to check that we actually fail\n> \t# when you attempt to use attr magic in commands that do not implement\n> -\t# attr magic. This test does not advocate git-add to stay that way,\n> -\t# though, but git-add is convenient as it has its own internal pathspec\n> -\t# parsing.\n> -\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n> +\t# attr magic. This test does not advocate stash push to stay that way.\n> +\t# When you teach the command to grok the pathspec, you need to find\n> +\t# another commnad to replace it for the test.\n\nThere is a typo here. Please do not expect your reviewers to always\noffer a perfect solution to your problem and blindly copy what they\nfed you. Instead, just like other project participants try to find\nbugs and improvement opportunities in your patch, please lend them\nsanity-checking eyeballs in return ;-)\n"},{"id":"483084","messageId":"20231011202008.609921-1-jojwang@google.com","threadId":"60314","inReplyTo":"xmqqbkda4ubp.fsf@gitster.g","subject":"[PATCH 1/1] add: enable attr pathspec magic","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-10-11T20:20:07Z","receivedAt":"2023-10-11T20:20:15Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit added files or exclude files based on file\nattributes. For example, the chromium project would like to use\nthis like \"git add --all ':(exclude,attr:submodule)'\", as submodules\nare managed in a unique way and often results in submodule changes\nthat users do not want in their commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\n\nWith this patch, attr is supported. It is possible that when the attr\npathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few\nGUARD_PATHSPEC() invocations. Later, to get a more user-friendly error\nmessage when attr was used with git-add, PATHSPEC_ATTR was added as a\nmask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\n\ngit-stash which goes through the same GUARD_PATHSPEC(), currently\ndoes not work with attr.\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u\nSo a PATHSPEC_ATTR mask has been added to its parse_pathspec and\nparse_pathspec_file().\n\nSigned-off-by: Joanna Wang \n---\n\n> Do you know what exactly is not ready, so that perhaps others can\n> help figuring out how to make it ready for the attr magic?\nI have filed a bug which describes what I have found:\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/T/#u\n\n> Why this reindent?\nFixed\n\n> There is a typo here.\nFixed\n\n\n builtin/add.c | 7 ++-\n builtin/stash.c | 7 ++-\n dir.c | 3 +-\n t/t6135-pathspec-with-attrs.sh | 106 +++++++++++++++++++++++++++++++--\n 4 files changed, 112 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex c27254a5cd..2de83964a3 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 1ad496985a..af1b3a7146 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -1760,7 +1760,10 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\tparse_pathspec(&ps, 0, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n+ // For parse_pathspec() and parse_pathspec_file() below, PATHSPEC_ATTR is blocked for git stash\n+ // because the magic attr does not get properly parsed when the PATHSPEC_PREFIX_ORIGIN flag is\n+ // used, resulting in incorrect file filtering for attr.\n+\tparse_pathspec(&ps, PATHSPEC_ATTR, PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n \t\t prefix, argv);\n \n \tif (pathspec_from_file) {\n@@ -1773,7 +1776,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,\n \t\tif (ps.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&ps, 0,\n+\t\tparse_pathspec_file(&ps, PATHSPEC_ATTR,\n \t\t\t\t PATHSPEC_PREFER_FULL | PATHSPEC_PREFIX_ORIGIN,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n \t} else if (pathspec_file_nul) {\ndiff --git a/dir.c b/dir.c\nindex 8486e4d56f..ee3f3777df 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..403ee5e6b6 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,97 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate stash push to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git stash push \":(attr:labelB)\" 2>actual &&\n+\ttest_i18ngrep \"magic not supported\" actual\n+'\n+\n+test_expect_success 'fail if attr magic is used in --pathspec-from-file when not implemented' '\n+\t# This is like the test above but for attr magic passed in via --pathspec-from-file.\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(attr:labelB)\n+\tEOF\n+\ttest_must_fail git stash push --pathspec-from-file=pathspec_file 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.609.gbb76f46606-goog\n\n"},{"id":"484329","messageId":"20231102015554.34089-1-jojwang@google.com","threadId":"60314","inReplyTo":"20231011202008.609921-1-jojwang@google.com","subject":"[PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-02T01:55:53Z","receivedAt":"2023-11-02T01:56:02Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit files or exclude files based on file\nattributes during git-add and git-stash.\nFor example, the chromium project would like to use this like\n\"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\nunique way and often results in submodule changes that users do not want in\ntheir commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add and git-stash, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\nHowever, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\nThis was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\nwhich was the case for git-stash. More details here:\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/\n\nIt is possible that when the attr pathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.\n\nLater, to get a more user-friendly error message when attr was used with git-add,\nPATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\nHowever, this user-friendly error message was never added for git-stash.\n\nSigned-off-by: Joanna Wang \n\n---\n\nPTAL.\nI've updated this to include the fix for git-stash. I was initially going to fix this bug [1]\nseparately, but I thought it would make more sense to put everything in one patch so attr\ncould be enabled for both git-add and git-stash and tested.\n\n[1] https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/\n\n\n\n builtin/add.c | 7 ++-\n dir.c | 3 +-\n pathspec.c | 11 +++-\n t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n 4 files changed, 118 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 5126d2ede3..d46e4d10e9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 16fdb03f2a..4d1cd039be 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex bb1efe1f39..40bd8a8819 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -109,7 +109,7 @@ static struct pathspec_magic {\n \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n };\n \n-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n {\n \tint i;\n \tstrbuf_addstr(sb, \":(\");\n@@ -118,6 +118,13 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n \t\t\tif (sb->buf[sb->len - 1] != '(')\n \t\t\t\tstrbuf_addch(sb, ',');\n \t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t\t\tif (pathspec_magic[i].bit & PATHSPEC_ATTR) {\n+\t\t\t\t/* extract and insert the attr magic value */\n+\t\t\t\tchar* p = strstr(element, \"attr:\");\n+\t\t\t\tchar buff[128];\n+\t\t\t\tsscanf(p, \"attr%[^)|,]\", buff);\n+\t\t\t\tstrbuf_addstr(sb, buff);\n+\t\t\t}\n \t\t}\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n@@ -493,7 +500,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e46fa176ed 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git stash push' '\n+\tcat <<-\\EOF >expect &&\n+\tA\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n+\tgit stash show --include-untracked --name-status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.820.g83a721a137-goog\n\n"},{"id":"484330","messageId":"xmqqfs1ooil2.fsf@gitster.g","threadId":"60314","inReplyTo":"20231102015554.34089-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-02T05:00:25Z","receivedAt":"2023-11-02T05:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n> I've updated this to include the fix for git-stash. I was\n> initially going to fix this bug [1] separately, but I thought it\n> would make more sense to put everything in one patch so attr could\n> be enabled for both git-add and git-stash and tested.\n\nI think that is perfectly fine, as long as it is described well in\nthe proposed log message what is done in the patch, and how two\nseemingly different things done in the patch are so closely linked\nthat it makes sense to do so in a single patch.\n\nWill queue.\n\nThanks.\n\n> diff --git a/pathspec.c b/pathspec.c\n> index bb1efe1f39..40bd8a8819 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -109,7 +109,7 @@ static struct pathspec_magic {\n> \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n> };\n> \n> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n> {\n> \tint i;\n> \tstrbuf_addstr(sb, \":(\");\n> @@ -118,6 +118,13 @@ static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n> \t\t\tif (sb->buf[sb->len - 1] != '(')\n> \t\t\t\tstrbuf_addch(sb, ',');\n> \t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n> +\t\t\tif (pathspec_magic[i].bit & PATHSPEC_ATTR) {\n> +\t\t\t\t/* extract and insert the attr magic value */\n> +\t\t\t\tchar* p = strstr(element, \"attr:\");\n\nIn our codebase, the asterisk sticks to the variable, not to the\ntype, i.e.\n\n\t\t\t\tchar *p = strstr(element, \"attr:\");\n\n> +\t\t\t\tchar buff[128];\n\nWill this fixed-size buffer make an inviting target for script\nkiddies, as pathspec often come from the command line and under\ncontrol by whoever is making a request?\n\nAren't we going to copy what is in the elt literally from where\nattr: appears up to the next delimiter like ',', ')'? I am not sure\nif we need a separate buffer. Would something along this line work?\n\n\t\t\t\tstrbuf_add(sb, p, strcspn(p, \",)\");\n\nI am unsure if this \"unparsing\" is the way we want to go. For one,\njust like %(attr:foo) can take an arbitrary end-user supplied string\nin the \"foo\" part, we can have new pathspec magic in the future that\nwill take an arbitrary end-user supplied string as its value. And the\nabove unparsing code will be utterly confused when that value\nhappens to contain \"attr:\" as its substring, e.g., \n\n $ git add . \":(exclude,frotz:diattr:0,attr:submodule)\"\n\nAlso, do we (and if not right now, then do we want to) support\ngiving more than one attribute?\n\n $ git add \":(attr:text,attr:small)*.py\"\n\nNot supporting multiple attributes would be OK for now, but when it\nbecomes needed, the \"unparse using the bits in the element_magic\"\ndoes not look like the right approach.\n\nIndeed, if you are going to pass the original \"elt\" string *anyway*,\nyou have all the info in there that you need. I wonder if it makes\nsense to get rid of the \"unsigned magic\" bitset from the parameter,\nand discard the loop over the pathspec_magic[] array and do\nsomething like:\n\n - if the original \"elt\" already has some magic (i.e., begins\n with \":(\" and not in the literal mode), then copy them\n literally and textually from \":(\" up to the closing \")\", but\n also insert the necessary \"prefix:\" magic;\n\n - otherwise, give \":(prefix:)\" magic.\n\nwithout even attempting to unparse the bits in \"element_magic\"?\n\nThanks.\n"},{"id":"484374","messageId":"20231102175316.2229631-1-jojwang@google.com","threadId":"60314","inReplyTo":"xmqqfs1ooil2.fsf@gitster.g","subject":"[PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-02T17:53:15Z","receivedAt":"2023-11-02T17:53:24Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit files or exclude files based on file\nattributes during git-add and git-stash.\nFor example, the chromium project would like to use this like\n\"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\nunique way and often results in submodule changes that users do not want in\ntheir commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add and git-stash, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\nHowever, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\nThis was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\nwhich was the case for git-stash. More details here:\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/\n\nIt is possible that when the attr pathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.\n\nLater, to get a more user-friendly error message when attr was used with git-add,\nPATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\nHowever, this user-friendly error message was never added for git-stash.\n\nSigned-off-by: Joanna Wang \n\n---\n\n> Indeed, if you are going to pass the original \"elt\" string *anyway*,\n> you have all the info in there that you need. I wonder if it makes\n> sense to get rid of the \"unsigned magic\" bitset from the parameter,\nThis was my initial strategy but ran into trouble when the magic was\nin shorthand form. Upon closer look at how the shorthand works\n(e.g. shorthand and longhand can never mix so\n':!/(attr:chicken)file' would make <(attr:chicken)file> the match string)\nI tried this again by processing the forms separately.\nIt would still need both the element and element_magic, but I think it\naddresses the concerns with future changes where multiple magic match\nvalues could be allowed and the match values could be any string. These\nchanges would be fine as long as there is no overlap between magic that\ntakes a user-supplied value and magic that can be expressed in shorthand.\n\n\n builtin/add.c | 7 ++-\n dir.c | 3 +-\n pathspec.c | 25 +++++---\n t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n 4 files changed, 125 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 5126d2ede3..d46e4d10e9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 16fdb03f2a..4d1cd039be 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex bb1efe1f39..588a2cde4d 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -109,16 +109,23 @@ static struct pathspec_magic {\n \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n };\n \n-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n {\n-\tint i;\n-\tstrbuf_addstr(sb, \":(\");\n-\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n-\t\tif (magic & pathspec_magic[i].bit) {\n-\t\t\tif (sb->buf[sb->len - 1] != '(')\n-\t\t\t\tstrbuf_addch(sb, ',');\n-\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\tif (element[1] != '(') {\n+\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n+\t\tstrbuf_addstr(sb, \":(\");\n+\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n+\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n+\t\t\t\t\tstrbuf_addch(sb, ',');\n+\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t\t\t}\n \t\t}\n+\t} else {\n+\t\t/* For an element in longhand form, we simply copy everything up to the final ')' */\n+\t\tint len = strchr(element, ')') - element;\n+\t\tstrbuf_add(sb, element, len);\n+\t}\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n@@ -493,7 +500,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e46fa176ed 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git stash push' '\n+\tcat <<-\\EOF >expect &&\n+\tA\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n+\tgit stash show --include-untracked --name-status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.869.gea05f2083d-goog\n\n"},{"id":"484377","messageId":"xmqqil6jlu3m.fsf@gitster.g","threadId":"60314","inReplyTo":"20231102175316.2229631-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-02T21:32:13Z","receivedAt":"2023-11-02T21:32:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joanna Wang writes:\n\n>> Indeed, if you are going to pass the original \"elt\" string *anyway*,\n>> you have all the info in there that you need. I wonder if it makes\n>> sense to get rid of the \"unsigned magic\" bitset from the parameter,\n> This was my initial strategy but ran into trouble when the magic was\n> in shorthand form. Upon closer look at how the shorthand works\n> (e.g. shorthand and longhand can never mix so\n> ':!/(attr:chicken)file' would make <(attr:chicken)file> the match string)\n> I tried this again by processing the forms separately.\n> It would still need both the element and element_magic, but I think it\n> addresses the concerns with future changes where multiple magic match\n> values could be allowed and the match values could be any string.\n\nThe \"bits\" were acceptable for things like \"exclude\" and \"icase\"\nbecause it does not matter how many times you gave them and they do\nnot take any additional parameters, but attr is different in that it\ntakes a value, and multiple instances with different values can be\ngiven. It is lucky that we did not allow mixing the short and long\nforms ;-)\n\n> These changes would be fine as long as there is no overlap between\n> magic that takes a user-supplied value and magic that can be\n> expressed in shorthand.\n\nIndeed. Thanks for thinking this through.\n\n> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n> {\n> -\tint i;\n> -\tstrbuf_addstr(sb, \":(\");\n> -\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n> -\t\tif (magic & pathspec_magic[i].bit) {\n> -\t\t\tif (sb->buf[sb->len - 1] != '(')\n> -\t\t\t\tstrbuf_addch(sb, ',');\n> -\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n\nAt this point in the code, is it guaranteed that element[0] is ':'\nand never a NUL? Also is it guaranteed that element has ')'\nsomewhere later if element[1] is '('?\n\n\"Otherwise the caller would have failed to parse the pathspec magic\ninto the magic bits, and this helper function would not have been\ncalled\" is the answer I am expecting, but I didn't check if the\ncaller refrains from calling us. It would be better to have a brief\ncomment explaining why a seemingly loose parsing of element[] string\nis OK to save future readers from wondering the same thing as I did\nhere.\n\n> +\tif (element[1] != '(') {\n> +\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n> +\t\tstrbuf_addstr(sb, \":(\");\n> +\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n> +\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n> +\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n> +\t\t\t\t\tstrbuf_addch(sb, ',');\n> +\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n> +\t\t\t}\n> \t\t}\n> +\t} else {\n> +\t\t/* For an element in longhand form, we simply copy everything up to the final ')' */\n\nA comment that is a bit on the overly-long side.\n\n> +\t\tint len = strchr(element, ')') - element;\n> +\t\tstrbuf_add(sb, element, len);\n> +\t}\n> \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n> }\n\nCome to think of it, this part of the change could stand on its own\nas an independent bugfix. I wonder if this existing bug caused by\nfailing to copy the value of \"attr:\" is triggerable from a codepath\nthat already allows PATHSPEC_ATTR magic. Not absolutely required\nwhen the rest of the patch is in reasonably close to the finish\nline, but it would narrow the scope of the new feature proper to\ntreat this as a separate and independent fix, on which the new\nfature depends on.\n\nThanks for working on fixing this rather old bug. I think we should\nhave noticed when we added the support for the \"attr\" magic to the\npathspec API.\n\n"},{"id":"484381","messageId":"xmqq34xnlny7.fsf@gitster.g","threadId":"60314","inReplyTo":"xmqqil6jlu3m.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-02T23:45:04Z","receivedAt":"2023-11-02T23:45:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\t} else {\n>> +\t\t/* For an element in longhand form, we simply copy everything up to the final ')' */\n>\n> A comment that is a bit on the overly-long side.\n>\n>> +\t\tint len = strchr(element, ')') - element;\n>> +\t\tstrbuf_add(sb, element, len);\n\nIn practice, nobody sane would write a pathspec magic that is over\n2GB in size, so this would not matter unless we are facing a\npotential attacker, but as the third parameter strbuf_add() takes is\nof type size_t, it would not hurt to define \"len\" as the same type\nas well.\n\n> Thanks for working on fixing this rather old bug. I think we should\n> have noticed when we added the support for the \"attr\" magic to the\n> pathspec API.\n\n"},{"id":"484394","messageId":"20231103143508.896745-1-jojwang@google.com","threadId":"60314","inReplyTo":"xmqq34xnlny7.fsf@gitster.g","subject":"[PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-03T14:35:07Z","receivedAt":"2023-11-03T14:35:15Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit files or exclude files based on file\nattributes during git-add and git-stash.\nFor example, the chromium project would like to use this like\n\"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\nunique way and often results in submodule changes that users do not want in\ntheir commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add and git-stash, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\nHowever, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\nThis was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\nwhich was the case for git-stash.\n(But originally filed here:\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)\n\nFurthermore, while other commands hit this code path it did not result in unexpected\nbehavior because this bug only impacts the pathspec->items->original field which is\nNOT used to filter paths. However, git-stash does use pathspec->items->original when\nbuilding args used to call other git commands.\n(See add_pathspecs() usage and implementation in stash.c)\n\nIt is possible that when the attr pathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.\n\nLater, to get a more user-friendly error message when attr was used with git-add,\nPATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\nHowever, this user-friendly error message was never added for git-stash.\n\nSigned-off-by: Joanna Wang \n\n---\n\n> At this point in the code, is it guaranteed that element[0] is ':'\n> and never a NUL? Also is it guaranteed that element has ')'\n> somewhere later if element[1] is '('?\nNo sorry, we can only assume this if there was element magic detected\nby parse_element_magic(). So if element magic is not 0, we know the body\nis in the expected form (either long or short).\nI have added comments and a check for magic to guard against this.\n\n> I wonder if this existing bug caused by\n> failing to copy the value of \"attr:\" is triggerable from a codepath\n> that already allows PATHSPEC_ATTR magic.\nWhile there are other commands that go through the prefix_magic path (like `git-add -i`),\nAFAICT they are not actually impacted by the bug. The bug only impacts the value of\npathspec->items->original which is not used during pathspec matching. But git-stash\nuses `original` to pass in as args to other commands it calls. I've included this\ndetail in the description above.\n\n> but as the third parameter strbuf_add() takes is of type size_t, it would not\n> hurt to define \"len\" as the same type as well.\nThanks for spotting. fixed.\n\n builtin/add.c | 7 ++-\n dir.c | 3 +-\n pathspec.c | 36 ++++++++---\n t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n 4 files changed, 136 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 5126d2ede3..d46e4d10e9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 16fdb03f2a..4d1cd039be 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex bb1efe1f39..2f8721cc15 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -109,16 +109,34 @@ static struct pathspec_magic {\n \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n };\n \n-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n {\n-\tint i;\n-\tstrbuf_addstr(sb, \":(\");\n-\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n-\t\tif (magic & pathspec_magic[i].bit) {\n-\t\t\tif (sb->buf[sb->len - 1] != '(')\n-\t\t\t\tstrbuf_addch(sb, ',');\n-\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t/* No magic was found in element, just add prefix magic */\n+\tif (magic == 0) {\n+\t\tstrbuf_addf(sb, \":(prefix:%d)\", prefixlen);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * At this point we known that parse_element_magic() was able to extract some pathspec\n+\t * magic from element. So we know element is correctly formatted in either shorthand\n+\t * or longhand form\n+\t */\n+\tif (element[1] != '(') {\n+\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n+\t\tstrbuf_addstr(sb, \":(\");\n+\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n+\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n+\t\t\t\t\tstrbuf_addch(sb, ',');\n+\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t\t\t}\n \t\t}\n+\t} else {\n+\t\t/* For the longhand form, we copy everything up to the final ')' */\n+\t\tsize_t len = strchr(element, ')') - element;\n+\t\tstrbuf_add(sb, element, len);\n+\t}\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n@@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e46fa176ed 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git stash push' '\n+\tcat <<-\\EOF >expect &&\n+\tA\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n+\tgit stash show --include-untracked --name-status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.869.gea05f2083d-goog\n\n"},{"id":"484399","messageId":"20231103153128.GA8857@www2.mrbrklyn.com","threadId":"60314","inReplyTo":"20231103143508.896745-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Ruben Safir","fromEmail":"ruben@mrbrklyn.com","sentAt":"2023-11-03T15:31:28Z","receivedAt":"2023-11-03T15:39:20Z","isPatch":true,"sender":{"key":"ruben@mrbrklyn.com","avatar":null},"body":"Is that really you name or is it a bad joke?\n\n\nOn Fri, Nov 03, 2023 at 02:35:07PM +0000, Joanna Wang wrote:\n> This lets users limit files or exclude files based on file\n> attributes during git-add and git-stash.\n> For example, the chromium project would like to use this like\n> \"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\n> unique way and often results in submodule changes that users do not want in\n> their commits.\n> \n> This does not change any attr magic implementation. It is only adding\n> attr as an allowed pathspec in git-add and git-stash, which was previously\n> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\n> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\n> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\n> which was the case for git-stash.\n> (But originally filed here:\n> https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)\n> \n> Furthermore, while other commands hit this code path it did not result in unexpected\n> behavior because this bug only impacts the pathspec->items->original field which is\n> NOT used to filter paths. However, git-stash does use pathspec->items->original when\n> building args used to call other git commands.\n> (See add_pathspecs() usage and implementation in stash.c)\n> \n> It is possible that when the attr pathspec feature was first added in\n> b0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n> \"PATHSPEC_ATTR\" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.\n> \n> Later, to get a more user-friendly error message when attr was used with git-add,\n> PATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()\n> 84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\n> However, this user-friendly error message was never added for git-stash.\n> \n> Signed-off-by: Joanna Wang \n> \n> ---\n> \n> > At this point in the code, is it guaranteed that element[0] is ':'\n> > and never a NUL? Also is it guaranteed that element has ')'\n> > somewhere later if element[1] is '('?\n> No sorry, we can only assume this if there was element magic detected\n> by parse_element_magic(). So if element magic is not 0, we know the body\n> is in the expected form (either long or short).\n> I have added comments and a check for magic to guard against this.\n> \n> > I wonder if this existing bug caused by\n> > failing to copy the value of \"attr:\" is triggerable from a codepath\n> > that already allows PATHSPEC_ATTR magic.\n> While there are other commands that go through the prefix_magic path (like `git-add -i`),\n> AFAICT they are not actually impacted by the bug. The bug only impacts the value of\n> pathspec->items->original which is not used during pathspec matching. But git-stash\n> uses `original` to pass in as args to other commands it calls. I've included this\n> detail in the description above.\n> \n> > but as the third parameter strbuf_add() takes is of type size_t, it would not\n> > hurt to define \"len\" as the same type as well.\n> Thanks for spotting. fixed.\n> \n> builtin/add.c | 7 ++-\n> dir.c | 3 +-\n> pathspec.c | 36 ++++++++---\n> t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n> 4 files changed, 136 insertions(+), 18 deletions(-)\n> \n> diff --git a/builtin/add.c b/builtin/add.c\n> index 5126d2ede3..d46e4d10e9 100644\n> --- a/builtin/add.c\n> +++ b/builtin/add.c\n> @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t * Check the \"pathspec '%s' did not match any files\" block\n> \t * below before enabling new magic.\n> \t */\n> -\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n> +\tparse_pathspec(&pathspec, 0,\n> \t\t PATHSPEC_PREFER_FULL |\n> \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n> \t\t prefix, argv);\n> @@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\tif (pathspec.nr)\n> \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n> \n> -\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n> +\t\tparse_pathspec_file(&pathspec, 0,\n> \t\t\t\t PATHSPEC_PREFER_FULL |\n> \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n> \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n> @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n> \t\t\t PATHSPEC_LITERAL |\n> \t\t\t PATHSPEC_GLOB |\n> \t\t\t PATHSPEC_ICASE |\n> -\t\t\t PATHSPEC_EXCLUDE);\n> +\t\t\t PATHSPEC_EXCLUDE |\n> +\t\t\t PATHSPEC_ATTR);\n> \n> \t\tfor (i = 0; i < pathspec.nr; i++) {\n> \t\t\tconst char *path = pathspec.items[i].match;\n> diff --git a/dir.c b/dir.c\n> index 16fdb03f2a..4d1cd039be 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n> \t\t PATHSPEC_LITERAL |\n> \t\t PATHSPEC_GLOB |\n> \t\t PATHSPEC_ICASE |\n> -\t\t PATHSPEC_EXCLUDE);\n> +\t\t PATHSPEC_EXCLUDE |\n> +\t\t PATHSPEC_ATTR);\n> \n> \tfor (i = 0; i < pathspec->nr; i++) {\n> \t\tconst struct pathspec_item *item = &pathspec->items[i];\n> diff --git a/pathspec.c b/pathspec.c\n> index bb1efe1f39..2f8721cc15 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -109,16 +109,34 @@ static struct pathspec_magic {\n> \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n> };\n> \n> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n> {\n> -\tint i;\n> -\tstrbuf_addstr(sb, \":(\");\n> -\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n> -\t\tif (magic & pathspec_magic[i].bit) {\n> -\t\t\tif (sb->buf[sb->len - 1] != '(')\n> -\t\t\t\tstrbuf_addch(sb, ',');\n> -\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n> +\t/* No magic was found in element, just add prefix magic */\n> +\tif (magic == 0) {\n> +\t\tstrbuf_addf(sb, \":(prefix:%d)\", prefixlen);\n> +\t\treturn;\n> +\t}\n> +\n> +\t/*\n> +\t * At this point we known that parse_element_magic() was able to extract some pathspec\n> +\t * magic from element. So we know element is correctly formatted in either shorthand\n> +\t * or longhand form\n> +\t */\n> +\tif (element[1] != '(') {\n> +\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n> +\t\tstrbuf_addstr(sb, \":(\");\n> +\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n> +\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n> +\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n> +\t\t\t\t\tstrbuf_addch(sb, ',');\n> +\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n> +\t\t\t}\n> \t\t}\n> +\t} else {\n> +\t\t/* For the longhand form, we copy everything up to the final ')' */\n> +\t\tsize_t len = strchr(element, ')') - element;\n> +\t\tstrbuf_add(sb, element, len);\n> +\t}\n> \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n> }\n> \n> @@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n> \t\tstruct strbuf sb = STRBUF_INIT;\n> \n> \t\t/* Preserve the actual prefix length of each pattern */\n> -\t\tprefix_magic(&sb, prefixlen, element_magic);\n> +\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n> \n> \t\tstrbuf_addstr(&sb, match);\n> \t\titem->original = strbuf_detach(&sb, NULL);\n> diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\n> index f70c395e75..e46fa176ed 100755\n> --- a/t/t6135-pathspec-with-attrs.sh\n> +++ b/t/t6135-pathspec-with-attrs.sh\n> @@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n> \tfileSetLabel label\n> \tfileValue label=foo\n> \tfileWrongLabel label☺\n> +\tnewFileA* labelA\n> +\tnewFileB* labelB\n> \tEOF\n> \techo fileSetLabel label1 >sub/.gitattributes &&\n> \tgit add .gitattributes sub/.gitattributes &&\n> \tgit commit -m \"add attributes\"\n> '\n> \n> +test_expect_success 'setup .gitignore' '\n> +\tcat <<-\\EOF >.gitignore &&\n> +\tactual\n> +\texpect\n> +\tpathspec_file\n> +\tEOF\n> +\tgit add .gitignore &&\n> +\tgit commit -m \"add gitignore\"\n> +'\n> +\n> test_expect_success 'check specific set attr' '\n> \tcat <<-\\EOF >expect &&\n> \tfileSetLabel\n> @@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n> test_expect_success 'check unspecified attr' '\n> \tcat <<-\\EOF >expect &&\n> \t.gitattributes\n> +\t.gitignore\n> \tfileA\n> \tfileAB\n> \tfileAC\n> @@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n> test_expect_success 'check unspecified attr (2)' '\n> \tcat <<-\\EOF >expect &&\n> \tHEAD:.gitattributes\n> +\tHEAD:.gitignore\n> \tHEAD:fileA\n> \tHEAD:fileAB\n> \tHEAD:fileAC\n> @@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n> test_expect_success 'check multiple unspecified attr' '\n> \tcat <<-\\EOF >expect &&\n> \t.gitattributes\n> +\t.gitignore\n> \tfileC\n> \tfileNoLabel\n> \tfileWrongLabel\n> @@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n> \ttest_i18ngrep \"Only one\" actual\n> '\n> \n> -test_expect_success 'fail if attr magic is used places not implemented' '\n> +test_expect_success 'fail if attr magic is used in places not implemented' '\n> \t# The main purpose of this test is to check that we actually fail\n> \t# when you attempt to use attr magic in commands that do not implement\n> -\t# attr magic. This test does not advocate git-add to stay that way,\n> -\t# though, but git-add is convenient as it has its own internal pathspec\n> -\t# parsing.\n> -\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n> +\t# attr magic. This test does not advocate check-ignore to stay that way.\n> +\t# When you teach the command to grok the pathspec, you need to find\n> +\t# another command to replace it for the test.\n> +\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n> \ttest_i18ngrep \"magic not supported\" actual\n> '\n> \n> +test_expect_success 'check that attr magic works for git stash push' '\n> +\tcat <<-\\EOF >expect &&\n> +\tA\tsub/newFileA-foo\n> +\tEOF\n> +\t>sub/newFileA-foo &&\n> +\t>sub/newFileB-foo &&\n> +\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n> +\tgit stash show --include-untracked --name-status >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check that attr magic works for git add --all' '\n> +\tcat <<-\\EOF >expect &&\n> +\tsub/newFileA-foo\n> +\tEOF\n> +\t>sub/newFileA-foo &&\n> +\t>sub/newFileB-foo &&\n> +\tgit add --all \":(exclude,attr:labelB)\" &&\n> +\tgit diff --name-only --cached >actual &&\n> +\tgit restore -W -S . &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check that attr magic works for git add -u' '\n> +\tcat <<-\\EOF >expect &&\n> +\tsub/fileA\n> +\tEOF\n> +\t>sub/newFileA-foo &&\n> +\t>sub/newFileB-foo &&\n> +\t>sub/fileA &&\n> +\t>sub/fileB &&\n> +\tgit add -u \":(exclude,attr:labelB)\" &&\n> +\tgit diff --name-only --cached >actual &&\n> +\tgit restore -S -W . && rm sub/new* &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check that attr magic works for git add ' '\n> +\tcat <<-\\EOF >expect &&\n> +\tfileA\n> +\tfileB\n> +\tsub/fileA\n> +\tEOF\n> +\t>fileA &&\n> +\t>fileB &&\n> +\t>sub/fileA &&\n> +\t>sub/fileB &&\n> +\tgit add \":(exclude,attr:labelB)sub/*\" &&\n> +\tgit diff --name-only --cached >actual &&\n> +\tgit restore -S -W . &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check that attr magic works for git -add .' '\n> +\tcat <<-\\EOF >expect &&\n> +\tsub/fileA\n> +\tEOF\n> +\t>fileA &&\n> +\t>fileB &&\n> +\t>sub/fileA &&\n> +\t>sub/fileB &&\n> +\tcd sub &&\n> +\tgit add . \":(exclude,attr:labelB)\" &&\n> +\tcd .. &&\n> +\tgit diff --name-only --cached >actual &&\n> +\tgit restore -S -W . &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n> +\tcat <<-\\EOF >pathspec_file &&\n> +\t:(exclude,attr:labelB)\n> +\tEOF\n> +\tcat <<-\\EOF >expect &&\n> +\tsub/newFileA-foo\n> +\tEOF\n> +\t>sub/newFileA-foo &&\n> +\t>sub/newFileB-foo &&\n> +\tgit add --all --pathspec-from-file=pathspec_file &&\n> +\tgit diff --name-only --cached >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'abort on giving invalid label on the command line' '\n> \ttest_must_fail git ls-files . \":(attr:☺)\"\n> '\n> -- \n> 2.42.0.869.gea05f2083d-goog\n> \n\n-- \nSo many immigrant groups have swept through our town\nthat Brooklyn, like Atlantis, reaches mythological\nproportions in the mind of the world - RI Safir 1998\nhttp://www.mrbrklyn.com \n\nDRM is THEFT - We are the STAKEHOLDERS - RI Safir 2002\nhttp://www.nylxs.com - Leadership Development in Free Software\nhttp://www2.mrbrklyn.com/resources - Unpublished Archive \nhttp://www.coinhangout.com - coins!\nhttp://www.brooklyn-living.com \n\nBeing so tracked is for FARM ANIMALS and extermination camps, \nbut incompatible with living as a free human being. -RI Safir 2013\n\n"},{"id":"484405","messageId":"20231103163449.1578841-1-jojwang@google.com","threadId":"60314","inReplyTo":"20231103143508.896745-1-jojwang@google.com","subject":"[PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Joanna Wang","fromEmail":"jojwang@google.com","sentAt":"2023-11-03T16:34:48Z","receivedAt":"2023-11-03T16:34:56Z","isPatch":true,"sender":{"key":"jojwang@google.com","avatar":"https://avatars.githubusercontent.com/u/120585822?v=4"},"body":"This lets users limit files or exclude files based on file\nattributes during git-add and git-stash.\nFor example, the chromium project would like to use this like\n\"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\nunique way and often results in submodule changes that users do not want in\ntheir commits.\n\nThis does not change any attr magic implementation. It is only adding\nattr as an allowed pathspec in git-add and git-stash, which was previously\nblocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\nHowever, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\nThis was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\nwhich was the case for git-stash.\n(Bug originally filed here:\nhttps://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)\n\nFurthermore, while other commands hit this code path it did not result in unexpected\nbehavior because this bug only impacts the pathspec->items->original field which is\nNOT used to filter paths. However, git-stash does use pathspec->items->original when\nbuilding args used to call other git commands.\n(See add_pathspecs() usage and implementation in stash.c)\n\nIt is possible that when the attr pathspec feature was first added in\nb0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few GUARD_PATHSPEC() invocations.\n\nLater, to get a more user-friendly error message when attr was used with git-add,\nPATHSPEC_ATTR was added as a mask to git-add's invocation of parse_pathspec()\n84d938b732 (add: do not accept pathspec magic 'attr', 2018-09-18).\nHowever, this user-friendly error message was never added for git-stash.\n\nSigned-off-by: Joanna Wang \n\n---\n\nI fixed a typo above. \"(But originally\" -> \"(Bug originally\"\n\n> So if element magic is not 0, we know the body is in the expected\n> form (either long or short).\nAlso while this is still true AFAICT, I found an existing bug in parse_short_magic()\nwhere it does not handle paths that start with \":\" where \":\" is part of the\nfile name. e.g. \"git stash push -- ':chicken'\" is not handled correcly. But I feel\nthis is a seperate topic from this patch. So I'll file a bug to address it seperately.\n\n builtin/add.c | 7 ++-\n dir.c | 3 +-\n pathspec.c | 36 ++++++++---\n t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n 4 files changed, 136 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 5126d2ede3..d46e4d10e9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 16fdb03f2a..4d1cd039be 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex bb1efe1f39..2f8721cc15 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -109,16 +109,34 @@ static struct pathspec_magic {\n \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n };\n \n-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n+static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n {\n-\tint i;\n-\tstrbuf_addstr(sb, \":(\");\n-\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n-\t\tif (magic & pathspec_magic[i].bit) {\n-\t\t\tif (sb->buf[sb->len - 1] != '(')\n-\t\t\t\tstrbuf_addch(sb, ',');\n-\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t/* No magic was found in element, just add prefix magic */\n+\tif (magic == 0) {\n+\t\tstrbuf_addf(sb, \":(prefix:%d)\", prefixlen);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * At this point we known that parse_element_magic() was able to extract some pathspec\n+\t * magic from element. So we know element is correctly formatted in either shorthand\n+\t * or longhand form\n+\t */\n+\tif (element[1] != '(') {\n+\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n+\t\tstrbuf_addstr(sb, \":(\");\n+\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n+\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n+\t\t\t\t\tstrbuf_addch(sb, ',');\n+\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t\t\t}\n \t\t}\n+\t} else {\n+\t\t/* For the longhand form, we copy everything up to the final ')' */\n+\t\tsize_t len = strchr(element, ')') - element;\n+\t\tstrbuf_add(sb, element, len);\n+\t}\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n@@ -493,7 +511,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e46fa176ed 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git stash push' '\n+\tcat <<-\\EOF >expect &&\n+\tA\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n+\tgit stash show --include-untracked --name-status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.42.0.869.gea05f2083d-goog\n\n"},{"id":"484425","messageId":"xmqqsf5mgl0r.fsf@gitster.g","threadId":"60314","inReplyTo":"20231103163449.1578841-1-jojwang@google.com","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-04T05:11:48Z","receivedAt":"2023-11-04T05:11:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. The compiled result from this version looks quite good. As\nyou started a new round with typofix, let me start the final\nnitpicking.\n\nJoanna Wang writes:\n\n> This lets users limit files or exclude files based on file\n> attributes during git-add and git-stash.\n> For example, the chromium project would like to use this like\n> \"git add --all ':(exclude,attr:submodule)'\", as submodules are managed in a\n> unique way and often results in submodule changes that users do not want in\n> their commits.\n\nI would say\n\n Allow users to limit or exclude files based on file attributes\n during git-add and git-stash.\n\n For example, the chromium project would like to use \n\n $ git add . ':(exclude,attr:submodule)'\n\n as submodules are managed in a unique way and often results in\n submodule changes that users do not want in their commits.\n\nexcept that I would prefer to see \"in a unique way\" rephrased.\nWould \"are managed outside Git using external tools\" be a more\naccurate description?\n\nIt may be true that the chromium project handles submodules unlike\nall the other projects, but you guys being alone is not an important\npart of the reason why we might want this new feature to help users.\nWhen another project adopts the way how chromium manages the\nsubmodules, forbidding its end-users from running \"git add\" to\nrecord the changes to their submodules, you guys will no longer be\n\"unique\", but the need for this feature will still exist (and the\ndemand may even be stronger). The reason why this feature helps\nshould be stated here.\n\n> This does not change any attr magic implementation. It is only adding\n> attr as an allowed pathspec in git-add and git-stash, which was previously\n> blocked by GUARD_PATHSPEC and a pathspec mask in parse_pathspec()).\n\nAdd blank line here between paragraphs.\n\n> However, this does fix a bug in prefix_magic() where attr values were unintentionally removed.\n> This was hit whenever parse_pathspec() is called with PATHSPEC_PREFIX_ORIGIN as a flag,\n> which was the case for git-stash.\n> (Bug originally filed here:\n\nWrap overlong lines here and in the rest of the proposed log message.\n\n> diff --git a/pathspec.c b/pathspec.c\n> index bb1efe1f39..2f8721cc15 100644\n> --- a/pathspec.c\n> +++ b/pathspec.c\n> @@ -109,16 +109,34 @@ static struct pathspec_magic {\n> \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n> };\n> \n> -static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n> +static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic, const char *element)\n> {\n\nThis is overly long.\n\n> +\t/* No magic was found in element, just add prefix magic */\n> +\tif (magic == 0) {\n\nStyle:\n\n\tif (!magic) {\n\n> +\t\tstrbuf_addf(sb, \":(prefix:%d)\", prefixlen);\n> +\t\treturn;\n> +\t}\n> +\n\n> +\t/*\n> +\t * At this point we known that parse_element_magic() was able to extract some pathspec\n> +\t * magic from element. So we know element is correctly formatted in either shorthand\n> +\t * or longhand form\n> +\t */\n\nOverly long, with a typo. \"At this point, we know that ...\".\n\n> +\tif (element[1] != '(') {\n> +\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n> +\t\tstrbuf_addstr(sb, \":(\");\n> +\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n> +\t\t\tif ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n\nOverly long, with a stylo.\n\n> +\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n> +\t\t\t\t\tstrbuf_addch(sb, ',');\n> +\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n> +\t\t\t}\n> \t\t}\n> +\t} else {\n> +\t\t/* For the longhand form, we copy everything up to the final ')' */\n> +\t\tsize_t len = strchr(element, ')') - element;\n> +\t\tstrbuf_add(sb, element, len);\n> +\t}\n> \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n> }\n\nHere is what I ended up queuing.\n\nThanks.\n\n---- >8 -------- >8 -------- >8 -------- >8 ----\nFrom: Joanna Wang \nDate: Fri, 3 Nov 2023 16:34:48 +0000\nSubject: [PATCH] attr: enable attr pathspec magic for git-add and git-stash\n\nAllow users to limit or exclude files based on file attributes\nduring git-add and git-stash.\n\nFor example, the chromium project would like to use\n\n $ git add . ':(exclude,attr:submodule)'\n\nas submodules are managed by an external tool, forbidding end users\nto record changes with \"git add\". Allowing \"git add\" to often\nrecords changes that users do not want in their commits.\n\nThis commit does not change any attr magic implementation. It is\nonly adding attr as an allowed pathspec in git-add and git-stash,\nwhich was previously blocked by GUARD_PATHSPEC and a pathspec mask\nin parse_pathspec()).\n\nHowever, we fix a bug in prefix_magic() where attr values were\nunintentionally removed. This was triggerable when parse_pathspec()\nis called with PATHSPEC_PREFIX_ORIGIN as a flag, which was the case\nfor git-stash (Bug originally filed here [*])\n\nFurthermore, while other commands hit this code path it did not\nresult in unexpected behavior because this bug only impacts the\npathspec->items->original field which is NOT used to filter\npaths. However, git-stash does use pathspec->items->original when\nbuilding args used to call other git commands. (See add_pathspecs()\nusage and implementation in stash.c)\n\nIt is possible that when the attr pathspec feature was first added\nin b0db704652 (pathspec: allow querying for attributes, 2017-03-13),\n\"PATHSPEC_ATTR\" was just unintentionally left out of a few\nGUARD_PATHSPEC() invocations.\n\nLater, to get a more user-friendly error message when attr was used\nwith git-add, PATHSPEC_ATTR was added as a mask to git-add's\ninvocation of parse_pathspec() 84d938b732 (add: do not accept\npathspec magic 'attr', 2018-09-18). However, this user-friendly\nerror message was never added for git-stash.\n\n[Reference]\n * https://lore.kernel.org/git/CAMmZTi-0QKtj7Q=sbC5qhipGsQxJFOY-Qkk1jfkRYwfF5FcUVg@mail.gmail.com/)\n\nSigned-off-by: Joanna Wang \nSigned-off-by: Junio C Hamano \n---\n builtin/add.c | 7 ++-\n dir.c | 3 +-\n pathspec.c | 39 +++++++++---\n t/t6135-pathspec-with-attrs.sh | 108 +++++++++++++++++++++++++++++++--\n 4 files changed, 139 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 5126d2ede3..d46e4d10e9 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t * Check the \"pathspec '%s' did not match any files\" block\n \t * below before enabling new magic.\n \t */\n-\tparse_pathspec(&pathspec, PATHSPEC_ATTR,\n+\tparse_pathspec(&pathspec, 0,\n \t\t PATHSPEC_PREFER_FULL |\n \t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t prefix, argv);\n@@ -433,7 +433,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\tif (pathspec.nr)\n \t\t\tdie(_(\"'%s' and pathspec arguments cannot be used together\"), \"--pathspec-from-file\");\n \n-\t\tparse_pathspec_file(&pathspec, PATHSPEC_ATTR,\n+\t\tparse_pathspec_file(&pathspec, 0,\n \t\t\t\t PATHSPEC_PREFER_FULL |\n \t\t\t\t PATHSPEC_SYMLINK_LEADING_PATH,\n \t\t\t\t prefix, pathspec_from_file, pathspec_file_nul);\n@@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \t\t\t PATHSPEC_LITERAL |\n \t\t\t PATHSPEC_GLOB |\n \t\t\t PATHSPEC_ICASE |\n-\t\t\t PATHSPEC_EXCLUDE);\n+\t\t\t PATHSPEC_EXCLUDE |\n+\t\t\t PATHSPEC_ATTR);\n \n \t\tfor (i = 0; i < pathspec.nr; i++) {\n \t\t\tconst char *path = pathspec.items[i].match;\ndiff --git a/dir.c b/dir.c\nindex 16fdb03f2a..4d1cd039be 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2179,7 +2179,8 @@ static int exclude_matches_pathspec(const char *path, int pathlen,\n \t\t PATHSPEC_LITERAL |\n \t\t PATHSPEC_GLOB |\n \t\t PATHSPEC_ICASE |\n-\t\t PATHSPEC_EXCLUDE);\n+\t\t PATHSPEC_EXCLUDE |\n+\t\t PATHSPEC_ATTR);\n \n \tfor (i = 0; i < pathspec->nr; i++) {\n \t\tconst struct pathspec_item *item = &pathspec->items[i];\ndiff --git a/pathspec.c b/pathspec.c\nindex bb1efe1f39..d132eefac2 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -109,16 +109,37 @@ static struct pathspec_magic {\n \t{ PATHSPEC_ATTR, '\\0', \"attr\" },\n };\n \n-static void prefix_magic(struct strbuf *sb, int prefixlen, unsigned magic)\n+static void prefix_magic(struct strbuf *sb, int prefixlen,\n+\t\t\t unsigned magic, const char *element)\n {\n-\tint i;\n-\tstrbuf_addstr(sb, \":(\");\n-\tfor (i = 0; i < ARRAY_SIZE(pathspec_magic); i++)\n-\t\tif (magic & pathspec_magic[i].bit) {\n-\t\t\tif (sb->buf[sb->len - 1] != '(')\n-\t\t\t\tstrbuf_addch(sb, ',');\n-\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t/* No magic was found in element, just add prefix magic */\n+\tif (!magic) {\n+\t\tstrbuf_addf(sb, \":(prefix:%d)\", prefixlen);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * At this point, we know that parse_element_magic() was able\n+\t * to extract some pathspec magic from element. So we know\n+\t * element is correctly formatted in either shorthand or\n+\t * longhand form\n+\t */\n+\tif (element[1] != '(') {\n+\t\t/* Process an element in shorthand form (e.g. \":!/\") */\n+\t\tstrbuf_addstr(sb, \":(\");\n+\t\tfor (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n+\t\t\tif ((magic & pathspec_magic[i].bit) &&\n+\t\t\t (pathspec_magic[i].mnemonic)) {\n+\t\t\t\tif (sb->buf[sb->len - 1] != '(')\n+\t\t\t\t\tstrbuf_addch(sb, ',');\n+\t\t\t\tstrbuf_addstr(sb, pathspec_magic[i].name);\n+\t\t\t}\n \t\t}\n+\t} else {\n+\t\t/* For the longhand form, we copy everything up to the final ')' */\n+\t\tsize_t len = strchr(element, ')') - element;\n+\t\tstrbuf_add(sb, element, len);\n+\t}\n \tstrbuf_addf(sb, \",prefix:%d)\", prefixlen);\n }\n \n@@ -493,7 +514,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,\n \t\tstruct strbuf sb = STRBUF_INIT;\n \n \t\t/* Preserve the actual prefix length of each pattern */\n-\t\tprefix_magic(&sb, prefixlen, element_magic);\n+\t\tprefix_magic(&sb, prefixlen, element_magic, elt);\n \n \t\tstrbuf_addstr(&sb, match);\n \t\titem->original = strbuf_detach(&sb, NULL);\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex f70c395e75..e46fa176ed 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -64,12 +64,24 @@ test_expect_success 'setup .gitattributes' '\n \tfileSetLabel label\n \tfileValue label=foo\n \tfileWrongLabel label☺\n+\tnewFileA* labelA\n+\tnewFileB* labelB\n \tEOF\n \techo fileSetLabel label1 >sub/.gitattributes &&\n \tgit add .gitattributes sub/.gitattributes &&\n \tgit commit -m \"add attributes\"\n '\n \n+test_expect_success 'setup .gitignore' '\n+\tcat <<-\\EOF >.gitignore &&\n+\tactual\n+\texpect\n+\tpathspec_file\n+\tEOF\n+\tgit add .gitignore &&\n+\tgit commit -m \"add gitignore\"\n+'\n+\n test_expect_success 'check specific set attr' '\n \tcat <<-\\EOF >expect &&\n \tfileSetLabel\n@@ -150,6 +162,7 @@ test_expect_success 'check specific value attr (2)' '\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileA\n \tfileAB\n \tfileAC\n@@ -175,6 +188,7 @@ test_expect_success 'check unspecified attr' '\n test_expect_success 'check unspecified attr (2)' '\n \tcat <<-\\EOF >expect &&\n \tHEAD:.gitattributes\n+\tHEAD:.gitignore\n \tHEAD:fileA\n \tHEAD:fileAB\n \tHEAD:fileAC\n@@ -200,6 +214,7 @@ test_expect_success 'check unspecified attr (2)' '\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n+\t.gitignore\n \tfileC\n \tfileNoLabel\n \tfileWrongLabel\n@@ -239,16 +254,99 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '\n \ttest_i18ngrep \"Only one\" actual\n '\n \n-test_expect_success 'fail if attr magic is used places not implemented' '\n+test_expect_success 'fail if attr magic is used in places not implemented' '\n \t# The main purpose of this test is to check that we actually fail\n \t# when you attempt to use attr magic in commands that do not implement\n-\t# attr magic. This test does not advocate git-add to stay that way,\n-\t# though, but git-add is convenient as it has its own internal pathspec\n-\t# parsing.\n-\ttest_must_fail git add \":(attr:labelB)\" 2>actual &&\n+\t# attr magic. This test does not advocate check-ignore to stay that way.\n+\t# When you teach the command to grok the pathspec, you need to find\n+\t# another command to replace it for the test.\n+\ttest_must_fail git check-ignore \":(attr:labelB)\" 2>actual &&\n \ttest_i18ngrep \"magic not supported\" actual\n '\n \n+test_expect_success 'check that attr magic works for git stash push' '\n+\tcat <<-\\EOF >expect &&\n+\tA\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit stash push --include-untracked -- \":(exclude,attr:labelB)\" &&\n+\tgit stash show --include-untracked --name-status >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --all' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -W -S . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add -u' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add -u \":(exclude,attr:labelB)\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . && rm sub/new* &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add ' '\n+\tcat <<-\\EOF >expect &&\n+\tfileA\n+\tfileB\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tgit add \":(exclude,attr:labelB)sub/*\" &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git -add .' '\n+\tcat <<-\\EOF >expect &&\n+\tsub/fileA\n+\tEOF\n+\t>fileA &&\n+\t>fileB &&\n+\t>sub/fileA &&\n+\t>sub/fileB &&\n+\tcd sub &&\n+\tgit add . \":(exclude,attr:labelB)\" &&\n+\tcd .. &&\n+\tgit diff --name-only --cached >actual &&\n+\tgit restore -S -W . &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'check that attr magic works for git add --pathspec-from-file' '\n+\tcat <<-\\EOF >pathspec_file &&\n+\t:(exclude,attr:labelB)\n+\tEOF\n+\tcat <<-\\EOF >expect &&\n+\tsub/newFileA-foo\n+\tEOF\n+\t>sub/newFileA-foo &&\n+\t>sub/newFileB-foo &&\n+\tgit add --all --pathspec-from-file=pathspec_file &&\n+\tgit diff --name-only --cached >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'abort on giving invalid label on the command line' '\n \ttest_must_fail git ls-files . \":(attr:☺)\"\n '\n-- \n2.43.0-rc0\n\n"},{"id":"484427","messageId":"CAPig+cRR0PfWP0_uoevQnctSosb-k9cp3b08-sQNV5OPuQ2tAg@mail.gmail.com","threadId":"60314","inReplyTo":"xmqqsf5mgl0r.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-11-04T06:28:25Z","receivedAt":"2023-11-04T06:28:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano wrote:\n> > + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n>\n> Overly long, with a stylo.\n>\n> Here is what I ended up queuing.\n>\n> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n> + if ((magic & pathspec_magic[i].bit) &&\n> + (pathspec_magic[i].mnemonic)) {\n\nNit: The no-value-added parentheses around\n`pathspec_magic[i].mnemonic` can also be dropped.\n"},{"id":"484430","messageId":"xmqqfs1mgfcd.fsf@gitster.g","threadId":"60314","inReplyTo":"CAPig+cRR0PfWP0_uoevQnctSosb-k9cp3b08-sQNV5OPuQ2tAg@mail.gmail.com","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-04T07:14:26Z","receivedAt":"2023-11-04T07:14:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sat, Nov 4, 2023 at 1:12 AM Junio C Hamano wrote:\n>> > + if ((magic & pathspec_magic[i].bit) && (pathspec_magic[i].mnemonic != '\\0')) {\n>>\n>> Overly long, with a stylo.\n>>\n>> Here is what I ended up queuing.\n>>\n>> + for (int i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {\n>> + if ((magic & pathspec_magic[i].bit) &&\n>> + (pathspec_magic[i].mnemonic)) {\n>\n> Nit: The no-value-added parentheses around\n> `pathspec_magic[i].mnemonic` can also be dropped.\n\nYeah, thanks for spotting.\n"},{"id":"484681","messageId":"xmqqwmuqy0cb.fsf@gitster.g","threadId":"60314","inReplyTo":"xmqqsf5mgl0r.fsf@gitster.g","subject":"Re: [PATCH 1/1] attr: enable attr pathspec magic for git-add and git-stash","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-09T23:27:00Z","receivedAt":"2023-11-09T23:27:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Thanks. The compiled result from this version looks quite good. As\n> you started a new round with typofix, let me start the final\n> nitpicking.\n> ...\n> Here is what I ended up queuing.\n>\n> Thanks.\n\nAny comments on the \"counterproposal to fix nitpicks\"? I'd like to\nmake sure the original author is either OK with the update (in which\ncase I can just move what I have in my tree forward) or unhappy with\nit and plan to send in an update on their own (in which case I can\nwait for the next iteration), in a case like this.\n\nThanks.\n"}]}