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

Re: [PATCH v3 03/19] refs.c: make ref_transaction_commit return an error string

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Apr 28, 2014, 17:46 UTC
Message-ID
<CAL=YDW=CBnWU3t05MwAYkGP+0cLPdVqypM-jjZhCzUiWEjCwsA@mail.gmail.com>
In-Reply-To
<20140425221001.GB9218@google.com>
Good points.
On Fri, Apr 25, 2014 at 3:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> Ronnie Sahlberg wrote:
>
>> Let ref_transaction_commit return an optional error string that describes
>> the transaction failure.  Start by returning the same error as update_ref_lock
>> returns, modulo the initial error:/fatal: preamble.
>
> s/returns/prints/?
Done, and then was deleted when I reworded the message.
Show 7 quoted lines
>
>> This will make it easier for callers to craft better error messages when
>> a transaction call fails.
>
> Interesting.  Can you give an example?  What kind of behavior are we
> expecting in callers other than die()-ing or cleaning up and then
> die()-ing?

I was thinking a bit too far ahead. You could in theory keep logging multiple lock failures during the _commit() and then when the transaction fails and returns it will have appended a list of all refs that failed and not just the first ref that failed.

Show 23 quoted lines
>
> I like this more than having the caller pass in a flag/callback/etc to
> decide how noisy to be and whether to gracefully accept errors or exit.
> So it seems like an improvement, but may always returning error()
> would be better --- more context would help in clarifying this.
>
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,
>>   * Commit all of the changes that have been queued in transaction, as
>>   * atomically as possible.  Return a nonzero value if there is a
>>   * problem.  The ref_transaction is freed by this function.
>> + * If error is non-NULL it will return an error string that describes
>> + * why a commit failed. This string must be free()ed by the caller.
>>   */
>>  int ref_transaction_commit(struct ref_transaction *transaction,
>> -                        const char *msg, enum action_on_err onerr);
>> +                        const char *msg, char **err,
>> +                        enum action_on_err onerr);
>
> Is the idea that if I pass in a pointer &err then
> ref_transaction_commit will take the action described by onerr *and*
> write its error message to err?

Temporarily, yes. Shortly after this patch I remove the onerr argument completely. But I want to keep the "pass error back to caller" and "get rid of onerr" as two separate patches. I think it is easier to follow the flow of changes if they are done in two separate steps.

>
> Probably squashing with patch 07 would make this easier to read (and
> wouldn't require changing any messages at that point).
See above.
Show 32 quoted lines
>
> [...]
>> --- a/refs.c
>> +++ b/refs.c
> [...]
>> @@ -3443,6 +3447,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
>>                                              update->flags,
>>                                              &update->type, onerr);
>>               if (!update->lock) {
>> +                     if (err) {
>> +                             const char *str = "Cannot lock the ref '%s'.";
>> +                             *err = xmalloc(PATH_MAX + 24);
>> +                             snprintf(*err, PATH_MAX + 24, str,
>> +                                      update->refname);
>> +                     }
>
> Might be clearer to use a helper similar to path.c::mkpathdup
>
>         char *aprintf(const char *fmt, ...)
>         {
>                 char *result;
>                 struct strbuf sb = STRBUF_INIT;
>                 va_list args;
>
>                 va_start(args, fmt);
>                 strbuf_vaddf(&sb, fmt, args);
>                 va_end(args);
>
>                 return strbuf_detach(&sb);
>         }
>
> or to have the caller pass in a pointer to strbuf instead of char *.
strbuf as argument is probably the right thing to do. I am doing that change.
Show 5 quoted lines
>
> The rest looks good to me.
>
> Thanks,
> Jonathan
Previous: Jonathan NiederNext: Ronnie Sahlberg
Message 6 of 48 in “Use ref transactions from most callers”
  1. 00/19 Use ref transactions from most callersRonnie Sahlberg, Apr 25, 2014
  2. 01/19 refs.c: constify the sha arguments for ref_transaction_create|delete|updateRonnie Sahlberg, Apr 25, 2014
  3. 02/19 refs.c: allow passing NULL to ref_transaction_freeRonnie Sahlberg, Apr 25, 2014
  4. 03/19 refs.c: make ref_transaction_commit return an error stringRonnie Sahlberg, Apr 25, 2014
  5. Jonathan NiederApr 25, 2014
  6. Ronnie SahlbergApr 28, 2014
  7. 04/19 refs.c: return error string from ref_update_reject_duplicates on failureRonnie Sahlberg, Apr 25, 2014
  8. Jonathan NiederApr 25, 2014
  9. Ronnie SahlbergApr 28, 2014
  10. 05/19 update-ref.c: use the error string from _commit to print better messageRonnie Sahlberg, Apr 25, 2014
  11. Jonathan NiederApr 25, 2014
  12. Ronnie SahlbergApr 28, 2014
  13. 06/19 refs.c: make update_ref_write to return error string on failureRonnie Sahlberg, Apr 25, 2014
  14. Jonathan NiederApr 25, 2014
  15. Ronnie SahlbergApr 28, 2014
  16. 07/19 refs.c: remove the onerr argument to ref_transaction_commitRonnie Sahlberg, Apr 25, 2014
  17. Jonathan NiederApr 25, 2014
  18. Ronnie SahlbergApr 28, 2014
  19. 08/19 refs.c: change ref_transaction_update() to do error checking and return statusRonnie Sahlberg, Apr 25, 2014
  20. 09/19 refs.c: change ref_transaction_create to do error checking and return statusRonnie Sahlberg, Apr 25, 2014
  21. 10/19 refs.c: ref_transaction_delete to check for error and return statusRonnie Sahlberg, Apr 25, 2014
  22. 11/19 tag.c: use ref transactions when doing updatesRonnie Sahlberg, Apr 25, 2014
  23. Michael HaggertyApr 25, 2014
  24. Ronnie SahlbergApr 28, 2014
  25. 12/19 replace.c: use the ref transaction functions for updatesRonnie Sahlberg, Apr 25, 2014
  26. Michael HaggertyApr 25, 2014
  27. Ronnie SahlbergApr 28, 2014
  28. 13/19 commit.c: use ref transactions for updatesRonnie Sahlberg, Apr 25, 2014
  29. 14/19 sequencer.c: use ref transactions for all ref updatesRonnie Sahlberg, Apr 25, 2014
  30. 15/19 fast-import.c: change update_branch to use ref transactionsRonnie Sahlberg, Apr 25, 2014
  31. 16/19 branch.c: use ref transaction for all ref updatesRonnie Sahlberg, Apr 25, 2014
  32. Michael HaggertyApr 25, 2014
  33. Ronnie SahlbergApr 28, 2014
  34. Michael HaggertyApr 29, 2014
  35. Ronnie SahlbergApr 29, 2014
  36. Junio C HamanoApr 29, 2014
  37. 17/19 refs.c: change update_ref to use a transactionRonnie Sahlberg, Apr 25, 2014
  38. Michael HaggertyApr 25, 2014
  39. Ronnie SahlbergApr 28, 2014
  40. 18/19 refs.c: free the transaction before returning when number of updates is 0Ronnie Sahlberg, Apr 25, 2014
  41. 19/19 refs.c: pass **transaction to commit and have it clear the pointerRonnie Sahlberg, Apr 25, 2014
  42. Michael HaggertyApr 26, 2014
  43. Ronnie SahlbergApr 28, 2014
  44. Michael HaggertyApr 29, 2014
  45. Ronnie SahlbergApr 29, 2014
  46. Michael HaggertyApr 30, 2014
  47. Jonathan NiederApr 25, 2014
  48. Ronnie SahlbergApr 25, 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.