Junio,
Sorry about that sloppy test script; that'll teach me to try cleaning
up a patch late at night.
On Fri, Jan 3, 2020 at 11:56 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
> I do not think I have seen enough to justify addition of "--",
That's fine; I was just trying to be thorough (also, it was easy to
test.) I was taught: if you accept any -options, honor -- as well. If
you're not concerned about that, that's fine with me.
> but
> addition of tests by itself may have a value, and I'd prefer to see
> it as its own patch. But I see hits for
>
> git grep 'git bisect run' t/
>
> already in t6030, so "adds an actual test script", while it is not
> telling a lie, may be giving a false impression that it is adding
> something new that has been missing.
Mmh, I didn't see those 'bisect run' tests in there; clearly, I didn't
look hard enough. I was distracted by several factors, including the
filename (t/t6030-bisect-porcelain.sh: I associated "porcelain" with
"output intended to be parsed". Since run doesn't have any meaningful
parse-able output, I did not expect to find it there.) and the fact
that bisect apparently has all of the test cases in one single script,
when (for example) rev-list currently has 23 different test scripts.
> > +exec </dev/null
>
> All of our test scripts are designed to be used unattended with the
> use of test_expect_* harness helpers. Can you tell us why this new
> file alone needs to dissociate the input to _all_ its subproesses by
> doing this, while others do not have to?
Ironically, that line was copied straight out of the top of t6030.
>
> > +. ./test-lib.sh
> > +
> > +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
>
> Yuck. I have to say this is trying to be too clever and cute than
> its worth. Doesn't it defeat test-lint, for example?
Actually, it doesn't. test-lint is too devious :-o
I was trying to adapt the script that I initially wrote to set up a
repo for manual testing. A heredoc was the easiest way to achieve
that without having to worry too much about escaping.
<more advice on readability and portability snipped>
> Thanks.
No, thank you, for putting up with me :)
Anyway, it sounds like: (a) the feature my patch adds isn't wanted or
needed, and (b) the functionality I *thought* I was adding test
coverage for already has test coverage, so we can kill this particular
thread entirely.
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
