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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 10, 2012, 16:45 UTC
Message-ID: <7v62d7qzu9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1334072868-9435-2-git-send-email-nhorman@tuxdriver.com>

Neil Horman <nhorman@tuxdriver.com> writes:

Show 47 quoted lines

> git cherry-pick fails when picking a non-ff commit that is empty.  The advice
> given with the failure is that a git-commit --allow-empty should be issued to
> explicitly add the empty commit during the cherry pick.  This option allows a
> user to specify before hand that they want to keep the empty commit.  This
> eliminates the need to issue both a cherry pick and a commit operation.
>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> ---
>  Documentation/git-cherry-pick.txt |    9 +++++++++
>  builtin/commit.c                  |    6 +++---
>  builtin/revert.c                  |    2 ++
>  sequencer.c                       |    7 +++++--
>  sequencer.h                       |    1 +
>  5 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index fed5097..730237a 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -103,6 +103,15 @@ effect to your index in a row.
>  	cherry-pick'ed commit, then a fast forward to this commit will
>  	be performed.
>  
> +--allow-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.
> +
>  --strategy=<strategy>::
>  	Use the given merge strategy.  Should only be used once.
>  	See the MERGE STRATEGIES section in linkgit:git-merge[1]
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 3714582..0cd10ab 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -56,10 +56,10 @@ N_("You asked to amend the most recent commit, but doing so would make\n"
>  "remove the commit entirely with \"git reset HEAD^\".\n");
>  
>  static const char empty_cherry_pick_advice[] =
> -N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\n"
> -"If you wish to commit it anyway, use:\n"
> +N_("The previous cherry-pick is empty.\n"
> +"If the commit was created empty, please use:\n"

After reading this three times, I have to say that the updated wording do not look like an improvement for two reasons.

 (1) After a failed cherry-pick, the index can match the current HEAD for
     two reasons.  Either the original cherry-pick was attempting to pick
     an empty commit (which is likely to be a mistake unless you are doing
     something unusual like creating an empty commit in the first place),
     or the change in the original commit was already found in the current
     version (may be result of a conflict resolution).  The message before
     your change used "possibly" to hint this, and if the reader gets it,
     it is understandable why the reader is seeing this advise.  Updated
     message loses this information by simply saying "is empty".

 (2) The message is given by the "git commit" command.  "If the commit was
     created empty" looks confusing.  Even though I can understand that
     "the commit" refers to the original commit the user tried to
     cherry-pick before running this command while reviewing this patch, I
     suspect that the reader who sees this message may not be able to tell
     if the "git commit" command created a possibly empty commit and then
     telling the user to do something further on _that_ commit, or if it
     is referring to the commit the user tried to pick with the previous
     "git cherry-pick" command.

That is, unless you are making "git cherry-pick --allow-empty" not to stop and leave it to "git commit" to clean it up. If that were the case (which is not, after applying this patch alone), then this message will be issued only when a conflict resolution resulted in an empty commit, so "If the commit you were trying to cherry-pick was empty to begin with" would not apply, either.