Re: [PATCH v4] add-patch: response to unknown command

From

Rubén Justo <rjusto@gmail.com>

Date

Apr 25, 2024, 22:09 UTC

Message-ID

<5854e33c-b84e-47a3-94c6-c5fe8c9a4c98@gmail.com>

In-Reply-To

<xmqqv845dug2.fsf@gitster.g>

On Thu, Apr 25, 2024 at 02:05:33PM -0700, Junio C Hamano wrote:

I am assuming that this change will precede my series.

I leave some nits below.

> diff --git c/add-patch.c w/add-patch.c
> index 7be142d448..c28ad380ed 100644
> --- c/add-patch.c
> +++ w/add-patch.c
> @@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)
>  	va_list args;
>  
>  	va_start(args, fmt);
> -	fputs(s->s.error_color, stderr);
> -	vfprintf(stderr, fmt, args);
> -	fputs(s->s.reset_color, stderr);
> -	fputc('\n', stderr);
> +	fputs(s->s.error_color, stdout);
> +	vfprintf(stdout, fmt, args);
> +	fputs(s->s.reset_color, stdout);
> +	fputc('\n', stdout);

If we're going to use stdout, perhaps we can be less explicit?

diff --git a/add-patch.c b/add-patch.c
index 447e8166d2..a204224dae 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -293,10 +293,10 @@ static void err(struct add_p_state *s, const char *fmt, ...)
        va_list args;

        va_start(args, fmt);
-       fputs(s->s.error_color, stderr);
-       vfprintf(stderr, fmt, args);
-       fputs(s->s.reset_color, stderr);
-       fputc('\n', stderr);
+       puts(s->s.error_color);
+       vprintf(fmt, args);
+       puts(s->s.reset_color);
+       putchar('\n');
        va_end(args);
 }

> @@ -1780,9 +1780,9 @@ int run_add_p(struct repository *r, enum add_p_mode mode,
>  			break;
>  
>  	if (s.file_diff_nr == 0)
> -		fprintf(stderr, _("No changes.\n"));
> +		err(&s, _("No changes."));

Nice.

>  	else if (binary_count == s.file_diff_nr)
> -		fprintf(stderr, _("Only binary files changed.\n"));
> +		err(&s, _("Only binary files changed."));

Nice.

> diff --git c/t/t3701-add-interactive.sh w/t/t3701-add-interactive.sh
> index 482d5c117e..a315ec99a3 100755
> --- c/t/t3701-add-interactive.sh
> +++ w/t/t3701-add-interactive.sh
> @@ -43,17 +43,17 @@ force_color () {
>  }
>  
>  test_expect_success 'warn about add.interactive.useBuiltin' '
> -	cat >expect <<-\EOF &&
> +	cat >expect.error <<-\EOF &&

This expect.error is what we are going to test with the output on
stderr ... 

>  	warning: the add.interactive.useBuiltin setting has been removed!
>  	See its entry in '\''git help config'\'' for details.
> -	No changes.

however, this line no longer goes to stderr.  OK.

>  	EOF
>  
>  	for v in = =true =false
>  	do
> -		git -c "add.interactive.useBuiltin$v" add -p >out 2>actual &&
> -		test_must_be_empty out &&
> -		test_cmp expect actual || return 1
> +		git -c "add.interactive.useBuiltin$v" add -p >actual 2>error &&
> +		echo "No changes." >expect &&

Why not prepare this file above, out of the loop?

> +		test_cmp expect actual &&
> +		test_cmp expect.error error || return 1
>  	done
>  '
>  
> @@ -348,13 +348,13 @@ test_expect_success 'different prompts for mode change/deleted' '
>  
>  test_expect_success 'correct message when there is nothing to do' '
>  	git reset --hard &&
> -	git add -p 2>err &&
> -	test_grep "No changes" err &&
> +	git add -p >out &&
> +	test_grep "No changes" out &&
>  	printf "\\0123" >binary &&
>  	git add binary &&
>  	printf "\\0abc" >binary &&
> -	git add -p 2>err &&
> -	test_grep "Only binary files changed" err
> +	git add -p >out &&
> +	test_grep "Only binary files changed" out
>  '
>  
>  test_expect_success 'setup again' '