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

[PATCH v2 0/3] refs: small followups to the migration corruption fix

From
Karthik Nayak <karthik.188@gmail.com>
Date
Jan 21, 2025, 03:34 UTC
Message-ID
<20250121-461-corrupted-reftable-followup-v2-0-37e26c7a79b4@gmail.com>
In-Reply-To
<20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com>

This is a follow up to the bug that was reported [1] around `git refs migrate --ref-format=reftable` where the migration would fail for repositories with reflogs with lots of entries. This was caused due to a mismatch in the reftable's header and footer, specifically WRT the 'max_update_index'.

While there was a fix posted. This series is a small followup to fix
some of the topics discussed there:
1. To mark `ref_transaction_update_reflog()` as static since it is only
used internally within 'refs.c'.
2. To change the type of 'max_index' from 'unsigned int' to 'uint64_t'.
This would be much safer for large repositories with millions of files
and on 32bit systems.
3. To add a safeguard to prevent 'update_index' changes post any record
addition. This is a preventive measure to ensure such bugs don't arise
in the future.

This is based on top of master 757161efcc (Sync with Git 2.48.1, 2025-01-13) with 'kn/reflog-migration-fix' merged in.

[1]: https://lore.kernel.org/r/Z4UbkcmJAU1MT-Rs@tapette.crustytoothpaste.net
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Changes in v2:
- Commit 1: Keep the function comments.
- Commit 3: Modify the requirement to be for any record writes instead
of the first block write. This ensures that the limits need to be set 
before any records being added. Instead of using `BUG()`, return 
`REFTABLE_API_ERROR`. Handle all callers as needed.
- Link to v1: https://lore.kernel.org/r/20250117-461-corrupted-reftable-followup-v1-0-70ee605ae3fe@gmail.com
---
Karthik Nayak (3):
      refs: mark `ref_transaction_update_reflog()` as static
      refs: use 'uint64_t' for 'ref_update.index'
      reftable: prevent 'update_index' changes after adding records
 refs.c                          | 24 ++++++++++++++++--------
 refs.h                          | 14 --------------
 refs/refs-internal.h            |  4 ++--
 refs/reftable-backend.c         | 22 ++++++++++++++++------
 reftable/reftable-error.h       |  1 +
 reftable/reftable-writer.h      | 24 ++++++++++++++----------
 reftable/stack.c                |  6 ++++--
 reftable/writer.c               | 13 +++++++++++--
 t/unit-tests/t-reftable-stack.c |  8 +++++---
 9 files changed, 69 insertions(+), 47 deletions(-)
---
Range-diff versus v1:
1:  82a4f950dd ! 1:  5821ddfbe2 refs: mark `ref_transaction_update_reflog()` as static
    @@ refs.c: int ref_transaction_update(struct ref_transaction *transaction,
     -				  const char *committer_info, unsigned int flags,
     -				  const char *msg, unsigned int index,
     -				  struct strbuf *err)
    ++/*
    ++ * Similar to`ref_transaction_update`, but this function is only for adding
    ++ * a reflog update. Supports providing custom committer information. The index
    ++ * field can be utiltized to order updates as desired. When not used, the
    ++ * updates default to being ordered by refname.
    ++ */
     +static int ref_transaction_update_reflog(struct ref_transaction *transaction,
     +					 const char *refname,
     +					 const struct object_id *new_oid,
2:  51ed04e95d = 2:  8025cfadf2 refs: use 'uint64_t' for 'ref_update.index'
3:  34e3001e29 < -:  ---------- reftable: prevent 'update_index' changes after header write
-:  ---------- > 3:  4b0416ce20 reftable: prevent 'update_index' changes after adding records
---

base-commit: a5aa44e7930761cb900813d971b4105f901818fb change-id: 20250117-461-corrupted-reftable-followup-eb0e4fd1a723

Thanks
- Karthik
Previous: Karthik NayakNext: Karthik Nayak
Message 10 of 30 in “refs: small followups to the migration corruption fix”
  1. 0/3 refs: small followups to the migration corruption fixKarthik Nayak, Jan 17, 2025
  2. 1/3 refs: mark `ref_transaction_update_reflog()` as staticKarthik Nayak, Jan 17, 2025
  3. Patrick SteinhardtJan 17, 2025
  4. Karthik NayakJan 20, 2025
  5. 2/3 refs: use 'uint64_t' for 'ref_update.index'Karthik Nayak, Jan 17, 2025
  6. 3/3 reftable: prevent 'update_index' changes after header writeKarthik Nayak, Jan 17, 2025
  7. Patrick SteinhardtJan 17, 2025
  8. Karthik NayakJan 20, 2025
  9. Karthik NayakJan 20, 2025
  10. 0/3 refs: small followups to the migration corruption fixKarthik Nayak, Jan 21, 2025
  11. 1/3 refs: mark `ref_transaction_update_reflog()` as staticKarthik Nayak, Jan 21, 2025
  12. 2/3 refs: use 'uint64_t' for 'ref_update.index'Karthik Nayak, Jan 21, 2025
  13. 3/3 reftable: prevent 'update_index' changes after adding recordsKarthik Nayak, Jan 21, 2025
  14. Patrick SteinhardtJan 21, 2025
  15. Karthik NayakJan 21, 2025
  16. 0/3 refs: small followups to the migration corruption fixKarthik Nayak, Jan 22, 2025
  17. 1/3 refs: mark `ref_transaction_update_reflog()` as staticKarthik Nayak, Jan 22, 2025
  18. 2/3 refs: use 'uint64_t' for 'ref_update.index'Karthik Nayak, Jan 22, 2025
  19. 3/3 reftable: prevent 'update_index' changes after adding recordsKarthik Nayak, Jan 22, 2025
  20. Patrick SteinhardtJan 22, 2025
  21. Junio C HamanoJan 22, 2025
  22. Junio C HamanoJan 22, 2025
  23. undefined behavior in unit tests, was Re: [PATCH v3 3/3] reftable: prevent 'update_index' changes after adding recordsJeff King, Feb 1, 2025
  24. Phillip WoodFeb 1, 2025
  25. Patrick SteinhardtFeb 3, 2025
  26. Junio C HamanoFeb 3, 2025
  27. Jeff KingFeb 3, 2025
  28. Patrick SteinhardtFeb 3, 2025
  29. Karthik NayakFeb 3, 2025
  30. Jeff KingFeb 3, 2025

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.