git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v4 0/2] tests(mingw): avoid super-slow mingw_test_cmp

From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
Date
Nov 14, 2022, 14:06 UTC
Message-ID
<pull.1309.v4.git.1668434812.gitgitgadget@gmail.com>
In-Reply-To
<pull.1309.v3.git.1668290855.gitgitgadget@gmail.com>

A few months ago, directly after sending a patch to fix a performance regression due to a mis-use of test_cmp [https://lore.kernel.org/git/b9203ea247776332e4b6f519aa27d541207adc2f.1659097724.git.gitgitgadget@gmail.com/], I got curious to see whether Git for Windows had the same issue. And it did not: it passes t5351 in 22 seconds, even while using test_cmp to compare pack files [https://github.com/git-for-windows/git/blob/3922f62f0d5991e9fe0a0817ebf89a91339c7705/t/t5351-unpack-large-objects.sh#L90].

The explanation is of course that Git for Windows uses a test helper for test_cmp that is written in C, instead of the Bash function. And C is much faster than a Bash function, especially on Windows. This is especially sad when said Bash code is only used on Windows. So I originally had pulled out this helper from the years-long effort to let Git for Windows use BusyBox' ash to run the test suite. The result was a single-patch contribution of a change that had been in Git for Windows since June 2018. Unfortunately, this tried-and-tested code was rejected by the Git maintainer.

Let's fall back to the next-best solution: git diff --no-index, which the Git maintainer seems to like. The downside is that the diff machinery does a lot more than a simple cmp clone, and therefore a lot more things can go wrong that might make it look like a test case is failing when the fault is somewhere else entirely. There is one way to find out whether this is a valid concern.

Changes since v3:
 * Fixed the subject of the cover letter (which should have been adjusted in
   v3)
 * Elaborated the paragraph about the historical context of this patch
Changes since v2:
 * Dropped the test helper, using diff --no-index instead.
Changes since v1:
 * Fixed double "with" in the commit message.
 * Renamed the test helper to text-cmp.
 * Made the diff --no-index call more robust by using a double-dash
   separator.
Johannes Schindelin (2):
  t0021: use Windows-friendly `pwd`
  tests(mingw): avoid very slow `mingw_test_cmp`
 t/t0021-conversion.sh   |  4 +--
 t/test-lib-functions.sh | 66 -----------------------------------------
 t/test-lib.sh           |  2 +-
 3 files changed, 3 insertions(+), 69 deletions(-)
base-commit: 23b219f8e3f2adfb0441e135f0a880e6124f766c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1309%2Fdscho%2Fmingw-test-cmp-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1309/dscho/mingw-test-cmp-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1309
Range-diff vs v3:
 1:  b38b8fb5a85 = 1:  b38b8fb5a85 t0021: use Windows-friendly `pwd`
 2:  a7f4265ceb2 ! 2:  128b1f348d8 tests(mingw): avoid very slow `mingw_test_cmp`
     @@ Commit message
          `mingw_test_cmp` code with MSYS2's Bash, i.e. the Bash that Git for
          Windows uses. And a lot more readable.
      
     -    Note: Earlier attempts at fixing this involved a test helper that avoids
     -    the overhead of the diff machinery, in favor of implementing a behavior
     -    that is more in line with what `mingw_test_cmp` does now, but that
     -    attempt saw a lot of backlash and distractions during review and was
     -    therefore abandoned.
     +    The original reason why Git's test suite needs the `mingw_test_cmp`
     +    function at all (and why `cmp` is not good enough) is that Git's test
     +    suite is not actually trying to compare binary files when it calls
     +    `test_cmp`, but it compares text files. And those text files can contain
     +    CR/LF line endings depending on the circumstances.
     +
     +    Note: The original fix in the Git for Windows project implemented a test
     +    helper that avoids the overhead of the diff machinery, in favor of
     +    implementing a behavior that is more in line with what `mingw_test_cmp`
     +    does now. This was done to minimize the risk in using something as
     +    complex as the diff machinery to perform something as simple as
     +    determining whether text output is identical to the expected output or
     +    not. This approach has served Git for Windows well for years, but the
     +    attempt to upstream this saw a lot of backlash and distractions during
     +    the review, was disliked by the Git maintainer and was therefore
     +    abandoned. For full details, see the thread at
     +    https://lore.kernel.org/git/pull.1309.git.1659106382128.gitgitgadget@gmail.com/t
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
-- 
gitgitgadget
Previous: Junio C HamanoNext: Johannes Schindelin via GitGitGadget
Message 43 of 58 in “tests: replace mingw_test_cmp with a helper in C”
  1. tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Jul 29, 2022
  2. Johannes SchindelinJul 29, 2022
  3. Junio C HamanoJul 29, 2022
  4. Johannes SchindelinSep 6, 2022
  5. René ScharfeSep 7, 2022
  6. Junio C HamanoSep 7, 2022
  7. Re* [PATCH] tests: replace mingw_test_cmp with a helper in CJunio C Hamano, Sep 7, 2022
  8. René ScharfeSep 7, 2022
  9. Junio C HamanoSep 8, 2022
  10. René ScharfeSep 8, 2022
  11. Ævar Arnfjörð BjarmasonSep 8, 2022
  12. Johannes SchindelinSep 8, 2022
  13. Junio C HamanoSep 8, 2022
  14. 0/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Sep 6, 2022
  15. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Sep 6, 2022
  16. 2/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Sep 6, 2022
  17. Ævar Arnfjörð BjarmasonSep 7, 2022
  18. Ævar Arnfjörð BjarmasonSep 7, 2022
  19. Junio C HamanoSep 7, 2022
  20. Johannes SchindelinSep 7, 2022
  21. 0/2 tests: replace mingw_test_cmp with a helper in CJohannes Schindelin via GitGitGadget, Nov 12, 2022
  22. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Nov 12, 2022
  23. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Nov 12, 2022
  24. Taylor BlauNov 13, 2022
  25. Johannes SchindelinNov 14, 2022
  26. Junio C HamanoNov 18, 2022
  27. Taylor BlauNov 19, 2022
  28. Ævar Arnfjörð BjarmasonNov 19, 2022
  29. Johannes SixtNov 19, 2022
  30. René ScharfeNov 19, 2022
  31. Torsten BögershausenNov 20, 2022
  32. Johannes SixtNov 21, 2022
  33. Junio C HamanoNov 21, 2022
  34. Phillip WoodNov 14, 2022
  35. Johannes SchindelinNov 14, 2022
  36. Ævar Arnfjörð BjarmasonNov 14, 2022
  37. Johannes SchindelinNov 14, 2022
  38. Ævar Arnfjörð BjarmasonNov 14, 2022
  39. Junio C HamanoNov 18, 2022
  40. Taylor BlauNov 19, 2022
  41. Ævar Arnfjörð BjarmasonNov 19, 2022
  42. Junio C HamanoNov 21, 2022
  43. 0/2 tests(mingw): avoid super-slow mingw_test_cmpJohannes Schindelin via GitGitGadget, Nov 14, 2022
  44. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Nov 14, 2022
  45. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Nov 14, 2022
  46. Taylor BlauNov 14, 2022
  47. Johannes SchindelinNov 18, 2022
  48. Taylor BlauNov 18, 2022
  49. Johannes SchindelinNov 20, 2022
  50. Taylor BlauNov 21, 2022
  51. 0/2 tests(mingw): avoid super-slow mingw_test_cmpJohannes Schindelin via GitGitGadget, Dec 6, 2022
  52. 2/2 tests(mingw): avoid very slow `mingw_test_cmp`Johannes Schindelin via GitGitGadget, Dec 6, 2022
  53. Ævar Arnfjörð BjarmasonDec 6, 2022
  54. Johannes SixtDec 6, 2022
  55. René ScharfeDec 6, 2022
  56. Junio C HamanoDec 7, 2022
  57. Taylor BlauDec 7, 2022
  58. 1/2 t0021: use Windows-friendly `pwd`Johannes Schindelin via GitGitGadget, Dec 6, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.