On Thu, Sep 08, 2011 at 03:42:55PM +0300, Daniel Shahaf wrote:
> Stefan Sperling wrote on Thu, Sep 08, 2011 at 14:21:34 +0200:
> > On Thu, Sep 08, 2011 at 01:39:24AM -0000, danie...@apache.org wrote:
> > > Author: danielsh
> > > Date: Thu Sep 8 01:39:23 2011
> > > New Revision: 1166496
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1166496&view=rev
> > > Log:
> > > On the fs-successor-ids branch, factor out repeated logic.
> > >
> > > This misbehaves a little when I test it, but eyeballing the diff convinces
> > > me that those the behaviour of the code must be identical before/after
> > > this
> > > patch... so I'm committing it anyway :).
> >
> > How does it misbehave?
> >
>
> I chmod'd revs/ shards and files and successors/ shards and files, and
> committed revisions ('svn mkdir file://$PWD/r1/$RANDOM -mm' in a loop)
> until new shards were opened, and then I examined the permissions on the
> new shards. The permissions weren't always preserved, and I think in at
> least one case the 0100 bit was removed from a directory.
Ah, OK. Some edge-case bug...
> > > +static svn_error_t *
> > > +make_shard_dir(const char *(*path_some_shard)(svn_fs_t *,
> > > + svn_revnum_t,
> > > + apr_pool_t *),
> >
> > This is awkward.
> > Why not pass the shard path as an argument to this function?
> >
>
> What would making this change gain?
The function declaration within the parameter list looks hideous.
We usually define a typedef where we pass functions as parameters.
I don't think we should do this here because it is not a situation
where we need a callback pattern.
It may look clever and all but it causes unnecessary noise when
reading the code IMO.
>
> > > + svn_fs_t *fs,
> > > + svn_revnum_t revision,
> > > + apr_pool_t *pool)
> > > +{
> > > + /* We don't care if this fails because the shard already existed
> > > + * for some reason. */
> > > + svn_error_t *err;
> > > + const char *new_dir;
> > > +
> > > + new_dir = path_some_shard(fs, revision, pool);
> > > + err = svn_io_dir_make(new_dir, APR_OS_DEFAULT, pool);
> > > + if (err && !APR_STATUS_IS_EEXIST(err->apr_err))
> > > + SVN_ERR(err);
> > > + svn_error_clear(err);
> > > + SVN_ERR(svn_fs_fs__dup_perms(new_dir,
> > > + svn_dirent_dirname(new_dir, pool),
> > > + pool));
> > > +
> > > + return SVN_NO_ERROR;
> > > +}
