Closed (fixed)
Project:
Drupal.org infrastructure
Component:
Other
Priority:
Critical
Category:
Task
Assigned:
Reporter:
Created:
6 Dec 2007 at 03:44 UTC
Updated:
21 Aug 2014 at 21:00 UTC
Jump to comment: Most recent, Most recent file
I was starting to get really depressed about http://drupal.org/node/152832 and http://drupal.org/node/97084 -- I was doing some testing and discovered that when you add files with a sticky tag, CVS's taginfo hook is never invoked. :( That's why we've got so many bogus branches in our repository. So, for example:
% cvs -d:pserver:[blah] co -r DRUPAL-5--1-0 -l contributions/modules/signup
% cd contributions/modules
% mkdir badness
% cvs add badness
Directory /cvs/drupal-contrib/contributions/modules/badness added to the repository
--> Using per-directory sticky tag `DRUPAL-5--1-0'
% cd badness
% touch badness.module
% cvs add badness.module
cvs add: scheduling file `badness.module' for addition on branch `DRUPAL-5--1-0'
cvs add: use `cvs commit' to add this file permanently
% cvs commit -m "see how easy it is to make life miserable?"
cvs commit: Examining .
/cvs/drupal-contrib/contributions/modules/badness/Attic/badness.module,v <-- badness.module
new revision: 1.1.2.1; previous revision: 1.1
RCS file: /cvs/drupal-contrib/contributions/modules/badness/Attic/badness.module,v
Working file: badness.module
head: 1.1
branch:
locks: strict
access list:
symbolic names:
DRUPAL-5--1-0: 1.1.0.2
keyword substitution: kv
total revisions: 2; selected revisions: 2
description:
----------------------------
revision 1.1
date: 2007-12-06 03:31:10 +0000; author: dww; state: dead; commitid: 21de47576cfe4567;
branches: 1.1.2;
file badness.module was initially added on branch DRUPAL-5--1-0.
----------------------------
revision 1.1.2.1
date: 2007-12-06 03:31:10 +0000; author: dww; state: Exp; lines: +0 -0; commitid: 21de47576cfe4567;
see how easy it is to make life miserable?
=============================================================================
Never once was taginfo invoked to see if this was a valid branch. :( (I just removed that from our repo, sorry for the spam to the CVS commit list)
HOWEVER, I HAVE A SOLUTION! ;)
I realized I had already solved a similar problem for the CVS repo at my day job, many years ago. However, we don't use pserver there, so I was afraid it wasn't going to work. :( However, I just tested this extensively on my laptop, and it appears that pserver will work much the same way! YAY!!
Here's the deal:
When pserver runs on the server, the commit ends up in a tmp directory with a copy of the file you want to commit, and a local CVS directory in that tmp dir with all the metadata. It's sort of like you just ftp'ed the modified/new files to a tiny little temp cvs workspace on the server's filesystem, and then the commit happens normally from the local machine. So, in our commitinfo hook, we can actually just test if CVS/Tag exists in the current directory (the temp dir) to see if there's a sticky tag for this commit. If so, we get suspicious, and we inspect the tag. We can validate the tag against our existing regular expressions that taginfo uses. If it's invalid, we can print a helpful error message on the spot and abort the commit, which will prevent the tag from getting implicitly created!
YAY, BABY!!!
Take that, confused drupal contrib repo users! ;)
This actually makes it worth while to clean up the existing mess, since I'll have much more hope that we can keep the house clean once we get to a sane state. Phew!
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | xcvs-validate-branch-on-commit.patch_2.txt | 4.01 KB | dww |
| #1 | xcvs-validate-branch-on-commit.patch.txt | 3.19 KB | dww |
Comments
Comment #1
dwwPretty sure this will do it. Any objections or comments? Also, the error message for the copy on cvs.d.o should probably include a link to a handbook page somewhere about this problem, and what maintainers should do if they trigger the error. But, I just want to make sure this is really working and bullet proof first, then I'll worry about polishing up the rough edges with fancy error reporting, etc.
Also, I'm *pretty* sure we'll always get a CVS/Tag file if there's a sticky tag during a commit, but I suppose it might be even more fool proof to inspect CVS/Entries and check every line in there for sticky tags and validate all of them. Anyone happen to have thoughts on if that's overkill or worth doing? We could do both, I suppose, to be extra-paranoid.
Comment #2
dwwComment #3
dwwBetter patch that also parses and validates everything in the CVS/Entries file. This saves the day if a given directory happens to have multiple files with different sticky tags (in the Drupal contrib repo, you can never be too safe).
I tested a bunch of permutations with single commits spanning multiple sticky tags, in different subdirectories, etc. The deal is that commitinfo gets invoked separately in every directory or subdirectory of the commit operation. So, regardless of where the user is when they run commit, xcvs-commitinfo.php can always just test the local CVS directory for these two files, and doesn't itself have to worry about recursing through the directory tree. However, if a single directory has multiple sticky tags, the only way we can catch it is via the logic in this patch which also inspects CVS/Entries.
I haven't actually tested this on d.o yet, but that's the next step. Pretty sure it should all work flawlessly, but I've gotten used to things *never* working like I expect once I move from a local test site to d.o... So, any able-minded reviewers who care to look, please do. Otherwise, I'll just plan to roll this out when I'm in IRC and have a small team of enthusiasts ready to test things out and make sure valid stuff is still allowed, and invalid stuff is successfully thwarted.
Cheers,
-Derek
Comment #4
killes@www.drop.org commented+ if (file_exists('CVS/Entries')) {
+ $entries = file('CVS/Entries');
This looks wrong, shouldn't that be file_get_contents?
Comment #5
chx commenteddunno, file is a nice and valid php command, gives you an array, one line, one element . often we do not like it because file_get_contents is faster (mmap i believe) and also because file has the line terminating char for each line which almost ended up with trimming... but still -- it's valid.
Comment #6
dwwyeah, I need to iterate over each line anyway, so it seemed easier to just read it in as an array, instead of getting the whole contents as a single string, and then having to split it on newlines or something...
Comment #7
dwwhttp://drupal.org/node/198469 is an initial draft of the handbook page about this. we can include a pointer to it in the error message this patch generates (specific t the cvs.d.o configuration, i wouldn't want to put it into the code in xcvs itself). comments/edits/suggestions welcome.
Comment #8
dwwWoot! Now committed and deployed on cvs.d.o:
And yet, soon thereafter:
Resulting in http://drupal.org/cvs?commit=90473 ;)
Comment #9
(not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #10
jpetso commentedPorted to Version Control API too, see issue #207402: Prevent creating branches through the backdoor.