Re: [BUG] :(attr) pathspecs can die("BUG") in the tree-walker

From: Brandon Williams <bmwill@google.com>
Date: May 9, 2017, 22:52 UTC
Message-ID: <20170509225219.GB106700@google.com>
In-Reply-To: <20170509222432.3dxt7osjt2zjtaiw@sigill.intra.peff.net>

On 05/09, Jeff King wrote:

Show 27 quoted lines

> I was playing with the new :(attr) pathspecs in the upcoming v2.13
> today, and noticed:
> 
>   $ git ls-files -- ':(attr:-diff)'
>   t/t0110/url-1
>   t/t0110/url-10
>   [etc]
> 
> So far so good.
> 
>   $ git ls-tree HEAD -- ':(attr:-diff)'
>   fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr'
> 
> Bummer, but I understand that sometimes the options need to be plumbed
> through to work everywhere.
> 
>   $ git log HEAD -- ':(attr:-diff)'
>   fatal: BUG:tree-walk.c:947: unsupported magic 40
> 
> Whoops. This is presumably ls-tree is protected, but I think we are
> missing a GUARD_PATHSPEC call somewhere.
> 
> This isn't a huge deal, as the correct behavior is probably to die like
> ls-tree does, but we probably shouldn't be hitting BUG assertions as a
> general rule.
> 
> -Peff

The die("BUG: ..."); is from a GAURD_PATHSPEC call. What really needs to happen is to update the magic_mask passed into the parse_pathspec call which initializes the pathspec object. Its this magic_mask which catches unsupported magic and prints a better message.

I guess this means I (or someone else :D) should audit all the parse_pathspec calls and ensure that 'attr' magic is turned off until we won't run into these GAURD_PATHSPEC die's.

-- 
Brandon Williams