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

[PATCH 3/2] t5309: mark delta-cycle failover tests as passing

From
Jeff King <peff@peff.net>
Date
Aug 30, 2014, 13:23 UTC
Message-ID
<20140830132311.GA14709@peff.net>
In-Reply-To
<20140829205538.GD29456@peff.net>

t5309 checks that we detect and fail when there is a cycle of deltas (i.e., A is a delta on B, which is a delta on A). It also checks the cases where we have such a cycle, but we have an extra copy of the object either in the same pack or in another one. These cases are recoverable, because we can use the alternate copy to reconstruct the delta object.

However, it did not work in practice (and the tests were marked as expect_failure) because it led to us trying to resolve some deltas multiple times, something we assert() should not happen. E.g., we'd see the full base A, use it to resolve delta B on top of A, then use that to resolve the delta A on top of B, and then finally try resolving B on top of A again.

Since the previous commit, we handle duplicate bases more gracefully, and the tests pass. While we're here, let's also make the tests a little more robust:

  1. For the case of finding the extra base in the same
     pack, we do not need to use --fix-thin; it should (and
     does) work without it.
  2. Confirm that the resulting pack has a duplicate object.
     In the case of using --fix-thin to pull the object from
     another pack, the duplicate is added to the pack by us
     (this is a good thing, as otherwise future readers
     would encounter the cycle again).
Signed-off-by: Jeff King <peff@peff.net>
---
An obvious follow-on to the other two patches.

The implications of this make me slightly nervous, though. In the --fix-thin case, the resulting pack will have 3 objects:

  - A as a delta on B
  - B as a delta on A
  - a full copy of either A (or B) provided by --fix-thin

We create a .idx that has duplicate entries for A. If a reader is trying to reconstruct B and they find the full copy of A, they're fine. If they find the delta copy, what happens?

Ideally the reader would say "hey, I can't reconstruct A here, let me try to find another copy". But I am not sure if that happens, or if we are even capable of finding another copy of A (certainly we can find one in another pack, but I do not think we are smart enough to find a duplicate in the same pack).

By definition, we have a copy in another pack here (that is where the --fix-thin base came from), but there is nothing to guarantee that the other pack lives on.

 t/t5309-pack-delta-cycles.sh | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index 3e7861b..5309095 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -56,13 +56,14 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
 	test_must_fail git index-pack --fix-thin --stdin <cycle.pack
 '
 
-test_expect_failure 'failover to an object in another pack' '
+test_expect_success 'failover to an object in another pack' '
 	clear_packs &&
 	git index-pack --stdin <ab.pack &&
-	git index-pack --stdin --fix-thin <cycle.pack
+	git index-pack --stdin --fix-thin <cycle.pack &&
+	test_must_fail git index-pack --strict --stdin --fix-thin <cycle.pack
 '
 
-test_expect_failure 'failover to a duplicate object in the same pack' '
+test_expect_success 'failover to a duplicate object in the same pack' '
 	clear_packs &&
 	{
 		pack_header 3 &&
@@ -71,7 +72,8 @@ test_expect_failure 'failover to a duplicate object in the same pack' '
 		pack_obj $A
 	} >recoverable.pack &&
 	pack_trailer recoverable.pack &&
-	git index-pack --fix-thin --stdin <recoverable.pack
+	git index-pack --stdin <recoverable.pack &&
+	test_must_fail git index-pack --strict --stdin <recoverable.pack
 '
 
 test_done
-- 
2.1.0.346.ga0367b9
Previous: Junio C HamanoNext: Jeff King
Message 27 of 30 in “[BUG] resolved deltas”
  1. Petr StodulkaAug 21, 2014
  2. Petr StodulkaAug 21, 2014
  3. Martin von GagernAug 22, 2014
  4. René ScharfeAug 23, 2014
  5. Jeff KingAug 23, 2014
  6. Jeff KingAug 23, 2014
  7. Jeff KingAug 23, 2014
  8. René ScharfeAug 25, 2014
  9. Jeff KingAug 28, 2014
  10. Jeff KingAug 28, 2014
  11. Jeff KingAug 28, 2014
  12. Jeff KingAug 28, 2014
  13. Junio C HamanoAug 28, 2014
  14. Jeff KingAug 29, 2014
  15. 1/2 index-pack: fix race condition with duplicate basesJeff King, Aug 29, 2014
  16. 2/2 index-pack: handle duplicate base objects gracefullyJeff King, Aug 29, 2014
  17. Junio C HamanoAug 29, 2014
  18. Jeff KingAug 29, 2014
  19. Shawn PearceAug 30, 2014
  20. Jeff KingAug 30, 2014
  21. René ScharfeAug 30, 2014
  22. Jeff KingAug 31, 2014
  23. René ScharfeAug 31, 2014
  24. Shawn PearceAug 31, 2014
  25. Jeff KingAug 31, 2014
  26. Junio C HamanoAug 31, 2014
  27. 3/2 t5309: mark delta-cycle failover tests as passingJeff King, Aug 30, 2014
  28. Jeff KingAug 31, 2014
  29. Junio C HamanoSep 2, 2014
  30. Shawn PearceAug 25, 2014

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.