Junio C Hamano <gitster@xxxxxxxxx> writes:
>> -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
> ...
> 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.
We can do something like the attached, instead of discarding the
protection from future breakage. You're welcome to find another
command and magic that are less likely to be implemented than
check-ignore with attr magic and replace them, of course. I just
eyeballed the hits from
$ git grep -A2 parse_pathspec\(
and picked one that has pathspec magic restriction (i.e., the second
parameter is not zero) and appears near the surface (i.e., the one
in blame.c for example is a bad target, because it is to ensure that
a pathspec that is internally created out of an end-user provided
pathname is treated only literally, and is impossible to pass a
pathspec with offending magic to that codepath from the outside) to
make it easier to pass offending magic from the command line.
A tangent (to the topic of testing, but relevant to the whole
patch). I notice 'stash' is mentioned on the topic, but I do not
see changes to the codepath that is specific to 'stash', and changes
to the tests do not demonstrate existing breakage. The lack of code
changes probably is because something shared, which is pretected
with magic guard lifted by the patch, is called from 'stash' as well
as 'add', or something? Whatever the reason is, it would be better
to document it in the proposed log message, and have a test to make
sure that the command works with the attr magic.
Alternatively, leave the mention of 'stash' out of the patch, if it
is not affected, perhaps?
In any case, here is a replacement for the removed test.
diff --git c/t/t6135-pathspec-with-attrs.sh w/t/t6135-pathspec-with-attrs.sh
index f70c395e75..b450cb5b3a 100755
--- c/t/t6135-pathspec-with-attrs.sh
+++ w/t/t6135-pathspec-with-attrs.sh
@@ -242,10 +242,10 @@ test_expect_success 'fail on multiple attr specifiers in one pathspec item' '
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 &&
+ # attr magic. This test does not advocate check-ignore to stay that way.
+ # When you teach the command to grok the pathspec, you need to find
+ # another commnad to replace it for the test.
+ test_must_fail git check-ignore ":(attr:labelB)" 2>actual &&
test_i18ngrep "magic not supported" actual
'
