Re: [PATCH] merge-file: consider core.crlf when writing merge markers

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Nov 25, 2015, 11:09 UTC
Message-ID: <alpine.DEB.1.00.1511251205430.1686@s15462909.onlinehome-server.info>
In-Reply-To: <5654E9B6.3090801@drbeat.li>

Hi Beat,

On Tue, 24 Nov 2015, Beat Bolli wrote:

Show 22 quoted lines

> On 24.11.15 23:43, Beat Bolli wrote:
> > On 24.11.15 09:21, Johannes Schindelin wrote:
> >>
> >> On Mon, 23 Nov 2015, Beat Bolli wrote:
> >>
> >>> When merging files in repos with core.eol = crlf, git merge-file
> >>> inserts just a LF at the end of the merge markers. Files with mixed
> >>> line endings cause trouble in Windows editors and e.g.
> >>> contrib/git-jump, where an unmerged file in a run of "git jump
> >>> merge" is reported as simply "binary file matches".
> >>
> >> Wow, what a beautiful contribution!
> >>
> >> I wonder how difficult it would be to make this work with
> >> gitattributes, i.e. when .gitattributes' `eol` setting disagrees with
> >> core.eol.
> > 
> > I have implemented this according to your algorithm. Now, I have to
> > set core.autocrlf to true for making the new test pass. Setting
> > core.eol no longer has an effect on the merge markers. Is this
> > expected? (I haven't set any attributes)
>

No, this is not expected. I guess that I was a bit careless in my suggestion (it was just a sketch, after all, not a full implementation):

Show 8 quoted lines

> enum eol eol_for_path(const char *path, const char *src, size_t len)
> {
>         struct conv_attrs ca;
>         struct text_stat stats;
> 
>         convert_attrs(&ca, path);
>         if (output_eol(ca.crlf_action) != EOL_CRLF)
>                 return EOL_LF;

At this point, output_eol(ca.crlf_action) would be EOL_UNSET, I guess, in which case we would have to consult core_eol, of course. So it is not enough to test for EOL_CRLF but we really need to test for EOL_CRLF, EOL_LF and EOL_UNSET explicitly (I would use a switch() statement and use a default: rule instead of handling EOL_UNSET specifically).

Ciao, Dscho