Re: [PATCH] add-patch: response to invalid option

From: phillip.wood123@gmail.com <phillip.wood123@gmail.com>
Date: Apr 16, 2024, 09:41 UTC
Message-ID: <15f9252c-212f-43eb-84f3-6046fb2fab38@gmail.com>
In-Reply-To: <4e2bc660-ee33-4641-aca5-783d0cefcd23@gmail.com>

Hi Rubén

Thanks for working on this, it is a nice follow up to your last series.

On 15/04/2024 20:00, Rubén Justo wrote:

>
> +		} else {
> +			err(s, _("Unknown option '%s' (use '?' for help)"),

As this is an interactive program I think "Unknown key" would be clearer.

Show 17 quoted lines

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index bc55255b0a..b38fd5388a 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -61,6 +61,16 @@ test_expect_success 'setup (initial)' '
>   	echo more >>file &&
>   	echo lines >>file
>   '
> +
> +test_expect_success 'invalid option' '
> +	cat >expect <<-EOF &&
> +	Unknown option ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
> +	EOF
> +	test_write_lines W |
> +	git -c core.filemode=true add -p 2>actual &&
> +	test_cmp expect actual
> +'

I was confused by this test as "add -p" doesn't seem to be printing anything apart from the error. That's because it only captures stderr (quite why an interactive program is writing its some of its output to stderr and the rest to stdout is another question). I think we want to capture the whole output otherwise we can't assert that the help isn't printed. Something like this (which also adds coverage for '?' and 'p')

diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index bc55255b0a8..0fc7d4b5d89 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -1130,4 +1130,26 @@ test_expect_success 'reset -p with unmerged files' '
          test_must_be_empty staged
  '
  
+test_expect_success 'invalid key' '
+        echo changed >file &&
+        test_write_lines æ \? p q | force_color git add -p >actual.colored 2>&1 &&
+        test_decode_color <actual.colored >actual &&
+        force_color git diff >diff.colored &&
+        test_decode_color <diff.colored >diff.decoded &&
+        cat diff.decoded >expect &&
+        cat >>expect <<-EOF &&
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>Unknown key ${SQ}æ${SQ}. Use ${SQ}?${SQ} for help<RESET>
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET><BOLD;RED>y - stage this hunk
+        n - do not stage this hunk
+        q - quit; do not stage this hunk or any of the remaining ones
+        a - stage this hunk and all later hunks in the file
+        d - do not stage this hunk or any of the later hunks in the file
+        <RESET><BOLD;RED>e - manually edit the current hunk<RESET>
+        <BOLD;RED>p - print the current hunk<RESET>
+        <BOLD;RED>? - print help<RESET>
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET>$(sed -n "/@/,\$ p" diff.decoded)
+        <BOLD;BLUE>(1/1) Stage this hunk [y,n,q,a,d,e,p,?]? <RESET>
+        EOF
+        test_cmp expect actual
+'
+
  test_done

Best Wishes

Phillip