Re: [PATCH 1/2] worktree: send "chatty" messages to stderr

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 3, 2021, 13:07 UTC
Message-ID: <CAPig+cQ1+Hm=2jNKais-X6ahKCkG4Xo9Z4WsqTYryETc9a4mTw@mail.gmail.com>
In-Reply-To: <211203.86fsrat592.gmgdl@evledraar.gmail.com>

On Fri, Dec 3, 2021 at 4:19 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 27 quoted lines

> On Thu, Dec 02 2021, Eric Sunshine wrote:
> >       git worktree add --detach orig &&
> >       sed s,orig/\.git$,moved/.git, .git/worktrees/orig/gitdir >expect &&
> >       mv orig moved &&
> > -     git worktree repair moved >out 2>err &&
> > +     git worktree repair moved 2>err &&
> >       test_cmp expect .git/worktrees/orig/gitdir &&
> > -     test_i18ngrep "gitdir incorrect" out &&
> > -     test_must_be_empty err
> > +     test_i18ngrep "gitdir incorrect" err
> >  '
>
> This is just a "for bonus points", but maybe we could/should while we're
> at it harden and make the tests more exhaustive by checking the full
> output of both, e.g.
>
>         cat >actual.out <<-\EOF &&
>         Preparing worktree (checking out 'bar')
>         EOF
>         cat >actual.err <<-\EOF &&
>         fatal: 'bar' is already checked out at '.../wherever'
>         EOF
>         <cmd> [...]
>         test_cmp expect.out actual.out &&
>         test_cmp expect.err actual.err
>
> Doesn't need a re-roll etc., just if you're interested... :)

To be clear, with the application of the current patch, both of those messages would need to be in the `actual.err` file, and `actual.out` would be empty; not one message in each file as in your snippet.

That aside, there's still potentially output which is outside the control of git-worktree. In the case of this particular negative test, your suggestion should work, but for a positive test, it would be harder and uglier (though, of course, not impossible). For instance, for a successful `git worktree add`, the output is:

    Preparing worktree (new branch 'foobar')
    Updating files: 100% (3993/3993), done.
    HEAD is now at abe6bb3905 Gobbledygook

The subsequent lines come from git-reset (which, by the way, is sending "HEAD is now at" to stdout, though they probably should be on stderr, but that's a separate issue).

Anyhow, such a change to the tests should be a separate topic. The user-facing problem addressed by the current patch series need not be held up by a behind-the-scenes change to testing.