Fix .dockerignore wildcard negation directory descent by Honny1 · Pull Request #6927 · podman-container-tools/buildah

Honny1 · 2026-06-24T13:18:34Z

Buildah skipped excluded directories when negation patterns contained wildcards (For example: !**/*.go), because the descent check only matched literal prefixes. Extract the literal prefix before the first wildcard and descend when the directory is at or under it; when the prefix is empty, always descend.

Same bug as Docker's classic builder (moby/moby#30018, moby/moby#45608).

Fixes: #6615

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed `.dockerignore`/`.containerignore` handling of wildcard negation patterns (e.g. `!**/*.go`, `!*/*.go`). Previously, files matching these patterns inside excluded directories were incorrectly omitted from the build context.

Honny1 · 2026-06-25T08:45:03Z

PTAL @podman-container-tools/buildah-maintainers @podman-container-tools/buildah-reviewers

simonbrauner

LGTM

simonbrauner · 2026-06-29T18:57:10Z

 			// Check for dockerignore-style exclusion of this item.
 			if rel != "." {
-				excluded, err := pm.Matches(filepath.ToSlash(rel)) //nolint:staticcheck
+				excluded, err := pm.IsMatch(filepath.ToSlash(rel))


Probably out of scope, but the deprecated call could be fixed there as well? https://github.com/podman-container-tools/buildah/blob/main/copier/copier.go#L1087

simek-m · 2026-06-30T10:59:34Z

-		}
-	}
-	return false
+	return copier.IncludeDirectoryAnyway(path, pm)


Is there a reason to have the private function that only delegates to the public (commented) one?

simek-m · 2026-06-30T11:39:07Z

+		// #6615: see dockerignore-exclude-kind-of-deep-subdir-dot
+		withoutDocker:       true,
+		fsSkip:              []string{"(dir):subdir:mtime"},
+		compatScratchConfig: types.OptionalBoolFalse,


Was the changed compatScratchConfig from types.OptionalBoolTrue to OptionalBoolFalse because it wasn't needed?

Also, there are removals of failOnExtraFSContent. Is that correct?

simek-m · 2026-06-30T12:08:15Z

+// descended into because a negation pattern in pm might match files under it.
+// It handles literal prefix matches (e.g. !cmd/main.go for dir "cmd") and
+// wildcard negations (e.g. !**/*.go, !*/*.go, !src/*/config.yaml).
+func IncludeDirectoryAnyway(dirPath string, pm *fileutils.PatternMatcher) bool {


Something like ShouldDescendExcludedDir might be a more descriptive name.

Also, I think that the and wildcard negations (e.g. !**/*.go, !*/*.go, !src/*/config.yaml) part is not completely correct. !src/*/config.yaml is IMHO handled by the literal prefix match.

The comment might mention that the wildcard-aware check may intentionally overmatch for wildcards in the middle (it's below).

simek-m · 2026-06-30T12:32:09Z

+			want:     true,
+		},
+		{
+			name:     "non-exclusion patterns are ignored",


TC like?:

{ name: "dir prefix match is ignored", path: "cmds", patterns: []string{"!cmd/main.go"}, want: false, },

Honny1 · 2026-06-30T14:48:52Z

The failed test is an unrelated flake.

simek-m

All comments reflected, LGTM.

simonbrauner · 2026-07-01T11:38:17Z

 	// Matches uses filepath.FromSlash() to convert candidates before
 	// checking if they match the patterns it's been given, implying that
 	// it expects Unix-style paths.


Just a detail, but the comment uses the old function name Matches. It may be misleading now, and the content may no longer be relevant.

Agree, then LGTM

mtrmac

(Just a pretty brief look, I didn’t read the tests at all.)

mtrmac · 2026-07-01T17:40:32Z

+		// if the directory is at or under that literal prefix, a file beneath this
+		// directory could match the negation, so keep descending.
+		if firstWild := strings.IndexAny(slashPattern, "*?["); firstWild >= 0 {
+			literalPrefix := strings.TrimRight(slashPattern[:firstWild], "/")


What about !cmd/image*? TrimRight will not find the /.

@mtrmac Oh, I thought it's handled with strings.HasPrefix before, but no - only for cmd with !cmd/image*, but not cmd/images with !cmd/image* or !cmd/image*/main.go with cmd/images/main.go.

mtrmac · 2026-07-01T17:41:49Z

+// the literal prefix before the first wildcard and may intentionally
+// overmatch (descend into directories that won't ultimately contain matches),
+// which is safe because actual file-level matching happens later.
+func ShouldDescendExcludedDir(dirPath string, pm *fileutils.PatternMatcher) bool {


This (or some variant of this) should probably be in c/storage/pkg/fileutils instead: c/storage/pkg/archive.tarWithOptionsTo` contains a similar exclusion check with the same problem.

(If we did end up doing this Buildah-only, I think having this in something like internal/exclusions would be better, not to commit to it as a stable API — but that’s a Buildah maintainer decision.)

Agreed, we definitely don't want to add this as an exported API that we have to keep working, even if/after how exclusion matching rules change.

I have moved that into internal/excludes. Should I create a PR with the fix in c/storage as a follow-up?

nalind

I'm not very enthusiastic about updating conformance tests which test "ignores" behavior so that they don't use a baseline.

nalind · 2026-07-06T18:46:16Z

+// the literal prefix before the first wildcard and may intentionally
+// overmatch (descend into directories that won't ultimately contain matches),
+// which is safe because actual file-level matching happens later.
+func ShouldDescendExcludedDir(dirPath string, pm *fileutils.PatternMatcher) bool {


Agreed, we definitely don't want to add this as an exported API that we have to keep working, even if/after how exclusion matching rules change.

nalind · 2026-07-06T18:51:21Z

 	// checking if they match the patterns it's been given, implying that
 	// it expects Unix-style paths.
-	matches, err := pm.Matches(filepath.ToSlash(rel)) //nolint:staticcheck
+	matches, err := pm.IsMatch(filepath.ToSlash(rel))


IsMatch() does more work to return the same result as Matches(), it's not worth switching which one we use.

Honny1 · 2026-07-09T14:02:35Z

I'm not very enthusiastic about updating conformance tests which test "ignores" behavior so that they don't use a baseline.

I am also not a big fan of this, but the root cause is that go-dockerclient sends the build context through moby/go-archive's TarWithOptions, which has the same bug (moby/moby#30018, moby/moby#45608). Docker BuildKit handles it correctly, but since the framework filters client-side before the context reaches BuildKit, we can't get a valid baseline.

nalind · 2026-07-13T21:34:19Z

-  run_buildah 1 run myctr ls -l subdir/
+  # !*/sub1* un-excludes subdir/sub1.txt
+  run_buildah run myctr ls -l subdir/
+  run_buildah run myctr ls -l subdir/sub1.txt


This should probably be making sure that nothing else was copied to this directory.

Something like this

run_buildah run myctr find subdir -mindepth 1 -print assert "$output" = "subdir/sub1.txt"

would make sure that nothing else was copied there.

Honny1 · 2026-07-14T12:32:14Z

PTAL @nalind

nalind · 2026-07-14T17:50:51Z

+		// Docker BuildKit handles it correctly, but we cannot compare
+		// through this framework.
+		// TODO: re-enable Docker comparison when moby/go-archive fixes
+		// wildcard negation handling in TarWithOptions.


We supply the InputStream field in the options, not the ContextDir, so we do have control over which parts of the build context we transfer.

Ah, thanks, I didn't realize that.

Buildah skipped excluded directories when negation patterns contained wildcards (For example: !**/*.go), because the descent check only matched literal prefixes. Extract the literal prefix before the first wildcard and descend when the directory is at or under it; when the prefix is empty, always descend. Same bug as Docker's classic builder (moby/moby#30018, moby/moby#45608). Fixes: podman-container-tools#6615 Signed-off-by: Jan Rodák <hony.com@seznam.cz>

Honny1 force-pushed the negation-filter branch 2 times, most recently from fcba319 to f65f4ed Compare June 25, 2026 08:26

Honny1 marked this pull request as ready for review June 25, 2026 08:44

simonbrauner reviewed Jun 29, 2026

View reviewed changes

simek-m reviewed Jun 30, 2026

View reviewed changes

Honny1 force-pushed the negation-filter branch from f65f4ed to 8811b09 Compare June 30, 2026 14:18

Honny1 requested review from simek-m and simonbrauner June 30, 2026 14:48

simek-m approved these changes Jul 1, 2026

View reviewed changes

simonbrauner reviewed Jul 1, 2026

View reviewed changes

Honny1 force-pushed the negation-filter branch from 8811b09 to c9bda3c Compare July 1, 2026 16:36

Honny1 requested review from inknos and simonbrauner July 1, 2026 16:41

mtrmac reviewed Jul 1, 2026

View reviewed changes

nalind reviewed Jul 6, 2026

View reviewed changes

Honny1 force-pushed the negation-filter branch from c9bda3c to 01c5159 Compare July 9, 2026 13:51

Honny1 requested review from mtrmac and nalind July 9, 2026 14:05

nalind reviewed Jul 13, 2026

View reviewed changes

Honny1 force-pushed the negation-filter branch from 01c5159 to 8021cd0 Compare July 14, 2026 12:31

nalind reviewed Jul 14, 2026

View reviewed changes

Honny1 force-pushed the negation-filter branch from 8021cd0 to 636f3ad Compare July 15, 2026 17:14

Honny1 requested a review from nalind July 15, 2026 17:15

Uh oh!

Conversation

Honny1 commented Jun 24, 2026

What type of PR is this?

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

Honny1 commented Jun 25, 2026

Uh oh!

simonbrauner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Honny1 commented Jun 30, 2026

Uh oh!

simek-m left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nalind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Honny1 commented Jul 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Honny1 commented Jul 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!