On Wed, Aug 01, 2012 at 10:52:34AM -0700, Junio C Hamano wrote:
> Chris Webb <chris@xxxxxxxxxxxx> writes:
>
> [summary: this, when 59a8fde does not have any commit log message,
> refuses to commit]
>
Thanks for CC'ing me on this. I'm on vacation currently, but will look at this
in detail as soon as I'm back next week
Neil
> > $ git cherry-pick 59a8fde
> > Aborting commit due to empty commit message.
>
> > I can see that this check could make sense when the message has been
> > modified, but it seems strange when it hasn't, and isn't ideal behaviour
> > when called from rebase -i. (We otherwise make sure we call git commit with
> > --allow-empty-message to avoid problems with reordering or editing empty
> > commits.)
> >
> > I could just remove the check in the 'message unmodified' case with
> > something like
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index bf078f2..cf8bc05 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -306,6 +306,7 @@ static int run_git_commit(const char *defmsg, struct replay_opts *opts,
> > if (!opts->edit) {
> > argv_array_push(&array, "-F");
> > argv_array_push(&array, defmsg);
> > + argv_array_push(&array, "--allow-empty-message");
> > }
> >
> > if (allow_empty)
> >
> > but perhaps there are other users of the sequencer for whom this check is
> > desirable? If so, would an --allow-empty-message to git cherry-pick be a
> > better plan, which git rebase -i can use where appropriate?
>
> A few random thoughts.
>
> - Any Porcelain commands that implement the sequencing workflow, if
> they know what message to use when they internally run "commit"
> without allowing the user to edit the message, share the same
> issue.
>
> - We generally try to encourage users to describe commits, and
> commits with empty log messages are strongly frowned upon.
>
> In that sense, one could argue that cherry-pick did the right
> thing when it gave control back to you upon seeing an empty
> message. The user is given a chance to fix the commit by running
> "git commit" at that point to give it a descriptive message.
>
> - These Porcelain programs, however, work from existing commits,
> and the reason why "git commit" invoked by them may be stopped
> due to empty log message is because the original commits had
> empty log message to begin with. The user must have done so on
> purpose (e.g. by using "commit --allow-empty-message").
>
> In that sense, it is likely that the user will simply choose to
> run "git commit --allow-empty-message", even if given a chance by
> "cherry-pick" to correct the empty log message. This is a
> counter-point to the "give the user a chance to fix" above.
> We _might_ not be adding much value to the system by giving the
> control back to the user.
>
> - We had a similar discussion on what should happen when one step
> in "cherry-pick" results in the same tree as the commit the
> 'pick' builds on (i.e. an empty change). The situation is a bit
> different from yours, because unlike the log message, an empty
> change can result by either (1) the original was an empty change,
> or (2) the change picked was already present in the updated base.
> We added "--keep-redundant-commits" and "--allow-empty" options
> to underlying "cherry-pick" to support this distinction.
>
> We may want to follow suit by triggering your change above only
> when "cherry-pick --allow-empty-message" was given. This is
> siding with the "give the user a chance to fix" viewpoint to
> choose the default, and giving the users a way to overriding it.
>
> - Regarding the choice of default between "--allow-empty-message"
> vs "--no-allow-empty-message", one could argue that the best
> choice of the default depends on the Porcelain command.
>
> - A non-range cherry-pick (e.g. "cherry-pick A B C") is a strong
> hint from the user that the user wants to replay the specific
> commits that are named on the command line. This fact may
> favor "the user must have done so on purpose" viewpoint over
> "give the user a chance to fix" viewpoint; defaulting to
> "--allow-empty-message" (and "--allow-empty", and perhaps
> "--keep-redundant-commits") might be more convenient for a
> non-range cherry-pick.
>
> - A range cherry-pick (e.g. "cherry-pick A..B") and "rebase -i",
> on the other hand, are primarily used to rebuild (and reorder
> in the case of "rebase -i") the history to clean it up, which
> may favor "give the user a chance to fix", i.e. defaulting not
> to enable "--allow-empty"-anything might be more convenient for
> a sequencing operation over a range in general.
>
> But from the bigger UI consistency point of view, it would be
> chaotic to change the default of some options for a single
> command depending on the nature of the operand, so I would
> recommend against going this route, and pick one view between
> "give the user a chance to fix" or "the user must have done so on
> purpose" and apply it consistently.
>
> My recommendation, backed by the above line of thought, is to add
> support for the "--allow-empty-message" option to both "rebase [-i]"
> and "cherry-pick", defaulting to false.
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html