Re: [PATCH v6 13/42] commit.c: use ref transactions for updates
On Wed, May 14, 2014 at 6:11 PM, Jonathan Nieder jrnie...@gmail.com wrote: Ronnie Sahlberg wrote: [...] +++ b/builtin/commit.c @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix) [...] @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_release(author_ident); free_commit_extra_headers(extra); - ref_lock = lock_any_ref_for_update(HEAD, -!current_head -? NULL -: current_head-object.sha1, -0, NULL); - if (!ref_lock) { - rollback_index_files(); - die(_(cannot lock HEAD ref)); - } - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); else strbuf_addch(sb, '\n'); strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg)); strbuf_insert(sb, strlen(reflog_msg), : , 2); - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, HEAD, sha1, +current_head ? +current_head-object.sha1 : NULL, +0, !!current_head) || + ref_transaction_commit(transaction, sb.buf, err)) { rollback_index_files(); - die(_(cannot update HEAD ref)); + die(_(HEAD: cannot update ref: %s), err.buf); Same question about !transaction (it also applies to later patches but I won't mention it any more). The error message changed from fatal: cannot lock HEAD ref to fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'. Does the message from ref_transaction_commit always say what ref was being updated when it failed? If so, it's tempting to just use the message as-is: fatal: Cannot lock the ref 'HEAD' If the caller should add to the message, it could say something about the context --- e.g., fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD' Looking at that, die(%s, err.buf) seems simplest since even if git commit was being called in a loop, it's already clear that git was trying to lock HEAD to advance it. Changed it to die(%s, err.buf) as you suggested. Many thanks for the reviews so far! regards ronnie sahlberg -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 13/42] commit.c: use ref transactions for updates
Ronnie Sahlberg wrote: [...] +++ b/builtin/commit.c @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix) [...] @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_release(author_ident); free_commit_extra_headers(extra); - ref_lock = lock_any_ref_for_update(HEAD, -!current_head -? NULL -: current_head-object.sha1, -0, NULL); - if (!ref_lock) { - rollback_index_files(); - die(_(cannot lock HEAD ref)); - } - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); else strbuf_addch(sb, '\n'); strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg)); strbuf_insert(sb, strlen(reflog_msg), : , 2); - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, HEAD, sha1, +current_head ? +current_head-object.sha1 : NULL, +0, !!current_head) || + ref_transaction_commit(transaction, sb.buf, err)) { rollback_index_files(); - die(_(cannot update HEAD ref)); + die(_(HEAD: cannot update ref: %s), err.buf); Same question about !transaction (it also applies to later patches but I won't mention it any more). The error message changed from fatal: cannot lock HEAD ref to fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'. Does the message from ref_transaction_commit always say what ref was being updated when it failed? If so, it's tempting to just use the message as-is: fatal: Cannot lock the ref 'HEAD' If the caller should add to the message, it could say something about the context --- e.g., fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD' Looking at that, die(%s, err.buf) seems simplest since even if git commit was being called in a loop, it's already clear that git was trying to lock HEAD to advance it. Thanks, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 13/42] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates. Make sure we pass a NULL pointer to ref_transaction_update if have_old is false. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/commit.c | 23 ++- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index f0b7906..6117123 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1541,11 +1541,12 @@ int cmd_commit(int argc, const char **argv, const char *prefix) const char *index_file, *reflog_msg; char *nl; unsigned char sha1[20]; - struct ref_lock *ref_lock; struct commit_list *parents = NULL, **pptr = parents; struct stat statbuf; struct commit *current_head = NULL; struct commit_extra_header *extra = NULL; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (argc == 2 !strcmp(argv[1], -h)) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1667,16 +1668,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_release(author_ident); free_commit_extra_headers(extra); - ref_lock = lock_any_ref_for_update(HEAD, - !current_head - ? NULL - : current_head-object.sha1, - 0, NULL); - if (!ref_lock) { - rollback_index_files(); - die(_(cannot lock HEAD ref)); - } - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); @@ -1685,9 +1676,15 @@ int cmd_commit(int argc, const char **argv, const char *prefix) strbuf_insert(sb, 0, reflog_msg, strlen(reflog_msg)); strbuf_insert(sb, strlen(reflog_msg), : , 2); - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + transaction = ref_transaction_begin(); + if (!transaction || + ref_transaction_update(transaction, HEAD, sha1, + current_head ? + current_head-object.sha1 : NULL, + 0, !!current_head) || + ref_transaction_commit(transaction, sb.buf, err)) { rollback_index_files(); - die(_(cannot update HEAD ref)); + die(_(HEAD: cannot update ref: %s), err.buf); } unlink(git_path(CHERRY_PICK_HEAD)); -- 2.0.0.rc1.351.g4d2c8e4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html