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