Re: [RFC/PATCH v5] git on Mac OS and precomposed unicode

Torsten Bögershausen [off-list ref] writes:

Knowing that Mac OS X writes file names as precomposed to disk,
and treats precomposed and decomposed file names as equal, git under Mac OS X
can be improved to revert the unicode decomposition of file names.

The basic idea looks really good, but there are a few nits.

Thanks.

The user needs to activate this feature manually.
She typically sets core.precomposedunicode to "true" on HFS and VFAT,
or file systems mounted via SAMBA onto a Linux box.

What do you mean by the last part?  A Linux box that mounts a
filesystem from MacOS X box via cifs?  Or MacOS X mounts a
filesystem from a Linux box over cifs?

+core.precomposedunicode::
+	This option is only used by Mac OS implementation of git.
+	When core.precomposedunicode=true, git reverts the unicode decomposition
+	of filenames done by Mac OS. This is useful when sharing a repository
+	between Mac OS and Linux or	Windows.

Why this funny inter-word spacing?

+	(Git for Windows/msysGit 1.7.10 is needed, or git under cygwin 1.7).
+	When false, file names are handled fully transparent by git, which means
+	that file names are stored as decomposed unicode in the	repository.

I am assuming, after reading from this section, that the answer to
my earlier question is "MacOS X that mounts over cifs from whatever
file server" (i.e. the latter).  

It often is a good idea, after writing "X , which means that Y", if
you can just drop X and go strait to Y.  The result often becomes
much more clear.

As this section is clearly labeled as "Mac OS only", I think in this
case it is OK to say "when false, file names are always stored as
decomposed unicode..." for now.

Given that your compat/precomposed_utf8.[ch] looks like it does not
use anything MacOS X specific (other than "UTF-8-MAC" given to
iconv(3) API), do you foresee that this might become useful on non
Mac build of Git in the future?

Probing for case-insensitiveness and probing for UTF-8 mangling are
logically related and do not deserve a separation with the extra
blank line.

Do you really need "../"?  I thought we compiled with -I.. from the
Makefile.

+#include <stdlib.h>
+#include <string.h>
+#include <stdio.h>
+#include <stdint.h>

And as you are including "cache.h" (which in turn includes
"git-compat-util.h"), I doubt you would need to include these
yourself, and if you do not need them, I would prefer not to see
them.

+#include "precomposed_utf8.h"
+
+const static char *repo_encoding = "UTF-8";
+const static char *path_encoding = "UTF-8-MAC";
+
+
+/* Code borrowed from utf8.c */
+#if defined(OLD_ICONV) || (defined(__sun__) && !defined(_XPG6))
+	typedef const char * iconv_ibp;
+#else
+	typedef char * iconv_ibp;
+#endif

Seeing "defined(__sun__)" here, is this not for Mac OS X-only after
all?  If so, I'd have to ask you to come up with a better name than
"mac_os_precomposed_unicode". It is primarily about sanitizing
decomposed utf-8 and it only is a happenstance that Mac OS X is the
most likely platform people may need this feature on (iow, "mac_os"
is not the fundamental part of this issue; having to sanitize paths
of decomposed UTF-8 is).

+static char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv)
+{
+	size_t outsz, outalloc;
+	char *out, *outpos;
+	iconv_ibp cp;
+
+	outsz = insz;
+	outalloc = outsz + 1; /* for terminating NUL */
+	out = xmalloc(outalloc);
+	outpos = out;
+	cp = (iconv_ibp)in;
+
+	while (1) {
+	...
+	}
+	return out;
+}

Shouldn't this part be using a new helper function refactored out of
the utf8.c::reencode_string() function, instead of cutting and
pasting?

+static size_t has_utf8(const char *s, size_t maxlen, size_t *strlen_c)
+{
+	const uint8_t *utf8p = (const uint8_t*) s;
+	size_t strlen_chars = 0;
+	size_t ret = 0;
+
+	if ((!utf8p) || (!*utf8p))
+		return 0;
+
+	while((*utf8p) && maxlen) {
+		if (*utf8p & 0x80)
+			ret++;
+		strlen_chars++;
+		utf8p++;
+		maxlen--;
+	}
+	if (strlen_c)
+		*strlen_c = strlen_chars;
+
+	return ret;
+}
+
+
+void probe_utf8_pathname_composition(char *path, int len)
+{
+	const static char *auml_nfc = "\xc3\xa4";
+	const static char *auml_nfd = "\x61\xcc\x88";
+	int output_fd;
+	if (mac_os_precomposed_unicode != -1)
+		return; /* We found it defined in the global config, respect it */
+	path[len] = 0;
+	strcpy(path + len, auml_nfc);
+	output_fd = open(path, O_CREAT|O_EXCL|O_RDWR, 0600);
+	if (output_fd >=0) {
+		close(output_fd);
+		path[len] = 0;
+		strcpy(path + len, auml_nfd);
+		/* Indicate the the user, that we can configure it to true */
+		if (0 == access(path, R_OK))
+			git_config_set("core.precomposedunicode", "false");
+		path[len] = 0;
+		strcpy(path + len, auml_nfc);
+		unlink(path);
+	}

Now this function has figured out if the filesystem mangles composed
UTF-8 pathnames, shouldn't it flip mac_os_precomposed_unicode to
either 0 or 1 before it returns?

+}
+
+
+void precompose_argv(int argc, const char **argv)
+{
+	int i = 0;
+	const char *oldarg;
+	char *newarg;
+	iconv_t ic_precompose;
+
+	if (mac_os_precomposed_unicode != 1)
+		return;

Could the mac_os_precomposed_unicode flag be -1 (unknown) here?
Otherwise just write

	if (!mac_os_precomposed_unicode)
        	return;

+PRECOMPOSED_UTF_DIR * precomposed_utf8_opendir(const char *dirname)

Asterisk sticks to the identifier, i.e.

    PRECOMPOSED_UTF_DIR *precomposed_utf8_opendir(const char *dirname)

+{
+	PRECOMPOSED_UTF_DIR *precomposed_utf8_dir;
+	precomposed_utf8_dir = xmalloc(sizeof(PRECOMPOSED_UTF_DIR));
+
+	precomposed_utf8_dir->dirp = opendir(dirname);
+	if (!precomposed_utf8_dir->dirp) {
+		free(precomposed_utf8_dir);
+		return NULL;
+	}
+	precomposed_utf8_dir->ic_precompose = iconv_open(repo_encoding, path_encoding);
+	if (precomposed_utf8_dir->ic_precompose == (iconv_t) -1) {
+		closedir(precomposed_utf8_dir->dirp);
+		free(precomposed_utf8_dir);
+		return NULL;

Hrm, I wonder what value the "errno" variable should be set to when
this happens.

+	}
+
+	return precomposed_utf8_dir;
+}
+
+struct dirent * precomposed_utf8_readdir(PRECOMPOSED_UTF_DIR *precomposed_utf8_dirp)

Likewise.

+{
+	struct dirent *res;
+	size_t namelen = 0;
+
+	res = readdir(precomposed_utf8_dirp->dirp);
+	if (res && (mac_os_precomposed_unicode == 1) && has_utf8(res->d_name, (size_t)-1, &namelen)) {

Likewise.

+		int ret_errno = errno;
+		size_t outsz = sizeof(precomposed_utf8_dirp->dirent_nfc.d_name) - 1; /* one for \0 */

A few issues:

 - Why (-1)?  Don't you need terminating NUL at the end of the
   output anyway?

 - sizeof(d_name) is most likely incorrect (Cf.
   http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html)

 - Is the reason why it is appropriate to update "res" in-place
   because turning a decomposed name into precomposed form will
   always yield a shorter result?  Is that guaranteed?

+		/* Copy all data except the name */
+		memcpy(&precomposed_utf8_dirp->dirent_nfc, res,
+		       sizeof(precomposed_utf8_dirp->dirent_nfc)-sizeof(precomposed_utf8_dirp->dirent_nfc.d_name));
+		errno = 0;
+
+		cnt = iconv(precomposed_utf8_dirp->ic_precompose, &cp, &insz, &outpos, &outsz);
+		if (cnt < sizeof(precomposed_utf8_dirp->dirent_nfc.d_name) -1) {

s/-1)/- 1)/;

Can't this iconv() fail and return -1 here, which would be smaller
than the size of the structure minus one?

+			*outpos = 0;
+			errno = ret_errno;
+			return &precomposed_utf8_dirp->dirent_nfc;
+		}
+		errno = ret_errno;
+	}
+	return res;
+}

The same comments for #include applies here.

+typedef struct {
+	iconv_t ic_precompose;
+	DIR *dirp;
+	struct dirent dirent_nfc;
+} PRECOMPOSED_UTF_DIR;

Note that "struct dirent" can be defined with d_name[1]; aren't you
risking the memory after this structure to be overwritten with a
longer name?