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

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Apr 10, 2012 at 09:45:46AM -0700, Junio C Hamano wrote:
> Neil Horman <nhorman@tuxdriver.com> writes:
> 
> > 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".
> 
I can re-instate the possible language if you like, I don't have a problem with
that.

>  (2) The message is given by the "git commit" command.  "If the commit was
>      created empty" looks confusing.  Even though I can understand that
Its coded within the git commit command code, but is only ever displayed if
whence is GIT_CHERRY_PICK, so as far as I can see, from a users perspective,
this will only be seen if they type git cherry-pick on the command line.

>      "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.
> 
Given that this message is displayed because the index and HEAD have no changes
leading to what would be an empty commit, which is by default no allowed unless
expressly enabled, I don't think its that confusing at all.  "The commit" can
only refer to the commit being cherry picked, since there is no other.

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

Ok, I see what you're saying.  This advice makes sense if you issue git
cherry-pick, but not if you issue git cherry-pick --allow-empty.  I think the
additional advice provided when you add in the keep-redundant-commits patch
though, assuming I re-add the possibly language above, yes?
Neil


> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>