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

[Neil Horman <nhorman@tuxdriver.com>]

On Sun, Apr 15, 2012 at 12:42:12PM +0200, Clemens Buchacher wrote:
> On Fri, Apr 13, 2012 at 02:45:05PM -0400, Neil Horman wrote:
> >
> > +                     OPT_BOOLEAN(0, "keep-redundant-commits", &opts->keep_if_made_empty, "keep redundant, empty commits"),
> 
> For consistency, I'd prefer if the variable name were the same as the
> option. Then I wouldn't have to keep the option<->variable translation
> in mind.
> 
Ok

> >
> > +     if (!empty && !opts->keep_if_made_empty) {
> [...]
> > +                     return 0;
> [...]
> >       if (opts->allow_empty)
> > +             argv_array_push(&array, "--allow-empty");
> > +
> > +     rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> 
> I find it a bit strange, that if we cherry-pick a commit that was
> already empty, we _do_ call git commit (and error out), but if we find a
> commit that is made empty, we do _not_ call git commit and quietly
> succeed (in not doing anything). But I suppose that is the legacy
> behavior?
> 
Correct, more or less.  The legacy behavior is to call git commit unilaterally.
With this change, we still do that, but we pass --allow-empty to the commit
command, allowing it to preserve the empty commit.  If we specify
keep-redundant-commits, we skip the check to see if the new commit is empty and
create the new (now empty) commit.  The only change is that if we do not specify
keep-redundant-commits, we check to see if the commit is made empty in
git-cherry-pick and skip it if it is.  We could, instead of returning prior to
calling git-commit, use that test to override the keep_empty option below, so
that we don't pass --allow-empty to git-commit instead.  That would preserve the
prior code path, but for no real advatage, as the outcome is the same, and this
way saves us having to fork the git-commit command, which I think is
adventageous.

> > +
> > +		rc = !hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1);
> > +
> > +		if (rc)
> 
> rc is short for run_command, for which it stores the return value, no?
> Let's not abuse the variable like this and instead use the result
> directly:
> 
No.  rc is short for return code, generically used to represent the value
to be returned from a function. Its used in this fashion in any number of
places, both in git and any other project. 

>  if (!hashcmp(active_cache_tree->sha1, head_commit->tree->object.sha1))
> 
> If you make the entire paragraph leading up to this a separate function,
> say index_is_empty(), then the above would read more naturally like
> this:
> 
>  if (!opts->keep_if_made_empty && !empty && index_is_empty())
> 
> > +			/*
> > +			 * The head tree and the parent tree match
> > +			 * meaning the commit is empty.  Since it wasn't created
> 
> Don't you mean the head tree and the index?
> 
yeah.

> > +			 * empty (based on the previous test), we can conclude
> > +			 * the commit has been made redundant.  Since we don't
> > +			 * want to keep redundant commits, just skip this one
> > +			 */
> > +			return 0;
> > +	}
> > +
> > +	argv_array_init(&array);
> > +	argv_array_push(&array, "commit");
> > +	argv_array_push(&array, "-n");
> >  
> > -	args[i++] = "commit";
> > -	args[i++] = "-n";
> >  	if (opts->signoff)
> > -		args[i++] = "-s";
> > +		argv_array_push(&array, "-s");
> >  	if (!opts->edit) {
> > -		args[i++] = "-F";
> > -		args[i++] = defmsg;
> > +		argv_array_push(&array, "-F");
> > +		argv_array_push(&array, defmsg);
> >  	}
> > +
> >  	if (opts->allow_empty)
> > -		args[i++] = "--allow-empty";
> > +		argv_array_push(&array, "--allow-empty");
> > +
> > +
> 
> Why two newlines?
> 
> > +	rc = run_command_v_opt(array.argv, RUN_GIT_CMD);
> > +	argv_array_clear(&array);
> > +	return rc;
> > +}
> 
> Looks good to me otherwise.
>