Re: What's cooking in git.git (Jan 2017, #02; Sun, 15)

From: Jeff King <peff@peff.net>
Date: Jan 16, 2017, 22:00 UTC
Message-ID: <20170116220014.bwi5xi2br56lyqsw@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1701161746200.3469@virtualbox>

On Mon, Jan 16, 2017 at 06:06:35PM +0100, Johannes Schindelin wrote:

Show 10 quoted lines

> >      And I think that would apply to any input parameter we show via
> >      error(), etc, if it is connected to a newline (ideally we would
> >      show newlines as "?", too, but we cannot tell the difference
> >      between ones from parameters, and ones that are part of the error
> >      message).
> 
> I think it is doing users a really great disservice to munge up CR or LF
> into question marks. I *guarantee* you that it confuses users. And not
> because they are dumb, but because the code violates the Law of Least
> Surprise.

I'm not sure if you realize that with stock git, the example from your test looks like this (at least in my terminal):

  $ git.v2.11.0 rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
  ': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

The "\r" causes us to overwrite the rest of the message, including the actual filename. With my patch it's:

  $ git rev-parse --abbrev-ref "$(printf 'CR/LF\r\n')" >/dev/null
  fatal: ambiguous argument 'CR/LF?': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'

I am certainly sympathetic to the idea that the "?" is ugly and potentially confusing. But I think it's at least a step forward for this particular example.

I'll snip liberally from the rest of your response, because I think what JSixt wrote covers it.

Show 18 quoted lines

> > > While at it, let's lose the unnecessary curly braces.
> > 
> > Please don't. Obviously C treats the "if/else" as a single unit, but
> > IMHO it's less error-prone to include the braces any time there are
> > multiple visual lines. E.g., something like:
> > 
> >   while (foo)
> > 	if (bar)
> > 		one();
> > 	else
> > 		two();
> > 	three();
> > 
> > is much easier to spot as wrong when you would require braces either
> > way (and not relevant here, but I'd say that even an inner block with a
> > comment deserves braces for the same reason).
> 
> There is no documentation about the preferred coding style.

Documentation/CodingGuidelines says:

 - We avoid using braces unnecessarily.  I.e.

        if (bla) {
                x = 1;
        }

   is frowned upon.  A gray area is when the statement extends
   over a few lines, and/or you have a lengthy comment atop of
   it.  Also, like in the Linux kernel, if there is a long list
   of "else if" statements, it can make sense to add braces to
   single line blocks.

I think this is pretty clearly the "gray area" mentioned there. Which yes, does not say "definitely do it this way", but I hope makes it clear that you're supposed to use judgement about readability.

-Peff