Re: [PATCH] rebase: add --allow-empty-message option

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 2, 2018, 23:29 UTC
Message-ID: <xmqqy3kbas2h.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <9d0414b869f21f38b81f92ee0619fd76410cbcfc.1517552392.git.sky@genki.is>

Genki Sky <sky@genki.is> writes:

Show 19 quoted lines

> --> Motivation
>
> commit 4bee95847 ("cherry-pick: add --allow-empty-message option", 2012-08-02)
> started doing this work, but it was never completed. For more discussion
> on why this approach was chosen, see the thread beginning here:
>
>   https://public-inbox.org/git/20120801111658.GA21272@arachsys.com/
>
> https://stackoverflow.com/q/8542304 also shows this as a desirable
> feature, and that the workaround is non-trivial to get right.
>
> --> Implementation
>
> Add a new --allow-empty-message flag. Propagate it to all calls of 'git
> commit', 'git cherry-pick', and 'git rebase--helper' within the rebase
> scripts.
>
> Signed-off-by: Genki Sky <sky@genki.is>
> ---

Do you have our project history so that you can try running "git log" to realize that the above does not quite match how people write their log messages? If not, please obtain one and do so ;-)

 - We discourage log messages from not explaining what *it* needs to
   explain itself and only referring to external resources.  The
   first part of the above is a typical anti-pattern.  The only
   thing readers can gather from "Motivation" part without refering
   to outside resources is it is a moral follow-up of 4bee95847
   whatever "this work" is doing, without being told what approach
   "this approach" means, etc.  URLs are good as supporting info,
   but there must be something they are meant to support readable in
   the log message itself.

 - Also we do not organize our log messages as "-->" bulletted
   chapters.  For this particular commit, once the first part
   becomes self-sufficient, I think it is sufficient to drop these
   bulletted headlines and have two paragraphs (first describing the
   problem being solved, then describing how the patch realizes that
   solution).

I think the changes to the code are sensible. As Dscho said, I found the new test script somewhat iffy. Does it have to be a completely new test script (as opposed to an additional test or two to an existing tests for rebase that checks a similar feature like keep-empty)? Would it make it simpler to piggy back on an existing one?