Re: [PATCH] fsmonitor: check the untracked cache after a trivial response

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

>> +	for (i = 0; i < ucd->dirs_nr; i++) {
>> +		/* not written to the index, see write_one_dir() */
>> +		if (!ucd->dirs[i]->recurse)
>> +			continue;
>> +		strbuf_addch(path, '/');
>> +		strbuf_addstr(path, ucd->dirs[i]->name);
>> +		nr_invalidated += invalidate_stale_dirs(uc, ucd->dirs[i],
>> +							istate, path);
>
> I'm conditioned to worry about recursive calls without depth limits, but
> I suppose that we are limited by the untracked cache data structure itself,
> which was presumably created recursively without hitting stack limits.
>
> We could make this iterative using a stack-based data structure, but that
> would be more complicated than this is worth, probably. These are not used
> in server environments and the .git/index file shouldn't be considered
> "unsafe" or else we have bigger safety problems.

Good reasoning.  Thanks for spelling it out.

>> +	/*
>> +	 * The index is also read and written by commands that do not
>> +	 * run in the top-level directory of the worktree.
>> +	 */
>> +	worktree = repo_get_work_tree(istate->repo);
>> +	if (!worktree)
>> +		return 0;
>
> I wonder if this is the right response if we fail to get a worktree. Maybe
> we should invalidate the entire untracked cache in this case, along with a
> warning that we couldn't evaluate the worktree? Or should we return -1 to
> signal an error?

It probably is safer to invalidate and continue.  If there is truly
not a worktree, untracked cache entries do not matter ;-)

Thanks for a review.