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

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Aleksandr Mezin wrote on Sat, Apr 17, 2021 at 18:17:20 +0600:
> 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)?
> 

I'm trying to ensure that the patch doesn't conflict with things we may
want to implement in the longer term.  To make this assessment, I am
trying to get an idea in broad strokes of what we might want to implement
in the longer term.  That's why I'm asking these things.  It's not
featuritis.

(If it were featuritis, I'd have mentioned again that idea about
implementing a hook that takes as arguments a list of backends detected,
and chooses the one to use.  And I'd have proposed to sort the arguments
by basedir descending and vcs style-value-wise, to make it easy to
implement the "choose nearest" semantics.  And I'd have mentioned the
idea of calling $vcs_info several times, each time with only one backend
enabled, and printing everything that results — a generalization of what
I proposed for Bart's use-case.  You needn't respond to this paragraph.)

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

The fact that the function vcs_info expects VCS_INFO_detect_${vcs} to
set vcs_comm[basedir] to a particular value.

> (and where).

Wherever the interface that a VCS_INFO_detect_${vcs} function should
implement is documented.

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

There's also comments at the top of VCS_INFO_bydir_detect that document
that function's use of specific $vcs_comm keys.

The fact that vcs_comm is not documented in the manual or in
Misc/vcs_info-examples is expected, because those document the public
API, which $vcs_comm isn't part of.

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

OK.

By the way, could all these :P's be a performance problem in some setups?
This is the detection code, not the "get data" code, and the performance
considerations are different.  Most VCS_INFO_detect_${foo} functions
just do stat()s upwards, for example (git is an exception).

> Should I send v5 now?

+1, since part of v4 has been committed.

> >
> > > +            if (( choose_first_available )) || (( ${#basedir_realpath} > ${#chosen_basedir_realpath} )) ; then

I may have mentioned this in a previous thread: Is the lengths
comparison the right thing to do?  It would be if it was known that
$basedir_realpath was under $chosen_basedir_realpath (i.e., in
$chosen_basedir_realpath/**/*), as opposed to being, say, /foo and
/foobar, or /lorem and /ipsum.

Cheers,

Daniel

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