Re: [PATCH bg/format-patch-p-noop] log-tree: always add --- marker when options are patch and a stat

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 4, 2009, 05:49 UTC
Message-ID: <7vy6mmltz9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1257283456-7007-1-git-send-email-bebarino@gmail.com>

Stephen Boyd <bebarino@gmail.com> writes:

Show 13 quoted lines

> Previously, the three dash marker (---) would only be added if the diff
> output format was a patch and diffstat (usually -p and --stat). Now that
> patches are always generated by format-patch regardless of the stat
> format being used (--stat, --raw, --numstat, etc.), always add the three
> dash marker when a patch is being generated and a stat option is used.
>
> This allows users to choose the stat format they want and unifies the
> format of patches with stats. It also make patches easier to apply when
> generated by format-patch with non-standard stat options as the stat is
> no longer considered part of the commit message.
>
> Signed-off-by: Stephen Boyd <bebarino@gmail.com>
> ---

I actually am more worried about 68daa64 from 14 months ago, as I vaguely recall seeing an explicit user request that in some community the diffstat information is unwanted on their mailing list, and I am reasonably sure that "-p suppresses --stat" was done deliberately to satisfy them (even though it may have been a suboptimal UI and --no-stat might have been a lot more straightforward).

Even though I personally find the stat information very useful, I would be happier if somebody reverts the bg/format-patch-p-noop series and instead fixes the regression caused by 68daa64, and does so without touching any output from the low-level plumbing like diff-tree that may be used by scripts.

With older (say 1.6.0) git, format-patch with the -p option does not give these three-dash lines, and it does look funny. Even though the same funniness appears only when you use --raw or --numstat with the current code, if we fix "-p" to suppress the default "--stat", this will become an issue again.

> I'm not sure this is wanted though and I guess this could break people's
> scripts. Are people actually using --numstat or --raw to put the stat into
> the commit message?

I am not worried so much about "format-patch --any-option" output; I think it is sane to have three-dash line in it and that is what people expect to see.

I however think people used "diff-tree --pretty --raw" as a mechanism to obtain statistics. A script can easily see where the header is and where messages are (they are four-space indented), and what remains after stripping them give you the list of paths each commit touches. --numstat was invented to help this kind of application gather line-level statistics more easily, and I am a bit reluctant to suddenly start giving three-dash in their output. It will upset what is reading from the pipe downstream.

In an ideal world, I would probably say:

 * format-patch should have three-dash after the commit message, no matter
   what format the patch is asked for, and it always will give patch text.

 * format-patch -p should be reinstated as a way to ask for "just patch
   text, no diffstat".  Introducing a new option --no-stat _in addition_
   to improve the UI is Ok.

 * format-patch -U<n> should not be mistaken as a request to suppress
   diffstat; what 68daa64 _tried_ to do was worthy.

 * Other commands of "log" family that understand -p/-U<n> to produce
   patch text should also give three-dash after the log message, and no
   three-dash when they don't produce patch text.