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

Re: [PATCH v3 12/19] initial_ref_transaction_commit(): check for duplicate refs

From
Junio C Hamano <gitster@pobox.com>
Date
Jun 23, 2015, 17:44 UTC
Message-ID
<xmqqd20me2uq.fsf@gitster.dls.corp.google.com>
In-Reply-To
<558906A4.8060106@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 20 quoted lines
> On 06/22/2015 11:06 PM, Junio C Hamano wrote:
> ...
>> What I am wondering is if we could turn the safety logic that appear
>> here (i.e. no existing refs must be assumed by the set of updates,
>> etc.)  into an optimization cue and implement this as a special case
>> helper to ref_transaction_commit(), i.e.
>> 
>> 	ref_transaction_commit(...)
>>         {
>> 		if (updates are all initial creation &&
>>                     no existing refs in repository)
>> 			return initial_ref_transaction_commit(...);
>> 		/* otherwise we do the usual thing */
>> 		...
>> 	}
>> 
>> and have "clone" call ref_transaction_commit() as usual.
>
> The safety logic in this function is (approximately) necessary, but not
> sufficient, to guarantee safety.

Oh, no question about it, and you do not even have to bring up an insane "user runs random commands while Git is hard working on it" non use-case ;-)

Show 7 quoted lines
> One of the shortcuts that it takes is
> not locking the references while they are being created. Therefore, it
> would be unsafe for one process to call ref_transaction_commit() while
> another is calling initial_ref_transaction_commit(). So the caller has
> to "know" somehow that no other processes are working in the repository
> for this optimization to be safe. It conveys that knowledge by calling
> initial_ref_transaction_commit() rather than ref_transaction_commit().

OK. So the answer to my first question "is the initial creation logic too fragile" is a resounding "yes"; the caller should know that it is too crazy for the user to be competing with what it is doing before deciding to call initial_ref_transaction_commit(), hence we cannot automatically detect if it is safe from within ref_transaction_commit() to use this logic as an optimization.

Show 11 quoted lines
> But I think if anything it would make more sense to go the other direction:
>
> * Teach ref_transaction_commit() an option that asks it to write
>   references updates to packed-refs instead of loose refs (but
>   locking the references as usual).
>
> * Change clone to use ref_transaction_commit() like everybody
>   else, passing it the new REFS_WRITE_TO_PACKED_REFS option.
>
> Then clone would participate in the normal locking protocol, and it
> wouldn't *matter* if another process runs before the clone is finished.

Yeah, I thought that was actually I was driving at, and doing so without that write-to-packed-refs option, which I'd prefer to leave it as an optimization inside ref_transaction_commit().

Except that I missed that the initial_* variant is even more aggressive (i.e. not locking), so no such optimization is safe.

Show 9 quoted lines
> There would also be some consistency benefits. For example, if
> core.logallrefupdates is set globally or on the command line, the
> initial reference creations would be reflogged. And other operations
> that write references in bulk could use the new
> REFS_WRITE_TO_PACKED_REFS option to prevent loose reference proliferation.
>
> But I don't think any of this is a problem in practice, and I think we
> can live with using the optimized-but-not-100%-safe
> initial_ref_transaction_commit() for cloning.
OK.
Previous: Michael HaggertyNext: Michael Haggerty
Message 16 of 24 in “Improve "refs" encapsulation and speed up deletes”
  1. 00/19 Improve "refs" encapsulation and speed up deletesMichael Haggerty, Jun 22, 2015
  2. 01/19 delete_ref(): move declaration to refs.hMichael Haggerty, Jun 22, 2015
  3. 02/19 remove_branches(): remove temporaryMichael Haggerty, Jun 22, 2015
  4. 03/19 delete_ref(): handle special case more explicitlyMichael Haggerty, Jun 22, 2015
  5. 04/19 delete_refs(): new function for the refs APIMichael Haggerty, Jun 22, 2015
  6. 05/19 delete_refs(): make error message more genericMichael Haggerty, Jun 22, 2015
  7. 06/19 delete_refs(): bail early if the packed-refs file cannot be rewrittenMichael Haggerty, Jun 22, 2015
  8. 07/19 prune_remote(): use delete_refs()Michael Haggerty, Jun 22, 2015
  9. 08/19 prune_refs(): use delete_refs()Michael Haggerty, Jun 22, 2015
  10. 09/19 repack_without_refs(): make function privateMichael Haggerty, Jun 22, 2015
  11. 10/19 initial_ref_transaction_commit(): function for initial ref creationMichael Haggerty, Jun 22, 2015
  12. 11/19 refs: remove some functions from the module's public interfaceMichael Haggerty, Jun 22, 2015
  13. 12/19 initial_ref_transaction_commit(): check for duplicate refsMichael Haggerty, Jun 22, 2015
  14. Junio C HamanoJun 22, 2015
  15. Michael HaggertyJun 23, 2015
  16. Junio C HamanoJun 23, 2015
  17. 13/19 initial_ref_transaction_commit(): check for ref D/F conflictsMichael Haggerty, Jun 22, 2015
  18. 14/19 refs: move the remaining ref module declarations to refs.hMichael Haggerty, Jun 22, 2015
  19. 15/19 refs.h: add some parameter names to function declarationsMichael Haggerty, Jun 22, 2015
  20. 16/19 check_branch_commit(): make first parameter constMichael Haggerty, Jun 22, 2015
  21. 17/19 update_ref(): don't read old reference value before deleteMichael Haggerty, Jun 22, 2015
  22. 18/19 cmd_update_ref(): make logic more straightforwardMichael Haggerty, Jun 22, 2015
  23. 19/19 delete_ref(): use the usual convention for old_sha1Michael Haggerty, Jun 22, 2015
  24. Junio C HamanoJun 22, 2015

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.