Re: [PATCH] reintegrate branch mergeinfo authz check (for glasser)

On Jan 10, 2008 2:58 AM, Karl Fogel <kfogel_at_red-bean.com> wrote:
> "David Glasser" <glasser_at_davidglasser.net> writes:
> >> + for (i = 0; i < mergeinfo->nelts; i++)
> >
> > Uh, isn't mergeinfo an apr_hash_t **? This is not how you iterate
> > over a hash...
>
> WTF? It *compiled*, sheesh. (Haven't tested at all yet; I just
> wanted your review on the general approach, though am very glad to
> have you spot things like this too :-) ).

Yeah, the general approach looks good to me.

*well*. I would be tempted to write the whole function without the
whole "optimize things if everything is readable by checking
readable_paths != paths" thing, since I think it would be a whole lot
simpler to always make a new readable_paths array (in the part above
your hash), and to cache all the return values from the authz function
in the first half in some other hash. Seems simpler than the twisty
conditional logic you have here, and the cost of copying one array of
strings shouldn't be too high. ie:

readable_paths = paths
authz_cache = {}

if authz_func:
  readable_paths = []
  for path in paths:
    readable = authz_func(path)
    authz_cache[path] = readable # but protect this to make sure that
"no" is different from "didn't check"
    if readable:
    readable_paths.append(path)

if len(readable_paths) > 0:
  mergeinfo_catalog = svn_fs_get_mergeinfo(readable_paths)

if authz_func:
  for path in mergeinfo_catalog.keys():
    if authz_cache[path] == False:
      mergeinfo_catalog.delete_key(path)
    if authz_cache[path] exists:
      continue
    readable = authz_func(path)
    if not readable:
      mergeinfo_catalog.delete_key(path)

Your version looks correct, I think think mine is more straightforward.

--dave

-- 
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org