From: Mark Jamsek <mark@jamsek.com>
Subject: Re: ignored files can't handle trailing slashes
To: Lucas <lucas@sexy.is>, gameoftrees@openbsd.org
Date: Mon, 6 Feb 2023 16:54:02 +1100

On 23-02-05 06:35PM, Stefan Sperling wrote:
> 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.

Yes, this is certainly more intuitive and matches behaviour with similar
applications.

ok with or without tiny style nit inline

> 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];

Please consider leaving a blank line after declaration

> +				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
> 
> 

-- 
Mark Jamsek <fnc.bsdbox.org>
GPG: F2FF 13DE 6A06 C471 CA80  E6E2 2930 DC66 86EE CF68