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

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

local sed_cmd=
local remote
# ref names and therefore remote names cannot contain '*?[]^' so we
# only need to escape '.$/'. Using 'sort -r' means that if there is a
# remote called "github" and another called "github/alice" we will try
# and strip "github/alice" first.
for remote in $(__git_remotes | sort -r)
do
     remote="${remote//./\\./}"
     remote="${remote//\$/\\\$/}"
     remote="${remote//\//\\\//}"

Just FYI since it took me hours to figure this out myself: I think this 
would break tests on macos because of an old version of bash that 
handles backslashes weirdly. I think removing the double quotes would 
work around that issue, and be safe because word splitting doesn't 
happen in assignments.

     sed_cmd="${sed_cmd} -e s/^refs\/remotes\/$remote\/// -e t"
done

Mostly just a note to myself if I end up using this idea: I think a 
space in $remote would break this because bash would split up the arg to 
sed. There's probably some way to fix that with extra escaping though?

__git for-each-ref --format="$fer_pfx$sfx" \
     ${GIT_COMPLETION_IGNORE_CASE+--ignore-case} \
     "refs/remotes/*/$cur_*" "refs/remotes/*/$cur_*/**" |

This would search for $cur_ in the wrong place because * only matches 
one path component, right? Changing to ** might help, but then it would 
match in places more it shouldn't.

sed $sed_cmd | sort -u

should work and means we're only forking three extra processes no matter 
how many remotes the user has. I'm not sure if it changes the output 
order when GIT_COMPLETION_IGNORE_CASE is set though.

Best Wishes

Phillip