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

From: Derrick Stolee <derrickstolee@github.com>
Date: Jun 22, 2022, 11:25 UTC
Message-ID: <69256646-13b0-5619-3161-8d8e319fad50@github.com>
In-Reply-To: <xmqqsfnx6xbh.fsf@gitster.g>

On 6/22/22 2:12 AM, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 15 quoted lines

> 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