Ævar Arnfjörð Bjarmason wrote: > Modernize test code added in ce567d1867a (Add test to show that > show-branch misses out the 8th column, 2008-07-23) and > 11ee57bc4c4 (sort_in_topological_order(): avoid setting a commit flag, > 2008-07-23) to use test helpers. > > I'm renaming "out" to "actual" for consistency with other tests, and > introducing a "branches.sorted" file in the setup, to make it clear > that it's important that the list be sorted in this particular way. That is better. > The "show-branch" output is indented with spaces, which would cause > complaints under "git show --check" with an indented here-doc > block. Let's prefix the lines with "> " to work around that, and to > make it clear that the leading whitespace is important. I'm not sure this is an improvement. To me the original code is just fine. Also, I don't think writing an 'expect' file belong in a setup step. Additionally I would do this change in a separate patch. > We can also get rid of the hardcoding of "main" added here in > 334afbc76fb (tests: mark tests relying on the current default for > `init.defaultBranch`, 2020-11-18). For this test we're setting up an > "initial" commit anyway, and now that we've moved over to test_commit > we can reference that instead. That's also good. All the changes in this patch look good to me, however, they are smashed together in a way that makes the review harder, I see: 1. Use test_commit 2. Rename out to actual 3. Use >actual instead of > actual 4. Use test_seq instead of $numbers 5. Use branches.sorted instead of branch$i 6. Use test_config instead of git config 7. Use internal sed 's/^ //' instead of outside cat I'm on-board with 6 out of 7, but if these were done it at least 2 patches, they would be clearer. I in fact would prefer one patch per change (although maybe squash 3 with 2). Cheers. -- Felipe Contreras
