Joanna Wang <jojwang@xxxxxxxxxx> writes: > This lets users limit added/stashed files or exclude files based on file > attributes. For example, the chromium project would like to use > this like "git add --all ':(exclude,attr:submodule)'", as submodules > are managed in a unique way and often results in submodule changes > that users do not want in their commits. > ... Everything else before the following line was written quite well (perhaps except for the commit title on the Subject: header). Very pleasant to see in the first iteration of a new contributor. > Any bugs this may trigger will be fixed in follow-up patches. This is rather a poor statement to make, as it hints that there are known breakages this change will reveal that you are not telling us, although I suspect that it is not the case. But in case there are such existing-but-not-revealed bugs, the above is not how the world works around here. Instead, any existing code whose bug has been hidden behind the GUARD_PATHSPEC() and known to be revealed with this change should be fixed with N preliminary clean-up patches, and then finally this patch would become the last [N+1/N+1] step of such a N+1 patch series. If no bugs are known to be revealed by applying this code change, then we should just drop the above comment. It is common to see a patch that changes behaviour without intending to break anything has unintended and unfortunate side effects, and we cannot really avoid it. The fallouts from such a change will be fixed after they are discovered, and that is not something we need or want to repeat at the end of every commit message ;-) > I have tested this thoroughly with different flags, non-root directories, > and other magic pathspecs, but may be unaware of some edge cases. These are good things to add to the new test this patch adds. Your one time testing will not protect the current code that works for these undocumented cases from future breakages, but when made into a part of the test suite, it will. > diff --git a/builtin/add.c b/builtin/add.c > index c27254a5cd..ef0b8d55fd 100644 > --- a/builtin/add.c > +++ b/builtin/add.c > @@ -424,7 +424,7 @@ int cmd_add(int argc, const char **argv, const char *prefix) > * Check the "pathspec '%s' did not match any files" block > * below before enabling new magic. > */ > - parse_pathspec(&pathspec, PATHSPEC_ATTR, > + parse_pathspec(&pathspec, 0, > PATHSPEC_PREFER_FULL | > PATHSPEC_SYMLINK_LEADING_PATH, > prefix, argv); > @@ -504,7 +504,8 @@ int cmd_add(int argc, const char **argv, const char *prefix) > PATHSPEC_LITERAL | > PATHSPEC_GLOB | > PATHSPEC_ICASE | > - PATHSPEC_EXCLUDE); > + PATHSPEC_EXCLUDE | > + PATHSPEC_ATTR); Both hunks look quite as expected. Looking good. > diff --git a/dir.c b/dir.c > index 8486e4d56f..55c9607b1a 100644 > --- a/dir.c > +++ b/dir.c > @@ -2173,13 +2173,7 @@ static int exclude_matches_pathspec(const char *path, int pathlen, > if (!pathspec || !pathspec->nr) > return 0; > > - GUARD_PATHSPEC(pathspec, > - PATHSPEC_FROMTOP | > - PATHSPEC_MAXDEPTH | > - PATHSPEC_LITERAL | > - PATHSPEC_GLOB | > - PATHSPEC_ICASE | > - PATHSPEC_EXCLUDE); > + GUARD_PATHSPEC(pathspec, PATHSPEC_ALL_MAGIC); Hmph, is it a good idea in general to use ALL_MAGIC in guard? The programmer who wrote this not just promises that everything in the current set of PATHSPEC_FOO bits are supported in this codepath right now, but also says any *new* PATHSPEC_FOO magic will be automatically supported. How does the updated code guarantee it? > @@ -239,14 +253,18 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' ' > test_i18ngrep "Only one" actual > ' > > -test_expect_success 'fail if attr magic is used places not implemented' ' > - # The main purpose of this test is to check that we actually fail > - # when you attempt to use attr magic in commands that do not implement > - # attr magic. This test does not advocate git-add to stay that way, > - # though, but git-add is convenient as it has its own internal pathspec > - # parsing. > - test_must_fail git add ":(attr:labelB)" 2>actual && > - test_i18ngrep "magic not supported" actual As the comment says, this test is primarily about making sure that parse_pathspec() that limits the allowed pathspec magic and GUARD_PATHSPEC() that forbids certain magic are working as expected. It is strongly preferrable, instead of butchering this test that guards these two mechanisms from being broken, to find a command that still has some restriction on the magic it allows, and use it to make sure they still trigger and give "magic not supported" error message. IOW, do not remove the above test, but find something other than "add" that is suitable to still follow the original intention of the test. It is, of course, very much welcome to add a new test below to protect the new feature. IIRC, "add --all" and "add -u" used somewhat different codepaths to find and update the changed paths. Don't we need a test each for both new paths (which "--all" is about and these two new files created in the test are) and also tracked paths (which "-u" would try to enumerate and update the index with)? For that matter, wouldn't "add . ':(exclude)...'" be also something we need to check? Or do these three all use pretty-much the same codepath under the hood? > +test_expect_success 'check that attr magic works for git-add' ' > + # attr magic was previously blocked for git-add. With attr magic > + # enabled for git-add, add a basic test to make sure it works as > + # expected and add more tests if more bugs are discovered. These three lines are unwanted. "was X, now Y" and "do Z now" may belong to the log message where previous state and new state are captured in the commit. But not in the tracked contents. 6 months down the road, when reading t6135 in order to further polish the tests, nobody cares if "add" did not support the attr magic in the past. The title of the test already says the test is about attr magic used in pathspec fed to "git add", so "add a basic test" is unnecessary to say. > + cat <<-\EOF >expect && > + sub/newFileA-foo > + EOF > + touch sub/newFileA-foo && > + touch sub/newFileB-foo && Unless it matters that these files have recent timestamps, do not use "touch", merely to ensure presence of a file. We often use a simple redirection >sub/newFileA-foo && for such purpose (I thought we had it somewhere in the coding guidelines document, but if not, we should add it). > + git add --all ":(exclude,attr:labelB)" && > + git diff --name-only --cached >actual && > + test_cmp expect actual > ' So we have two new paths (I do not offhand know if there are existing paths that are already tracked), and one is with label and the other is without. We tell "add" to grab all paths except for the paths with the label and see that we do not see the one with the label. OK. Thanks.
