Re: [PATCH 07/11] commit.c: use ref transactions for updates
On Sat, Apr 19, 2014 at 12:23 PM, Michael Haggerty mhag...@alum.mit.edu wrote: On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: Change commit.c to use ref transactions for all ref updates. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/commit.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d9550c5..b8e4389 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1541,11 +1541,11 @@ 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; if (argc == 2 !strcmp(argv[1], -h)) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1667,12 +1667,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); The old version, above, contemplates that current_head might be NULL... - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); @@ -1681,14 +1675,22 @@ 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 (!ref_lock) { + transaction = ref_transaction_begin(); + if (!transaction) { rollback_index_files(); - die(_(cannot lock HEAD ref)); + die(_(HEAD: cannot start transaction)); } - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + if (ref_transaction_update(transaction, HEAD, sha1, +current_head-object.sha1, +0, !!current_head)) { ...but here you dereference current_head without checking it first. It upsets me that the test suite didn't catch this NULL pointer dereference. Either 1. current_head cannot in fact be NULL, in which case the commit message should explain that fact and the code should be simplified or 2. the test suite is incomplete. If so, it would be great if you would add a test that exercises this branch of the code (and catches your error), and then fix the error. Current_head can in fact be NULL here but we never actually dereference any pointer in this case since !!current_head is 0. current_head-object.sha1 just computes current_head + offsetof(commit, object) + offsetof(object, sha1) so we compute and pass a non-NULL garbage pointer into ref_transaction_update() But since !!current_head is 0 this mean we never actually dereference this pointer. Way to subtle for its own good. I will change ref_transaction_update and friends to use an additional test to detect some subset of this kind of bug : if (!have_old old_sha1) die(have_old is false but old_sha1 is not NULL); 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 07/11] commit.c: use ref transactions for updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: Change commit.c to use ref transactions for all ref updates. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/commit.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d9550c5..b8e4389 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1541,11 +1541,11 @@ 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; if (argc == 2 !strcmp(argv[1], -h)) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1667,12 +1667,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); The old version, above, contemplates that current_head might be NULL... - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); @@ -1681,14 +1675,22 @@ 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 (!ref_lock) { + transaction = ref_transaction_begin(); + if (!transaction) { rollback_index_files(); - die(_(cannot lock HEAD ref)); + die(_(HEAD: cannot start transaction)); } - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + if (ref_transaction_update(transaction, HEAD, sha1, +current_head-object.sha1, +0, !!current_head)) { ...but here you dereference current_head without checking it first. It upsets me that the test suite didn't catch this NULL pointer dereference. Either 1. current_head cannot in fact be NULL, in which case the commit message should explain that fact and the code should be simplified or 2. the test suite is incomplete. If so, it would be great if you would add a test that exercises this branch of the code (and catches your error), and then fix the error. rollback_index_files(); die(_(cannot update HEAD ref)); } + if (ref_transaction_commit(transaction, sb.buf, +UPDATE_REFS_QUIET_ON_ERR)) { + rollback_index_files(); + die(_(cannot commit HEAD ref)); + } unlink(git_path(CHERRY_PICK_HEAD)); unlink(git_path(REVERT_HEAD)); Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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 07/11] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/commit.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/builtin/commit.c b/builtin/commit.c index d9550c5..b8e4389 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1541,11 +1541,11 @@ 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; if (argc == 2 !strcmp(argv[1], -h)) usage_with_options(builtin_commit_usage, builtin_commit_options); @@ -1667,12 +1667,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); - nl = strchr(sb.buf, '\n'); if (nl) strbuf_setlen(sb, nl + 1 - sb.buf); @@ -1681,14 +1675,22 @@ 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 (!ref_lock) { + transaction = ref_transaction_begin(); + if (!transaction) { rollback_index_files(); - die(_(cannot lock HEAD ref)); + die(_(HEAD: cannot start transaction)); } - if (write_ref_sha1(ref_lock, sha1, sb.buf) 0) { + if (ref_transaction_update(transaction, HEAD, sha1, + current_head-object.sha1, + 0, !!current_head)) { rollback_index_files(); die(_(cannot update HEAD ref)); } + if (ref_transaction_commit(transaction, sb.buf, + UPDATE_REFS_QUIET_ON_ERR)) { + rollback_index_files(); + die(_(cannot commit HEAD ref)); + } unlink(git_path(CHERRY_PICK_HEAD)); unlink(git_path(REVERT_HEAD)); -- 1.9.1.513.gd486896 -- 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