Re: [PATCH 10/10] fetch tests: fix needless and buggy re-quoting
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jun 22, 2022, 15:21 UTC
- Message-ID
- <220622.86iloslnlt.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <69256646-13b0-5619-3161-8d8e319fad50@github.com>
On Wed, Jun 22 2022, Derrick Stolee wrote:
Show 24 quoted lines
> 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.