patchAdd testcase for merging in a CRLF repo, showing that conflict file is in LF only

An LF only conflict file results in the resolved file being in LF, the commit is in LF and a warning saying that LF will be replaced by CRLF, and the working dir ends up with a mix of CRLF and LF files.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 (Resend due to "git reset --hard initial" instead of "git reset
 --hard a", in the first testcase)
 
 Sorry, no patch to actually *fix* the problem.
 Someone who knows the code in question will probably find the solution in a
 fraction of the time that I would.
 Also note that :1:file, :2:file and :3:file all are also in LF format, and not
 CRLF, which you would want if core.autocrlf == true.

 t/t6033-merge-crlf.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
 create mode 100755 t/t6033-merge-crlf.sh

Marius Storm-Olsen schrieb:

After reading these 3 lines I've no idea what you are talking about. Can you translate this to English, please? ;-)

Then you should use test_expect_failure instead of test_expect_success. And maybe also mention it in the commit message.

	test_must_fail git merge side &&

-- Hannes

Johannes Sixt said the following on 09.06.2008 15:37:

Certainly :-) It means that if you work on a repo with core.autocrlf == true, you'd expect every text file to have CRLF EOLs. However, if you by some operation, get a conflict, then the conflicted file has LF EOLs. Now, of course you'd go about resolving the files conflict, and then 'git add <file>'. When you do that, you'll get the warning saying that LF will be replaced by CRLF. Then you commit. The end result is that you have a workingdir with a mix of LF and CRLF files, which after some more operations may trigger a "whole file changed" diff, due to the workingdir file now having LF EOLs.

Well, the test case is written in a way that it *should* pass (iow, it _expects_ a success), but it currently doesn't. So, the goal is that someone, who is more intimate with the code, can just run the testcase until it passes (fixing in between each run, of course ;-)

Ah, I checked a few other testcases, where I saw the ! construct. I don't mind changing it, if it's important. Does it add 'feature' to the testcase by using test_must_fail, instead of '!' ?

Thanks

-- .marius

Marius Storm-Olsen schrieb:

Aha! Care to write it this way in the commit message in the next round? ;)

test_expect_failure has changed its meaning. It's now used to say precisly what you describe here.

It means: "We should expect this command sequence to complete successfully, but we know that there is a bug in a git command, and hence we must expect failure until it is fixed."

Such a test is marked as "still broken", and the test run is not interrupted. If the bug is fixed, the test is marked as "FIXED" until the 'test_expect_failure' is turned into 'test_expect_success'.

'! git cmd' says that any unusual exit is ok, even a segfault and incorrect usage. 'test_must_fail git cmd' says that only deliberate error exits are ok.

-- Hannes

Marius Storm-Olsen said the following on 09.06.2008 16:46:

..actually, it's more like applying patches and cherry-picking then breaks when the touch the same file, if I recall correctly.

Thanks for all the pointers, I'll send an updated patch tomorrow.

-- .marius

From: Marius Storm-Olsen <marius@trolltech.com>

If you work on a repo with core.autocrlf == true, you would expect every text file to have CRLF EOLs. However, if you by some operation, get a conflict, then the conflicted file has LF EOLs.

Now, of course you'd go about resolving the files conflict, and then 'git add <file>'. When you do that, you'll get the warning saying that LF will be replaced by CRLF. Then you commit. The end result is that you have a workingdir with a mix of LF and CRLF files, which after some more operations may trigger a "whole file changed" diff, due to the workingdir file now having LF EOLs.

An LF only conflict file results in the resolved file being in LF, the commit is in LF and a warning saying that LF will be replaced by CRLF, and the working dir ends up with a mix of CRLF and LF files.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 t/t6033-merge-crlf.sh |   52 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 52 insertions(+), 0 deletions(-)
 create mode 100755 t/t6033-merge-crlf.sh

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-merge-recursive.c |    8 ++++++++
 t/t6033-merge-crlf.sh     |    2 +-
 2 files changed, 9 insertions(+), 1 deletions(-)

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

Fairly straightforward fix, except that I suspect this needs to be done only for regular files and not symlinks.

I think entry.c:write_entry() shows how this should be done.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	On Mon, 9 Jun 2008, Junio C Hamano wrote:

	> Fairly straightforward fix, except that I suspect this needs to 
	> be done only for regular files and not symlinks.
	> 
	> I think entry.c:write_entry() shows how this should be done.

	Right.  And the relevant clause is actually already there.  D'oh.

 builtin-merge-recursive.c |   12 +++++++++++-
 t/t6033-merge-crlf.sh     |    2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)

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

Well, you actually have "double d'oh". "This ought to be a symlink but the filesystem is lacking, so we instead write out what the readlink from such a symlink would return" codepath should not convert_to_worktree().

I'll fix it up, no need to resend.  Thanks for the fix.

Hi,

On Mon, 9 Jun 2008, Junio C Hamano wrote:

I actually thought about that a bit, and just assumed that the rest of the Git code respects autocrlf for "fake" symlinks.

IMO it makes no sense at all to write the textual symlink files without CR/LF when the user clearly asked for it with autocrlf = true. After all, it _is_ a text file then.

But yes, I tried to save some time and did not check.

Ciao, Dscho

When you use 'git show <rev>:<file>' or 'git show :<stage>:<file>', the objects are shows as they are in the object store, ignoring the core.autocrlf configuration.

This series adds testcases which checks the stage files in a merge conflict, and a fix for the problem.

Running all testcases before and after the fix reveals no regressions:

Before patch series:
    ./aggregate-results.sh test-results/t*-*
    fixed   1
    success 3374
    failed  0
    broken  2
    total   3377
    
After patch series:    
    ./aggregate-results.sh test-results/t*-*
    fixed   1
    success 3377
    failed  0
    broken  2
    total   3380
    rm -f -r 'trash directory' test-results

Marius Storm-Olsen (2):
  Add testcases for verifying that staged files in a conflict are CRLF,
    when core.autocrlf = true
  Ensure that objects shown in a core.autocrlf = true repo have CRLF
    EOLs

 builtin-log.c         |   19 ++++++++++++++-----
 t/t6033-merge-crlf.sh |   18 ++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)

When you 'git show :2:<file>' in a conflict, the file should have CRLF EOLs, if the repo is configured with core.autocrlf = true.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 t/t6033-merge-crlf.sh |   18 ++++++++++++++++++
 1 files changed, 18 insertions(+), 0 deletions(-)

When you show an object, it should be shown with the EOLs which the repo is configured for, and not how it's stored internally in the object store.

Signed-off-by: Marius Storm-Olsen <marius@trolltech.com>
---
 builtin-log.c         |   19 ++++++++++++++-----
 t/t6033-merge-crlf.sh |    6 +++---
 2 files changed, 17 insertions(+), 8 deletions(-)

Hi,

On Tue, 10 Jun 2008, Marius Storm-Olsen wrote:

I think this is the correct behaviour: inside the object repository, the files are supposed to be LF clean.

Likewise, things in the unmerged stages are in the index, which again is not the working directory, so they should be LF clean.

_Only_ when writing a file to the working directory, it should get clobbered.

Ciao, Dscho

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

I'd agree with your argument on general principle, but it might make sense to give an option to let you say "here is a blob contents, and use the attribute for this path to munge it out to the filesystem." I am not sure if that belongs to "git show" Porcelain, though. It _could_ be more like:

        git checkout-blob $blob_sha1 $path

that (1) reads the blob object specified by its object name, (2) grabs attribute for the $path, and (3) applies convert_to_worktree() filtering given that attribute and deposits the results to $path.

Alternatively, the interface could be:

        git cat-file blob $blob_sha1 |
        git filter-blob --use-attr-for=$path >$path.old

so that you can then do:

	git diff --no-index $path.old $path

I dunno.

Junio C Hamano said the following on 11.06.2008 00:25:

Well, consider this:
Say you are merging two branches, and know that you want to just use 
the parts which conflict from the branch being merged in. Then you 
simply do:
	git merge side
	git show :3:file.txt > file.txt
	git add file.txt
	git commit ...blah blah...

Now, with the current behavior, this workflow breaks for core.autocrlf=true repos. Given that 'git show' *is* porcelain, I'd expect it to work 'naturally' in my workflow, and not dump raw object store content. However, I also see that it can be useful at times. Almost makes me consider a --raw option to 'git show' for those seldom cases. IMO, 'git show' *should* care about autocrlf. Not doing so is just confusing to the end-user.

The fact that the stage files are in the index doesn't matter. I'd want CRLF files from 'git show v1.5.6-rc0:builtin-log.c' as well.

Marius Storm-Olsen <marius@trolltech.com> writes:

There is always "git cat-file -p" which is porcelain, and should not care about attributes (perhaps with the exception of explicitely told so with some command option).

Hi,

On Wed, 11 Jun 2008, Marius Storm-Olsen wrote:

This is not really how I would do things.  I would do

	git checkout side file.txt here.

The _point_ is: "git show" is supposed to show you the contents _in the repository_. For example, no smudge/clean filters will be heeded, and neither other attributes.

Further, "git show" will work without any problems in any bare repository.

In other words: "git show" is _not_ an operation on a working directory.

"git checkout" is.  So use that instead.

Do not confuse porcelain with "works on the working directory".

But it _does_ matter!

The index works on raw objects, not on smudged files.  Period.

Ciao, Dscho

Johannes Schindelin said the following on 11.06.2008 21:06:

Uhm, 'git checkout side file.txt' is not the same file content 
(ignoring EOLs please) as 'git show :3:file.txt'.
Ref: user-manual.html#conflict-resolution

You are describing "git cat-file". IMO, "git show" should have more consideration towards the repo settings. I doubt anyone, excluding yourself and a few more old-timers, think the content they get out from "git show <file>" is *not* the content they'll get when they decide to "git checkout <file>". For most people the commands a mostly the same, except that "show" just stdout-dumps the content, while "checkout" writes it to disk. The subtle difference there is simply just confusing, and is what we need to fix so people won't find Git so hard to use. It's all about usability. Let "git cat-file" do raw dumps, and "git show" what most people would expect.

Seen another way: If you "git show" any object, they are formated in a nice way for the user to see the output; not raw dumps. There's no reason why the user should even consider that when they show a plain blob, *then* it's raw (in the sense that EOLs are not handled properly).

The "show" command is too nice and convenient for it to have such a disrespect for the user.

Sure, it writes to stdout, and not to file. People understand that.

See above. Nobody expect it to touch files. However, any repo (even bare) still has a config file though, and "git show" should respect its settings.

"git checkout" doesn't munge :<stage>:, which is what the documentation is referring to when it comes to conflict resolution.

I don't. But I'm trying to see the workflow from a non-git-master POV, you're obviously not.

You misunderstood me. I don't smudged files in the index.

Marius Storm-Olsen <marius@trolltech.com> writes:

Bruce, I think the comment in this quoted section is wrong.

True, the combined diff can show only the interesting differences between the three, but that is not because we munge stage #2 and #3. They contain verbatim copies from the HEAD and the MERGE_HEAD respectively, and the combining logic runs three-way diffs between the three stages to discard the hunks that the result comes solely from either stage #2 or stage #3.

So perhaps something like this is in order...

---

 Documentation/user-manual.txt |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

On Thu, Jun 12, 2008 at 12:33:01PM -0700, Junio C Hamano wrote:

Oops, thanks for catching that! I don't know how I got the idea it worked that way.

(Is there any advantage, then, to the :n:filename syntax to a user? Is it useful in any cases when they couldn't use HEAD or MERGE_HEAD instead? If not I might be tempted to cut this bit entirely (or postpone it till later.)

--b.

"J. Bruce Fields" <bfields@fieldses.org> writes:

I'm not sure, but I think that while HEAD and MERGE_HEAD vs :n: differ in the tree represented (in the index trivial / tree conflicts are resolved) they have the same file contents for conflicting files.

I think that :n: syntax is just shorter, especially for the ancestor (c.f. $(git merge-base HEAD MERGE_HEAD)).

And of course there is octopus merge to be considered...

"J. Bruce Fields" <bfields@fieldses.org> writes:

I am somewhat torn between the two.

This section is only about merge conflicts, so using "checkout HEAD path" would be a good substitute. The text flows better that way, because the previous paragraph talks about HEAD and MERGE_HEAD.

When people run "am -3", however, they may wish that they learned how the notation to name blob objects in the index (e.g. :2:path) can be used to examine and resolve the conflict, as there is no HEAD/MERGE_HEAD in that usage context.

On Thu, 2008-06-12 at 13:45 -0700, Junio C Hamano wrote:

Hi Junio,

I was planning on specifically pointing out the :n: forms as well. So I'm watching this one a bit carefully and would appreciate a bit of long-term guidance on the issue here.

Thanks, jdl

Junio C Hamano wrote:

...

Aah, that certainly clears things up a bit. A good patch I'd say.

However, it doesn't change the fact that IMO "git show" should respect core.autocrlf, while "git cat-file" shouldn't.

I think many would consider
     git show MERGE_HEAD:file.txt > file.txt
before
     git checkout MERGE_HEAD file.txt
if only because they'd be scared to do a "checkout" in the middle of a 
merge conflict.

Personally I think the latter is nice, short and sweet, but that doesn't mean that it's less scary for people staring out on git. The fact that the two commands above are *not* identical in result, are the kind of things that we need to iron out, to make git more accessible to the general public.

-- .marius