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

From: Jeff King <peff@peff.net>
Date: Jun 16, 2021, 11:50 UTC
Message-ID: <YMnllMXvRJKSdGID@coredump.intra.peff.net>
In-Reply-To: <87k0muxcd7.fsf@evledraar.gmail.com>

On Wed, Jun 16, 2021 at 01:38:55PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 23 quoted lines

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

My thought was that the subshell takes us back to the original state, regardless of what it was. As opposed to "set +f" which takes us back to a particular state. But it is unlikely that we'd have done a global "set -f" before calling this, so maybe that is being overly conservative.

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

Yes, I think we can assume that if it supports one, it supports the other.

-Peff