From: Torsten Bögershausen Date: Thu, 14 Jul 2016 02:09:02 GMT Subject: Re: What's cooking in git.git (Jun 2016, #05; Thu, 16) Message-ID: In-Reply-To: <20160712222054.GA10128@kitenet.net> On 07/13/2016 12:20 AM, Joey Hess wrote: > Torsten Bögershausen wrote re jh/clean-smudge-annex: >> 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. > OK, I think I missed that work (not enough time for Git at the moment) Junio, please help me out, do we have a cache here now? I tried to figure out that following your attr branch, but failed. > 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 have that on the list, but seems to be uneccesary now. > > I think it would be safe enough to remove both asserts, at least as the > code is structured now. > OK.