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
Any comments on this version? Documentation is probably still bad, but I tried my best.
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
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
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
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[@]}")
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
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[@]}")
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[@]}") >
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
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