On 7/25/2013 16:52, Joe Mistachkin wrote:

Warren Young wrote:

I guess they already got taken out of the trunk.  I did my spelunking in
a current pull of the tree.

I'm simply searching trunk for "__CYGWIN__".

I was stuck on a branch from February. <wince>

Now that I'm actually looking at the trunk, back into the cave we go:

add.c, line 25: You should be #including windows.h here instead of providing these prototypes. That makes Cygwin's w32api package a build dependency, but if fossil.exe is going to do registry API calls when built with Cygwin, that seems a reasonable dependency.

add.c, filenames_are_case_sensitive(): Keep it. The Cygwin registry setting *is* a better default here than the Unix case default.

blob.c, blob_write_to_file(): Why not let the OS catch this error? I'd take this test out for native Windows, too.

blob.c, BOM and UTF-16 awareness: I think this code is well-intentioned for Cygwin, but probably not quite right.

For Cygwin, you should probably assume UTF-8 by default, as for any modern *ix. Text editors under Cygwin default to UTF-8, not UTF-16 as do native Windows Unicode text editors. If there is a BOM, you should probably do the conversion from UTF-16 to UTF-8 on all platforms, not just Windows and Cygwin.

checkin.c, commit_warning(): Really?  Why can't you use iconv(3)?

db.c, db_open(): Wrong.  Fix or remove.

First, hard-coding a conversion to "/cygdrive/%c/%s" will break if the user has changed the cygdrive mount point in /etc/fstab. This is more common than you may realize. If you mount cygdrive at the root, you can use shorter quasi-DOS paths like /c/Windows. I use this feature.

Second, if Fossil really must mung Windows drive paths to POSIX paths on Cygwin instead of passing paths blindly to the OS like any other program would, it should be done at a higher level than the DB layer.

If you really must mung paths, ask the Cygwin DLL to do it for you, via cygwin_conv_path(). This fixes the cygdrive mount point problem, too.

Personally, I'd say remove the ifdef entirely.

db.c, db_open_config(), user DB location: This function has its heart in the right place, but it's awfully brittle.

Consider just the native Windows case. If, the first time you run Fossil, LOCALAPPDATA isn't defined, it will put the DB in %APPDATA%. But if LOCALAPPDATA is then later defined, it doesn't continue searching the fall-back hierarchy to find its earlier DB file. It will create a new one in %LOCALAPPDATA% and ignore the one it created earlier, losing any settings you had.

The following algorithm provides proper fall-back behavior for native Windows and ensures that pure Cygwin users get the ~/.fossil file they expected:

1. Search the three native Windows environment variable directories for _fossil, using the first one found.

2. If none are found and this is Cygwin, try ~/.fossil, too.

3. If the DB still isn't found, create a new one in ~/.fossil if this is Cygwin, or in %FOO%/_fossil on native Windows, where %FOO% is the highest-precedence environment variable found.

Alternatively, add a feature to Fossil that lets you override the hard-coded DB file location(s) with a high-precedence environment variable. (That is, a second EV on Unix, and a fourth on Windows.) Say, FOSSILDB. Then you can set it to c:\cygwin\home\foo to get interoperability between native and Cygwin fossil.exe. There might be Unix users who can find a use for this feature, too.

db.c, db_open_config(), '.' -> '_' smashing: Cargo cult detected.

Windows NT derivatives and programs written with NT-derived OSes in mind treat files with a leading dot like any other file. The only programs that don't are those that insist on having a file name extension -- which shouldn't be given .fossil files to begin with -- and programs with DOS/Win9x heritages.

Cygwin dropped Win9x support in 2009. Microsoft dropped Win9x support in 2006. DOS and Win16 compatibility got dropped for 64-bit systems in Vista, in 2007. We no longer need to pussy-foot around leading dots on Windows in 2013.

The comment claims "some window systems [have] problems" with file names beginning with a dot. Are any of these systems still supported by Microsoft? And, "many apps" have problems? Quantify "many". If nonzero, how many of these require access to Fossil DB files?

db.c, cmd_open(): Same as above.

For that matter, I think Fossil 2.0 should use .fslckout on native Windows, too, and avoid the need for any special-casing here at all. (Why 2.0? It's a breaking change, so it shouldn't be done lightly.)

db.c, db_open_config(), file access test: This should be unconditional, or be removed. Unix gets by without it, so there must be some higher level code that checks whether the DB was opened for writing.

Surely "chmod 400 ~/.fossil" is a problem on Unix, too?

db.c, db_open_repository(): Another bogus /cygdrive hack. Remove or replace with a cygwin_conv_path() call.

db.c, line 2115:  Keep this.  It's correct.

file.c, drive letter awareness: Cygwin will yell at you if you use paths like "z:/path/to/whatever" but it does cope with them. Until that changes, Fossil should follow the same rules if it's going to bother caring about path name details in the first place. The code has to be there for the _WIN32 case, so it doesn't cost anything.

Anyone thinking Cygwin is wrong to yell at you for using drive letters should think on why file_without_drive_letter() in file.c doesn't have an "ifdef __CYGWIN__" in it. Try patching your Fossil to make this code run under Cygwin and see how far you get with that.

Those who know why this will fail should think on whether Fossil should be trying to do its own drive letter stuff on Cygwin in the first place.

file.c, backslash hackery: Remove. Cygwin users depending on Fossil to fix backslashes for them need a good smack with a clue-by-four.

file.c, case smashing: Broken, two ways.

First, Cygwin can be configured to be case-sensitive. It should be looking at the Cygwin registry setting, as add.c does.

Second, it's ignoring the "case-sensitive" Fossil user setting. Never mind Cygwin, shouldn't the native Windows build of Fossil obey "=on" if you set it?

sqlite3.c, th_tcl.c: Already dealt with above.

utf8.c: Use iconv(3) in the Unix cases to address the "TODO"s, then get rid of the __CYGWIN__ ifdefs so Cygwin uses this new code. Then you can get rid of the Win32 API prototypes at the top of the file. (No need to #include windows.h here, as with add.c, since the goal is to remove the dependence on the Win32 API here entirely.)

There are also some file name conversions in here, which are adequately addressed by commentary above.
fossil-users mailing list

Reply via email to