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

From: Clemens Buchacher <[email-masked]>
Date: Sun, 15 Apr 2012 12:42:12 +0200

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.

>
> +     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?

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

 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?

> +			 * 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.