Created attachment 238033 [details] [review]
live-g-file: test hidden files

Since the patch uses g_file_set_contents(), which overwrites files, this wouldn't work if we used it for two files; but since we don't and I didn't want to complicate the patch too much, I've just used that.

Created attachment 238034 [details] [review]
Don't use PATH_MAX

This patch uses getline(), which is standard since POSIX.1-2008, which I guess is fine. If it's not I can add appropriate checks and only use it if HAVE_GETLINE is defined.

Nevermind those patches, I've found an issue with them; I'll fix it and repost them.

Created attachment 238041 [details] [review]
live-g-file: test hidden files

This version of the first patch doesn't overwrite the .hidden file. It now tests with two files in the .hidden file to make sure it works fine.

Made a mistake while testing this new version and thought the other patch was broken, but it's fine. The tests pass with and without PATH_MAX.

getline() is a GNU extension that only made it into POSIX in 2008.  This is going to cause more compatibility issues than PATH_MAX ever did....

Created attachment 238045 [details] [review]
Don't use PATH_MAX

Understandable.

Checking for getline in configure and then using it if available and fgets if not is going to make the code ugly, so this new version just uses g_file_get_contents() and makes the code more readable than the initial version (IMHO).

This is a better approach but pretty inefficient...  particularly since you keep measuring the length of the strv on each loop iteration... but also because it creates/destroys quite a lot of heap objects.

Can't we just define PATH_MAX to something large in the case that we don't have it from the OS?

(In reply to comment #7)
> This is a better approach but pretty inefficient...  particularly since you
> keep measuring the length of the strv on each loop iteration... but also
> because it creates/destroys quite a lot of heap objects.

Should be pretty easy to fix the patch to pass ownership of the elements to the hash, and then just g_free() the strv container.

> Can't we just define PATH_MAX to something large in the case that we don't have
> it from the OS?

The Hurd people dislike that since the whole point of their OS is to allow arbitrarily long filenames and stuff.

The argument against PATH_MAX I see is quite simply that we almost never use it elsewhere in GLib, so let's be consistent.

Created attachment 238142 [details] [review]
Don't use PATH_MAX

Thank you for the reviews.

Here's a new version of the patch that should address them.

- It only calculates the number of elems in the array once.
- It reduces the number of allocations to n + 2 (where n = number of entries in the .hidden file). One for g_file_get_contents(), n+1 in g_strsplit() (one per element plus the array). The status quo does n allocations as it needs to g_memdup() each entry into the hash table, so there's not a big performance loss.

Created attachment 238722 [details] [review]
glocalfileinfo: Stop using PATH_MATH for .hidden

We were using PATH_MAX to size a static array for reading lines from
the .hidden file.  Some platforms (Hurd) don't declare a PATH_MAX.

Switch to using g_file_get_contents() and g_str_split('\n') instead.

Also take the time to clean up a bit with a switch to using a 'set mode'
GHashTable (since this code was originally written before we had those).

This patch is largely based on a patch from Emilio Pozuelo Monfort (who
also reported the bug).

===

Here's an updated patch that is better in a few ways.  Review?

Created attachment 238723 [details] [review]
glocalfileinfo: Stop using PATH_MATH for .hidden

Sorry about that.  git merge chewed the first patch.

Review of attachment 238723 [details] [review]:

Looks right to me.

Except s/PATH_MATH/PATH_MAX/ in the commit message of course.

Attachment 238041 [details] pushed as 605c4ca - live-g-file: test hidden files