zsh-workers
 help / color / mirror / code / Atom feed
From: Daniel Shahaf <d.s@daniel.shahaf.name>
To: Aleksandr Mezin <mezin.alexander@gmail.com>
Cc: zsh-workers@zsh.org
Subject: Re: [PATCH v4] vcs_info: choose backend by basedir
Date: Mon, 29 Mar 2021 16:56:29 +0000	[thread overview]
Message-ID: <20210329165629.GA2761@tarpaulin.shahaf.local2> (raw)
In-Reply-To: <20201114193703.1428617-1-mezin.alexander@gmail.com>

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@gmail.com>
> ---
> 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[@]}")


  parent reply	other threads:[~2021-03-29 16:56 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-14 19:37 Aleksandr Mezin
2020-12-03  4:01 ` Aleksandr Mezin
2020-12-04 15:56   ` Daniel Shahaf
2021-03-27 20:40     ` Lawrence Velázquez
2021-03-29 16:04 ` Daniel Shahaf
2021-03-29 16:56 ` Daniel Shahaf [this message]
2021-04-10 20:49   ` Lawrence Velázquez
2021-04-17 12:17   ` Aleksandr Mezin
2021-04-17 13:16     ` Daniel Shahaf
2021-05-09 20:28       ` Lawrence Velázquez
2021-05-31  1:13         ` Lawrence Velázquez

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210329165629.GA2761@tarpaulin.shahaf.local2 \
    --to=d.s@daniel.shahaf.name \
    --cc=mezin.alexander@gmail.com \
    --cc=zsh-workers@zsh.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).