[Box Backup-dev] Reviewing code -- help!

[Ben Summers]

On 7 Dec 2005, at 12:19, Martin Ebourne wrote:

> Sorry, those diff commands are rubbish - I'd forgotten there's stuff
> already merged on trunk, so these diffs end up with those changes
> reversed. Use the following ones below:
>
> chromi's optimised diffing:
>
> svn diff -r1:HEAD http://bbdev.fluffy.co.uk/svn/box/chromi/diffopt

That can be merged.

The controversial use of goto isn't too bad in this case, but let's  
address that later after we've got all the code in truck.


>
> My autoconf:
>
> svn diff -r11:HEAD http://bbdev.fluffy.co.uk/svn/box/martin/autoconf

That's good to go too.


>
> As I understand it the win32 branch to be merged is:
>
> svn diff http://bbdev.fluffy.co.uk/svn/box/chris/win32/merge/00- 
> martins-original-autoconf http://bbdev.fluffy.co.uk/svn/box/chris/ 
> win32/merge/01-apply-win32-patch-11/

WinNamedPipe should be an IOStream derived class, then use  
IOStreamGetLine not PipeGetLine.

WinNamedPipe::Write may not work in edge cases where it doesn't write  
all the bytes given.

The .bat, config and installer files should not be in lib/win32 as  
they're not generic win32 stuff. I suggest bin/bbackupd/win32

If any code is copied and pasted from sources, can there be a note of  
where it came from? I know Nick did it from some BSD licensed code,  
though.

Not keen on the global and member function names beginning with lower  
case letters (and other style "violations").


But really, I think it better to get it in the tree and fix it up  
later, rather than delay any longer. But it would be nice if the  
first three could be address beforehand.




>
> Chris also has subsequent compile fixes for cygwin and mingw in
> merge/02-compile-on-cygwin

svn diff http://bbdev.fluffy.co.uk/svn/box/chris/win32/merge/01-apply- 
win32-patch-11/ http://bbdev.fluffy.co.uk/svn/box/chris/win32/merge/ 
02-compile-on-cygwin/

this change:

-AX_CHECK_MOUNT_POINT(, [AC_MSG_ERROR([[cannot work out how to  
discover mount points on your platform]])])
+AX_CHECK_MOUNT_POINT([],[])

I'm not an autoconf sort of person, but that looks worrying to me?  
But otherwise there's only minor things.


> merge/03-compile-with-mingw-on-win32

svn diff http://bbdev.fluffy.co.uk/svn/box/chris/win32/merge/02- 
compile-on-cygwin/ http://bbdev.fluffy.co.uk/svn/box/chris/win32/ 
merge/03-compile-with-mingw-on-win32/

I'm not convinced about this one, mainly because it has a load of GPL  
code in there. The other changes seem minor though.


Does everyone else agree with these?

If so, can Martin (seeing as he's good at SVN and I'm afraid of  
merging) merge chromi/diffopt and martin/autoconf into trunk?

Thanks!

Ben