Re: Runaway sed memory use in test on older sed+glibc (was "Re: [PATCH v6 1/3] test: add helper functions for git-bundle")

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Jun 01 2021, Jiang Xin wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> 于2021年5月27日周四 下午8:49写道：
>>
>> But no, the issue as it turns out is not Perl v.s. Sed, it's that
>> there's some bug in the shellscript / tooling version (happens with both
>> dash 0.5.7-4 and bash 4.3-11+deb8u2 on that box) where those expansions
>> like ${A%${A#??????0?}} resolve to nothing.
>
> That's the root cause.  It can be reproduced by running the following
> test script:
>
> ```
> #!/bin/sh
> # test script: test.sh
>
> test_commit_setvar () {
>         var=$1 &&
>         oid=1234567890123456789012345678901234567890 &&
>         eval $var=$oid
> }
>
> test_commit_setvar A
> echo "A: $A"
> echo "Abbrev of A: ${A%${A#???????}}"
> ```
>
> By running different version of dash, we can see that dash 0.5.7 fail the test:
>
> ```
> $ /opt/dash/0.5.11/bin/dash test.sh
> A: 1234567890123456789012345678901234567890
> Abbrev of A: 1234567
>
> $ /opt/dash/0.5.7/bin/dash test.sh
> A: 1234567890123456789012345678901234567890
> Abbrev of A:
> ```
>
> This issue can be fixed using the following example:
>
> ```
> #!/bin/sh
>
> test_commit_setvar () {
>         var=$1 &&
>         oid=1234567890123456789012345678901234567890 &&
>         suffix=${oid#???????} &&
>         oid=${oid%$suffix} &&
>         eval $var=$oid
> }
>
> test_commit_setvar A
> echo "Abbrev of A: $A"
> ```

*nod*

>> Anyway, looking at this whole test file with fresh eyes this pattern
>> seems very strange. You duplicated most of test_commit with this
>> test_commit_setvar. It's a bit more verbosity but why not just use:
>>
>>     test_commit ...
>>     A=$(git rev-parse HEAD)
>
> The function "test_commit()" in "test-lib-function.sh" always creates
> tags and it cannot make merge commit. So I rewrite a new function
> which reuse the scaffold of "test_commit".

It's had a --no-tag since 3803a3a099 (t: add --no-tag option to
test_commit, 2021-02-09). I also have patches in "next" to add more
options, you can just add more, having a --merge and maybe a way to tell
it to eval the rev-parse into a given variable seem like sensible
additions.

> BTW, sorry for the late reply, will send patch later.

My main point was that looking at this I think it's very much over the
complexity v.s. benefit line on the "complexity" side.

Even if there wasn't a --no-tag just using "test_commit" with a "git tag
-d" and "commit_X=$(git rev-parse HEAD)" is less magical and more
readable.

I.e. the mostly copy/pasted from test-lib-functions.sh function is ~70
lines, the whole setup function is 50 lines.

And as I noted with the whitespace getting lost in the munging the end
result is actually less reliable than just doing a test_cmp with $(git
rev-parse ...) instead of <COMMIT-XYZ>.

If you were trying to avoid the whitespace warnings then see the
"'s/Z$//'" pattern in t0000-basic.sh for how we've usually tested that,
i.e. had a "Z" at the end mark intentional whitespace for
test_cmp-alike.

There's a big value in the test suite being mostly consistent (which it
somewhat isn't, but we're hopefully getting there). I.e. the goal isn't
to optimize each test file to be as small as possible, but to e.g. have
the next person maintaining it not wondering where <COMMIT-P> comes
from, understanding some test_commit-alike that eval's variables into
existence, how it's subtly different (if at all) from test_commit etc.