Re: [PATCH v5] vcs_info: choose backend by basedir

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

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

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

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

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

Aleksandr Mezin wrote on Fri, Jun 04, 2021 at 12:14:21 +0600:
> Previously, vcs_info iterated over enabled VCS backends and output repository
> status from the first backend that works (i.e. first backend that can find a
> repository).
> 
> But I prefer a different behavior: I want to see the status of the repository
> closest to the current working directory. For example: if there is a Mercurial
> repository inside a Git repository, and I 'cd' into the Mercurial repository,
> I want to see the status of Mercurial repo, even if 'git' comes before 'hg' in
> the 'enable' list.
> 

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

> This patch adds a new algorithm for vcs_info to choose the backend:
> 
> 1. Among backends whose basedir isn't an ancestor of the current directory,
> old algorithm is used: whichever comes first in 'enable' list.
> 
> 2. If all backends return basedirs that are ancestors of the current directory,
> the one with basedir closest to the current directory is chosen.
> 
> As far as I know, unless the user has set basedir manually (GIT_WORK_TREE),
> all backend return basedirs that are ancestors of the current directory.
> Backends that return non-ancestor basedirs have higher priority to show the
> status of that manually-configured repository.
> 

Okay, so the order is actually:

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

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

My question from 48329 stands:
.
	There doesn't seem to be any way to shadow a GIT_WORK_TREE setting,
	other than to temporarily set the enable/disable styles so as to exclude
	git entirely.  Is that a problem?
.
To be clear, the question is why $GIT_WORK_TREE should have precedence
over VCS_INFO_detect_fossil() regardless of style settings.

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

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

> The algorithm used by vcs_info can be configured using 'choose-closest-backend'
> zstyle:
> 
>     zstyle ':vcs_info:*' choose-closest-backend yes
> 
> By default, old algorithm is used.
> 
> It can also be configured per-backend:
> 
>     zstyle ':vcs_info:hg:*' choose-closest-backend no
> 
> This effectively moves the backend into group (1), as if its basedir is never
> an ancestor of the current directory. Not sure how useful this is though.

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

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

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

Cheers,

Daniel