Re: [PATCH v4] vcs_info: choose backend by basedir

[Aleksandr Mezin <mezin.alexander@xxxxxxxxx>]

On Mon, Mar 29, 2021 at 10:56 PM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
>
> Let me first summarize the previous threads:
>
> - The current default is that backends are used in order of the «enable»
>   style: first detected, wins.
>
> - Frank prefers this remain the default.  (workers/47488, workers/47509)
>
> - Aleksandr implemented a mode in which the backend whose worktree root
>   directory is closest to $PWD would be used.
>
> - Bart's use-case (workers/44931) wouldn't be served by choosing the
>   "deepest" worktree but by choosing whichever one has local mods.
>   (workers/47490)
>
> - It was proposed to address Bart's use-case by calling vcs_info twice
>   with different user-context strings and corresponding zstyle settings.
>
> - Oliver proposed some independent improvements in workers/47492.
>
> If any of that is inaccurate, speak up.  The named people are Bcc'd.
>
> The patch being replied to contained some git- and cvs-specific changes
> which have been committed.  Aleksandr expressed concerns about those in
> workers/44929, but as far as I can see, the part that has been committed
> is a no-op, other than the fact that vcs_comm[basedir] will be populated
> earlier.
>
> So, first things first: Does implementing a "choose the VCS whose root
> is deepest" mode that's off by default sound like a reasonable way
> forward?  Any concerns with that?  Any concerns with the algorithm
> described in Aleksandr's log message (just the log message,
> independently of the unidiff)?
>
> Below a couple of review points on my account.  Not a full review; just
> what I spotted from reading the diff.
>
> Aleksandr Mezin wrote on Sun, Nov 15, 2020 at 01:37:03 +0600:
> > Previously, vcs_info iterated over enabled VCS backends and output repository
> > status from the first backend that works (i.e. first backend that can find a
> > repository).
> >
> > But I prefer a different behavior: I want to see the status of the repository
> > closest to the current working directory. For example: if there is a Mercurial
> > repository inside a Git repository, and I 'cd' into the Mercurial repository,
> > I want to see the status of Mercurial repo, even if 'git' comes before 'hg' in
> > the 'enable' list.
> >
> > This patch adds a new algorithm for vcs_info to choose the backend:
> >
> > 1. Among backends whose basedir isn't an ancestor of the current directory,
> > old algorithm is used: whichever comes first in 'enable' list.
> >
> > 2. If all backends return basedirs that are ancestors of the current directory,
> > the one with basedir closest to the current directory is chosen.
> >
> > As far as I know, unless the user has set basedir manually (GIT_WORK_TREE),
> > all backend return basedirs that are ancestors of the current directory.
> > Backends that return non-ancestor basedirs have higher priority to show the
> > status of that manually-configured repository.
> >
>
> Do any other backends allow the worktree to be somewhere other than an
> ancestor of $PWD?  Git can, as mentioned above.  Subversion can't.
> (svn(1) always operates on the path given by the positional arguments,
> if any, else on $PWD.)  I'm unsure about others.
>
> There doesn't seem to be any way to shadow a GIT_WORK_TREE setting,
> other than to temporarily set the enable/disable styles so as to exclude
> git entirely.  Is that a problem?

Can it be addressed later in a separate patch (if necessary at all)?

>
> Should we implement, say, a hook that gets arguments of the form
> «${vcs}\0${worktree_root}» and sets $reply to the one to use?  We could
> pass the arguments sorted by disk path and by order of the 'enable'
> style.  I suspect this would be overengineering.
>
> > The algorithm used by vcs_info can be configured using 'choose-closest-backend'
> > zstyle:
> >
> >     zstyle ':vcs_info:*' choose-closest-backend yes
> >
> > By default, old algorithm is used.
> >
> > It can also be configured per-backend:
> >
> >     zstyle ':vcs_info:hg:*' choose-closest-backend no
> >
> > This effectively moves the backend into group (1), as if its basedir is never
> > an ancestor of the current directory. Not sure how useful this is though.
> >
> > Signed-off-by: Aleksandr Mezin <mezin.alexander@xxxxxxxxx>
> > ---
> > v4: back to boolean zstyle, keep old behavior by default, documentation
> >
> >  Completion/Zsh/Command/_zstyle                |  1 +
> >  Doc/Zsh/contrib.yo                            | 41 +++++++++++++++++++
> >  Functions/VCS_Info/vcs_info                   | 30 ++++++++++++--
> >
> > +++ b/Functions/VCS_Info/vcs_info
> > @@ -124,10 +126,30 @@ vcs_info () {
> >          fi
> >          vcs_comm=()
> >          VCS_INFO_get_cmd
> > -        VCS_INFO_detect_${vcs} && (( found = 1 )) && break
> > +        if VCS_INFO_detect_${vcs}; then
> > +            basedir_realpath="${vcs_comm[basedir]:P}"
>
> If vcs_comm[basedir] starts to be used like that, it should be
> documented as an internal API.  (In comments, not yodl)

I don't get what exactly needs to be documented (and where). vcs_comm
doesn't seem to be documented anywhere (except the comment "vcs_comm
is used internally for passing values among VCS_INFO_* functions. It
is not part of the public API.").

>
> > +            zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" choose-closest-backend
> > +            choose_first_available=$?
> > +
> > +            [[ "${PWD:P}/" == "${basedir_realpath}"/* ]] || choose_first_available=1
>
> If I'm not mistaken, this condition will false negative when
> [[ $basedir_realpath == / ]] and [[ $PWD != / ]].

Yes. Should probably be `[[ "${PWD:P}/" == "${basedir_realpath}"/* ]]
|| [[ "${basedir_realpath}" == "/" ]] || choose_first_available=1`.

Should I send v5 now?


>
> > +            if (( choose_first_available )) || (( ${#basedir_realpath} > ${#chosen_basedir_realpath} )) ; then
> > +                chosen_vcs="${vcs}"
> > +                chosen_vcs_comm=("${(kv)vcs_comm[@]}")
> > +                chosen_backend_misc=("${(kv)backend_misc[@]}")
> > +                chosen_basedir_realpath="${basedir_realpath}"
> > +
> > +                (( choose_first_available )) && break
> > +            fi
> > +        fi
> >      done
> >
> > -    (( found == 0 )) && {
> > +    vcs="${chosen_vcs}"
> > +    vcs_comm=("${(kv)chosen_vcs_comm[@]}")
> > +    backend_misc=("${(kv)chosen_backend_misc[@]}")