[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] Re: [PATCH] A test for "Can't get entries" error

From: Daniel Shahaf <d.s_at_daniel.shahaf.name>
Date: Thu, 22 Nov 2018 00:51:37 +0000

Julian Foad wrote on Wed, 21 Nov 2018 16:00 +0000:
> 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.

The comment is somewhat out of date, as it refers to a local variable ID
that doesn't exist. I share your concern, however: I wondered if after
taking the 'shortcut' the loop was entered with some invariant not
holding and if patching the svn_fs_fs__dag_open() call was simply
covering up that underlying issue; however, in the end I think the
issue is real.

It _does_ look odd that open_path_allow_null is checked in two places,
though. I suppose the second check could be removed and deferred to the
next iteration. At that point we might also be able to find a way to
reproduce the original error even in a codepath that doesn't take the
'shortcut', in order to increase our confidence in the patch.

While we're at this function, does anyone understand why directory[1] is
accessed without checking whether directory[0] is not NUL? There is
a comment there, but it doesn't enlighten me. (However, I haven't run
'blame' on that comment yet.) Even if it's correct, is there any reason
not to add an SVN_ERR_ASSERT(directory[0]) there?

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

Have you read the docstring of the open_path_allow_null enumerator?


Received on 2018-11-22 01:51:49 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.