From: Lucas <lucas@sexy.is>
Subject: ignored files can't handle trailing slashes
To: gameoftrees@openbsd.org
Date: Sun, 05 Feb 2023 14:05:30 +0000

Hello list,

As I shared on IRC, when `got import -I dir/` is used, `dir/` is *not*
ignored: fnmatch is called with the name in dirent's d_name, which
doesn't include a trailing slash if the entry is of type directory.
Do note that calling got_path_strip_trailing_slashes on `-I` argument
is not an option: 'dir*/' can have some nasty side effects. The patch
for regress/cmdline/import.sh below makes this explicit.

Same can be observed for .{cvs,git}ignore.

As for the fix, I believe it's outside of my reach for the time being.
The snippet below fixes it for import's case in a quite rough way: I
don't like malloc'ing on every iteration, but it seems the only way to
do it without screwing -portable (POSIX only mandates for d_name in
dirent, and doesn't specify a max size).

The best way to actually fix it should be as stsp@ suggested: unify the
ignore matching in both got import and the commands that deal with
.{cvs,git}ignore, and then fix the matching.

	--- lib/repository.c
	+++ lib/repository.c
	@@ -2113,10 +2113,28 @@ write_tree(struct got_object_id **new_tree_id, const c
				continue;
	 
			TAILQ_FOREACH(pe, ignores, entry) {
	-			if (fnmatch(pe->path, de->d_name, 0) == 0) {
	-				ignore = 1;
	-				break;
	+			err = got_path_dirent_type(&type, path_dir, de);
	+			if (err)
	+				goto done;
	+			if (type == DT_DIR) {
	+				char *ignore_path;
	+
	+				ignore_path = strdup(pe->path);
	+				if (ignore_path == NULL) {
	+					err = got_error_from_errno("strdup");
	+					goto done;
	+				}
	+				got_path_strip_trailing_slashes(ignore_path);
	+				if (fnmatch(ignore_path, de->d_name, 0) == 0)
	+					ignore = 1;
	+				free(ignore_path);
	+			} else {
	+				if (fnmatch(pe->path, de->d_name, 0) == 0)
	+					ignore = 1;
				}
	+
	+			if (ignore)
	+				break;
			}
			if (ignore)
				continue;

-Lucas


diff refs/heads/main refs/heads/not-ignored-trailing-slashes
commit - 30bd0f8ea103b4839e685df105cbdf5a033b3ebd
commit + d1a8b6ec6f8d93b6435ab7ed5aded7866f89a67a
blob - 9ce21cacfc1fea14fa9289ed4180e0640cfdffdf
blob + a3dc1741d2e66938c4431310078b0b3ec6248dbb
--- regress/cmdline/import.sh
+++ regress/cmdline/import.sh
@@ -395,6 +395,44 @@ test_import_empty_dir() {
 	test_done "$testroot" "$ret"
 }
 
+test_import_ignores_trailing_slashes() {
+	local testname=import_ignores_trailing_slashes
+	local testroot=`mktemp -d "$GOT_TEST_ROOT/got-test-$testname-XXXXXXXX"`
+
+	gotadmin init $testroot/repo
+
+	mkdir $testroot/tree
+	echo alpha >$testroot/tree/alpha
+	mkdir $testroot/tree/beta
+	echo gamma >$testroot/tree/beta/gamma
+	echo delta-file >$testroot/tree/delta-file
+	mkdir $testroot/tree/delta-dir
+	echo epsilon >$testroot/tree/delta-dir/epsilon
+
+	got import -I beta/ -I delta-*/ \
+		-m 'init' -r $testroot/repo $testroot/tree > $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	local head_commit=`git_show_head $testroot/repo`
+	echo "A  $testroot/tree/alpha" >> $testroot/stdout.expected
+	echo "A  $testroot/tree/delta-file" >> $testroot/stdout.expected
+	echo "Created branch refs/heads/main with commit $head_commit" \
+		>> $testroot/stdout.expected
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "xfail trailing slashes"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_import_empty_dir() {
 	local testname=import_empty_dir
 	local testroot=`mktemp -d "$GOT_TEST_ROOT/got-test-$testname-XXXXXXXX"`
@@ -494,5 +532,6 @@ run_test test_import_empty_dir
 run_test test_import_detached_head
 run_test test_import_requires_new_branch
 run_test test_import_ignores
+run_test test_import_ignores_trailing_slashes
 run_test test_import_empty_dir
 run_test test_import_symlink
blob - 6168491dcdae298a50000ab17dd1b5af061cce7e
blob + d822260020e8a31a235f66e7a1a6e8e9cae92b9c
--- regress/cmdline/status.sh
+++ regress/cmdline/status.sh
@@ -695,6 +695,36 @@ test_status_status_code() {
 	test_done "$testroot" "$ret"
 }
 
+test_status_gitignore_trailing_slashes() {
+	local testroot=`test_init status_gitignore_trailing_slashes`
+
+	got checkout $testroot/repo $testroot/wt > /dev/null
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		test_done "$testroot" "$ret"
+		return 1
+	fi
+
+	echo "unversioned file" > $testroot/wt/foo
+	echo "unversioned file" > $testroot/wt/epsilon/bar
+	echo "unversioned file" > $testroot/wt/epsilon/boo
+	echo "unversioned file" > $testroot/wt/epsilon/moo
+	echo "epsilon/" > $testroot/wt/.gitignore
+
+	echo '?  .gitignore' > $testroot/stdout.expected
+	echo '?  foo' >> $testroot/stdout.expected
+	(cd $testroot/wt && got status > $testroot/stdout)
+
+	cmp -s $testroot/stdout.expected $testroot/stdout
+	ret=$?
+	if [ $ret -ne 0 ]; then
+		diff -u $testroot/stdout.expected $testroot/stdout
+		test_done "$testroot" "xfail trailing slashes"
+		return 1
+	fi
+	test_done "$testroot" "$ret"
+}
+
 test_status_status_code() {
 	local testroot=`test_init status_status_code`
 
@@ -1044,6 +1074,7 @@ run_test test_status_status_code
 run_test test_status_many_paths
 run_test test_status_cvsignore
 run_test test_status_gitignore
+run_test test_status_gitignore_trailing_slashes
 run_test test_status_status_code
 run_test test_status_suppress
 run_test test_status_empty_file