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

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 21, 2024, 19:37 UTC
Message-ID: <c8f0f709-4a0d-469f-876a-63940705746c@gmail.com>
In-Reply-To: <b1c725f3-0e92-4723-a7ca-8d001d60b89e@gmail.com>

On Sun, Apr 21, 2024 at 02:18:32PM +0100, phillip.wood123@gmail.com wrote:

Show 20 quoted lines

> > +	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
> > +	(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
> > +	git add -p -- command <command >actual 2>&1 &&
> > +	test_cmp expect actual
> 
> It is really good to see us testing the whole of the program output now but
> have you tested this with GIT_TEST_DEFAULT_HASH=sha256? All the other tests
> use diff_cmp() to avoid test failures caused by the index line depending on
> the hash function. Alternatively you could run "git diff" to create that
> part of the expected output.

Oops. Thank you for pointing this out. I'll reroll with a fix for it later today.

> It is a shame we're not testing '?' as we have changed the implementation -
> before this commit '?' was just another unknown command as far as the
> implementation was concerned, now we treat it differently.

I think testing for '?' deserves its own series.

> It would be nice
> to see some test coverage for the 'p' command you recently added as well at
> some point.

Yes.  I'll send some tests for it soon.

Thanks.