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

From

Junio C Hamano <gitster@pobox.com>

Date

Apr 25, 2024, 21:05 UTC

Message-ID

<xmqqv845dug2.fsf@gitster.g>

In-Reply-To

<xmqqedatfay3.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> That's very reminiscent of [1]. Although, unlike [1], the output
>> presented to the user in this case is (I suppose) less likely to be
>> messed up; only the combined captured output is probably affected. So,
>> capturing stdout and stderr separately would indeed be a good idea.
>
> Hmph, something along this line?
>
> It loses to capture how the output should be intermixed, which is
> essential to validate what the end-user should see.  As we can see
> in the attached patch, we cannot express that "Unknown ..." should
> come in between two "Stage addition?" questions, which is a downside.
>
> Between adding fflush() before err() writes, and updating err() to
> write to the standard output stream, I am in favor of the latter for
> its simplicity (of the mental model of the resulting code, not of
> the patch that is required to do so).

The latter, which I claimed to prefer, would look like this.

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);
 	va_end(args);
 }
 
@@ -1326,7 +1326,7 @@ static int apply_for_checkout(struct add_p_state *s, struct strbuf *diff,
 		err(s, _("Nothing was applied.\n"));
 	} else
 		/* As a last resort, show the diff to the user */
-		fwrite(diff->buf, diff->len, 1, stderr);
+		fwrite(diff->buf, diff->len, 1, stdout);
 
 	return 0;
 }
@@ -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."));
 	else if (binary_count == s.file_diff_nr)
-		fprintf(stderr, _("Only binary files changed.\n"));
+		err(&s, _("Only binary files changed."));
 
 	add_p_state_clear(&s);
 	return 0;
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 &&
 	warning: the add.interactive.useBuiltin setting has been removed!
 	See its entry in '\''git help config'\'' for details.
-	No changes.
 	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 &&
+		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' '