Single callback for commit signing in rebase w/ git_buf

Reduces the number of callbacks for signing a commit during a rebase operation to just one callback. That callback has 2 out git_buf parameters for signature and signature field. We use git_buf here, because we cannot make any assumptions about the heap allocator a user of the library might be using.

include/git2/rebase.h

@@ -27,8 +27,8 @@ GIT_BEGIN_DECL
  * Rebase commit signature callback.
  *
  * The callback will be called with the commit content, giving a user an
- * opportunity to sign the commit content in a rebase.
- * The signature parameter will be owned by LibGit2 after this callback returns.
+ * opportunity to sign the commit content in a rebase. The signature_field
+ * buf may be left empty to specify the default field.
  *
  * When the callback:
  * - returns GIT_PASSTHROUGH, no signature will be added to the commit.
@@ -36,22 +36,7 @@ GIT_BEGIN_DECL
  * - returns GIT_OK, the signature parameter is expected to be filled.
  */
 typedef int (*git_rebase_commit_signature_cb)(
-	char **signature, const char *commit_content, void *payload);
-
-/**
- * Rebase commit signature field callback.
- *
- * The callback will be called if a signature_cb was called and successful.
- * This callback will provide the field that a user is signing in a git_rebase_commit.
- * The signature_field parameter will be owned by LibGit2 after this callback returns.
- *
- * When the callback:
- * - returns GIT_PASSTHROUGH, signature_field is expected to remain null.
- * - returns < 0, git_rebase_commit will be aborted.
- * - returns GIT_OK, the signature_field parameter is expected to be filled.
- */
-typedef int (*git_rebase_commit_signature_field_cb)(
-	char **signature_field, void *payload);
+	git_buf *signature, git_buf *signature_field, const char *commit_content, void *payload);
 
 /**
  * Rebase options
@@ -113,13 +98,6 @@ typedef struct {
 	git_rebase_commit_signature_cb signature_cb;
 
 	/**
-	 * If provided and the signature_cb is provided, this will be called asking
-	 * for the field to write the signature to. Can be skipped with GIT_PASSTHROUGH.
-	 * This field is only used when performing git_rebase_commit.
-	 */
-	git_rebase_commit_signature_field_cb signature_field_cb;
-
-	/**
 	 * This will be passed to each of the callbacks in this struct
 	 * as the last parameter.
 	 */
@@ -170,7 +148,7 @@ typedef enum {
 #define GIT_REBASE_OPTIONS_VERSION 1
 #define GIT_REBASE_OPTIONS_INIT \
 	{ GIT_REBASE_OPTIONS_VERSION, 0, 0, NULL, GIT_MERGE_OPTIONS_INIT, \
-	  GIT_CHECKOUT_OPTIONS_INIT, 0, 0, NULL }
+	  GIT_CHECKOUT_OPTIONS_INIT, 0, NULL }
 
 /** Indicates that a rebase operation is not (yet) in progress. */
 #define GIT_REBASE_NO_OPERATION SIZE_MAX

src/rebase.c

@@ -945,8 +945,9 @@ static int rebase_commit__create(
 	git_commit *current_commit = NULL, *commit = NULL;
 	git_tree *parent_tree = NULL, *tree = NULL;
 	git_oid tree_id, commit_id;
-	git_buf commit_content = GIT_BUF_INIT;
-	char *signature = NULL, *signature_field = NULL;
+	git_buf commit_content = GIT_BUF_INIT, commit_signature = GIT_BUF_INIT,
+		signature_field = GIT_BUF_INIT;
+	const char *signature_field_as_string = NULL;
 	int error;
 
 	operation = git_array_get(rebase->operations, rebase->current);
@@ -985,21 +986,22 @@ static int rebase_commit__create(
 				message_encoding, message, tree, 1, (const git_commit **)&parent_commit)) < 0)
 			goto done;
 
-		if ((error = rebase->options.signature_cb(&signature, git_buf_cstr(&commit_content),
-				rebase->options.payload)) < 0 && error != GIT_PASSTHROUGH)
+		if ((error = rebase->options.signature_cb(&commit_signature, &signature_field,
+				git_buf_cstr(&commit_content), rebase->options.payload)) < 0 &&
+				error != GIT_PASSTHROUGH)
 			goto done;
 
 		if (error != GIT_PASSTHROUGH) {
-			if (rebase->options.signature_field_cb &&
-				(error = rebase->options.signature_field_cb(&signature_field, rebase->options.payload)) < 0) {
-				if (error == GIT_PASSTHROUGH)
-					assert(signature_field == NULL);
-				else
-					goto done;
+			if (git_buf_is_allocated(&signature_field)) {
+				assert(git_buf_contains_nul(&signature_field));
+				signature_field_as_string = git_buf_cstr(&signature_field);
 			}
 
+			assert(git_buf_is_allocated(&commit_signature));
+			assert(git_buf_contains_nul(&commit_signature));
 			if ((error = git_commit_create_with_signature(&commit_id, rebase->repo,
-					git_buf_cstr(&commit_content), signature, signature_field)) < 0)
+					git_buf_cstr(&commit_content), git_buf_cstr(&commit_signature),
+					signature_field_as_string)))
 				goto done;
 		}
 	}
@@ -1019,10 +1021,8 @@ done:
 	if (error < 0)
 		git_commit_free(commit);
 
-	if (signature)
-		free(signature);
-	if (signature_field)
-		free(signature_field);
+	git_buf_dispose(&commit_signature);
+	git_buf_dispose(&signature_field);
 	git_buf_dispose(&commit_content);
 	git_commit_free(current_commit);
 	git_tree_free(parent_tree);

tests/rebase/sign.c

@@ -25,9 +25,14 @@ committer Rebaser <rebaser@rebaser.rb> 1405694510 +0000\n\
 \n\
 Modification 3 to gravy\n";
 
-int signature_cb_passthrough(char **signature, const char *commit_content, void *payload)
+int signature_cb_passthrough(
+  git_buf *signature,
+  git_buf *signature_field,
+  const char *commit_content,
+  void *payload)
 {
-  cl_assert_equal_p(NULL, *signature);
+  cl_assert_equal_b(false, git_buf_is_allocated(signature));
+  cl_assert_equal_b(false, git_buf_is_allocated(signature_field));
   cl_assert_equal_s(expected_commit_content, commit_content);
   cl_assert_equal_p(NULL, payload);
   return GIT_PASSTHROUGH;
@@ -79,7 +84,11 @@ committer Rebaser <rebaser@rebaser.rb> 1405694510 +0000\n";
   git_rebase_free(rebase);
 }
 
-int signature_cb_gpg(char **signature, const char *commit_content, void *payload)
+int signature_cb_gpg(
+  git_buf *signature,
+  git_buf *signature_field,
+  const char *commit_content,
+  void *payload)
 {
   const char *gpg_signature = "-----BEGIN PGP SIGNATURE-----\n\
 \n\
@@ -98,22 +107,17 @@ cttVRsdOoego+fiy08eFE+aJIeYiINRGhqOBTsuqG4jIdpdKxPE=\n\
 =KbsY\n\
 -----END PGP SIGNATURE-----";
 
+  cl_assert_equal_b(false, git_buf_is_allocated(signature));
+  cl_assert_equal_b(false, git_buf_is_allocated(signature_field));
   cl_assert_equal_s(expected_commit_content, commit_content);
   cl_assert_equal_p(NULL, payload);
 
-  *signature = strdup(gpg_signature);
+  cl_git_pass(git_buf_set(signature, gpg_signature, strlen(gpg_signature) + 1));
   return GIT_OK;
 }
 
-int signature_field_cb_passthrough(char **signature_field, void *payload)
-{
-  cl_assert_equal_p(NULL, *signature_field);
-  cl_assert_equal_p(NULL, payload);
-  return GIT_PASSTHROUGH;
-}
-
 /* git checkout gravy ; git rebase --merge veal */
-void test_gpg_signature(bool use_passthrough)
+void test_rebase_sign__gpg_with_no_field(void)
 {
   git_rebase *rebase;
   git_reference *branch_ref, *upstream_ref;
@@ -145,8 +149,6 @@ gpgsig -----BEGIN PGP SIGNATURE-----\n\
  -----END PGP SIGNATURE-----\n";
 
   rebase_opts.signature_cb = signature_cb_gpg;
-  if (use_passthrough)
-    rebase_opts.signature_field_cb = signature_field_cb_passthrough;
 
   cl_git_pass(git_reference_lookup(&branch_ref, repo, "refs/heads/gravy"));
   cl_git_pass(git_reference_lookup(&upstream_ref, repo, "refs/heads/veal"));
@@ -176,30 +178,26 @@ gpgsig -----BEGIN PGP SIGNATURE-----\n\
   git_rebase_free(rebase);
 }
 
-/* git checkout gravy ; git rebase --merge veal */
-void test_rebase_sign__gpg_with_no_field_cb(void)
-{
-  test_gpg_signature(false);
-}
 
-/* git checkout gravy ; git rebase --merge veal */
-void test_rebase_sign__gpg_with_passthrough_field_cb(void)
+int signature_cb_magic_field(
+  git_buf *signature,
+  git_buf *signature_field,
+  const char *commit_content,
+  void *payload)
 {
-  test_gpg_signature(true);
-}
+  const char *signature_content = "magic word: pretty please";
+  const char * signature_field_content = "magicsig";
 
-int signature_cb_magic_field(char **signature, const char *commit_content, void *payload)
-{
+  cl_assert_equal_b(false, git_buf_is_allocated(signature));
+  cl_assert_equal_b(false, git_buf_is_allocated(signature_field));
   cl_assert_equal_s(expected_commit_content, commit_content);
   cl_assert_equal_p(NULL, payload);
-  *signature = strdup("magic word: pretty please");
-  return GIT_OK;
-}
 
-int signature_field_cb_magic_field(char **signature_field, void *payload)
-{
-  cl_assert_equal_p(NULL, payload);
-  *signature_field = strdup("magicsig");
+  cl_git_pass(git_buf_set(signature, signature_content,
+    strlen(signature_content) + 1));
+  cl_git_pass(git_buf_set(signature_field, signature_field_content,
+    strlen(signature_field_content) + 1));
+
   return GIT_OK;
 }
 
@@ -221,7 +219,6 @@ committer Rebaser <rebaser@rebaser.rb> 1405694510 +0000\n\
 magicsig magic word: pretty please\n";
 
   rebase_opts.signature_cb = signature_cb_magic_field;
-  rebase_opts.signature_field_cb = signature_field_cb_magic_field;
 
   cl_git_pass(git_reference_lookup(&branch_ref, repo, "refs/heads/gravy"));
   cl_git_pass(git_reference_lookup(&upstream_ref, repo, "refs/heads/veal"));