From: Jacob Abel <jacobabel@nullpo.dev>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Phillip Wood" <phillip.wood123@gmail.com>,
"Rubén Justo" <rjusto@gmail.com>, "Taylor Blau" <me@ttaylorr.com>,
rsbecker@nexbridge.com
Subject: Re: [PATCH v9 2/8] t2400: print captured git output when finished
Date: Tue, 18 Apr 2023 03:53:55 +0000 [thread overview]
Message-ID: <olztmib77r35mx33a655obqpxui6coj74hfxoxfvcudnkpbqns@ixerneqaai45> (raw)
In-Reply-To: <xmqq8reqkyfz.fsf@gitster.g>
On 23/04/17 02:09PM, Junio C Hamano wrote:
> Jacob Abel <jacobabel@nullpo.dev> writes:
>
> > test_expect_success 'add --quiet' '
> > + test_when_finished "git worktree remove -f -f another-worktree" &&
> > + test_when_finished cat actual >&2 &&
>
> I doubt that this redirection does anything you expect it do.
> Doesn't it redirect the standard output that is emitted by the
> test_when_finished shell function when it registers another
> test_cleanup scriptlet to the standard error, and when test_cleanup
> is indeed run, wouldn't "cat actual" send its output to the standard
> output?
Yes that's correct. I figured "grab from stderr, cat to stderr" but yes
this isn't necessarily what we want here. Dropping the `>&2` causes it to
work as expected.
>
> No, I am not suggesting to write the line as:
>
> test_when_finished "cat >&2 actual" &&
>
> > git worktree add --quiet another-worktree main 2>actual &&
> > test_must_be_empty actual
>
> The reason why I do not suggest "fixing" the above is because
> test_must_be_empty, when fails, does this:
>
> test_must_be_empty () {
> test "$#" -ne 1 && BUG "1 param"
> test_path_is_file "$1" &&
> if test -s "$1"
> then
> echo "'$1' is not empty, it contains:"
> cat "$1"
> return 1
> fi
> }
>
> i.e. it sends the contents of "actual" to the standard output
> already. When it succeeds, of course "actual" is empty, and there
> is no point in showing its contents.
>
> So "sh t2400-*.sh -x -i" already shows "cat actual" output. Try
> the attached patch on top of this one and running it would show
> the above message shown by test_must_be_empty and the contents of
> the file 'actual'. "git worktree remove" fails and your "cat" in
> the test_cleanup does not even trigger, by the way.
That should not be the case. From what I've seen, the test cleanup is
executed in reverse order from the order they are declared with
`test_when_finished`. So as long as `cat` is the last command added to test
cleanup it should always execute immediately after the first command in the
script fails. And as long as the `cat` is added immediately before the
`git worktree add`, that means it should be the most recently added in the
event that command fails.
>
> There may be cases where having something like this might help, but
> running the test with "-x" is not it---that case is already covered
> by what test_must_be_empty gives us, I think.
>
> [...]
I attached an example below to try to illustrate the issue I was attempting
to solve. If `git worktree add ... 2>actual` fails, redirecting stderr to
actual eats the output that would normally show w/ `-x`. Then because a
command fails, it never reaches the `test_must_be_empty`.
Test results of running `sh t2400-*.sh -x` for this test when
`git worktree add` fails (caused in this case by adding `--bad-arg` to the
command):
expecting success of 2400.37 'add --quiet':
test_when_finished "git worktree remove -f -f another-worktree" &&
test_when_finished cat actual >&2 &&
git worktree add --quiet --bad-arg another-worktree main 2>actual &&
test_must_be_empty actual
++ test_when_finished 'git worktree remove -f -f another-worktree'
++ test 0 = 0
++ test_cleanup='{ git worktree remove -f -f another-worktree
} && (exit "$eval_ret"); eval_ret=$?; :'
++ test_when_finished cat actual
++ test 0 = 0
++ test_cleanup='{ cat actual
} && (exit "$eval_ret"); eval_ret=$?; { git worktree remove -f -f another-worktree
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git worktree add --quiet --bad-arg another-worktree main
error: last command exited with $?=129
++ cat actual
error: unknown option `bad-arg'
usage: git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]
[(-b | -B) <new-branch>] <path> [<commit-ish>]
-f, --force checkout <branch> even if already checked out in other worktree
-b <branch> create a new branch
-B <branch> create or reset a branch
-d, --detach detach HEAD at named commit
--checkout populate the new working tree
--lock keep the new working tree locked
--reason <string> reason for locking
-q, --quiet suppress progress reporting
--track set up tracking mode (see git-branch(1))
--guess-remote try to match the new branch name with a remote-tracking branch
++ exit 129
++ eval_ret=129
++ git worktree remove -f -f another-worktree
fatal: 'another-worktree' is not a working tree
++ eval_ret=128
++ :
not ok 37 - add --quiet
The same test but with the `test_when_finished cat actual` removed:
expecting success of 2400.37 'add --quiet':
test_when_finished "git worktree remove -f -f another-worktree" &&
git worktree add --quiet --bad-arg another-worktree main 2>actual &&
test_must_be_empty actual
++ test_when_finished 'git worktree remove -f -f another-worktree'
++ test 0 = 0
++ test_cleanup='{ git worktree remove -f -f another-worktree
} && (exit "$eval_ret"); eval_ret=$?; :'
++ git worktree add --quiet --bad-arg another-worktree main
error: last command exited with $?=129
++ git worktree remove -f -f another-worktree
fatal: 'another-worktree' is not a working tree
++ eval_ret=128
++ :
not ok 37 - add --quiet
next prev parent reply other threads:[~2023-04-18 3:54 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-17 9:33 [PATCH v9 0/8] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2023-04-17 9:33 ` [PATCH v9 1/8] worktree add: include -B in usage docs Jacob Abel
2023-04-17 9:33 ` [PATCH v9 2/8] t2400: print captured git output when finished Jacob Abel
2023-04-17 21:09 ` Junio C Hamano
2023-04-18 3:53 ` Jacob Abel [this message]
2023-04-18 16:34 ` Junio C Hamano
2023-04-19 13:23 ` Jacob Abel
2023-04-19 13:36 ` Jacob Abel
2023-04-19 15:41 ` Junio C Hamano
2023-04-19 16:50 ` Jacob Abel
2023-04-17 9:33 ` [PATCH v9 3/8] t2400: refactor "worktree add" opt exclusion tests Jacob Abel
2023-04-17 21:30 ` Junio C Hamano
2023-04-20 2:46 ` Jacob Abel
2023-04-17 9:34 ` [PATCH v9 4/8] t2400: add tests to verify --quiet Jacob Abel
2023-04-17 21:33 ` Junio C Hamano
2023-04-20 2:48 ` Jacob Abel
2023-04-17 9:34 ` [PATCH v9 5/8] worktree add: add --orphan flag Jacob Abel
2023-04-17 9:34 ` [PATCH v9 6/8] worktree add: introduce "try --orphan" hint Jacob Abel
2023-04-17 9:34 ` [PATCH v9 7/8] worktree add: extend DWIM to infer --orphan Jacob Abel
2023-04-17 9:34 ` [PATCH v9 8/8] worktree add: emit warn when there is a bad HEAD Jacob Abel
2023-04-20 3:05 ` [PATCH v9 0/8] worktree: Support `--orphan` when creating new worktrees Jacob Abel
2023-05-01 21:51 ` Junio C Hamano
2023-05-02 5:48 ` Jacob Abel
2023-05-17 21:47 ` [RESEND PATCH v10 " Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 1/8] worktree add: include -B in usage docs Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 2/8] t2400: cleanup created worktree in test Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 3/8] t2400: refactor "worktree add" opt exclusion tests Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 4/8] t2400: add tests to verify --quiet Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 5/8] worktree add: add --orphan flag Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 6/8] worktree add: introduce "try --orphan" hint Jacob Abel
2023-05-17 21:48 ` [RESEND PATCH v10 7/8] worktree add: extend DWIM to infer --orphan Jacob Abel
2023-08-09 6:47 ` RESEND [PATCH " Teng Long
2023-08-11 17:43 ` Jacob Abel
2023-05-17 21:49 ` [RESEND PATCH v10 8/8] worktree add: emit warn when there is a bad HEAD Jacob Abel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=olztmib77r35mx33a655obqpxui6coj74hfxoxfvcudnkpbqns@ixerneqaai45 \
--to=jacobabel@nullpo.dev \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
--cc=phillip.wood123@gmail.com \
--cc=rjusto@gmail.com \
--cc=rsbecker@nexbridge.com \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).