Chris Webb <chris@xxxxxxxxxxxx> 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.
--
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