Item12139: Fix configuration checking of Regular Expressions, and add some missing checkers.

Been looking at all the regex checkers (candidates for fast recheck), found more issues.

Lumping them all here as they're all bugfixes - plus one port from otherwiki to support off-line tasks.

AccessibleENV - had no checker.

Cache/DependencyFilter - checker passed value instead of key name. Fixed, results in taint crash - CGI values aren't untainted coming in. Fixed that.

Email ValidTLD - no checker; also complied by Foswiki.pm early in init and crashes so can't run configure. Now reverts to default, but needs a logging mechanism that works that early, but still allows access to configure.

Register/EmailFilter - no checker

Also in Valuer: { should be \{ in regex in regex. Off-line tasks feature: flag to call string2value with query, which is needed for composite config items (e.g. off-line task schedules. Been in otherwiki for over a year. 1 of 2.

Committed to trunk, needs merge.

-- TimotheLitt - 11 Oct 2012

Looks like the MANIFEST change for this inadvertently waited until rev 15564. Sorry about that.

If I try to fix for this item, it'll only get messier. When merging, take both patchsets and it won't be an issue.

Should our famous commit gatekeeper script check for Added files and warn if there's no MANIFEST update? I know there are legitimate cases of adding to svn before committing to release, but "Added new file dir/foo.pm: Did you mean to update MANIFEST" might be a good thing... Or maybe a report of "files you committed to svn that aren't in any MANIFEST and aren't marked DONTSHIP"?

-- TimotheLitt - 11 Oct 2012

We already have this. It would have eventually been caught. Part of the build process is a check manifest.

-- GeorgeClark - 11 Oct 2012

Well, I must admit that we should at least fix the autobuild to email the error somewhere to foswiki-svn. Presently it seems that the MANIFST problems reported during build aren't sent.

-- PaulHarvey - 12 Oct 2012

I'm not sure how a later than check-in validator can work for modules (like config checkers) that are silently ignored if missing.

check-in time is when it's easiest to fix the MANIFEST without getting tangled in other changes, and when the developer has his/her intentions at the front of this mind.

-- TimotheLitt - 12 Oct 2012

There are quite a few support files that are not in the MANIFEST. And there is more than one MANIFEST. Sometimes files are created that are not ready to be distributed and are intentionally omitted from the MANIFEST.

See tools/check_manifest.pl. It performs several steps:

• Merges together the default extension and core manifests
• Verifies that all files listed in the merged MANIFEST are in the repo (git or svn)
• Checks that the file system permissions are consistent with the MANIFEST values
• Verifies that all files in the repo are in the MANIFEST

Running that check now, it shows a number of files, graphics psd and other source files we don't distribute, Work in progress files that we've chosen not to distribute yet, developer specific files that are only needed for a repo pseudo-install. And probably a few errors that we'll have to chase down. If we add check-in time gatekeeper, then we would also need to add an exceptions, for all the stuff that is not ready for the MANIFEST. I'd prefer not to have to deal with it. It's really not a big deal as RM to run the check prior to each build and deal with the issues.

-- GeorgeClark - 12 Oct 2012

Your call, of course. I do understand the issues, but I don't think it's hard to solve.

If it were me, I think I'd add a svn property (e.g. "notshipped") to such files.

Then a developer has to explicitly set "don't ship" prior to commit. The tools wouldn't have to guess, and RMs wouldn't have to track down commits in a manual review.

With such a flag, the rules are simple:

• For shipped files (the default)
  • Adding or deleting a file always has a MANIFEST in the checkin.
  • A checkin that adds a file without a MANIFEST that includes it, or one that deletes a file with a MANIFEST that still includes is a hard error.
• For nonshipped files: the converse.
  • No MANIFEST checked-in (by anyone) can include such a file.
• A MANIFEST commit must include exactly the files added or deleted.
• A MANIFEST commit with no files (e.g. to change a comment or permissions) can't add or delete files.

• For commits that just add or delete the property, apply the rule for the new state.
  • If it's added, some manifest has to be changed to delete the file.
  • If it's removed, a manifest has to be changed to add the file.

Finally, for shipped files, the commit script can add a shipitem property (e.g. shipitem=IpPlugin) that records which manifest should include the file. That provides some redundancy for the checker script to exploit, and makes it easy to ask svn "what ships this file?".

I am a long-time friend of release managers, so I tend to encourage automation that lets them not spend time on stuff that can be automated away. Those "not a big deal" things add up, and mistakes can happen.

But I'll stop beating this horse, as I don't have to do the work.

-- TimotheLitt - 12 Oct 2012

Back to the change: Untainting a ref doesn't work. (Oddly, didn't happen to me.) Sorry about that. Commiting a temporary patch to skip untainting in that case so others don't trip over it. Will be a second commit moving this earlier in the food chain.

Decided this is the wrong place to try to solve taint issues, removed the attempt that was one part of this patch. Back to needs merge.

-- TimotheLitt - 12 Oct 2012

Commenting is disabled while not logged in