Hi Peff, On Wed, 31 Aug 2022, Jeff King wrote: > On Wed, Aug 31, 2022 at 11:36:19AM -0400, Jeff King wrote: > > > > I'm struggling to understand the need for this change from the explanation > > > in the commit message as it seems to me to assume the line being deleted is > > > the hunk header when in fact it is deleting the diff header. > > > > FWIW, as the author of the original test, I'm also confused about why it > > needs to be changed. The filter I wrote in the original test was just > > "echo too-short". It was switched to "sed 1d" because the original did > > not read the input at all, which racily caused Git to see SIGPIPE: > > > > https://lore.kernel.org/git/20200113170417.GK32750@xxxxxxxxxx/ > > > > Switching to "exit 0" would bring that problem back. But I think "sed q" > > potentially does, too, because sed will quit without reading all of the > > input. We really do want something like "sed 1d" that changes the number > > of lines, but ensures that the filter reads to EOF. > > By the way, the test change is needed for it to pass with the change in > patch 2, where we become more lenient about parsing the hunk header. > That implies to me that the builtin version's check for one-to-one line > correspondence is broken, but we didn't notice. And that's exactly the case. It was an off-by-one bug ;-) > In the perl version, that test fails because it triggers the > mismatched-output check: > > $ GIT_TEST_ADD_I_USE_BUILTIN=false ./t3701-add-interactive.sh --verbose-only=56 > [...] > fatal: mismatched output from interactive.diffFilter > hint: Your filter must maintain a one-to-one correspondence > hint: between its input and output lines. > ok 56 - detect bogus diffFilter output > > but the builtin version complains about the hunk header: > > $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56 > [...] > error: could not parse colored hunk header '?[31m-10?[m' > ok 56 - detect bogus diffFilter output > > Once patch 2 is applied, we stop complaining there, and we _should_ > complain that the number of lines isn't the same. But we don't: > > $ GIT_TEST_ADD_I_USE_BUILTIN=true ./t3701-add-interactive.sh --verbose-only=56 > test_must_fail: command succeeded: git add -p > not ok 56 - detect bogus diffFilter output Yes, that matches my analysis, and I already pushed a new iteration before even noticing your mail, only waiting for the CI run to finish before I submit it. Ciao, Dscho
