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@xxxxxxxxxxxxxx>
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
