Re: [PATCH 1/1] unpack-trees: skip lstat based on fsmonitor

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

"Utsav Shah via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Utsav Shah <utsav@dropbox.com>
>
> git stash runs git reset --hard as its final step, which can be fairly slow on a large repository.
> This change lets us skip that if fsmonitor thinks those files aren't modified.
>
> git stash goes from ~8s -> 2s on my repository after this change.

Please line-wrap overlong lines.

More importantly, "stash" may be a mere symptom that does not
deserve this much emphasis.  What you improved directly is "git
reset --hard" isn't it?

    The fsmonitor may know that a path hasn't been modified but 
    "git reset --hard" did not pay attention to it and performed
    its own check based on ie_match_stat(), which was inefficient.

or something like that?

>  	if (old && same(old, a)) {
>  		int update = 0;
> -		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old)) {
> +		if (o->reset && o->update && !ce_uptodate(old) && !ce_skip_worktree(old) &&
> +		    !(old->ce_flags & CE_FSMONITOR_VALID)) {

I wonder if !ce_uptodate(old) should say "this one is up to date and
not modified" when CE_FSMONITOR_VALID bit is set.  Are there other
codepaths that use ce_uptodate(ce) to decide to do X without paying
attention to CE_FSMONITOR_VALID bit?  If there are, are they buggy
in the same way as you found this instance, or do they have legitimate
reason why they only check ce_uptodate(ce) and ignore fsmonitor?

If there isn't, would it make sense to get rid of CE_FSMONITOR_VALID
bit and have fsmonitor directly set CE_UPTODATE bit instead?  That
would make this fix unnecessary and fix other codepaths that check
only ce_uptodate() without checking fsmonitor.