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

Re: [PATCH v2 17/25] commit_lock_file(): make committing an unlocked lockfile a NOP

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Apr 14, 2014, 08:21 UTC
Message-ID
<534B9A88.1000101@alum.mit.edu>
In-Reply-To
<20140407193131.GC19342@sigill.intra.peff.net>
On 04/07/2014 09:31 PM, Jeff King wrote:
Show 18 quoted lines
> On Mon, Apr 07, 2014 at 01:33:59AM +0200, Michael Haggerty wrote:
> 
>> It was previously a bug to call commit_lock_file() with a lock_file
>> object that was not active (an illegal access would happen within the
>> function).  It was presumably never done, but this would be an easy
>> programming error to overlook.  So guard the file-renaming code with
>> an if statement to change committing an unlocked file into a NOP.
>> [...]
> 
> Yeah, I would have expected a die("BUG") here.
> 
> I think it is worth making it a fatal mistake and catching it. Rolling
> back an uninitialized lockfile is probably OK; we are canceling an
> operation that never started. But committing a lockfile that we didn't
> actually fill out could be a sign of a serious error, and we may be
> propagating a bogus success code. E.g., imagine that receive-pack claims
> to have written your ref, but actually commit_lock_file was a silent
> NOP. I'd much rather have it die loudly so we can track down the case.
OK, I will change this to a fatal error in v3.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Jeff KingNext: Michael Haggerty
Message 23 of 37 in “Lockfile correctness and refactoring”
  1. 00/25 Lockfile correctness and refactoringMichael Haggerty, Apr 6, 2014
  2. 01/25 api-lockfile: expand the documentationMichael Haggerty, Apr 6, 2014
  3. Jeff KingApr 7, 2014
  4. 02/25 unable_to_lock_die(): rename function from unable_to_lock_index_die()Michael Haggerty, Apr 6, 2014
  5. Jeff KingApr 7, 2014
  6. Michael HaggertyApr 8, 2014
  7. 03/25 rollback_lock_file(): do not clear filename redundantlyMichael Haggerty, Apr 6, 2014
  8. 04/25 rollback_lock_file(): set fd to -1Michael Haggerty, Apr 6, 2014
  9. 05/25 lockfile: unlock file if lockfile permissions cannot be adjustedMichael Haggerty, Apr 6, 2014
  10. 06/25 hold_lock_file_for_append(): release lock on errorsMichael Haggerty, Apr 6, 2014
  11. 07/25 lock_file(): always add lock_file object to lock_file_listMichael Haggerty, Apr 6, 2014
  12. 08/25 struct lock_file: replace on_list field with flags fieldMichael Haggerty, Apr 6, 2014
  13. 09/25 lockfile.c: document the various states of lock_file objectsMichael Haggerty, Apr 6, 2014
  14. 10/25 lockfile: define a constant LOCK_SUFFIX_LENMichael Haggerty, Apr 6, 2014
  15. 11/25 delete_ref_loose(): don't muck around in the lock_file's filenameMichael Haggerty, Apr 6, 2014
  16. 12/25 prepare_index(): declare return value to be (const char *)Michael Haggerty, Apr 6, 2014
  17. 13/25 write_packed_entry_fn(): convert cb_data into a (const int *)Michael Haggerty, Apr 6, 2014
  18. 14/25 lock_file(): exit early if lockfile cannot be openedMichael Haggerty, Apr 6, 2014
  19. 15/25 remove_lock_file(): call rollback_lock_file()Michael Haggerty, Apr 6, 2014
  20. 16/25 commit_lock_file(): inline temporary variableMichael Haggerty, Apr 6, 2014
  21. 17/25 commit_lock_file(): make committing an unlocked lockfile a NOPMichael Haggerty, Apr 6, 2014
  22. Jeff KingApr 7, 2014
  23. Michael HaggertyApr 14, 2014
  24. 18/25 lockfile: avoid transitory invalid statesMichael Haggerty, Apr 6, 2014
  25. Johannes SixtApr 7, 2014
  26. Michael HaggertyApr 7, 2014
  27. Johannes SixtApr 7, 2014
  28. Michael HaggertyApr 7, 2014
  29. Jeff KingApr 7, 2014
  30. 19/25 try_merge_strategy(): remove redundant lock_file allocationMichael Haggerty, Apr 6, 2014
  31. 20/25 try_merge_strategy(): use a statically-allocated lock_file objectMichael Haggerty, Apr 6, 2014
  32. 21/25 commit_lock_file(): use a strbuf to manage temporary spaceMichael Haggerty, Apr 6, 2014
  33. 22/25 Change lock_file::filename into a strbufMichael Haggerty, Apr 6, 2014
  34. 23/25 resolve_symlink(): use a strbuf for internal scratch spaceMichael Haggerty, Apr 6, 2014
  35. 24/25 resolve_symlink(): take a strbuf parameterMichael Haggerty, Apr 6, 2014
  36. 25/25 trim_last_path_elm(): replace last_path_elm()Michael Haggerty, Apr 6, 2014
  37. Jeff KingApr 7, 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.