From: Stefan Sperling <stsp@stsp.name>
Subject: Re: leading separators in ignore patterns
To: Evan Silberman <evan@jklol.net>
Cc: gameoftrees@openbsd.org
Date: Mon, 19 May 2025 10:48:12 +0200

On Sat, May 17, 2025 at 01:38:23PM -0700, Evan Silberman wrote:
> I'm back with a patch.
> 
> - Skip past leading slashes in ignore patterns. In gitignore(7), these
>   trigger the behavior got has by default. Add regress test.
> - Lift documentation of ignore patterns to a subsection, referenced by
>   the `add` and `status` docs.
> - Refine documentation of ignore patterns to discuss the extensions to
>   glob(7) as a whole, and then list caveats relative to git(1) and
>   cvs(1).
> - Add a file to the test worktree in the main gitignore regress test in
>   order to register a known delta to gitignore(7) that was ambiguously
>   documented previously: for the pattern a/**/foo, git will ignore
>   a/foo, but got will not.

Thinking about is some more, you have convinced me that matching "/foo"
like "foo" won't hurt. Matched paths are relative to the work tree so
there is no point in trying to match a path in the root directory.
 
> The one change to worktree.c here originally happened in read_ignores()
> but I moved it to match_path() after further consideration. Not sure
> about my style here, is it unnecessary to use new locals for advancing
> the pattern pointer/decreasing the length?

I don't think it matters much. One reason in favour of local variables
is that it makes debugging a bit it easier because we can then always
see which values the arguments had when the function was entered.

> I note that `got import` references ignore patterns but the patterns
> given as -I params are not matched the same way. Maybe `got import`
> should respect .gitignore/.cvsignore?

Ideally, -I matching would use the same semantics as ignore files.

The reason that repository.c has its own copy of match_ignores() is likely
because I wanted to avoid a dependency of repository.c on worktree.c.
And then the 2 match_ignore() implementations grew out of sync over time.
Perhaps moving the implementation to lib/ignores.c and making both sides
call into that would be best. But let's deal with your current patch first.

> Happy to split this up a bit if it makes review easier, most of the
> diffstat is from the man page reworks.

I have one small suggestion for your patch. In any case, your patch
looks very good to me.

>  	/*
> +	 * For gitignore(7) compatibility, ignore leading slashes
> +	 */
> +	if (len > 0 && pat[0] == '/') {
> +		pat++;
> +		len--;
> +	}

If the above was a while-loop instead of an if-statement then you could
do the following in your test and the test would keep passing:

> +test_status_gitignore_leading_slashes() {

> +	echo "/foo" > $testroot/wt/.gitignore

	echo "//////nu" >> $testroot/wt/.gitignore

> +	echo '?  .gitignore' > $testroot/stdout.expected
> +	echo '?  epsilon/bar' >> $testroot/stdout.expected
> +	echo '?  epsilon/foo' >> $testroot/stdout.expected
> +	echo '?  epsilon/nu/baz' >> $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
> +	fi
> +	test_done "$testroot" "$ret"
> +}