From: "Adam Johnson" <me@adamj•eu>
To: "Junio C Hamano" <gitster@pobox•com>,
"Adam Johnson" <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, "Thomas Gummerer" <t.gummerer@gmail•com>,
"Elijah Newren" <newren@gmail•com>,
"Phillip Wood" <phillip.wood@dunelm•org.uk>,
"Victoria Dye" <vdye@github•com>
Subject: Re: [PATCH] stash: reuse cached index entries in --patch temporary index
Date: Fri, 22 May 2026 21:53:53 +0100 [thread overview]
Message-ID: <9e2058b9-4c0d-4c4b-8a65-0eb4869a815c@app.fastmail.com> (raw)
In-Reply-To: <xmqqse7m6deh.fsf@gitster.g>
> I however have to wonder if simply replacing the external process
> invocation with "git read-tree -m HEAD" (i.e., oneway merge) gives
> a similar speed-up.
Good idea, I just tried this, but it does not help. The subprocess runs
with GIT_INDEX_FILE set to a temporary index, so oneway_merge
never uses the CE_FSMONITOR_VALID fast path.
The in-process approach is necessary because it lets us set
opts.src_index to the real index with cached stat and fsmonitor data,
before switching GIT_INDEX_FILE.
> What I read from the proposed log message is that the change is
> purely about performance and should not change any behaviour. Why
> do we need a new test in t/t3904? I would not have surprised if we
> saw a new test in t/perf/, though.
Ah yeah, my bad. I added this while iterating to catch a bug I introduced,
but it's not necessary for the final patch. Will remove.
On Wed, 20 May 2026, at 03:08, Junio C Hamano wrote:
> "Adam Johnson via GitGitGadget" <gitgitgadget@gmail•com> writes:
>
> > From: Adam Johnson <me@adamj•eu>
> >
> > `git stash -p` prepares the interactive selection by creating a
> > temporary index at HEAD, switching `GIT_INDEX_FILE` to it, and then
> > running the `add -p` machinery.
> >
> > That temporary index was created by running `git read-tree HEAD`. The
> > resulting index had no useful cached stat data or fsmonitor-valid bits
> > from the real index. When `run_add_p()` refreshed that temporary index
> > before showing the first prompt, it could end up lstat(2)-ing every
> > tracked file, even in a repository where `git diff` and `git restore -p`
> > can use fsmonitor to avoid that work.
> >
> > Create the temporary index in-process instead. Use `unpack_trees()` to
> > reset the real index contents to HEAD while writing the result to the
> > temporary index path. For paths whose index entries already match HEAD,
> > `oneway_merge()` reuses the existing cache entries, preserving their
> > cached stat data and `CE_FSMONITOR_VALID` state.
>
> Clever. As the fsmonitor_valid bit is in-core only, updating the
> index in-process would be an obvious and probably the only sensible
> way to preserve it.
>
> I however have to wonder if simply replacing the external process
> invocation with "git read-tree -m HEAD" (i.e., oneway merge) gives
> a similar speed-up.
>
> > This makes the refresh performed by `run_add_p()` behave like the one
> > used by `git restore -p`: unchanged paths can be skipped via fsmonitor
> > instead of being scanned again.
> >
> > In a 206k file repository with `core.fsmonitor` enabled and a one-line
> > change in one file, time to first prompt dropped from 34.774 seconds to
> > 0.659 seconds.
>
> Interesting.
>
> > diff --git a/t/t3904-stash-patch.sh b/t/t3904-stash-patch.sh
> > index 90a4ff2c10..4b3241c8cd 100755
> > --- a/t/t3904-stash-patch.sh
> > +++ b/t/t3904-stash-patch.sh
> > @@ -84,6 +84,24 @@ test_expect_success 'none of this moved HEAD' '
> > verify_saved_head
> > '
> >
> > +test_expect_success 'stash -p with unmodified tracked files present' '
> > + git reset --hard &&
> > + echo line1 >alpha &&
> > + echo line1 >beta &&
> > + git add alpha beta &&
> > + git commit -m "add alpha and beta" &&
> > + echo line2 >>alpha &&
> > + echo y | git stash -p &&
> > + echo line1 >expect &&
> > + test_cmp expect alpha &&
> > + test_cmp expect beta &&
> > + git stash pop &&
> > + printf "line1\nline2\n" >expect &&
> > + test_cmp expect alpha &&
> > + echo line1 >expect &&
> > + test_cmp expect beta
> > +'
>
> What I read from the proposed log message is that the change is
> purely about performance and should not change any behaviour. Why
> do we need a new test in t/t3904? I would not have surprised if we
> saw a new test in t/perf/, though.
>
> Thanks.
>
next prev parent reply other threads:[~2026-05-22 20:54 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-19 12:43 [PATCH] stash: reuse cached index entries in --patch temporary index Adam Johnson via GitGitGadget
2026-05-20 2:08 ` Junio C Hamano
2026-05-22 20:53 ` Adam Johnson [this message]
2026-05-20 2:26 ` Junio C Hamano
2026-05-22 20:55 ` Adam Johnson
2026-05-22 23:12 ` [PATCH v2] " Adam Johnson via GitGitGadget
2026-06-01 21:33 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9e2058b9-4c0d-4c4b-8a65-0eb4869a815c@app.fastmail.com \
--to=me@adamj$(echo .)eu \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=newren@gmail$(echo .)com \
--cc=phillip.wood@dunelm$(echo .)org.uk \
--cc=t.gummerer@gmail$(echo .)com \
--cc=vdye@github$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox