Re: Git branch outputs usage message on stderr

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2025, 23:32 UTC
Message-ID: <xmqqplknvek2.fsf@gitster.g>
In-Reply-To: <20250115222728.GA132248@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> I don't know if we'd want something like this on top. If somebody is
> interested in just doing all the conversions in the near-term, we could
> do without the optional flag.

Ah, you are much more practical than I am ;-) I was wondering if we want a list of "these commands have already been updated" and behave differently.

> Currently t0012 is permissive and allows either behavior. We'd like it
> to eventually enforce that help goes to stdout, and teaching it to do so
> identifies the commands that need to be changed. But during the
> transition period, we don't want to enforce that for most test runs.

Yeah, that is why the "git branch -h" thing has been left broken for such a long time.

Show 13 quoted lines

> So let's introduce a flag that will let most test runs use the
> permissive behavior, and people interested in converting commands can
> run:
>
>   GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh
>
> to see the failures. Eventually (when all builtins have been converted)
> we'll remove this flag entirely and always check the strict behavior.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t0012-help.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Nice.

It is a tangent, but I wonder how many among the 40 really needed to use usage_with_options() to react to "-h" in the first place. In other words, these manual checks for "-h" are done only because the code _wants_ to react to "-h" before it calls parse_options(), but does everybody who _wants_ to do so really _needs_ to do so? You already have shown that "gir branch" did not have to, and to me, 40 among 100+ felt way too many.

Show 15 quoted lines

> diff --git a/t/t0012-help.sh b/t/t0012-help.sh
> index 1d273d91c2..9c7ae9fd36 100755
> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -255,9 +255,16 @@ do
>  		(
>  			GIT_CEILING_DIRECTORIES=$(pwd) &&
>  			export GIT_CEILING_DIRECTORIES &&
> -			test_expect_code 129 git -C sub $builtin -h >output 2>&1
> +			test_expect_code 129 git -C sub $builtin -h >output 2>err
>  		) &&
> -		test_grep usage output
> +		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
> +		then
> +			test_must_be_empty err &&

This may be a bit stricter than needed (things other than usage may need to be spitted out), but it is sufficent to declare that we will deal with any potential fallout only after it becomes necessary ;-).

Show 7 quoted lines

> +			test_grep usage output
> +		else
> +			test_grep usage output ||
> +			test_grep usage err
> +		fi
>  	'
>  done <builtins

Will queue.  Thanks.