On Mon, 2017-06-26 at 14:59 -0700, Junio C Hamano wrote: > We don't usually make a bullet list in log message. Please stick to > a plain prose. > > "Previously" is superflous. Say what it does (e.g. "The commit > template adds optional parts without extra blank lines to its normal > output") in present tense and explain the ramifications of it > (e.g. "I personally find that this makes it harder to find the > optional bit"). > Corrected it. > Perhaps you would want to ensure that this change (if you find it > valuable) will not get broken by other people in the future by > writing a new test that ensures that these extra blank lines are > always there when you think they are needed? > See comment below. > I personally do not find these new blank lines are necessary, and > this change wastes vertical screen real estate which is a limited > resource, but that may be just me. I on the other hand do not think > the result of this patch is overly worse than the status quo, either. > I thought it's not good to trade-off readability for vertical space as the ultimate aim of the commit template (at least to me) is to convey information to the user about the commit that he's going to make. For which, I thought it made more sense to improve it's readability by adding new lines between different sections rather than constrain the output within a few lines. In case it's not worth it, I'll revert it back which isn't that big an issue, anyway. In case it's not suggested to revert this back, I'll add the tests. > We do not use // comment in most parts of our codebase that are > supposed to be platform neutral (iow, compat/ is exempt). > It seems to be a result of my ignorance that C allowed '//' comments only recently. > > @@ -877,8 +879,7 @@ static int prepare_to_commit(const char > > *index_file, const char *prefix, > > (int)(ci.name_end - > > ci.name_begin), ci.name_begin, > > (int)(ci.mail_end - > > ci.mail_begin), ci.mail_begin); > > > > - if (ident_shown) > > - status_printf_ln(s, GIT_COLOR_NORMAL, > > "%s", ""); > > + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); > > // Add new line for clarity > > This does ensure that an extra blank line appears after the optional > section (either after the "only/include assumed" message, or "writing > for somebody else" message). > > If we were to go with this sparser output, I think we also should > give an extra blank line before and after the "HEAD detached from > cafebabe" message you would see: > > $ git checkout HEAD^0 > $ git commit --allow-empty -o > > or "On branch blah" if you are on a branch. I think your change > adds a blank before, but it does not have a separation before > "Changes not staged for commit" line. > I actually didn't think of modifying that in order to keep it in line with the output of `git status`. Further, to me, adding *this* new line before the "Changes not staged for commit" (or something in it's place) seems to be wasting some vertical space with an added drawback that it's not in line with `git status`. > Having said that, to be quite honest, I think this "assuming --only" > message outlive > d its usefulness. This was necessary in very early > days of Git because originally "git commit foo" did "git add foo && > git commit" (i.e. "-i" was the default) and then later when we made > "--only" the new default in order to match everybody else's SCM, we > needed to remind users of older versions of Git that "git commit foo" > now means "git commit --only foo", not "git commit -i foo" which they > may have been used to. These days, hopefully nobody expects the "-i" > semantics when they do "git commit foo", so perhaps it may be a > better > change to _remove_ the message altogether. > > And with that done, I wouldn't have reservations on this change > (i.e. "is it worth wasting extra screen real estate, especially in > the vertical direction?"), as instead of wasting 2 lines to give a > message that is no longer useful in today's world, it will be > removing one line ;-) > You're right! It is worth saving lines if it's not useful these days. I have sent another mail to the mailing list to get any other possible improvements to the status. http://public-inbox.org/git/1498583779.2737.4.camel@xxxxxxxxx/ -- Regards, Kaartic Sivaraam <kaarticsivaraam91196@xxxxxxxxx>
