Re: [PATCH] t3910: show failure of core.precomposeunicode with decomposed filenames

[Jeff King <peff@peff.net>]

On Mon, Apr 28, 2014 at 03:35:02PM -0400, Jeff King wrote:

> Since such entries are in the minority, and because cache_entry is
> already a variable-length struct, I think you could get away with
> sticking it after the "name" field, and then comparing like:
> 
>   const char *ce_normalized_name(struct cache_entry *ce, size_t *len)
>   {
> 	const char *ret;
> 
> 	/* Normal, fast path */
> 	if (!(ce->ce_flags & CE_NORMALIZED_NAME)) {
> 		len = ce_namelen(ce);
> 		return ce->name;
> 	}
> 
> 	/* Slow path for normalized names */
> 	ret = ce->name + ce->namelen + 1;
> 	*len = strlen(name);
> 	return ret;
>   }

That's the reading half. We would also need to create the normalized
names for each cache_entry. I took a look at that this afternoon. It
turns out we make cache_entry structs in quite a few places.  So I
thought I'd start with converting them all to a function like:

  struct cache_entry *cache_entry_alloc(const char *name, size_t len);

And then once converted, we could teach it to normalize the name as
appropriate. That interface does improve many of the callers, but there
are a few tricky ones.

For example, in checkout.c:update_some (and one or two other spots), we
actually have the path broken into two parts, and we combine them while
writing into the cache_entry. We could obviously combine them into a
single buffer beforehand, but that means extra copying in reasonably hot
code paths. It would be slightly ugly but perhaps reasonable to have:

  cache_entry_alloc_two(const char *one, size_t one_len,
                        const char *two, size_t two_len);

But then I got to unpack-trees. It has the whole path broken down across
a linked list.  I'm not sure what would be least terrible interface
here. Again, we could format to a buffer and copy, but I'm hesitant to
do it on this code path.

I'm not sure if I'm just being paranoid about the extra memcpys (it's
not _that_ tight a loop, since after all, we are generally zlib
inflating to get the tree data, and the filenames are not all _that_
long).

I dunno. I just hate the idea of tradeoffs for this OS-X-only fix
permeating the rest of the code on other platforms. But maybe Knuth
should be hitting me with his premature optimization clue-stick.

-Peff