Re: [PATCH] Makefile: make NO_ICONV really mean "no iconv"

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Sun, Jun 17, 2018 at 1:32 PM Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>> On Friday 15 June 2018 01:13 PM, Eric Sunshine wrote:
>> > On Fri, Jun 15, 2018 at 2:58 AM Simon Ruderich <simon@ruderich.org> wrote:
>> >> Should we put the part about MacOS's make into the commit
>> >> message? Seems like relevant information for future readers.
>> >
>> > No. The bit of commentary mentioning MacOS's very old 'make' was just
>> > talking about a possible alternate way of implementing the change.
>> > That alternative was not chosen, so talking about old 'make' in the
>> > commit message would be confusing for readers.
>>
>> Interesting. Documentation/SubmittinPatches reads:
>>
>>     The body should provide a meaningful commit message, which:
>>     <snip>
>>     . alternate solutions considered but discarded, if any.
>>
>> The consensus has changed, maybe? In which case, should we remove that
>> statement from there?
>
> Whether or not to talk about alternate solutions in the commit message
> is a judgment call. Same for deciding what belongs in the commit
> message proper and what belongs in the "commentary" section of a
> patch. A patch author should strive to convey the problem succinctly
> in the commit message, to not overload the reader with unnecessary (or
> confusing) information, while, at the same time, not be sparing with
> information which is genuinely needed to understand the problem and
> solution.
>
> Often, this can be done without talking about alternatives; often even
> without spelling out the solution in detail or at all since the
> solution may be "obvious", given a well-written problem description.
> Complex cases, or cases in which multiple solutions may be or seem
> valid, on the other hand, might warrant talking about those alternate
> solutions, so we probably don't want to drop that bullet point.
> Perhaps, instead, it can be re-worded a bit to make it sound something
> other than mandatory (but I can't think of a good way to phrase it;
> maybe you can?).

Yup, "if any" is a bad thing to say, as it does not set the bar for
that "any" random garbage idea.  A phrase like "when appropriate" is
a relatively safe but mostly useless cop-out, as these guidelines
are written primarily for those who don't yet have proper yardsticks
to gauge what is appropriate and what isn't.

I think it maybe better to either drop it or make it a sample way to
do the second point, i.e. if there are seemingly valid alternative
which may entice readers, explaining why the alternative does not
work well and the solution you chose works better *is* a good way to
justify the way you chose in your change.  Off the top of my head,
something like this?  I am not very happy with the text, though.


 Documentation/SubmittingPatches | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index 2488544407..4294d0f068 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -125,10 +125,12 @@ The body should provide a meaningful commit message, which:
 . explains the problem the change tries to solve, i.e. what is wrong
   with the current code without the change.
 
-. justifies the way the change solves the problem, i.e. why the
-  result with the change is better.
+. justifies the way the change solves the problem, i.e. why the result
+  with the change is better (e.g. explaining the reason why an
+  seemingly obvious alternative does not work but the solution in the
+  patch does may be a good way to illustrate the nature of the problem
+  and how your approach fits it better).
 
-. alternate solutions considered but discarded, if any.
 
 [[imperative-mood]]
 Describe your changes in imperative mood, e.g. "make xyzzy do frotz"