Re: What's cooking in git.git (Jun 2016, #05; Thu, 16)

From: Joey Hess <id@joeyh.name>
Date: Jul 12, 2016, 22:20 UTC
Message-ID: <20160712222054.GA10128@kitenet.net>
In-Reply-To: <1f76a4d3-68b1-db33-5c7b-dc5ab104a3ef@web.de>

Torsten Bögershausen wrote re jh/clean-smudge-annex:

Show 14 quoted lines

> The thing is that we need to check the file system to find .gitatttibutes,
> even if we just did it 1 nanosecond ago.
> 
> So the .gitattributes is done 3 times:
> -1 would_convert_to_git_filter_fd(
> -2 assert(would_convert_to_git_filter_fd(path));
> -3 convert.c/convert_to_git_filter_fd()
> 
> The only situation where this could be useful is when the .gitattributes
> change between -1 and -2,
> but then they would have changed between -1 and -3, and convert.c
> will die().
> 
> Does it make sense to remove -2 ?

There's less redundant work going on than at first seems, because .gitattribute files are only read once and cached. Verified by strace.

So, the redundant work is only in the processing that convert_attrs() does of the cached .gitattributes.

Notice that there was a similar redundant triple call to convert_attrs() before my patch set:

1. index_fd checks would_convert_to_git_filter_fd
2. index_stream_convert_blob does assert(would_convert_to_git_filter_fd(path))
   (Again redundantly since 1. is its only caller and has already
   checked.)
3. in convert_to_git_filter_fd

If convert_attrs() is somehow expensive, it might be worth passing a struct conv_attrs * into the functions that currently call convert_attrs(). But it does not look expensive to me.

I think it would be safe enough to remove both asserts, at least as the code is structured now.

-- 
see shy jo