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 [off-list ref] 于2021年6月1日周二 下午8:04写道：

On Tue, Jun 01 2021, Jiang Xin wrote:

quoted

Ævar Arnfjörð Bjarmason [off-list ref] 于2021年5月27日周四 下午8:49写道：

quoted

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*

quoted

quoted

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.

quoted

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.

Will send a patch for quick fix for t6020 which is broken on older
version of bash.

After changes on "test_commit()" of "test-lib-function.sh" has been
merge to master branch, I will try to refactor t6020 again to remove
`test_commit_setvar()` and reuse `test_commit()`.