On Wed, Oct 9, 2019 at 7:14 PM Daniel Shahaf <d...@daniel.shahaf.name> wrote: > > Branko Čibej wrote on Wed, Oct 09, 2019 at 15:45:32 +0200: > > On 09.10.2019 15:00, Johan Corveleyn wrote: > > > During that entire discussion thread the only objections raised were > > > about "enforcing it in the repos layer / server directly". No-one > > > objected to the proposal(s) to solve the issue via pre-commit hooks. > > > > Validating contents (such as line endings based on svn:eol-style) is a > > perfect fit for hooks. Not normalising of course. :) > > The repos layer validates svn:* revprops in svn_repos__validate_prop(). > > The FS layer doesn't do that validation. > > The result of that is that old repositories with invalid svn:* properties can > be dump/load'd but can't be svnsync'.d > > --- > > As to separation of concerns, that's a valid point, but we should be > consistent > about what concerns belong where. The validation of svn:* props and of > contents of files with svn:eol-style set belongs in the same layer, doesn't > it? > > Therefore, I think there are two options: > > - These validation belongs in the repos layer. The simpler/lower layer that > doesn't do such validation is the FS layer. svn:eol-style validation > belongs > in the repos layer. (And if svnsync needs to bypass it, we'll design a way > for it to do so.) > > - These validations belong $elsewhere. svn:eol-style validation will be added > $elsewhere and svn_repos__validate_prop() will be moved $elsewhere. > > My interpretation: I don't have an opinion yet, except that if we move the > validation out of the repos layer, I won't be quite as sure any more what the > difference between the FS layer and the repos layer _is_. The FS layer > exposes > a filesystem that's a 0-indexed array of trees [implemented by the DAG layer]. > The repos layer does… what, exactly, on top of that?
Thinking about it again some more, I guess I agree that this really belongs in the same place as svn_repos__validate_prop(), and really should be in the repos layer (or in any case, more widely / automatically / standardly available than just a hook script, where it depends too much on the individual sysadmin). In fact, back in 2012 I wasn't happy with the consensus gravitating towards "solve this in a pre-commit hook" (except as a stop-gap measure). But the "standard pre-commit utility" seemed to be the best attainable idea that would receive support from most devs, at the time. Earlier in the current thread Brane said: "Validating contents (such as line endings based on svn:eol-style) is a perfect fit for hooks." But I think this kind of content validation is different from "application-level content validation", like "follow coding style X", or "commit message should contain an issue key", or "I want to enforce that .c files have svn:eol-style=native". Such application-level content rules have no effect whatsoever on the workings of SVN itself. SVN clients don't care about those, only the applications (IDE, users, ...) do. In contrast, having a "non-LF-normalized with svn:eol-style=native" in the repository breaks "svn diff" (all lines shown as different) and "svn status" (if timestamps mismatch, contents are checksummed, and the file shows up as modified while it isn't -- causing confusion and even worse, spurious tree conflicts). So in my eyes this is more of an obligatory validation, to preserve the sanity of your "svn ecosystem". A bit like the sha1 collision protection (yes, the repository can store sha1 collisions perfectly fine, but they wreak havoc amongst your clients / working copies if you let them enter your repository). The "havoc" caused by an SVN-4065 file is orders of magnitude less severe than what a sha1 collision causes (entire working copy hosed), but still, it's not nice. (Julian: sorry for my frustrated reaction earlier, and thanks for staying constructive, as always ... you were right, the discussion was clearly not over yet :-)) -- Johan
