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

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

Op 2025-03-02 om 09:17 schreef Phillip Wood:

quoted

Hi David

On 02/03/2025 07:45, David Mandelberg via GitGitGadget wrote:

quoted

From: David Mandelberg <redacted>

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.

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.