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

From: Clemens Buchacher <drizzd@aon.at>
Date: Apr 15, 2012, 10:42 UTC
Message-ID: <20120415104212.GC6263@ecki>
In-Reply-To: <1334342707-3326-3-git-send-email-nhorman@tuxdriver.com>

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?

Show 28 quoted lines

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