Re: [PATCH v3 2/4] git-cherry-pick: Add keep-redundant-commits option

On Tue, Apr 10, 2012 at 10:04:32AM -0700, Junio C Hamano wrote:

Neil Horman [off-list ref] writes:

quoted

+	/* keep_if_made_empty implies allow_empty */
+	if (opts->keep_if_made_empty)
+		opts->allow_empty = 1;
+

 
OK.

quoted

diff --git a/sequencer.c b/sequencer.c
index 71929ba..5d033db 100644
--- a/sequencer.c
+++ b/sequencer.c

@@ -13,6 +13,7 @@
 #include "rerere.h"
 #include "merge-recursive.h"
 #include "refs.h"
+#include "argv-array.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 

@@ -258,26 +259,102 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
  * If we are revert, or if our cherry-pick results in a hand merge,
  * we had better say that the current user is responsible for that.
  */
-static int run_git_commit(const char *defmsg, struct replay_opts *opts)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts, int empty)
 {
-	/* 7 is max possible length of our args array including NULL */
-	const char *args[7];
-	int i = 0;
+	struct argv_array array;
+	int rc;
+
+	if (!empty && !opts->keep_if_made_empty) {
+		const char *argv[] = { "diff-index", "--quiet", "--exit-code",
+					"--cached", "HEAD", NULL };
+
+		/*
+ 		 * If we run git diff-index with the above option and it returns
+ 		 * zero, then there have been no changes made to the index by
+ 		 * this patch, i.e. its empty.  Since our previous empty test
+ 		 * indicated that this patch was not created empty, its been made
+ 		 * redundant.  Since keep_if_made_empty is not set, we just skip
+ 		 * it
+ 		 */
+		if (run_command_v_opt(argv, RUN_GIT_CMD) == 0)
+			return 0;

Wouldn't it be far simpler to do this without forking diff-index?  I
haven't followed the codepath leading to this, but it should be very easy
to find a commit object that corresponds to the current HEAD and peek the
tree object in it to find out the current tree, and because the original
function is going to run an as-is "git commit", the tree you are going to
commit should be available by calling cache_tree_update() like write-tree
does.  If they match, you are trying to create an empty commit.  E.g. (error
checking elided):

	unsigned char head_sha1[20];
	struct commit *head_commit;

	resolve_ref_unsafe("HEAD", head_sha1, 1, NULL);
        head_commit = lookup_commit(head_sha1);
        parse_commit(head_commit);

        if (!cache_tree_fully_valid(active_cache_tree))
		cache_tree_update(active_cache_tree, active_cache,
				active_nr, 0);
	return hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);

quoted

+static int is_original_commit_empty(struct commit *commit)
+{
+	struct argv_array argv_array;
+	struct child_process cp;
+	char ptree[40], pptree[40];
+	int ret = 0;
+
+	argv_array_init(&argv_array);
+	memset(&cp, 0, sizeof(struct child_process));
+
+	argv_array_push(&argv_array, "rev-parse");
+	argv_array_pushf(&argv_array, "%s^{tree}", sha1_to_hex(commit->object.sha1));

Likewise.  You have the commit object, so just make sure it was parsed,
and peek its tree object.  Also do the same for its parent commit.  Now
you have two trees, so you can compare their object names.  E.g. (error
checking elided):

Honestly, I wasn't aware there was a faster way.  In my last version you
suggested using git diff-index and git rev-parse (allbeit the latter was for use
in the git-rebase script, and I just reused it).  Regardless, I was going with
your previous suggestions.  I can re-do these to use these new suggestions.

Neil