On 23/04/17 02:09PM, Junio C Hamano wrote:
> Jacob Abel <jacobabel@xxxxxxxxxx> 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
