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

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Apr 10, 2012 at 10:04:32AM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > +	/* keep_if_made_empty implies allow_empty */
> > +	if (opts->keep_if_made_empty)
> > +		opts->allow_empty = 1;
> > +
>  
> OK.
> 
> > 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);
> 
> > +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