On Fri, Mar 26, 2021 at 06:16:26PM +0200, Stavros Ntentos wrote: > > 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). > > ... 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. > > 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). > 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@xxxxxxxxx/#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
