Re: [PATCH v4 17/29] tests: t3440: create expect files at point of use

[Phillip Wood <phillip.wood123@gmail.com>]

On 15/10/2025 14:58, Li Chen wrote:
> Hi Kristoffer,
> 
> Thanks for the review suggestions! I'll address them in the next version.
> 
>   ---- On Wed, 15 Oct 2025 04:41:33 +0800  Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com> wrote ---
>   > Now you start to change the test suite/file that you created for this
>   > series.  There shouldn’t be a need to do a test file-only patch/commit
>   > for a fresh series.
>   >
>   > I saw in one of your patches that you removed `--keep-empty` from a test
>   > because “that is the default”.  I also saw Phillip’s comment somewhere
>   > that said the same thing.
>   >
>   > The goal with maturing series is not to add patches on top in each round
>   > (if that’s what you are doing).  It is to recreate them as if the series
>   > was perfectly written to begin with; if one patch introduces
>   > `--trailers` and a test file, then there should be no need with
>   > follow-up patches that improve the test file style, refactors it, and
>   > so on.
> 
> Thanks for the tip. I split the changes into separate commits to ease review,
>   as Phillip suggested in https://lore.kernel.org/git/d4c9f082-52be-48d9-b817-fcb8a72e1bd7@gmail.com/.
> 
> It seems I may have overdone it? If so, I'll try for a better balance in the next version.
I asked that you did not refactor code at the same time as you moved it. 
I was expecting a handful of patches, not twenty-nine. The point that 
Kristoffer makes about this patch is perfectly valid - you add a new 
test and then correct it in a later patch. Instead you should correct 
the test where it is introduced as Kirstoffer suggested. Looking at the 
first patch in this series there seems to have been some 
miscommunication because it has exactly the same problem as V3. The code 
that is moved from builtin/interpret-trailers.c to trailer.c is heavily 
refactored at the same time. Variable names are changed and the code is 
rearranged so that "git diff --color-moved 
--color-moved-ws=ignore-indentation-change" detects barely any moved 
lines. I'll try and leave some more detailed feedback on the first few 
patches of V5 in the next few days.

Thanks

Phillip