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

Re: [PATCH v2 00/33] Yet more preparation for reference backends

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Jun 10, 2016, 12:50 UTC
Message-ID
<575AB797.809@alum.mit.edu>
In-Reply-To
<cover.1462550456.git.mhagger@alum.mit.edu>
On 05/06/2016 06:13 PM, Michael Haggerty wrote:
Show 6 quoted lines
> [...]
> This patch series is also available from my GitHub repo [2] as branch
> "split-under-lock".
> 
> [1] http://thread.gmane.org/gmane.comp.version-control.git/292772
> [2] https://github.com/mhagger/git

I was reading this area of the code again, and I found a problem in this patch series. Now that ref_updates can be added to ref_transactions while they are being processed, it is not correct to store a pointer to transaction->updates in ref_transaction_commit() here [1], because the array might be moved by realloc() if it grows during the function. The problem wasn't detected during testing because an added commit would have to cross an ALLOC_GROW boundary to trigger the bug.

The fix is obvious but it is textually quite a few lines. For good measure, the same fix should be made in initial_ref_transaction_commit() here [2].

The most logical place to fix this is by expanding commit "ref_transaction_commit(): remove local variable n" [3], so I've done that and force pushed the result to my GitHub account [4] as branch "split-under-lock".

Junio, if you want to incorporate this revised version of the branch in your big rewind of next, then we can pretend that the bug was never there :-) Otherwise, tell me in what form you would like the fix and I will be happy to provide it.

Sorry for finding this problem so late in the process.
Michael

[1] https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3559 [2] https://github.com/mhagger/git/blob/088c8f756c86581ff25371983ef409044b348bb9/refs/files-backend.c#L3725 [3] http://article.gmane.org/gmane.comp.version-control.git/293801 [4] https://github.com/mhagger/git

Previous: Junio C HamanoNext: Junio C Hamano
Message 49 of 51 in “Yet more preparation for reference backends”
  1. 00/33 Yet more preparation for reference backendsMichael Haggerty, May 6, 2016
  2. 01/33 t1404: demonstrate a bug resolving referencesMichael Haggerty, May 6, 2016
  3. 02/33 commit_ref(): if there is an empty dir in the way, delete itMichael Haggerty, May 6, 2016
  4. 03/33 read_raw_ref(): don't get confused by an empty directoryMichael Haggerty, May 6, 2016
  5. 04/33 safe_create_leading_directories(): improve docstringMichael Haggerty, May 6, 2016
  6. 05/33 remove_dir_recursively(): add docstringMichael Haggerty, May 6, 2016
  7. 06/33 refname_is_safe(): use skip_prefix()Michael Haggerty, May 6, 2016
  8. 07/33 refname_is_safe(): don't allow the empty stringMichael Haggerty, May 6, 2016
  9. 08/33 refname_is_safe(): insist that the refname already be normalizedMichael Haggerty, May 6, 2016
  10. 09/33 commit_ref_update(): write error message to *err, not stderrMichael Haggerty, May 6, 2016
  11. 10/33 rename_ref(): remove unneeded local variableMichael Haggerty, May 6, 2016
  12. 11/33 ref_transaction_commit(): remove local variable nMichael Haggerty, May 6, 2016
  13. 12/33 read_raw_ref(): rename flags argument to typeMichael Haggerty, May 6, 2016
  14. 13/33 read_raw_ref(): clear *type at start of functionMichael Haggerty, May 6, 2016
  15. 14/33 read_raw_ref(): rename symref argument to referentMichael Haggerty, May 6, 2016
  16. 15/33 read_raw_ref(): improve docstringMichael Haggerty, May 6, 2016
  17. 16/33 read_raw_ref(): move docstring to header fileMichael Haggerty, May 6, 2016
  18. 17/33 lock_ref_sha1_basic(): remove unneeded local variableMichael Haggerty, May 6, 2016
  19. 18/33 refs: make error messages more consistentMichael Haggerty, May 6, 2016
  20. 19/33 ref_transaction_create(): disallow recursive pruningMichael Haggerty, May 6, 2016
  21. 20/33 ref_transaction_commit(): correctly report close_ref() failureMichael Haggerty, May 6, 2016
  22. 21/33 delete_branches(): use resolve_refdup()Michael Haggerty, May 6, 2016
  23. 22/33 refs: allow log-only updatesMichael Haggerty, May 6, 2016
  24. 23/33 refs: don't dereference on renameMichael Haggerty, May 6, 2016
  25. 24/33 verify_refname_available(): adjust constness in declarationMichael Haggerty, May 6, 2016
  26. 25/33 add_update(): initialize the whole ref_updateMichael Haggerty, May 6, 2016
  27. 26/33 lock_ref_for_update(): new functionMichael Haggerty, May 6, 2016
  28. 27/33 unlock_ref(): move definition higher in the fileMichael Haggerty, May 6, 2016
  29. 28/33 ref_transaction_update(): check refname_is_safe() at a minimumMichael Haggerty, May 6, 2016
  30. 29/33 refs: resolve symbolic refs firstMichael Haggerty, May 6, 2016
  31. Jeff KingMay 12, 2016
  32. Jeff KingMay 12, 2016
  33. Michael HaggertyMay 13, 2016
  34. 29/33 refs: resolve symbolic refs firstMichael Haggerty, May 13, 2016
  35. Jeff KingMay 13, 2016
  36. Jeff KingMay 13, 2016
  37. Torsten BögershausenMay 14, 2016
  38. 30/33 lock_ref_for_update(): don't re-read non-symbolic referencesMichael Haggerty, May 6, 2016
  39. 31/33 lock_ref_for_update(): don't resolve symrefsMichael Haggerty, May 6, 2016
  40. 32/33 commit_ref_update(): remove the flags parameterMichael Haggerty, May 6, 2016
  41. 33/33 lock_ref_sha1_basic(): only handle REF_NODEREF modeMichael Haggerty, May 6, 2016
  42. David TurnerMay 9, 2016
  43. Junio C HamanoMay 9, 2016
  44. Michael HaggertyMay 9, 2016
  45. Junio C HamanoMay 9, 2016
  46. Jeff KingMay 12, 2016
  47. Junio C HamanoMay 12, 2016
  48. Junio C HamanoMay 10, 2016
  49. Michael HaggertyJun 10, 2016
  50. Junio C HamanoJun 10, 2016
  51. [ADDENDUM v4] Yet more preparation for reference backendsMichael Haggerty, Jun 13, 2016

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.