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

Re: [PATCH/RFC 6/7] refs: add update_refs for multiple simultaneous updates

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 29, 2013, 17:39 UTC
Message-ID
<xmqqhae85rbl.fsf@gitster.dls.corp.google.com>
In-Reply-To
<518ba77096664a679e4c7212e4cc4d496c6b38d3.1377784597.git.brad.king@kitware.com>
Brad King <brad.king@kitware.com> writes:
Show 47 quoted lines
> Add 'struct ref_update' to encode the information needed to update or
> delete a ref (name, new sha1, optional old sha1, no-deref flag).  Add
> function 'update_refs' accepting an array of updates to perform.  First
> acquire locks on all refs with verified old values.  Then update or
> delete all refs accordingly.  Fail if any one lock cannot be obtained or
> any one old value does not match.
>
> Though the refs themeselves cannot be modified together in a single
> atomic transaction, this function does enable some useful semantics.
> For example, a caller may create a new branch starting from the head of
> another branch and rewind the original branch at the same time.  This
> transfers ownership of commits between branches without risk of losing
> commits added to the original branch by a concurrent process, or risk of
> a concurrent process creating the new branch first.
>
> Signed-off-by: Brad King <brad.king@kitware.com>
> ---
>  refs.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  refs.h |   11 +++++++++++
>  2 files changed, 77 insertions(+)
>
> diff --git a/refs.c b/refs.c
> index 5a6c14e..0a0c19e 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -3238,6 +3238,72 @@ int update_ref(const char *action, const char *refname,
>  	return update_ref_write(action, refname, sha1, lock, onerr);
>  }
>  
> +int update_refs(const char *action, struct ref_update *updates,
> +		int n, enum action_on_err onerr)
> +{
> +	int ret = 0, delnum = 0, i;
> +	int *types;
> +	struct ref_lock **locks;
> +	const char **delnames;
> +
> +	if (!updates || !n)
> +		return 0;
> +
> +	/* Allocate work space: */
> +	types = xmalloc(sizeof(int)*n);
> +	locks = xmalloc(sizeof(struct ref_lock*)*n);
> +	delnames = xmalloc(sizeof(const char*)*n);
> +
> +	/* Acquire all locks while verifying old values: */
> +	for (i=0; i < n; ++i) {
Style:
	for (i = 0; i < n; i++) {
Show 7 quoted lines
> +		locks[i] = update_ref_lock(updates[i].ref_name,
> +					   updates[i].old_sha1,
> +					   updates[i].flags,
> +					   &types[i], onerr);
> +		if (!locks[i])
> +			break;
> +	}

Is it asking for AB-BA deadlock? If so, is the caller responsible for avoiding it?

Show 55 quoted lines
> +	/* Abort if we did not get all locks: */
> +	if (i < n) {
> +		while (--i >= 0)
> +			unlock_ref(locks[i]);
> +		free(types);
> +		free(locks);
> +		free(delnames);
> +		return 1;
> +	}
> +
> +	/* Perform updates first so live commits remain referenced: */
> +	for (i=0; i < n; ++i)
> +		if (!is_null_sha1(updates[i].new_sha1)) {
> +			ret |= update_ref_write(action,
> +						updates[i].ref_name,
> +						updates[i].new_sha1,
> +						locks[i], onerr);
> +			locks[i] = 0; /* freed by update_ref_write */
> +		}
> +
> +	/* Perform deletes now that updates are safely completed: */
> +	for (i=0; i < n; ++i)
> +		if (locks[i]) {
> +			delnames[delnum++] = locks[i]->ref_name;
> +			ret |= delete_ref_loose(locks[i], types[i]);
> +		}
> +	ret |= repack_without_refs(delnames, delnum);
> +	for (i=0; i < delnum; ++i)
> +		unlink_or_warn(git_path("logs/%s", delnames[i]));
> +	clear_loose_ref_cache(&ref_cache);
> +	for (i=0; i < n; ++i)
> +		if (locks[i])
> +			unlock_ref(locks[i]);
> +
> +	free(types);
> +	free(locks);
> +	free(delnames);
> +	return ret;
> +}
> +
>  struct ref *find_ref_by_name(const struct ref *list, const char *name)
>  {
>  	for ( ; list; list = list->next)
> diff --git a/refs.h b/refs.h
> index 2cd307a..5763f3a 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -214,6 +214,17 @@ int update_ref(const char *action, const char *refname,
>  		const unsigned char *sha1, const unsigned char *oldval,
>  		int flags, enum action_on_err onerr);
>  
> +struct ref_update {
> +	const char *ref_name;
> +	unsigned char new_sha1[20];
> +	unsigned char *old_sha1;

This asymmetry is rather curious and will later become problematic when we have more users of this API. Maybe your callers happen have static storage to keep old_sha1[] outside this structure but do not have it for new_sha1[] for some unknown reason, which may not apply to later callers we may want to add.

Show 9 quoted lines
> +	int flags;
> +};
> +
> +/** lock all refs and then write all of them */
> +int update_refs(const char *action, struct ref_update *updates,
> +		int n, enum action_on_err onerr);
> +
>  extern int parse_hide_refs_config(const char *var, const char *value, const char *);
>  extern int ref_is_hidden(const char *);
Previous: Brad KingNext: Brad King
Message 16 of 106 in “Multiple simultaneously locked ref updates”
  1. 0/7 Multiple simultaneously locked ref updatesBrad King, Aug 29, 2013
  2. 1/7 reset: rename update_refs to reset_refsBrad King, Aug 29, 2013
  3. Junio C HamanoAug 29, 2013
  4. Brad KingAug 29, 2013
  5. 2/7 refs: report ref type from lock_any_ref_for_updateBrad King, Aug 29, 2013
  6. Junio C HamanoAug 29, 2013
  7. Brad KingAug 29, 2013
  8. 3/7 refs: factor update_ref steps into helpersBrad King, Aug 29, 2013
  9. 4/7 refs: factor delete_ref loose ref step into a helperBrad King, Aug 29, 2013
  10. Junio C HamanoAug 29, 2013
  11. Brad KingAug 29, 2013
  12. 5/7 refs: add function to repack without multiple refsBrad King, Aug 29, 2013
  13. Junio C HamanoAug 29, 2013
  14. Brad KingAug 29, 2013
  15. 6/7 refs: add update_refs for multiple simultaneous updatesBrad King, Aug 29, 2013
  16. Junio C HamanoAug 29, 2013
  17. Brad KingAug 29, 2013
  18. Junio C HamanoAug 29, 2013
  19. Brad KingAug 29, 2013
  20. Brad KingAug 29, 2013
  21. 7/7 update-ref: support multiple simultaneous updatesBrad King, Aug 29, 2013
  22. Junio C HamanoAug 29, 2013
  23. Brad KingAug 29, 2013
  24. Martin FickAug 29, 2013
  25. Brad KingAug 29, 2013
  26. Junio C HamanoAug 29, 2013
  27. Brad KingAug 29, 2013
  28. Junio C HamanoAug 29, 2013
  29. Brad KingAug 29, 2013
  30. 0/8 Multiple simultaneously locked ref updatesBrad King, Aug 30, 2013
  31. 1/8 reset: rename update_refs to reset_refsBrad King, Aug 30, 2013
  32. 2/8 refs: report ref type from lock_any_ref_for_updateBrad King, Aug 30, 2013
  33. 3/8 refs: factor update_ref steps into helpersBrad King, Aug 30, 2013
  34. Junio C HamanoSep 1, 2013
  35. Brad KingSep 2, 2013
  36. 4/8 refs: factor delete_ref loose ref step into a helperBrad King, Aug 30, 2013
  37. Michael HaggertyAug 31, 2013
  38. Brad KingSep 2, 2013
  39. 5/8 refs: add function to repack without multiple refsBrad King, Aug 30, 2013
  40. 6/8 refs: add update_refs for multiple simultaneous updatesBrad King, Aug 30, 2013
  41. Michael HaggertyAug 31, 2013
  42. Brad KingSep 2, 2013
  43. Junio C HamanoSep 1, 2013
  44. Brad KingSep 2, 2013
  45. Michael HaggertySep 3, 2013
  46. Brad KingSep 3, 2013
  47. 7/8 update-ref: support multiple simultaneous updatesBrad King, Aug 30, 2013
  48. Junio C HamanoAug 30, 2013
  49. Brad KingSep 2, 2013
  50. Michael HaggertyAug 31, 2013
  51. Brad KingSep 2, 2013
  52. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Aug 30, 2013
  53. Eric SunshineSep 1, 2013
  54. Brad KingSep 2, 2013
  55. Michael HaggertyAug 31, 2013
  56. 0/8 Multiple simultaneously locked ref updatesBrad King, Sep 2, 2013
  57. 1/8 reset: rename update_refs to reset_refsBrad King, Sep 2, 2013
  58. 2/8 refs: report ref type from lock_any_ref_for_updateBrad King, Sep 2, 2013
  59. 3/8 refs: factor update_ref steps into helpersBrad King, Sep 2, 2013
  60. 4/8 refs: factor delete_ref loose ref step into a helperBrad King, Sep 2, 2013
  61. 5/8 refs: add function to repack without multiple refsBrad King, Sep 2, 2013
  62. 6/8 refs: add update_refs for multiple simultaneous updatesBrad King, Sep 2, 2013
  63. 7/8 update-ref: support multiple simultaneous updatesBrad King, Sep 2, 2013
  64. Brad KingSep 2, 2013
  65. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Sep 2, 2013
  66. Eric SunshineSep 3, 2013
  67. Brad KingSep 3, 2013
  68. 0/8 Multiple simultaneously locked ref updatesBrad King, Sep 4, 2013
  69. 1/8 reset: rename update_refs to reset_refsBrad King, Sep 4, 2013
  70. 2/8 refs: report ref type from lock_any_ref_for_updateBrad King, Sep 4, 2013
  71. 3/8 refs: factor update_ref steps into helpersBrad King, Sep 4, 2013
  72. 4/8 refs: factor delete_ref loose ref step into a helperBrad King, Sep 4, 2013
  73. 5/8 refs: add function to repack without multiple refsBrad King, Sep 4, 2013
  74. 6/8 refs: add update_refs for multiple simultaneous updatesBrad King, Sep 4, 2013
  75. 7/8 update-ref: support multiple simultaneous updatesBrad King, Sep 4, 2013
  76. Junio C HamanoSep 4, 2013
  77. Brad KingSep 4, 2013
  78. Junio C HamanoSep 4, 2013
  79. Brad KingSep 5, 2013
  80. Junio C HamanoSep 5, 2013
  81. Brad KingSep 5, 2013
  82. Junio C HamanoSep 4, 2013
  83. Brad KingSep 4, 2013
  84. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Sep 4, 2013
  85. 0/8 Multiple simultaneously locked ref updatesBrad King, Sep 9, 2013
  86. 7/8 update-ref: support multiple simultaneous updatesBrad King, Sep 9, 2013
  87. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Sep 9, 2013
  88. 0/8 Multiple simultaneously locked ref updatesBrad King, Sep 10, 2013
  89. 1/8 reset: rename update_refs to reset_refsBrad King, Sep 10, 2013
  90. Ramkumar RamachandraSep 10, 2013
  91. 2/8 refs: report ref type from lock_any_ref_for_updateBrad King, Sep 10, 2013
  92. 3/8 refs: factor update_ref steps into helpersBrad King, Sep 10, 2013
  93. 4/8 refs: factor delete_ref loose ref step into a helperBrad King, Sep 10, 2013
  94. 5/8 refs: add function to repack without multiple refsBrad King, Sep 10, 2013
  95. 6/8 refs: add update_refs for multiple simultaneous updatesBrad King, Sep 10, 2013
  96. 7/8 update-ref: support multiple simultaneous updatesBrad King, Sep 10, 2013
  97. Eric SunshineSep 10, 2013
  98. Brad KingSep 11, 2013
  99. Eric SunshineSep 11, 2013
  100. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Sep 10, 2013
  101. Eric SunshineSep 10, 2013
  102. Junio C HamanoSep 10, 2013
  103. 8/8 update-ref: add test cases covering --stdin signatureBrad King, Sep 11, 2013
  104. Junio C HamanoSep 10, 2013
  105. Brad KingSep 10, 2013
  106. Junio C HamanoSep 10, 2013

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.