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
fossil-users@lists.fossil-scm.org
http://lists.fossil-scm.org:8080/cgi-bin/mailman/listinfo/fossil-users
