On 11/4/2018 7:02 PM, Junio C Hamano wrote:
Ben Peart <peartben@xxxxxxxxx> writes:
+ if (*dtype == DT_UNKNOWN)
+ *dtype = get_dtype(NULL, istate, pathname, pathlen);
We try to defer paying cost to determine unknown *dtype as late as
possible by having this call in last_exclude_matching_from_list(),
and not here. If we are doing this, we probably should update the
callpaths that call last_exclude_matching_from_list() to make the
caller responsible for doing get_dtype() and drop the lazy finding
of dtype from the callee. Alternatively, the new "is excluded from
vfs" helper can learn to do the lazy get_dtype() just like the
existing last_exclude_matching_from_list() does. I suspect the
latter may be simpler.
In is_excluded_from_virtualfilesystem() dtype can't be lazy because it
is always needed (which is why I test and die if it isn't known).
You make a call to that function even when virtual-file-system hook
is not in use, i.e. instead of the caller saying
if (is_vfs_in_use()) {
*dtype = get_dtype(...);
if (is_excluded_from_vfs(...) > 0)
return 1;
}
your caller makes an unconditional call to is_excluded_from_vfs().
Isn't that the only reason why you break the laziness of determining
dtype?
You can keep the caller simple by making an unconditional call, but
maintain the laziness by updating the callig convention to pass
dtype (not *dtype) to the function, e.g..
if (is_excluded_from_vfs(pathname, pathlen, dtype) > 0)
return 1;
and then at the beginning of the helper
if (is_vfs_in_use())
return -1; /* undetermined */
*dtype = get_dtype(...);
... whatever logic it has now ...
no?
Oops! You're right, I docall get_dtype() even if the vfs isn't in use.
I'll add an additional test to avoid doing that. Thank you.
I did look into moving the delay load logic into is_excluded_from_vfs()
but get_dtype() is static to dir.c and I'd prefer to keep it that way if
possible.
Your comments are all feedback on the code - how it was implemented,
style, etc. Any thoughts on whether this is something we could/should
merge into master (after any necessary cleanup)? Would anyone else
find this interesting/helpful?
I am pretty much neutral. Not strongly opposed to it, but not all
that interested until seeing its integration with the "userland" to
see how the whole thing works ;-)
