zsh-workers
 help / color / mirror / code / Atom feed
* [RFC PATCH v3] vcs_info: choose backend by basedir
@ 2020-10-29  1:27 Aleksandr Mezin
  2020-10-29  2:26 ` Frank Terbeck
  2020-11-01 23:09 ` Aleksandr Mezin
  0 siblings, 2 replies; 9+ messages in thread
From: Aleksandr Mezin @ 2020-10-29  1:27 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 changes how vcs_info chooses 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 enabled 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.

It's possible to revert to the old behavior of vcs_info, by setting:

    zstyle ':vcs_info:*' backend-choice enablement-order

It can also be set per-backend:

    zstyle ':vcs_info:hg:*' backend-choice enablement-order

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.

I've omitted the documentation from this patch. If the code (and the approcah)
here is ok, I'll work on the documentation later.

Signed-off-by: Aleksandr Mezin <mezin.alexander@gmail.com>
---
 .../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 ++++++++++++++++---
 5 files changed, 44 insertions(+), 17 deletions(-)

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..de8efc6b4 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_closest
+
     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 -m ":vcs_info:${vcs}:${usercontext}:${rrn}" backend-choice enablement-order
+            choose_closest=$?
+
+            [[ "${PWD:P}/" != "${basedir_realpath}"/* ]] && choose_closest=0
+
+            if (( ! choose_closest )) || (( ${#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_closest )) && 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.1



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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-10-29  1:27 [RFC PATCH v3] vcs_info: choose backend by basedir Aleksandr Mezin
@ 2020-10-29  2:26 ` Frank Terbeck
  2020-10-30  6:55   ` Aleksandr Mezin
  2020-11-01 23:09 ` Aleksandr Mezin
  1 sibling, 1 reply; 9+ messages in thread
From: Frank Terbeck @ 2020-10-29  2:26 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Hi,

Aleksandr Mezin wrote:
[…]
> It's possible to revert to the old behavior of vcs_info, by setting:
>
>     zstyle ':vcs_info:*' backend-choice enablement-order

Like I said before, I'm still against changing the default.


Regards, Frank


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-10-29  2:26 ` Frank Terbeck
@ 2020-10-30  6:55   ` Aleksandr Mezin
  2020-10-30 16:47     ` Daniel Shahaf
  0 siblings, 1 reply; 9+ messages in thread
From: Aleksandr Mezin @ 2020-10-30  6:55 UTC (permalink / raw)
  To: Frank Terbeck; +Cc: zsh-workers, Daniel Shahaf

On Thu, Oct 29, 2020 at 8:26 AM Frank Terbeck <ft@zsh.org> wrote:
> Like I said before, I'm still against changing the default.

And Daniel wants the default behavior to be changed, If I understood correctly?


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-10-30  6:55   ` Aleksandr Mezin
@ 2020-10-30 16:47     ` Daniel Shahaf
  0 siblings, 0 replies; 9+ messages in thread
From: Daniel Shahaf @ 2020-10-30 16:47 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: Frank Terbeck, zsh-workers

Aleksandr Mezin wrote on Fri, 30 Oct 2020 12:55 +0600:
> On Thu, Oct 29, 2020 at 8:26 AM Frank Terbeck <ft@zsh.org> wrote:
> > Like I said before, I'm still against changing the default.  
> 
> And Daniel wants the default behavior to be changed, If I understood correctly?

When consensus fails to be reached, status quo is the default, so please adjust the patch accordingly, until and unless consensus otherwise is reached.  (If so, that can be done in a follow-up.)

Please also update _zstyle (the completion function) if you haven't already, per previous request.

Thanks!

Daniel
(will reply to the other questions to me too, yes.  I'm replying not in chronological order.)


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-10-29  1:27 [RFC PATCH v3] vcs_info: choose backend by basedir Aleksandr Mezin
  2020-10-29  2:26 ` Frank Terbeck
@ 2020-11-01 23:09 ` Aleksandr Mezin
  2020-11-04  6:38   ` Daniel Shahaf
  1 sibling, 1 reply; 9+ messages in thread
From: Aleksandr Mezin @ 2020-11-01 23:09 UTC (permalink / raw)
  To: zsh-workers

On Thu, Oct 29, 2020 at 7:28 AM Aleksandr Mezin
<mezin.alexander@gmail.com> wrote:
> It's possible to revert to the old behavior of vcs_info, by setting:
>
>     zstyle ':vcs_info:*' backend-choice enablement-order
>

Now I'm not sure if an "enum-like" zstyle is a good idea.

1) Should I check somewhere that the string value is valid (i.e. it's
either empty, or one of the predefined strings)?

2) Extensibility: I don't think there will be another different mode
that should be controlled by this zstyle. More likely, there will be
parameters like "prefer non-ancestor vcs directories or not". And IMO
they should have their own boolean zstyles, and not something like
`backend-choice closest-vcs-dir-prefer-ancestors`. And even more
likely, nobody will add anything there


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-11-01 23:09 ` Aleksandr Mezin
@ 2020-11-04  6:38   ` Daniel Shahaf
  2020-11-05  0:50     ` Aleksandr Mezin
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Shahaf @ 2020-11-04  6:38 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Sorry for the delayed replies; I've been offline.  More below.

Aleksandr Mezin wrote on Mon, 02 Nov 2020 05:09 +0600:
> On Thu, Oct 29, 2020 at 7:28 AM Aleksandr Mezin
> <mezin.alexander@gmail.com> wrote:
> > It's possible to revert to the old behavior of vcs_info, by setting:
> >
> >     zstyle ':vcs_info:*' backend-choice enablement-order
> >  
> 
> Now I'm not sure if an "enum-like" zstyle is a good idea.
> 

Fair question.

> 1) Should I check somewhere that the string value is valid (i.e. it's
> either empty, or one of the predefined strings)?

(This question presumes we'll be keeping the enum-like design, so I'll
presume the same in my answer.)

If possible, please write the code so as to be forward compatible with
any new enum values we may add in the future.  That is, when reviewing,
we'll consider several cases:

- Style isn't set
- Style is set to one of the predefined strings
- Style is set to a string that will be predefined in a future version
  or to an invalid string (the code can't tell the difference)

Less likely cases include:

- The style is set to the empty array («zstyle $pattern $style» with no further arguments)
- The style is set to the empty string («zstyle $pattern $style ''»)
- The style is set to one of the predefined strings, plus some
  additional words

> 2) Extensibility: I don't think there will be another different mode
> that should be controlled by this zstyle. More likely, there will be
> parameters like "prefer non-ancestor vcs directories or not". And IMO
> they should have their own boolean zstyles, and not something like
> `backend-choice closest-vcs-dir-prefer-ancestors`. And even more
> likely, nobody will add anything there

About extensibility, needs for extensibility aren't always foreseen.

As to their own boolean styles, I'm not sure I agree.  zstyle values are
arrays, so we could let parameters be provided in additional words, as
in:
.
    zstyle $pattern $style enablement-order foo=bar
    zstyle $pattern $style closest-.vcs-dir lorem=ipsum

Hmm.  Perhaps it would make more sense to name the style "disk-layout",
and then the extra parameters could be used to choose whether to select
the closest or farthest .${vcs} dir.

Or perhaps an enum really is overkill, as you say.

Frank?

Cheers,

Daniel


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-11-04  6:38   ` Daniel Shahaf
@ 2020-11-05  0:50     ` Aleksandr Mezin
  2020-11-05 14:49       ` Daniel Shahaf
  0 siblings, 1 reply; 9+ messages in thread
From: Aleksandr Mezin @ 2020-11-05  0:50 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Wed, Nov 4, 2020 at 12:38 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> About extensibility, needs for extensibility aren't always foreseen.

I guess they can't be foreseen both ways, i.e. extensibility may be
needed where you didn't expect, but also maybe extensibility won't be
needed where you expected and designed. But creating a "more
extensible" interface takes more time, and this way you usually have
more complex code. So maybe this "premature extensibility" isn't worth
it?

>
> As to their own boolean styles, I'm not sure I agree.  zstyle values are
> arrays, so we could let parameters be provided in additional words, as
> in:
> .
>     zstyle $pattern $style enablement-order foo=bar
>     zstyle $pattern $style closest-.vcs-dir lorem=ipsum
>
> Hmm.  Perhaps it would make more sense to name the style "disk-layout",
> and then the extra parameters could be used to choose whether to select
> the closest or farthest .${vcs} dir.
>
> Or perhaps an enum really is overkill, as you say.

After your reply, for me it now seems to be even more overkill than before


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-11-05  0:50     ` Aleksandr Mezin
@ 2020-11-05 14:49       ` Daniel Shahaf
  2020-11-09 16:11         ` Aleksandr Mezin
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Shahaf @ 2020-11-05 14:49 UTC (permalink / raw)
  To: Aleksandr Mezin; +Cc: zsh-workers

Aleksandr Mezin wrote on Thu, 05 Nov 2020 06:50 +0600:
> On Wed, Nov 4, 2020 at 12:38 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > About extensibility, needs for extensibility aren't always foreseen.  
> 
> I guess they can't be foreseen both ways, i.e. extensibility may be
> needed where you didn't expect, but also maybe extensibility won't be
> needed where you expected and designed. But creating a "more
> extensible" interface takes more time, and this way you usually have
> more complex code. So maybe this "premature extensibility" isn't worth
> it?

That's possible, of course.  If you think that's the case in the
specific zstyle API design question under discussion, perhaps you can
convince us of your opinion.


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

* Re: [RFC PATCH v3] vcs_info: choose backend by basedir
  2020-11-05 14:49       ` Daniel Shahaf
@ 2020-11-09 16:11         ` Aleksandr Mezin
  0 siblings, 0 replies; 9+ messages in thread
From: Aleksandr Mezin @ 2020-11-09 16:11 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers

On Thu, Nov 5, 2020 at 8:49 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> That's possible, of course.  If you think that's the case in the
> specific zstyle API design question under discussion, perhaps you can
> convince us of your opinion.

Sorry, I don't understand. Do you need more arguments in favor of a
'simple' solution? Because I've already provided all arguments I had
(more extensible == more complex, but necessity is, at least,
questionable)

Or maybe this can be counted as another argument: AFAIK, nobody else
tried to alter vcs_info's behavior with nested repositories. So,
likely, nobody will try to extend it in the future (most people work
with only git repositories, I think). And if it's not important and
nobody cares, simpler is better.


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

end of thread, other threads:[~2020-11-09 16:11 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-29  1:27 [RFC PATCH v3] vcs_info: choose backend by basedir Aleksandr Mezin
2020-10-29  2:26 ` Frank Terbeck
2020-10-30  6:55   ` Aleksandr Mezin
2020-10-30 16:47     ` Daniel Shahaf
2020-11-01 23:09 ` Aleksandr Mezin
2020-11-04  6:38   ` Daniel Shahaf
2020-11-05  0:50     ` Aleksandr Mezin
2020-11-05 14:49       ` Daniel Shahaf
2020-11-09 16:11         ` Aleksandr Mezin

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