Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jun 16 2021, Jeff King wrote:

> On Wed, Jun 16, 2021 at 06:22:10PM +0900, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > ... Still, I kind of like the "set -f" version because it doesn't need
>> > the extra directory which could cause problems with "ls-files -o", etc,
>> > as you mentioned. You could also create the empty directory on the fly,
>> > though if "set -f" works portably, that seems less complicated to me.
>> 
>> FWIW, I share that.
>
> Here it is with some cosmetic cleanups and a commit message. I don't
> mean to preempt further discussion if Ævar prefers another route, but I
> want to make sure we didn't stall out.

I mildly prefer mine as noted in
https://lore.kernel.org/git/87pmwmxd6f.fsf@evledraar.gmail.com/; but I'd
mainly prefer just to fix the immediate breakage on master, so whatever
variant of reverting, taking yours or mine Junio prefers I'm fine with.

Just inline comments on the patch:

> @@ -732,14 +732,24 @@ match_pattern_list () {
>  	arg="$1"
>  	shift
>  	test -z "$*" && return 1
> -	for pattern_
> -	do
> -		case "$arg" in
> -		$pattern_)
> -			return 0
> -		esac
> -	done
> -	return 1
> +	# We need to use "$*" to get field-splitting, but we want to
> +	# disable globbing, since we are matching against an arbitrary
> +	# $arg, not what's in the filesystem. Using "set -f" accomplishes
> +	# that, but we must do it in a subshell to avoid impacting the
> +	# rest of the script. The exit value of the subshell becomes
> +	# the function's return value.
> +	(
> +		set -f
> +		for pattern_ in $*
> +		do
> +			case "$arg" in
> +			$pattern_)
> +				exit 0
> +				;;
> +			esac
> +		done
> +		exit 1
> +	)
>  }

Why not just start with a ret=1, set ret=0 if we have a match and break
from the loop, and then do a "set +f" afterwards? I.e. is there an
actual need for the subshell here.

I'm mildly paranoid about a new "set -<flag>" in the codebase for vague
fears of portability (as noted in my linked message), but whatever shell
supports "set -<flag>" surely supports the inverse with "set +<flag>",
no?