From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FROM,MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, UNPARSEABLE_RELAY autolearn=ham autolearn_force=no version=3.4.4 Received: (qmail 20109 invoked from network); 28 Oct 2020 01:34:29 -0000 Received: from zero.zsh.org (2a02:898:31:0:48:4558:7a:7368) by inbox.vuxu.org with ESMTPUTF8; 28 Oct 2020 01:34:29 -0000 ARC-Seal: i=1; cv=none; a=rsa-sha256; d=zsh.org; s=rsa-20200801; t=1603848869; b=sT+tTu1NQKhJh00QGxgQQWD63XYRz/Gf60HP0AvvWUpHLYEVG+6kKRy72WcMPvH1MTo4qEMDY5 kUbpgu5iBZZSXG1LQv5Yv0mE8Zroy1+q1183MyQoyMNMVYYlEfSQW7F+w3UEG3B6aSqbdx+Lxf oAE/l4eUeQpMPLdue9xpDVYLVMt/JAnXKUYH1kZ9E9wyePFl1LWkcnIWxzcOmhVD+peFkt2jCj rS25MmNOTeVPYwiorFNZ6NeUTU2syup72UBf5i7yDgDtYiwgmlPlphKZR1097Qw4CZMI0A38Oi Cmefxgt4JDcCEyHVlbHaDRl1ZAgfB287W2WSoX2GpraIuQ==; ARC-Authentication-Results: i=1; zsh.org; iprev=pass (mail-io1-f65.google.com) smtp.remote-ip=209.85.166.65; dkim=pass header.d=gmail.com header.s=20161025 header.a=rsa-sha256; dmarc=pass header.from=gmail.com; arc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed; d=zsh.org; s=rsa-20200801; t=1603848869; bh=zpo18wLhHNwijT5aTEN7Bna0dK4hqarPanOdV0zEFZI=; h=List-Archive:List-Owner:List-Post:List-Unsubscribe:List-Subscribe:List-Help: List-Id:Sender:Content-Transfer-Encoding:Content-Type:Cc:To:Subject: Message-ID:Date:From:In-Reply-To:References:MIME-Version:DKIM-Signature: DKIM-Signature; b=Ylhfj3jj552qWrP5PwpOYagarqjzmyWDzNO3cMGGt+KOrnwPaaYemlub0m0m4UBTOP65Tb0bze MSpo9pXMl0d2JCAA6qshfnGes0IO1CqMQ/ZLHRZ/mJr3GcmmuIEkWTDxfrXkbJ4ezclr+AAefS od+mq7I3/yfCzk+s1+2clD1srw2RikdcGjBxox0rjH4VO6BdQGULSnur+TUIxIKXMMGL09tPmm QjItellM5Rdj5WI7BvdLuKSlbTvR3UFtAqMPa+mZepn+cmU9cjvPYf4iCkzlz92PpWfYBMl4Ro 1+GDNgs1v3O1Z78P7VLPpJtYHoS/jNXOa0mQLlEH00iXIg==; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=zsh.org; s=rsa-20200801; h=List-Archive:List-Owner:List-Post:List-Unsubscribe: List-Subscribe:List-Help:List-Id:Sender:Content-Transfer-Encoding: Content-Type:Cc:To:Subject:Message-ID:Date:From:In-Reply-To:References: MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID; bh=+uFyIdrMKwBQi4tXBamEnCKVkpVXDX5Jgz2jNZDfWTU=; b=efNUo0CBOJT7ZxtR3oIcYogqBq fYT0xQppt0R3O294og01r1muoHbepxe31uPJ8wuglTQn8vmre/A7ea5G8zrdGLNdSEp3bIHNO66oT X1+7WCw3lthOrxXEH5XxQ+A5ALKW6KrdN03EExyjn779BdXJZpbRyUdKoj0DT4Q35rEW8g7ecb9OE xURDgrJhtSSISHrvEMRZK35UNU/AxMjfGmu+lrrAZPt5ZdLE4IzAcISn9HhVTu6lWG0WHneQVpan2 vV0o3nEuWBo+oTS0JPQLLZtVydoxvcmJzWFamD2b4FLOL60aJEe+Y8VqADO35KD/vjQ1Dra+j5jXq Ls42AQHg==; Received: from authenticated user by zero.zsh.org with local id 1kXaM3-000J1u-Un; Wed, 28 Oct 2020 01:34:24 +0000 Authentication-Results: zsh.org; iprev=pass (mail-io1-f65.google.com) smtp.remote-ip=209.85.166.65; dkim=pass header.d=gmail.com header.s=20161025 header.a=rsa-sha256; dmarc=pass header.from=gmail.com; arc=none Received: from mail-io1-f65.google.com ([209.85.166.65]:38208) by zero.zsh.org with esmtps (TLS1.3:TLS_AES_128_GCM_SHA256:128) id 1kXaLm-000IsW-No; Wed, 28 Oct 2020 01:34:07 +0000 Received: by mail-io1-f65.google.com with SMTP id y20so3686203iod.5 for ; Tue, 27 Oct 2020 18:34:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=+uFyIdrMKwBQi4tXBamEnCKVkpVXDX5Jgz2jNZDfWTU=; b=Yr7rNhviVc+lBLrcrD+QextoeN2zEkE2pRs9SBHIE1zf8Va0qiBTkup4DXPayP5ORa K+t54SJ1gjK2zHOdB0IJ1qBv9BvwuEoUnjAhTSkSv9jvuVLQl7e9jZDLtUJFcBAX+7DI 2I1z1H7d3snR0Faip4We53Y4sRwNdsL779yMtd14UBaGEe+CeaAOYz+CSz/FEpzhZQae Sc40JmitBiVVREsQlG8rnX5ohiC95JdcRkuPhTg+jQ1fufiTC3zkzT/rh7sXSDfwCg8/ SbVmTGRqB4DzTtZRUPFYO7OhzeGxvzZwgnjxb9UTK/ovQSt/FcwRnqcNFQajGpU820oa Aswg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=+uFyIdrMKwBQi4tXBamEnCKVkpVXDX5Jgz2jNZDfWTU=; b=SSQYtJWjREFi8CiZ+eApRyuhEZrwLfH2l+Is2am1nWvgogrFeK8x6P6DEHZpCSg08Z dyMfhW3eqbT+4guVugYOlSls7GNwvyfZxfTLT4SzYMDSzsSoyB0vn6pOGDqYbANzoyuj EoSipEq1oi0sDrg7/43aeZNfLYr+yqz7AwZ5rqOjLxW1jaYbacjvJ3278/w+syO4/3Gk XBYMAN9QRBXc+FQ3ycgwf4eprc6QP0qrzk8yeIgPNSGgL284nFcHD7z/Wko+/J7kmuTk 1BvVB7ef+ATG5ZMv0cc6/pBUQy2GD0ImFdUkL29FU7+XjWs5T8qxildoVa0IdVNHYtfB 3cAw== X-Gm-Message-State: AOAM5332uE6tF0LF1S8xSiGzNwYRiwKWB4pJPJh4TQU2MKVOQ9zXrW+1 QxfZl4qYfhGymhCvX2ilXuXCjo3p107RU3JGpkc= X-Google-Smtp-Source: ABdhPJxw5OXCl1LaHHt1Uezsk7bZ7nX+XYdkSaLJDdxZPJlvm4AXsHpvba+AeLoHKgcP832rTgyx1wPfNYw2creh6Ss= X-Received: by 2002:a05:6638:1189:: with SMTP id f9mr5115430jas.10.1603848845166; Tue, 27 Oct 2020 18:34:05 -0700 (PDT) MIME-Version: 1.0 References: <20201023083444.1565608-1-mezin.alexander@gmail.com> <20201023234855.5a0c6290@tarpaulin.shahaf.local2> In-Reply-To: <20201023234855.5a0c6290@tarpaulin.shahaf.local2> From: Aleksandr Mezin Date: Wed, 28 Oct 2020 07:33:54 +0600 Message-ID: Subject: Re: [PATCH] vcs_info: add 'find-deepest' zstyle To: Daniel Shahaf Cc: zsh-workers@zsh.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Seq: 47507 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: List-Subscribe: List-Unsubscribe: List-Post: List-Owner: List-Archive: Archived-At: On Sat, Oct 24, 2020 at 5:49 AM Daniel Shahaf wrot= e: > > Aleksandr Mezin wrote on Fri, 23 Oct 2020 14:34 +0600: > > Currently, vcs_info iterates over enabled VCS backends and outputs repo= sitory > > status from the first backend that works (i.e. first backend that can f= ind a > > repository). > > > > But I prefer a different behavior: I want to see the status of the repo= sitory > > closest to the current working directory. For example: if there is a Me= rcurial > > repository inside a Git repository, and I 'cd' into the Mercurial repos= itory, > > I want to see the status of Mercurial repo, even if 'git' comes before = 'hg' in > > the 'enable' list. > > I suppose that's the Right thing to do in most cases: e.g., it's > context-sensitive whereas the 'enable' style's value doesn't usually > depend on the current directory. > > (Separately, in your example it might be nice to indicate that there's > an "outside" Git repository, but that'd be a separate feature request.) > > > Because some people, apparently, want the old behavior of vcs_info, > > What's those people's argument? The default behaviour should be decided > on on this list, not simply announced to this list as a _fait accompli_. I can change the default to 'yes', no problem. I think I should have mentioned that this is a "v2" of this patch. Here are some important quotes from comments for the v1: Bart Schaefer wrote on Mon, Nov 25, 2019 at 01:13:21 -0800: > I have several directories that are BOTH git and CVS repositories, used f= or > keeping two different origins in sync. > > This is admittedly an unusual situation -- we have a legacy process that > expects to fetch sources from CVS, and developers collaborating through g= it > -- but I would be annoyed if vcs didn't find the git data because of the > CVS subdir. Previously, you proposed the following solution: > How about showing the information from the worktree whose root is deeper > (=3D closer to cwd)? Users can show the information for the other worktr= ee > by setting the 'enable' style in a :vcs_info:foo:* context and calling > =C2=ABvcs_info foo=C2=BB. ... > In my proposal, if {foo,foo/bar,foo/bar/baz}/CVS > and foo/bar/.git all exist, and cwd is foo/bar/baz/, git info would be sh= own > because the git root, foo/bar/, is closer (deeper) than the CVS root, foo= / =E2=80=94 > notwithstanding that foo/bar/baz/CVS exists. I thought this patch implements what you suggested, except for CVS. After re-reading the comments I think I made a mistake with VCS_INFO_detect_cvs, and should revert it to what it was in v1: > -[[ -d "./CVS" ]] && [[ -r "./CVS/Repository" ]] && return 0 > -return 1 > +[[ -d "./CVS" ]] && [[ -r "./CVS/Repository" ]] || return 1 > + > +local cvsbase=3D"." > +while [[ -d "${cvsbase}/../CVS" ]]; do > + cvsbase=3D"${cvsbase}/.." > +done > +vcs_comm[basedir]=3D${cvsbase:P} > + > +return 0 (and maybe fix that potentially infinite loop, yes) It will work for Bart's use case this way, and it's what VCS_INFO_get_data_cvs currently does. > > > I made it configurable, and, by default, old behavior is kept. To > > enable new behavior, enable 'find-deepest' zstyle: > > > > zstyle ':vcs_info:*' find-deepest yes > > > > I'd probably set this in my dotfiles: in the rare cases that I nest > repositories, I don't add files in the deeper repository to the > shallower repository. > > > After this change, vcs_info will try all enabled backends, and choose t= he one > > that returns the deepest basedir path. Usually it will be the repositor= y > > closest to the current directory - if all basedirs are ancestors of the > > current directory. I do not care about cases when a basedir isn't an an= cestor > > (for example, if GIT_WORK_TREE is pointing to an unrelated directory). > > Would you clarify the last sentence (not the parenthetical but the part > before it)? I do not have a strong opinion on how vcs_info should behave when some basedir isn't an ancestor of pwd. When some basedir is a subdirectory in pwd, vcs_info should likely choose that backend (because it should mean that the user specified that basedir explicitly - for example GIT_WORK_TREE). But when a basedir is neither ancestor nor subdir of pwd, simply choosing the backend based on path depth doesn't look quite correct to me. But I don't know what the correct way is, so I ignore the fact that vcs_info could behave weirdly (from the user's POV) in this case. Maybe vcs_info should actually prefer the basedir that's not an ancestor of pwd (i. e. choose it immediately when encountered, before all depth comparisons)? Because "basedir isn't an ancestor of pwd" is "the user somehow specified this directory explicitly". As far as I understand. > > > Also, this patch adjusts VCS_INFO_detect_git and VCS_INFO_detect_cvs to= make > > them set vcs_comm[basedir]. They were the only VCS_INFO_detect_* functi= ons not > > setting it. > > Thanks. I don't recall if there's "How to write a backend" > documentation I haven't found anything like that. > but so long as all existing backends get it right, we're > probably covered well enough in practice. Going forward, we could > heavily comment one of the backends and declare it documentation, as > with Test/B01cd.ztst. > > > Signed-off-by: Aleksandr Mezin > > --- > > Doc/Zsh/contrib.yo | 9 +++++++ > > .../VCS_Info/Backends/VCS_INFO_detect_cvs | 2 +- > > .../VCS_Info/Backends/VCS_INFO_detect_git | 1 + > > .../VCS_Info/Backends/VCS_INFO_get_data_git | 2 +- > > Functions/VCS_Info/vcs_info | 25 ++++++++++++++++--- > > Please update _zstyle. > > > 5 files changed, 33 insertions(+), 6 deletions(-) > > > > diff --git a/Doc/Zsh/contrib.yo b/Doc/Zsh/contrib.yo > > index 00f693664..88d00c0ac 100644 > > --- a/Doc/Zsh/contrib.yo > > +++ b/Doc/Zsh/contrib.yo > > @@ -1238,6 +1238,14 @@ of unapplied patches (for example with Mercurial= Queue patches). > > > > Used by the tt(quilt), tt(hg), and tt(git) backends. > > ) > > +kindex(find-deepest) > > +item(tt(find-deepest))( > > +By default, tt(vcs_info) tries backends in the order of 'enable' list,= and > > +returns the output of the first backend that can find a repository. Wh= en > > +tt(find-deepest) is set to tt(true), tt(vcs_info) will try all enabled= backends, > > +and will choose the one that > > Multiple tweaks here. How about: > > 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 successfu= lly detect a repository. > > When this style (tt(find-deepest)) is set to tt(true), tt(vcs_info) w= ill try all enabled backends, > and will operate on [...] > > Wordier, yes, but "the output of a backend" didn't parse for me. If I add the documentation written by you to my patch, I guess I should mention you in the commit message somehow? > > > +returns the deepest tt(basedir) path. > > "basedir" is an implementation term; readers won't know what it means. > Keeping it would be like having zshparam(1) document $? as "The value > of tt(lastval)". > > If /foo/.${vcs} and /foo/bar/.${vcs} both exist (same value of ${vcs} in > both) the style is set, the latter should be used. However, you don't > seem to be handling this case in any way. I guess that if this patch is > to be accepted, we'll have to go through all backends and verify that > they prefer /foo/bar/.${vcs} to /foo/.${vcs} when both exist and the > style is set appropriately. It's probably best if you go through > backends you can install via your distro and ask -workers@ volunteers to > go through any others. (Some backends will probably be left over, but > we can cross that bridge when we come to it.) If the current directory is /foo/bar in your example, then, I think, all backends, except CVS, will choose /foo/bar/.${vcs} Most of them simply use VCS_INFO_bydir_detect, git will definitely choose /foo/bar too. CVS is special just because it has its subdirectory in every directory of the working copy. VCS_INFO_get_data_cvs thinks that the last successive parent directory that still has CVS subdirectory is the root of the working copy. And I think it's correct. > > When /foo/.${vcs} and /foo/bar/.${vcs} both exist and =C2=ABfind-deepest= =C2=BB is > set to _=C2=ABfalse=C2=BB_, might someone expect /foo/.${vcs} to be used?= (After > all, one might reasonably presume flipping the style's value would > _somehow_ affect vcs_info's behaviour.) Perhaps we can name the style > more unambiguously? How about changing it from boolean to enum, as in: > . > zstyle ':vcs_info:*' detection-algorithm enablement-order > zstyle ':vcs_info:*' detection-algorithm deepest-.vcs-dir > . > That'd also be more extensible. Yep, I agree > > Separately, as implemented the style doesn't quite go by the _deepest_ > basedir path. Rather, it goes by the one that _has more path > components_.=C2=B9 That's not the same thing, at least if you take "X is > deeper than Y" to imply "X is a (possibly nested) subdirectory of Y", > which, I think, would be the appropriate semantics here. For instance, > given the pair of repository root paths {/foo/bar/baz, /lorem/ipsum}, > the patch will prefer /foo/bar/baz, even though it's not "deeper than" > /lorem/ipsum. I don't think setting find-deepest should affect the > relative precedence of /foo/bar/baz and /lorem/ipsum=E2=80=A6 I used the word 'deep/deepest' because I was thinking that "depth" in a tree is the length of the path from the root. Which is a number, and can be compared for any nodes. Is this incorrect? Maybe I should iterate over basedirs, and just check if "current" basedir is a subdir (maybe nested) of the current best match? I'm ok with this behavior too. Though I don't find it any better in "weird" cases - when some basedirs aren't ancestors of pwd > > =E2=80=A6 unless either of the two directories happens to be an ancestor = of cwd, > in which case there's an argument to be made that that directory should > be preferred. (I hope we don't have to worry about symlinks here.) > > Moreover, the above description wasn't entirely accurate. The patch > doesn't count path components; it counts slashes. That's not > equivalent, because of things like =C2=AB/foo/bar=C2=BB, =C2=AB/foo/bar/= =C2=BB, > =C2=AB/foo/./bar=C2=BB, =C2=AB/foo//bar=C2=BB, and =C2=ABbar=C2=BB (the l= ast one depends on cwd, of > course). For instance, =C2=AB/foo///bar=C2=BB will be considered deeper = than > =C2=AB/foo/bar/baz=C2=BB. I think vcs_comm[basedir] can never contain duplicate slashes - either the value assigned to it is expanded with ':P' modifier, or the vcs itself returns it in that form (for example, git rev-parse --show-toplevel removes duplicate slashes from GIT_WORK_TREE). If necessary, I can add an extra expansion with ':P' in vcs_info itself. > > I think a reasonable next step would be to nail down what repository > should be chosen, given $PWD and a set of existing .${vcs} dirs > (possibly more than one of those per value of ${vcs}). For instance: > > 1. The .${vcs} dir whose parent is an ancestor of ${PWD}; if there's > more than one of these, the closest ancestor; if there's more than > one of _these_, the first enabled. > > 2. If there's no such .${vcs} dir, then a .${vcs} dir from the first > backend in enablement order. If there's more than one of these, > then XXX. (TODO: What if the first backend in enablement order > identifies two repository directories, and one of them is an > ancestor of the other? Which one will be used?) > > > +Usually it will be the repository closest to the current directory. > > Documentation shouldn't say "usually" without also explaining what > happens in the _unusual_ case. > > > +++ b/Functions/VCS_Info/Backends/VCS_INFO_detect_cvs > > @@ -7,5 +7,5 @@ setopt localoptions NO_shwordsplit > > -[[ -d "./CVS" ]] && [[ -r "./CVS/Repository" ]] && return 0 > > +[[ -d "./CVS" ]] && [[ -r "./CVS/Repository" ]] && vcs_comm[basedir]= =3D.(:P) && return 0 > > =C2=AB.(:P)=C2=BB won't be expanded here. Oops. Also, again, it seems that I should revert to the previous version of my patch here - find the root CVS directory, like VCS_INFO_get_data_cvs does. > > Cheers, > > Daniel