zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH v4] vcs_info: choose backend by basedir
@ 2020-11-14 19:37 Aleksandr Mezin
  2020-12-03  4:01 ` Aleksandr Mezin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Aleksandr Mezin @ 2020-11-14 19:37 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.

Also, this patch adjusts VCS_INFO_detect_git and VCS_INFO_detect_cvs to make
them set vcs_comm[basedir], by moving basedir detection code from corresponding
VCS_INFO_get_data_* functions. VCS_INFO_detect_git and VCS_INFO_detect_cvs were
the only VCS_INFO_detect_* functions not setting it.

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 +++++++++++++++++++
 .../VCS_Info/Backends/VCS_INFO_detect_cvs     | 17 +++++++-
 .../VCS_Info/Backends/VCS_INFO_detect_git     |  1 +
 .../VCS_Info/Backends/VCS_INFO_get_data_cvs   | 11 +----
 .../VCS_Info/Backends/VCS_INFO_get_data_git   |  2 +-
 Functions/VCS_Info/vcs_info                   | 30 ++++++++++++--
 7 files changed, 86 insertions(+), 17 deletions(-)

diff --git a/Completion/Zsh/Command/_zstyle b/Completion/Zsh/Command/_zstyle
index 75acde5f7..e094b3da8 100644
--- a/Completion/Zsh/Command/_zstyle
+++ b/Completion/Zsh/Command/_zstyle
@@ -192,6 +192,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 00f693664..c0b730850 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/Backends/VCS_INFO_detect_cvs b/Functions/VCS_Info/Backends/VCS_INFO_detect_cvs
index 7a5ee1eef..a57959ec9 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_detect_cvs
+++ b/Functions/VCS_Info/Backends/VCS_INFO_detect_cvs
@@ -7,5 +7,18 @@ setopt localoptions NO_shwordsplit
 [[ $1 == '--flavours' ]] && return 1
 
 VCS_INFO_check_com ${vcs_comm[cmd]} || return 1
-[[ -d "./CVS" ]] && [[ -r "./CVS/Repository" ]] && return 0
-return 1
+if ! [[ -d "./CVS" ]] || ! [[ -r "./CVS/Repository" ]] ; then
+    return 1
+fi
+
+# Look for the most distant parent that still has a CVS subdirectory.
+local cvsbase="."
+cvsbase=${cvsbase:P}
+while [[ -d "${cvsbase:h}/CVS" ]]; do
+    cvsbase="${cvsbase:h}"
+    if [[ $cvsbase == '/' ]]; then
+        break
+    fi
+done
+
+vcs_comm[basedir]="${cvsbase}"
diff --git a/Functions/VCS_Info/Backends/VCS_INFO_detect_git b/Functions/VCS_Info/Backends/VCS_INFO_detect_git
index e4191f474..b7955de38 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_detect_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_detect_git
@@ -7,6 +7,7 @@ setopt localoptions NO_shwordsplit
 [[ $1 == '--flavours' ]] && { print -l git-p4 git-svn; return 0 }
 
 if VCS_INFO_check_com ${vcs_comm[cmd]} && vcs_comm[gitdir]="$(${vcs_comm[cmd]} rev-parse --git-dir 2> /dev/null)" ; then
+    vcs_comm[basedir]="$( ${vcs_comm[cmd]} rev-parse --show-toplevel 2> /dev/null )"
     if   [[ -d ${vcs_comm[gitdir]}/svn ]]             ; then vcs_comm[overwrite_name]='git-svn'
     elif [[ -d ${vcs_comm[gitdir]}/refs/remotes/p4 ]] ; then vcs_comm[overwrite_name]='git-p4' ; fi
     return 0
diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_cvs b/Functions/VCS_Info/Backends/VCS_INFO_get_data_cvs
index 9b828bd11..bc0d5cfe5 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_cvs
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_cvs
@@ -5,17 +5,8 @@
 setopt localoptions NO_shwordsplit
 local cvsbranch cvsbase
 
-# Look for the most distant parent that still has a CVS subdirectory.
+cvsbase="${vcs_comm[basedir]}"
 # VCS_INFO_detect_cvs ensured that ./CVS/Repository exists.
-cvsbase="."
-cvsbase=${cvsbase:P}
-while [[ -d "${cvsbase:h}/CVS" ]]; do
-    cvsbase="${cvsbase:h}"
-    if [[ $cvsbase == '/' ]]; then
-        break
-    fi
-done
-
 cvsbranch=$(< ./CVS/Repository)
 rrn=${cvsbase:t}
 cvsbranch=${cvsbranch##${rrn}/}
diff --git a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
index 79429c8e0..eb04d4b41 100644
--- a/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
+++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
@@ -138,7 +138,7 @@ VCS_INFO_git_handle_patches () {
 
 gitdir=${vcs_comm[gitdir]}
 VCS_INFO_git_getbranch ${gitdir}
-gitbase=$( ${vcs_comm[cmd]} rev-parse --show-toplevel 2> /dev/null )
+gitbase=${vcs_comm[basedir]}
 if [[ -z ${gitbase} ]]; then
     # Bare repository
     gitbase=${gitdir:P}
diff --git a/Functions/VCS_Info/vcs_info b/Functions/VCS_Info/vcs_info
index 786b61918..0769bc877 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
@@ -114,7 +114,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 +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}"
+
+            zstyle -t ":vcs_info:${vcs}:${usercontext}:${rrn}" choose-closest-backend
+            choose_first_available=$?
+
+            [[ "${PWD:P}/" == "${basedir_realpath}"/* ]] || choose_first_available=1
+
+            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.29.2



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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2020-11-14 19:37 [PATCH v4] vcs_info: choose backend by basedir Aleksandr Mezin
@ 2020-12-03  4:01 ` Aleksandr Mezin
  2020-12-04 15:56   ` Daniel Shahaf
  2021-03-29 16:04 ` Daniel Shahaf
  2021-03-29 16:56 ` Daniel Shahaf
  2 siblings, 1 reply; 11+ messages in thread
From: Aleksandr Mezin @ 2020-12-03  4:01 UTC (permalink / raw)
  To: zsh-workers

Any comments on this version?

Documentation is probably still bad, but I tried my best.


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2020-12-03  4:01 ` Aleksandr Mezin
@ 2020-12-04 15:56   ` Daniel Shahaf
  2021-03-27 20:40     ` Lawrence Velázquez
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Shahaf @ 2020-12-04 15:56 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Aleksandr Mezin wrote on Thu, Dec 03, 2020 at 10:01:57 +0600:
> Any comments on this version?

Thanks for the ping.  As to a review, I'm afraid I'm currently ENOTIME, so
someone else will have to do it.

In the meantime, I've added the patch to Etc/BUGS so it's not forgotten.

Thanks again,

Daniel


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2020-12-04 15:56   ` Daniel Shahaf
@ 2021-03-27 20:40     ` Lawrence Velázquez
  0 siblings, 0 replies; 11+ messages in thread
From: Lawrence Velázquez @ 2021-03-27 20:40 UTC (permalink / raw)
  To: zsh-workers; +Cc: Aleksandr Mezin

On Fri, Dec 4, 2020, at 10:56 AM, Daniel Shahaf wrote:
> Aleksandr Mezin wrote on Thu, Dec 03, 2020 at 10:01:57 +0600:
> > Any comments on this version?
> 
> Thanks for the ping.  As to a review, I'm afraid I'm currently ENOTIME, so
> someone else will have to do it.

Ping for Review II: The Quickening

vq


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2020-11-14 19:37 [PATCH v4] vcs_info: choose backend by basedir Aleksandr Mezin
  2020-12-03  4:01 ` Aleksandr Mezin
@ 2021-03-29 16:04 ` Daniel Shahaf
  2021-03-29 16:56 ` Daniel Shahaf
  2 siblings, 0 replies; 11+ messages in thread
From: Daniel Shahaf @ 2021-03-29 16:04 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Aleksandr Mezin wrote on Sun, Nov 15, 2020 at 01:37:03 +0600:
> Also, this patch adjusts VCS_INFO_detect_git and VCS_INFO_detect_cvs to make
> them set vcs_comm[basedir], by moving basedir detection code from corresponding
> VCS_INFO_get_data_* functions. VCS_INFO_detect_git and VCS_INFO_detect_cvs were
> the only VCS_INFO_detect_* functions not setting it.

I've committed this part.  The code wasn't wrong, but consistency
between backends is a good thing to have.

> +++ b/Functions/VCS_Info/Backends/VCS_INFO_get_data_git
> @@ -138,7 +138,7 @@ VCS_INFO_git_handle_patches () {
>  
>  gitdir=${vcs_comm[gitdir]}
>  VCS_INFO_git_getbranch ${gitdir}

It seems to me this call should be moved below the assignment of rrn, in
case VCS_INFO_detect_git ever wants to query zstyles.  (preëxisting issue)

> -gitbase=$( ${vcs_comm[cmd]} rev-parse --show-toplevel 2> /dev/null )
> +gitbase=${vcs_comm[basedir]}
>  if [[ -z ${gitbase} ]]; then
>      # Bare repository
>      gitbase=${gitdir:P}

(Haven't forgotten about the non-drive-by part of the patch, either.)

Cheers,

Daniel


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2020-11-14 19:37 [PATCH v4] vcs_info: choose backend by basedir Aleksandr Mezin
  2020-12-03  4:01 ` Aleksandr Mezin
  2021-03-29 16:04 ` Daniel Shahaf
@ 2021-03-29 16:56 ` Daniel Shahaf
  2021-04-10 20:49   ` Lawrence Velázquez
  2021-04-17 12:17   ` Aleksandr Mezin
  2 siblings, 2 replies; 11+ messages in thread
From: Daniel Shahaf @ 2021-03-29 16:56 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

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


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2021-03-29 16:56 ` Daniel Shahaf
@ 2021-04-10 20:49   ` Lawrence Velázquez
  2021-04-17 12:17   ` Aleksandr Mezin
  1 sibling, 0 replies; 11+ messages in thread
From: Lawrence Velázquez @ 2021-04-10 20:49 UTC (permalink / raw)
  To: Aleksandr Mezin, zsh-workers; +Cc: Daniel Shahaf

On Mon, Mar 29, 2021, at 12:56 PM, Daniel Shahaf 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.

bump

vq


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2021-03-29 16:56 ` Daniel Shahaf
  2021-04-10 20:49   ` Lawrence Velázquez
@ 2021-04-17 12:17   ` Aleksandr Mezin
  2021-04-17 13:16     ` Daniel Shahaf
  1 sibling, 1 reply; 11+ messages in thread
From: Aleksandr Mezin @ 2021-04-17 12:17 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Mon, Mar 29, 2021 at 10:56 PM Daniel Shahaf <d.s@daniel.shahaf.name> 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@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)

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


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2021-04-17 12:17   ` Aleksandr Mezin
@ 2021-04-17 13:16     ` Daniel Shahaf
  2021-05-09 20:28       ` Lawrence Velázquez
  0 siblings, 1 reply; 11+ messages in thread
From: Daniel Shahaf @ 2021-04-17 13:16 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

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@daniel.shahaf.name> 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@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)
> 
> 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[@]}")
> 


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2021-04-17 13:16     ` Daniel Shahaf
@ 2021-05-09 20:28       ` Lawrence Velázquez
  2021-05-31  1:13         ` Lawrence Velázquez
  0 siblings, 1 reply; 11+ messages in thread
From: Lawrence Velázquez @ 2021-05-09 20:28 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Hi Aleksandr,

On Sat, Apr 17, 2021, at 9:16 AM, Daniel Shahaf wrote:
> Aleksandr Mezin wrote on Sat, Apr 17, 2021 at 18:17:20 +0600:
> > Should I send v5 now?
> 
> +1, since part of v4 has been committed.

Just a reminder in case this slipped off your plate.

vq


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

* Re: [PATCH v4] vcs_info: choose backend by basedir
  2021-05-09 20:28       ` Lawrence Velázquez
@ 2021-05-31  1:13         ` Lawrence Velázquez
  0 siblings, 0 replies; 11+ messages in thread
From: Lawrence Velázquez @ 2021-05-31  1:13 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

On Sun, May 9, 2021, at 4:28 PM, Lawrence Velázquez wrote:
> Hi Aleksandr,
> 
> On Sat, Apr 17, 2021, at 9:16 AM, Daniel Shahaf wrote:
> > Aleksandr Mezin wrote on Sat, Apr 17, 2021 at 18:17:20 +0600:
> > > Should I send v5 now?
> > 
> > +1, since part of v4 has been committed.
> 
> Just a reminder in case this slipped off your plate.

second reminder

-- 
vq


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

end of thread, other threads:[~2021-05-31  1:13 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14 19:37 [PATCH v4] vcs_info: choose backend by basedir 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
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

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