git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 1/6] t0300: don't create unused file

From
Andrei Rybak <rybak.a.v@gmail.com>
Date
Apr 6, 2023, 21:01 UTC
Message-ID
<db2de983-9b1f-5efb-0fdc-cc704e6b875b@gmail.com>
In-Reply-To
<230406.86ttxttnir.gmgdl@evledraar.gmail.com>
On 06/04/2023 10:34, Ævar Arnfjörð Bjarmason wrote:
Show 35 quoted lines
> 
> On Tue, Apr 04 2023, Andrei Rybak wrote:
> 
>> Test 'credential config with partial URLs' in t0300-credentials.sh
>> contains three "git credential fill" invocations.  For two of the
>> invocations, the test asserts presence or absence of string "yep" in the
>> standard output.  For the third test it checks for an error message in
>> standard error.
>>
>> Don't redirect standard output of "git credential" to file "stdout" in
>> t0300-credentials.sh to avoid creating an unnecessary file when only
>> standard error is checked.
>>
>> Signed-off-by: Andrei Rybak <rybak.a.v@gmail.com>
>> ---
>>   t/t0300-credentials.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
>> index c66d91e82d..b8612ede95 100755
>> --- a/t/t0300-credentials.sh
>> +++ b/t/t0300-credentials.sh
>> @@ -808,7 +808,7 @@ test_expect_success 'credential config with partial URLs' '
>>   
>>   	git -c credential.$partial.helper=yep \
>>   		-c credential.with%0anewline.username=uh-oh \
>> -		credential fill <stdin >stdout 2>stderr &&
>> +		credential fill <stdin 2>stderr &&
>>   	test_i18ngrep "skipping credential lookup for key" stderr
>>   '
> 
> This goes for these changes in this series general: You're correct that
> this is useless now, but I don't think it follows that we should be
> removing the "redundant" code in all cases, rather than fixing the test
> to actually check these.

I'll reply to these on one-by-one. See also this review of a patch in part 1 of this series from Junio C Hamano:

   https://lore.kernel.org/git/xmqqsfe8s56p.fsf@gitster.g/
Show 23 quoted lines
> E.g. this will also make this test pass:
> 	
> 	diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
> 	index c66d91e82d8..62c2a0fd50e 100755
> 	--- a/t/t0300-credentials.sh
> 	+++ b/t/t0300-credentials.sh
> 	@@ -806,9 +806,11 @@ test_expect_success 'credential config with partial URLs' '
> 	 		return 1
> 	 	done &&
> 	
> 	+	cp stdout stdout.last &&
> 	 	git -c credential.$partial.helper=yep \
> 	 		-c credential.with%0anewline.username=uh-oh \
> 	 		credential fill <stdin >stdout 2>stderr &&
> 	+	test_cmp stdout.last stdout &&
> 	 	test_i18ngrep "skipping credential lookup for key" stderr
> 	 '
> 	
> 
> Does that make sense? No idea, I don't know the credential system well.
> 
> But isn't it worth testing that when we ask for this that we're getting
> some known output along with the warning?
Current version from branch "master" with more context lines:
> test_expect_success 'credential config with partial URLs' '
> 	echo "echo password=yep" | write_script git-credential-yep &&
> 	test_write_lines url=https://user@example.com/repo.git >stdin &&
The test is checking that "url=https://user@example.com/repo.git" matches ...
Show 11 quoted lines
> 	for partial in \
> 		example.com \
> 		user@example.com \
> 		https:// \
> 		https://example.com \
> 		https://example.com/ \
> 		https://user@example.com \
> 		https://user@example.com/ \
> 		https://example.com/repo.git \
> 		https://user@example.com/repo.git \
> 		/repo.git
... these partial URLs ...
Show 11 quoted lines
> 	do
> 		git -c credential.$partial.helper=yep \
> 			credential fill <stdin >stdout &&
> 		grep yep stdout ||
> 		return 1
> 	done &&
> 
> 	for partial in \
> 		dont.use.this \
> 		http:// \
> 		/repo
... but doesn't match these.
> 	do
> 		git -c credential.$partial.helper=yep \
> 			credential fill <stdin >stdout &&
> 		! grep yep stdout ||

Here "! grep yep stdout" ensures that git-credential-yep isn't launched for the three kinds of partial URLs. Otherwise, the actual content of "stdout" is ignored. It comes from script "askpass" (see bottom of file "t/lib-credential.sh").

Absence or presence of "yep" is a proxy for whether or not partial URL got matched or not, and that is what's important for this test. Adding assertions for output of "askpass" here would only obscure this fact, I think.

There are other tests in t030[0-3] that do check standard output for what the helper script "askpass" prints out -- those tests validate that the "git credentials" fallbacks to asking for credentials in the terminal in various situations. This is done via functions helper_test and helper_test_timeout from "t/lib-credential.sh".

Show 7 quoted lines
> 		return 1
> 	done &&
> 
> 	git -c credential.$partial.helper=yep \
> 		-c credential.with%0anewline.username=uh-oh \
> 		credential fill <stdin >stdout 2>stderr &&
> 	test_i18ngrep "skipping credential lookup for key" stderr

Here, the important part is that "git credential" reacts to invalid key being used: "credential.with%0anewline.username=uh-oh", and similarly, I think that adding assertions about "stdout" might not be a good idea.

>'

Perhaps Johannes Schindelin, author of commit 9a121b0d22 ("credential: handle `credential.<partial-URL>.<key>` again", 2020-04-24), could chime in.

Previous: Ævar Arnfjörð BjarmasonNext: Andrei Rybak
Message 12 of 58 in “t: fix unused files, part 2”
  1. 0/6 t: fix unused files, part 2Andrei Rybak, Apr 1, 2023
  2. 1/6 t0300: don't create unused fileAndrei Rybak, Apr 1, 2023
  3. Eric SunshineApr 2, 2023
  4. 5/6 t1502: don't create unused filesAndrei Rybak, Apr 1, 2023
  5. 2/6 t1300: fix config file syntax error descriptionsAndrei Rybak, Apr 1, 2023
  6. 3/6 t1300: don't create unused filesAndrei Rybak, Apr 1, 2023
  7. 4/6 t1450: don't create unused filesAndrei Rybak, Apr 1, 2023
  8. 6/6 t2019: don't create unused filesAndrei Rybak, Apr 1, 2023
  9. 0/6 t: fix unused files, part 2Andrei Rybak, Apr 3, 2023
  10. 1/6 t0300: don't create unused fileAndrei Rybak, Apr 3, 2023
  11. Ævar Arnfjörð BjarmasonApr 6, 2023
  12. Andrei RybakApr 6, 2023
  13. 2/6 t1300: fix config file syntax error descriptionsAndrei Rybak, Apr 3, 2023
  14. 4/6 t1450: don't create unused filesAndrei Rybak, Apr 3, 2023
  15. Ævar Arnfjörð BjarmasonApr 6, 2023
  16. Andrei RybakApr 6, 2023
  17. 6/6 t2019: don't create unused filesAndrei Rybak, Apr 3, 2023
  18. Ævar Arnfjörð BjarmasonApr 6, 2023
  19. Andrei RybakApr 7, 2023
  20. t2024: fix loose/strict local base branch DWIM testAndrei Rybak, Apr 8, 2023
  21. Junio C HamanoApr 10, 2023
  22. 5/6 t1502: don't create unused filesAndrei Rybak, Apr 3, 2023
  23. Øystein WalleApr 6, 2023
  24. Ævar Arnfjörð BjarmasonApr 6, 2023
  25. Andrei RybakApr 6, 2023
  26. 3/6 t1300: don't create unused filesAndrei Rybak, Apr 3, 2023
  27. Ævar Arnfjörð BjarmasonApr 6, 2023
  28. Andrei RybakApr 6, 2023
  29. git config tests for "'git config ignores pairs ..." (was Re: [PATCH v2 3/6] t1300: don't create unused files)Andrei Rybak, Apr 6, 2023
  30. 0/2 git config tests for "'git config ignores pairs ..."Andrei Rybak, Apr 14, 2023
  31. 1/2 t1300: drop duplicate testAndrei Rybak, Apr 14, 2023
  32. 2/2 t1300: check stderr for "ignores pairs" testsAndrei Rybak, Apr 14, 2023
  33. Andrei RybakApr 14, 2023
  34. 0/3 git config tests for "'git config ignores pairs ..."Andrei Rybak, Apr 18, 2023
  35. 1/3 t1300: drop duplicate testAndrei Rybak, Apr 18, 2023
  36. Junio C HamanoApr 18, 2023
  37. 2/3 t1300: check stderr for "ignores pairs" testsAndrei Rybak, Apr 18, 2023
  38. Junio C HamanoApr 18, 2023
  39. 3/3 t1300: add tests for missing keysAndrei Rybak, Apr 18, 2023
  40. Junio C HamanoApr 18, 2023
  41. Andrei RybakApr 18, 2023
  42. 0/3 git config tests for "'git config ignores pairs ..."Andrei Rybak, Apr 23, 2023
  43. 1/3 t1300: drop duplicate testAndrei Rybak, Apr 23, 2023
  44. 2/3 t1300: check stderr for "ignores pairs" testsAndrei Rybak, Apr 23, 2023
  45. 3/3 t1300: add tests for missing keysAndrei Rybak, Apr 23, 2023
  46. Junio C HamanoMay 1, 2023
  47. Andrei RybakMay 2, 2023
  48. 0/6 t: fix unused files, part 2Andrei Rybak, Apr 17, 2023
  49. 1/6 t0300: don't create unused fileAndrei Rybak, Apr 17, 2023
  50. 2/6 t1300: fix config file syntax error descriptionsAndrei Rybak, Apr 17, 2023
  51. 3/6 t1300: don't create unused filesAndrei Rybak, Apr 17, 2023
  52. 4/6 t1450: don't create unused filesAndrei Rybak, Apr 17, 2023
  53. 5/6 t1502: don't create unused filesAndrei Rybak, Apr 17, 2023
  54. 6/6 t2019: don't create unused filesAndrei Rybak, Apr 17, 2023
  55. Junio C HamanoMay 1, 2023
  56. Andrei RybakMay 2, 2023
  57. Elijah NewrenMay 3, 2023
  58. Junio C HamanoMay 3, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.