Follow-up Comment #1, patch #5155 (project cvs): This patch appears to remove some recent changes to the latest sources even as it adds in your new feature. This is undesirable.
The indentation for the lines added via patch.diff does not follow the HACKING guidelines. Indent by 4 spaces rather than by a tabstop. Additions to the client/server protocol need to be discussed and ratified on both the [EMAIL PROTECTED] and the [EMAIL PROTECTED] Please note that CVSNT already has some ACL based commands (chacl2, rchacl2, lsacl, rlsacl). In general, it would be nice to understand if there is any functional overlap between CVSNT and your new proposed feature addition to CVS. It is highly desirable that you include doc/cvs.texi (to add information on the new CVS commands and CVSROOT/config options) and doc/cvsclient.texi (The CVS client/server protocol) I am concerned that this FEATURE seems to make use of a set-uid or set-gid cvs executable on the server. I believe that the NetBSD folks have a number of SETXID_SUPPORT patches to make this safer, but the use of the SETXID_SUPPORT setup has not been well tested recently and may introduce security flaws. Could you address the setups where ownership and groups are being checked and modified? Is that only for :pserver: use or is it also available under :ext: (just looking at the patch it is not immediately obvious). I do understand that this is an ongoing http://cvsacl.sourceforge.net project and that it is desirable to get this code adopted into the CVS FEATURE release. I just want to make sure that everything needful is in place. Please replace code constructs like this: accessfile = xmalloc (strlen (current_parsed_root->directory) + sizeof (CVSROOTADM) + sizeof (CVSROOTADM_ACCESS) + 3); strcpy (accessfile, current_parsed_root->directory); strcat (accessfile, "/"); strcat (accessfile, CVSROOTADM); strcat (accessfile, "/"); strcat (accessfile, CVSROOTADM_ACCESS); with Xasprintf () calls like this: accessfile = Xasprintf ("%s/%s/%s", current_parsed_root->directory, CVSROOTADM, CVSROOTADM_ACCESS); If I understand the code correctly, there are times when a file called 'access' could be sprinkled throughout the repository. I suspect that CVSup might not do the right thing with such files (I have not looked closely). Do you know if access rights are properly copied in such situations? I suspect that the contrib/validate_repo.pl will need some updates to deal with such files. There may be more things that I missed, but that is my first pass feedback on the patch so far. _______________________________________________________ Reply to this item at: <http://savannah.nongnu.org/patch/?func=detailitem&item_id=5155> _______________________________________________ Message sent via/by Savannah http://savannah.nongnu.org/ _______________________________________________ Bug-cvs mailing list [email protected] http://lists.nongnu.org/mailman/listinfo/bug-cvs
