Re: [PATCH v3 3/3] test-lib: make BAIL_OUT() work in tests and prereq

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2021, 17:05 UTC
Message-ID: <xmqqh7c4i0jh.fsf@gitster.g>
In-Reply-To: <211122.86y25gz9q7.gmgdl@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Show 16 quoted lines

>> -exec 5>&1
>> -exec 6<&0
>> -exec 7>&2
>
> This doesn't break (I think) with your change here because you only
> manipulate >&5, but I think the post-image would be lot clearer if...
>
>>  if test "$verbose_log" = "t"
>>  then
>>  	exec 3>>"$GIT_TEST_TEE_OUTPUT_FILE" 4>&3
>
> ...this bit of code were moved up along with the "exec". They're
> currently intentionally snuggled together as we conditionally set the
> 3rd and 4th fd depending on verbose/tee settings right after the setup
> of 5/6/7, keeping them grouped in the post-image makes more sense than
> splitting them up here.

I actually have to wonder if the handling of 3 and 4 should be moved down, not up like you suggest, so that they are grouped together with the maybe_teardown_verbose and the maybe_setup_verbose helper functions. The lines we see here are the file descriptors that are always redirected, which is a bit different. Raising all of them up to group them together is also fine.

In any case, the comment in front of the block of exec wants to become a bit more detailed than just "# Set up additional fds", with an explanation about which FD is used for what.

And any change that involves handling of FD #4 probably wants to further include the BASH_XTRACEFD setting.

Thanks.