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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Junio

On 24/04/2024 16:35, Junio C Hamano wrote:
> phillip.wood123@gmail.com writes:
> 
>> On 22/04/2024 16:50, Junio C Hamano wrote:
>>> Rubén Justo <rjusto@gmail.com> writes:
>>>
>>>> 1:  0317594bce ! 1:  b418b03f15 add-patch: response to unknown command
>>>>       @@ t/t3701-add-interactive.sh: test_expect_success 'warn about add.interactive.useB
>>>>        +	test_when_finished "git reset --hard; rm -f command" &&
>>>>        +	echo W >command &&
>>>>        +	git add -N command &&
>>>>       -+	cat >expect <<-EOF &&
>>>>       -+	diff --git a/command b/command
>>>>       -+	new file mode 100644
>>>>       -+	index 0000000..a42d8ff
>>>>       -+	--- /dev/null
>>>>       -+	+++ b/command
>>>>       -+	@@ -0,0 +1 @@
>>>>       -+	+W
>>>>       ++	git diff command >expect &&
>>>>       ++	cat >>expect <<-EOF &&
>>>>        +	(1/1) Stage addition [y,n,q,a,d,e,p,?]? Unknown command ${SQ}W${SQ} (use ${SQ}?${SQ} for help)
>>>>        +	(1/1) Stage addition [y,n,q,a,d,e,p,?]?$SP
>>>>        +	EOF
>>> Interesting.
>>> My first reaction was "how is this different from checking just the
>>> last line of the actual output?  The early part of expect and actual
>>> both come from an internal invocation of 'git diff', and they must
>>> match by definition".
>>> But that may really be the point of this test.
>>
>> Yes - we want to make sure that we are not printing the help and the
>> only way to do that is to check the whole output
> 
> I was not questioning that part of the patch.  "My first reaction"
> was solely about use of "git diff" to replace the golden copy of
> expected result in the test itself, only to allow for use of
> different hash functions.  As you (or somebody else?) mentioned in
> an earlier review, diff_cmp is there for exactly that purpose.

Oh sorry I'd misunderstood

>>> That is, we may later decide to tweak the way "git diff" hunks are
>>> presented, and we expect that the way "git add -p" presents the
>>> hunks would change together with it automatically.
> 
> This argument cuts both ways, by the way.
> 
> Insisting that the output match the explicit expectation (not what
> "git diff" of the day produces) has a few advantages.  It makes the
> test more explicit and easy to see what output we are expecting, and
> more importantly, it forces us to update the test when we decide to
> tweak the output from the command being tested (i.e. hunk selection
> UI) and/or the output from "git diff" command.

There is also a practical argument against using "git diff" to generate 
the expected output as it only works if the diff contains a single hunk. 
If the diff contains more than one hunk "add -p" displays them separately.

Best Wishes

Phillip