From dev-return-38568-archive-asf-public=cust-asf.ponee.io@subversion.apache.org Wed Nov 21 17:00:22 2018 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id A815E180668 for ; Wed, 21 Nov 2018 17:00:21 +0100 (CET) Received: (qmail 85606 invoked by uid 500); 21 Nov 2018 16:00:20 -0000 Mailing-List: contact dev-help@subversion.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Delivered-To: mailing list dev@subversion.apache.org Received: (qmail 85596 invoked by uid 99); 21 Nov 2018 16:00:20 -0000 Received: from mail-relay.apache.org (HELO mailrelay2-lw-us.apache.org) (207.244.88.137) by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 21 Nov 2018 16:00:20 +0000 Received: from auth2-smtp.messagingengine.com (auth2-smtp.messagingengine.com [66.111.4.228]) by mailrelay2-lw-us.apache.org (ASF Mail Server at mailrelay2-lw-us.apache.org) with ESMTPSA id C57672E69 for ; Wed, 21 Nov 2018 16:00:19 +0000 (UTC) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailauth.nyi.internal (Postfix) with ESMTP id 543BB220E4 for ; Wed, 21 Nov 2018 11:00:19 -0500 (EST) Received: from web3 ([10.202.2.213]) by compute7.internal (MEProxy); Wed, 21 Nov 2018 11:00:19 -0500 X-ME-Sender: X-ME-Proxy: Received: by mailuser.nyi.internal (Postfix, from userid 99) id ED3259E59B; Wed, 21 Nov 2018 11:00:18 -0500 (EST) Message-Id: <1542816018.3058106.1584613232.4FC1048B@webmail.messagingengine.com> From: Julian Foad To: dev@subversion.apache.org MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="utf-8" X-Mailer: MessagingEngine.com Webmail Interface - ajax-3449945b Subject: Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error References: <55095032.ozZm5OQrnM@dmit10> <20181120072905.u6pc5pzphp2cz4so@tarpaulin.shahaf.local2> <1542703110.1751358.1582899104.3801B128@webmail.messagingengine.com> <1542706099.3133757.1582952768.6221A4B5@webmail.messagingengine.com> <20181121145523.kgwfhlrhiffm5ru7@tarpaulin.shahaf.local2> In-Reply-To: <20181121145523.kgwfhlrhiffm5ru7@tarpaulin.shahaf.local2> Date: Wed, 21 Nov 2018 16:00:18 +0000 Daniel Shahaf wrote: > Daniel Shahaf wrote on Tue, Nov 20, 2018 at 09:28:19 +0000: > > The test XPASSes with SVN_X_DOES_NOT_MARK_THE_SPOT=1 (see notes/knobs), > > so it's something to do with the caches. > > So, looking at subversion/libsvn_fs_fs/tree.c: [...] > In words: svn_fs_fs__dag_open() is asked to find the child "iota" of > r2:/B/mu, and instead of setting 'child' to NULL as the code expects, it > returns an error which percolates all the way to the client. > > The reason SVN_X_DOES_NOT_MARK_THE_SPOT fixes it is that it bypasses the > optimization earlier in the function. That optimization causes the the > very first iteration of the loop is to process "/B/mu". With caches > disabled, the first iteration of the loop processes "/" and the second > iteration processes "/B" and exits early, here: > > 1144 /* The path isn't finished yet; we'd better be in a directory. */ > 1145 if (svn_fs_fs__dag_node_kind(child) != svn_node_dir) > 1146 { > 1147 const char *msg; > 1148 > 1149 /* Since this is not a directory and we are looking for some > 1150 sub-path, that sub-path will not exist. That will be o.k., > 1151 if we are just here to check for the path's existence. */ > 1152 if (flags & open_path_allow_null) > 1153 { > 1154 parent_path = NULL; > 1155 break; > 1156 } > > So, we just need to make the svn_fs_fs__dag_open() call set child=NULL > so it can fall back to the existing logic for handling FLAGS: > > [[[ > Index: subversion/libsvn_fs_fs/tree.c > =================================================================== > --- subversion/libsvn_fs_fs/tree.c (revision 1845259) > +++ subversion/libsvn_fs_fs/tree.c (working copy) > @@ -1083,8 +1083,10 @@ > pool)); > if (cached_node) > child = cached_node; > - else > - SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool)); > + else if (svn_fs_fs__dag_node_kind(here) == svn_node_dir) > + SVN_ERR(svn_fs_fs__dag_open(&child, here, entry, pool, iterpool)); > + else > + child = NULL; > > /* "file not found" requires special handling. */ > if (child == NULL) > ]]] > > Makes sense? The top-of-loop comment says: "/* Whenever we are at the top of this loop: - HERE is our current directory, ..." As HERE is apparently NOT a directory in this case, I wonder if the comment simply should say something like "current path (usually a directory)", or whether anything else is amiss too. In reviewing the code I was unable to keep track of all the nuances of what happens (and should happen) in all the edge cases. Especially when 'flags & open_path_allow_null' is true and the requested path is a child of a non-directory like the "/B/mu/iota" in this case: that combination doesn't seem to be well documented, which makes me wonder what the callers expect it to do. -- - Julian