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

From: D. Ben Knoble <ben.knoble@gmail.com>
Date: Mar 6, 2025, 20:24 UTC
Message-ID: <CALnO6CBDjQgc2TNaXbBWiUNqWV=RgrL2Ry-7HzRu3Ozuqazzvw@mail.gmail.com>
In-Reply-To: <42dfc06e-8eb1-46da-9971-9d102f1390ad@mandelberg.org>

On Mon, Mar 3, 2025 at 2:25 PM David Mandelberg <david@mandelberg.org> wrote:

Show 24 quoted lines

>
> Op 2025-03-03 om 11:36 schreef phillip.wood123@gmail.com:
> >>> 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.

I don't think "abandoned" is the right characterization—at least, it's the completion script that Homebrew-installed Git users will get for Zsh, and it does re-use the Bash completion scripts (which therefore ought to stay portable between both if possible, otherwise we get stuff like 8776470cf3 (completion: repair config completion for Zsh, 2025-01-06)). The Zsh script has seen some fixups over time (such as my own 3c20acdf46 (completion: zsh: stop leaking local cache variable, 2024-04-30)).

>
> The tests don't automatically run under zsh though, right? Maybe I
> should try to figure out how to do that in a separate patch...

Correct—which is how bugs sneak in ;)

I'm willing to manually test the patch if I can understand how to reproduce the issue—it sounds like having a remote name with a slash is sufficient?

I started trying to test Zsh completion a while back when working on one of the patches mentioned above; I got as far as this hack [1], which is to say, not very far at all.

[1]: https://github.com/git/git/commit/d8918195f18a503aa1a42fed3c66a0af8d04131f

-- D. Ben Knoble