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

From: David Mandelberg <david@mandelberg.org>
Date: Mar 2, 2025, 20:34 UTC
Message-ID: <2323bb52-f43d-4f40-8955-4c648677a93e@mandelberg.org>
In-Reply-To: <c03192bc-68d3-4645-9bd3-93a338a7496f@gmail.com>

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

Show 24 quoted lines

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

Show 11 quoted lines

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

Show 9 quoted lines

> 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