* Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 @ 2022-06-20 18:04 rsbecker 2022-06-20 18:46 ` Derrick Stolee 2022-06-21 13:51 ` Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker 0 siblings, 2 replies; 37+ messages in thread From: rsbecker @ 2022-06-20 18:04 UTC (permalink / raw) To: 'Junio C Hamano'; +Cc: git Subtests t5510.69, 71, 97, 99, 125, 127, 147, 149, 151, 153, 155, 157, 159, 161, 163, 165, 167, 170 failed in rc1 on the NonStop ia64 platform. These were working in rc0, but after analysis, I do not think that is relevant. Here is what we see for subtest 69: expecting success of 5510.69 'link prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --prune origin refs/tags/*:refs/tags/*; branch:kept tag:pruned': ... git fetch "file:///home/jenkins/.jenkins/workspace/Git_Pipeline@2/t/trash directory.t5510-fetch/." "+refs/heads/*:refs/remotes/origin/*" && ... git$git_fetch_c fetch --prune "file:///home/jenkins/.jenkins/workspace/Git_Pipeline/t/trash directory.t5510-fetch/." refs/tags/*:refs/tags/* && What seems to be happening is that the test script is computing the wrong parent directory in the second case, cutting the @2. We use Jenkins for our builds and occasional multiple builds run. The @2 is Jenkins way of running in another place. This seems to be confusing the script parsing of the parent, causing the issue. When I rebuilt and test in the primary directory, the failures disappeared because no @2. t5562 also seems to have the same issue. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 18:04 Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker @ 2022-06-20 18:46 ` Derrick Stolee 2022-06-20 18:59 ` rsbecker 2022-06-21 13:51 ` Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker 1 sibling, 1 reply; 37+ messages in thread From: Derrick Stolee @ 2022-06-20 18:46 UTC (permalink / raw) To: rsbecker, 'Junio C Hamano'; +Cc: git, avarab On 6/20/22 2:04 PM, rsbecker@nexbridge.com wrote: > Subtests t5510.69, 71, 97, 99, 125, 127, 147, 149, 151, 153, 155, 157, 159, 161, 163, 165, 167, 170 failed in rc1 on the NonStop ia64 platform. These were working in rc0, but after analysis, I do not think that is relevant. Here is what we see for subtest 69: > > expecting success of 5510.69 'link prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --prune origin refs/tags/*:refs/tags/*; branch:kept tag:pruned': > ... > git fetch "file:///home/jenkins/.jenkins/workspace/Git_Pipeline@2/t/trash directory.t5510-fetch/." "+refs/heads/*:refs/remotes/origin/*" && > ... > git$git_fetch_c fetch --prune "file:///home/jenkins/.jenkins/workspace/Git_Pipeline/t/trash directory.t5510-fetch/." refs/tags/*:refs/tags/* && > > What seems to be happening is that the test script is computing the wrong parent directory in the second case, cutting the @2. We use Jenkins for our builds and occasional multiple builds run. The @2 is Jenkins way of running in another place. This seems to be confusing the script parsing of the parent, causing the issue. When I rebuilt and test in the primary directory, the failures disappeared because no @2. Yes, the script is incorrectly computing the remote URL. This is not a bug in Git, but a bug in the test script. The issue is this line (some tabs removed): new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') At this point, $remote_url contains the file path including the @ symbol. However, this perl invocation is dropping everything starting at the @ to the next slash. I'm not sure of a better way to accomplish what is trying to be done here (replace 'origin' with that specific url) without maybe causing other issues. This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09). Thanks, -Stolee ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 18:46 ` Derrick Stolee @ 2022-06-20 18:59 ` rsbecker 2022-06-20 20:00 ` Derrick Stolee 0 siblings, 1 reply; 37+ messages in thread From: rsbecker @ 2022-06-20 18:59 UTC (permalink / raw) To: 'Derrick Stolee', 'Junio C Hamano'; +Cc: git, avarab On June 20, 2022 2:46 PM, Derrick Stolee wrote: >On 6/20/22 2:04 PM, rsbecker@nexbridge.com wrote: >> Subtests t5510.69, 71, 97, 99, 125, 127, 147, 149, 151, 153, 155, 157, 159, 161, 163, >165, 167, 170 failed in rc1 on the NonStop ia64 platform. These were working in rc0, >but after analysis, I do not think that is relevant. Here is what we see for subtest >69: >> >> expecting success of 5510.69 'link prune fetch.prune=unset >remote.origin.prune=unset fetch.pruneTags=unset >remote.origin.pruneTags=unset --prune origin refs/tags/*:refs/tags/*; >branch:kept tag:pruned': >> ... >> git fetch >> "file:///home/jenkins/.jenkins/workspace/Git_Pipeline@2/t/trash >directory.t5510-fetch/." "+refs/heads/*:refs/remotes/origin/*" && ... >> git$git_fetch_c fetch --prune >> "file:///home/jenkins/.jenkins/workspace/Git_Pipeline/t/trash >> directory.t5510-fetch/." refs/tags/*:refs/tags/* && >> >> What seems to be happening is that the test script is computing the wrong >parent directory in the second case, cutting the @2. We use Jenkins for our builds >and occasional multiple builds run. The @2 is Jenkins way of running in another >place. This seems to be confusing the script parsing of the parent, causing the >issue. When I rebuilt and test in the primary directory, the failures disappeared >because no @2. > >Yes, the script is incorrectly computing the remote URL. This is not a bug in Git, but >a bug in the test script. > >The issue is this line (some tabs removed): > > new_cmdline=$(printf "%s" "$cmdline" | perl -pe >'s[origin(?!/)]["'"$remote_url"'"]g') > >At this point, $remote_url contains the file path including the @ symbol. However, >this perl invocation is dropping everything starting at the @ to the next slash. > >I'm not sure of a better way to accomplish what is trying to be done here (replace >'origin' with that specific url) without maybe causing other issues. > >This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as >fetch [<remote>], 2018-02-09). How about using sed instead of perl for this? ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 18:59 ` rsbecker @ 2022-06-20 20:00 ` Derrick Stolee 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason ` (3 more replies) 0 siblings, 4 replies; 37+ messages in thread From: Derrick Stolee @ 2022-06-20 20:00 UTC (permalink / raw) To: rsbecker, 'Junio C Hamano'; +Cc: git, avarab On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: > On June 20, 2022 2:46 PM, Derrick Stolee wrote: >> The issue is this line (some tabs removed): >> >> new_cmdline=$(printf "%s" "$cmdline" | perl -pe >> 's[origin(?!/)]["'"$remote_url"'"]g') >> >> At this point, $remote_url contains the file path including the @ symbol. However, >> this perl invocation is dropping everything starting at the @ to the next slash. >> >> I'm not sure of a better way to accomplish what is trying to be done here (replace >> 'origin' with that specific url) without maybe causing other issues. >> >> This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as >> fetch [<remote>], 2018-02-09). > > How about using sed instead of perl for this? I wasn't sure if using sed would create a different kind of replacement problem, but using single-quotes seems to get around that kind of issue. Please see the patch below. I'm currently running CI in a GGG PR [1] [1] https://github.com/gitgitgadget/git/pull/1267 Thanks, -Stolee --- >8 --- From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 2001 From: Derrick Stolee <derrickstolee@github.com> Date: Mon, 20 Jun 2022 15:52:09 -0400 Subject: [PATCH] t5510: replace 'origin' with URL more carefully The many test_configured_prune tests in t5510-fetch.sh test many combinations of --prune, --prune-tags, and using 'origin' or an explicit URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace 'origin' with this explicit URL. This URL is a "file:///" URL for the root of the $TRASH_DIRECTORY. However, if the current build tree has an '@' symbol, the replacement using perl fails. It drops the '@' as well as anything else in that directory name. You can verify this locally by cloning git.git into a "victim@03" directory and running the test script. To resolve this issue, replace the perl invocation with two sed commands. These two are used to ensure that we match exactly on the whole word 'origin'. We can guarantee that the word boundaries are spaces in our tests. The reason to use exact words is that sometimes a refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" which would cause an incorrect replacement. The two commands are used because there is not a clear POSIX way to match on word boundaries without getting extremely pedantic about what possible characters we could have at the boundaries. Reported-by: Randall Becker <rsbecker@nexbridge.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com> --- t/t5510-fetch.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7fa..8ca3aa5e931 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -853,7 +853,9 @@ test_configured_prune_type () { then new_cmdline=$cmdline_setup else - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') + new_cmdline=$(printf "%s" "$cmdline" | \ + sed "s~origin ~'$remote_url' ~g" | \ + sed "s~ origin~ '$remote_url'~g") fi if test "$fetch_prune_tags" = 'true' || -- 2.36.1.vfs.0.0 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:00 ` Derrick Stolee @ 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason 2022-06-20 20:43 ` Junio C Hamano 2022-06-20 20:34 ` SZEDER Gábor ` (2 subsequent siblings) 3 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-20 20:30 UTC (permalink / raw) To: Derrick Stolee; +Cc: rsbecker, 'Junio C Hamano', git On Mon, Jun 20 2022, Derrick Stolee wrote: > On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: >> On June 20, 2022 2:46 PM, Derrick Stolee wrote: > >>> The issue is this line (some tabs removed): >>> >>> new_cmdline=$(printf "%s" "$cmdline" | perl -pe >>> 's[origin(?!/)]["'"$remote_url"'"]g') >>> >>> At this point, $remote_url contains the file path including the @ symbol. However, >>> this perl invocation is dropping everything starting at the @ to the next slash. >>> >>> I'm not sure of a better way to accomplish what is trying to be done here (replace >>> 'origin' with that specific url) without maybe causing other issues. >>> >>> This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as >>> fetch [<remote>], 2018-02-09). >> >> How about using sed instead of perl for this? > > I wasn't sure if using sed would create a different kind of replacement > problem, but using single-quotes seems to get around that kind of issue. > > Please see the patch below. I'm currently running CI in a GGG PR [1] > > [1] https://github.com/gitgitgadget/git/pull/1267 > > Thanks, > -Stolee > > > --- >8 --- > > From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 2001 > From: Derrick Stolee <derrickstolee@github.com> > Date: Mon, 20 Jun 2022 15:52:09 -0400 > Subject: [PATCH] t5510: replace 'origin' with URL more carefully > > The many test_configured_prune tests in t5510-fetch.sh test many > combinations of --prune, --prune-tags, and using 'origin' or an explicit > URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch > <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace > 'origin' with this explicit URL. This URL is a "file:///" URL for the > root of the $TRASH_DIRECTORY. > > However, if the current build tree has an '@' symbol, the replacement > using perl fails. It drops the '@' as well as anything else in that > directory name. > > You can verify this locally by cloning git.git into a "victim@03" > directory and running the test script. > > To resolve this issue, replace the perl invocation with two sed > commands. These two are used to ensure that we match exactly on the > whole word 'origin'. We can guarantee that the word boundaries are > spaces in our tests. The reason to use exact words is that sometimes a > refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" which > would cause an incorrect replacement. The two commands are used because > there is not a clear POSIX way to match on word boundaries without > getting extremely pedantic about what possible characters we could have > at the boundaries. > > Reported-by: Randall Becker <rsbecker@nexbridge.com> > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > t/t5510-fetch.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 4620f0ca7fa..8ca3aa5e931 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > + new_cmdline=$(printf "%s" "$cmdline" | \ > + sed "s~origin ~'$remote_url' ~g" | \ > + sed "s~ origin~ '$remote_url'~g") > fi > > if test "$fetch_prune_tags" = 'true' || Thanks for looking at this. Checking this out again this whole quoting mess is a bit of a ... mess, I wonder if there's some better way to avoid this. Anyway: So, is this functionally the same as: diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7fa..9cd8b36f835 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -853,7 +853,9 @@ test_configured_prune_type () { then new_cmdline=$cmdline_setup else - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') + new_cmdline=$(printf "%s" "$cmdline" | + perl -pe 's[origin ]["'"$remote_url"'" ]g' | + perl -pe 's[ origin][ "'"$remote_url"'"]g') fi if test "$fetch_prune_tags" = 'true' || ? I don't mind the migration to "sed", but doing so to fix a bug makes this especially hard to analyze. I.e. you've gotten rid of the (?!/), I haven't re-looked at this enough to see if/how that's important. I just came up with the above as a quick hack, but for any proper migration to sed can't we do this in one command? In any case you never need "| \" in your scripts, just end the line with "|". ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason @ 2022-06-20 20:43 ` Junio C Hamano 2022-06-20 21:24 ` rsbecker 2022-06-20 21:33 ` Ævar Arnfjörð Bjarmason 0 siblings, 2 replies; 37+ messages in thread From: Junio C Hamano @ 2022-06-20 20:43 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: Derrick Stolee, Randall S. Becker, Git Mailing List I was expecting you to use \Q...\E (and passing $remote_url as an argument to perl script) actually. If you let $remote_url interpolated by shell into a script of the host language, whether perl or sed, you'd be responsible for quoting it appropriately for the host language yourself, and use of single-quote pair would not necessarily be sufficient, no? On Mon, Jun 20, 2022 at 1:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > > > On Mon, Jun 20 2022, Derrick Stolee wrote: > > > On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: > >> On June 20, 2022 2:46 PM, Derrick Stolee wrote: > > > >>> The issue is this line (some tabs removed): > >>> > >>> new_cmdline=$(printf "%s" "$cmdline" | perl -pe > >>> 's[origin(?!/)]["'"$remote_url"'"]g') > >>> > >>> At this point, $remote_url contains the file path including the @ symbol. However, > >>> this perl invocation is dropping everything starting at the @ to the next slash. > >>> > >>> I'm not sure of a better way to accomplish what is trying to be done here (replace > >>> 'origin' with that specific url) without maybe causing other issues. > >>> > >>> This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as > >>> fetch [<remote>], 2018-02-09). > >> > >> How about using sed instead of perl for this? > > > > I wasn't sure if using sed would create a different kind of replacement > > problem, but using single-quotes seems to get around that kind of issue. > > > > Please see the patch below. I'm currently running CI in a GGG PR [1] > > > > [1] https://github.com/gitgitgadget/git/pull/1267 > > > > Thanks, > > -Stolee > > > > > > --- >8 --- > > > > From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 2001 > > From: Derrick Stolee <derrickstolee@github.com> > > Date: Mon, 20 Jun 2022 15:52:09 -0400 > > Subject: [PATCH] t5510: replace 'origin' with URL more carefully > > > > The many test_configured_prune tests in t5510-fetch.sh test many > > combinations of --prune, --prune-tags, and using 'origin' or an explicit > > URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch > > <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace > > 'origin' with this explicit URL. This URL is a "file:///" URL for the > > root of the $TRASH_DIRECTORY. > > > > However, if the current build tree has an '@' symbol, the replacement > > using perl fails. It drops the '@' as well as anything else in that > > directory name. > > > > You can verify this locally by cloning git.git into a "victim@03" > > directory and running the test script. > > > > To resolve this issue, replace the perl invocation with two sed > > commands. These two are used to ensure that we match exactly on the > > whole word 'origin'. We can guarantee that the word boundaries are > > spaces in our tests. The reason to use exact words is that sometimes a > > refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" which > > would cause an incorrect replacement. The two commands are used because > > there is not a clear POSIX way to match on word boundaries without > > getting extremely pedantic about what possible characters we could have > > at the boundaries. > > > > Reported-by: Randall Becker <rsbecker@nexbridge.com> > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > > --- > > t/t5510-fetch.sh | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > > index 4620f0ca7fa..8ca3aa5e931 100755 > > --- a/t/t5510-fetch.sh > > +++ b/t/t5510-fetch.sh > > @@ -853,7 +853,9 @@ test_configured_prune_type () { > > then > > new_cmdline=$cmdline_setup > > else > > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > > + new_cmdline=$(printf "%s" "$cmdline" | \ > > + sed "s~origin ~'$remote_url' ~g" | \ > > + sed "s~ origin~ '$remote_url'~g") > > fi > > > > if test "$fetch_prune_tags" = 'true' || > > Thanks for looking at this. Checking this out again this whole quoting > mess is a bit of a ... mess, I wonder if there's some better way to > avoid this. Anyway: > > So, is this functionally the same as: > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 4620f0ca7fa..9cd8b36f835 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > + new_cmdline=$(printf "%s" "$cmdline" | > + perl -pe 's[origin ]["'"$remote_url"'" ]g' | > + perl -pe 's[ origin][ "'"$remote_url"'"]g') > fi > > if test "$fetch_prune_tags" = 'true' || > > ? > > I don't mind the migration to "sed", but doing so to fix a bug makes > this especially hard to analyze. I.e. you've gotten rid of the (?!/), I > haven't re-looked at this enough to see if/how that's important. > > I just came up with the above as a quick hack, but for any proper > migration to sed can't we do this in one command? > > In any case you never need "| \" in your scripts, just end the line with > "|". ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:43 ` Junio C Hamano @ 2022-06-20 21:24 ` rsbecker 2022-06-20 21:33 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 37+ messages in thread From: rsbecker @ 2022-06-20 21:24 UTC (permalink / raw) To: 'Junio C Hamano', 'Ævar Arnfjörð Bjarmason' Cc: 'Derrick Stolee', 'Git Mailing List' On June 20, 2022 4:43 PM, Junio C Hamano wrote: >I was expecting you to use \Q...\E (and passing $remote_url as an argument to >perl script) actually. If you let $remote_url interpolated by shell into a script of the >host language, whether perl or sed, you'd be responsible for quoting it >appropriately for the host language yourself, and use of single-quote pair would >not necessarily be sufficient, no? That would make it easier to debug. In the mean time, I am trying to get the environment to test with the problematic path and am having issues after renaming the directory. > >On Mon, Jun 20, 2022 at 1:34 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> >wrote: >> >> >> On Mon, Jun 20 2022, Derrick Stolee wrote: >> >> > On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: >> >> On June 20, 2022 2:46 PM, Derrick Stolee wrote: >> > >> >>> The issue is this line (some tabs removed): >> >>> >> >>> new_cmdline=$(printf "%s" "$cmdline" | perl -pe >> >>> 's[origin(?!/)]["'"$remote_url"'"]g') >> >>> >> >>> At this point, $remote_url contains the file path including the @ >> >>> symbol. However, this perl invocation is dropping everything starting at the >@ to the next slash. >> >>> >> >>> I'm not sure of a better way to accomplish what is trying to be >> >>> done here (replace 'origin' with that specific url) without maybe causing >other issues. >> >>> >> >>> This line was introduced by e1790f9245f (fetch tests: fetch <url> >> >>> <spec> as well as fetch [<remote>], 2018-02-09). >> >> >> >> How about using sed instead of perl for this? >> > >> > I wasn't sure if using sed would create a different kind of >> > replacement problem, but using single-quotes seems to get around that kind >of issue. >> > >> > Please see the patch below. I'm currently running CI in a GGG PR [1] >> > >> > [1] https://github.com/gitgitgadget/git/pull/1267 >> > >> > Thanks, >> > -Stolee >> > >> > >> > --- >8 --- >> > >> > From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 >> > 2001 >> > From: Derrick Stolee <derrickstolee@github.com> >> > Date: Mon, 20 Jun 2022 15:52:09 -0400 >> > Subject: [PATCH] t5510: replace 'origin' with URL more carefully >> > >> > The many test_configured_prune tests in t5510-fetch.sh test many >> > combinations of --prune, --prune-tags, and using 'origin' or an >> > explicit URL. Some machinery was introduced in e1790f9245f (fetch >> > tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) >> > to replace 'origin' with this explicit URL. This URL is a "file:///" >> > URL for the root of the $TRASH_DIRECTORY. >> > >> > However, if the current build tree has an '@' symbol, the >> > replacement using perl fails. It drops the '@' as well as anything >> > else in that directory name. >> > >> > You can verify this locally by cloning git.git into a "victim@03" >> > directory and running the test script. >> > >> > To resolve this issue, replace the perl invocation with two sed >> > commands. These two are used to ensure that we match exactly on the >> > whole word 'origin'. We can guarantee that the word boundaries are >> > spaces in our tests. The reason to use exact words is that sometimes >> > a refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" >> > which would cause an incorrect replacement. The two commands are >> > used because there is not a clear POSIX way to match on word >> > boundaries without getting extremely pedantic about what possible >> > characters we could have at the boundaries. >> > >> > Reported-by: Randall Becker <rsbecker@nexbridge.com> >> > Signed-off-by: Derrick Stolee <derrickstolee@github.com> >> > --- >> > t/t5510-fetch.sh | 4 +++- >> > 1 file changed, 3 insertions(+), 1 deletion(-) >> > >> > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index >> > 4620f0ca7fa..8ca3aa5e931 100755 >> > --- a/t/t5510-fetch.sh >> > +++ b/t/t5510-fetch.sh >> > @@ -853,7 +853,9 @@ test_configured_prune_type () { >> > then >> > new_cmdline=$cmdline_setup >> > else >> > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe >'s[origin(?!/)]["'"$remote_url"'"]g') >> > + new_cmdline=$(printf "%s" "$cmdline" | \ >> > + sed "s~origin ~'$remote_url' ~g" | \ >> > + sed "s~ origin~ >> > + '$remote_url'~g") >> > fi >> > >> > if test "$fetch_prune_tags" = 'true' || >> >> Thanks for looking at this. Checking this out again this whole quoting >> mess is a bit of a ... mess, I wonder if there's some better way to >> avoid this. Anyway: >> >> So, is this functionally the same as: >> >> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh >> index 4620f0ca7fa..9cd8b36f835 100755 >> --- a/t/t5510-fetch.sh >> +++ b/t/t5510-fetch.sh >> @@ -853,7 +853,9 @@ test_configured_prune_type () { >> then >> new_cmdline=$cmdline_setup >> else >> - new_cmdline=$(printf "%s" "$cmdline" | perl -pe >'s[origin(?!/)]["'"$remote_url"'"]g') >> + new_cmdline=$(printf "%s" "$cmdline" | >> + perl -pe 's[origin ]["'"$remote_url"'" ]g' | >> + perl -pe 's[ origin][ "'"$remote_url"'"]g') >> fi >> >> if test "$fetch_prune_tags" = 'true' || >> >> ? >> >> I don't mind the migration to "sed", but doing so to fix a bug makes >> this especially hard to analyze. I.e. you've gotten rid of the (?!/), >> I haven't re-looked at this enough to see if/how that's important. >> >> I just came up with the above as a quick hack, but for any proper >> migration to sed can't we do this in one command? >> >> In any case you never need "| \" in your scripts, just end the line >> with "|". ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:43 ` Junio C Hamano 2022-06-20 21:24 ` rsbecker @ 2022-06-20 21:33 ` Ævar Arnfjörð Bjarmason 1 sibling, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-20 21:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: Derrick Stolee, Randall S. Becker, Git Mailing List On Mon, Jun 20 2022, Junio C Hamano wrote: > I was expecting you to use \Q...\E (and passing $remote_url as an > argument to perl script) actually. If you let $remote_url > interpolated by shell into a script of the host language, whether perl > or sed, you'd be responsible for quoting it appropriately for the host > language yourself, and use of single-quote pair would not necessarily > be sufficient, no? Yes, I wasn't aiming to try to come up with an ideal Perl version, just wondering why it needed the s/perl/sed/ while-we're-at-it... ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:00 ` Derrick Stolee 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason @ 2022-06-20 20:34 ` SZEDER Gábor 2022-06-20 22:17 ` rsbecker 2022-06-20 22:20 ` [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) Derrick Stolee 3 siblings, 0 replies; 37+ messages in thread From: SZEDER Gábor @ 2022-06-20 20:34 UTC (permalink / raw) To: Derrick Stolee; +Cc: rsbecker, 'Junio C Hamano', git, avarab On Mon, Jun 20, 2022 at 04:00:03PM -0400, Derrick Stolee wrote: > On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: > > On June 20, 2022 2:46 PM, Derrick Stolee wrote: > > >> The issue is this line (some tabs removed): > >> > >> new_cmdline=$(printf "%s" "$cmdline" | perl -pe > >> 's[origin(?!/)]["'"$remote_url"'"]g') > >> > >> At this point, $remote_url contains the file path including the @ symbol. However, > >> this perl invocation is dropping everything starting at the @ to the next slash. > >> > >> I'm not sure of a better way to accomplish what is trying to be done here (replace > >> 'origin' with that specific url) without maybe causing other issues. > >> > >> This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as > >> fetch [<remote>], 2018-02-09). > > > > How about using sed instead of perl for this? > > I wasn't sure if using sed would create a different kind of replacement > problem, but using single-quotes seems to get around that kind of issue. In a 'sed s/regexp/replacement/' command the replacement part has a few characters with special meaning, and if those characters happen to appear in the path of the trash directory, then: $ ./t5510-fetch.sh --root='/tmp/foo\1/' ok 64 - name prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --no-prune; branch:kept tag:kept sed: -e expression #1, char 62: invalid reference \1 on `s' command's RHS sed: -e expression #1, char 62: invalid reference \1 on `s' command's RHS ok 65 - link prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --no-prune; branch:kept tag:kept ok 66 - name prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --prune; branch:pruned tag:kept sed: -e expression #1, char 62: invalid reference \1 on `s' command's RHS sed: -e expression #1, char 62: invalid reference \1 on `s' command's RHS not ok 67 - link prune fetch.prune=unset remote.origin.prune=unset fetch.pruneTags=unset remote.origin.pruneTags=unset --prune; branch:pruned tag:kept [...] # failed 28 among 183 test(s) > Please see the patch below. I'm currently running CI in a GGG PR [1] > > [1] https://github.com/gitgitgadget/git/pull/1267 > > Thanks, > -Stolee > > > --- >8 --- > > >From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 2001 > From: Derrick Stolee <derrickstolee@github.com> > Date: Mon, 20 Jun 2022 15:52:09 -0400 > Subject: [PATCH] t5510: replace 'origin' with URL more carefully > > The many test_configured_prune tests in t5510-fetch.sh test many > combinations of --prune, --prune-tags, and using 'origin' or an explicit > URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch > <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace > 'origin' with this explicit URL. This URL is a "file:///" URL for the > root of the $TRASH_DIRECTORY. > > However, if the current build tree has an '@' symbol, the replacement > using perl fails. It drops the '@' as well as anything else in that > directory name. > > You can verify this locally by cloning git.git into a "victim@03" > directory and running the test script. > > To resolve this issue, replace the perl invocation with two sed > commands. These two are used to ensure that we match exactly on the > whole word 'origin'. We can guarantee that the word boundaries are > spaces in our tests. The reason to use exact words is that sometimes a > refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" which > would cause an incorrect replacement. The two commands are used because > there is not a clear POSIX way to match on word boundaries without > getting extremely pedantic about what possible characters we could have > at the boundaries. > > Reported-by: Randall Becker <rsbecker@nexbridge.com> > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > t/t5510-fetch.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 4620f0ca7fa..8ca3aa5e931 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > + new_cmdline=$(printf "%s" "$cmdline" | \ > + sed "s~origin ~'$remote_url' ~g" | \ > + sed "s~ origin~ '$remote_url'~g") 'sed' can run multiple commands at once, e.g. 'sed -e s/// -e s///'. > fi > > if test "$fetch_prune_tags" = 'true' || > -- > 2.36.1.vfs.0.0 > > > ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 20:00 ` Derrick Stolee 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason 2022-06-20 20:34 ` SZEDER Gábor @ 2022-06-20 22:17 ` rsbecker 2022-06-20 22:20 ` [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) Derrick Stolee 3 siblings, 0 replies; 37+ messages in thread From: rsbecker @ 2022-06-20 22:17 UTC (permalink / raw) To: 'Derrick Stolee', 'Junio C Hamano'; +Cc: git, avarab >-----Original Message----- >From: Derrick Stolee <derrickstolee@github.com> >Sent: June 20, 2022 4:00 PM >To: rsbecker@nexbridge.com; 'Junio C Hamano' <gitster@pobox.com> >Cc: git@vger.kernel.org; avarab@gmail.com >Subject: Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 > >On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: >> On June 20, 2022 2:46 PM, Derrick Stolee wrote: > >>> The issue is this line (some tabs removed): >>> >>> new_cmdline=$(printf "%s" "$cmdline" | perl -pe >>> 's[origin(?!/)]["'"$remote_url"'"]g') >>> >>> At this point, $remote_url contains the file path including the @ >>> symbol. However, this perl invocation is dropping everything starting at the @ >to the next slash. >>> >>> I'm not sure of a better way to accomplish what is trying to be done >>> here (replace 'origin' with that specific url) without maybe causing other issues. >>> >>> This line was introduced by e1790f9245f (fetch tests: fetch <url> >>> <spec> as well as fetch [<remote>], 2018-02-09). >> >> How about using sed instead of perl for this? > >I wasn't sure if using sed would create a different kind of replacement problem, >but using single-quotes seems to get around that kind of issue. > >Please see the patch below. I'm currently running CI in a GGG PR [1] > >[1] https://github.com/gitgitgadget/git/pull/1267 > >Thanks, >-Stolee > > >--- >8 --- > >>From 1df4fc66d4a62adc7087d7d22c8d78842b4e9b4d Mon Sep 17 00:00:00 2001 >From: Derrick Stolee <derrickstolee@github.com> >Date: Mon, 20 Jun 2022 15:52:09 -0400 >Subject: [PATCH] t5510: replace 'origin' with URL more carefully > >The many test_configured_prune tests in t5510-fetch.sh test many combinations >of --prune, --prune-tags, and using 'origin' or an explicit URL. Some machinery was >introduced in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch >[<remote>], 2018-02-09) to replace 'origin' with this explicit URL. This URL is a >"file:///" URL for the root of the $TRASH_DIRECTORY. > >However, if the current build tree has an '@' symbol, the replacement using perl >fails. It drops the '@' as well as anything else in that directory name. > >You can verify this locally by cloning git.git into a "victim@03" >directory and running the test script. > >To resolve this issue, replace the perl invocation with two sed commands. These >two are used to ensure that we match exactly on the whole word 'origin'. We can >guarantee that the word boundaries are spaces in our tests. The reason to use >exact words is that sometimes a refspec is supplied, such as >"+refs/heads/*:refs/remotes/origin/*" which would cause an incorrect >replacement. The two commands are used because there is not a clear POSIX way >to match on word boundaries without getting extremely pedantic about what >possible characters we could have at the boundaries. > >Reported-by: Randall Becker <rsbecker@nexbridge.com> >Signed-off-by: Derrick Stolee <derrickstolee@github.com> >--- > t/t5510-fetch.sh | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > >diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7fa..8ca3aa5e931 >100755 >--- a/t/t5510-fetch.sh >+++ b/t/t5510-fetch.sh >@@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else >- new_cmdline=$(printf "%s" "$cmdline" | perl -pe >'s[origin(?!/)]["'"$remote_url"'"]g') >+ new_cmdline=$(printf "%s" "$cmdline" | \ >+ sed "s~origin ~'$remote_url' ~g" | \ >+ sed "s~ origin~ '$remote_url'~g") > fi > > if test "$fetch_prune_tags" = 'true' || Ok, this seems to work. I have the test environment set up so can retest other options. ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-20 20:00 ` Derrick Stolee ` (2 preceding siblings ...) 2022-06-20 22:17 ` rsbecker @ 2022-06-20 22:20 ` Derrick Stolee 2022-06-21 5:28 ` Johannes Sixt 2022-06-21 7:17 ` Jeff King 3 siblings, 2 replies; 37+ messages in thread From: Derrick Stolee @ 2022-06-20 22:20 UTC (permalink / raw) To: rsbecker, 'Junio C Hamano'; +Cc: git, avarab On 6/20/2022 4:00 PM, Derrick Stolee wrote: > On 6/20/22 2:59 PM, rsbecker@nexbridge.com wrote: >> On June 20, 2022 2:46 PM, Derrick Stolee wrote: > >>> The issue is this line (some tabs removed): >>> >>> new_cmdline=$(printf "%s" "$cmdline" | perl -pe >>> 's[origin(?!/)]["'"$remote_url"'"]g') >>> >>> At this point, $remote_url contains the file path including the @ symbol. However, >>> this perl invocation is dropping everything starting at the @ to the next slash. >>> >>> I'm not sure of a better way to accomplish what is trying to be done here (replace >>> 'origin' with that specific url) without maybe causing other issues. >>> >>> This line was introduced by e1790f9245f (fetch tests: fetch <url> <spec> as well as >>> fetch [<remote>], 2018-02-09). >> >> How about using sed instead of perl for this? > > I wasn't sure if using sed would create a different kind of replacement > problem, but using single-quotes seems to get around that kind of issue. > > Please see the patch below. I'm currently running CI in a GGG PR [1] Here is a v2 that updates the sed call based on the feedback so far. I'm basically switching to sed because I don't know perl and the discussion about how to fix this replacement command is completely over my head. That makes me think that using a simpler find-and-replace tool like sed would be good here. Thanks, -Stolee --- >8 --- From 7fae22e0ad09af00de5a294f61dfd29cb349feeb Mon Sep 17 00:00:00 2001 From: Derrick Stolee <derrickstolee@github.com> Date: Mon, 20 Jun 2022 15:52:09 -0400 Subject: [PATCH v2] t5510: replace 'origin' with URL more carefully The many test_configured_prune tests in t5510-fetch.sh test many combinations of --prune, --prune-tags, and using 'origin' or an explicit URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace 'origin' with this explicit URL. This URL is a "file:///" URL for the root of the $TRASH_DIRECTORY. However, if the current build tree has an '@' symbol, the replacement using perl fails. It drops the '@' as well as anything else in that directory name. You can verify this locally by cloning git.git into a "victim@03" directory and running the test script. To resolve this issue, replace the perl invocation with two sed commands. These two are used to ensure that we match exactly on the whole word 'origin'. We can guarantee that the word boundaries are spaces in our tests. The reason to use exact words is that sometimes a refspec is supplied, such as "+refs/heads/*:refs/remotes/origin/*" which would cause an incorrect replacement. The two commands are used because there is not a clear POSIX way to match on word boundaries without getting extremely pedantic about what possible characters we could have at the boundaries. Reported-by: Randall Becker <rsbecker@nexbridge.com> Signed-off-by: Derrick Stolee <derrickstolee@github.com> --- t/t5510-fetch.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7fa..c255a77e18a 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -853,7 +853,9 @@ test_configured_prune_type () { then new_cmdline=$cmdline_setup else - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') + new_cmdline=$(printf "%s" "$cmdline" | + sed -e "s~origin ~'$remote_url' ~g" \ + -e "s~ origin~ '$remote_url'~g") fi if test "$fetch_prune_tags" = 'true' || -- 2.36.1.220.g1fae7daf425 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-20 22:20 ` [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) Derrick Stolee @ 2022-06-21 5:28 ` Johannes Sixt 2022-06-21 7:17 ` Jeff King 1 sibling, 0 replies; 37+ messages in thread From: Johannes Sixt @ 2022-06-21 5:28 UTC (permalink / raw) To: Derrick Stolee; +Cc: git, avarab, rsbecker, 'Junio C Hamano' Am 21.06.22 um 00:20 schrieb Derrick Stolee: > @@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > + new_cmdline=$(printf "%s" "$cmdline" | > + sed -e "s~origin ~'$remote_url' ~g" \ > + -e "s~ origin~ '$remote_url'~g") I'm not sure if this is still an issue, but I recall that there are seds in the wild that do not grok an incomplete last line. Better make this `printf "%s\n" "$cmdline"` if `echo "$cmdline"` is not an option. > fi > > if test "$fetch_prune_tags" = 'true' || -- Hannes ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-20 22:20 ` [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) Derrick Stolee 2022-06-21 5:28 ` Johannes Sixt @ 2022-06-21 7:17 ` Jeff King 2022-06-21 9:29 ` SZEDER Gábor 2022-06-21 16:35 ` Junio C Hamano 1 sibling, 2 replies; 37+ messages in thread From: Jeff King @ 2022-06-21 7:17 UTC (permalink / raw) To: Derrick Stolee; +Cc: rsbecker, 'Junio C Hamano', git, avarab On Mon, Jun 20, 2022 at 06:20:07PM -0400, Derrick Stolee wrote: > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 4620f0ca7fa..c255a77e18a 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -853,7 +853,9 @@ test_configured_prune_type () { > then > new_cmdline=$cmdline_setup > else > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > + new_cmdline=$(printf "%s" "$cmdline" | > + sed -e "s~origin ~'$remote_url' ~g" \ > + -e "s~ origin~ '$remote_url'~g") > fi Doesn't this introduce a new problem if $remote_url contains a tilde? Unlikely, but I thought the point of the exercise was defending against funny paths. Getting this bullet-proof with sed is tricky, I think. I didn't follow all the other logic that that might need fixed, but handling this in perl is easy by passing the string on the command line, like: printf "%s" "$cmdline" | perl -pe ' BEGIN { $url = shift } s[origin(?!/)]['\''$url'\'']g; ' "$remote_url" For that matter, using printf and "perl -p" is a little silly, since we know there is only a single string to modify. Perhaps: perl -e ' my ($cmdline, $url) = @ARGV; $cmdline =~ s[origin(?!/)]['\''$url'\'']g; print $cmdline; ' -- "$cmdline" "$remote_url" And then further changes are easy: - you could replace the ad-hoc "I hope single quotes are enough" quoting of $url with a real regex - you can define $sq inside the perl script to avoid the gross '\'' quoting (or even avoid it entirely with quotemeta) So perhaps something like: perl -e ' my ($cmdline, $url) = @ARGV; $cmdline =~ s[origin(?!/)][quotemeta($url)]ge; print $cmdline; ' -- "$cmdline" "$remote_url" I don't mean to golf on this forever, but I wanted to show something concrete since you said you don't know perl well. I just think moving to sed introduces more opportunities for errors here, not fewer. :) -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 7:17 ` Jeff King @ 2022-06-21 9:29 ` SZEDER Gábor 2022-06-21 10:07 ` Jeff King 2022-06-21 16:35 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: SZEDER Gábor @ 2022-06-21 9:29 UTC (permalink / raw) To: Jeff King; +Cc: Derrick Stolee, rsbecker, 'Junio C Hamano', git, avarab On Tue, Jun 21, 2022 at 03:17:04AM -0400, Jeff King wrote: > On Mon, Jun 20, 2022 at 06:20:07PM -0400, Derrick Stolee wrote: > > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > > index 4620f0ca7fa..c255a77e18a 100755 > > --- a/t/t5510-fetch.sh > > +++ b/t/t5510-fetch.sh > > @@ -853,7 +853,9 @@ test_configured_prune_type () { > > then > > new_cmdline=$cmdline_setup > > else > > - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') > > + new_cmdline=$(printf "%s" "$cmdline" | > > + sed -e "s~origin ~'$remote_url' ~g" \ > > + -e "s~ origin~ '$remote_url'~g") > > fi > > Doesn't this introduce a new problem if $remote_url contains a tilde? > Unlikely, but I thought the point of the exercise was defending against > funny paths. And it doesn't replace a standalone "origin" word, either, which does occur a few times in the test script. > So perhaps something like: > > perl -e ' > my ($cmdline, $url) = @ARGV; > $cmdline =~ s[origin(?!/)][quotemeta($url)]ge; I don't like this "(?!/)" magic, because I haven't got the slightest idea of what it might do by merely looking at it, and these characters are not exactly easy to search for. The good old "add a space prefix and suffix" trick can help to easily match the "origin" word even when it stands alone, but, alas, the result is still not as simple as I'd like with the \s and the string concatenation: perl -e ' new_cmdline=$(perl -e ' my ($cmdline, $url) = @ARGV; $cmdline =~ s[\sorigin\s][" " . quotemeta($url) . " "]ge; print $cmdline; ' -- " $cmdline " "$remote_url") (The 'sed' variant looks good to me: printf " %s " "$cmdline" |sed "s~ origin ~ '$remote_url' ~g" but then again it just trades one set of troublesome characters for an other.) > print $cmdline; > ' -- "$cmdline" "$remote_url" > > I don't mean to golf on this forever, but I wanted to show something > concrete since you said you don't know perl well. I just think moving to > sed introduces more opportunities for errors here, not fewer. :) > > -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 9:29 ` SZEDER Gábor @ 2022-06-21 10:07 ` Jeff King 0 siblings, 0 replies; 37+ messages in thread From: Jeff King @ 2022-06-21 10:07 UTC (permalink / raw) To: SZEDER Gábor Cc: Derrick Stolee, rsbecker, 'Junio C Hamano', git, avarab On Tue, Jun 21, 2022 at 11:29:15AM +0200, SZEDER Gábor wrote: > > So perhaps something like: > > > > perl -e ' > > my ($cmdline, $url) = @ARGV; > > $cmdline =~ s[origin(?!/)][quotemeta($url)]ge; > > I don't like this "(?!/)" magic, because I haven't got the slightest > idea of what it might do by merely looking at it, and these characters > are not exactly easy to search for. Yeah, I hadn't really dug into the rest of the thread and didn't understand what that part was trying to do. So I left it untouched in my examples as an exercise for the reader. :) > The good old "add a space prefix and suffix" trick can help to easily > match the "origin" word even when it stands alone, but, alas, the > result is still not as simple as I'd like with the \s and the string > concatenation: > > perl -e ' > new_cmdline=$(perl -e ' > my ($cmdline, $url) = @ARGV; > $cmdline =~ s[\sorigin\s][" " . quotemeta($url) . " "]ge; > print $cmdline; > ' -- " $cmdline " "$remote_url") If you do: $url = quotemeta($url); then you can drop the "e" from the regex, which gets rid of the gross concatenation: $cmdline =~ s[\sorigin\s][ $url ]; I think "\b" for a word boundary would avoid the whitespace prefix/suffix hackery, but it also matches non-alphabetics (like "/"). You could use alternation, though, like: $ cmdline='origin notorigin origin originnot origin/foo origin' $ remote_url=real_url $ perl -e ' my ($cmdline, $url) = @ARGV; $url = quotemeta($url); $cmdline =~ s/(\s|^)origin(\s|$)/$1$url$2/g; print $cmdline; ' "$cmdline" "$remote_url" real_url notorigin real_url originnot origin/foo real_url Negative lookbehind and lookahead get rid of the "$1" and "$2", but they are magical-looking, as you noted above. Possibly "/x" and some whitespace would make the whole thing more readable. -Peff ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 7:17 ` Jeff King 2022-06-21 9:29 ` SZEDER Gábor @ 2022-06-21 16:35 ` Junio C Hamano 2022-06-21 20:27 ` Junio C Hamano 1 sibling, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2022-06-21 16:35 UTC (permalink / raw) To: Jeff King; +Cc: Derrick Stolee, rsbecker, git, avarab Jeff King <peff@peff.net> writes: > And then further changes are easy: > > - you could replace the ad-hoc "I hope single quotes are enough" > quoting of $url with a real regex Yes, this is what I found a bit queasy about in the whole thing. > - you can define $sq inside the perl script to avoid the gross '\'' > quoting (or even avoid it entirely with quotemeta) > > So perhaps something like: > > perl -e ' > my ($cmdline, $url) = @ARGV; > $cmdline =~ s[origin(?!/)][quotemeta($url)]ge; > print $cmdline; > ' -- "$cmdline" "$remote_url" Yup, a solution along that line was what I expected to see from those who write Perl when I saw the discussion yesterday. FWIW, there are a few characters that won't be acceptable in the pathname for our source tree, including a single quote (I think some of our build procedures are loose in quoting, unlike parts of the Makefile that use $(FOO_SQ) for that) and a colon (testing the version of Git we just built needs the path of the build directory in GIT_EXEC_PATH, which is prepended to PATH at runtime during the test, but the colon means that the path is split into two there), so "I hope single quotes are enough" is not making things worse in practice, but quotemeta() is so simple---allowing us not having to worry about quoting is a very good thing. Thanks. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 16:35 ` Junio C Hamano @ 2022-06-21 20:27 ` Junio C Hamano 2022-06-21 21:13 ` Derrick Stolee 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2022-06-21 20:27 UTC (permalink / raw) To: git; +Cc: Derrick Stolee, rsbecker, Jeff King, avarab Junio C Hamano <gitster@pobox.com> writes: > Yup, a solution along that line was what I expected to see from > those who write Perl when I saw the discussion yesterday. Here is what I queued tentatively. This is not exactly new; e1790f92 (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) first appeared in v2.17.0 and we can live with the same glitch for few more weeks ;-) ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- From: Derrick Stolee <derrickstolee@github.com> Date: Mon, 20 Jun 2022 15:52:09 -0400 Subject: [PATCH] t5510: replace 'origin' with URL more carefully The many test_configured_prune tests in t5510-fetch.sh test many combinations of --prune, --prune-tags, and using 'origin' or an explicit URL. Some machinery was introduced in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) to replace 'origin' with this explicit URL. This URL is a "file:///" URL for the root of the $TRASH_DIRECTORY. However, if the current build tree has an '@' symbol, the replacement using perl fails. It drops the '@' as well as anything else in that directory name. You can observe this locally by cloning git.git into a "victim@03" directory and running the test script. As we are writing in Perl anyway, pass in the shell variables involved to the script as arguments and perform necessary string transformations inside it, instead of assuming that it is sufficient to enclose the $remote_url variable inside a pair of single quotes. Reported-by: Randall Becker <rsbecker@nexbridge.com> Original-patch-by: Derrick Stolee <derrickstolee@github.com> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t5510-fetch.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7f..b45879a760 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -853,7 +853,11 @@ test_configured_prune_type () { then new_cmdline=$cmdline_setup else - new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') + new_cmdline=$(perl -e ' + my ($cmdline, $url) = @ARGV; + $cmdline =~ s[origin(?!/)][quotemeta($url)]ge; + print $cmdline; + ' -- "$cmdline" "$remote_url") fi if test "$fetch_prune_tags" = 'true' || -- 2.37.0-rc1-99-g4626346e14 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 20:27 ` Junio C Hamano @ 2022-06-21 21:13 ` Derrick Stolee 2022-06-21 21:36 ` Junio C Hamano 0 siblings, 1 reply; 37+ messages in thread From: Derrick Stolee @ 2022-06-21 21:13 UTC (permalink / raw) To: Junio C Hamano, git; +Cc: rsbecker, Jeff King, avarab On 6/21/2022 4:27 PM, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> Yup, a solution along that line was what I expected to see from >> those who write Perl when I saw the discussion yesterday. > > Here is what I queued tentatively. This is not exactly new; > e1790f92 (fetch tests: fetch <url> <spec> as well as fetch > [<remote>], 2018-02-09) first appeared in v2.17.0 and we can > live with the same glitch for few more weeks ;-) There are also other tests that need similar updates before we consider this topic done. > ----- >8 --------- >8 --------- >8 --------- >8 --------- >8 ----- > From: Derrick Stolee <derrickstolee@github.com> > Original-patch-by: Derrick Stolee <derrickstolee@github.com> I recommend updating the authorship to yourself or Peff, since I was mostly useless in constructing the actual solution. Thanks, -Stolee ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) 2022-06-21 21:13 ` Derrick Stolee @ 2022-06-21 21:36 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2022-06-21 21:36 UTC (permalink / raw) To: Derrick Stolee; +Cc: git, rsbecker, Jeff King, avarab Derrick Stolee <derrickstolee@github.com> writes: > On 6/21/2022 4:27 PM, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> Yup, a solution along that line was what I expected to see from >>> those who write Perl when I saw the discussion yesterday. >> >> Here is what I queued tentatively. This is not exactly new; >> e1790f92 (fetch tests: fetch <url> <spec> as well as fetch >> [<remote>], 2018-02-09) first appeared in v2.17.0 and we can >> live with the same glitch for few more weeks ;-) > > There are also other tests that need similar updates before > we consider this topic done. If we can check with single-quotes and dollar-signs in addition to at-mark, that would be interesting. > I recommend updating the authorship to yourself or Peff, since I > was mostly useless in constructing the actual solution. Sure. That's what "tentatively" means. I didn't do anything (other than cheering from the sidelines), so... ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 00/10] t5510: fix the quoting mess 2022-06-21 21:36 ` Junio C Hamano @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 01/10] fetch tests: remove redundant test_unconfig() Ævar Arnfjörð Bjarmason ` (9 more replies) 0 siblings, 10 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Since the upthread quoting discussion pointed at a commit of mine I felt obligated to look at this in a deeper way. When I added those tests I carried forward some technical debt where we were passing arguments to a function as a string, instead of as multiple arguments which we could get with "$@", in hindsight that was a big mistake. As far as I can tell the proposed https://lore.kernel.org/git/xmqqy1xpg3th.fsf@gitster.g/ fixes the issue at hand, but this is the deeper and I think better direction to go in (but I could rebase in top of that fix if needed, although the conflict is trivial, keep this side). Now we don't use "perl", or "sed", or whatever to re-quote arguments in t/t5510-fetch.sh. There's still some weirdness in that test, e.g. the whole behavior of set_config_tristate(), but I had to stop somewhere. Ævar Arnfjörð Bjarmason (10): fetch tests: remove redundant test_unconfig() fetch tests: use named, not positional parameters fetch tests: use "local", &&-chaining, style etc. fetch tests: add a helper to avoid boilerplate fetch tests: pass "mode" parameter first, pave way for "$@" fetch tests: pass a list, not a string of arguments fetch tests: remove lazy variable setup fetch tests: remove shelling out for previously "lazy" variables fetch tests: stop implicitly adding refspecs fetch tests: fix needless and buggy re-quoting t/t5510-fetch.sh | 289 +++++++++++++++++++++++++++++------------------ 1 file changed, 178 insertions(+), 111 deletions(-) -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 01/10] fetch tests: remove redundant test_unconfig() 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-22 5:52 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 02/10] fetch tests: use named, not positional parameters Ævar Arnfjörð Bjarmason ` (8 subsequent siblings) 9 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason The test_unconfig() calls here were added as boilerplate in 737c5a9cde7 (fetch: make --prune configurable, 2013-07-13), and then faithfully reproduced in e249ce0ccdb (fetch tests: add scaffolding for the new fetch.pruneTags, 2018-02-09). But they were never necessary, so let's remove them. This actually improves our test coverage, as we'll now be asserting that whatever configuration we leave here (in the "one" block below) won't affect this particular "git fetch" command. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 4 ---- 1 file changed, 4 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 4620f0ca7fa..d784a761ba0 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -874,10 +874,6 @@ test_configured_prune_type () { git tag -f newtag && ( cd one && - test_unconfig fetch.prune && - test_unconfig fetch.pruneTags && - test_unconfig remote.origin.prune && - test_unconfig remote.origin.pruneTags && git fetch '"$cmdline_setup"' && git rev-parse --verify refs/remotes/origin/newbranch && git rev-parse --verify refs/tags/newtag -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] fetch tests: remove redundant test_unconfig() 2022-06-21 22:34 ` [PATCH 01/10] fetch tests: remove redundant test_unconfig() Ævar Arnfjörð Bjarmason @ 2022-06-22 5:52 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2022-06-22 5:52 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > The test_unconfig() calls here were added as boilerplate in > 737c5a9cde7 (fetch: make --prune configurable, 2013-07-13), and then > faithfully reproduced in e249ce0ccdb (fetch tests: add scaffolding for > the new fetch.pruneTags, 2018-02-09). But they were never necessary, unnecessary because ...? is it because nothing has happened in this directory before? is it because these will all be overriden by the command line options? is it because of something else? > so let's remove them. > This actually improves our test coverage, as we'll now be asserting > that whatever configuration we leave here (in the "one" block below) > won't affect this particular "git fetch" command. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > t/t5510-fetch.sh | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh > index 4620f0ca7fa..d784a761ba0 100755 > --- a/t/t5510-fetch.sh > +++ b/t/t5510-fetch.sh > @@ -874,10 +874,6 @@ test_configured_prune_type () { > git tag -f newtag && > ( > cd one && > - test_unconfig fetch.prune && > - test_unconfig fetch.pruneTags && > - test_unconfig remote.origin.prune && > - test_unconfig remote.origin.pruneTags && > git fetch '"$cmdline_setup"' && > git rev-parse --verify refs/remotes/origin/newbranch && > git rev-parse --verify refs/tags/newtag ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 02/10] fetch tests: use named, not positional parameters 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 01/10] fetch tests: remove redundant test_unconfig() Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 03/10] fetch tests: use "local", &&-chaining, style etc Ævar Arnfjörð Bjarmason ` (7 subsequent siblings) 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Use the named parameters we've already unpacked from "$@" rather than the positional paremeters. There's no functional changes here. This changes code that dates back to the initial introduction of the function in 737c5a9cde7 (fetch: make --prune configurable, 2013-07-13), although e.g. e249ce0ccdb (fetch tests: add scaffolding for the new fetch.pruneTags, 2018-02-09) added many more parameters. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index d784a761ba0..37e12de2475 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -868,7 +868,7 @@ test_configured_prune_type () { cmdline="$new_cmdline" fi - test_expect_success "$mode prune fetch.prune=$1 remote.origin.prune=$2 fetch.pruneTags=$3 remote.origin.pruneTags=$4${7:+ $7}; branch:$5 tag:$6" ' + 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" ' # make sure a newbranch is there in . and also in one git branch -f newbranch && git tag -f newtag && -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 03/10] fetch tests: use "local", &&-chaining, style etc. 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 01/10] fetch tests: remove redundant test_unconfig() Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 02/10] fetch tests: use named, not positional parameters Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 04/10] fetch tests: add a helper to avoid boilerplate Ævar Arnfjörð Bjarmason ` (6 subsequent siblings) 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Make use of &&-chaining, the "local" keyword, the more idiomatic "test -z" over comparison against "" etc. in the "test_configured_prune_type()" function. Let's also move the wrapper function to below the function itself. In e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) placed in there to minimize the diff size. While it's not incorrect to define these in this order (this isn't C), it's less confusing this way. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 37e12de2475..799e69dc1b1 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -821,20 +821,15 @@ set_config_tristate () { esac } -test_configured_prune () { - test_configured_prune_type "$@" "name" - test_configured_prune_type "$@" "link" -} - test_configured_prune_type () { - fetch_prune=$1 - remote_origin_prune=$2 - fetch_prune_tags=$3 - remote_origin_prune_tags=$4 - expected_branch=$5 - expected_tag=$6 - cmdline=$7 - mode=$8 + local fetch_prune="$1" && + local remote_origin_prune="$2" && + local fetch_prune_tags="$3" && + local remote_origin_prune_tags="$4" && + local expected_branch="$5" && + local expected_tag="$6" && + local cmdline="$7" && + local mode="$8" && if test -z "$cmdline_setup" then @@ -843,18 +838,18 @@ test_configured_prune_type () { remote_fetch="$(git -C one config remote.origin.fetch)" && cmdline_setup="\"$remote_url\" \"$remote_fetch\"" ' - fi + fi && if test "$mode" = 'link' then - new_cmdline="" + new_cmdline="" && - if test "$cmdline" = "" + if test -z "$cmdline" then new_cmdline=$cmdline_setup else new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') - fi + fi && if test "$fetch_prune_tags" = 'true' || test "$remote_origin_prune_tags" = 'true' @@ -863,10 +858,10 @@ test_configured_prune_type () { then new_cmdline="$new_cmdline refs/tags/*:refs/tags/*" fi - fi + fi && cmdline="$new_cmdline" - 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" ' # make sure a newbranch is there in . and also in one @@ -915,6 +910,13 @@ test_configured_prune_type () { esac ) ' + + return 0 +} + +test_configured_prune () { + test_configured_prune_type "$@" "name" && + test_configured_prune_type "$@" "link" } # $1 config: fetch.prune -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 04/10] fetch tests: add a helper to avoid boilerplate 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (2 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 03/10] fetch tests: use "local", &&-chaining, style etc Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" Ævar Arnfjörð Bjarmason ` (5 subsequent siblings) 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Add a new test_configured_prune_type_branch() helper to avoid the boilerplate introduced in 97716d217c1 (fetch: add a --prune-tags option and fetch.pruneTags config, 2018-02-09). Back then it was somewhat necessary, but since 6317972cff9 (fetch: make the --prune-tags work with <url>, 2018-02-09) these tests have been regular enough that we can always pass the "kept" argument for "link", and "pruned" for "name". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 799e69dc1b1..5d118a6a806 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -1006,22 +1006,19 @@ test_configured_prune unset unset unset true pruned kept \ # remote. However, because there's no implicit # +refs/heads/*:refs/remotes/origin/* refspec and supplying it on the # command-line negates --prune-tags, the branches will not be pruned. +test_configured_prune_type_branch () { + test_configured_prune_type "$1" "$2" "$3" "$4" pruned "$6" "$7" "name" + test_configured_prune_type "$1" "$2" "$3" "$4" kept "$6" "$7" "link" +} test_configured_prune_type unset unset unset unset kept kept "origin --prune-tags" "name" test_configured_prune_type unset unset unset unset kept kept "origin --prune-tags" "link" -test_configured_prune_type unset unset unset unset pruned pruned "origin --prune --prune-tags" "name" -test_configured_prune_type unset unset unset unset kept pruned "origin --prune --prune-tags" "link" -test_configured_prune_type unset unset unset unset pruned pruned "--prune --prune-tags origin" "name" -test_configured_prune_type unset unset unset unset kept pruned "--prune --prune-tags origin" "link" -test_configured_prune_type unset unset true unset pruned pruned "--prune origin" "name" -test_configured_prune_type unset unset true unset kept pruned "--prune origin" "link" -test_configured_prune_type unset unset unset true pruned pruned "--prune origin" "name" -test_configured_prune_type unset unset unset true kept pruned "--prune origin" "link" -test_configured_prune_type true unset true unset pruned pruned "origin" "name" -test_configured_prune_type true unset true unset kept pruned "origin" "link" -test_configured_prune_type unset true true unset pruned pruned "origin" "name" -test_configured_prune_type unset true true unset kept pruned "origin" "link" -test_configured_prune_type unset true unset true pruned pruned "origin" "name" -test_configured_prune_type unset true unset true kept pruned "origin" "link" +test_configured_prune_type_branch unset unset unset unset - pruned "origin --prune --prune-tags" +test_configured_prune_type_branch unset unset unset unset - pruned "--prune --prune-tags origin" +test_configured_prune_type_branch unset unset true unset - pruned "--prune origin" +test_configured_prune_type_branch unset unset unset true - pruned "--prune origin" +test_configured_prune_type_branch true unset true unset - pruned "origin" +test_configured_prune_type_branch unset true true unset - pruned "origin" +test_configured_prune_type_branch unset true unset true - pruned "origin" # When all remote.origin.fetch settings are deleted a --prune # --prune-tags still implicitly supplies refs/tags/*:refs/tags/* so -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (3 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 04/10] fetch tests: add a helper to avoid boilerplate Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-22 6:01 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 06/10] fetch tests: pass a list, not a string of arguments Ævar Arnfjörð Bjarmason ` (4 subsequent siblings) 9 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Change the "$mode" parameter to be passed first, and setup a command-line parser we'll be able to use for getting rid of many of the boilerplate parameters. This will allow us to unquote the command-line argument, and process fetch arguments as a list of "$@". For now we need to do more work to unpack these, but in a subsequent commit we'll be able to make the shell quote handling here much simpler. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 5d118a6a806..477b6dd4953 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -822,14 +822,29 @@ set_config_tristate () { } test_configured_prune_type () { + mode= && + while test $# != 0 + do + case "$1" in + --mode) + mode="$2" && + shift + ;; + *) + break + ;; + esac && + shift + done && + local fetch_prune="$1" && local remote_origin_prune="$2" && local fetch_prune_tags="$3" && local remote_origin_prune_tags="$4" && local expected_branch="$5" && local expected_tag="$6" && - local cmdline="$7" && - local mode="$8" && + shift 6 && + local cmdline="$@" && if test -z "$cmdline_setup" then @@ -915,8 +930,8 @@ test_configured_prune_type () { } test_configured_prune () { - test_configured_prune_type "$@" "name" && - test_configured_prune_type "$@" "link" + test_configured_prune_type --mode name "$@" && + test_configured_prune_type --mode link "$@" } # $1 config: fetch.prune @@ -1007,11 +1022,19 @@ test_configured_prune unset unset unset true pruned kept \ # +refs/heads/*:refs/remotes/origin/* refspec and supplying it on the # command-line negates --prune-tags, the branches will not be pruned. test_configured_prune_type_branch () { - test_configured_prune_type "$1" "$2" "$3" "$4" pruned "$6" "$7" "name" - test_configured_prune_type "$1" "$2" "$3" "$4" kept "$6" "$7" "link" + local cfg_fp="$1" && + local cfg_rnp="$2" && + local cfg_fpt="$3" && + local cfg_rnpt="$4" && + local arg_branch="$5" && + local arg_tag="$6" && + shift 6 && + + test_configured_prune_type --mode name "$cfg_fp" "$cfg_rnp" "$cfg_fpt" "$cfg_rnpt" pruned "$arg_tag" "$@" + test_configured_prune_type --mode link "$cfg_fp" "$cfg_rnp" "$cfg_fpt" "$cfg_rnpt" kept "$arg_tag" "$@" } -test_configured_prune_type unset unset unset unset kept kept "origin --prune-tags" "name" -test_configured_prune_type unset unset unset unset kept kept "origin --prune-tags" "link" +test_configured_prune_type --mode name unset unset unset unset kept kept "origin --prune-tags" +test_configured_prune_type --mode link unset unset unset unset kept kept "origin --prune-tags" test_configured_prune_type_branch unset unset unset unset - pruned "origin --prune --prune-tags" test_configured_prune_type_branch unset unset unset unset - pruned "--prune --prune-tags origin" test_configured_prune_type_branch unset unset true unset - pruned "--prune origin" @@ -1029,8 +1052,8 @@ test_expect_success 'remove remote.origin.fetch "one"' ' git config --unset-all remote.origin.fetch ) ' -test_configured_prune_type unset unset unset unset kept pruned "origin --prune --prune-tags" "name" -test_configured_prune_type unset unset unset unset kept pruned "origin --prune --prune-tags" "link" +test_configured_prune_type --mode name unset unset unset unset kept pruned "origin --prune --prune-tags" +test_configured_prune_type --mode link unset unset unset unset kept pruned "origin --prune --prune-tags" test_expect_success 'all boundary commits are excluded' ' test_commit base && -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" 2022-06-21 22:34 ` [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" Ævar Arnfjörð Bjarmason @ 2022-06-22 6:01 ` Junio C Hamano 0 siblings, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2022-06-22 6:01 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > + local cmdline="$@" && Unless you are using the magic trait of "$@" that lets you pass each positional parameter without getting split at internal $IFS, it is less confusing to write "$*", which will make it obvious that a later use of $cmdline will lose the distinction between inter-parameter spaces and a whitespace that was embedded inside an individual parameter. > @@ -915,8 +930,8 @@ test_configured_prune_type () { > } > > test_configured_prune () { > - test_configured_prune_type "$@" "name" && > - test_configured_prune_type "$@" "link" > + test_configured_prune_type --mode name "$@" && > + test_configured_prune_type --mode link "$@" > } > > # $1 config: fetch.prune > @@ -1007,11 +1022,19 @@ test_configured_prune unset unset unset true pruned kept \ > # +refs/heads/*:refs/remotes/origin/* refspec and supplying it on the > # command-line negates --prune-tags, the branches will not be pruned. > test_configured_prune_type_branch () { > - test_configured_prune_type "$1" "$2" "$3" "$4" pruned "$6" "$7" "name" > - test_configured_prune_type "$1" "$2" "$3" "$4" kept "$6" "$7" "link" > + local cfg_fp="$1" && > + local cfg_rnp="$2" && > + local cfg_fpt="$3" && > + local cfg_rnpt="$4" && > + local arg_branch="$5" && > + local arg_tag="$6" && > + shift 6 && Unless we plan to never allow new parameters to be added (or existing one retired) from this helper, it would probably be easier to maintain if you wrote local cfg_fp="$1" && shift && local cfg_rnp="$1" && shift && ... ^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 06/10] fetch tests: pass a list, not a string of arguments 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (4 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 07/10] fetch tests: remove lazy variable setup Ævar Arnfjörð Bjarmason ` (3 subsequent siblings) 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Follow-up on the preceding commit where we've started to intercept these arguments as "$@" and pass them as a list, not as a string, before we'd only have a "$@" equivalent to "$1". We're still not doing anything really useful with these, but this is getting us towards fixing the quote handling in "test_configured_prune_type()". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 116 +++++++++++++++++++++++------------------------ 1 file changed, 58 insertions(+), 58 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 477b6dd4953..c56a00f1a17 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -940,82 +940,82 @@ test_configured_prune () { # $4 config: remote.<name>.pruneTags # $5 expect: branch to be pruned? # $6 expect: tag to be pruned? -# $7 git-fetch $cmdline: +# $7... git-fetch $cmdline: # -# $1 $2 $3 $4 $5 $6 $7 -test_configured_prune unset unset unset unset kept kept "" -test_configured_prune unset unset unset unset kept kept "--no-prune" -test_configured_prune unset unset unset unset pruned kept "--prune" +# $1 $2 $3 $4 $5 $6 $7... +test_configured_prune unset unset unset unset kept kept +test_configured_prune unset unset unset unset kept kept --no-prune +test_configured_prune unset unset unset unset pruned kept --prune test_configured_prune unset unset unset unset kept pruned \ - "--prune origin refs/tags/*:refs/tags/*" + --prune origin "refs/tags/*:refs/tags/*" test_configured_prune unset unset unset unset pruned pruned \ - "--prune origin refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" + --prune origin "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" -test_configured_prune false unset unset unset kept kept "" -test_configured_prune false unset unset unset kept kept "--no-prune" -test_configured_prune false unset unset unset pruned kept "--prune" +test_configured_prune false unset unset unset kept kept +test_configured_prune false unset unset unset kept kept --no-prune +test_configured_prune false unset unset unset pruned kept --prune -test_configured_prune true unset unset unset pruned kept "" -test_configured_prune true unset unset unset pruned kept "--prune" -test_configured_prune true unset unset unset kept kept "--no-prune" +test_configured_prune true unset unset unset pruned kept +test_configured_prune true unset unset unset pruned kept --prune +test_configured_prune true unset unset unset kept kept --no-prune -test_configured_prune unset false unset unset kept kept "" -test_configured_prune unset false unset unset kept kept "--no-prune" -test_configured_prune unset false unset unset pruned kept "--prune" +test_configured_prune unset false unset unset kept kept +test_configured_prune unset false unset unset kept kept --no-prune +test_configured_prune unset false unset unset pruned kept --prune -test_configured_prune false false unset unset kept kept "" -test_configured_prune false false unset unset kept kept "--no-prune" -test_configured_prune false false unset unset pruned kept "--prune" +test_configured_prune false false unset unset kept kept +test_configured_prune false false unset unset kept kept --no-prune +test_configured_prune false false unset unset pruned kept --prune test_configured_prune false false unset unset kept pruned \ - "--prune origin refs/tags/*:refs/tags/*" + --prune origin "refs/tags/*:refs/tags/*" test_configured_prune false false unset unset pruned pruned \ - "--prune origin refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" + --prune origin "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" -test_configured_prune true false unset unset kept kept "" -test_configured_prune true false unset unset pruned kept "--prune" -test_configured_prune true false unset unset kept kept "--no-prune" +test_configured_prune true false unset unset kept kept +test_configured_prune true false unset unset pruned kept --prune +test_configured_prune true false unset unset kept kept --no-prune -test_configured_prune unset true unset unset pruned kept "" -test_configured_prune unset true unset unset kept kept "--no-prune" -test_configured_prune unset true unset unset pruned kept "--prune" +test_configured_prune unset true unset unset pruned kept +test_configured_prune unset true unset unset kept kept --no-prune +test_configured_prune unset true unset unset pruned kept --prune -test_configured_prune false true unset unset pruned kept "" -test_configured_prune false true unset unset kept kept "--no-prune" -test_configured_prune false true unset unset pruned kept "--prune" +test_configured_prune false true unset unset pruned kept +test_configured_prune false true unset unset kept kept --no-prune +test_configured_prune false true unset unset pruned kept --prune -test_configured_prune true true unset unset pruned kept "" -test_configured_prune true true unset unset pruned kept "--prune" -test_configured_prune true true unset unset kept kept "--no-prune" +test_configured_prune true true unset unset pruned kept +test_configured_prune true true unset unset pruned kept --prune +test_configured_prune true true unset unset kept kept --no-prune test_configured_prune true true unset unset kept pruned \ - "--prune origin refs/tags/*:refs/tags/*" + --prune origin "refs/tags/*:refs/tags/*" test_configured_prune true true unset unset pruned pruned \ - "--prune origin refs/tags/*:refs/tags/* +refs/heads/*:refs/remotes/origin/*" + --prune origin "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" # --prune-tags on its own does nothing, needs --prune as well, same # for fetch.pruneTags without fetch.prune -test_configured_prune unset unset unset unset kept kept "--prune-tags" -test_configured_prune unset unset true unset kept kept "" -test_configured_prune unset unset unset true kept kept "" +test_configured_prune unset unset unset unset kept kept --prune-tags +test_configured_prune unset unset true unset kept kept +test_configured_prune unset unset unset true kept kept # These will prune the tags -test_configured_prune unset unset unset unset pruned pruned "--prune --prune-tags" -test_configured_prune true unset true unset pruned pruned "" -test_configured_prune unset true unset true pruned pruned "" +test_configured_prune unset unset unset unset pruned pruned --prune --prune-tags +test_configured_prune true unset true unset pruned pruned +test_configured_prune unset true unset true pruned pruned # remote.<name>.pruneTags overrides fetch.pruneTags, just like # remote.<name>.prune overrides fetch.prune if set. -test_configured_prune true unset true unset pruned pruned "" -test_configured_prune false true false true pruned pruned "" -test_configured_prune true false true false kept kept "" +test_configured_prune true unset true unset pruned pruned +test_configured_prune false true false true pruned pruned +test_configured_prune true false true false kept kept # When --prune-tags is supplied it's ignored if an explicit refspec is # given, same for the configuration options. test_configured_prune unset unset unset unset pruned kept \ - "--prune --prune-tags origin +refs/heads/*:refs/remotes/origin/*" + --prune --prune-tags origin "+refs/heads/*:refs/remotes/origin/*" test_configured_prune unset unset true unset pruned kept \ - "--prune origin +refs/heads/*:refs/remotes/origin/*" + --prune origin "+refs/heads/*:refs/remotes/origin/*" test_configured_prune unset unset unset true pruned kept \ - "--prune origin +refs/heads/*:refs/remotes/origin/*" + --prune origin "+refs/heads/*:refs/remotes/origin/*" # Pruning that also takes place if a file:// url replaces a named # remote. However, because there's no implicit @@ -1033,15 +1033,15 @@ test_configured_prune_type_branch () { test_configured_prune_type --mode name "$cfg_fp" "$cfg_rnp" "$cfg_fpt" "$cfg_rnpt" pruned "$arg_tag" "$@" test_configured_prune_type --mode link "$cfg_fp" "$cfg_rnp" "$cfg_fpt" "$cfg_rnpt" kept "$arg_tag" "$@" } -test_configured_prune_type --mode name unset unset unset unset kept kept "origin --prune-tags" -test_configured_prune_type --mode link unset unset unset unset kept kept "origin --prune-tags" -test_configured_prune_type_branch unset unset unset unset - pruned "origin --prune --prune-tags" -test_configured_prune_type_branch unset unset unset unset - pruned "--prune --prune-tags origin" -test_configured_prune_type_branch unset unset true unset - pruned "--prune origin" -test_configured_prune_type_branch unset unset unset true - pruned "--prune origin" -test_configured_prune_type_branch true unset true unset - pruned "origin" -test_configured_prune_type_branch unset true true unset - pruned "origin" -test_configured_prune_type_branch unset true unset true - pruned "origin" +test_configured_prune_type --mode name unset unset unset unset kept kept origin --prune-tags +test_configured_prune_type --mode link unset unset unset unset kept kept origin --prune-tags +test_configured_prune_type_branch unset unset unset unset - pruned origin --prune --prune-tags +test_configured_prune_type_branch unset unset unset unset - pruned --prune --prune-tags origin +test_configured_prune_type_branch unset unset true unset - pruned --prune origin +test_configured_prune_type_branch unset unset unset true - pruned --prune origin +test_configured_prune_type_branch true unset true unset - pruned origin +test_configured_prune_type_branch unset true true unset - pruned origin +test_configured_prune_type_branch unset true unset true - pruned origin # When all remote.origin.fetch settings are deleted a --prune # --prune-tags still implicitly supplies refs/tags/*:refs/tags/* so @@ -1052,8 +1052,8 @@ test_expect_success 'remove remote.origin.fetch "one"' ' git config --unset-all remote.origin.fetch ) ' -test_configured_prune_type --mode name unset unset unset unset kept pruned "origin --prune --prune-tags" -test_configured_prune_type --mode link unset unset unset unset kept pruned "origin --prune --prune-tags" +test_configured_prune_type --mode name unset unset unset unset kept pruned origin --prune --prune-tags +test_configured_prune_type --mode link unset unset unset unset kept pruned origin --prune --prune-tags test_expect_success 'all boundary commits are excluded' ' test_commit base && -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 07/10] fetch tests: remove lazy variable setup 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (5 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 06/10] fetch tests: pass a list, not a string of arguments Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 08/10] fetch tests: remove shelling out for previously "lazy" variables Ævar Arnfjörð Bjarmason ` (2 subsequent siblings) 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Remove the lazy test setup added in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09) to make it clear that these variables aren't changing across runs. We can also do away with the shell invocations here, but let's do that in a subsequent commit, for now this shows that it's safe to do this more than once. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index c56a00f1a17..7cfef0082c0 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -846,14 +846,9 @@ test_configured_prune_type () { shift 6 && local cmdline="$@" && - if test -z "$cmdline_setup" - then - test_expect_success 'setup cmdline_setup variable for subsequent test' ' - remote_url="file://$(git -C one config remote.origin.url)" && - remote_fetch="$(git -C one config remote.origin.fetch)" && - cmdline_setup="\"$remote_url\" \"$remote_fetch\"" - ' - fi && + remote_url="file://$(git -C one config remote.origin.url)" && + remote_fetch="$(git -C one config remote.origin.fetch)" && + cmdline_setup="\"$remote_url\" \"$remote_fetch\"" && if test "$mode" = 'link' then -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 08/10] fetch tests: remove shelling out for previously "lazy" variables 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (6 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 07/10] fetch tests: remove lazy variable setup Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 09/10] fetch tests: stop implicitly adding refspecs Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 10/10] fetch tests: fix needless and buggy re-quoting Ævar Arnfjörð Bjarmason 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Amend the variable assignments added in e1790f9245f (fetch tests: fetch <url> <spec> as well as fetch [<remote>], 2018-02-09). As of the last commit this test was slower as we'd do this on every run, but it was easier to see what was going on. But let's instead set this on the basis of $TRASH_DIRECTORY, which is a lot more obvious than the roundabout way of getting the configuration from the repo that we created earlier with: git clone . one Subsequent commits will make use of the new "$refspec_heads" for other purposes, so let's declare it as a variable, and use it instead of "$remote_fetch". Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 7cfef0082c0..54c7c86e5ca 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -837,6 +837,8 @@ test_configured_prune_type () { shift done && + local refspec_heads='+refs/heads/*:refs/remotes/origin/*' && + local fetch_prune="$1" && local remote_origin_prune="$2" && local fetch_prune_tags="$3" && @@ -846,9 +848,8 @@ test_configured_prune_type () { shift 6 && local cmdline="$@" && - remote_url="file://$(git -C one config remote.origin.url)" && - remote_fetch="$(git -C one config remote.origin.fetch)" && - cmdline_setup="\"$remote_url\" \"$remote_fetch\"" && + local remote_url="file://$TRASH_DIRECTORY/." && + local cmdline_setup="\"$remote_url\" \"$refspec_heads\"" if test "$mode" = 'link' then -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 09/10] fetch tests: stop implicitly adding refspecs 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (7 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 08/10] fetch tests: remove shelling out for previously "lazy" variables Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 10/10] fetch tests: fix needless and buggy re-quoting Ævar Arnfjörð Bjarmason 9 siblings, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason Remove the implicit behavior added in 97716d217c1 (fetch: add a --prune-tags option and fetch.pruneTags config, 2018-02-09) of providing the heads/tags refspecs and the "$remote_url" for tests that needed the "--mode link". In that case we need the URI on the command-line, and would then add these refspecs. Note that this was added in other cases, but these are the only ones where it mattered, i.e. we redundantly modified the command-line before. Yes, the "\"$remote_url\"" quoting here is buggy, but so is the pre-image in the same way, but removing this edge case will make it easier to eventually deal with that. So let's make that variable non-local for now and pass it in. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- t/t5510-fetch.sh | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh index 54c7c86e5ca..73964bebced 100755 --- a/t/t5510-fetch.sh +++ b/t/t5510-fetch.sh @@ -848,7 +848,7 @@ test_configured_prune_type () { shift 6 && local cmdline="$@" && - local remote_url="file://$TRASH_DIRECTORY/." && + remote_url="file://$TRASH_DIRECTORY/." && # NOT local yet! local cmdline_setup="\"$remote_url\" \"$refspec_heads\"" if test "$mode" = 'link' @@ -862,15 +862,6 @@ test_configured_prune_type () { new_cmdline=$(printf "%s" "$cmdline" | perl -pe 's[origin(?!/)]["'"$remote_url"'"]g') fi && - if test "$fetch_prune_tags" = 'true' || - test "$remote_origin_prune_tags" = 'true' - then - if ! printf '%s' "$cmdline\n" | grep -q refs/remotes/origin/ - then - new_cmdline="$new_cmdline refs/tags/*:refs/tags/*" - fi - fi && - cmdline="$new_cmdline" fi && @@ -990,19 +981,36 @@ test_configured_prune true true unset unset pruned pruned \ # --prune-tags on its own does nothing, needs --prune as well, same # for fetch.pruneTags without fetch.prune test_configured_prune unset unset unset unset kept kept --prune-tags -test_configured_prune unset unset true unset kept kept -test_configured_prune unset unset unset true kept kept +test_configured_prune_type --mode name unset unset true unset kept kept +test_configured_prune_type --mode link unset unset true unset kept kept \ + origin "refs/tags/*:refs/tags/*" +test_configured_prune_type --mode name unset unset unset true kept kept +test_configured_prune_type --mode link unset unset unset true kept kept \ + origin "refs/tags/*:refs/tags/*" # These will prune the tags test_configured_prune unset unset unset unset pruned pruned --prune --prune-tags -test_configured_prune true unset true unset pruned pruned -test_configured_prune unset true unset true pruned pruned + +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\"" \ + "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\"" \ + "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 true unset true unset pruned pruned -test_configured_prune false true false true pruned pruned -test_configured_prune true false true false kept kept +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\"" \ + "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\"" \ + "refs/tags/*:refs/tags/*" "+refs/heads/*:refs/remotes/origin/*" +test_configured_prune true false true false kept kept # When --prune-tags is supplied it's ignored if an explicit refspec is # given, same for the configuration options. -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* [PATCH 10/10] fetch tests: fix needless and buggy re-quoting 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason ` (8 preceding siblings ...) 2022-06-21 22:34 ` [PATCH 09/10] fetch tests: stop implicitly adding refspecs Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 ` Ævar Arnfjörð Bjarmason 2022-06-22 6:12 ` Junio C Hamano 9 siblings, 1 reply; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-21 22:34 UTC (permalink / raw) To: git Cc: Junio C Hamano, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King, Ævar Arnfjörð Bjarmason 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(-) 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 -- 2.36.1.1239.gfba91521d90 ^ permalink raw reply related [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting 2022-06-21 22:34 ` [PATCH 10/10] fetch tests: fix needless and buggy re-quoting Ævar Arnfjörð Bjarmason @ 2022-06-22 6:12 ` Junio C Hamano 2022-06-22 11:25 ` Derrick Stolee 0 siblings, 1 reply; 37+ messages in thread From: Junio C Hamano @ 2022-06-22 6:12 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: git, Derrick Stolee, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King Æ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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting 2022-06-22 6:12 ` Junio C Hamano @ 2022-06-22 11:25 ` Derrick Stolee 2022-06-22 15:21 ` Ævar Arnfjörð Bjarmason 2022-06-22 15:44 ` Junio C Hamano 0 siblings, 2 replies; 37+ messages in thread From: Derrick Stolee @ 2022-06-22 11:25 UTC (permalink / raw) To: Junio C Hamano, Ævar Arnfjörð Bjarmason Cc: git, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King 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 ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting 2022-06-22 11:25 ` Derrick Stolee @ 2022-06-22 15:21 ` Ævar Arnfjörð Bjarmason 2022-06-22 15:44 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-06-22 15:21 UTC (permalink / raw) To: Derrick Stolee Cc: Junio C Hamano, git, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting 2022-06-22 11:25 ` Derrick Stolee 2022-06-22 15:21 ` Ævar Arnfjörð Bjarmason @ 2022-06-22 15:44 ` Junio C Hamano 1 sibling, 0 replies; 37+ messages in thread From: Junio C Hamano @ 2022-06-22 15:44 UTC (permalink / raw) To: Derrick Stolee Cc: Ævar Arnfjörð Bjarmason, git, rsbecker, SZEDER Gábor, Johannes Sixt, Jeff King 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. ^ permalink raw reply [flat|nested] 37+ messages in thread
* RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 2022-06-20 18:04 Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker 2022-06-20 18:46 ` Derrick Stolee @ 2022-06-21 13:51 ` rsbecker 1 sibling, 0 replies; 37+ messages in thread From: rsbecker @ 2022-06-21 13:51 UTC (permalink / raw) To: rsbecker, 'Junio C Hamano'; +Cc: git On June 20, 2022 2:04 PM, I wrote: >To: 'Junio C Hamano' <gitster@pobox.com> >Cc: git@vger.kernel.org >Subject: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 > >Subtests t5510.69, 71, 97, 99, 125, 127, 147, 149, 151, 153, 155, 157, 159, 161, 163, >165, 167, 170 failed in rc1 on the NonStop ia64 platform. These were working in rc0, >but after analysis, I do not think that is relevant. Here is what we see for subtest >69: > >expecting success of 5510.69 'link prune fetch.prune=unset >remote.origin.prune=unset fetch.pruneTags=unset >remote.origin.pruneTags=unset --prune origin refs/tags/*:refs/tags/*; >branch:kept tag:pruned': >... > git fetch >"file:///home/jenkins/.jenkins/workspace/Git_Pipeline@2/t/trash >directory.t5510-fetch/." "+refs/heads/*:refs/remotes/origin/*" && ... > git$git_fetch_c fetch --prune >"file:///home/jenkins/.jenkins/workspace/Git_Pipeline/t/trash directory.t5510- >fetch/." refs/tags/*:refs/tags/* && > >What seems to be happening is that the test script is computing the wrong parent >directory in the second case, cutting the @2. We use Jenkins for our builds and >occasional multiple builds run. The @2 is Jenkins way of running in another place. >This seems to be confusing the script parsing of the parent, causing the issue. >When I rebuilt and test in the primary directory, the failures disappeared because >no @2. > >t5562 also seems to have the same issue. A more complete list of tests failing in path with @2 is - and this is likely caused by different constructs: t5300, t5510, t5516, t5562, t5601, t7528. ^ permalink raw reply [flat|nested] 37+ messages in thread
end of thread, other threads:[~2022-06-22 15:48 UTC | newest] Thread overview: 37+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-20 18:04 Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker 2022-06-20 18:46 ` Derrick Stolee 2022-06-20 18:59 ` rsbecker 2022-06-20 20:00 ` Derrick Stolee 2022-06-20 20:30 ` Ævar Arnfjörð Bjarmason 2022-06-20 20:43 ` Junio C Hamano 2022-06-20 21:24 ` rsbecker 2022-06-20 21:33 ` Ævar Arnfjörð Bjarmason 2022-06-20 20:34 ` SZEDER Gábor 2022-06-20 22:17 ` rsbecker 2022-06-20 22:20 ` [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1) Derrick Stolee 2022-06-21 5:28 ` Johannes Sixt 2022-06-21 7:17 ` Jeff King 2022-06-21 9:29 ` SZEDER Gábor 2022-06-21 10:07 ` Jeff King 2022-06-21 16:35 ` Junio C Hamano 2022-06-21 20:27 ` Junio C Hamano 2022-06-21 21:13 ` Derrick Stolee 2022-06-21 21:36 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 00/10] t5510: fix the quoting mess Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 01/10] fetch tests: remove redundant test_unconfig() Ævar Arnfjörð Bjarmason 2022-06-22 5:52 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 02/10] fetch tests: use named, not positional parameters Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 03/10] fetch tests: use "local", &&-chaining, style etc Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 04/10] fetch tests: add a helper to avoid boilerplate Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@" Ævar Arnfjörð Bjarmason 2022-06-22 6:01 ` Junio C Hamano 2022-06-21 22:34 ` [PATCH 06/10] fetch tests: pass a list, not a string of arguments Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 07/10] fetch tests: remove lazy variable setup Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 08/10] fetch tests: remove shelling out for previously "lazy" variables Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 09/10] fetch tests: stop implicitly adding refspecs Ævar Arnfjörð Bjarmason 2022-06-21 22:34 ` [PATCH 10/10] fetch tests: fix needless and buggy re-quoting Ævar Arnfjörð Bjarmason 2022-06-22 6:12 ` Junio C Hamano 2022-06-22 11:25 ` Derrick Stolee 2022-06-22 15:21 ` Ævar Arnfjörð Bjarmason 2022-06-22 15:44 ` Junio C Hamano 2022-06-21 13:51 ` Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1 rsbecker
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).