guberm / tortoisegit

Automatically exported from code.google.com/p/tortoisegit
0 stars 0 forks source link

Line endings shown during patch file review are incorrect and don't match commit #2017

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Perform a commit that contains a mixture of LF and CR-LF line endings
2. Review the commit - The line endings show correctly
3. Make a patch file for that commit
4. Reset branch (or master) to the commit before (perform a hard reset)
5. Review the patch file

What is the expected output? What do you see instead?

The line endings in the patch review to match those in the commit review.

Unfortunately, all the line endings in the patch review look like CR-LF even 
though, if you open then up the patch file in Textpad or your favourite hex 
editor, they are correct.

If you then apply the patch file the correct line endings have been applied and 
reviewing the commit (also) shows the correct line endings.

It seems that TortoiseMerge shows incorrect line endings during the patch file 
review stage.

What version of TortoiseGit and msysgit are you using? On
what operating system?

1.8.6.0
1.8.4.msysgit
Windows 8 Professional

Please provide any additional information below.

Original issue reported on code.google.com by steve_ho...@hotmail.com on 24 Nov 2013 at 6:26

GoogleCodeExporter commented 9 years ago
Could you please provide the setting value of AutoCrlf and SafeCrlf?

Original comment by yuelinho...@gmail.com on 25 Nov 2013 at 12:52

GoogleCodeExporter commented 9 years ago
autocrlf is ticked
safecrlf is blank

Both options are greyed out and not editable via  TortoiseGit settings.

I hope this helps.

Steve

Original comment by steve_ho...@hotmail.com on 25 Nov 2013 at 9:53

GoogleCodeExporter commented 9 years ago
Hi Steve:

Thanks.

That is "Effective".
"AutoCrlf is ticked, SafeCrlf is blank." also confused me at the beginning.

Now,
in Settings/Git/config source,
select "Local" / "Global" / "System",
please provide the setting value of AutoCrlf and SafeCrlf in these config 
source.

Original comment by yuelinho...@gmail.com on 26 Nov 2013 at 3:20

GoogleCodeExporter commented 9 years ago
Ah yes, sorry about that.

         Local Global System
AutoCrlf  Dot   Dot    Tick
SafeCrlf Blank Blank  Blank

Kind Regards

Steve

Original comment by steve_ho...@hotmail.com on 26 Nov 2013 at 7:50

GoogleCodeExporter commented 9 years ago
SafeCrlf is all blank...??? (effective value is warn?)

Actually, I don't understand exactly about your report.
But I need to confirm your settings first.
Then, I can do some test based on your settings.

Could you confirm the steps in the following link I made for this issue?
https://code.google.com/p/tgit-wiki-ylh/wiki/zi2017?ts=1385521743&updated=zi2017

Original comment by yuelinho...@gmail.com on 27 Nov 2013 at 3:24

GoogleCodeExporter commented 9 years ago
Hi,

I've had a look at your reproduction steps and I typically I see an LF where 
you have a CRLF in step 2 - Note: From my own settings I would expect an LF ;-)

As such, this lead me to question my own reproduction steps so I have performed 
some additional testing this afternoon to see if I can be more specific 
(helpful).

If I edit an existing file which has a mixture of LF and CRLF's in the file and 
insert an LF, all works as expected. If I insert a CRLF then that also works as 
expected.

However, the problem I was seeing is only when adding new files that contain a 
mixture of LF and CRLF line endings. So my original reproduction steps are not 
100% correct - sorry for the confusion there :(

I have taken some screenshots which I can send you / attach, if need be, that 
show you my working scenario and non-working scenario.

Cheers for your assistance with this.

Kind Regards

Steve

Original comment by steve_ho...@hotmail.com on 27 Nov 2013 at 3:26

GoogleCodeExporter commented 9 years ago
About your screenshots, perhaps attach them here. (Please attach *.png file.)

If you add files with mixture EOL and want them all CRLF after commit, you can 
delete the files and do Revert on them.

If you want mixture EOL actually and realize what you are doing, set AutoCrlf 
to false(NOT Recommended for Windows User).

My testing result on different settings of AutoCrlf and SafeCrlf:
https://code.google.com/p/tgit-wiki-ylh/wiki/y_Settings_EOL?ts=1385609289&update
d=y_Settings_EOL
For your information.

Original comment by yuelinho...@gmail.com on 28 Nov 2013 at 5:35

GoogleCodeExporter commented 9 years ago
Hi Steve:

Any news?

Original comment by yuelinho...@gmail.com on 4 Dec 2013 at 2:11

GoogleCodeExporter commented 9 years ago
Hi,

Sorry for the delay. Please find attached 4 screenshots tested in a branch in 
my curl repository. A description of the 4 steps is as follows:

* Step 1 - Added a file with a mixture of CR and LF characters (For the purpose 
of this test, this is copy of an existing test xml file, rather than a new file 
that I would normally do)
* Step 2 - Produced a patch file
* Step 3 - Performed hard reset
* Step 4 - Reviewed patch file which shows CRLF for all line endings

Original comment by steve_ho...@hotmail.com on 9 Dec 2013 at 9:16

Attachments:

GoogleCodeExporter commented 9 years ago
I think that is what git does when set AutoCrlf to true in my understanding.

Original comment by yuelinho...@gmail.com on 10 Dec 2013 at 5:14

GoogleCodeExporter commented 9 years ago
Hiya,

I personally don't think this is correct and hence my bug report.

The patch files that are generated contain the correct line endings, which are 
correctly used when I apply a patch file. Why doesn't the patch review / diff 
tool also honour the line endings from the patch file at this point?

This makes reviewing contributions to curl from other contributors that don't 
have write access to the repository, more long winded than it should be for me.

It means that I go to review the patch, notice that all the line endings are 
CRLF in the diff tool and have to then check them with a text editor such as 
Textpad that shows the difference in text mode or view in Hex mode if I want to 
be doubly sure, before continuing with the review.

Alternatively I apply the patch file, and then review it, but again this is an 
extra step that I may forget to do. If I do forget to check the line endings 
properly then I sometimes end up pushing incorrect line endings into the 
repository which then breaks our autobuilds and I have to following it up with 
the another commit :(

For the most part things work perfectly except the diff tool when reviewing 
patch files.

Many thanks for your assistance so far.

Kind Regards

Steve

Original comment by steve_ho...@hotmail.com on 10 Dec 2013 at 8:54

GoogleCodeExporter commented 9 years ago
Hi Project Members:

Reproduced Steps for AutoCrlf = false:

1. Commit file with mixing EOL
2. Review Log -> Mixing EOL
3. Format Patch
4. Check EOL of Patch File -> Mixing EOL
5. Create and Switch to Testing Branch
6. Review Patch with TortoiseGitMerge

Expected: Mixing EOL
Instead: All CRLF EOL

If AutoCrlf is true,
also should show all LF when review patch with TortoiseGitMerge.

Other information:

Wiki updated for reproduced steps:
https://code.google.com/p/tgit-wiki-ylh/wiki/zi2017?ts=1386741480&updated=zi2017

Because AutoCrlf may not set to true, I have come full cycle. :P
Although I think it does not related to the setting of AutoCrlf finally.

----

Hi Steve:

I got your key point. :)

Other information for you:

If you set AutoCrlf to true and commit file with mixing EOL, the file stored in 
 repository is all LF and the file in working tree is still mixing EOL at the 
moment.
You can delete that file, then revert it, you will get the file with all CRLF 
EOL.
That is what git does if set AutoCrlf to true.

I guess you would set AutoCrlf to false in your case finally, so that the files 
with mixing EOL will be stored correctly.

Original comment by yuelinho...@gmail.com on 11 Dec 2013 at 8:40

GoogleCodeExporter commented 9 years ago
I think it is the same root cause with Issue #2003.

Original comment by yuelinho...@gmail.com on 21 Mar 2014 at 8:48

GoogleCodeExporter commented 9 years ago
SORRY for comment #13, it is not true.
The root cause of Issue #2003 is the CatCommand.

Original comment by yuelinho...@gmail.com on 30 Jun 2014 at 6:46

GoogleCodeExporter commented 9 years ago
Hi Steve:

I got a simple fix for this issue at the beginning.
But I found it's not that easy for me to fix it after studying it more and more.
I am still working on this. (wow, so long time... :P )

Before fixing the issue, I got one more question to you.
What kinda EOL does let your autobuilds failed?
Mixed EOL does? LF EOL does? or CRLF does?
Maybe I can give you a workaround first.

Thank you.

Yue Lin Ho

Original comment by yuelinho...@gmail.com on 14 Jul 2014 at 1:50

GoogleCodeExporter commented 9 years ago
What's the status of this issue?

Original comment by sstrickr...@googlemail.com on 29 Mar 2015 at 2:42

GoogleCodeExporter commented 9 years ago
First, I am sorry that I don't work on it for a long time.

I don't figure out what the user's problem really is, but I can reproduce one 
situation which shows the issue.

Just re-tested it. Still reproducible.
Reproduced steps and a testing repo can be found here:
https://code.google.com/p/tgit-wiki-ylh/wiki/zi2017?ts=1386741480&updated=zi2017

IIRC, the problem is that TortoiseGitMerge has it own EOL logic for showing and 
that logic comes from svn.
In other words, TortoiseGitMerge try to predict the EOL should be, then showing.
In git, something different from that logic, that leads to that the EOL is not 
showed correctly.
But, the patch can be applied correctly.
*I think it is only EOL showing problem.*

I guess I can fix that logic, but I consider another thing in the mean time.
Why TortoiseMerge/TortoiseGitMerge try to predict EOLs?
Should that logic be dropped?
I mean drop that logic and fix this issue another way, instead fix that logic.
Let it respect how VCS deal with the EOLs.
Agree?

^_^

Original comment by yuelinho...@gmail.com on 30 Mar 2015 at 3:14