On Sun, Feb 05, 2023 at 06:04:33PM +0100, Stefan Sperling wrote:
> On Sun, Feb 05, 2023 at 02:05:30PM +0000, Lucas wrote:
> > 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.
> 
> The patch below is what I would suggest.
> 
> This extends the existing test case to cover this issue and also avoids
> the extra allocation. We must pay the cost of an extra strlcpy in case
> the user passes a slash because we should not modify pe->path in-place.

This version keeps the dt_type check from your diff intact, which I missed
in my previous diff. Now our behaviour with trailing slashes should match
that of Git, as documented in gitignores(5). Also adds test coverage for
trying to match a file with a trailing slash, and updates the man page.

diff 27749ea2ddbc482ad434ed865e0f855313db0a27 ca0533dbf0088e75debf5fe1dcd01f6966c79ec1
commit - 27749ea2ddbc482ad434ed865e0f855313db0a27
commit + ca0533dbf0088e75debf5fe1dcd01f6966c79ec1
blob - 2f2d51be710c9d1ff0291d1284b33f43eb85e145
blob + 2ca478571820e0d61303cd5f523733a851e57a62
--- got/got.1
+++ got/got.1
@@ -117,6 +117,9 @@ follows the globbing rules documented in
 .Ar pattern
 follows the globbing rules documented in
 .Xr glob 7 .
+Ignore patterns which end with a slash,
+.Dq / ,
+will only match directories.
 .It Fl m Ar message
 Use the specified log message when creating the new commit.
 Without the
blob - 9b6bfeb03479b844dcb5ea253742cfce88a0b45f
blob + 4b85e08013a59b8957a0dd096df8805d4a1d4d23
--- lib/repository.c
+++ lib/repository.c
@@ -2153,8 +2153,25 @@ write_tree(struct got_object_id **new_tree_id, const c
 		    strcmp(de->d_name, "..") == 0)
 			continue;
 
+		err = got_path_dirent_type(&type, path_dir, de);
+		if (err)
+			goto done;
+
 		TAILQ_FOREACH(pe, ignores, entry) {
-			if (fnmatch(pe->path, de->d_name, 0) == 0) {
+			if (type == DT_DIR && pe->path_len > 0 &&
+			    pe->path[pe->path_len - 1] == '/') {
+				char stripped[PATH_MAX];
+				if (strlcpy(stripped, pe->path,
+				    sizeof(stripped)) >= sizeof(stripped)) {
+					err = got_error(GOT_ERR_NO_SPACE);
+					goto done;
+				}
+				got_path_strip_trailing_slashes(stripped);
+				if (fnmatch(stripped, de->d_name, 0) == 0) {
+					ignore = 1;
+					break;
+				}
+			} else if (fnmatch(pe->path, de->d_name, 0) == 0) {
 				ignore = 1;
 				break;
 			}
@@ -2162,10 +2179,6 @@ write_tree(struct got_object_id **new_tree_id, const c
 		if (ignore)
 			continue;
 
-		err = got_path_dirent_type(&type, path_dir, de);
-		if (err)
-			goto done;
-
 		if (type == DT_DIR) {
 			err = import_subdir(&new_te, de, path_dir,
 			    ignores, repo, progress_cb, progress_arg);
blob - 9ce21cacfc1fea14fa9289ed4180e0640cfdffdf
blob + 28fd8607f9a6b87c382e3669204da90a4ec3c6ac
--- regress/cmdline/import.sh
+++ regress/cmdline/import.sh
@@ -374,7 +374,7 @@ test_import_ignores() {
 	mkdir $testroot/tree
 	make_test_tree $testroot/tree
 
-	got import -I alpha -I '*lta*' -I '*silon' \
+	got import -I alpha -I 'beta/' -I '*lta*' -I '*silon/' \
 		-m 'init' -r $testroot/repo $testroot/tree > $testroot/stdout
 	ret=$?
 	if [ $ret -ne 0 ]; then