Re: [RFC PATCH v1 1/2] pathspec: warn: long and short forms are incompatible

From: Jeff King <peff@peff.net>
Date: Mar 27, 2021, 09:41 UTC
Message-ID: <YF792/TkS3Ssw9NS@coredump.intra.peff.net>
In-Reply-To: <20210326161626.28648-1-133706+stdedos@users.noreply.github.com>

On Fri, Mar 26, 2021 at 06:16:26PM +0200, Stavros Ntentos wrote:

Show 7 quoted lines

> > We avoid using variable-length arrays in our codebase. ...
> 
> Hear hear, however, I wanted to avoid the "small mess"
> that allocate/free would cause (one or more of ++sloc, labels, if-nesting); ...
> 
> > But two, they are limited in size and the failure mode is not graceful. ...
> ... however, my main issue is that - I don't know what's a sane allocation size.

I don't think a VLA gets you out of knowing the allocation size. Either way, you are using strlen(entry).

But I do think avoiding allocating altogether is better (as I showed in my previous response).

Show 8 quoted lines

> > ... The "4096" is scary here ...
> While scary, it is "a safe" upper high.
> The first time the string ends up in pathspec.c for processing it's here:
> 	entry = argv[i];
> which comes from here
> 	parse_pathspec(pathspec, magic_mask, flags, prefix, parsed_file.v);
> 
> and I don't know what's the maximum size of `parsed_file.v[0]`

There is no reasonable maximum size you can assume. Using 4096 is most definitely not a safe upper bound.

However, as I said, I don't think it is doing anything useful in the first place. You have sized the destination buffers as large as the original string, so they must be large enough to hold any subset of the original. Dropping them would be equally correct, but less distracting to a reader.

Show 6 quoted lines

> > Is this "%4096[^)]" actually valid? I don't think scanf understands
> > regular expressions.
> 
> I was suprised too - but it's not regex.
> 
> see: https://www.cplusplus.com/reference/cstdio/scanf/#parameters - 4th/3rd row from the end

Thanks, this is a corner of scanf I haven't looked at (mostly because again, we generally avoid scanf in our code base entirely).

Show 12 quoted lines

> I think it will help you see what I am trying to achieve if you read at the warning message / testcase
> https://lore.kernel.org/git/20210326024005.26962-2-stdedos+git@gmail.com/#iZ30t:t6132-pathspec-exclude.sh
> 
> And, to clean up the testcase:
> 	git log --oneline --format=%s -- ':!(glob)**/file'
> 
> I guess it should be now obvious what am I targetting:
> 
> If someone naively mixes short and long pathspec magics (`!`, and `(glob)`),
> short form takes precedence and ignores long magic / assumes long magic as part of path.
> 
> (If it's not obvious, all the more reason to include such warning)

I understand the overall goal. I am not sure why slashes in the flags section are a reliable indicator that this mixing is not happening and we should not show the warning.

It also feels like any checks like this should be relying on the existing pathspec-magic parser a bit more. I don't know the pathspec code that well, but surely at some point it has a notion of which parts are magic flags (e.g., after parse_element_magic in init_pathspec_item).

-Peff