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

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

quoted

     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.

quoted

quoted

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