Re: [PATCH 1/4] git-cherry-pick: add keep-empty option

From: Jeff King <peff@peff.net>
Date: Mar 30, 2012, 21:15 UTC
Message-ID: <20120330211513.GB20734@sigill.intra.peff.net>
In-Reply-To: <1333136922-12872-2-git-send-email-nhorman@tuxdriver.com>

On Fri, Mar 30, 2012 at 03:48:39PM -0400, Neil Horman wrote:

Show 6 quoted lines

> +--keep-empty:
> +	If a commit is not a fast forward, or if fast forwarding is not allowed,
> +	cherry-picking an empty commit will fail, indicating that an explicit
> +	invokation of git commit --allow-empty is required.  This option
> +	overrides that behavior, allowing empty commits to be preserved
> +	automatically in a cherry-pick

This didn't parse very well for me. A commit cannot be "a fast forward" by itself. Fast-forwarding is an operation that depends on the relationship between commits.

I think what you are trying to say is that this option is used only if the "--ff" logic does not kick in. Maybe it would be clearer to get to the point early, and mention --ff later, like:

  --keep-empty:
          By default, cherry-picking an empty commit will fail,
          indicating that an explicit invocation of `git commit
          --allow-empty` is required. This option overrides that
          behavior, allowing empty commits to be preserved automatically
          in a cherry-pick. Note that when "--ff" is in effect, empty
          commits that meet the "fast-forward" requirement will be kept
          even without this option.

Like Junio, I agree this should simply be called --allow-empty.

Show 13 quoted lines

> diff --git a/sequencer.c b/sequencer.c
> index a37846a..71929ba 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -260,8 +260,8 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>   */
>  static int run_git_commit(const char *defmsg, struct replay_opts *opts)
>  {
> -	/* 6 is max possible length of our args array including NULL */
> -	const char *args[6];
> +	/* 7 is max possible length of our args array including NULL */
> +	const char *args[7];
>  	int i = 0;

It might be nice to refactor this to use argv_array, which handles the allocation automatically.

> +	if (opts->allow_empty)
> +		args[i++] = "--allow-empty";
> +

What happens if I cherry-pick a commit that is not empty, but that becomes empty because its changes have already been applied?

-Peff