git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Chris Webb <chris@arachsys.com>
Cc: git@vger.kernel.org, Neil Horman <nhorman@tuxdriver.com>
Subject: Re: Cherry-picking commits with empty messages
Date: Wed, 01 Aug 2012 10:52:34 -0700	[thread overview]
Message-ID: <7vd33afqjh.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <20120801111658.GA21272@arachsys.com> (Chris Webb's message of "Wed, 1 Aug 2012 12:16:59 +0100")

Chris Webb <chris@arachsys.com> writes:

[summary: this, when 59a8fde does not have any commit log message,
refuses to commit]

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

  reply	other threads:[~2012-08-01 17:52 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-08-01 11:16 Cherry-picking commits with empty messages Chris Webb
2012-08-01 17:52 ` Junio C Hamano [this message]
2012-08-01 18:15   ` Angus Hammond
2012-08-01 22:26     ` Junio C Hamano
2012-08-02 10:10       ` Angus Hammond
2012-08-02  8:55   ` Chris Webb
2012-08-02 10:38     ` [PATCH] cherry-pick: add --allow-empty-message option Chris Webb
2012-08-06 10:57       ` Neil Horman
2012-08-06 11:00         ` Chris Webb
2012-08-06 11:11           ` Neil Horman
2012-08-03  0:22   ` Cherry-picking commits with empty messages Neil Horman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7vd33afqjh.fsf@alter.siamese.dyndns.org \
    --to=gitster@pobox.com \
    --cc=chris@arachsys.com \
    --cc=git@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).