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

Hi David

On 02/03/2025 20:34, David Mandelberg wrote:

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

quoted

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

quoted

From: David Mandelberg <redacted>

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'm not really sure what a "large" number looks like for people who 
track a lot of repositories. On windows forking is pretty slow (our 
shell-based test suite is incredibly slow on that platform) so I think 
it makes sense to try and avoid adding new processes.

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.

Yes, if there were a 100 remotes that's a bit of a fork-bomb.

quoted

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.

Thanks, I'm not familiar with bash's extensions to parameter 
substitution. The completions can also but used under zsh 
(git-completion.zsh is pretty much abandoned I think) but it looks like 
bash and zsh agree on this expansion.

quoted

     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?

I should have said in the comment above that ref names cannot contain 
space, tab, newline etc. so we don't have to worry about the shell's 
word splitting splitting the patterns. They cannot contain backslashes 
either.

quoted

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

Oh, good point I'd missed that. Could we change sed to run with '-n' and 
generate an expression that looks like

     -e s|^refs/remotes/$remote/\($cur_\)|\1|p -e t

with the appropriate escaping of $remote and $cur_ so that we only print 
the output of for-each-ref that matches $cur_?

Best Wishes

Phillip

quoted

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