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

Re: [PATCH v2 4/5] repack: use tempfiles for signal cleanup

From
Junio C Hamano <gitster@pobox.com>
Date
Oct 23, 2022, 00:14 UTC
Message-ID
<xmqqtu3vflau.fsf@gitster.g>
In-Reply-To
<Y1RUI8ny2mexxwKm@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> Ugh, I think this test is racy. I saw a failure via SIGPIPE on OS X in
> CI, and running it locally with --stress confirmed. I think the problem
> is in our method to trigger pack-objects to fail for the cruft pack. We
> pass a bogus command line argument, so pack-objects exits more or less
> immediately. But the parent git-repack process is trying to write to its
> stdin (to name packs to keep/exclude). So that write may succeed or fail
> based on how quickly the child dies.
Yeah, I saw flaky failures myself during my integration tests.
Show 18 quoted lines
> Some options:
>
>   - find a different way to convince repack to die. The most likely is
>     probably corrupting the cruft objects in some way such that
>     pack-objects dies, but not until it starts doing real work.
>
>   - mark the test_must_fail with ok=sigpipe. In most case this is a
>     band-aid, but here we still test what we want. The .tmp cleanup
>     should kick in from a die() or from a signal death, so either is
>     sufficient for our purposes.
>
>   - teach git-repack to ignore sigpipe. We don't actually check the
>     writes to pack-objects (since they're done by fprintf), but we'd
>     notice its failing exit code. And arguably this is improving the
>     user experience. Saying "pack-objects died with an error" is more
>     useful than a sigpipe.
>
> Thoughts?

I agree that for this particular one the second one is a reasonable thing to do in the context of the test. The third one may actually be a better fix, exactly for the reason you state here.

Thanks.
Previous: Jeff KingNext: Jeff King
Message 29 of 34 in “repack tempfile-cleanup signal deadlock”
  1. 0/4 repack tempfile-cleanup signal deadlockJeff King, Oct 21, 2022
  2. 1/4 repack: convert "names" util bitfield to arrayJeff King, Oct 21, 2022
  3. Junio C HamanoOct 21, 2022
  4. Taylor BlauOct 21, 2022
  5. Jeff KingOct 21, 2022
  6. Junio C HamanoOct 21, 2022
  7. Jeff KingOct 21, 2022
  8. Junio C HamanoOct 21, 2022
  9. Jeff KingOct 22, 2022
  10. 2/4 repack: populate extension bits incrementallyJeff King, Oct 21, 2022
  11. Taylor BlauOct 21, 2022
  12. Jeff KingOct 21, 2022
  13. Taylor BlauOct 21, 2022
  14. Jeff KingOct 21, 2022
  15. 3/4 repack: use tempfiles for signal cleanupJeff King, Oct 21, 2022
  16. Junio C HamanoOct 21, 2022
  17. Jeff KingOct 21, 2022
  18. Taylor BlauOct 21, 2022
  19. Jeff KingOct 22, 2022
  20. Jeff KingOct 22, 2022
  21. 4/4 repack: drop remove_temporary_files()Jeff King, Oct 21, 2022
  22. Taylor BlauOct 21, 2022
  23. 0/5 repack tempfile-cleanup signal deadlockJeff King, Oct 22, 2022
  24. 1/5 repack: convert "names" util bitfield to arrayJeff King, Oct 22, 2022
  25. 2/5 repack: populate extension bits incrementallyJeff King, Oct 22, 2022
  26. 3/5 repack: expand error message for missing pack filesJeff King, Oct 22, 2022
  27. 4/5 repack: use tempfiles for signal cleanupJeff King, Oct 22, 2022
  28. Jeff KingOct 22, 2022
  29. Junio C HamanoOct 23, 2022
  30. Jeff KingOct 23, 2022
  31. Junio C HamanoOct 23, 2022
  32. Jeff KingOct 23, 2022
  33. Junio C HamanoOct 23, 2022
  34. 5/5 repack: drop remove_temporary_files()Jeff King, Oct 22, 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.