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

Re: [PATCH 05/11] tag.c: use ref transactions when doing updates

From
Michael Haggerty <mhagger@alum.mit.edu>
Date
Apr 19, 2014, 19:12 UTC
Message-ID
<5352CAB2.2000909@alum.mit.edu>
In-Reply-To
<1397763987-4453-6-git-send-email-sahlberg@google.com>
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
Show 44 quoted lines
> Change tag.c to use ref transactions for all ref updates.
> 
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/tag.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 40356e3..dbeacc5 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	struct strbuf ref = STRBUF_INIT;
>  	unsigned char object[20], prev[20];
>  	const char *object_ref, *tag;
> -	struct ref_lock *lock;
>  	struct create_tag_options opt;
>  	char *cleanup_arg = NULL;
>  	int annotate = 0, force = 0, lines = -1;
> @@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	const char *msgfile = NULL, *keyid = NULL;
>  	struct msg_arg msg = { 0, STRBUF_INIT };
>  	struct commit_list *with_commit = NULL;
> +	struct ref_transaction *transaction;
>  	struct option options[] = {
>  		OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
>  		{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
> @@ -641,11 +641,14 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>  	if (annotate)
>  		create_tag(object, tag, &buf, &opt, prev, object);
>  
> -	lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
> -	if (!lock)
> -		die(_("%s: cannot lock the ref"), ref.buf);
> -	if (write_ref_sha1(lock, object, NULL) < 0)
> -		die(_("%s: cannot update the ref"), ref.buf);
> +	transaction = ref_transaction_begin();
> +	if (!transaction)
> +		die(_("%s: cannot start transaction"), ref.buf);
> +	if (ref_transaction_update(transaction, ref.buf, object, prev,
> +				   0, !is_null_sha1(prev)))
> +		die(_("%s: cannot update transaction"), ref.buf);
> +	if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
> +		die(_("%s: cannot commit transaction"), ref.buf);

I wonder whether these error messages are meaningful to the user. To the user, it's all just a failure to create the tag, right? Does it help to tell the user that the error was in the "start transaction", "update transaction", or "commit transaction" phase?

In other words, maybe it would be just as good to do
if (!transaction ||
    ref_transaction_update(transaction, ref.buf, object, prev,
			   0, !is_null_sha1(prev)) ||
    ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR))
	die(_("%s: cannot update the ref"), ref.buf);

But then I would also hope that somebody (probably ref_transaction_commit() now, but later maybe ref_transaction_update()) tells the user whether the problem was the inability to lock the reference, or the consistency check of old_sha1, or whatever.

The next question is whether the generic error messages that the refs code generates is good enough, or whether the caller would rather learn about the reason for an error and generate its own, more specific error message. Please pay attention to this as you rewrite callers: by relying more on centralized code, are we losing out unacceptably on error message specificity?

Oh and by the way, given that you pass UPDATE_REFS_DIE_ON_ERR, ref_transaction_commit() should never return a nonzero value, should it?

>  	if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>  		printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>  
> 
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
Previous: Ronnie SahlbergNext: Ronnie Sahlberg
Message 12 of 24 in “Use ref transactions from most callers”
  1. 00/11 Use ref transactions from most callersRonnie Sahlberg, Apr 17, 2014
  2. 01/11 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Apr 17, 2014
  3. Michael HaggertyApr 19, 2014
  4. 02/11 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  5. Michael HaggertyApr 19, 2014
  6. Jonathan NiederApr 25, 2014
  7. 03/11 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Apr 17, 2014
  8. Michael HaggertyApr 19, 2014
  9. 04/11 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, Apr 17, 2014
  10. Michael HaggertyApr 19, 2014
  11. 05/11 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Apr 17, 2014
  12. Michael HaggertyApr 19, 2014
  13. 06/11 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Apr 17, 2014
  14. Michael HaggertyApr 19, 2014
  15. 07/11 commit.c: use ref transactions for updatesRonnie Sahlberg, Apr 17, 2014
  16. Michael HaggertyApr 19, 2014
  17. Ronnie SahlbergApr 21, 2014
  18. 08/11 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Apr 17, 2014
  19. 09/11 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Apr 17, 2014
  20. 10/11 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Apr 17, 2014
  21. 11/11 walker.c: use ref transaction for ref updatesRonnie Sahlberg, Apr 17, 2014
  22. Michael HaggertyApr 19, 2014
  23. Junio C HamanoApr 21, 2014
  24. Ronnie SahlbergApr 21, 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.