zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH v5] vcs_info: choose backend by basedir
@ 2021-06-04  6:14 Aleksandr Mezin
  2021-06-20 18:52 ` Lawrence Velázquez
                   ` (3 more replies)
  0 siblings, 4 replies; 8+ messages in thread
From: Aleksandr Mezin @ 2021-06-04  6:14 UTC (permalink / raw)
  To: zsh-workers; +Cc: Aleksandr Mezin

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.

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
v5: described expected vcs_info[basedir] use in a comment, addressed Daniel's
questions in comments.

 Completion/Zsh/Command/_zstyle |  1 +
 Doc/Zsh/contrib.yo             | 41 ++++++++++++++++++++++
 Functions/VCS_Info/vcs_info    | 62 +++++++++++++++++++++++++++++++---
 3 files changed, 100 insertions(+), 4 deletions(-)

diff --git a/Completion/Zsh/Command/_zstyle b/Completion/Zsh/Command/_zstyle
index 9d06076e4..b7a73c5a2 100644
--- a/Completion/Zsh/Command/_zstyle
+++ b/Completion/Zsh/Command/_zstyle
@@ -193,6 +193,7 @@ styles=(
   quilt-standalone       v:bool
   quilt-patch-dir        v:_directories
   quiltcommand           v:_command_names
+  choose-closest-backend v:bool
 
   chpwd			 z:bool
   progress		 z:progress
diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
index a972f08d6..67823716a 100644
--- a/Doc/Zsh/contrib.yo
+++ b/Doc/Zsh/contrib.yo
@@ -1238,6 +1238,46 @@ of unapplied patches (for example with Mercurial Queue patches).
 
 Used by the tt(quilt), tt(hg), and tt(git) backends.
 )
+kindex(choose-closest-backend)
+item(tt(choose-closest-backend))(
+By default, tt(vcs_info) tries backends in the order given by the tt(enable)
+style, and operates on the repository detected by the first backend to
+successfully detect a repository.
+
+If this style (tt(choose-closest-backend)) is set to tt(true) for some (or all)
+backends, all backends will be divided into two groups:
+
+1) Backends, for which this style is set to tt(true), that detect a
+repository/working copy that is an ancestor of the current directory (including
+the current directory itself).
+
+2) All other backends: backends with tt(choose-closest-backend) unset/set to
+tt(false); and backends that detected a repository/working copy that's not an
+ancestor of the current directory, even if tt(choose-closest-backend) is set to
+tt(true) for them.
+
+First, tt(vcs_info) will try backends from the 2nd group in the order given by
+the tt(enable) style, and operate on the repository detected by the first
+backend to successfully detect a repository (the default behavior of
+tt(vcs_info), only limited to backends from the 2nd group).
+
+If no backends from the 2nd group detected a repository (or if there are no
+backends in that group), tt(vcs_info) will try backends from the 1st group.
+tt(vcs_info) will then operate on the repository closest to the current working
+directory.
+
+Usually, all VCS backends search for repositories only in ancestors of the
+current directory, and choose the repository closest to the current directory.
+So if tt(choose-closest-backend) is set to tt(true) globally for all backends,
+tt(vcs_info) will effectively search for the repository closest to the current
+directory.
+
+However, there is at least one backend that can detect a repository in an
+arbitrary location - Git backend. tt(GIT_WORK_TREE) can be set to a path that
+doesn't point to an ancestor of the current directory. In this case, even if
+tt(choose-closest-backend) is set to tt(true) globally for all backends,
+the repository/working copy pointed by tt(GIT_WORK_TREE) will be preferred.
+)
 enditem()
 
 The default values for these styles in all contexts are:
@@ -1272,6 +1312,7 @@ sitem(tt(quiltcommand))(quilt)
 sitem(tt(patch-format))(var(backend dependent))
 sitem(tt(nopatch-format))(var(backend dependent))
 sitem(tt(get-unapplied))(false)
+sitem(tt(choose-closest-backend))(false)
 endsitem()
 
 In normal tt(formats) and tt(actionformats) the following replacements are
diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index 786b61918..85f7ca0ab 100644
--- a/Functions/VCS_Info/vcs_info
+++ b/Functions/VCS_Info/vcs_info
@@ -54,7 +54,7 @@ vcs_info () {
     [[ -r . ]] || return 0
 
     local pat
-    local -i found retval
+    local -i retval
     local -a enabled disabled dps
     local usercontext vcs rrn quiltmode
     local -x LC_MESSAGES
@@ -64,6 +64,9 @@ vcs_info () {
     # vcs_comm is used internally for passing values among VCS_INFO_* functions.
     # It is not part of the public API.
     #
+    # VCS_INFO_detect_* functions should set vcs_comm[basedir] to the path to
+    # the root of the working copy.
+    #
     # hook_com, backend_misc, and user_data are public API; see zshcontrib(1)
     # and Misc/vcs_info-examples.
     local -A vcs_comm hook_com backend_misc user_data
@@ -114,7 +117,9 @@ vcs_info () {
 
     VCS_INFO_maxexports
 
-    (( found = 0 ))
+    local -A chosen_vcs_comm chosen_backend_misc
+    local chosen_vcs basedir_realpath chosen_basedir_realpath choose_first_available
+
     for vcs in ${enabled} ; do
         [[ -n ${(M)disabled:#${vcs}} ]] && continue
         if (( ${+functions[VCS_INFO_detect_${vcs}]} == 0 )) ; then
@@ -124,10 +129,59 @@ vcs_info () {
         fi
         vcs_comm=()
         VCS_INFO_get_cmd
-        VCS_INFO_detect_${vcs} && (( found = 1 )) && break
+        if VCS_INFO_detect_${vcs}; then
+            # Most backends already set vcs_comm[basedir] to an absolute path
+            # with symlinks resolved:
+            #
+            # * Backends using VCS_INFO_bydir_detect:
+            #   bzr, cdv, darcs, fossil, hg, mtn, p4, svn
+            #
+            # * cvs - custom logic, but uses :P too
+            #
+            # * git - $(git rev-parse --show-toplevel) resolves symlinks and
+            #   returns an absolute path
+            #
+            # * for tla (GNU Arch) I can't find any online documentation or
+            #   packages (seems to be dead)
+            #
+            # * svk doesn't use :P modifier and returns the path as is from
+            #   the config file (as far as I understand)
+            #
+            # So I'm not sure whether to modify the backends or just resolve
+            # the path here to be sure. VCS_INFO_get_data_* usually reads
+            # vcs_comm[basedir] too. In particular, I'm not sure if it's safe to
+            # resolve symlinks in vcs_comm[basedir] for svk.
+            #
+            # Because most backends already do :P substitution, I expect all
+            # the info needed to resolve the path to be cached in memory by the
+            # OS (so it shouldn't cause performance issues).
+            basedir_realpath="${vcs_comm[basedir]:P}"
+
+            zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" choose-closest-backend
+            choose_first_available=$?
+
+            [[ "${PWD:P}/" == "${basedir_realpath}"/* ]] || [[ "${basedir_realpath}" == "/" ]] || choose_first_available=1
+
+            # If choose_first_available is 0 at this point, all basedir_realpaths
+            # (and thus chosen_basedir_realpath) are parents (and thus prefixes)
+            # of PWD:P. So comparing their lengths is enough to figure out which
+            # one is closer to 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[@]}")
+
+    [ -z "${vcs}" ] && {
         vcs='-quilt-'; quiltmode='standalone'
         VCS_INFO_quilt standalone || VCS_INFO_set --nvcs
         return 0
-- 
2.31.1



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] vcs_info: choose backend by basedir
  2021-06-04  6:14 [PATCH v5] vcs_info: choose backend by basedir Aleksandr Mezin
@ 2021-06-20 18:52 ` Lawrence Velázquez
  2021-07-18 18:42   ` Lawrence Velázquez
  2021-11-23 16:03 ` Aleksandr Mezin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 8+ messages in thread
From: Lawrence Velázquez @ 2021-06-20 18:52 UTC (permalink / raw)
  To: zsh-workers; +Cc: Aleksandr Mezin

On Fri, Jun 4, 2021, at 2:14 AM, Aleksandr Mezin wrote:
> This patch adds a new algorithm for vcs_info to choose the backend:
>
> [...]
>
> ---
> v4: back to boolean zstyle, keep old behavior by default, documentation
> v5: described expected vcs_info[basedir] use in a comment, addressed Daniel's
> questions in comments.

ping for review

-- 
vq


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] vcs_info: choose backend by basedir
  2021-06-20 18:52 ` Lawrence Velázquez
@ 2021-07-18 18:42   ` Lawrence Velázquez
  2021-07-28 18:00     ` Bart Schaefer
  0 siblings, 1 reply; 8+ messages in thread
From: Lawrence Velázquez @ 2021-07-18 18:42 UTC (permalink / raw)
  To: zsh-workers; +Cc: Aleksandr Mezin

On Sun, Jun 20, 2021, at 2:52 PM, Lawrence Velázquez wrote:
> On Fri, Jun 4, 2021, at 2:14 AM, Aleksandr Mezin wrote:
> > This patch adds a new algorithm for vcs_info to choose the backend:
> >
> > [...]
> >
> > ---
> > v4: back to boolean zstyle, keep old behavior by default, documentation
> > v5: described expected vcs_info[basedir] use in a comment, addressed Daniel's
> > questions in comments.
> 
> ping for review

second ping

-- 
vq


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] vcs_info: choose backend by basedir
  2021-07-18 18:42   ` Lawrence Velázquez
@ 2021-07-28 18:00     ` Bart Schaefer
  0 siblings, 0 replies; 8+ messages in thread
From: Bart Schaefer @ 2021-07-28 18:00 UTC (permalink / raw)
  To: Lawrence Velázquez; +Cc: Zsh hackers list, Aleksandr Mezin

On Sun, Jul 18, 2021 at 11:43 AM Lawrence Velázquez <larryv@zsh.org> wrote:
>
> second ping

I don't have any remaining complaint/comment about the behavior, but
as I'm pretty far from familiar with the VCS_info code, I'm not going
to undertake to apply it.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] vcs_info: choose backend by basedir
  2021-06-04  6:14 [PATCH v5] vcs_info: choose backend by basedir Aleksandr Mezin
  2021-06-20 18:52 ` Lawrence Velázquez
@ 2021-11-23 16:03 ` Aleksandr Mezin
  2021-11-26  8:08 ` Daniel Shahaf
  2021-11-26  8:09 ` RFC: Remove vcs_info backends cdv, svk, tla? (was: Re: [PATCH v5] vcs_info: choose backend by basedir) Daniel Shahaf
  3 siblings, 0 replies; 8+ messages in thread
From: Aleksandr Mezin @ 2021-11-23 16:03 UTC (permalink / raw)
  To: zsh-workers, Daniel Shahaf

Ping

On Fri, Jun 4, 2021 at 12:14 PM Aleksandr Mezin
<mezin.alexander@gmail.com> wrote:
>
> 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.
>
> 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
> v5: described expected vcs_info[basedir] use in a comment, addressed Daniel's
> questions in comments.
>
>  Completion/Zsh/Command/_zstyle |  1 +
>  Doc/Zsh/contrib.yo             | 41 ++++++++++++++++++++++
>  Functions/VCS_Info/vcs_info    | 62 +++++++++++++++++++++++++++++++---
>  3 files changed, 100 insertions(+), 4 deletions(-)
>
> diff --git a/Completion/Zsh/Command/_zstyle b/Completion/Zsh/Command/_zstyle
> index 9d06076e4..b7a73c5a2 100644
> --- a/Completion/Zsh/Command/_zstyle
> +++ b/Completion/Zsh/Command/_zstyle
> @@ -193,6 +193,7 @@ styles=(
>    quilt-standalone       v:bool
>    quilt-patch-dir        v:_directories
>    quiltcommand           v:_command_names
> +  choose-closest-backend v:bool
>
>    chpwd                         z:bool
>    progress              z:progress
> diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo
> index a972f08d6..67823716a 100644
> --- a/Doc/Zsh/contrib.yo
> +++ b/Doc/Zsh/contrib.yo
> @@ -1238,6 +1238,46 @@ of unapplied patches (for example with Mercurial Queue patches).
>
>  Used by the tt(quilt), tt(hg), and tt(git) backends.
>  )
> +kindex(choose-closest-backend)
> +item(tt(choose-closest-backend))(
> +By default, tt(vcs_info) tries backends in the order given by the tt(enable)
> +style, and operates on the repository detected by the first backend to
> +successfully detect a repository.
> +
> +If this style (tt(choose-closest-backend)) is set to tt(true) for some (or all)
> +backends, all backends will be divided into two groups:
> +
> +1) Backends, for which this style is set to tt(true), that detect a
> +repository/working copy that is an ancestor of the current directory (including
> +the current directory itself).
> +
> +2) All other backends: backends with tt(choose-closest-backend) unset/set to
> +tt(false); and backends that detected a repository/working copy that's not an
> +ancestor of the current directory, even if tt(choose-closest-backend) is set to
> +tt(true) for them.
> +
> +First, tt(vcs_info) will try backends from the 2nd group in the order given by
> +the tt(enable) style, and operate on the repository detected by the first
> +backend to successfully detect a repository (the default behavior of
> +tt(vcs_info), only limited to backends from the 2nd group).
> +
> +If no backends from the 2nd group detected a repository (or if there are no
> +backends in that group), tt(vcs_info) will try backends from the 1st group.
> +tt(vcs_info) will then operate on the repository closest to the current working
> +directory.
> +
> +Usually, all VCS backends search for repositories only in ancestors of the
> +current directory, and choose the repository closest to the current directory.
> +So if tt(choose-closest-backend) is set to tt(true) globally for all backends,
> +tt(vcs_info) will effectively search for the repository closest to the current
> +directory.
> +
> +However, there is at least one backend that can detect a repository in an
> +arbitrary location - Git backend. tt(GIT_WORK_TREE) can be set to a path that
> +doesn't point to an ancestor of the current directory. In this case, even if
> +tt(choose-closest-backend) is set to tt(true) globally for all backends,
> +the repository/working copy pointed by tt(GIT_WORK_TREE) will be preferred.
> +)
>  enditem()
>
>  The default values for these styles in all contexts are:
> @@ -1272,6 +1312,7 @@ sitem(tt(quiltcommand))(quilt)
>  sitem(tt(patch-format))(var(backend dependent))
>  sitem(tt(nopatch-format))(var(backend dependent))
>  sitem(tt(get-unapplied))(false)
> +sitem(tt(choose-closest-backend))(false)
>  endsitem()
>
>  In normal tt(formats) and tt(actionformats) the following replacements are
> diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
> index 786b61918..85f7ca0ab 100644
> --- a/Functions/VCS_Info/vcs_info
> +++ b/Functions/VCS_Info/vcs_info
> @@ -54,7 +54,7 @@ vcs_info () {
>      [[ -r . ]] || return 0
>
>      local pat
> -    local -i found retval
> +    local -i retval
>      local -a enabled disabled dps
>      local usercontext vcs rrn quiltmode
>      local -x LC_MESSAGES
> @@ -64,6 +64,9 @@ vcs_info () {
>      # vcs_comm is used internally for passing values among VCS_INFO_* functions.
>      # It is not part of the public API.
>      #
> +    # VCS_INFO_detect_* functions should set vcs_comm[basedir] to the path to
> +    # the root of the working copy.
> +    #
>      # hook_com, backend_misc, and user_data are public API; see zshcontrib(1)
>      # and Misc/vcs_info-examples.
>      local -A vcs_comm hook_com backend_misc user_data
> @@ -114,7 +117,9 @@ vcs_info () {
>
>      VCS_INFO_maxexports
>
> -    (( found = 0 ))
> +    local -A chosen_vcs_comm chosen_backend_misc
> +    local chosen_vcs basedir_realpath chosen_basedir_realpath choose_first_available
> +
>      for vcs in ${enabled} ; do
>          [[ -n ${(M)disabled:#${vcs}} ]] && continue
>          if (( ${+functions[VCS_INFO_detect_${vcs}]} == 0 )) ; then
> @@ -124,10 +129,59 @@ vcs_info () {
>          fi
>          vcs_comm=()
>          VCS_INFO_get_cmd
> -        VCS_INFO_detect_${vcs} && (( found = 1 )) && break
> +        if VCS_INFO_detect_${vcs}; then
> +            # Most backends already set vcs_comm[basedir] to an absolute path
> +            # with symlinks resolved:
> +            #
> +            # * Backends using VCS_INFO_bydir_detect:
> +            #   bzr, cdv, darcs, fossil, hg, mtn, p4, svn
> +            #
> +            # * cvs - custom logic, but uses :P too
> +            #
> +            # * git - $(git rev-parse --show-toplevel) resolves symlinks and
> +            #   returns an absolute path
> +            #
> +            # * for tla (GNU Arch) I can't find any online documentation or
> +            #   packages (seems to be dead)
> +            #
> +            # * svk doesn't use :P modifier and returns the path as is from
> +            #   the config file (as far as I understand)
> +            #
> +            # So I'm not sure whether to modify the backends or just resolve
> +            # the path here to be sure. VCS_INFO_get_data_* usually reads
> +            # vcs_comm[basedir] too. In particular, I'm not sure if it's safe to
> +            # resolve symlinks in vcs_comm[basedir] for svk.
> +            #
> +            # Because most backends already do :P substitution, I expect all
> +            # the info needed to resolve the path to be cached in memory by the
> +            # OS (so it shouldn't cause performance issues).
> +            basedir_realpath="${vcs_comm[basedir]:P}"
> +
> +            zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" choose-closest-backend
> +            choose_first_available=$?
> +
> +            [[ "${PWD:P}/" == "${basedir_realpath}"/* ]] || [[ "${basedir_realpath}" == "/" ]] || choose_first_available=1
> +
> +            # If choose_first_available is 0 at this point, all basedir_realpaths
> +            # (and thus chosen_basedir_realpath) are parents (and thus prefixes)
> +            # of PWD:P. So comparing their lengths is enough to figure out which
> +            # one is closer to 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[@]}")
> +
> +    [ -z "${vcs}" ] && {
>          vcs='-quilt-'; quiltmode='standalone'
>          VCS_INFO_quilt standalone || VCS_INFO_set --nvcs
>          return 0
> --
> 2.31.1
>


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v5] vcs_info: choose backend by basedir
  2021-06-04  6:14 [PATCH v5] vcs_info: choose backend by basedir Aleksandr Mezin
  2021-06-20 18:52 ` Lawrence Velázquez
  2021-11-23 16:03 ` Aleksandr Mezin
@ 2021-11-26  8:08 ` Daniel Shahaf
  2021-11-26  8:09 ` RFC: Remove vcs_info backends cdv, svk, tla? (was: Re: [PATCH v5] vcs_info: choose backend by basedir) Daniel Shahaf
  3 siblings, 0 replies; 8+ messages in thread
From: Daniel Shahaf @ 2021-11-26  8:08 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Sorry for how long this is taking.  I've not had much time recently.
(I haven't even had time to commit all my own patches.)

I think the summary as of right now (before this post) is still
workers/48329.  (I did another review after that, but answers to it have
been incorporated into v5 here.)

-workers@: given that several people have mentioned they weren't
familiar with vcs_info enough to review/apply this, I'd like to
particularly solicit reviews of the new functionality, as described in
the log message and docs patch.  I'm not looking for review of the
implementation or for copyediting of the docs, but for reviews of the
functionality described in the docs.

Before I get to the details, I'd like to repeat that I'm aware of how
old this patch is — it's two years old this week — and I'll adjust my
review granularity accordingly, as far as reasonable without cutting
corners on my committability standards.

Aleksandr Mezin wrote on Fri, Jun 04, 2021 at 12:14:21 +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.
> 

We seem to have consensus to add such a functionality, established in
workers/48329 (grep for "First things first").

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

Okay, so the order is actually:

1. Backends that returned a non-ancestor directory
2. First detected backend out of those that don't have choose-closest-backend set
3. Closest ancestor backend out of those that do have choose-closest-backend set

Now, I'm sure that achieves your original design goal (from 44918) of
preferring hg to git inside ~/git-repo/hg-repo, but overall it seems
rather arbitrary.  What's the rationale for this particular order?  How
does one remember what has precedence over what?  (E.g., command-line
options generally have precedence over environment variables because
command-line options are assumed to have been set "closer" to running
the command.)

My question from 48329 stands:
.
	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?
.
To be clear, the question is why $GIT_WORK_TREE should have precedence
over VCS_INFO_detect_fossil() regardless of style settings.

I think we could simplify this design by dropping the ability to set the
style on a per-backend basis.  Let there be two modes: the current one
(of detected backends, choose whichever comes first in the 'enable'
style's value) and the new one (of detected backends, choose the one
whose worktree root is deeper), with a global knob to choose between
them.  That should suffice.

Furthermore, why doesn't the first mode suffice on its own?  That is:
for your ~/git-repo/hg-repo situation, why is it not sufficient to
ensure that hg precedes git in the value of the 'enable' style?
I couldn't find the answer in the past threads.

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

Well, I suppose one might set:
.
      zstyle ':vcs_info:*' choose-closest-backend yes
      zstyle ':vcs_info:git:*' choose-closest-backend no
.
The effect of the second line would be to give git precedence when it's
the outer VCS.  I don't immediately see a use-case for this.  Perhaps if
someone has several sibling worktrees, and they then create an outer
worktree encompassing all inner worktrees so as to make/revert changes in
lockstep across all trees?  That's relevant if the outer worktree isn't
of the same VCS as the inner ones.

> Signed-off-by: Aleksandr Mezin <mezin.alexander@gmail.com>
> ---
> v4: back to boolean zstyle, keep old behavior by default, documentation
> v5: described expected vcs_info[basedir] use in a comment, addressed Daniel's
> questions in comments.

Hope this helps.  I'm sure it feels like we're going backwards, or in
circles, but one always feels a jolt backwards when a car starts moving ☺

Cheers,

Daniel


^ permalink raw reply	[flat|nested] 8+ messages in thread

* RFC: Remove vcs_info backends cdv, svk, tla?  (was: Re: [PATCH v5] vcs_info: choose backend by basedir)
  2021-06-04  6:14 [PATCH v5] vcs_info: choose backend by basedir Aleksandr Mezin
                   ` (2 preceding siblings ...)
  2021-11-26  8:08 ` Daniel Shahaf
@ 2021-11-26  8:09 ` Daniel Shahaf
  2021-11-26  9:43   ` Oliver Kiddle
  3 siblings, 1 reply; 8+ messages in thread
From: Daniel Shahaf @ 2021-11-26  8:09 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Aleksandr Mezin wrote on Fri, Jun 04, 2021 at 12:14:21 +0600:
> @@ -124,10 +129,59 @@ vcs_info () {
>          vcs_comm=()
>          VCS_INFO_get_cmd
> -        VCS_INFO_detect_${vcs} && (( found = 1 )) && break
> +        if VCS_INFO_detect_${vcs}; then
> +            # Most backends already set vcs_comm[basedir] to an absolute path
> +            # with symlinks resolved:
> +            #
> +            # * Backends using VCS_INFO_bydir_detect:
> +            #   bzr, cdv, darcs, fossil, hg, mtn, p4, svn
> +            #
> +            # * cvs - custom logic, but uses :P too
> +            #
> +            # * git - $(git rev-parse --show-toplevel) resolves symlinks and
> +            #   returns an absolute path
> +            #
> +            # * for tla (GNU Arch) I can't find any online documentation or
> +            #   packages (seems to be dead)
> +            #

There's https://savannah.gnu.org/projects/gnu-arch/.  It says tla is
"decommissioned" and hasn't had a release in 16 years.  There's also
<https://www.gnu.org/software/gnu-arch/>, but it doesn't alter the
bottom line.

> +            # * svk doesn't use :P modifier and returns the path as is from
> +            #   the config file (as far as I understand)
> +            #

Does anyone still use svk?  It was a "distributed version control for
Subversion" thing (https://metacpan.org/dist/SVK), but:

- It hasn't had a release since 2010

- Its homepage redirects elsewhere

- Its repository is 404 Not Found

- I haven't seen any mention of it on svn's lists in years

- The list of features at the top of its man page seems to be mostly
  features that git-svn and hgsubversion already take care of.  "Working
  copy without .svn" _might_ be an exception, if they mean no text
  bases; but they could just mean "there's a single .${vcs} dir at the
  top directory, rather than foo/bar/.${vcs} inside every versioned
  foo/bar/ directory", in which case, that's not a selling point either
  nowadays.

>

Looking at the other backends, cdv hasn't had a release in 16 years,
either.  Every other backend seems to be active to one degree or
another.

> +            # So I'm not sure whether to modify the backends or just resolve
> +            # the path here to be sure. VCS_INFO_get_data_* usually reads
> +            # vcs_comm[basedir] too. In particular, I'm not sure if it's safe to
> +            # resolve symlinks in vcs_comm[basedir] for svk.

The cdv, svk, tla backends presumably have no remaining users; having
them at all imposes extra costs on people making «vcs_info»-wide changes;
and if we remove them, we can always recover them from history if they
turn out to have users after all.

So, shall we remove these three backends?

Cheers,

Daniel


> +            # Because most backends already do :P substitution, I expect all
> +            # the info needed to resolve the path to be cached in memory by the
> +            # OS (so it shouldn't cause performance issues).
> +            basedir_realpath="${vcs_comm[basedir]:P}"


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: RFC: Remove vcs_info backends cdv, svk, tla? (was: Re: [PATCH v5] vcs_info: choose backend by basedir)
  2021-11-26  8:09 ` RFC: Remove vcs_info backends cdv, svk, tla? (was: Re: [PATCH v5] vcs_info: choose backend by basedir) Daniel Shahaf
@ 2021-11-26  9:43   ` Oliver Kiddle
  0 siblings, 0 replies; 8+ messages in thread
From: Oliver Kiddle @ 2021-11-26  9:43 UTC (permalink / raw)
  To: Aleksandr Mezin, zsh-workers

Daniel Shahaf wrote:
> Aleksandr Mezin wrote on Fri, Jun 04, 2021 at 12:14:21 +0600:
> > +            # * Backends using VCS_INFO_bydir_detect:
> > +            #   bzr, cdv, darcs, fossil, hg, mtn, p4, svn

I don't think bazaar and monotone are exactly still in wide use either.
Bazaar appears to have had a fork (breezy) for Python 3.

> There's https://savannah.gnu.org/projects/gnu-arch/.  It says tla is
> "decommissioned" and hasn't had a release in 16 years.  There's also

It is still packaged in a few distributions but otherwise all signs are
that it is fairly long dead.

> Does anyone still use svk?  It was a "distributed version control for
> Subversion" thing (https://metacpan.org/dist/SVK), but:

I attempted to use it at the time and found it to be more trouble than
it was worth. git-svn was a much better replacement. I really can't
imagine anyone has stuck with it ever since.

> The cdv, svk, tla backends presumably have no remaining users; having
> them at all imposes extra costs on people making «vcs_info»-wide changes;
> and if we remove them, we can always recover them from history if they
> turn out to have users after all.

I agree.

> So, shall we remove these three backends?

In my view, yes.

Oliver


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-11-26  9:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-04  6:14 [PATCH v5] vcs_info: choose backend by basedir Aleksandr Mezin
2021-06-20 18:52 ` Lawrence Velázquez
2021-07-18 18:42   ` Lawrence Velázquez
2021-07-28 18:00     ` Bart Schaefer
2021-11-23 16:03 ` Aleksandr Mezin
2021-11-26  8:08 ` Daniel Shahaf
2021-11-26  8:09 ` RFC: Remove vcs_info backends cdv, svk, tla? (was: Re: [PATCH v5] vcs_info: choose backend by basedir) Daniel Shahaf
2021-11-26  9:43   ` Oliver Kiddle

Code repositories for project(s) associated with this 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).