My point wasn't the validation of svn:mergeinfo revprops, it was whether the validation of svn:mergeinfo nodeprops should happen in svn_repos__validate_prop().
I'm not against telling that function whether it's validating a nodeprop or a revprop, and applying different logic in either case. Philip Martin wrote on Fri, Jul 08, 2011 at 20:03:48 +0100: > Daniel Shahaf <[email protected]> writes: > > >> Perhaps. svn_repos__validate_prop is also used by > >> svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo > >> there? > >> > > > > Why not? No one should be setting svn:mergeinfo as a txnprop (or > > revprop). (and if they do, they shouldn't use the svn:* namespace) > > > >> We should probably split the validate function into two, one for node > >> props and one for revprops. > >> > > > > Given that we don't have any SVN_PROP_* whose semantics differ when it's > > set as a revprop v. when it's set as a nodeprop, I'm not sure what this > > would gain; I'm ±0. > > Huh? They differ totally. It makes no sense to check svn:mergeinfo > syntax valid set as a revprop, we should either allow all values or > none. > > > Unless, perhaps, you want to add verification that svn:mergeinfo isn't > > set as a revprop and svn:log isn't set as a nodeprop? Feel free to do > > so, but then I suggest you'll also teach svnsync et al to strip those > > (ill-set) properties to avoid breaking any repositories out there that > > do have svn:mergeinfo revprops and svn:log nodeprops. > > Old repositories are already problem. The existing svn:mergeinfo > validation is going to prevent people dumping/loading repositories with > invalid svn:mergeinfo on nodes. We don't want to add to the problem by > adding syntax checking where we don't need it unless we *also* add the > stripping code. > > -- > uberSVN: Apache Subversion Made Easy > http://www.uberSVN.com
