Re: g_file_test()

[Owen Taylor <otaylor redhat com>]

Tim Janik <timj gtk org> writes:

> On 3 Sep 2001, Owen Taylor wrote:
> 
> > Tim Janik <timj gtk org> writes:
> > 
> > > On Mon, 6 Aug 2001, Tim Janik wrote:
> > > 
> > > > 
> > > > typedef enum
> > > > {
> > > >   G_FILE_TEST_IS_REGULAR    = 1 << 0,
> > > >   G_FILE_TEST_IS_SYMLINK    = 1 << 1,
> > > >   G_FILE_TEST_IS_DIR        = 1 << 2,
> > > >   G_FILE_TEST_IS_EXECUTABLE = 1 << 3,
> > > >   G_FILE_TEST_EXISTS        = 1 << 4
> > > > } GFileTest;
> > > > 
> > > > gboolean g_file_test         (const gchar  *filename,
> > > >                               GFileTest     test);
> > > > 
> > > > having used g_file_test() in a couple places now, i have to say that:
> > > > 1) it definitely needs G_FILE_TEST_IS_READABLE and G_FILE_TEST_IS_WRITABLE
> > > > 2) it should return TRUE only if all tests succeeded.
> > > 
> > > are there actually any objections/concerns not yet raised on this issue?
> > > if not, i'd like to fix matters ASAP.
> > 
> > OK, the original discussion is at:
> > 
> >  http://mail.gnome.org/archives/gtk-devel-list/2000-October/msg00154.html
> > 
> > The primary concern that came up with making it all-of is that 
> > that would conflict with the current semantics of g_file_test()
> > in gnome-libs, which I think is 
> 
> first, and for the record, i strongly dislike the idea of gnome-libs introducing
> random g_* stuff with broken semantics, and then relying on us to fold those
> changes back into glib while keeping their broken semantics. i consider this
> illegitimate namespace clutter. that being said, read below on how i think we
> should handle the current situation.

I think people agree that this was broken now. 

Be that as it may _we cannot change the meaning of g_file_test() just
because we think that gnome-libs using that name.
 
> > I agree that all-of is more what I would expect, but I don't
> > think the any-of behavior is so ridiculous that I would be
> > willing to declare programs that expect it buggy. I think if
> > we change the behavior, we have to change the name.
> 
> grepping through my checked out GNOME CVS stuff, the only case where
> OR semantics are currently being used is stuff like:
> 
> ./gnome-libs/libgnomeui/gnome-icon-entry.c:     if(!p || !g_file_test (p,G_FILE_TEST_ISLINK|G_FILE_TEST_ISFILE) ||
> ./gnome-libs/libgnomeui/gnome-pixmap-entry.c:   if(!p || !g_file_test(p,G_FILE_TEST_ISLINK|G_FILE_TEST_ISFILE) ||
> ./nautilus/libnautilus-private/nautilus-theme.c: || !g_file_test (theme_to_install_path, G_FILE_TEST_EXISTS | G_FILE_TEST_ISDIR)) {
> ./control-center/capplets/background-properties/property-background.c:  if(!p || !g_file_test (p,G_FILE_TEST_ISLINK|G_FILE_TEST_ISFILE) ||
> ./control-center/capplets/file-types/file-types-icon-entry.c:   if(!p || !g_file_test (p,G_FILE_TEST_ISLINK|G_FILE_TEST_ISFILE)
> ./glib/gmodule/gmodule.c:      if (!g_file_test (file_name, G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR))
> 
> so there're three cases to isolate:
> nautilus:   G_FILE_TEST_EXISTS | G_FILE_TEST_ISDIR	(old API)
> gnome-libs: G_FILE_TEST_ISLINK | G_FILE_TEST_ISFILE	(old API)
> glib:       G_FILE_TEST_EXISTS | G_FILE_TEST_IS_REGULAR	(new API)
> 
> the test in nautilus is somewhat bogus, it isn't actually assuming OR semantics,
> since if ISDIR succeeds, EXISTS also will. the author probably meant "file exists
> and is a directory", i.e. AND semantics (though EXISTS is extraneous here).
> 
> the gnome-libs test for G_FILE_TEST_ISLINK | G_FILE_TEST_ISFILE is probably
> the only one that code that authors would actually want to make with OR
> semantics, these kinds of old checks will be catched with glib-2.0 though,
> because we'ce changed the API to read G_FILE_TEST_IS_SYMLINK |
> G_FILE_TEST_IS_REGULAR here (i.e. different flag names).

The plan was to keep the flag values in GLib the same as the values in
gnome-libs, and leave the old values around so we didn't just break
all uses of g_file_test(). This is how libgnome for GNOME-2.0 works
now.

> this is not a very usefull test though, since we're using stat(2) not
> lstat(2) in g_file_test().

Should we be using lstat not stat? Or should we be using lstat for
SYMLINK and fstat for everything else? (Perl seems to do this for -l
with a quick strace checck.) I don't think thecurrent behavior is what
people would expect. 
 
> > (I'd think the reason why it's any-of is what you get when you
> > read:
> > 
> >  g_file_test (G_FILE_TEST_IS_REGULAR | G_FILE_TEST_IS_SYMLINK);
> > 
> > Yep, "test file is regular or file is symlink".)
> 
> there's no real point in making this check:

Errr, you just spent a lot of time proving that my randomly written
example of syntax wasn't useful....

> > Possibilities for an all-of name include:
> > 
> >  g_file_test_is_all ()
> >  g_file_test_all ()
> >  g_file_is_all ()
> >  g_file_is ()
> 
> if we did introduce g_file_test_all() for having AND semantics,
> we should still remove g_file_test(), since most people will assume
> AND semantics intuitively.

I'm not sure about this. Apparently, whoever wrote g_file_test() originally
expected OR semantics...

> i don't think we'll introduce real problems with just changing g_file_test()
> to have AND semantics though, the old API uses of G_FILE_TEST_ISLINK |
> G_FILE_TEST_ISFILE will be caught and have to be changed anyways.
> and in fact, we should get rid of G_FILE_TEST_IS_SYMLINK, since we don't use
> lstat(2).
> 
> > I think G_FILE_TEST_IS_READABLE/IS_WRITEABLE are open to abuse, and
> > its pretty hard to get them exactly right, considering things like:
> > 
> >  - read-only mounts
> >  - Multiple groups for a user
> >  - ACLs on AFS
> >  - Behavior of access as root and with setuid
> > 
> > Basically, the only way to safely tell if you would be able
> > to read/write a file is to actually do it. 
> > 
> > Does that mean we shouldn't add them? It may be that if we don't,
> > people will write the tests themselves and do it worse.  I don't
> > know...
> 
> read/write are no different to handle than the current
> G_FILE_TEST_IS_EXECUTABLE which is done via acces(2).
> so adding G_FILE_TEST_IS_READABLE and G_FILE_TEST_IS_WRITEABLE just
> means adding R_OK and W_OK to the mask passed in to access(2) (which
> also provides AND semantics already).
> 
> > When do you see them being useful?
> 
> it should be clear that code which reads from/writes to a file/device should
> sanely handle errors already (EINTR, disk full and whatnot), but with
> that in mind, adding readable/writable flags to the test mask if you have
> to do g_file_test() anyways, can be quite convenient. e.g. to figure whether
> a device or a directory contained in a PATH-alike variable is readily accessible
> (and before handling it over to some lower layers which do the actual IO).

This is usually a symptom of API's like the old gdk-pixbuf API which
don't provide proper error handling... but anyways, I don't really care
much about this issue, except that we really cannot in any circumstances
change g_file_test() from being or to and. That just goes against any
principle of API maintainence.

Regards,
                                               Owen