[Bug] Pathspec matching breaks the add command

[Bug] Pathspec matching breaks the add command

[smaudet]

The following:

git add -u :\(glob,attr:-someAttr\):src/**

Produces an error that, according to the source code, should never be visible to the user. This attribute/pathspec *should* be supported according to the documentation provided by git:

fatal: BUG:builtin/add.c:498: unsupported magic 40

It looks like the documentation claims more features than the source code supports, perhaps incorrectly because I think you shouldn't be restricted to a set of attributes, and the source code
doesn't properly do its job anyways and never handles this scenario.

This should be fixed, if I have any time I'll look into what it would take to submit a patch, but I don't have the time for this right now.

Re: [Bug] Pathspec matching breaks the add command

[Duy Nguyen]

On Mon, Sep 17, 2018 at 3:55 AM <smaudet@sebastianaudet.com> wrote:
>
> The following:
>
> git add -u :\(glob,attr:-someAttr\):src/**
>
> Produces an error that, according to the source code, should never be visible to the user. This attribute/pathspec *should* be supported according to the documentation provided by git:
>
> fatal: BUG:builtin/add.c:498: unsupported magic 40

Brandon, b0db704652 (pathspec: allow querying for attributes -
2017-03-13) added attr support to match_pathspec(), but this add.c
manipulates pathspec directly and (I think) does not support 'attr'
attribute, which is correctly caught here.

I think we need to update the parse_pathspec() call in this file to
declare 'attr' not supported. Then we get a friendlier error (or you
make this code work too, that's even better). You may want to go
through all parse_pathspec() call and make sure that if the pathspec
is not consumed only by match_pathspec() then it should reject 'attr',
otherwise we'll get this "BUG" again.
-- 
Duy

Brandon Williams's E-Mail bouncing

[Ævar Arnfjörð Bjarmason]

[Sorry about the double send in private, forgot to CC the list]

On Mon, Sep 17 2018, Duy Nguyen wrote:

> Brandon, b0db704652[...]

I noticed a few days ago that CC's to bmwill@google.com bounce. Has he
left Google, and if so is he still interested in being CC'd on Git stuff
(maybe he'll chime in), or not interested?

Aside from that particular address bouncing (or not, maybe Google's MX
just dislikes me), it would be nice if git
{format-patch,send-email,check-contacts} & the .mailmap format would
support and understand some way to map addresses to
e.g. noreply@example.com, and prune them out appropriately. We have a
lot of addresses from past authors which bounce, and where no current
contact address is known.

Re: Brandon Williams's E-Mail bouncing

[Stefan Beller]

On Mon, Sep 17, 2018 at 10:51 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> Aside from that particular address bouncing [...] it would be nice if git
> {format-patch,send-email,check-contacts} & the .mailmap format would
> support and understand some way to map addresses to
> e.g. noreply@example.com, and prune them out appropriately. We have a
> lot of addresses from past authors which bounce, and where no current
> contact address is known.

Tombstones, eh?

For send-email I would very much prefer the proactive warning instead
of silently dropping that email address as I would want to know, so I can
try to follow up somehow and deal with it as-is.

I think the main purpose of the mail map file until now was to consolidate
multiple identities into one (e.g. misspellings or capitalisation issues in
names, different email addresses for all the same person), now you want to
extend it to also contain more information about an author-ident, such as
emails being active? Or recording if someone is not interested in the project
anymore? I think recording that an email bouncesl is just duplicating
information
that can be easily checked. Recording that a particular contributor doesn't
have time/interest any more is slightly different, but IMHO the mail map file
is also not very well suited for that endeavor.

Oh, the above paragraph is written with git - the tool - in mind, not
the git.git
repository where we can (ab)use our own tooling. ;-)

So mapping all bouncing emails to no-reply@example.com would map them
to the same author ident, which we would not want. (e.g. we'd still want to
know how many different people contributed so far, hence keep them separate).
We'd need to have unique email addresses that still explain their intent,
i.e. <user>+no-reply@example.org would work?

Thanks,
Stefan

[PATCH] add: do not accept pathspec magic 'attr'

[Nguyễn Thái Ngọc Duy]

Commit b0db704652 (pathspec: allow querying for attributes -
2017-03-13) adds new pathspec magic 'attr' but only with
match_pathspec(). "git add" has some pathspec related code that still
does not know about 'attr' and will bail out:

    $ git add ':(attr:foo)'
    fatal: BUG:dir.c:1584: unsupported magic 40

A better solution would be making this code support 'attr'. But I
don't know how much work is needed (I'm not familiar with this new
magic). For now, let's simply reject this magic with a friendlier
message:

    $ git add ':(attr:foo)'
    fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'

Reported-by: smaudet@sebastianaudet.com
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Since Brandon is currently unreachable, let's do this for now. I
 might eventually find time to go over pathspec code and see if I can
 add 'attr' support to the rest of the commands, but no promise.

 builtin/add.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/add.c b/builtin/add.c
index 9916498a29..0b64bcdebe 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -454,7 +454,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, 0,
+	parse_pathspec(&pathspec, PATHSPEC_ATTR,
 		       PATHSPEC_PREFER_FULL |
 		       PATHSPEC_SYMLINK_LEADING_PATH,
 		       prefix, argv);
-- 
2.19.0.rc0.337.ge906d732e7

Re: [PATCH] add: do not accept pathspec magic 'attr'

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Commit b0db704652 (pathspec: allow querying for attributes -
> 2017-03-13) adds new pathspec magic 'attr' but only with
> match_pathspec(). "git add" has some pathspec related code that still
> does not know about 'attr' and will bail out:
>
>     $ git add ':(attr:foo)'
>     fatal: BUG:dir.c:1584: unsupported magic 40
>
> A better solution would be making this code support 'attr'. But I
> don't know how much work is needed (I'm not familiar with this new
> magic). For now, let's simply reject this magic with a friendlier
> message:
>
>     $ git add ':(attr:foo)'
>     fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'
>
> Reported-by: smaudet@sebastianaudet.com
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Since Brandon is currently unreachable, let's do this for now.

Thanks.  This certainly make it better than the status quo.

Re: [PATCH] add: do not accept pathspec magic 'attr'

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Commit b0db704652 (pathspec: allow querying for attributes -
>> 2017-03-13) adds new pathspec magic 'attr' but only with
>> match_pathspec(). "git add" has some pathspec related code that still
>> does not know about 'attr' and will bail out:
>>
>>     $ git add ':(attr:foo)'
>>     fatal: BUG:dir.c:1584: unsupported magic 40
>>
>> A better solution would be making this code support 'attr'. But I
>> don't know how much work is needed (I'm not familiar with this new
>> magic). For now, let's simply reject this magic with a friendlier
>> message:
>>
>>     $ git add ':(attr:foo)'
>>     fatal: :(attr:foo): pathspec magic not supported by this command: 'attr'
>>
>> Reported-by: smaudet@sebastianaudet.com
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>> ---
>>  Since Brandon is currently unreachable, let's do this for now.
>
> Thanks.  This certainly make it better than the status quo.

And of course, there is an interesting glitch found after I almost
finish day's integration cycle X-<.

-- >8 --
Subject: [PATCH] fixup! add: do not accept pathspec magic 'attr'

[Add the following when squashing]

Update t6135 so that the expected error message is from the
"graceful" rejection codepath, not "oops, we were supposed to reject
the request to trigger this magic" codepath.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t6135-pathspec-with-attrs.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index 77b8cef661..e436a73962 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -164,11 +164,11 @@ test_expect_success 'fail if attr magic is used places not implemented' '
 	# 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 "unsupported magic" actual
+	test_i18ngrep "magic not supported" actual
 '
 
 test_expect_success 'abort on giving invalid label on the command line' '
 	test_must_fail git ls-files . ":(attr:☺)"
 '
-- 
2.19.0-216-g2d3b1c576c