Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-06-16 11:44:43
Possibly related (same subject, not in this thread)
- 2021-06-17 · Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs · Junio C Hamano <hidden>
- 2021-06-17 · Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs · Junio C Hamano <hidden>
- 2021-06-16 · Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs · Jeff King <hidden>
- 2021-06-16 · Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs · Ævar Arnfjörð Bjarmason <hidden>
- 2021-06-16 · Re: [PATCH] test-lib: fix "$remove_trash" regression and match_pattern_list() bugs · Jeff King <hidden>
On Wed, Jun 16 2021, Jeff King wrote:
On Wed, Jun 16, 2021 at 06:22:10PM +0900, Junio C Hamano wrote:quoted
Jeff King [off-list ref] writes:quoted
... 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/ (local); 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:
quoted hunk ↗ jump to hunk
@@ -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?