Re: [PATCH 3/3] completion: fix bugs with slashes in remote names
- From
- phillip.wood123@gmail.com <phillip.wood123@gmail.com>
- Date
- Mar 3, 2025, 16:36 UTC
- Message-ID
- <65d903c0-6b4b-4a55-b7e4-4a277417f0f1@gmail.com>
- In-Reply-To
- <2323bb52-f43d-4f40-8955-4c648677a93e@mandelberg.org>
Hi David
On 02/03/2025 20:34, David Mandelberg wrote:
Show 12 quoted lines
> Op 2025-03-02 om 09:17 schreef Phillip Wood: >> On 02/03/2025 07:45, David Mandelberg via GitGitGadget wrote: >>> From: David Mandelberg <david@mandelberg.org> >>> >> 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.
Show 17 quoted lines
> 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.
Show 17 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.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.
Show 6 quoted lines
>> 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.
Show 7 quoted lines
>> __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
Show 10 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 >