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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 31, 2019, 00:04 UTC
Message-ID: <20191231000420.32396-1-jonathantanmy@google.com>
In-Reply-To: <xmqqbls4aznl.fsf@gitster-ct.c.googlers.com>

Show 10 quoted lines

> > 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]:

Show 14 quoted lines

> 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/