{"thread":{"id":"45914","subject":"[BUG] :(attr) pathspecs can die(\"BUG\") in the tree-walker","startedAt":"2017-05-09T22:24:40Z","lastAt":"2017-05-10T01:26:00Z","messageCount":3,"participants":["Jeff King","Brandon Williams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"319232","messageId":"20170509222432.3dxt7osjt2zjtaiw@sigill.intra.peff.net","threadId":"45914","inReplyTo":null,"subject":"[BUG] :(attr) pathspecs can die(\"BUG\") in the tree-walker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-09T22:24:32Z","receivedAt":"2017-05-09T22:24:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I was playing with the new :(attr) pathspecs in the upcoming v2.13\ntoday, and noticed:\n\n  $ git ls-files -- ':(attr:-diff)'\n  t/t0110/url-1\n  t/t0110/url-10\n  [etc]\n\nSo far so good.\n\n  $ git ls-tree HEAD -- ':(attr:-diff)'\n  fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr'\n\nBummer, but I understand that sometimes the options need to be plumbed\nthrough to work everywhere.\n\n  $ git log HEAD -- ':(attr:-diff)'\n  fatal: BUG:tree-walk.c:947: unsupported magic 40\n\nWhoops. This is presumably ls-tree is protected, but I think we are\nmissing a GUARD_PATHSPEC call somewhere.\n\nThis isn't a huge deal, as the correct behavior is probably to die like\nls-tree does, but we probably shouldn't be hitting BUG assertions as a\ngeneral rule.\n\n-Peff\n"},{"id":"319236","messageId":"20170509225219.GB106700@google.com","threadId":"45914","inReplyTo":"20170509222432.3dxt7osjt2zjtaiw@sigill.intra.peff.net","subject":"Re: [BUG] :(attr) pathspecs can die(\"BUG\") in the tree-walker","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-09T22:52:19Z","receivedAt":"2017-05-09T22:52:26Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/09, Jeff King wrote:\n> I was playing with the new :(attr) pathspecs in the upcoming v2.13\n> today, and noticed:\n> \n>   $ git ls-files -- ':(attr:-diff)'\n>   t/t0110/url-1\n>   t/t0110/url-10\n>   [etc]\n> \n> So far so good.\n> \n>   $ git ls-tree HEAD -- ':(attr:-diff)'\n>   fatal: :(attr:-diff): pathspec magic not supported by this command: 'attr'\n> \n> Bummer, but I understand that sometimes the options need to be plumbed\n> through to work everywhere.\n> \n>   $ git log HEAD -- ':(attr:-diff)'\n>   fatal: BUG:tree-walk.c:947: unsupported magic 40\n> \n> Whoops. This is presumably ls-tree is protected, but I think we are\n> missing a GUARD_PATHSPEC call somewhere.\n> \n> This isn't a huge deal, as the correct behavior is probably to die like\n> ls-tree does, but we probably shouldn't be hitting BUG assertions as a\n> general rule.\n> \n> -Peff\n\nThe die(\"BUG: ...\"); is from a GAURD_PATHSPEC call.  What really needs\nto happen is to update the magic_mask passed into the parse_pathspec\ncall which initializes the pathspec object.  Its this magic_mask which\ncatches unsupported magic and prints a better message.\n\nI guess this means I (or someone else :D) should audit all the\nparse_pathspec calls and ensure that 'attr' magic is turned off until we\nwon't run into these GAURD_PATHSPEC die's.\n\n-- \nBrandon Williams\n"},{"id":"319245","messageId":"20170510012550.3qs3ra2c7ehbnixs@sigill.intra.peff.net","threadId":"45914","inReplyTo":"20170509225219.GB106700@google.com","subject":"Re: [BUG] :(attr) pathspecs can die(\"BUG\") in the tree-walker","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-10T01:25:50Z","receivedAt":"2017-05-10T01:26:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 09, 2017 at 03:52:19PM -0700, Brandon Williams wrote:\n\n> >   $ git log HEAD -- ':(attr:-diff)'\n> >   fatal: BUG:tree-walk.c:947: unsupported magic 40\n> > \n> > Whoops. This is presumably ls-tree is protected, but I think we are\n> > missing a GUARD_PATHSPEC call somewhere.\n> > \n> > This isn't a huge deal, as the correct behavior is probably to die like\n> > ls-tree does, but we probably shouldn't be hitting BUG assertions as a\n> > general rule.\n> \n> The die(\"BUG: ...\"); is from a GAURD_PATHSPEC call.  What really needs\n> to happen is to update the magic_mask passed into the parse_pathspec\n> call which initializes the pathspec object.  Its this magic_mask which\n> catches unsupported magic and prints a better message.\n\nAh, right, I always get the pathspec safety bits mixed up. I think the\nparse_pathspec() call in setup_revisions is a bit permissive:\n\n  parse_pathspec(&revs->prune_data, 0, 0,\n\t\t revs->prefix, prune_data.path);\n\nYou _shouldn't_ need to audit all the callers when you add new pathspec\nmagic. The callers should be masking out the bits that they know they\nsupport, but this one isn't.  It looks like there are a number of such\npermissive calls, though. I guess it was an attempt to not have to list\nthem manually at each call site (but then we suffer from the exact\nproblem I ran into).\n\n> I guess this means I (or someone else :D) should audit all the\n> parse_pathspec calls and ensure that 'attr' magic is turned off until we\n> won't run into these GAURD_PATHSPEC die's.\n\nOf course it would be nicer still if the 'attr' magic actually worked\nvia the tree-walking code. :)\n\n-Peff\n"}]}