Re: [PATCH v4 1/2] merge-file: let conflict markers match end-of-line style of the context

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 27, 2016, 19:32 UTC
Message-ID: <xmqq8u3a95st.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1601272011570.2964@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

[administrivia: I finally managed to correct e-mail address of peff@, which has been giving me bounces for all the previous messages in the thread]

> Your response is also an indicator to me that future myself will find the
> same code just as confusing as you did, though.
>
> Maybe need_cr -> eol_is_crlf?

The crucial question is how well the variable conveys its meaning when it has -1 as its value. "need_cr? -- I don't know yet" is as clear as "eol_is_crlf? -- I don't know yet", if not clearer. I personally do not think "eol_is_crlf" is an improvement. It makes it unclear _whose_ eol the variable is talking about. It can be misread as talking about one or more of the files that are being merged and mistaken as a statement of a fact (i.e. "we inspected and know the input file's eol is CRLF").

Compared to that, it is clear that "need_cr" is talking about what the EOL convention the resulting file should be.

I briefly wondered if the if/if (need_cr)/... cascade that inspects (conditionally up to) three variants might become cleaner if the polarity of that variable is flipped (we are allowed to use CRLF only when we know that among all of the three variants that we can determine the line termination convention used, all of them use CRLF), but I didn't think it through seriously.