commit: safer commit creation with reference update The current version of the commit creation and amend function are unsafe to use when passing the update_ref parameter, as they do not check that the reference at the moment of update points to what the user expects. Make sure that we're moving history forward when we ask the library to update the reference for us by checking that the first parent of the new commit is the current value of the reference. We also make sure that the ref we're updating hasn't moved between the read and the write. Similarly, when amending a commit, make sure that the current tip of the branch is the commit we're amending.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 153 154 155 156 157 158 159 160 161 162 163 164 165 166 167 168 169 170 171 172 173 174 175 176 177 178 179 180 181 182 183 184 185 186 187 188 189 190 191 192 193 194 195 196 197 198 199 200 201 202 203 204 205 206 207 208 209 210 211 212 213 214 215 216 217 218 219 220 221
diff --git a/include/git2/commit.h b/include/git2/commit.h
index 834330b..fb53a70 100644
--- a/include/git2/commit.h
+++ b/include/git2/commit.h
@@ -254,7 +254,8 @@ GIT_EXTERN(int) git_commit_nth_gen_ancestor(
* is not direct, it will be resolved to a direct reference.
* Use "HEAD" to update the HEAD of the current branch and
* make it point to this commit. If the reference doesn't
- * exist yet, it will be created.
+ * exist yet, it will be created. If it does exist, the first
+ * parent must be the tip of this branch.
*
* @param author Signature with author and author time of commit
*
@@ -329,7 +330,7 @@ GIT_EXTERN(int) git_commit_create_v(
*
* The `update_ref` value works as in the regular `git_commit_create()`,
* updating the ref to point to the newly rewritten commit. If you want
- * to amend a commit that is not currently the HEAD of the branch and then
+ * to amend a commit that is not currently the tip of the branch and then
* rewrite the following commits to reach a ref, pass this as NULL and
* update the rest of the commit chain and ref separately.
*
diff --git a/src/commit.c b/src/commit.c
index 255debe..227d5c4 100644
--- a/src/commit.c
+++ b/src/commit.c
@@ -34,6 +34,35 @@ void git_commit__free(void *_commit)
git__free(commit);
}
+static int update_ref_for_commit(git_repository *repo, git_reference *ref, const char *update_ref, const git_oid *id, const git_signature *committer)
+{
+ git_reference *ref2 = NULL;
+ int error;
+ git_commit *c;
+ const char *shortmsg;
+ git_buf reflog_msg = GIT_BUF_INIT;
+
+ if ((error = git_commit_lookup(&c, repo, id)) < 0) {
+ return error;
+ }
+
+ shortmsg = git_commit_summary(c);
+ git_buf_printf(&reflog_msg, "commit%s: %s",
+ git_commit_parentcount(c) == 0 ? " (initial)" : "",
+ shortmsg);
+ git_commit_free(c);
+
+ if (ref) {
+ error = git_reference_set_target(&ref2, ref, id, committer, git_buf_cstr(&reflog_msg));
+ git_reference_free(ref2);
+ } else {
+ error = git_reference__update_terminal(repo, update_ref, id, committer, git_buf_cstr(&reflog_msg));
+ }
+
+ git_buf_free(&reflog_msg);
+ return error;
+}
+
int git_commit_create_from_callback(
git_oid *id,
git_repository *repo,
@@ -46,6 +75,9 @@ int git_commit_create_from_callback(
git_commit_parent_callback parent_cb,
void *parent_payload)
{
+ git_reference *ref = NULL;
+ int error = 0, matched_parent = 0;
+ const git_oid *current_id = NULL;
git_buf commit = GIT_BUF_INIT;
size_t i = 0;
git_odb *odb;
@@ -53,10 +85,31 @@ int git_commit_create_from_callback(
assert(id && repo && tree && parent_cb);
+ if (update_ref) {
+ error = git_reference_lookup_resolved(&ref, repo, update_ref, 10);
+ if (error < 0 && error != GIT_ENOTFOUND)
+ return error;
+ }
+ giterr_clear();
+
+ if (ref)
+ current_id = git_reference_target(ref);
+
git_oid__writebuf(&commit, "tree ", tree);
- while ((parent = parent_cb(i++, parent_payload)) != NULL)
+ while ((parent = parent_cb(i, parent_payload)) != NULL) {
git_oid__writebuf(&commit, "parent ", parent);
+ if (i == 0 && current_id && git_oid_equal(current_id, parent))
+ matched_parent = 1;
+ i++;
+ }
+
+ if (ref && !matched_parent) {
+ git_reference_free(ref);
+ git_buf_free(&commit);
+ giterr_set(GITERR_OBJECT, "failed to create commit: current tip is not the first parent");
+ return GIT_EMODIFIED;
+ }
git_signature__writebuf(&commit, "author ", author);
git_signature__writebuf(&commit, "committer ", committer);
@@ -78,24 +131,8 @@ int git_commit_create_from_callback(
git_buf_free(&commit);
if (update_ref != NULL) {
- int error;
- git_commit *c;
- const char *shortmsg;
- git_buf reflog_msg = GIT_BUF_INIT;
-
- if (git_commit_lookup(&c, repo, id) < 0)
- goto on_error;
-
- shortmsg = git_commit_summary(c);
- git_buf_printf(&reflog_msg, "commit%s: %s",
- git_commit_parentcount(c) == 0 ? " (initial)" : "",
- shortmsg);
- git_commit_free(c);
-
- error = git_reference__update_terminal(repo, update_ref, id,
- committer, git_buf_cstr(&reflog_msg));
-
- git_buf_free(&reflog_msg);
+ error = update_ref_for_commit(repo, ref, update_ref, id, committer);
+ git_reference_free(ref);
return error;
}
@@ -242,6 +279,8 @@ int git_commit_amend(
{
git_repository *repo;
git_oid tree_id;
+ git_reference *ref;
+ int error;
assert(id && commit_to_amend);
@@ -266,9 +305,27 @@ int git_commit_amend(
git_oid_cpy(&tree_id, git_tree_id(tree));
}
- return git_commit_create_from_callback(
- id, repo, update_ref, author, committer, message_encoding, message,
+ if (update_ref) {
+ if ((error = git_reference_lookup_resolved(&ref, repo, update_ref, 5)) < 0)
+ return error;
+
+ if (git_oid_cmp(git_commit_id(commit_to_amend), git_reference_target(ref))) {
+ git_reference_free(ref);
+ giterr_set(GITERR_REFERENCE, "commit to amend is not the tip of the given branch");
+ return -1;
+ }
+ }
+
+ error = git_commit_create_from_callback(
+ id, repo, NULL, author, committer, message_encoding, message,
&tree_id, commit_parent_for_amend, (void *)commit_to_amend);
+
+ if (!error && update_ref) {
+ error = update_ref_for_commit(repo, ref, NULL, id, committer);
+ git_reference_free(ref);
+ }
+
+ return error;
}
int git_commit__parse(void *_commit, git_odb_object *odb_obj)
diff --git a/tests/commit/commit.c b/tests/commit/commit.c
index 38397d2..fa181b7 100644
--- a/tests/commit/commit.c
+++ b/tests/commit/commit.c
@@ -38,6 +38,10 @@ void test_commit_commit__create_unexisting_update_ref(void)
cl_git_pass(git_commit_create(&oid, _repo, "refs/heads/foo/bar", s, s,
NULL, "some msg", tree, 1, (const git_commit **) &commit));
+ /* fail because the parent isn't the tip of the branch anymore */
+ cl_git_fail(git_commit_create(&oid, _repo, "refs/heads/foo/bar", s, s,
+ NULL, "some msg", tree, 1, (const git_commit **) &commit));
+
cl_git_pass(git_reference_lookup(&ref, _repo, "refs/heads/foo/bar"));
cl_assert(!git_oid_cmp(&oid, git_reference_target(ref)));
diff --git a/tests/object/commit/commitstagedfile.c b/tests/object/commit/commitstagedfile.c
index 3e7b3c0..9758ea9 100644
--- a/tests/object/commit/commitstagedfile.c
+++ b/tests/object/commit/commitstagedfile.c
@@ -175,6 +175,10 @@ void test_object_commit_commitstagedfile__amend_commit(void)
cl_git_pass(git_commit_amend(
&new_oid, old_commit, "HEAD", NULL, NULL, NULL, "Initial commit", NULL));
+ /* fail because the commit isn't the tip of the branch anymore */
+ cl_git_fail(git_commit_amend(
+ &new_oid, old_commit, "HEAD", NULL, NULL, NULL, "Initial commit", NULL));
+
cl_git_pass(git_commit_lookup(&new_commit, repo, &new_oid));
cl_assert_equal_i(0, git_commit_parentcount(new_commit));
@@ -182,6 +186,7 @@ void test_object_commit_commitstagedfile__amend_commit(void)
assert_commit_is_head(new_commit);
git_commit_free(old_commit);
+
old_commit = new_commit;
/* let's amend the tree of that last commit */
@@ -192,6 +197,10 @@ void test_object_commit_commitstagedfile__amend_commit(void)
cl_git_pass(git_tree_lookup(&tree, repo, &tree_oid));
cl_assert_equal_i(2, git_tree_entrycount(tree));
+ /* fail to amend on a ref which does not exist */
+ cl_git_fail_with(GIT_ENOTFOUND, git_commit_amend(
+ &new_oid, old_commit, "refs/heads/nope", NULL, NULL, NULL, "Initial commit", tree));
+
cl_git_pass(git_commit_amend(
&new_oid, old_commit, "HEAD", NULL, NULL, NULL, "Initial commit", tree));
git_tree_free(tree);