Support patterns starting with ** in ignore files

[simonbrauner]

Fixes: podman-container-tools/buildah#6417

[mtrmac]

I don’t know that much about this code…. but is this that much of a special case? I’d expect p.compile() to handle this, from a very quick glance.

Also the ContainsAny(…, "*?[]") check makes me suspicious. If **.md is supposed to work, why not **.[Cc]++? And if the latter is supposed to work, the current code would still expect p.compile() handle it; isn’t it easier to do it that way only?

[simonbrauner]

If **.md is supposed to work, why not **.[Cc]++?

This is a special case in Docker. https://github.com/moby/patternmatcher/blob/main/patternmatcher.go#L388

If the line begins with two asterisks and what follows does not contain the characters *?[] (and also an \ escaped character which I did not cover yet), it matches:

a) anything that has what is after ** as a suffix https://github.com/moby/patternmatcher/blob/main/patternmatcher.go#L328
b) if the third character is /, it matches files with the same name as what follows after the /. https://github.com/moby/patternmatcher/blob/main/patternmatcher.go#L332

a) currently does not work, we treat **.md as file .md in any directory, while docker treats it as path ending with .md.
b) this should be consistent now. We don't need the special case docker has because we simply treat what follows after / as regex.

I’d expect p.compile() to handle this, from a very quick glance.

In docker's code, there are multiple modes that are determined during the compilation. It seemed to me that the function does too many things at once, so I thought I'd cover the case outside.

But maybe it would be better to do the docker-style detection in compile and then just build a regex based on it? So that from outside of compile, it would not be visible.

Or other idea would be to just write to documentation that **.md works differently from docker, but it's okay because docker does not document what **.md should mean. https://docs.docker.com/build/concepts/context/#matching. And encourage users to use the more verbose **/*.md.

but is this that much of a special case?

It is for us to decide whether this is special enough for us to include it as an exception. Docker parses the line, determines a mode and then matches based on the mode. Podman does not have any modes and just interprets everything as regex.

[simonbrauner]

@nalind what do you think? What would be a reasonable resolution of the issue?

[mtrmac]

In docker's code, there are multiple modes that are determined during the compilation.

Those were not [documented to be] intended as “different semantics”; moby/patternmatcher@2d3e837 added them as a performance optimization.

AFAICT https://docs.docker.com/build/concepts/context/#matching documents that ** is supposed to match directories only (and https://git-scm.com/docs/gitignore, which seems to have been the motivating syntax, is even more strict).

So all in all I think this is really just a bug in Docker.

moby/patternmatcher#12, let’s see what happens.

[mtrmac]

It seemed to me that the function does too many things at once,

That’s a good reason to look for a way for a split a function… [1]

so I thought I'd cover the case outside.

… but… the split must make sense. The way I think about it, “what does this function do (and when it is used)” is a question that should have a fairly simple answer. Every extra condition or special case “it does everything correctly except if …, in that case call something else” is a smell indicating that there probably is a somewhat different function boundary that allows expressing the function’s purpose in a simpler way, or that probably there is a different way to document that function boundary.

---

[1] “does too many things” is subtly different is “is long an tediously does one clearly delineated thing in many steps”. Pattern.compile might be closer to the latter… especially the use of strings.Builder adds quite a lot of lines and visual noise.

[nalind]

@nalind what do you think? What would be a reasonable resolution of the issue?

Ultimately, we do aim for comparable behavior - it's why we added the carveout for an exact prefix match when we did. I, too, would have guessed that it would be incorporated into the pattern-to-regexp transformation. I'd want to see where that issue in pattermatcher goes to determine if this behavior is something to be caught up with, or to be corrected.

[simonbrauner]

Okay, thanks for feedback. We'll see what happens in the issue, but for now I updated the code so that compile is in charge of building the regex and determining the type, and match matches according to the type. Similarly to how the code looks in docker.