| Message ID | 92ddcd1f668906348e20a682cd737d90bb38ddc6.1710800549.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a bug in configuration parsing, and improve tests and documentation | expand |
Dragan Simic <dsimic@manjaro.org> writes: > +test_expect_success 'setup whitespace config' ' > + tr "X" "\011" >.git/config <<-\EOF > + [section] > + Xsolid = rock > + Xsparse = big XX blue > + XsparseAndTail = big XX blue > + XsparseAndTailQuoted = "big XX blue " > + XsparseAndBiggerTail = big XX blue X X > + XsparseAndBiggerTailQuoted = "big XX blue X X" > + XsparseAndBiggerTailQuotedPlus = "big XX blue X X"X > + XheadAndTail = Xbig blue > + XheadAndTailQuoted = "Xbig blue " > + XheadAndTailQuotedPlus = "Xbig blue " > + Xannotated = big blueX# to be discarded > + XannotatedQuoted = "big blue"X# to be discarded > + EOF > +' If you want to write tests where leading and trailing whitespace matter in them, making these invisible characters visible is a good way to convey your intention to your readers. sed -e "s/Q/ /g" \ -e "s/^|//" \ -e "s/[$]$//" <<-\EOF |[section] | solid = rock $ | sparse = tab and space Q $ EOF This is still true even if we assume there is no patch corruption while the e-mail is in transit. It is safe to assume that the receiving end of your patches uses "git am --whitespace=fix" and a common way to protect against it is to use the opposite of "cat -e" as a convention, additional to the "'|' is the left edge of paper" and "Q stands for HT" conventions.
On 2024-03-20 07:42, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> +test_expect_success 'setup whitespace config' ' >> + tr "X" "\011" >.git/config <<-\EOF >> + [section] >> + Xsolid = rock >> + Xsparse = big XX blue >> + XsparseAndTail = big XX blue >> + XsparseAndTailQuoted = "big XX blue " >> + XsparseAndBiggerTail = big XX blue X X >> + XsparseAndBiggerTailQuoted = "big XX blue X X" >> + XsparseAndBiggerTailQuotedPlus = "big XX blue X X"X >> + XheadAndTail = Xbig blue >> + XheadAndTailQuoted = "Xbig blue " >> + XheadAndTailQuotedPlus = "Xbig blue " >> + Xannotated = big blueX# to be discarded >> + XannotatedQuoted = "big blue"X# to be discarded >> + EOF >> +' > > If you want to write tests where leading and trailing whitespace > matter in them, making these invisible characters visible is a good > way to convey your intention to your readers. > > sed -e "s/Q/ /g" \ > -e "s/^|//" \ > -e "s/[$]$//" <<-\EOF > |[section] > | solid = rock $ > | sparse = tab and space Q $ > EOF > > This is still true even if we assume there is no patch corruption > while the e-mail is in transit. It is safe to assume that the > receiving end of your patches uses "git am --whitespace=fix" and a > common way to protect against it is to use the opposite of "cat -e" > as a convention, additional to the "'|' is the left edge of paper" > and "Q stands for HT" conventions. Agreed, will make it so in the v4. I already had some thoughts about protecting the trailing whitespace, and making obvious it wasn't included by mistake. Though, I'll have to use 'X' istead of 'Q' for HTs, because the option names already contain 'Q' characters.
On 2024-03-20 07:46, Dragan Simic wrote: > On 2024-03-20 07:42, Junio C Hamano wrote: >> Dragan Simic <dsimic@manjaro.org> writes: >> >>> +test_expect_success 'setup whitespace config' ' >>> + tr "X" "\011" >.git/config <<-\EOF >>> + [section] >>> + Xsolid = rock >>> + Xsparse = big XX blue >>> + XsparseAndTail = big XX blue >>> + XsparseAndTailQuoted = "big XX blue " >>> + XsparseAndBiggerTail = big XX blue X X >>> + XsparseAndBiggerTailQuoted = "big XX blue X X" >>> + XsparseAndBiggerTailQuotedPlus = "big XX blue X X"X >>> + XheadAndTail = Xbig blue >>> + XheadAndTailQuoted = "Xbig blue " >>> + XheadAndTailQuotedPlus = "Xbig blue " >>> + Xannotated = big blueX# to be discarded >>> + XannotatedQuoted = "big blue"X# to be discarded >>> + EOF >>> +' >> >> If you want to write tests where leading and trailing whitespace >> matter in them, making these invisible characters visible is a good >> way to convey your intention to your readers. >> >> sed -e "s/Q/ /g" \ >> -e "s/^|//" \ >> -e "s/[$]$//" <<-\EOF >> |[section] >> | solid = rock $ >> | sparse = tab and space Q $ >> EOF >> >> This is still true even if we assume there is no patch corruption >> while the e-mail is in transit. It is safe to assume that the >> receiving end of your patches uses "git am --whitespace=fix" and a >> common way to protect against it is to use the opposite of "cat -e" >> as a convention, additional to the "'|' is the left edge of paper" >> and "Q stands for HT" conventions. > > Agreed, will make it so in the v4. I already had some thoughts > about protecting the trailing whitespace, and making obvious it > wasn't included by mistake. > > Though, I'll have to use 'X' istead of 'Q' for HTs, because the > option names already contain 'Q' characters. Oh, I just saw that you've already picked this patch up in the "seen" branch. Would you prefer if I make this change and submit the v4, or to perform the change in the already planned follow-up patches, which would also clean up some other tests a bit?
Dragan Simic <dsimic@manjaro.org> writes: > Oh, I just saw that you've already picked this patch up in the "seen" > branch. Would you prefer if I make this change and submit the v4, or > to perform the change in the already planned follow-up patches, which > would also clean up some other tests a bit? The purpose of the "seen" branch is to bundle the branches the maintainer happens to have seen, and to remind the maintainer that the topics in them might turn out to be interesting when they are polished. Nothing more than that. Consider that a topic only in "seen" is not part of "git" yet. The contributors can use it to anticipate what topics from others may cause conflict with their own work, and find people who are working on these topics to talk to before the potential conflicts get out of control. It would be a good idea to fork from maint or master to grow a topic and to test (1) it by itself, (2) a temporary merge of it to 'next' and (3) a temporary merge to it to 'seen', before publishing it.
Hello Junio, On 2024-03-20 15:28, Junio C Hamano wrote: > Dragan Simic <dsimic@manjaro.org> writes: > >> Oh, I just saw that you've already picked this patch up in the "seen" >> branch. Would you prefer if I make this change and submit the v4, or >> to perform the change in the already planned follow-up patches, which >> would also clean up some other tests a bit? > > The purpose of the "seen" branch is to bundle the branches the > maintainer happens to have seen, and to remind the maintainer that > the topics in them might turn out to be interesting when they are > polished. Nothing more than that. Consider that a topic only in > "seen" is not part of "git" yet. > > The contributors can use it to anticipate what topics from others > may cause conflict with their own work, and find people who are > working on these topics to talk to before the potential conflicts > get out of control. It would be a good idea to fork from maint or > master to grow a topic and to test (1) it by itself, (2) a temporary > merge of it to 'next' and (3) a temporary merge to it to 'seen', > before publishing it. Thank you for the clarification. Hence, I'll send the v4.
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 31c387868708..7688362826ea 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -11,6 +11,96 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh +test_expect_success 'setup whitespace config' ' + tr "X" "\011" >.git/config <<-\EOF + [section] + Xsolid = rock + Xsparse = big XX blue + XsparseAndTail = big XX blue + XsparseAndTailQuoted = "big XX blue " + XsparseAndBiggerTail = big XX blue X X + XsparseAndBiggerTailQuoted = "big XX blue X X" + XsparseAndBiggerTailQuotedPlus = "big XX blue X X"X + XheadAndTail = Xbig blue + XheadAndTailQuoted = "Xbig blue " + XheadAndTailQuotedPlus = "Xbig blue " + Xannotated = big blueX# to be discarded + XannotatedQuoted = "big blue"X# to be discarded + EOF +' + +test_expect_success 'no internal whitespace' ' + echo "rock" >expect && + git config --get section.solid >actual && + test_cmp expect actual +' + +test_expect_success 'internal whitespace' ' + echo "big QQ blue" | q_to_tab >expect && + git config --get section.sparse >actual && + test_cmp expect actual +' + +test_expect_success 'internal and trailing whitespace' ' + echo "big QQ blue" | q_to_tab >expect && + git config --get section.sparseAndTail >actual && + test_cmp expect actual +' + +test_expect_success 'internal and trailing whitespace, all quoted' ' + echo "big QQ blue " | q_to_tab >expect && + git config --get section.sparseAndTailQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'internal and more trailing whitespace' ' + echo "big QQ blue" | q_to_tab >expect && + git config --get section.sparseAndBiggerTail >actual && + test_cmp expect actual +' + +test_expect_success 'internal and more trailing whitespace, all quoted' ' + echo "big QQ blue Q Q" | q_to_tab >expect && + git config --get section.sparseAndBiggerTailQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'internal and more trailing whitespace, not all quoted' ' + echo "big QQ blue Q Q" | q_to_tab >expect && + git config --get section.sparseAndBiggerTailQuotedPlus >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace' ' + echo "big blue" >expect && + git config --get section.headAndTail >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace, all quoted' ' + echo "Qbig blue " | q_to_tab >expect && + git config --get section.headAndTailQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace, not all quoted' ' + echo "Qbig blue " | q_to_tab >expect && + git config --get section.headAndTailQuotedPlus >actual && + test_cmp expect actual +' + +test_expect_success 'inline comment' ' + echo "big blue" >expect && + git config --get section.annotated >actual && + test_cmp expect actual +' + +test_expect_success 'inline comment, quoted' ' + echo "big blue" >expect && + git config --get section.annotatedQuoted >actual && + test_cmp expect actual +' + test_expect_success 'clear default config' ' rm -f .git/config ' @@ -1066,9 +1156,25 @@ test_expect_success '--null --get-regexp' ' test_cmp expect result ' -test_expect_success 'inner whitespace kept verbatim' ' - git config section.val "foo bar" && - test_cmp_config "foo bar" section.val +test_expect_success 'inner whitespace kept verbatim, spaces only' ' + echo "foo bar" >expect && + git config section.val "foo bar" && + git config --get section.val >actual && + test_cmp expect actual +' + +test_expect_success 'inner whitespace kept verbatim, horizontal tabs only' ' + echo "fooQQbar" | q_to_tab >expect && + git config section.val "$(cat expect)" && + git config --get section.val >actual && + test_cmp expect actual +' + +test_expect_success 'inner whitespace kept verbatim, horizontal tabs and spaces' ' + echo "foo Q bar" | q_to_tab >expect && + git config section.val "$(cat expect)" && + git config --get section.val >actual && + test_cmp expect actual ' test_expect_success SYMLINKS 'symlinked configuration' '
Add a handful of additional automated tests, to improve the coverage of configuration file entries whose values contain internal whitespace, leading and/or trailing whitespace, which may or may not be enclosed within quotation marks, or which contain an additional inline comment. At the same time, rework one already existing automated test a bit, to ensure consistency with the newly added tests. This change introduced no functional changes to the already existing test. Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- Notes: Changes in v3: - Removed a few unnecessary invocations of x_to_tab() - As pointed out by Eric Sunshine, [3] it's better not to introduce new random helper functions, so x_to_tab() was replaced by a direct invocation of tr(1), in one case that requires use of literal 'Q' characters, and by invocations of q_to_tab(), in the remaining cases that actually allow use of 'Q' characters to represent HTs - Dropped the change of the name of an already existing test, to not distract the reviewers, as suggested by Eric Sunshine [4] - Renamed the first added test, as suggested by Eric Sunshine, [4] to make it consistent with the expected way for naming setup tests Changes in v2: - All new tests and one already existing test reworked according to Eric Sunshine's review suggestions; [1][2] the already existing test was reworked a bit to ensure consistency - Added a Helped-by tag [1] https://lore.kernel.org/git/CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com/ [2] https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/ [3] https://lore.kernel.org/git/CAPig+cSLb+Rsy81itvw9Tfvqv9vvKSPgO_ER9fWL04XZrgFvwg@mail.gmail.com/T/#u [4] https://lore.kernel.org/git/CAPig+cTVmQzC38DympSEtPNhgY=-+dYbZmkr0RTRbhG-hp2fmQ@mail.gmail.com/ t/t1300-config.sh | 112 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 109 insertions(+), 3 deletions(-)