Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[rsbecker]

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.

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[Derrick Stolee]

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

RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[rsbecker]

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?

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[Derrick Stolee]

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


 

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[Ævar Arnfjörð Bjarmason]

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
"|".

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[Junio C Hamano]

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
> "|".

RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[rsbecker]

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 "|".

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[Ævar Arnfjörð Bjarmason]

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

Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[SZEDER Gábor]

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

RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[rsbecker]

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

[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]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Johannes Sixt]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Jeff King]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[SZEDER Gábor]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Jeff King]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Junio C Hamano]

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.

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Junio C Hamano]

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

Re: [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]

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

Re: [PATCH v2] t5510: replace 'origin' with URL more carefully (was Re: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1)

[Junio C Hamano]

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

[PATCH 00/10] t5510: fix the quoting mess

[Æ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

[PATCH 01/10] fetch tests: remove redundant test_unconfig()

[Æ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

Re: [PATCH 01/10] fetch tests: remove redundant test_unconfig()

[Junio C Hamano]

Æ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

[PATCH 02/10] fetch tests: use named, not positional parameters

[Æ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

[PATCH 03/10] fetch tests: use "local", &&-chaining, style etc.

[Æ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

[PATCH 04/10] fetch tests: add a helper to avoid boilerplate

[Æ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

[PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@"

[Æ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

Re: [PATCH 05/10] fetch tests: pass "mode" parameter first, pave way for "$@"

[Junio C Hamano]

Æ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 &&
	...

[PATCH 06/10] fetch tests: pass a list, not a string of arguments

[Æ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

[PATCH 07/10] fetch tests: remove lazy variable setup

[Æ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

[PATCH 08/10] fetch tests: remove shelling out for previously "lazy" variables

[Æ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

[PATCH 09/10] fetch tests: stop implicitly adding refspecs

[Æ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

[PATCH 10/10] fetch tests: fix needless and buggy re-quoting

[Æ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

Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting

[Junio C Hamano]

Æ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.

Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting

[Derrick Stolee]

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

Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting

[Ævar Arnfjörð Bjarmason]

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.

Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting

[Junio C Hamano]

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.

RE: Test Failure t5510,t5562 - was RE: [ANNOUNCE] Git v2.37.0-rc1

[rsbecker]

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.