Re: [PATCH 3/3] completion: fix bugs with slashes in remote names

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Mar 3, 2025, 19:33 UTC
Message-ID: <Z8YEIo28bLS+t+20@szeder.dev>
In-Reply-To: <2323bb52-f43d-4f40-8955-4c648677a93e@mandelberg.org>

On Sun, Mar 02, 2025 at 03:34:07PM -0500, David Mandelberg wrote:

Show 28 quoted lines

> Op 2025-03-02 om 09:17 schreef Phillip Wood:
> > Hi David
> > 
> > On 02/03/2025 07:45, David Mandelberg via GitGitGadget wrote:
> > > From: David Mandelberg <david@mandelberg.org>
> > > 
> > > Previously, some calls to for-each-ref passed fixed numbers of path
> > > components to strip from refs, assuming that remote names had no slashes
> > > in them. This made completions like:
> > > 
> > > git push github/dseomn :com<Tab>
> > > 
> > > Result in:
> > > 
> > > git push github/dseomn :dseomn/completion-remote-slash
> > > 
> > > With this patch, it instead results in:
> > > 
> > > git push github/dseomn :completion-remote-slash
> > 
> > This sounds like a useful improvement and I like the idea, but I think
> > running "git for-each-ref" once for each remote is not going to scale
> > very well for people who have a lot of remotes. I think it would be
> > better to try and strip "refs/remote/$remote/" outside of "git for-each-
> > ref". I've not tested it but I think something like
> 
> Good point, I hadn't thought of that. Do you have a rough estimate of what
> "a lot of remotes" is? 100ish, maybe?

5.

In Git for Windows fork()-ing subshells and fork()+exec()-ing processes is rather costly, about an order of magnitude slower than on Linux. The rough equivalent of the body of your loop, with two subshells and a git process:

  time { a=$(echo 1) ; b=$(echo 2) ; git for-each-ref >/dev/null ; }

takes on average 0.17s on a windows box I have access to (with fully packed refs, and merely 4 refs in total). So guess at about 4-5 remotes it would take over a second to react to my TABs...

I would rather try to go in the opposite direction to see whether 'git for-each-ref' could be taught to strip the "refs/remote/$remote/" prefix with a format specifier option like '%(refname:strip=remote)'.

That would surely be faster than any shell filtering we might come up with, and would also save us from the trouble of escaping glob and/or regex metacharacters for shell/sed pattern matching.

Show 17 quoted lines

> I'd like to do some testing to get
> actual performance numbers before trying to optimize this, because I think
> the optimization has some drawbacks, see below.
> 
> If optimization is needed, another approach is to parallelize the forks:
> 
> {
> 	local fer_pids=
> 	for ...
> 	do
> 		__git for-each-ref ... &
> 		fer_pids="$fer_pids $!"
> 	done
> 	test -z "$fer_pids" || wait $fer_pids
> } | sort | uniq -u
> 
> That might cause spikes in cpu/memory/disk usage that aren't ideal though.

Please don't do this, the completion script must not forkbomb the system.