Bug 39914

Has the spec been discussed in the freedesktop.org context?  I'd be more open to implementation if it's moved to fdo.

(In reply to comment #1)
> Has the spec been discussed in the freedesktop.org context?  I'd be more open
> to implementation if it's moved to fdo.

Various backup and archive tools have already adopted the spec, including tar, dar, and obnam.  Various programs already use it to tag their caches, including ccache, bzflag, and libdvdcss.  This seems sufficient to make implementation useful; in my case, I'd like it for use with the --exclude-caches options of both tar and obnam.

Adopting it as an fd.o standard seems like a fine idea, but entirely orthogonal to implementing it, and certainly not a prerequisite. :)

Patches are welcome :).

proposed patch: http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914&id=76cab7541d88c59dcf18c11c6904d382e3ebf4d2

Correct the comment...
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914&id=b6af6a06da68becff835370f4b9e313a2bc1d9de

Has anyone tested or reviewed this patch yet?

Hmm, I guess it needs to be updated a bit for Windows.

(In reply to comment #5)
> Correct the comment...
> http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914&id=b6af6a06da68becff835370f4b9e313a2bc1d9de

Looks mostly reasonable.  You don't need line-continuation backslashes for the string constant, since each line has its own quoted string and all of them will get concatenated.  Also, the sentence "Otherwise it will ignores" needs grammatical fixing.  Other than that, the patch looks fine; ship it.

(In reply to comment #7)
> Hmm, I guess it needs to be updated a bit for Windows.

Other than the use of '/' rather than whatever portable way fontconfig specifies path separators, your patch *looks* like it should work on other platforms.

Thanks for the comment.

http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914

should works...

(In reply to comment #10)
> Thanks for the comment.
> 
> http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914
> 
> should works...

You still have a line continuation character that you don't need.

Otherwise, the patch looks reasonable.  I *think* fontconfig might have an existing mechanism for converting pathnames with '/' to Windows paths with '\\', but what you've written works, and someone with more knowledge of the Windows port can improve it later.

With the last line continuation character removed:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Thanks. fixed in 4f7f3bf9.

First:

+ static const FcChar8 cache_tag_contents[] =
+ "Signature: 8a477f597d28d172789f06886806bc55\n"
+ "# This file is a cache directory tag created by fontconfig.\n"
+ "# For information about cache directory tags, see:\n"
+ "# http://www.brynosaurus.com/cachedir/\n";
+ static size_t cache_tag_contents_size = 0;
+ FcBool ret = FcTrue;
+
+ if (cache_tag_contents_size == 0)
+ cache_tag_contents_size = strlen((char *)cache_tag_contents);

You can simply use "sizeof (cache_tag_contents)".  No need for runtime strlen().


+ write(fd, cache_tag_contents, cache_tag_contents_size);

write() can fail with -EINTR.  Just use stdio or something.


Last but not least.  This is the wrong place to create this file.  Calling fc-cache is by all means considered optional.  The file should be create in the library.  Now, I'm inclined to say we should try creating it only when creating the cache directory, but that wouldn't help existing users.  I'd rather not add a stat() to every application's startup just for this file...

(In reply to comment #13)
> First:
> 
> + static const FcChar8 cache_tag_contents[] =
> + "Signature: 8a477f597d28d172789f06886806bc55\n"
> + "# This file is a cache directory tag created by fontconfig.\n"
> + "# For information about cache directory tags, see:\n"
> + "# http://www.brynosaurus.com/cachedir/\n";
> + static size_t cache_tag_contents_size = 0;
> + FcBool ret = FcTrue;
> +
> + if (cache_tag_contents_size == 0)
> + cache_tag_contents_size = strlen((char *)cache_tag_contents);
> 
> You can simply use "sizeof (cache_tag_contents)".  No need for runtime
> strlen().

If you want to use sizeof, you'd need sizeof(cache_tag_contents)-1, since sizeof counts the trailing '\0'.  More importantly, though, compilers such as GCC know how to optimize strlen(constant_string) to a constant.

> + write(fd, cache_tag_contents, cache_tag_contents_size);
> 
> write() can fail with -EINTR.  Just use stdio or something.

True.

Thanks for the comment.

(In reply to comment #13)
> Last but not least.  This is the wrong place to create this file.  Calling
> fc-cache is by all means considered optional.  The file should be create in the
> library.  Now, I'm inclined to say we should try creating it only when creating
> the cache directory, but that wouldn't help existing users.  I'd rather not add
> a stat() to every application's startup just for this file...

Then we should keep the function in fc-cache to create this file at least. we could simply skip this process when updating caches at the startup time if necessary. creating CACHEDIR.TAG would be an optional then.

Maybe you can do it in fc-cache, and in the library only when we are creating a non-existing cache directory.

(In reply to comment #16)
> Maybe you can do it in fc-cache, and in the library only when we are creating a
> non-existing cache directory.

Infact that is necessary.  For the system-wide cache dir, rpm, etc, call fc-cache and the directory already exists.  For user fonts, user never calls fc-cache, and the first time the directory does not exist.  So we need both paths.

(In reply to comment #17)
> (In reply to comment #16)
> > Maybe you can do it in fc-cache, and in the library only when we are creating a
> > non-existing cache directory.
> 
> Infact that is necessary.  For the system-wide cache dir, rpm, etc, call
> fc-cache and the directory already exists.  For user fonts, user never calls
> fc-cache, and the first time the directory does not exist.  So we need both
> paths.

Sure. the easier way to share the efforts would be to have an API to do it in the library and call it as needed though, I don't think it's necessary for public and would rather be better doing it internally. so I'm pondering to have it as the internal API though, you said in another bug the tools like fc-cache and so on shouldn't include fcint.h. I don't want to have duplicate code as far as possible though...

Yes, all the utilities just use the public API.  We sure can add new API for this.

Okay, the patch is updated:
http://cgit.freedesktop.org/~tagoh/fontconfig/commit/?h=bz39914

hmm, just wondering if there are any way to hide this API. actually it's not related to fontconfig functionality at all. we should export scanDirs() in fc-cache instead of it perhaps?

reopened anyway. it's hard to keep it on track with FIXED status.

Okay, added instread:

FcPublic void
FcCacheCreateTagFile (const FcConfig *config);

that should be better than taking the cache_dir directly.

fixed in 06d6b7c3.