RE: [PATCH v2 2/4] show-branch tests: modernize test code

From: Felipe Contreras <felipe.contreras@gmail.com>
Date: Jun 17, 2021, 21:29 UTC
Message-ID: <60cbbe9d89cbd_1299208fa@natae.notmuch>
In-Reply-To: <patch-2.4-27f94abaed-20210617T105245Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason wrote:

Show 8 quoted lines

> 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.

Show 5 quoted lines

> 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