| Message ID | 9a73e7d3cbb9ea210ed1098c5a304b0f5d5e1a2e.1710646998.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix a bug in configuration parsing, and improve tests and documentation | expand |
On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> wrote: > 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> > --- > [2] https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/ > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > @@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > +test_expect_success 'create test configuration' ' In [2] above, I intentionally suggested naming this new test "setup whitespace" because "setup" is a common name used in the test suite for this sort of test which prepares state for subsequent tests. Using a common name (such as "setup") is important since it facilitates running only specific tests within a script in which you are interested rather than having to run all tests. The section "Skipping Tests" in t/README says this: Sometimes there may be multiple tests with e.g. "setup" in their name that are needed and rather than figuring out the number for all of them we can just use "setup" as a substring/glob to match against the test description: $ sh ./t0050-filesystem.sh --run=setup,9-11 or one could select both the setup tests and the rename ones (assuming all relevant tests had those words in their descriptions): $ sh ./t0050-filesystem.sh --run=setup,rename > + x_to_tab >.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 > +' The <<- operator strips all leading TAB characters, so the extra indentation you've placed inside the "[section]" section is stripped off. Thus, what you have above is the same as: x_to_tab >.git/config <<-\EOF [section] Xsolid = rock ... EOF On a related note, it's not clear why you use 'X' to insert a TAB at the beginning of each line. As I understand it, the configuration file reader does not require such indentation, thus doing so is wasted. Moreover, it confuses readers of this code (and reviewers) into thinking that something unusual is going on, and leads to questions such as this one: Why do you use 'X' to insert a TAB at the beginning of the line? > -test_expect_success 'clear default config' ' > +test_expect_success 'clear default configuration' ' > rm -f .git/config > ' It's probably not worth a reroll, but it's usually better to avoid this sort of do-nothing noise-change since it distracts reviewers from the primary changes made by the patch. > @@ -1066,9 +1156,25 @@ test_expect_success '--null --get-regexp' ' > -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 > +' I appreciate the revised test title ("spaces only") which indicates that these aren't TABs which were missed when converting to use q_to_tab() or x_to_tab().
On Sun, Mar 17, 2024 at 12:21 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> wrote: > > diff --git a/t/t1300-config.sh b/t/t1300-config.sh > > @@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > > +test_expect_success 'create test configuration' ' > > In [2] above, I intentionally suggested naming this new test "setup > whitespace" [...] Of course, I meant [1]. [1]: https://lore.kernel.org/git/CAPig+cRMPNExbG34xJ0w5npUc3DDwxQUGS_AQfam_mi4s53=sA@mail.gmail.com/
On 2024-03-17 05:21, Eric Sunshine wrote: > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> 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> >> --- >> [2] >> https://lore.kernel.org/git/CAPig+cRG8eFxepkaiN54H+fa7D=rFGsmEHdvTP+HSSaLO_6T_A@mail.gmail.com/ >> >> diff --git a/t/t1300-config.sh b/t/t1300-config.sh >> @@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME >> +test_expect_success 'create test configuration' ' > > In [2] above, I intentionally suggested naming this new test "setup > whitespace" because "setup" is a common name used in the test suite > for this sort of test which prepares state for subsequent tests. Using > a common name (such as "setup") is important since it facilitates > running only specific tests within a script in which you are > interested rather than having to run all tests. The section "Skipping > Tests" in t/README says this: > > Sometimes there may be multiple tests with e.g. "setup" in their > name that are needed and rather than figuring out the number for > all of them we can just use "setup" as a substring/glob to match > against the test description: > > $ sh ./t0050-filesystem.sh --run=setup,9-11 > > or one could select both the setup tests and the rename ones > (assuming all relevant tests had those words in their > descriptions): > > $ sh ./t0050-filesystem.sh --run=setup,rename Totally agreed, thanks for pointing this out. Will be fixed in v3. >> + x_to_tab >.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 >> +' > > The <<- operator strips all leading TAB characters, so the extra > indentation you've placed inside the "[section]" section is stripped > off. Thus, what you have above is the same as: > > x_to_tab >.git/config <<-\EOF > [section] > Xsolid = rock > ... > EOF Yes, I was already aware that such indentation ends up wasted, but having it makes the test a bit more readable. At least in my opinion, but if you find it better not to have such whitespace, for the sake of consistency, I'll happily remove this indentation in the v3. > On a related note, it's not clear why you use 'X' to insert a TAB at > the beginning of each line. As I understand it, the configuration file > reader does not require such indentation, thus doing so is wasted. > Moreover, it confuses readers of this code (and reviewers) into > thinking that something unusual is going on, and leads to questions > such as this one: Why do you use 'X' to insert a TAB at the beginning > of the line? Well, resorting to always not having such instances of 'X' to provide leading indentation in test configuration files may actually make some bugs go undetected in some tests. To me, having leading indentation is to be expected in the real configuration files in the field, thus providing the same indentation in a test configuration feels natural to me, despite admittedly making the test configuration a bit less readable. Of course, consistency is good, but variety is also good when it comes to automated tests. I'm not very familiar with the tests in git, so please let me know if consistency outweights variety in this case, and I'll happily remove the leading "X" indentations in the v3. >> -test_expect_success 'clear default config' ' >> +test_expect_success 'clear default configuration' ' >> rm -f .git/config >> ' > > It's probably not worth a reroll, but it's usually better to avoid > this sort of do-nothing noise-change since it distracts reviewers from > the primary changes made by the patch. The v3 is already inevitable, so I'll drop this change. >> @@ -1066,9 +1156,25 @@ test_expect_success '--null --get-regexp' ' >> -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 >> +' > > I appreciate the revised test title ("spaces only") which indicates > that these aren't TABs which were missed when converting to use > q_to_tab() or x_to_tab(). Thanks. :)
On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-03-17 05:21, Eric Sunshine wrote: > > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> > > wrote: > >> + x_to_tab >.git/config <<-\EOF > >> + [section] > >> + Xsolid = rock > >> + Xsparse = big XX blue > >> + ... > >> + EOF > >> +' > > > > The <<- operator strips all leading TAB characters, so the extra > > indentation you've placed inside the "[section]" section is stripped > > off. Thus, what you have above is the same as: > > > > x_to_tab >.git/config <<-\EOF > > [section] > > Xsolid = rock > > ... > > EOF > > Yes, I was already aware that such indentation ends up wasted, but > having > it makes the test a bit more readable. At least in my opinion, but if > you > find it better not to have such whitespace, for the sake of consistency, > I'll happily remove this indentation in the v3. Readability wasn't my reason for bringing this up. As a reviewer, every time a question pops into my mind as I'm reading the code, that indicates that something about the code is unclear or that the commit message doesn't properly explain why it was done in this way. People coming across this code in the future may have the same questions but they won't have the benefit of being able to easily ask you why it was done this way. So, the ideal patch is one in which the reviewer reads the code and simply nods in understanding without having to question any of the author's choices. And the ideal test is one in which does the bare minimum to verify that the condition being checked is in fact correct; there should be no extraneous code or behavior which could mislead the reader into wondering why it was done that way. In this particular case, it wasn't clear whether you, as author, realized that all leading TABs are stripped from the heredoc body, and whether or not that mattered to you and whether or not you expected it to be significant to the test's behavior. > > On a related note, it's not clear why you use 'X' to insert a TAB at > > the beginning of each line. As I understand it, the configuration file > > reader does not require such indentation, thus doing so is wasted. > > Moreover, it confuses readers of this code (and reviewers) into > > thinking that something unusual is going on, and leads to questions > > such as this one: Why do you use 'X' to insert a TAB at the beginning > > of the line? > > Well, resorting to always not having such instances of 'X' to provide > leading indentation in test configuration files may actually make some > bugs go undetected in some tests. To me, having leading indentation is > to be expected in the real configuration files in the field, thus > providing > the same indentation in a test configuration feels natural to me, > despite > admittedly making the test configuration a bit less readable. > > Of course, consistency is good, but variety is also good when it comes > to automated tests. I'm not very familiar with the tests in git, so > please let me know if consistency outweights variety in this case, and > I'll happily remove the leading "X" indentations in the v3. My assumption, perhaps incorrectly, was that existing tests already verified correct behavior of leading whitespace and that the tests added by this patch were about internal whitespace. If that's not the case (and perhaps I didn't fully digest the commit message) then my question about the leading "X" is off the mark. If these new tests are also checking leading whitespace behavior, then to improve coverage, would it make sense to have the leading "X" on some lines but not others?
Eric Sunshine <sunshine@sunshineco.com> writes: >> >> + x_to_tab >.git/config <<-\EOF >> >> + [section] >> >> + Xsolid = rock >> >> + Xsparse = big XX blue >> >> + ... >> >> + EOF >> >> +' Just this part. > My assumption, perhaps incorrectly, was that existing tests already > verified correct behavior of leading whitespace and that the tests > added by this patch were about internal whitespace. If that's not the > case (and perhaps I didn't fully digest the commit message) then my > question about the leading "X" is off the mark. > > If these new tests are also checking leading whitespace behavior, then > to improve coverage, would it make sense to have the leading "X" on > some lines but not others? If "<<-" (I have here-doc but please strip the leading tabs because I am aligning the here-doc with them) gets in the way for testing material with leading tabs, the way to write and preprocess such a here-doc is: sed -e 's/^|//' -e 's/Q/ /g' >.git/config <<-\EOF |[section] | solid = rock | sparse = big QQ blue | ... EOF It will make it clear where the left-edge of the "sheet of paper" is, removal of leading '|' does not get in the way of using '|' in the middle of the line if needed, and Q being the least used letter makes them stand out more in the middle of the line. As it is obvious that what is before solid and sparse is a tab (otherwise you would not be using that '|' trick), you do not have to write Xsolid or Qsolid there and still the result is much easier to read.
Hello Eric, On 2024-03-18 03:48, Eric Sunshine wrote: > On Sun, Mar 17, 2024 at 12:50 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-03-17 05:21, Eric Sunshine wrote: >> > On Sat, Mar 16, 2024 at 11:48 PM Dragan Simic <dsimic@manjaro.org> >> > wrote: >> >> + x_to_tab >.git/config <<-\EOF >> >> + [section] >> >> + Xsolid = rock >> >> + Xsparse = big XX blue >> >> + ... >> >> + EOF >> >> +' >> > >> > The <<- operator strips all leading TAB characters, so the extra >> > indentation you've placed inside the "[section]" section is stripped >> > off. Thus, what you have above is the same as: >> > >> > x_to_tab >.git/config <<-\EOF >> > [section] >> > Xsolid = rock >> > ... >> > EOF >> >> Yes, I was already aware that such indentation ends up wasted, >> but having it makes the test a bit more readable. At least in >> my opinion, but if you find it better not to have such whitespace, >> for the sake of consistency, I'll happily remove this indentation >> in the v3. > > Readability wasn't my reason for bringing this up. As a reviewer, > every time a question pops into my mind as I'm reading the code, that > indicates that something about the code is unclear or that the commit > message doesn't properly explain why it was done in this way. People > coming across this code in the future may have the same questions but > they won't have the benefit of being able to easily ask you why it was > done this way. I see. How about including a small comment in the t1300 that would explain the additional indentation? As a note, there are already more tests in the t1300 that contain such indentation, so maybe we shoulddo something with those existing tests as well; the above-proposed comment, which would be placed at the very beginning of t1300, may provide a satisfactory explanation for all the tests in t1300 that contain such additional indentation. Another option would be to either add the indentation to all relevant tests in the t1300, or to remove the indentation from all tests in the t1300 that already contain it. I'd be happy to implement and submit patches that do that, after we choose the direction we want to follow. > So, the ideal patch is one in which the reviewer reads the code and > simply nods in understanding without having to question any of the > author's choices. And the ideal test is one in which does the bare > minimum to verify that the condition being checked is in fact correct; > there should be no extraneous code or behavior which could mislead the > reader into wondering why it was done that way. > > In this particular case, it wasn't clear whether you, as author, > realized that all leading TABs are stripped from the heredoc body, and > whether or not that mattered to you and whether or not you expected it > to be significant to the test's behavior. I fully agree with the most of your points above. The only slight disagreement would be about the bare minimum when it comes to automated tests; in my, opinion, it's actually good to have a few twisties here and there, simply to exercise the underlying logic better by such tests. This probably opens a question whether the automated tests should be orthogonal? Perhaps the majority od them should, but IMHO having a few composite tests can only help with the validation of the underlying logic. >> > On a related note, it's not clear why you use 'X' to insert a TAB at >> > the beginning of each line. As I understand it, the configuration file >> > reader does not require such indentation, thus doing so is wasted. >> > Moreover, it confuses readers of this code (and reviewers) into >> > thinking that something unusual is going on, and leads to questions >> > such as this one: Why do you use 'X' to insert a TAB at the beginning >> > of the line? >> >> Well, resorting to always not having such instances of 'X' to provide >> leading indentation in test configuration files may actually make some >> bugs go undetected in some tests. To me, having leading indentation >> is >> to be expected in the real configuration files in the field, thus >> providing the same indentation in a test configuration feels natural >> to >> me, despite admittedly making the test configuration a bit less >> readable. >> >> Of course, consistency is good, but variety is also good when it comes >> to automated tests. I'm not very familiar with the tests in git, so >> please let me know if consistency outweights variety in this case, and >> I'll happily remove the leading "X" indentations in the v3. > > My assumption, perhaps incorrectly, was that existing tests already > verified correct behavior of leading whitespace and that the tests > added by this patch were about internal whitespace. If that's not the > case (and perhaps I didn't fully digest the commit message) then my > question about the leading "X" is off the mark. Frankly, I can't be 100% sure without spending quite a lot of time, but I don't think that the already existing tests in the t1300 were tailored specifically to cover the verification of the leading whitespace, i.e. of the indentation in git configuration files. To me, it seems more like a random choice of including the indentation or not. Such a random approach may actually be good, despite bringing back the above-mentioned question about the orthogonality of the tests. > If these new tests are also checking leading whitespace behavior, then > to improve coverage, would it make sense to have the leading "X" on > some lines but not others? Good point, despite that not being the main purpose of the added tests. I'll see to add a couple of tests that check the handling of indentation, possibly at some places in the t1300 that fit the best; improving the tests coverage can only help in the long run.
Hello Junio, On 2024-03-18 05:38, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > >>> >> + x_to_tab >.git/config <<-\EOF >>> >> + [section] >>> >> + Xsolid = rock >>> >> + Xsparse = big XX blue >>> >> + ... >>> >> + EOF >>> >> +' > > Just this part. > >> My assumption, perhaps incorrectly, was that existing tests already >> verified correct behavior of leading whitespace and that the tests >> added by this patch were about internal whitespace. If that's not the >> case (and perhaps I didn't fully digest the commit message) then my >> question about the leading "X" is off the mark. >> >> If these new tests are also checking leading whitespace behavior, then >> to improve coverage, would it make sense to have the leading "X" on >> some lines but not others? > > If "<<-" (I have here-doc but please strip the leading tabs because > I am aligning the here-doc with them) gets in the way for testing > material with leading tabs, the way to write and preprocess such a > here-doc is: > > sed -e 's/^|//' -e 's/Q/ /g' >.git/config <<-\EOF > |[section] > | solid = rock > | sparse = big QQ blue > | ... > EOF > > It will make it clear where the left-edge of the "sheet of paper" > is, removal of leading '|' does not get in the way of using '|' in > the middle of the line if needed, and Q being the least used letter > makes them stand out more in the middle of the line. As it is > obvious that what is before solid and sparse is a tab (otherwise you > would not be using that '|' trick), you do not have to write Xsolid > or Qsolid there and still the result is much easier to read. This looks quite neat. Furthermore, I think we should also consider the already existing tests in the t1300 that contain such indentation. As I already explained in my earlier response to Eric, [1] the choice of including the indentation or not seems random to me, so we should perhaps consider taking some broader approach. How about this as a plan for moving forward: 1) Sprinkle a couple of tests onto the t1300, which try to be focused on the verification of the indentation-handling logic; maybe those additional tests could be even seen as redundant, but I think they can only help with the test coverage 2) Create a new helper function that uses the logic you described above, to make it simpler to include the indentation into configs 3) Finally, propagate the use of this new helper function into the new test and the already existing tests in the t1300 that already include the indentation I'd be happy to implement all of the above-proposed steps in the next couple of days. Sure, it would be quite time-consuming, especially the third proposed step, but it should be worth it in the long run. [1] https://lore.kernel.org/git/c579edaac0d67a6ff46fe02072bddbb4@manjaro.org/
On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-03-18 03:48, Eric Sunshine wrote: > > Readability wasn't my reason for bringing this up. As a reviewer, > > every time a question pops into my mind as I'm reading the code, that > > indicates that something about the code is unclear or that the commit > > message doesn't properly explain why it was done in this way. People > > coming across this code in the future may have the same questions but > > they won't have the benefit of being able to easily ask you why it was > > done this way. > > I see. How about including a small comment in the t1300 that would > explain the additional indentation? I'm just one reviewer. Unless others chime in with similar observations or questions regarding the patch, I don't think such a comment is necessary. Aside from the other more significant points (such as not introducing x_to_tab(), using "setup" in the function title, etc.), this is extremely minor, and what you have here is "good enough" (though you may want to take Junio's suggestion of using a leading "|" to protect indentation). > As a note, there are already more tests in the t1300 that contain such > indentation, so maybe we shoulddo something with those existing tests > as well; the above-proposed comment, which would be placed at the very > beginning of t1300, may provide a satisfactory explanation for all the > tests in t1300 that contain such additional indentation. > > Another option would be to either add the indentation to all relevant > tests in the t1300, or to remove the indentation from all tests in the > t1300 that already contain it. I'd be happy to implement and submit > patches that do that, after we choose the direction we want to follow. It would be better to keep this series focused on its primary goal of fixing a bug rather than being held hostage to an ever increasing set of potential cleanups. Such cleanups can be done as separate patch series either atop this series or alongside it. Let's land this series first, and then, if you wish, tackle those other less significant issues. > > If these new tests are also checking leading whitespace behavior, then > > to improve coverage, would it make sense to have the leading "X" on > > some lines but not others? > > Good point, despite that not being the main purpose of the added tests. > I'll see to add a couple of tests that check the handling of > indentation, > possibly at some places in the t1300 that fit the best; improving the > tests coverage can only help in the long run. As above, such additional tests probably aren't mandatory for this bug-fix series. As a reviewer, I'd like to see fewer and fewer changes between each version of a patch series; the series should converge so that it can land rather than diverge from iteration to iteration. Such additional leading-whitespace tests may be perfectly appropriate for a follow-up series.
On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-03-18 05:38, Junio C Hamano wrote: > > sed -e 's/^|//' -e 's/Q/ /g' >.git/config <<-\EOF > > |[section] > > | solid = rock > > | sparse = big QQ blue > > | ... > > EOF > > > This looks quite neat. Furthermore, I think we should also consider > the already existing tests in the t1300 that contain such indentation. > As I already explained in my earlier response to Eric, [1] the choice > of including the indentation or not seems random to me, so we should > perhaps consider taking some broader approach. > > How about this as a plan for moving forward: > > 1) Sprinkle a couple of tests onto the t1300, which try to be > focused on the verification of the indentation-handling logic; > maybe those additional tests could be even seen as redundant, > but I think they can only help with the test coverage > > 2) Create a new helper function that uses the logic you described > above, to make it simpler to include the indentation into configs > > 3) Finally, propagate the use of this new helper function into the > new test and the already existing tests in the t1300 that already > include the indentation > > I'd be happy to implement all of the above-proposed steps in the next > couple of days. Sure, it would be quite time-consuming, especially the > third proposed step, but it should be worth it in the long run. As noted in my other response, while such cleanups may be nice in the long-run, the bug-fix patch series under discussion should not be held hostage by an ever-increasing set of potential cleanups. Let's focus on landing the current series; these tangential cleanups can be done in a separate series.
Eric Sunshine <sunshine@sunshineco.com> writes: > As a reviewer, I'd like to see fewer and fewer changes between > each version of a patch series; the series should converge so that > it can land rather than diverge from iteration to iteration. Well said. Thanks.
On 2024-03-18 20:17, Eric Sunshine wrote: > On Mon, Mar 18, 2024 at 4:17 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-03-18 03:48, Eric Sunshine wrote: >> > Readability wasn't my reason for bringing this up. As a reviewer, >> > every time a question pops into my mind as I'm reading the code, that >> > indicates that something about the code is unclear or that the commit >> > message doesn't properly explain why it was done in this way. People >> > coming across this code in the future may have the same questions but >> > they won't have the benefit of being able to easily ask you why it was >> > done this way. >> >> I see. How about including a small comment in the t1300 that would >> explain the additional indentation? > > I'm just one reviewer. Unless others chime in with similar > observations or questions regarding the patch, I don't think such a > comment is necessary. Aside from the other more significant points > (such as not introducing x_to_tab(), using "setup" in the function > title, etc.), this is extremely minor, and what you have here is "good > enough" (though you may want to take Junio's suggestion of using a > leading "|" to protect indentation). Just to reiterate, both x_to_tab() and the test naming have already been addressed in the future v3 of this series. >> As a note, there are already more tests in the t1300 that contain such >> indentation, so maybe we shoulddo something with those existing tests >> as well; the above-proposed comment, which would be placed at the >> very >> beginning of t1300, may provide a satisfactory explanation for all the >> tests in t1300 that contain such additional indentation. >> >> Another option would be to either add the indentation to all relevant >> tests in the t1300, or to remove the indentation from all tests in the >> t1300 that already contain it. I'd be happy to implement and submit >> patches that do that, after we choose the direction we want to follow. > > It would be better to keep this series focused on its primary goal of > fixing a bug rather than being held hostage to an ever increasing set > of potential cleanups. Such cleanups can be done as separate patch > series either atop this series or alongside it. Let's land this series > first, and then, if you wish, tackle those other less significant > issues. Thanks, I totally agree. >> > If these new tests are also checking leading whitespace behavior, then >> > to improve coverage, would it make sense to have the leading "X" on >> > some lines but not others? >> >> Good point, despite that not being the main purpose of the added >> tests. >> I'll see to add a couple of tests that check the handling of >> indentation, >> possibly at some places in the t1300 that fit the best; improving the >> tests coverage can only help in the long run. > > As above, such additional tests probably aren't mandatory for this > bug-fix series. As a reviewer, I'd like to see fewer and fewer changes > between each version of a patch series; the series should converge so > that it can land rather than diverge from iteration to iteration. Such > additional leading-whitespace tests may be perfectly appropriate for a > follow-up series. Agreed once again. Let's wrap this up, and I'll come back with the follow-up patches.
On 2024-03-18 20:21, Eric Sunshine wrote: > On Mon, Mar 18, 2024 at 4:37 AM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-03-18 05:38, Junio C Hamano wrote: >> > sed -e 's/^|//' -e 's/Q/ /g' >.git/config <<-\EOF >> > |[section] >> > | solid = rock >> > | sparse = big QQ blue >> > | ... >> > EOF >> > >> This looks quite neat. Furthermore, I think we should also consider >> the already existing tests in the t1300 that contain such indentation. >> As I already explained in my earlier response to Eric, [1] the choice >> of including the indentation or not seems random to me, so we should >> perhaps consider taking some broader approach. >> >> How about this as a plan for moving forward: >> >> 1) Sprinkle a couple of tests onto the t1300, which try to be >> focused on the verification of the indentation-handling logic; >> maybe those additional tests could be even seen as redundant, >> but I think they can only help with the test coverage >> >> 2) Create a new helper function that uses the logic you described >> above, to make it simpler to include the indentation into configs >> >> 3) Finally, propagate the use of this new helper function into the >> new test and the already existing tests in the t1300 that already >> include the indentation >> >> I'd be happy to implement all of the above-proposed steps in the next >> couple of days. Sure, it would be quite time-consuming, especially >> the >> third proposed step, but it should be worth it in the long run. > > As noted in my other response, while such cleanups may be nice in the > long-run, the bug-fix patch series under discussion should not be held > hostage by an ever-increasing set of potential cleanups. Let's focus > on landing the current series; these tangential cleanups can be done > in a separate series. Totally agreed, let's keep this plan for the follow-up patches.
diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 31c387868708..37ed078721ea 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -11,7 +11,97 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh -test_expect_success 'clear default config' ' +test_expect_success 'create test configuration' ' + x_to_tab >.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 XX blue" | x_to_tab >expect && + git config --get section.sparse >actual && + test_cmp expect actual +' + +test_expect_success 'internal and trailing whitespace' ' + echo "big XX blue" | x_to_tab >expect && + git config --get section.sparseAndTail >actual && + test_cmp expect actual +' + +test_expect_success 'internal and trailing whitespace, all quoted' ' + echo "big XX blue " | x_to_tab >expect && + git config --get section.sparseAndTailQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'internal and more trailing whitespace' ' + echo "big XX blue" | x_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 XX blue X X" | x_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 XX blue X X" | x_to_tab >expect && + git config --get section.sparseAndBiggerTailQuotedPlus >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace' ' + echo "big blue" | x_to_tab >expect && + git config --get section.headAndTail >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace, all quoted' ' + echo "Xbig blue " | x_to_tab >expect && + git config --get section.headAndTailQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'leading and trailing whitespace, not all quoted' ' + echo "Xbig blue " | x_to_tab >expect && + git config --get section.headAndTailQuotedPlus >actual && + test_cmp expect actual +' + +test_expect_success 'inline comment' ' + echo "big blue" | x_to_tab >expect && + git config --get section.annotated >actual && + test_cmp expect actual +' + +test_expect_success 'inline comment, quoted' ' + echo "big blue" | x_to_tab >expect && + git config --get section.annotatedQuoted >actual && + test_cmp expect actual +' + +test_expect_success 'clear default configuration' ' 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 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/ t/t1300-config.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 110 insertions(+), 4 deletions(-)