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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 10, 2012, 17:04 UTC
Message-ID: <7vy5q3pl9i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1334072868-9435-3-git-send-email-nhorman@tuxdriver.com>

Neil Horman <nhorman@tuxdriver.com> writes:

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

OK.

Show 39 quoted lines

> 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);

Show 12 quoted lines

> +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):

	const unsigned char *ptree_sha1;

	parse_commit(commit);
        if (commit->parents) {
		struct commit *parent = commit->parents->item;
                parse_commit(parent);
                ptree_sha1 = parent->tree->object.sha1;
	} else {
        	ptree_sha1 = EMPTY_TREE_SHA1_BIN; /* commit is root */
        }
	return hashcmp(ptree_sha1, commit->tree->object.sha1);

The higher code structure of this patch looks good, but the lower level implementation details are done way too inefficiently.