Hi all,

Having some trouble creating patches that actually work. (On Windows)
I've been using git to get FPC trunk and
git diff --no-prefix > %temp%\mypatch.diff
rem --no-prefix : Do not show any source or destination prefix. to create patches.

Using Msysgit:
git --version
git version 1.7.6.msysgit.0

However, if I try to apply the patch with
cd /d c:\development\fpc\source
rem use fpc binutils patch
patch -p0 < %temp%\mypatch.diff
I often get a nasty error:
patching file `packages/fcl-extra/src/win/ServiceManager.pas' Assertion failed: hunk, file patch.c, line 321

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

Patch version:
patch --version
patch 2.5
Copyright 1988 Larry Wall
Copyright 1997 Free Software Foundation, Inc.

This program comes with NO WARRANTY, to the extent permitted by law. You may redistribute copies of this program
under the terms of the GNU General Public License. For more information about these matters, see the file named COPYING.

written by Larry Wall with lots o' patches by Paul Eggert

As an alternative, I've used diff with two directories like this: diff -u --minimal --recursive --unified -N olddir newdir > mypatch.diff but that may have some issues with Linux/Windows line endings..

Any suggestions for creating/applying better patches using git or diff?

Thanks,
Reinier

In our previous episode, Reinier Olislagers said:

I don't know about GIT, but some tips:

1. Play with lineending. (I use cygwin's dos2unix and unix2dos to convert)
2. Also make sure that the generated diffs are in the universal format.

I'm sure it is because patch contains non CRLF linefeeds, as I had such errors previously, not even using Git. It seems the "patch" binary only supports Windows EOLs.

Thanks, Marco & cobines:

That seems to be it!

Unified format: yep, running either
git diff --no-prefix > %temp%\mypatch.diff
or
git diff --no-prefix --unified=3 > %temp%\mypatch.diff gives the same results.

git diff (in my install at least) seems to generate Unix line endings. If I do something like
cd /d C:\Development\Fpc\Source\packages\fcl-extra\src\win git diff --no-prefix > %temp%\gitpatch.diff
rem convert unix line endings to dos:
sfk lf-to-crlf %temp%\gitpatch.diff
rem using swiss file knife, for some reason I hate cygwin ;) ren %temp%\gitpatch.diff gitpatch_convertedtodoslineending.diff patch -p5 < %temp%\gitpatch_convertedtodoslineending.diff it seems to work.

Unfortunately, git diff doesn't seem to have any arguments for line end conversion, but there must be some setting/config somewhere that I had set incorrectly...

I'll get back when I know more...

Thanks!

Reinier

Ok, I've looked at it some more:
Example:
packages\fcl-extra\src\win\ServiceManager.pas

svn:
via web:
http://svn.freepascal.org/svn/fpc/trunk/packages/fcl-extra/src/win/ServiceManager.pas => Unix line endings
Local svn:
svn proplist -v packages\fcl-extra\src\win\ServiceManager.pas svn:eol-style
native
so:
=> WINDOWS LINE ENDINGS
Therefore patch.exe on Windows probably works

Git:
https://github.com/graemeg/freepascal/raw/7026b7669fd422f88ffe33174dac1725c0295427/packages/fcl-extra/src/win/ServiceManager.pas => Unix line endings!!!!
via git pull etc
=> Unix line endings
not surprising...

Seems the SVN client is converting line endings to my native format.

Seems also that there is no way to get git to convert remote LF to local CRLF...

Should I give up on git and just switch to SVN?

I'm sure some more knowledgeable/alert person will have a suitable answer!

Thanks,
Reinier

06.10.2011 16:37, Reinier Olislagers пишет:

there is git config option "core.autocrlf" try to use it

Thanks Alex, I already fiddled with that. I'll try again with core.autocrlf set to true and false to make sure it doesn't work...

06.10.2011 18:59, Reinier Olislagers пишет:

not "true" not "false", but "input". Read this if have not yet : http://progit.org/book/ch7-1.html http://help.github.com/line-endings/

Don't be so quick to blame the tool, rather learn how to use the tool! If you want Git to convert EOL characters to your native platform, then tell it to do so during the Git installation (which you clearly skipped or didn't understand the options), or specify it later as a global or per repository option (normally before a 'git clone' process).

Here it show the option you somehow skipped to notice in the Git installation: http://opensoft.homeip.net:8080/~graemeg/msysgit-1.png

To find out more about the core.autocrlf option look it the help: git help config
....then search for 'core.autocrlf'

Reinier Olislagers wrote

Thanks Alex, I already fiddled with that. I'll try again with core.autocrlf set to true and false to make sure it doesn't work...

As far as I know, Git will only do the conversion when you clone a new repository, or when you pull new changes. If you had the setting wrong at the time you did the clone, the problem is on you. Simply change the setting, and get another clone (use --depth if you don't want a full clone again)

Alternatively, you might be able to get away with only specifying the following command line option with you 'git diff' command. I use this if I made a cloned repository under Linux, but share that repository with a Windows VM session (thus not cloned from inside the VM).

--ignore-space-at-eol

'git help diff' will give you more such options.

06.10.2011 19:15, Graeme Geldenhuys пишет:

No, git do conversion on checkout and commit.

Thanks for the clear explanation.

Don't be so quick to blame the tool, rather learn how to use the tool!

Not really, I did suggest that smarter users than me might have a solution... ;)

Thanks, that matches with what I found out.

Here it show the option you somehow skipped to notice in the Git installation: http://opensoft.homeip.net:8080/~graemeg/msysgit-1.png

Did see that, thought about it & obviously picked the wrong one ;)

To find out more about the core.autocrlf option look it the help: git help config ....then search for 'core.autocrlf'

Yep, had seen that, but it wasn't too clear to a git newb like me though. Still, I've been finding out more - by trial and error ;)

I had supposed that FPC stored its text files in CRLF format but obviously both the Git and SVN repositories contain LF data.

I'll get back to the list when necessary. Thanks for the help.

thanks,
Reinier

Thanks Alex, that's a fairly clear explanation.

Reinier

Alex & the rest, seems I'm doing something wrong when using input: rem go to right directory:
cd /d c:\development\fpc\
rem get rid of any gunk there:
rmdir /s /q source
rem set autocrlf to input for entire git system
git config --global core.autocrlf input
rem check setting - yes, input:
git config --global core.autocrlf
input
mkdir c:\development\fpc\source
cd /d c:\development\fpc\source
git clone --depth 5 git://github.com/graemeg/freepascal.git . rem check situation here - yes, still input:
git config core.autocrlf
input
rem now use notepad++ to open a file:
npp packages\fcl-extra\src\win\ServiceManager.pas

!!!notepad++ shows it has Unix line endings!!!!
Could be that I'm misinterpreting the docs/sites, but isn't input supposed to convert the LFs in the repository to CRLFs?

Tried again using core.autocrlf true, this does result in Windows line endings.
Creating a patch using git diff --no-prefix gives Unix line endings - as expected, really.
So while it could be used, I'd have to remember to run unix2dos over it
- that is, if people expect a Windows patch (

Am I doing something stupid with autocrlf input?

Thanks,
Reinier

06.10.2011 22:41, Reinier Olislagers пишет:

Sorry, my mistake. In your situation "autocrlf yes" should be used.

Reinier Olislagers wrote

rem check setting - yes, input: git config --global core.autocrlf input

The Git installation under Windows set mine to true (not input).

So while it could be used, I'd have to remember to run unix2dos over it - that is, if people expect a Windows patch (

I don't think that is needed. I believe the 'patch' program will sort that out by itself. I have sent numerous patches to FPC and Lazarus (both those repositories are git ones on my Linux system). Nobody has ever complained that the EOL style was wrong. I also don't specify the --no-prefix when creating patches. Again, the 'patch' program can handle those a/... b/... prefixes for the committer without problems.

Regards,
- Graeme -

Graeme Geldenhuys wrote

On 06/10/2011 20:41, Reinier Olislagers wrote: The Git installation under Windows set mine to true (not input).

true it is, see other message ;)

Thanks, did a quick test with svn/patch. SVN diff gives Windows line endings. FPC patch applying diff works.

However, on Windows, the git diff doesn't seem to work, patch as supplied by FPC on windows freaks out on Unix line endings: cd /d C:\Development\Fpc\source
notepad packages\fcl-extra\src\win\ServiceManager.pas rem do some changes
git diff > c:\windows\temp\gitdiff.diff
rem results in unix line endings, see attachment
type c:\windows\temp\gitdiff.diff
diff --git a/packages/fcl-extra/src/win/ServiceManager.pas b/packages/fcl-extra
src/win/ServiceManager.pas
index 47bd1cc..af62487 100644
--- a/packages/fcl-extra/src/win/ServiceManager.pas +++ b/packages/fcl-extra/src/win/ServiceManager.pas @@ -136,7 +136,8 @@ type

     procedure GetServiceStatus(SHandle : THandle; Var Status :

TServiceStatus)
overload;

     procedure GetServiceStatus(ServiceName : String; Var Status :

TServiceStat
s); overload;

     Property  Handle : THandle Read FHandle;

- Property Acces : DWord read FAccess Write FAccess;
+ Property Access : DWord read FAccess Write FAccess;
+ Property Acces : DWord read FAccess Write FAccess; deprecated; //Kept for
compatibility

     Property  Services : TServiceEntries Read FServices;
   published
     { Published declarations }

rem now try to reverse the change:
patch --dry-run -R -p1 < c:\windows\temp\gitdiff.diff Assertion failed: hunk, file patch.c, line 321

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

ok, converted LF=>CRLF into c:\windows\temp\gitdiff_crlf.diff (attached) patch -R -p1 < c:\windows\temp\gitdiff_crlf.diff
works

Patch version:
C:\Development\Fpc\source>patch -v
patch 2.5
Copyright 1988 Larry Wall
Copyright 1997 Free Software Foundation, Inc.

This program comes with NO WARRANTY, to the extent permitted by law. You may redistribute copies of this program
under the terms of the GNU General Public License. For more information about these matters, see the file named COPYING.

written by Larry Wall with lots o' patches by Paul Eggert

File modification date 30 May 2005, 21:05, 28.160 bytes

In our previous episode, Graeme Geldenhuys said:

I usually apply patches on FreeBSD, and in rarer cases Linux.

Even up to date patch doesn't always process lineendings properly btw. I have to dos2unix often on *nix too.

Recently a lot of patches with incorrect filenames, or no filenames if the patch is for a single file, or even not in universal format have been submitted, and it takes some work (doable, but annoying) to apply them.

Since the patches aren't tagged with what generated them, I assumed it was GIT based.

Tried on Debian with subversion, build-essential installed: Normal git diff: patch < git.diff
doesn't work - will need patch -p1 < git.diff

Marco van de Voort wrote

Even up to date patch doesn't always process lineendings properly btw. I have to dos2unix often on *nix too.

Interesting. I would think sharing code between platform with patches is a bog-standard task these days, and all tools in question should work perfectly with such files. Oh well.

Recently a lot of patches with incorrect filenames, or no filenames if the patch is for a single file, or even not in universal format have been submitted, and it takes some work (doable, but annoying) to apply them.

In that case you do much more effort that I. I simply return to sender and tell them to send me a universal patch format, and a link to my website on how to generate patches.

Since the patches aren't tagged with what generated them, I assumed it was GIT based.

That would be an incorrect assumption.

Simply dumping a 'git diff' output to file, always includes the '--git' hint in the first line. See below.

Alternatively, using 'git format-patch' which generates patch files from local commits, also includes the git name including the git version number in the generated patch files.

----8<-------------8<-------------8<-------------8<-------------8<---- $ git diff
diff --git a/Source/revision.inc b/Source/revision.inc index f827943..a64eedc 100644
--- a/Source/revision.inc
+++ b/Source/revision.inc
@@ -1 +1 @@
- '3.0.9';
+ '3.0.9.126.g0560.dirty';

----8<-------------8<-------------8<-------------8<-------------8<----

I believe your "real culprit" might be Lazarus IDE itself. eg: When it detects external file changes it pops up with a diff window. That window shows non-universal patch format output, and no filename or path relating to the patches either. So maybe somebody has been copy and pasting that. Yuck! Here is a small example from Lazarus IDE itself.

----8<-------------8<-------------8<-------------8<-------------8<---- ***************
*** 1,4 ****
! unit mmUtils;

  {$I M2Defines.inc}

--- 1,5 ----
! unit mmUtils;
!

  {$I M2Defines.inc}

***************
*** 7,12 ****
--- 8,14 ----
uses
Classes,
SysUtils,
+
mmTypes,
tiQuery,

  	Model;

----8<-------------8<-------------8<-------------8<-------------8<----

Maybe somebody should file a bug report with Lazarus IDE project and tell them to switch to the more often used universal patch format.

Regards,
- Graeme -

Reinier Olislagers wrote

doesn't work - will need patch -p1 < git.diff

Correct, and anybody that has applied a handful of patches or more in there time would have known that already. ;-)

Regards,
- Graeme -

Sure, but why add to the aggravation - SVN diff doesn't do this, so one less difference to worry about:
--- packages/fcl-extra/src/win/ServiceManager.pas (revision 19392)

Just trying to make life simpler for our dear hard-working committers.... even if that takes hours of annoying mailing list messages ;)

Reinier

Reinier Olislagers wrote

Sure, but why add to the aggravation - SVN diff doesn't do this, so one less difference to worry about:

It's still in the universal patch format, with or without the path prefixes. Also nobody should commit a patch blindly. They should review the patch, at which point they will also have to take note of where the patch should be applied (root src directory, some other sub directory, same directory as the changed file etc.). By the time they actually commit the patch they will know what patch command to use.

I don't consider any of this an aggravation or extra annoyance - just the standard procedure for reviewing and applying any patch. So even though you went to extra effort in creating your patch (kudos to you), it hardly makes a difference to the person applying the patch.

Regards,
- Graeme -

Some instructions for applying patches to Linux kernel source were listing the requirement for it to be applicable with patch -p1, so why worry :)

L.

Ok, so there's some options:
1. Get a better patch.exe on Windows that deals with LF.
2. Output patches using CRLF. Inconvenience Linux/Unix users
3. Output patches using LF. Inconvenience Windows users

I'm going with option 2, as I'm running Windows and people will probably excpect CRLF patches from me (in line with SVN diff). (Of course, I could always use 3 and use git patch...)

Created a batch file fpdiff.cmd:
@echo off
rem Uses git on windows to create a patch with crlf endings. rem Requirements:
rem git, linespace converter program (sfk, unix2dos) rem git core.autocrlf setting is true
rem Tested git core.whitespace trailing-space,space-before-tab,cr-at-eol but had no effect
set OUTPUTNAME=your.diff
del %OUTPUTNAME% > NUL 2>1
rem Git diff gives Unix LF line endings;
rem we're on Windows so convert to CRLF
rem Also, use --no-prefix to strip git prefixes from path rem Redirect errors to bit bucket
rem YOU NEED TO USE CALL to call msysgit git commands: rem http://osdir.com/ml/version-control.msysgit/2008-04/msg00012.html call git diff --unified=3 --no-prefix > %OUTPUTNAME% 2>NUL rem Use swiss file knife to convert, but could
rem also use cygwin unix2dos
sfk lf-to-crlf %OUTPUTNAME%
echo Done, output in %OUTPUTNAME%

Gives output like attachment.

I think I'm satisfied. Hope the committers will be too, when I submit the next patch...

I get your point.

Call it personal preference then - at least with my own patches I'll know I'd have to always apply them in src root ;)

Thanks,
Reinier

git patches can be applied using
http://svn.freepascal.org/svn/fpcbuild/scripts/florian/unix/svn-apply on unix.

But it's much easier to review a patch after it is applied (not committed!) because the full context is available. So everything which is not relative to the fpcsrc root dir is extra work.

In our previous episode, Graeme Geldenhuys said:

The usual problem I guess. Unix tools don't deal pretty well with Windows or on Windows.

I clean out old bugs. The chance on getting a valid response on old patches is low.

But I don't care so much about the -p parameter or the lineendings. That is minor work and fixable.

However people should really, really use universal patches. They are much more likely to apply cleanly, even after some time.

Possible. Many of the lowest quality bugreports are a passed on from Lazarus. As of late, pretty stale half year old reports are being moved.

Maybe somebody should file a bug report with Lazarus IDE project and tell them to switch to the more often used universal patch format.

I don't think this is a procedural problem. Lazarus simply gets the brunt of the bugreports (and then specially the clueless ones). Probably manpower is simply lacking.

You can try to make the Lazarus people take the blame, but that doesn't solve anything. I recognize the situation, it is similar with FPC around 2000, when there was a constant influx of clueless TP newvbies.

Some people extra working on triaging bugreports in the lazarus project would probably solve this too.

And if no files are added/removed/moved a simple "patch -Np 0 < the-diff.patch" is sufficient as well. :D (just tested)

Regards,
Sven