Re: [PATCH v4 17/29] tests: t3440: create expect files at point of use
From: Li Chen <hidden>
Date: 2025-10-28 10:26:47
Hi Phillip, ---- On Thu, 23 Oct 2025 17:04:34 +0800 Phillip Wood [off-list ref] wrote --- > 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 [off-list ref] 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/ (local). > > > > 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. I mistakenly misunderstood that you meant changes between each patchset version should be reflected by adding new patches. Now I understand that you mean refactoring the original code needs to be reflected in new patches for review. Thank you very Thank you for telling me about --color-moved, and I found that git log also has this parameter. This option is very amazing. I will do as you requested in the next version. I sincerely apologize for the misunderstanding and wasted time. Regards, Li