Re: git 2.40.1 tree-diff crashes with (glob) magic on pathspecs

From: Jeff King <peff@peff.net>
Date: Jun 1, 2023, 13:30 UTC
Message-ID: <20230601133057.GA2458601@coredump.intra.peff.net>
In-Reply-To: <xmqqsfbbd9oi.fsf@gitster.g>

On Thu, Jun 01, 2023 at 02:38:05PM +0900, Junio C Hamano wrote:

Show 7 quoted lines

> > So follow-mode does not actually work with wildcards, but we err on the
> > side of not rejecting them outright. In that sense, the simplest "fix"
> > here would be to allow :(glob) to match the '#if 0' section, like:
> 
> Is it "fix" or widening the wound, though?  The runtime BUG is very
> unpleasant to see, but silently giving a possibly wrong result would
> be even worse, I am afraid.

I think it's the same wrong result we'd give in other circumstances, which in some ways makes it orthogonal. I.e., this is already wrong:

  git log --follow 'foo*'

Doing:

  git --noglob-pathspecs --follow 'foo*'

makes it right (assuming you really have a literal '*' in your pathname). And then doing:

  git --noglob-pathspecs --follow ':(glob)foo*'

makes it wrong again, but in exactly the same way as the first case. Which is why I said "orthogonal" above, because it is really just twiddling options to cancel each other out and reach the same broken state. ;)

All of that said, I do think the patch I showed earlier is pretty ugly, and it is not even fixing all of the problems (you can trigger the same BUG() for ":(icase)", etc). I have a series which moves us in a better direction, but I need to put a few finishing touches on it. I'll hopefully send it later today.

Show 7 quoted lines

> If somebody cares about the "--follow" mode very deeply, the "upon
> finding that the path disappears, run the rename detection with the
> parent and switch the (single) path to follow to the old path in the
> parent" logic must be updated to keep track of these pathspecs per
> traversal paths.  If false positives are allowed, an approximation
> that may be easier to implement is to add paths to the set of paths
> to be followed every time such a rename is found.

Yes, it would be nice to fix all of these --follow bugs once and for all. But that's a pretty big task. I think in the meantime, it is not too hard to make Jim's case behave more sensibly.

-Peff