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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

On Tue, 26 Jan 2016, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >
> >> +static int is_cr_needed(xdfenv_t *xe1, xdfenv_t *xe2, xdmerge_t *m)
> >> +{
> >> +	int needs_cr;
> >> +
> >> +	/* Match post-images' preceding, or first, lines' end-of-line style */
> >> +	needs_cr = is_eol_crlf(&xe1->xdf2, m->i1 ? m->i1 - 1 : 0);
> >> +	if (needs_cr)
> >> +		needs_cr = is_eol_crlf(&xe2->xdf2, m->i2 ? m->i2 - 1 : 0);
> >> +	/* Look at pre-image's first line, unless we already settled on LF */
> >> +	if (needs_cr)
> >> +		needs_cr = is_eol_crlf(&xe1->xdf1, 0);
> >> +	/* If still undecided, use LF-only */
> >> +	return needs_cr < 0 ? 0 : needs_cr;
> >> +}
> >
> > Retrying with other images when needs_cr is either -1 (unknown) or 1
> > (known to be true) was tricky; I had to read it twice and think
> > about it for 30 seconds before convincing myself that the code does
> > what the log message specifies it should.
> >
> > That is probably because I was thinking in terms of "do we know that
> > we need to add a CR?"; if I read "needs_cr" in my head as "we might
> > want to add a CR", everything becomes much more clear, but perhaps
> > it is just me.
> >
> > The return value of this function is definitely "do we need and want
> > to add a CR", and it is appropriately named.
> >
> > Thanks.
> 
> Just in case it was unclear, none of the comment above means I want
> any part of the patch redone--I am happy with this patch as-is.

Thanks for saying that... I was about to try to make things clearer, but I
could not think of a better term than "needs_cr". The existing code
already has "add_nl" (which means: add a newline *iff* the block to be
copied does not yet have a newline), which I also found confusing.

Oh wait, maybe "eol_is_crlf" instead of "needs_cr"?

Ciao,
Dscho