[Bug] Pathspec matching breaks the add command

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.

On Mon, Sep 17, 2018 at 3:55 AM <smaudet@sebastianaudet.com> wrote:

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.

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

On Mon, Sep 17 2018, Duy Nguyen wrote:

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.

On Mon, Sep 17, 2018 at 10:51 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

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(-)

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

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

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

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(-)