search

gornostal / Modific

Highlight lines changed since the last commit (supports Git, SVN, Bazaar, Mercurial and TFS) / ST2(3) plugin
614 stars 44 forks source link

Stray CR character after replace_modified_part command #96

Closed markusgritsch closed 8 years ago

markusgritsch commented 9 years ago

Unmodified file: 1

Deleting a line and saving the file: 2

Invoking the command replace_modified_part: 3

Note the spurious CR appearing at the end of the reverted line.

I am using Git for Windows 2.5. Changing "default_line_ending" has no effect.

Git settings, but seems to also have no effect: core.autocrlf = true

gornostal commented 9 years ago

@markusgritsch, can you create a gist with a file (or a part of it), where you see the issue? Also, please provide your git version.

Thanks.

markusgritsch commented 9 years ago

I already mentioned that I use Git for Windows 2.5. However, I also had the same behaviour with previous versions of Git.

I think the crucial thing to reproduce the bug is to have a file which is in the repository with Unix (LF) line endings which gets checked out on Windows with Windows (CR LF) line endigs. For example this file https://github.com/nodemcu/nodemcu-firmware/blob/master/Makefile has LF line endings in the repo (https://github.com/nodemcu/nodemcu-firmware/raw/master/Makefile) but my local clone of this file has CR LF line endings.

So here is an example with exactly that file: 1

Now, if I delete line 3 2

And then press CTRL+ALT+R 3

the spurious CR appears.

Thanks for looking into this.

gornostal commented 9 years ago

Hehe, I was wondering what is Window 2.5. I think I know how to fix it. I'll create a fix, then you verify it, because I don't have access to Windows machine right now.

markusgritsch commented 9 years ago

Great, looking forward to it.

gornostal commented 9 years ago

I just pushed commit to master.

markusgritsch commented 9 years ago

No, this does not change anything. Note, even if I change join_lines() to return "", a spurious CR appears (of course without any text from the original line):

3

gornostal commented 9 years ago

Made another fix. Hope that helps.

markusgritsch commented 9 years ago

Unfortunately, still the same.

gornostal commented 9 years ago

I just made another fix. And I'm pretty sure it will work.

markusgritsch commented 9 years ago

Still no change. If it helps, when doing git diff Makefile > tmp.diff the file tmp.diff has Unix (LF) line endings.

gornostal commented 9 years ago

OK. I'll get back to this when I get access to a Windows machine.

markusgritsch commented 9 years ago

All right.

markusgritsch commented 8 years ago

Hi gornostal, any chance you will take another look into this issue?

gornostal commented 8 years ago

Sorry for the delay. I'll try to find time for this today or tomorrow.

Thanks for the reminder!

gornostal commented 8 years ago

Could not reproduce the bug today.

Here is what I did:

  1. downloaded and installed Git-2.6.3-64.bit.exe on Windows 7
  2. downloaded and installed ST 3
  3. git config --global core.autocrlf true
  4. git checkout https://github.com/nodemcu/nodemcu-firmware
  5. modified a few lines in Makefile
  6. pressed Ctrl+Alt+R
  7. Modific reverted lines just fine, no CR symbols in the end of the line
markusgritsch commented 8 years ago

Did you delete some lines in the Makefile so that they were added back when pressing Ctrl+Alt+R?

Reverting works fine with modified lines and added lines. The problem appears only when deleting lines from the original file.

gornostal commented 8 years ago

Oh.. I missed that in your description. Will try again today.

markusgritsch commented 8 years ago

Have you been able to reproduce it?

gornostal commented 8 years ago

Not yet. Got distracted at work that day. Downloading virtual box...

gornostal commented 8 years ago

I reproduced it and here is what I found out:

CR appears only if I use \r\n to join the lines. When I change it to \n, it joins them just fine.

Can you find the line with "\r\n" in Modific.py and change that to \n, then let me know if it works for you?

If it does I will simply add a new config option to manually change line endings, because as it turns out, we cannot use windows line endings on windows in some cases like yours.

gornostal commented 8 years ago

Checked in changes to issue-96 branch. Will merge when @markusgritsch confirms it's working.

markusgritsch commented 8 years ago

Yes, this seems to solve the issue, thanks!

But why add a "line_ending" configuration option? Who would need to set this to something else than \n?

gornostal commented 8 years ago

It's good to hear it works.

I thought I could break the functionality for someone who needs \r\n, but maybe you are right. I'll use \n.

P.S. Thanks for being insistent :)

markusgritsch commented 8 years ago

You are welcome ;) I like Modific a lot, and this was the only outstanding issue I had with it. Great that it's fixed now.

Concerning the config option: I think, it is unnecessary. Mind you: The problem with the stray \r characters appeard on a file, which had Windows (\r\n) line endings, and even there the config option has to be set to \n.

Besides: If it would be different from file to file, it would not be practicat to have to change the config option every time.