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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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?

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)

> +            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 != / ]].

> +            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[@]}")