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

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

From
Junio C Hamano <gitster@pobox.com>
Date
Apr 1, 2014, 19:46 UTC
Message-ID
<xmqqppl0zvcs.fsf@gitster.dls.corp.google.com>
In-Reply-To
<1395683820-17304-21-git-send-email-mhagger@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
Show 8 quoted lines
> This change is mostly clerical: the parse_cmd_*() functions need to
> use local variables rather than a struct ref_update to collect the
> arguments needed for each update, and then call ref_transaction_*() to
> queue the change rather than building up the list of changes at the
> caller side.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

With the implementation of ref_transaction at this point in the series it does not matter, but the updated code in this patch means that it is perfectly acceptable to do this sequence:

    ref_transaction_begin();
    ref_transaction_update();
    ...
    ref_transaction_update();
    die();

without ever calling ref_transaction_rollback() API function. Depending on the future backends, we may want to ensure rollback is called, no? And if that is the case, we would want to prepare callers of the API with some at-exit facility to call rollback, no?

Other than that, the code looks straight-forward.
Thanks.
Show 224 quoted lines
>  builtin/update-ref.c | 142 +++++++++++++++++++++++++++------------------------
>  1 file changed, 75 insertions(+), 67 deletions(-)
>
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index bbc04b2..2c8678b 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
>  	NULL
>  };
>  
> -static int updates_alloc;
> -static int updates_count;
> -static struct ref_update **updates;
> +static struct ref_transaction *transaction;
>  
>  static char line_termination = '\n';
>  static int update_flags;
>  
> -static struct ref_update *update_alloc(void)
> -{
> -	struct ref_update *update;
> -
> -	/* Allocate and zero-init a struct ref_update */
> -	update = xcalloc(1, sizeof(*update));
> -	ALLOC_GROW(updates, updates_count + 1, updates_alloc);
> -	updates[updates_count++] = update;
> -
> -	/* Store and reset accumulated options */
> -	update->flags = update_flags;
> -	update_flags = 0;
> -
> -	return update;
> -}
> -
>  /*
>   * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
>   * and append the result to arg.  Return a pointer to the terminator.
> @@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const char **next,
>  
>  static const char *parse_cmd_update(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("update: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->new_sha1,
> -			    "update", update->ref_name,
> +	if (parse_next_sha1(input, &next, new_sha1, "update", refname,
>  			    PARSE_SHA1_ALLOW_EMPTY))
> -		die("update %s: missing <newvalue>", update->ref_name);
> +		die("update %s: missing <newvalue>", refname);
>  
> -	update->have_old = !parse_next_sha1(input, &next, update->old_sha1,
> -					    "update", update->ref_name,
> -					    PARSE_SHA1_OLD);
> +	have_old = !parse_next_sha1(input, &next, old_sha1, "update", refname,
> +				    PARSE_SHA1_OLD);
>  
>  	if (*next != line_termination)
> -		die("update %s: extra input: %s", update->ref_name, next);
> +		die("update %s: extra input: %s", refname, next);
> +
> +	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_create(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("create: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->new_sha1,
> -			    "create", update->ref_name, 0))
> -		die("create %s: missing <newvalue>", update->ref_name);
> +	if (parse_next_sha1(input, &next, new_sha1, "create", refname, 0))
> +		die("create %s: missing <newvalue>", refname);
>  
> -	if (is_null_sha1(update->new_sha1))
> -		die("create %s: zero <newvalue>", update->ref_name);
> +	if (is_null_sha1(new_sha1))
> +		die("create %s: zero <newvalue>", refname);
>  
>  	if (*next != line_termination)
> -		die("create %s: extra input: %s", update->ref_name, next);
> +		die("create %s: extra input: %s", refname, next);
> +
> +	ref_transaction_create(transaction, refname, new_sha1, update_flags);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_delete(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> +	char *refname;
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update = update_alloc();
> -
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("delete: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->old_sha1,
> -			    "delete", update->ref_name, PARSE_SHA1_OLD)) {
> -		update->have_old = 0;
> +	if (parse_next_sha1(input, &next, old_sha1, "delete", refname,
> +			    PARSE_SHA1_OLD)) {
> +		have_old = 0;
>  	} else {
> -		if (is_null_sha1(update->old_sha1))
> -			die("delete %s: zero <oldvalue>", update->ref_name);
> -		update->have_old = 1;
> +		if (is_null_sha1(old_sha1))
> +			die("delete %s: zero <oldvalue>", refname);
> +		have_old = 1;
>  	}
>  
>  	if (*next != line_termination)
> -		die("delete %s: extra input: %s", update->ref_name, next);
> +		die("delete %s: extra input: %s", refname, next);
> +
> +	ref_transaction_delete(transaction, refname, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
>  
>  static const char *parse_cmd_verify(struct strbuf *input, const char *next)
>  {
> -	struct ref_update *update;
> -
> -	update = update_alloc();
> +	char *refname;
> +	unsigned char new_sha1[20];
> +	unsigned char old_sha1[20];
> +	int have_old;
>  
> -	update->ref_name = parse_refname(input, &next);
> -	if (!update->ref_name)
> +	refname = parse_refname(input, &next);
> +	if (!refname)
>  		die("verify: missing <ref>");
>  
> -	if (parse_next_sha1(input, &next, update->old_sha1,
> -			    "verify", update->ref_name, PARSE_SHA1_OLD)) {
> -		update->have_old = 0;
> +	if (parse_next_sha1(input, &next, old_sha1, "verify", refname,
> +			    PARSE_SHA1_OLD)) {
> +		hashclr(new_sha1);
> +		have_old = 0;
>  	} else {
> -		hashcpy(update->new_sha1, update->old_sha1);
> -		update->have_old = 1;
> +		hashcpy(new_sha1, old_sha1);
> +		have_old = 1;
>  	}
>  
>  	if (*next != line_termination)
> -		die("verify %s: extra input: %s", update->ref_name, next);
> +		die("verify %s: extra input: %s", refname, next);
> +
> +	ref_transaction_update(transaction, refname, new_sha1, old_sha1,
> +			       update_flags, have_old);
> +
> +	update_flags = 0;
> +	free(refname);
>  
>  	return next;
>  }
> @@ -355,13 +359,17 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>  		die("Refusing to perform update with empty message.");
>  
>  	if (read_stdin) {
> +		int ret;
> +		transaction = ref_transaction_begin();
> +
>  		if (delete || no_deref || argc > 0)
>  			usage_with_options(git_update_ref_usage, options);
>  		if (end_null)
>  			line_termination = '\0';
>  		update_refs_stdin();
> -		return update_refs(msg, updates, updates_count,
> -				   UPDATE_REFS_DIE_ON_ERR);
> +		ret = ref_transaction_commit(transaction, msg,
> +					     UPDATE_REFS_DIE_ON_ERR);
> +		return ret;
>  	}
>  
>  	if (end_null)
Previous: Michael HaggertyNext: Michael Haggerty
Message 45 of 65 in “Clean up update-refs --stdin and implement ref_transaction”
  1. 00/27 Clean up update-refs --stdin and implement ref_transactionMichael Haggerty, Mar 24, 2014
  2. 01/27 t1400: Fix name and expected result of one testMichael Haggerty, Mar 24, 2014
  3. Junio C HamanoMar 31, 2014
  4. Michael HaggertyMar 31, 2014
  5. 02/27 t1400: Provide more usual input to the commandMichael Haggerty, Mar 24, 2014
  6. Junio C HamanoMar 31, 2014
  7. 03/27 parse_arg(): Really test that argument is properly terminatedMichael Haggerty, Mar 24, 2014
  8. Junio C HamanoMar 31, 2014
  9. Michael HaggertyMar 31, 2014
  10. 04/27 t1400: Add some more tests involving quoted argumentsMichael Haggerty, Mar 24, 2014
  11. 05/27 refs.h: Rename the action_on_err constantsMichael Haggerty, Mar 24, 2014
  12. 06/27 update_refs(): Fix constnessMichael Haggerty, Mar 24, 2014
  13. Junio C HamanoMar 31, 2014
  14. Michael HaggertyMar 31, 2014
  15. Junio C HamanoMar 31, 2014
  16. 07/27 update-ref --stdin: Read the whole input at onceMichael Haggerty, Mar 24, 2014
  17. 08/27 parse_cmd_verify(): Copy old_sha1 instead of evaluating <oldvalue> twiceMichael Haggerty, Mar 24, 2014
  18. 09/27 update-ref.c: Extract a new function, parse_refname()Michael Haggerty, Mar 24, 2014
  19. 10/27 update-ref --stdin: Improve error messages for invalid valuesMichael Haggerty, Mar 24, 2014
  20. 11/27 update-ref --stdin: Make error messages more consistentMichael Haggerty, Mar 24, 2014
  21. 12/27 update-ref --stdin: Simplify error messages for missing oldvaluesMichael Haggerty, Mar 24, 2014
  22. 13/27 t1400: Test that stdin -z update treats empty <newvalue> as zerosMichael Haggerty, Mar 24, 2014
  23. Junio C HamanoMar 31, 2014
  24. Michael HaggertyMar 31, 2014
  25. 14/27 update-ref.c: Extract a new function, parse_next_sha1()Michael Haggerty, Mar 24, 2014
  26. Brad KingMar 26, 2014
  27. 15/27 update-ref --stdin -z: Deprecate interpreting the empty string as zerosMichael Haggerty, Mar 24, 2014
  28. Junio C HamanoMar 31, 2014
  29. 16/27 t1400: Test one mistake at a timeMichael Haggerty, Mar 24, 2014
  30. Brad KingMar 26, 2014
  31. Junio C HamanoMar 31, 2014
  32. Michael HaggertyMar 31, 2014
  33. 17/27 update-ref --stdin: Improve the error message for unexpected EOFMichael Haggerty, Mar 24, 2014
  34. 18/27 update-ref --stdin: Harmonize error messagesMichael Haggerty, Mar 24, 2014
  35. Junio C HamanoMar 31, 2014
  36. Michael HaggertyMar 31, 2014
  37. Michael HaggertyApr 1, 2014
  38. Junio C HamanoApr 2, 2014
  39. 19/27 refs: Add a concept of a reference transactionMichael Haggerty, Mar 24, 2014
  40. Brad KingMar 26, 2014
  41. Michael HaggertyMar 26, 2014
  42. Junio C HamanoApr 1, 2014
  43. Michael HaggertyApr 2, 2014
  44. 20/27 update-ref --stdin: Reimplement using reference transactionsMichael Haggerty, Mar 24, 2014
  45. Junio C HamanoApr 1, 2014
  46. Michael HaggertyApr 2, 2014
  47. Junio C HamanoApr 3, 2014
  48. Michael HaggertyApr 4, 2014
  49. 21/27 refs: Remove API function update_refs()Michael Haggerty, Mar 24, 2014
  50. Junio C HamanoApr 1, 2014
  51. 22/27 struct ref_update: Rename field "ref_name" to "refname"Michael Haggerty, Mar 24, 2014
  52. Junio C HamanoApr 1, 2014
  53. Michael HaggertyApr 2, 2014
  54. 23/27 struct ref_update: Store refname as a FLEX_ARRAY.Michael Haggerty, Mar 24, 2014
  55. Junio C HamanoApr 1, 2014
  56. 24/27 ref_transaction_commit(): Introduce temporary variablesMichael Haggerty, Mar 24, 2014
  57. Junio C HamanoApr 1, 2014
  58. 25/27 struct ref_update: Add a lock memberMichael Haggerty, Mar 24, 2014
  59. 26/27 struct ref_update: Add type fieldMichael Haggerty, Mar 24, 2014
  60. Junio C HamanoApr 1, 2014
  61. Michael HaggertyApr 2, 2014
  62. Junio C HamanoApr 2, 2014
  63. 27/27 ref_transaction_commit(): Work with transaction->updates in placeMichael Haggerty, Mar 24, 2014
  64. Brad KingMar 26, 2014
  65. Michael HaggertyMar 26, 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.