Re: [PATCH v2 1/1] commit: display advice hints when commit fails

[Jonathan Tan <jonathantanmy@google.com>]

> > This fix was about "we do not want to unconditionally drop the
> > advice messages when we reject the attempt to commit and show the
> > output like 'git status'", wasn't it?  The earlier single-liner fix
> > in v1 that flips s->hints just before calling run_status() before
> > rejecting the attempt to commit was a lot easier to reason about, as
> > the fix was very focused and to the point.  Why are we seeing this
> > many (seemingly unrelated) changes?
> 
> In any case, here is what I tentatively have in my tree (with heavy
> rewrite to the proposed log message).

Junio, what are your plans over what you have in your tree? If you'd
like to hear Heba's opinion on it, then she can chime in; if you'd like
a review, then I think it's good to go in.

I think the main area of discussion is whether we should go with Heba's
attempt to address Emily's comment [1]:

> I think the intent of that commit was to not put hints into the editor,
> so does it make sense to instead wrap this guy:
> 
>   /*                                                                       
>    * Most hints are counter-productive when the commit has                 
>    * already started.                                                      
>    */                                                                      
>   s->hints = 0;  
> 
> in "if (use_editor)"?
> 
> I didn't try it on my end. Maybe it won't help much, because we think
> we're going to use the editor right up until we realize it's not
> committable?

And I think the answer to that is "s" is used throughout the function in
various ways (in particular, used to print statuses both to stdout and
to the message template) so any wrapping or corralling of scope would
just make things more complicated. In particular, the way Heba did it in
v2 is more unclear - at the time of setting s->hints = 0, it's done
within a "if (use_editor && include_status)" block, but (as far as I can
tell) the commit message template might also be used when there is no
editor - for example, as input to a hook. And more importantly, when
s->hints is reset to the config, we don't know at that point that the
next status is going to stdout. So I think it's better just to use the
v1 way.

The second area of discussion I see is in the commit message. Commit
messages have to balance brevity and comprehensiveness, and this can be
a subjective matter, but I think Junio's strikes a good balance.

[1] https://lore.kernel.org/git/20191217224541.GA230678@google.com/