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

Re: [PATCH v3 19/19] refs.c: pass **transaction to commit and have it clear the pointer

From
Ronnie Sahlberg <sahlberg@google.com>
Date
Apr 28, 2014, 17:59 UTC
Message-ID
<CAL=YDWkSdiUd-6A60ncGaDrFV2pc5WtRMv8iCSHHqFLkKH=pfw@mail.gmail.com>
In-Reply-To
<535B0C6F.509@alum.mit.edu>
On Fri, Apr 25, 2014 at 6:31 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
Show 25 quoted lines
> On 04/25/2014 06:14 PM, Ronnie Sahlberg wrote:
>> Change ref_transaction_commit to take a pointer to a pointer for the
>> transaction. This allows us to clear the transaction pointer from within
>> ref_transaction_commit so that it becomes NULL in the caller.
>>
>> This makes transaction handling in the callers much nicer since instead of
>> having to write horrible code like :
>>       t = ref_transaction_begin();
>>       if ((!t ||
>>           ref_transaction_update(t, refname, sha1, oldval, flags,
>>                                  !!oldval)) ||
>>           (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
>>               ref_transaction_rollback(t);
>>
>> we can now just do the much nicer
>>       t = ref_transaction_begin();
>>       if (!t ||
>>           ref_transaction_update(t, refname, sha1, oldval, flags,
>>                                  !!oldval) ||
>>           ref_transaction_commit(&t, action, &err)) {
>>               ref_transaction_rollback(t);
>
> I understand the motivation for this change, but passing
> pointer-to-pointer is unconventional in a case like this.  Unfortunately
> I ran out of steam for the night before I could think about alternatives.
I see.
Yes passing a pointer to pointer is not ideal.
But I still want to be able to use the pattern
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(t, ...)) {
               ref_transaction_rollback(t);

Maybe the problem is that ref_transaction_commit() implicitely also frees the transaction.

What about changing ref_transaction_commit() would NOT free the transaction and thus a caller would always have to explicitely free the transaction afterwards?

Something like this :
       t = ref_transaction_begin();
       if (!t ||
           ref_transaction_update(t, ...) ||
           ref_transaction_commit(&t, ...)) {
               ref_transaction_rollback(t);
       }
       ref_transaction_free(t);
Show 207 quoted lines
>
> Regarding patches 01-18, I agree with Jonathan's comments and made a few
> of my own.  I definitely don't think that code like "!(t = NULL)" should
> stay in our code base as more than a transitional state.  But I'd have
> to play with the alternatives before I can form an opinion on your
> suggestion in this patch.
>
> Thanks for working on this!
>
> Michael
>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  branch.c             |  4 ++--
>>  builtin/commit.c     |  2 +-
>>  builtin/replace.c    |  2 +-
>>  builtin/tag.c        |  2 +-
>>  builtin/update-ref.c |  2 +-
>>  fast-import.c        |  7 +++----
>>  refs.c               | 18 ++++++++++--------
>>  refs.h               |  3 ++-
>>  sequencer.c          |  7 +++----
>>  9 files changed, 24 insertions(+), 23 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 23cde1e..5d68467 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -303,14 +303,14 @@ void create_branch(const char *head,
>>                       if (!transaction ||
>>                           ref_transaction_update(transaction, ref.buf, sha1,
>>                                                  NULL, 0, 0) ||
>> -                         ref_transaction_commit(transaction, msg, &err))
>> +                         ref_transaction_commit(&transaction, msg, &err))
>>                         die_errno(_("%s: failed to write ref: %s"),
>>                                   ref.buf, err);
>>               } else {
>>                       if (!transaction ||
>>                           ref_transaction_create(transaction, ref.buf, sha1,
>>                                                  0) ||
>> -                         ref_transaction_commit(transaction, msg, &err))
>> +                         ref_transaction_commit(&transaction, msg, &err))
>>                         die_errno(_("%s: failed to write ref: %s"),
>>                                   ref.buf, err);
>>               }
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 7e4c306..3142827 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1682,7 +1682,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>>                                  current_head ?
>>                                  current_head->object.sha1 : NULL,
>>                                  0, !!current_head) ||
>> -         ref_transaction_commit(transaction, sb.buf, &err)) {
>> +         ref_transaction_commit(&transaction, sb.buf, &err)) {
>>               rollback_index_files();
>>               die(_("HEAD: cannot update ref: %s"), err);
>>       }
>> diff --git a/builtin/replace.c b/builtin/replace.c
>> index cf0f56d..51e9ddf 100644
>> --- a/builtin/replace.c
>> +++ b/builtin/replace.c
>> @@ -162,7 +162,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
>>       if (!transaction ||
>>           ref_transaction_update(transaction, ref, repl, prev,
>>                                  0, !is_null_sha1(prev)) ||
>> -         ref_transaction_commit(transaction, NULL, &err))
>> +         ref_transaction_commit(&transaction, NULL, &err))
>>         die(_("%s: failed to replace ref: %s"), ref, err);
>>
>>       return 0;
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index dd53fb4..60b57a1 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -646,7 +646,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
>>       if (!transaction ||
>>           ref_transaction_update(transaction, ref.buf, object, prev,
>>                                  0, !is_null_sha1(prev)) ||
>> -         ref_transaction_commit(transaction, NULL, &err))
>> +         ref_transaction_commit(&transaction, NULL, &err))
>>         die(_("%s: cannot update the ref: %s"), ref.buf, err);
>>       if (force && !is_null_sha1(prev) && hashcmp(prev, object))
>>               printf(_("Updated tag '%s' (was %s)\n"), tag, find_unique_abbrev(prev, DEFAULT_ABBREV));
>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
>> index a600ab3..4a0901d 100644
>> --- a/builtin/update-ref.c
>> +++ b/builtin/update-ref.c
>> @@ -373,7 +373,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
>>               if (end_null)
>>                       line_termination = '\0';
>>               update_refs_stdin();
>> -             if (ref_transaction_commit(transaction, msg, &err))
>> +             if (ref_transaction_commit(&transaction, msg, &err))
>>                       die("update_ref failed: %s", err);
>>               return 0;
>>       }
>> diff --git a/fast-import.c b/fast-import.c
>> index a2b05fa..3ce2f47 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1703,11 +1703,10 @@ static int update_branch(struct branch *b)
>>               }
>>       }
>>       transaction = ref_transaction_begin();
>> -     if ((!transaction ||
>> +     if (!transaction ||
>>           ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
>> -                                0, 1)) ||
>> -         (ref_transaction_commit(transaction, msg, &err) &&
>> -          !(transaction = NULL))) {
>> +                                0, 1) ||
>> +         ref_transaction_commit(&transaction, msg, &err)) {
>>               ref_transaction_rollback(transaction);
>>               return error("Unable to update branch %s: %s", b->name, err);
>>       }
>> diff --git a/refs.c b/refs.c
>> index ffa9c83..0b60250 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3401,10 +3401,10 @@ int update_ref(const char *action, const char *refname,
>>       char *err = NULL;
>>
>>       t = ref_transaction_begin();
>> -     if ((!t ||
>> +     if (!t ||
>>           ref_transaction_update(t, refname, sha1, oldval, flags,
>> -                                !!oldval)) ||
>> -         (ref_transaction_commit(t, action, &err) && !(t = NULL))) {
>> +                                !!oldval) ||
>> +         ref_transaction_commit(&t, action, &err)) {
>>            const char *str = "update_ref failed for ref '%s': %s";
>>
>>               ref_transaction_rollback(t);
>> @@ -3444,16 +3444,17 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,
>>       return 0;
>>  }
>>
>> -int ref_transaction_commit(struct ref_transaction *transaction,
>> +int ref_transaction_commit(struct ref_transaction **transaction,
>>                          const char *msg, char **err)
>>  {
>>       int ret = 0, delnum = 0, i;
>>       const char **delnames;
>> -     int n = transaction->nr;
>> -     struct ref_update **updates = transaction->updates;
>> +     int n = (*transaction)->nr;
>> +     struct ref_update **updates = (*transaction)->updates;
>>
>>       if (!n) {
>> -             ref_transaction_free(transaction);
>> +             ref_transaction_free(*transaction);
>> +             *transaction = NULL;
>>               return 0;
>>       }
>>
>> @@ -3527,7 +3528,8 @@ cleanup:
>>               if (updates[i]->lock)
>>                       unlock_ref(updates[i]->lock);
>>       free(delnames);
>> -     ref_transaction_free(transaction);
>> +     ref_transaction_free(*transaction);
>> +     *transaction = NULL;
>>       return ret;
>>  }
>>
>> diff --git a/refs.h b/refs.h
>> index 8135131..85127f2 100644
>> --- a/refs.h
>> +++ b/refs.h
>> @@ -270,8 +270,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,
>>   * 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.
>> + * *transaction is reset to NULL in this call.
>>   */
>> -int ref_transaction_commit(struct ref_transaction *transaction,
>> +int ref_transaction_commit(struct ref_transaction **transaction,
>>                          const char *msg, char **err);
>>
>>  /** Lock a ref and then write its file */
>> diff --git a/sequencer.c b/sequencer.c
>> index 7d59f58..3f6eced 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -283,11 +283,10 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,
>>       strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
>>
>>       transaction = ref_transaction_begin();
>> -     if ((!transaction ||
>> +     if (!transaction ||
>>           ref_transaction_update(transaction, "HEAD", to, from,
>> -                                0, !unborn)) ||
>> -         (ref_transaction_commit(transaction, sb.buf, &err) &&
>> -          !(transaction = NULL))) {
>> +                                0, !unborn) ||
>> +         ref_transaction_commit(&transaction, sb.buf, &err)) {
>>               ref_transaction_rollback(transaction);
>>               strbuf_release(&sb);
>>               return error(_("HEAD: Could not fast-forward: %s\n"), err);
>>
>
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/
Previous: Michael HaggertyNext: Michael Haggerty
Message 43 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.