| Message ID | patch-10.10-54129b94a77-20220621T222854Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | t5510: fix the quoting mess | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > - remote_url="file://$TRASH_DIRECTORY/." && # NOT local yet! > - local cmdline_setup="\"$remote_url\" \"$refspec_heads\"" Good riddance ;-) > + local remote_url="file://$TRASH_DIRECTORY/." && > > ... > - git fetch '"$cmdline_setup"' && > + git fetch "$remote_url" "$refspec_heads" && > ... > + git$git_fetch_c fetch \ > + ${arg_fetch_prune:+--prune} \ > + ${arg_fetch_no_prune:+--no-prune} \ > + ${arg_fetch_prune_tags:+--prune-tags} \ > + ${arg_fetch_origin:+origin} \ > + ${arg_fetch_url:+"$remote_url"} \ > + ${arg_fetch_refspec_tags:+"refs/tags/*:refs/tags/*"} \ > + ${arg_fetch_refspec_heads:+"+refs/heads/*:refs/remotes/origin/*"} && > + This makes it a lot clearer, with no perl, no sed, no eval. It does become louder, but should be easier to follow in general ... > test_configured_prune_type --mode link true unset true unset pruned pruned \ > - "\"$remote_url\"" \ > + REMOTE_URL \ > "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" ... except for a magic like this one. We may remember the REMOTE_URL -> $remote_url trick used here this week, but I am not sure if we find it sensible in 3 months. But overall I think this makes it simpler. I am not 100% sold on the necessity of lengthy earlier steps, though. Thanks.
On 6/22/22 2:12 AM, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > This makes it a lot clearer, with no perl, no sed, no eval. It does > become louder, but should be easier to follow in general ... > >> test_configured_prune_type --mode link true unset true unset pruned pruned \ >> - "\"$remote_url\"" \ >> + REMOTE_URL \ >> "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" > > ... except for a magic like this one. > > We may remember the REMOTE_URL -> $remote_url trick used here this > week, but I am not sure if we find it sensible in 3 months. > > But overall I think this makes it simpler. I am not 100% sold on > the necessity of lengthy earlier steps, though. When I saw that this was a series with 10 patches (without reading anything else) I expected that you had created a test-lib.sh helper that allowed replacing a word in a string with another string, and then the remaining patches were fixing the other tests that have similar breaks when using "@" in the path. (Heck, I'd even take a "test-tool replace-word <string> <word> <replacement>" implementation to avoid all of these issues we have due to using scripting languages that rely on special characters to define the match and replacement operation.) It seems like this isn't the last time we are going to have a problem with string replacement like this, and having a well-defined helper would go far. The rest of the changes to the test script seem more complicated than necessary for what _should_ be a simple problem. Thanks, -Stolee
On Wed, Jun 22 2022, Derrick Stolee wrote: > On 6/22/22 2:12 AM, Junio C Hamano wrote: >> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> This makes it a lot clearer, with no perl, no sed, no eval. It does >> become louder, but should be easier to follow in general ... >> >>> test_configured_prune_type --mode link true unset true unset pruned pruned \ >>> - "\"$remote_url\"" \ >>> + REMOTE_URL \ >>> "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" >> >> ... except for a magic like this one. >> >> We may remember the REMOTE_URL -> $remote_url trick used here this >> week, but I am not sure if we find it sensible in 3 months. >> >> But overall I think this makes it simpler. I am not 100% sold on >> the necessity of lengthy earlier steps, though. > > When I saw that this was a series with 10 patches (without reading > anything else) I expected that you had created a test-lib.sh helper > that allowed replacing a word in a string with another string, and > then the remaining patches were fixing the other tests that have > similar breaks when using "@" in the path. I guess we could have such a helper, but I can't imagine a use-case for it where the answer wouldn't be the same as what this series suggests: Just stop passing a quoted argument list as one giant string. > (Heck, I'd even take a "test-tool replace-word <string> <word> > <replacement>" implementation [...] If we need to replace a string we can use sed or perl, but much better is to not need to do so in the first place. E.g. the "origin" match we had before is now just "origin" in a case statement. I'm assuming that you mean a test-tool to do something similar to strvec_split() (or whatever the quoted variant was?). > to avoid all of these issues we have > due to using scripting languages that rely on special characters > to define the match and replacement operation.) We've got plenty of issues inherent in shellscript as a language with no little in the way of complex types (e.g. no hashes). But in this case the language gives us the right type (list), we just weren't using it. No? > It seems like this isn't the last time we are going to have a > problem with string replacement like this, and having a well-defined > helper would go far. ...I guess I'm not seeing the use-case for it, in this case it was "just pass a list then", wouldn't that be the case in other similar scenarios. > The rest of the changes to the test script seem more complicated > than necessary for what _should_ be a simple problem. I tried to optimize it for ease of review as opposed to diff size or patch numbers, so e.g. doing mechanical replacements in their own commits. Do you have any specific things in mind that you think are too complicated? Yes, it should ideally not be such a pain, but as we made wide use of a string instead of a list digging ourselves out of that hole took some doing. But I really think fixing the underlying issue is worth it, as opposed to just plastering over it.
Derrick Stolee <derrickstolee@github.com> writes: > It seems like this isn't the last time we are going to have a > problem with string replacement like this, and having a well-defined > helper would go far. I think the idea of [10/10] is to use shell itself as a well-defined helper, with "string replacement" being "$variable_interpolation". Which isn't a bad approach, I would say. > The rest of the changes to the test script seem more complicated > than necessary for what _should_ be a simple problem. True. I am not sure which parts are unnecessary "churn while at it, burying the most interesting and beneficial one at the end as a hostage", and which ones are absolutely needed to reach [10/10]. Perhaps all of them are the latter? I dunno.
diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 73964bebced..730968d4cd7 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -837,6 +837,7 @@ test_configured_prune_type () { shift done && + local refspec_tags='refs/tags/*:refs/tags/*' && local refspec_heads='+refs/heads/*:refs/remotes/origin/*' && local fetch_prune="$1" && @@ -846,23 +847,59 @@ test_configured_prune_type () { local expected_branch="$5" && local expected_tag="$6" && shift 6 && - local cmdline="$@" && + local cmdline="" && + local arg_fetch_prune="" && + local arg_fetch_no_prune="" && + local arg_fetch_prune_tags="" && + local arg_fetch_origin="" && + local arg_fetch_url="" && + local arg_fetch_refspec_tags="" && + local arg_fetch_refspec_heads="" && + while test $# != 0 + do + cmdline="${cmdline:+$cmdline }$1" && + case "$1" in + --prune) + arg_fetch_prune=t + ;; + --no-prune) + arg_fetch_no_prune=t + ;; + --prune-tags) + arg_fetch_prune_tags=t + ;; + origin) + arg_fetch_origin=t + ;; + REMOTE_URL) + arg_fetch_url=t + ;; + $refspec_tags) + arg_fetch_refspec_tags=t + ;; + $refspec_heads) + arg_fetch_refspec_heads=t + ;; + *) + BUG "unknown argument: '$1'" + ;; + esac && + shift + done && - remote_url="file://$TRASH_DIRECTORY/." && # NOT local yet! - local cmdline_setup="\"$remote_url\" \"$refspec_heads\"" + local remote_url="file://$TRASH_DIRECTORY/." && if test "$mode" = 'link' then - new_cmdline="" && - if test -z "$cmdline" then - new_cmdline=$cmdline_setup - else - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') - fi && - - cmdline="$new_cmdline" + arg_fetch_refspec_heads=t + arg_fetch_url=t + elif test -n "$arg_fetch_origin" + then + arg_fetch_origin= + arg_fetch_url=t + fi fi && test_expect_success "$mode prune fetch.prune=$fetch_prune remote.origin.prune=$remote_origin_prune fetch.pruneTags=$fetch_prune_tags remote.origin.pruneTags=$remote_origin_prune_tags${cmdline:+ $cmdline}; branch:$expected_branch tag:$expected_tag" ' @@ -871,7 +908,7 @@ test_configured_prune_type () { git tag -f newtag && ( cd one && - git fetch '"$cmdline_setup"' && + git fetch "$remote_url" "$refspec_heads" && git rev-parse --verify refs/remotes/origin/newbranch && git rev-parse --verify refs/tags/newtag ) && @@ -893,7 +930,15 @@ test_configured_prune_type () { then git_fetch_c="" fi && - git$git_fetch_c fetch '"$cmdline"' && + git$git_fetch_c fetch \ + ${arg_fetch_prune:+--prune} \ + ${arg_fetch_no_prune:+--no-prune} \ + ${arg_fetch_prune_tags:+--prune-tags} \ + ${arg_fetch_origin:+origin} \ + ${arg_fetch_url:+"$remote_url"} \ + ${arg_fetch_refspec_tags:+"refs/tags/*:refs/tags/*"} \ + ${arg_fetch_refspec_heads:+"+refs/heads/*:refs/remotes/origin/*"} && + case "$expected_branch" in pruned) test_must_fail git rev-parse --verify refs/remotes/origin/newbranch @@ -993,22 +1038,22 @@ test_configured_prune unset unset unset unset pruned pruned --prune --prune-tags test_configured_prune_type --mode name true unset true unset pruned pruned test_configured_prune_type --mode link true unset true unset pruned pruned \ - "\"$remote_url\"" \ + REMOTE_URL \ "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" test_configured_prune_type --mode name unset true unset true pruned pruned test_configured_prune_type --mode link unset true unset true pruned pruned \ - "\"$remote_url\"" \ + REMOTE_URL \ "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" # remote.<name>.pruneTags overrides fetch.pruneTags, just like # remote.<name>.prune overrides fetch.prune if set. test_configured_prune_type --mode name true unset true unset pruned pruned test_configured_prune_type --mode link true unset true unset pruned pruned \ - "\"$remote_url\"" \ + REMOTE_URL \ "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" test_configured_prune_type --mode name false true false true pruned pruned test_configured_prune_type --mode link false true false true pruned pruned \ - "\"$remote_url\"" \ + REMOTE_URL \ "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" test_configured_prune true false true false kept kept
Change the test_configured_prune_type() function to take full advantage of its arguments being passed in as a list, rather than needing hacks to work around its quoting issues. When the test_configured_prune() function was implemented in 737c5a9cde7 (fetch: make --prune configurable, 2013-07-13) it passed in its arguments to "git fetch" as one argument (although at the time only one argument was passed).. Then in preparation for passing more arguments the first quoting hack was added in 82f34e03e91 (fetch tests: double quote a variable for interpolation, 2018-02-09), with e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) following after that. This was all to implement the "git fetch --prune-tags" feature in 97716d217c1 (fetch: add a --prune-tags option and fetch.pruneTags config, 2018-02-09). At the time the edge cases introduced by this quoting were a known issue, but the alternative was a larger refactoring of this test file. In preceding commits we've done that refactoring, so let's finally take advantage of it. As reported in [1] the existing workaround(s) weren't enough. We can now drop the "perl" (or "sed" or whatever, see [2]), this will also handle other cases, such as those mentioned in [3]. 1. https://lore.kernel.org/git/00a401d884d0$32885890$979909b0$@nexbridge.com/ 2. https://lore.kernel.org/git/495bd957-43dc-f252-657d-2969bb7ad5f3@github.com/ 3. https://lore.kernel.org/git/YrFwcL2dRS%2Fv7xAw@coredump.intra.peff.net/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 79 +++++++++++++++++++++++++++++++++++++----------- 1 file changed, 62 insertions(+), 17 deletions(-)