Re: [PATCH 07/11] commit.c: use ref transactions for updates
From: Michael Haggerty <hidden>
Date: 2016-06-15 23:00:44
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote:
quoted hunk ↗ jump to hunk
Change commit.c to use ref transactions for all ref updates. Signed-off-by: Ronnie Sahlberg <redacted> --- 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...
quoted hunk ↗ jump to hunk
- 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 mhagger@alum.mit.edu http://softwareswirl.blogspot.com/