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
