[Cvs-dev] valid tag identifiers

[Mark D. Baushke]

Hi Folks,

Okay, I have a hack on top of Jim's latest patch which tightens up
error checking on valid tag identifiers which he credits to a report
by Alan Harder <address@hidden> that I apparently missed seeing.

The current state of the CVS sources is that they do not pass the 
'make check' sanity.sh tests.

Given that I know of multiple sites that converted from RCS to CVS, it
is not reasonable to tell them that all of their old tag names are no
longer available to them. So, I suggest that a checkout or update using
the old tags should still work. I am not as certain what to do if a
branch tag is being used and someone tries to add a new file to the
branch... I suspect that this current set of patches will disallow
adding the new files to the 'illegal' tag in that case. However, I have
not yet written any tests for that situation.

Below you will find where I have reverted the RCS_check_tag to the old
form and added Jim's version of the function as CVS_check_tag. I also
pulled in the reserved name checks from RCS_settag.

In all places that use a tag, RCS_check_tag is called. In all places
that might wish to create a new tag based on input from the mainline,
CVS_check_tag is called.

To be honest, I am still not sure I consider the restrictions to CVS
tags as something other than RCS tags to be wise. How many times are
folks using 8-bit characters out of the ISO 8859-1 or ISO 8859-5
character set rather than the "C" locale in tag names?

All of the characters with umlaut's and the like become illegal with
Jim's original change and I suspect this may be highly undesirable.

It may be desirable to change how the CVS_check_tag works to have it
based on a CVSROOT/config option to provide a subset of the characters
that are legal out of those that are currently not illegal.

Proposal 1: Revert Jim's latest changes completely back to the RCS tag
            name semantics.

Proposal 2: Adopt the set of hacks below on top of Jim's changes which
            tries to preserve the ability of a user to use existing tags
            in the repository, but only allow creation of new tags that
            match the CVS criteria.

Proposal 3: Add CVSROOT/config glue to specify the correct set of
            new tags.

I really don't have time to implement #3 and I have not looked closely
at the hack below for implications of other kinds of use.

Also note that I am not sure how well this will play with a mixture of
CVS and CVSNT clients and servers.

        -- Mark

This patch is still undergoing 'make check'

Index: ChangeLog
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/ChangeLog,v
retrieving revision 1.3457
diff -u -p -r1.3457 ChangeLog
--- ChangeLog   24 Jun 2006 03:05:10 -0000      1.3457
+++ ChangeLog   24 Jun 2006 22:35:32 -0000
@@ -1,3 +1,22 @@
+2006-06-24  Mark D. Baushke  <address@hidden>
+
+       * rcs.c (RCS_check_tag): Restore RCS tag name contstraints.
+       (CVS_check_tag): Impose CVS tag name constraints. Include checks
+       for TAG_BASE and TAG_HEAD illegal names here instead of in
+       RCS_settag.
+       (RCS_settag): Move TAG_BASE and TAG_HEAD checks to CVS_check_tag.
+       * rcs.h (CVS_check_tag): New prototype.
+       * admin.c (admin_fileproc): Use CVS_check_tag for new tag names.
+       * import.c (import): Ditto.
+       * tag.c (cvstag, tag_check_valid): Ditto.
+       * sanity.sh (basica): Add quotes around BASE in error message.
+       (tag-space): Use correct error message.
+       (tag-valid): Escape ^ character in tagname and use correct error
+       messages.
+       (admin-2-28-5.1): Prepare valid RCS old tag (but invalid CVS tag)
+       for use with -ntag:rev switch.
+       (admin-28-6): Use correct error message.
+
 2006-06-24  Jim Hyslop <address@hidden>
 
        * rcs.c: Tighten up error checking on valid tag identifiers.
Index: admin.c
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/admin.c,v
retrieving revision 1.117
diff -u -p -r1.117 admin.c
--- admin.c     13 Jun 2006 22:29:48 -0000      1.117
+++ admin.c     24 Jun 2006 22:35:32 -0000
@@ -1039,7 +1039,7 @@ admin_fileproc (void *callerdat, struct 
                if ((*p == 0 && (rev = RCS_head (rcs)))
                     || (rev = RCS_tag2rev (rcs, p))) /* tag2rev may exit */
                {
-                   RCS_check_tag (tag); /* exit if not a valid tag */
+                   CVS_check_tag (tag); /* exit if not a valid tag */
                    RCS_settag (rcs, tag, rev);
                    free (rev);
                }
Index: import.c
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/import.c,v
retrieving revision 1.178
diff -u -p -r1.178 import.c
--- import.c    4 May 2006 14:55:32 -0000       1.178
+++ import.c    24 Jun 2006 22:35:33 -0000
@@ -209,7 +209,7 @@ import (int argc, char **argv)
     {
        int j;
 
-       RCS_check_tag (argv[i]);
+       CVS_check_tag (argv[i]);
        for (j = 1; j < i; j++)
            if (strcmp (argv[j], argv[i]) == 0)
                error (1, 0, "tag `%s' was specified more than once", argv[i]);
Index: rcs.c
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/rcs.c,v
retrieving revision 1.374
diff -u -p -r1.374 rcs.c
--- rcs.c       24 Jun 2006 03:05:10 -0000      1.374
+++ rcs.c       24 Jun 2006 22:35:34 -0000
@@ -3373,12 +3373,45 @@ RCS_check_kflag (const char *arg)
 
 
 /*
+ * Do some consistency checks on the symbolic tag... These should equate
+ * pretty close to what RCS checks, though I don't know for certain.
+ */
+void
+RCS_check_tag (const char *tag)
+{
+    char *invalid = "$,.:;@";          /* invalid RCS tag characters */
+    const char *cp;
+
+    /*
+     * The first character must be an alphabetic letter. The remaining
+     * characters cannot be non-visible graphic characters, and must not be
+     * in the set of "invalid" RCS identifier characters.
+     */
+    if (isalpha ((unsigned char) *tag))
+    {
+       for (cp = tag; *cp; cp++)
+       {
+           if (!isgraph ((unsigned char) *cp))
+               error (1, 0, "tag `%s' has non-visible graphic characters",
+                      tag);
+           if (strchr (invalid, *cp))
+               error (1, 0, "tag `%s' must not contain the characters `%s'",
+                      tag, invalid);
+       }
+    }
+    else
+       error (1, 0, "tag `%s' must start with a letter", tag);
+}
+
+
+
+/*
  * Do some consistency checks on the symbolic tag... This is much stricter
  * to what RCS checks: we allow letters, underscores, dashes and numbers.
  * RCS allows a lot more characters.
  */
 void
-RCS_check_tag (const char *tag)
+CVS_check_tag (const char *tag)
 {
     const char *cp;
 
@@ -3387,13 +3420,16 @@ RCS_check_tag (const char *tag)
     {
        for (cp = tag; *cp; cp++)
        {
-           if (!isalnum ((unsigned char) *cp) && *cp != '_' && *cp != '-' )
+           if (!isalnum ((unsigned char) *cp) && *cp != '_' && *cp != '-')
                error (1, 0, "tag `%s' may contain only letters, numbers, "
                        "_ and -", tag);
        }
     }
     else
        error (1, 0, "tag `%s' must start with a letter", tag);
+
+    if (STREQ (tag, TAG_BASE) || STREQ (tag, TAG_HEAD))
+       error (1, 0, "Attempt to add reserved tag name `%s'", tag);
 }
 
 
@@ -6199,19 +6235,6 @@ RCS_settag (RCSNode *rcs, const char *ta
     if (rcs->flags & PARTIAL)
        RCS_reparsercsfile (rcs, NULL, NULL);
 
-    /* FIXME: This check should be moved to RCS_check_tag.  There is no
-       reason for it to be here.  */
-    if (STREQ (tag, TAG_BASE)
-       || STREQ (tag, TAG_HEAD))
-    {
-       /* Print the name of the tag might be considered redundant
-          with the caller, which also prints it.  Perhaps this helps
-          clarify why the tag name is considered reserved, I don't
-          know.  */
-       error (0, 0, "Attempt to add reserved tag name %s", tag);
-       return 1;
-    }
-
     /* A revision number of NULL means use the head or default branch.
        If rev is not NULL, it may be a symbolic tag or branch number;
        expand it to the correct numeric revision or branch head. */
Index: rcs.h
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/rcs.h,v
retrieving revision 1.84
diff -u -p -r1.84 rcs.h
--- rcs.h       24 Apr 2006 18:50:27 -0000      1.84
+++ rcs.h       24 Jun 2006 22:35:34 -0000
@@ -264,6 +264,7 @@ int RCS_datecmp (const char *date1, cons
 time_t RCS_getrevtime (RCSNode * rcs, const char *rev, char *date, int fudge);
 List *RCS_symbols (RCSNode *rcs);
 void RCS_check_tag (const char *tag);
+void CVS_check_tag (const char *tag);
 int RCS_valid_rev (const char *rev);
 List *RCS_getlocks (RCSNode *rcs);
 void freercsnode (RCSNode ** rnodep);
Index: sanity.sh
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/sanity.sh,v
retrieving revision 1.1158
diff -u -p -r1.1158 sanity.sh
--- sanity.sh   24 Jun 2006 03:05:10 -0000      1.1158
+++ sanity.sh   24 Jun 2006 22:35:40 -0000
@@ -3360,8 +3360,7 @@ ${SPROG} "'\[tag aborted\]: correct the 
 initial revision: 1\.1"
          dotest_fail basica-5a \
            "${testcvs} -q tag BASE sdir/ssdir/ssfile" \
-"${SPROG} tag: Attempt to add reserved tag name BASE
-${SPROG} \[tag aborted\]: failed to set tag BASE to revision 1\.1 in 
${CVSROOT_DIRNAME}/first-dir/sdir/ssdir/ssfile,v"
+"${SPROG} \[tag aborted\]: Attempt to add reserved tag name \`BASE'"
          dotest basica-5b "${testcvs} -q tag NOT_RESERVED" \
 'T sdir/ssdir/ssfile'
 
@@ -8777,7 +8776,7 @@ $SPROG add: use \`$SPROG commit' to add 
          dotest_fail tag-space-1 "$testcvs tag ' spacetag '" \
 "$SPROG \[tag aborted\]: tag \` spacetag ' must start with a letter"
          dotest_fail tag-space-2 "$testcvs tag 'spacetag '" \
-"$SPROG \[tag aborted\]: tag \`spacetag ' has non-visible graphic characters"
+"$SPROG \[tag aborted\]: tag \`spacetag ' may contain only letters, numbers, _ 
and -"
 
          if $remote; then
            # Verify that this isn't a client check.
@@ -8796,7 +8795,7 @@ tag
 EOF
 
            dotest tag-space-4 "$testcvs server" \
-"E $SPROG \[tag aborted\]: tag \`spacetag ' has non-visible graphic characters
+"E $SPROG \[tag aborted\]: tag \`spacetag ' may contain only letters, numbers, 
_ and -
 error  " <<EOF
 Root $CVSROOT_DIRNAME
 UseUnchanged
@@ -8948,7 +8947,7 @@ EOF
       dotest_fail tag-valid-15 "$testcvs -Q tag a%" \
 "$SPROG \[tag aborted\]: tag \`a%' may contain only letters, numbers, _ and -"
 
-      dotest_fail tag-valid-16 "$testcvs -Q tag a^" \
+      dotest_fail tag-valid-16 "$testcvs -Q tag a\^" \
 "$SPROG \[tag aborted\]: tag \`a^' may contain only letters, numbers, _ and -"
 
       dotest_fail tag-valid-17 "$testcvs -Q tag a\(" \
@@ -27680,8 +27679,11 @@ ${SPROG} \[admin aborted\]: revision .2\
 ${SPROG} \[admin aborted\]: tag .1\.a\.2. must start with a letter"
 
          # Confirm that a missing tag is not a fatal error.
-         dotest admin-28-5.1 "${testcvs} -Q tag BO+GUS file1" ''
-         dotest_fail admin-28-5.2 "${testcvs} admin -ntagten:BO+GUS file2 
file1"  \
+         dotest admin-28-5.1 "${testcvs} -Q tag BO-GUS file1" ''
+         sed s/BO-GUS/BO+GUS/ < $CVSROOT_DIRNAME/first-dir/file1,v > file1,v
+         chmod a=r file1,v
+         modify_repo mv -f file1,v $CVSROOT_DIRNAME/first-dir/file1,v
+         dotest_fail admin-28-5.2 "${testcvs} admin -ntagten:BO${PLUS}GUS 
file2 file1"  \
 "RCS file: ${CVSROOT_DIRNAME}/first-dir/file2,v
 ${SPROG} admin: ${CVSROOT_DIRNAME}/first-dir/file2,v: Symbolic name or 
revision BO${PLUS}GUS is undefined\.
 ${SPROG} admin: RCS file for .file2. not modified\.
@@ -27690,7 +27692,7 @@ done"
 
          dotest_fail admin-28-6 "${testcvs} admin -nq.werty:tagfour file2"  \
 "RCS file: ${CVSROOT_DIRNAME}/first-dir/file2,v
-${SPROG} \[admin aborted\]: tag .q\.werty. must not contain the characters ..*"
+${SPROG} \[admin aborted\]: tag .q\.werty. may contain only letters, numbers, 
_ and -"
 
          # Verify the archive
          #
Index: tag.c
===================================================================
RCS file: /cvsroot/cvs/ccvs/src/tag.c,v
retrieving revision 1.149
diff -u -p -r1.149 tag.c
--- tag.c       26 May 2006 19:25:57 -0000      1.149
+++ tag.c       24 Jun 2006 22:35:40 -0000
@@ -205,7 +205,7 @@ cvstag (int argc, char **argv)
        error (1, 0, "-d makes no sense with a date specification.");
     if (delete_flag && branch_mode)
        error (0, 0, "warning: -b ignored with -d options");
-    RCS_check_tag (symtag);
+    CVS_check_tag (symtag);
 
 #ifdef CLIENT_SUPPORT
     if (current_parsed_root->isremote)
@@ -1663,7 +1663,7 @@ Numeric tag %s invalid.  Numeric tags sh
     /* Verify that the tag is valid syntactically.  Some later code once made
      * assumptions about this.
      */
-    RCS_check_tag (name);
+    CVS_check_tag (name);
 
     if (is_in_val_tags (NULL, name)) return;