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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2019, 03:37 UTC
Message-ID: <xmqq8sp5a6cd.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <609c7c5047719a619ba22425dafc6ecd105e2cda.1572017008.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

> 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?

Show 5 quoted lines

>  	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.