Merge pull request #2056 from libgit2/rb/commit-amend

Add git_commit_amend API

include/git2/commit.h

@@ -242,8 +242,8 @@ GIT_EXTERN(int) git_commit_nth_gen_ancestor(
 /**
  * Create new commit in the repository from a list of `git_object` pointers
  *
- * The message will not be cleaned up automatically. You can do that with
+ * The message will **not** be cleaned up automatically. You can do that
- * the `git_message_prettify()` function.
+ * with the `git_message_prettify()` function.
  *
  * @param id Pointer in which to store the OID of the newly created commit
  *
@@ -291,20 +291,20 @@ GIT_EXTERN(int) git_commit_create(
 	const char *message_encoding,
 	const char *message,
 	const git_tree *tree,
-	int parent_count,
+	size_t parent_count,
 	const git_commit *parents[]);
 
 /**
  * Create new commit in the repository using a variable argument list.
  *
- * The message will be cleaned up from excess whitespace and it will be made
+ * The message will **not** be cleaned up automatically. You can do that
- * sure that the last line ends with a '\n'.
+ * with the `git_message_prettify()` function.
  *
  * The parents for the commit are specified as a variable list of pointers
  * to `const git_commit *`. Note that this is a convenience method which may
  * not be safe to export for certain languages or compilers
  *
- * All other parameters remain the same at `git_commit_create()`.
+ * All other parameters remain the same as `git_commit_create()`.
  *
  * @see git_commit_create
  */
@@ -317,9 +317,40 @@ GIT_EXTERN(int) git_commit_create_v(
 	const char *message_encoding,
 	const char *message,
 	const git_tree *tree,
-	int parent_count,
+	size_t parent_count,
 	...);
 
+/**
+ * Amend an existing commit by replacing only non-NULL values.
+ *
+ * This creates a new commit that is exactly the same as the old commit,
+ * except that any non-NULL values will be updated.  The new commit has
+ * the same parents as the old commit.
+ *
+ * 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
+ * rewrite the following commits to reach a ref, pass this as NULL and
+ * update the rest of the commit chain and ref separately.
+ *
+ * Unlike `git_commit_create()`, the `author`, `committer`, `message`,
+ * `message_encoding`, and `tree` parameters can be NULL in which case this
+ * will use the values from the original `commit_to_amend`.
+ *
+ * All parameters have the same meanings as in `git_commit_create()`.
+ *
+ * @see git_commit_create
+ */
+GIT_EXTERN(int) git_commit_amend(
+	git_oid *id,
+	const git_commit *commit_to_amend,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_tree *tree);
+
 /** @} */
 GIT_END_DECL
 #endif

include/git2/sys/commit.h

@@ -21,16 +21,18 @@
 GIT_BEGIN_DECL
 
 /**
- * Create new commit in the repository from a list of `git_oid` values
+ * Create new commit in the repository from a list of `git_oid` values.
  *
  * See documentation for `git_commit_create()` for information about the
  * parameters, as the meaning is identical excepting that `tree` and
  * `parents` now take `git_oid`.  This is a dangerous API in that nor
  * the `tree`, neither the `parents` list of `git_oid`s are checked for
  * validity.
+ *
+ * @see git_commit_create
  */
 GIT_EXTERN(int) git_commit_create_from_ids(
-	git_oid *oid,
+	git_oid *id,
 	git_repository *repo,
 	const char *update_ref,
 	const git_signature *author,
@@ -38,9 +40,41 @@ GIT_EXTERN(int) git_commit_create_from_ids(
 	const char *message_encoding,
 	const char *message,
 	const git_oid *tree,
-	int parent_count,
+	size_t parent_count,
 	const git_oid *parents[]);
 
+/**
+ * Callback function to return parents for commit.
+ *
+ * This is invoked with the count of the number of parents processed so far
+ * along with the user supplied payload.  This should return a git_oid of
+ * the next parent or NULL if all parents have been provided.
+ */
+typedef const git_oid *(*git_commit_parent_callback)(size_t idx, void *payload);
+
+/**
+ * Create a new commit in the repository with an callback to supply parents.
+ *
+ * See documentation for `git_commit_create()` for information about the
+ * parameters, as the meaning is identical excepting that `tree` takes a
+ * `git_oid` and doesn't check for validity, and `parent_cb` is invoked
+ * with `parent_payload` and should return `git_oid` values or NULL to
+ * indicate that all parents are accounted for.
+ *
+ * @see git_commit_create
+ */
+GIT_EXTERN(int) git_commit_create_from_callback(
+	git_oid *id,
+	git_repository *repo,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_oid *tree,
+	git_commit_parent_callback parent_cb,
+	void *parent_payload);
+
 /** @} */
 GIT_END_DECL
 #endif

src/commit.c

@@ -36,41 +36,8 @@ void git_commit__free(void *_commit)
 	git__free(commit);
 }
 
-int git_commit_create_v(
+int git_commit_create_from_callback(
-	git_oid *oid,
+	git_oid *id,
-	git_repository *repo,
-	const char *update_ref,
-	const git_signature *author,
-	const git_signature *committer,
-	const char *message_encoding,
-	const char *message,
-	const git_tree *tree,
-	int parent_count,
-	...)
-{
-	va_list ap;
-	int i, res;
-	const git_commit **parents;
-
-	parents = git__malloc(parent_count * sizeof(git_commit *));
-	GITERR_CHECK_ALLOC(parents);
-
-	va_start(ap, parent_count);
-	for (i = 0; i < parent_count; ++i)
-		parents[i] = va_arg(ap, const git_commit *);
-	va_end(ap);
-
-	res = git_commit_create(
-		oid, repo, update_ref, author, committer,
-		message_encoding, message,
-		tree, parent_count, parents);
-
-	git__free((void *)parents);
-	return res;
-}
-
-int git_commit_create_from_ids(
-	git_oid *oid,
 	git_repository *repo,
 	const char *update_ref,
 	const git_signature *author,
@@ -78,19 +45,20 @@ int git_commit_create_from_ids(
 	const char *message_encoding,
 	const char *message,
 	const git_oid *tree,
-	int parent_count,
+	git_commit_parent_callback parent_cb,
-	const git_oid *parents[])
+	void *parent_payload)
 {
 	git_buf commit = GIT_BUF_INIT;
-	int i;
+	size_t i = 0;
 	git_odb *odb;
+	const git_oid *parent;
 
-	assert(oid && repo && tree && parent_count >= 0);
+	assert(id && repo && tree && parent_cb);
 
 	git_oid__writebuf(&commit, "tree ", tree);
 
-	for (i = 0; i < parent_count; ++i)
+	while ((parent = parent_cb(i++, parent_payload)) != NULL)
-		git_oid__writebuf(&commit, "parent ", parents[i]);
+		git_oid__writebuf(&commit, "parent ", parent);
 
 	git_signature__writebuf(&commit, "author ", author);
 	git_signature__writebuf(&commit, "committer ", committer);
@@ -106,7 +74,7 @@ int git_commit_create_from_ids(
 	if (git_repository_odb__weakptr(&odb, repo) < 0)
 		goto on_error;
 
-	if (git_odb_write(oid, odb, commit.ptr, commit.size, GIT_OBJ_COMMIT) < 0)
+	if (git_odb_write(id, odb, commit.ptr, commit.size, GIT_OBJ_COMMIT) < 0)
 		goto on_error;
 
 	git_buf_free(&commit);
@@ -117,7 +85,7 @@ int git_commit_create_from_ids(
 		const char *shortmsg;
 		git_buf reflog_msg = GIT_BUF_INIT;
 
-		if (git_commit_lookup(&c, repo, oid) < 0)
+		if (git_commit_lookup(&c, repo, id) < 0)
 			goto on_error;
 
 		shortmsg = git_commit_summary(c);
@@ -126,7 +94,7 @@ int git_commit_create_from_ids(
 				shortmsg);
 		git_commit_free(c);
 
-		error = git_reference__update_terminal(repo, update_ref, oid,
+		error = git_reference__update_terminal(repo, update_ref, id,
 				committer, git_buf_cstr(&reflog_msg));
 
 		git_buf_free(&reflog_msg);
@@ -141,8 +109,23 @@ on_error:
 	return -1;
 }
 
-int git_commit_create(
+typedef struct {
-	git_oid *oid,
+	size_t total;
+	va_list args;
+} commit_parent_varargs;
+
+static const git_oid *commit_parent_from_varargs(size_t curr, void *payload)
+{
+	commit_parent_varargs *data = payload;
+	const git_commit *commit;
+	if (curr >= data->total)
+		return NULL;
+	commit = va_arg(data->args, const git_commit *);
+	return commit ? git_commit_id(commit) : NULL;
+}
+
+int git_commit_create_v(
+	git_oid *id,
 	git_repository *repo,
 	const char *update_ref,
 	const git_signature *author,
@@ -150,31 +133,144 @@ int git_commit_create(
 	const char *message_encoding,
 	const char *message,
 	const git_tree *tree,
-	int parent_count,
+	size_t parent_count,
+	...)
+{
+	int error = 0;
+	commit_parent_varargs data;
+
+	assert(tree && git_tree_owner(tree) == repo);
+
+	data.total = parent_count;
+	va_start(data.args, parent_count);
+
+	error = git_commit_create_from_callback(
+		id, repo, update_ref, author, committer,
+		message_encoding, message, git_tree_id(tree),
+		commit_parent_from_varargs, &data);
+
+	va_end(data.args);
+	return error;
+}
+
+typedef struct {
+	size_t total;
+	const git_oid **parents;
+} commit_parent_oids;
+
+static const git_oid *commit_parent_from_ids(size_t curr, void *payload)
+{
+	commit_parent_oids *data = payload;
+	return (curr < data->total) ? data->parents[curr] : NULL;
+}
+
+int git_commit_create_from_ids(
+	git_oid *id,
+	git_repository *repo,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_oid *tree,
+	size_t parent_count,
+	const git_oid *parents[])
+{
+	commit_parent_oids data = { parent_count, parents };
+
+	return git_commit_create_from_callback(
+		id, repo, update_ref, author, committer,
+		message_encoding, message, tree,
+		commit_parent_from_ids, &data);
+}
+
+typedef struct {
+	size_t total;
+	const git_commit **parents;
+	git_repository *repo;
+} commit_parent_data;
+
+static const git_oid *commit_parent_from_array(size_t curr, void *payload)
+{
+	commit_parent_data *data = payload;
+	const git_commit *commit;
+	if (curr >= data->total)
+		return NULL;
+	commit = data->parents[curr];
+	if (git_commit_owner(commit) != data->repo)
+		return NULL;
+	return git_commit_id(commit);
+}
+
+int git_commit_create(
+	git_oid *id,
+	git_repository *repo,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_tree *tree,
+	size_t parent_count,
 	const git_commit *parents[])
 {
-	int retval, i;
+	commit_parent_data data = { parent_count, parents, repo };
-	const git_oid **parent_oids;
 
-	assert(parent_count >= 0);
+	assert(tree && git_tree_owner(tree) == repo);
-	assert(git_object_owner((const git_object *)tree) == repo);
 
-	parent_oids = git__malloc(parent_count * sizeof(git_oid *));
+	return git_commit_create_from_callback(
-	GITERR_CHECK_ALLOC(parent_oids);
+		id, repo, update_ref, author, committer,
+		message_encoding, message, git_tree_id(tree),
+		commit_parent_from_array, &data);
+}
 
-	for (i = 0; i < parent_count; ++i) {
+static const git_oid *commit_parent_for_amend(size_t curr, void *payload)
-		assert(git_object_owner((const git_object *)parents[i]) == repo);
+{
-		parent_oids[i] = git_object_id((const git_object *)parents[i]);
+	const git_commit *commit_to_amend = payload;
+	if (curr >= git_array_size(commit_to_amend->parent_ids))
+		return NULL;
+	return git_array_get(commit_to_amend->parent_ids, curr);
+}
+
+int git_commit_amend(
+	git_oid *id,
+	const git_commit *commit_to_amend,
+	const char *update_ref,
+	const git_signature *author,
+	const git_signature *committer,
+	const char *message_encoding,
+	const char *message,
+	const git_tree *tree)
+{
+	git_repository *repo;
+	git_oid tree_id;
+
+	assert(id && commit_to_amend);
+
+	repo = git_commit_owner(commit_to_amend);
+
+	if (!author)
+		author = git_commit_author(commit_to_amend);
+	if (!committer)
+		committer = git_commit_committer(commit_to_amend);
+	if (!message_encoding)
+		message_encoding = git_commit_message_encoding(commit_to_amend);
+	if (!message)
+		message = git_commit_message(commit_to_amend);
+
+	if (!tree) {
+		git_tree *old_tree;
+		GITERR_CHECK_ERROR( git_commit_tree(&old_tree, commit_to_amend) );
+		git_oid_cpy(&tree_id, git_tree_id(old_tree));
+		git_tree_free(old_tree);
+	} else {
+		assert(git_tree_owner(tree) == repo);
+		git_oid_cpy(&tree_id, git_tree_id(tree));
 	}
 
-	retval = git_commit_create_from_ids(
+	return git_commit_create_from_callback(
-		oid, repo, update_ref, author, committer,
+		id, repo, update_ref, author, committer, message_encoding, message,
-		message_encoding, message,
+		&tree_id, commit_parent_for_amend, (void *)commit_to_amend);
-		git_object_id((const git_object *)tree), parent_count, parent_oids);
-
-	git__free((void *)parent_oids);
-
-	return retval;
 }
 
 int git_commit__parse(void *_commit, git_odb_object *odb_obj)
@@ -314,10 +410,9 @@ const char *git_commit_summary(git_commit *commit)
 				git_buf_putc(&summary, *msg);
 		}
 
-		if (summary.asize == 0)
+		commit->summary = git_buf_detach(&summary);
+		if (!commit->summary)
 			commit->summary = git__strdup("");
-		else
-			commit->summary = git_buf_detach(&summary);
 	}
 
 	return commit->summary;

src/signature.c

@@ -230,6 +230,8 @@ void git_signature__writebuf(git_buf *buf, const char *header, const git_signatu
 	int offset, hours, mins;
 	char sign;
 
+	assert(buf && sig);
+
 	offset = sig->when.offset;
 	sign = (sig->when.offset < 0) ? '-' : '+';
 

tests/object/commit/commitstagedfile.c

@@ -132,3 +132,79 @@ void test_object_commit_commitstagedfile__generate_predictable_object_ids(void)
 	git_tree_free(tree);
 	git_index_free(index);
 }
+
+static void assert_commit_tree_has_n_entries(git_commit *c, int count)
+{
+	git_tree *tree;
+	cl_git_pass(git_commit_tree(&tree, c));
+	cl_assert_equal_i(count, git_tree_entrycount(tree));
+	git_tree_free(tree);
+}
+
+static void assert_commit_is_head_(git_commit *c, const char *file, int line)
+{
+	git_commit *head;
+	cl_git_pass(git_revparse_single((git_object **)&head, repo, "HEAD"));
+	clar__assert(git_oid_equal(git_commit_id(c), git_commit_id(head)), file, line, "Commit is not the HEAD", NULL, 1);
+	git_commit_free(head);
+}
+#define assert_commit_is_head(C) assert_commit_is_head_((C),__FILE__,__LINE__)
+
+void test_object_commit_commitstagedfile__amend_commit(void)
+{
+	git_index *index;
+	git_oid old_oid, new_oid, tree_oid;
+	git_commit *old_commit, *new_commit;
+	git_tree *tree;
+
+	/* make a commit */
+
+	cl_git_mkfile("treebuilder/myfile", "This is a file\n");
+	cl_git_pass(git_repository_index(&index, repo));
+	cl_git_pass(git_index_add_bypath(index, "myfile"));
+	cl_repo_commit_from_index(&old_oid, repo, NULL, 0, "first commit");
+
+	cl_git_pass(git_commit_lookup(&old_commit, repo, &old_oid));
+
+	cl_assert_equal_i(0, git_commit_parentcount(old_commit));
+	assert_commit_tree_has_n_entries(old_commit, 1);
+	assert_commit_is_head(old_commit);
+
+	/* let's amend the message of the HEAD commit */
+
+	cl_git_pass(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));
+	assert_commit_tree_has_n_entries(new_commit, 1);
+	assert_commit_is_head(new_commit);
+
+	git_commit_free(old_commit);
+	old_commit = new_commit;
+
+	/* let's amend the tree of that last commit */
+
+	cl_git_mkfile("treebuilder/anotherfile", "This is another file\n");
+	cl_git_pass(git_index_add_bypath(index, "anotherfile"));
+	cl_git_pass(git_index_write_tree(&tree_oid, index));
+	cl_git_pass(git_tree_lookup(&tree, repo, &tree_oid));
+	cl_assert_equal_i(2, git_tree_entrycount(tree));
+
+	cl_git_pass(git_commit_amend(
+		&new_oid, old_commit, "HEAD", NULL, NULL, NULL, "Initial commit", tree));
+	git_tree_free(tree);
+
+	cl_git_pass(git_commit_lookup(&new_commit, repo, &new_oid));
+
+	cl_assert_equal_i(0, git_commit_parentcount(new_commit));
+	assert_commit_tree_has_n_entries(new_commit, 2);
+	assert_commit_is_head(new_commit);
+
+	/* cleanup */
+
+	git_commit_free(old_commit);
+	git_commit_free(new_commit);
+	git_index_free(index);
+}