Re: [PATCH 2/3] vcs_info/cvs: set vcs_comm[basedir] in VCS_INFO_detect_cvs

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Aleksandr Mezin wrote on Fri, Nov 29, 2019 at 19:41:51 +0600:
> On Fri, Nov 29, 2019 at 3:34 AM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Daniel Shahaf wrote on Thu, Nov 28, 2019 at 11:13:44 +0000:
> > > Aleksandr Mezin wrote on Tue, 26 Nov 2019 10:22 +00:00:
> > > > On Tue, Nov 26, 2019 at 10:52 AM Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> > > > > % cd foo
> > > > > [shows hg info]
> > > > > % export GIT_WORK_TREE=${PWD/foo/bar} GIT_DIR=${PWD/foo/bar}/.git
> > > > > [shows git info]
> > > > > %
> > > >
> > > > Interesting. I didn't notice that GIT_INFO_detect_git checks the exit
> > > > code of `git rev-parse --is-inside-work-tree`, not the actual value.
> > >
> > > I think it should check both the exit code and the output.
> >
> > Sorry, I didn't look closely enough.  In the example I gave (quoted above),
> > --is-inside-work-tree prints "false" and exits with code 0.  In that situation,
> > I think VCS_INFO_detect_git should detect git, and it does.  All in all,
> > I think the current code is correct [...]
> 
> Then I don't see a sane way to choose which VCS to show, other than
> how it's already done (first one detected from 'enabled'). For
> example, a hg work tree in cwd, and GIT_WORK_TREE is set to something
> else (maybe parent directory, maybe ../other-dir). Which one (hg or
> git) to choose? Why?

As I said in workers/44930 and workers/44936, I propose to show
information for the VCS whose worktree root is deeper; and if neither
worktree root is deeper than the other, _only then_ fall back to the
current behaviour of showing information for the VCS listed first in the
'enable' style.  I think this would address your use-case without
regressing any other.

(By "worktree root" I mean the directory shown by «git rev-parse
--show-toplevel», «svn info --show-item=wcroot», and their equivalents
for other VCS's.)

With this proposal, there are three cases to consider: ${basedirA:P}
and ${basedirB:P} may be either (a) equal, (b) siblings, or (c) parent
and child.  The proposal makes no difference for cases (a) and (b).  For
case (c), it allows the user to see info for either the parent vcs or
the child vcs by cd'ing appropriately.  That would be an improvement
over the current behaviour.

Yes, in cases (a) and (b) there is no obvious correct behaviour.
However, the fact that there's no obvious correct behaviour in a given
situation is simply a data point that should inform the design process;
nothing more, nothing less.  It doesn't make the problem unsolvable.

> I guess I'll have to live with my own fork of vcs_info, because any
> changes there will break it for someone else.

Let's not jump to conclusions.  In this case, I think you have several
courses of action open to you:

- Revise your patch series in light of the feedback received — for
  example, workers/44927 proposed two small changes to the
  «if (( ${#vcs_comm[basedir]:a} > ${#nearest_vcs_comm[basedir]:a} ))»
  line — and resubmit it.  [The changes are basically what I wrote in
  workers/44936, taking the bulleted list for an if/elif/else
  chain.  I think there already is consensus on those changes.]

- Propose some other API change; for example:

  + Make it possible to report on multiple VCS's at the same time (cf
    last paragraph of workers/44936).  This will cover not only the
    parent/child case but also the "same" and "sibling" cases.  I would
    recommend to do this by writing up a behaviour specification
    [ideally as a _pro forma_ patch to Doc/Zsh/contrib.yo] and putting
    it up for discussion, rather than by implementing anything.

  + Add a hook that gets the list of VCS's that have been recognized and
    sets $REPLY to the name of the VCS backend to show info for.

  + Document that in the the "same" and "sibling" cases, it is
    unspecified which VCS's info gets displayed — i.e., vcs_info will
    show the info for one of the backends, but we don't promise which
    one — and recommend to set styles as Mikael explained.

  + Something else.

The (( $+GIT_WORK_TREE )) && [[ -e ./.hg ]] case is an edge case.  Since
it doesn't work _today_, fixing it isn't a blocker to accepting the
patch.  It's perfectly fine to fix just the parent/child case and leave
the "same" and "sibling" cases as they are today.  (Patches should
introduce no regressions, but aren't expected to fix all existing bugs.)

In the meantime, the first two patches in this series can be applied,
can't they?  Or were you planning to revise and resubmit the CVS one to
address the (preëxisting) infinite loop?  The first two patches should
cause no behaviour change, I assume, other than making $hook_com[basedir]
available to hooks earlier.  Actually, should we document $hook_com[basedir]
in the manual?

Cheers,

Daniel

P.S.  I don't know what your background is, but in general, it's _very_
common for patch series to receive reviews and be resubmitted with
changes before they're accepted and merged; that's an everyday
occurrence, no more remarkable than seeing somebody give up their seat
on a city bus.