Re: [PATCH] bisect run: allow '--' as an options terminator

From: Stephen Oberholtzer <stevie@qrpff.net>
Date: Jan 6, 2020, 23:29 UTC
Message-ID: <CAD_xR9faremhsGP4G65vHj=cvskUbK1rpPrQisa2_4AD2AGNYw@mail.gmail.com>
In-Reply-To: <xmqq1rsfddkq.fsf@gitster-ct.c.googlers.com>

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@pobox.com> 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.

Show 9 quoted lines

> 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.

Show 6 quoted lines

> > +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.

Show 7 quoted lines

>
> > +. ./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