Derrick Stolee <stolee@xxxxxxxxx> writes: >>> + grep "The sparse index is expanding to a full index" err && >>> + >>> + git -C sparse-index sparse-checkout disable 2>err && >>> + test_line_count = 0 err >> >> I am not a huge fun of insisting that other code in the code path in >> which I got rid of an unwanted error message must stay silent. > ... > I would say that there are generally two reasons why I chose to check > that 'err' was empty here: > > 1. Using "! grep" feels flaky to me. If we changed the error message, > then we could accidentally cause the test to pass because we don't > see the old message. This is somewhat mitigated by having the same > test check for the existence of the message, so careful use of a > common variable might avoid this potential future. Yup. Duplicating is probably OK in practice as the eyes of the developer who changed the message will be pulled to this test when they notice the failure from the positive "grep" to notice the negated version you use to replace "The err file must be absolutely empty", but I agree that a common variable would be even safer. > + msg="The sparse index is expanding to a full index" && > - grep "The sparse index is expanding to a full index" err > + grep "$msg" err && > + > + git -C sparse-index sparse-checkout disable 2>err && > + ! grep "$msg" err Excellent. "test_grep" and "test_grep !" are better choices, though. > 2. If the output is currently empty, then testing that it stays empty > will be a more rigid test. It will help us notice a change of > behavior here, even if it is an intentional change. Such a stricter position cuts both ways. If we assume infinite engineering resource availablility on ongoing basis, yes, it may lead to a good discipline. But having millions of such tests that will _notice_ changes that are irrelevant to the thing the test is about (e.g., this thing is about sparse index expansion advice), we'd be setting ourselves to adjust numerous tests whose primary purpose has nothing to do with what we are changing. The choice also largely depends more on preference and taste that do not have one absolute and universal answer. I would personally prefer avoiding overly specific tests, but I also find it attractive if we can afford to be more specific in tests at times. > For the progress motivation, I'm not too worried because the progress > indicators depend on isatty(2)[*], so would not appear here even if the > command was slow or somehow GIT_PROGRESS_DELAY=0 was set. I already said that "progress" was a mere example, didn't I? Even if we expect we will not ever see unwanted progress indicators in this code, the general point still stands (iow, progress and "unsparsifying warning" are not the only things that output to the standard error stream). > [*] #leftoverbits: 'git sparse-checkout' commands do not have --progress > options, but could. The 'unpack_trees_options' structs have a member > called 'show_progress' that could be populated based on a user option. Nice. Thanks.
