Re: lockinfo

[Mark D. Baushke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Kendy Kutzner <kutzner@ira.uka.de> writes:

> On 2005-05-06T09:09:37-0700, Mark D. Baushke wrote:
> > To be considered for inclusion in a CVS FEATURE release, you should
> > provide changes to the doc/cvs.texinfo
> 
> attached

Comments included inline below...

> > as well as at least a few test
> > cases (if you don't understand how to work with sanity.sh that is okay,
> > but we need a series of step-by-step commands that illustrate how to
> > verify that everything works with your new feature and how the new
> > feature interacts with existing features).
> 
> I didn't get everything from sanity.sh, but here are some ideas for test
> cases:
> 
> - empty/nonexistent lockinfo: everything should work as ever
> - layout $CVSROOT/a, $CVSROOT/a/b, $CVSROOT/c
> - lockinfo contains
>       ^a false
>       ^a/b true
> - checkout a should fail
> - checkout b should work (i'm not too sure)

I believe CVS processes directories alphabetically depth-first. So, in
this case, b would also fail unless you explicitly did a

     cvs checkout a/b

> - checkout c should work
> 
> > For this particular addition, I have a problem with how it works.
> > 
> > Assumption: I have a repository where there exists one directory called
> > Private that is 'read-locked' such that userA is able to checkout
> > directory Private and userB is not.
> > 
> > If userB starts a 'cvs checkout -d top .' command. What should you
> > expect to happen:
> > 
> >   a) no files are checked out because Private will fail and that
> >      'should' stop the checkout command from providing partial
> >      information.
> > 
> >   b) all directories that are processed up to, but not including the
> >      Private directory are checked out. However, any files that
> >      would logically have been checked out of directories after
> >      "Private" are not checked out because the directory traversal
> >      will stop when "Private" is first reached.
> > 
> >   c) all directories other than "Private" will be checked out of the
> >      repository and the user will receive a "Lock Info failed" message
> >      that they may or may not notice indicating that the "Private"
> >      directory was not checked out.
> > 
> > With your current patch, I believe userB will observe behavior 'b',
> > but I am not sure if that is true or not.
> 
> As I see it, you are right, it should be 'b'.

Yeah, this is what I expected.

> > It is not clear to me if behavior 'a' or behavior 'c' is the correct
> > behavior, but I believe behavior 'b' is not desirable.
> 
> I see your worries. But i think that's the way CVS works:
> directory-by-directory. There is no commit-atomicity, there is no
> lock-atomicity. I think the behaviour is comparable to a scenario where
> userB does not have (file-system) write permissions to the directory.
> 
> I added the solicited changes to cvs.texinfo as well as new changes to
> src/mkmodules.c to create a default version of lockinfo.

In point of fact, the addition of this new feature may be a good time to
alter how cvs works. If you are explicitly controlling checkout
behavior, then you could choose to non-fatally skip directories for
which you do not have permissions... it would be as if they were really
private to some other set of users and not even visible to the user
implicitly trying to check them out. Although it may be desirable to
force an error if the user explictly asked for a 'read-locked' directory
on the command line instead of just running into one during the checkout
process.

Addition of this kind of 'read-lock' might be considered to be 'better'
than just use operating-system directory permissions.

I still remain unconvinced that the current implementation of your patch
is desirable.

> 
> Kendy
> 
> -- 
> 
> Only in cvs_my/: autom4te.cache
> diff -r -u cvs-1.12.12/doc/cvs.texinfo cvs_my/doc/cvs.texinfo
> --- cvs-1.12.12/doc/cvs.texinfo       2005-04-14 17:56:36.000000000 +0200
> +++ cvs_my/doc/cvs.texinfo    2005-05-09 17:45:41.070237832 +0200
> @@ -6569,6 +6569,16 @@
>  the change to @file{b/three.c} and not the change to
>  @file{a/two.c}.
>  
> +@noindent
> +Before the lock is created, the @file{loginfo} in CVSROOT is

The above line references `loginfo' instad of `lockinfo' which seems
wrong.

> +consulted. Every non-comment line in that file should
> +contain a directory pattern and a filter.  On the first
> +match between the current active directory and the pattern
> +the filter is executed with the directory as parameter. The
> +special pattern ALL matches always. When the exit code of
> +the filter is not zero, the lock creation failes. If the
> +exit code is zero, normal lock creation procedure continues.
> +

I suspect that some mention that this is for read-locks instead of
dealing with promotable or write locks... it also begs the question
as to the correct name for this file being 'readinfo' instead of
'lockinfo' ...

>  @node Watches
>  @section Mechanisms to track who is editing files
>  @cindex Watches
> Only in cvs_my/src: autom4te.cache
> diff -r -u cvs-1.12.12/src/cvs.h cvs_my/src/cvs.h
> --- cvs-1.12.12/src/cvs.h     2005-04-14 16:14:55.000000000 +0200
> +++ cvs_my/src/cvs.h  2005-05-09 17:46:39.380627969 +0200
> @@ -163,6 +163,7 @@
>  #define CVSROOTADM_CONFIG    "config"
>  #define      CVSROOTADM_HISTORY      "history"
>  #define      CVSROOTADM_IGNORE       "cvsignore"
> +#define CVSROOTADM_LOCKINFO  "lockinfo"
>  #define      CVSROOTADM_LOGINFO      "loginfo"
>  #define      CVSROOTADM_MODULES      "modules"
>  #define CVSROOTADM_NOTIFY    "notify"
> diff -r -u cvs-1.12.12/src/lock.c cvs_my/src/lock.c
> --- cvs-1.12.12/src/lock.c    2005-04-14 16:13:26.000000000 +0200
> +++ cvs_my/src/lock.c 2005-05-09 17:46:47.173076461 +0200
> @@ -476,7 +476,23 @@
>      }
>  }
>  
> +int
> +check_lock_info_proc(const char * rep, const char * filter, void * ud)
> +{
> +     const char * srepos = Short_Repository(rep);
> +     char * cmdline = Xasprintf("%s %s", filter, rep);
> +     run_setup (cmdline);
> +     free(cmdline);
> +     return(abs(run_exec(RUN_TTY, RUN_TTY, RUN_TTY, RUN_NORMAL | 
> RUN_SIGIGNORE)));
> +}
>  
> +int
> +check_lock_info(const char * xrep)
> +{
> +     int n;
> +     n = Parse_Info (CVSROOTADM_LOCKINFO, xrep, check_lock_info_proc, 
> PIOPT_ALL, NULL);
> +     return n;
> +}
>  
>  /*
>   * Create a lock file for readers
> @@ -489,6 +505,11 @@
>  
>      TRACE (TRACE_FUNCTION, "Reader_Lock(%s)", xrepository);
>  
> +    if (check_lock_info(xrepository)){
> +         error(0, 0, "Lock Info failed");
> +         return 1;
> +    }
> +
>      if (noexec || readonlyfs)
>       return 0;
>  
> diff -r -u cvs-1.12.12/src/mkmodules.c cvs_my/src/mkmodules.c
> --- cvs-1.12.12/src/mkmodules.c       2005-03-17 20:42:23.000000000 +0100
> +++ cvs_my/src/mkmodules.c    2005-05-09 17:50:17.167266049 +0200
> @@ -182,6 +182,25 @@
>      NULL
>  };
>  
> +static const char *const lockinfo_contents[] = {
> +    "# The \"lockinfo\" file is used to control read locks for 
> directories.\n",
> +    "# The filter on the right is invoked with the repository directory \n",
> +    "# to check.  A non-zero exit of the filter program will \n",
> +    "# cause the lock creation to be aborted.\n",
> +    "#\n",
> +    "# The first entry on a line is a regular expression which is tested\n",
> +    "# against the directory that the lock is created in, relative\n",
> +    "# to the $CVSROOT.  For the first match that is found, then the 
> remainder\n",
> +    "# of the line is the name of the filter to run.\n",
> +    "#\n",
> +    "# If the repository name does not match any of the regular expressions 
> in this\n",
> +    "# file, the \"DEFAULT\" line is used, if it is specified.\n",
> +    "#\n",
> +    "# If the name \"ALL\" appears as a regular expression it is always 
> used\n",
> +    "# in addition to the first matching regex or \"DEFAULT\".\n",
> +    NULL
> +};
> +
>  static const char *const taginfo_contents[] = {
>      "# The \"taginfo\" file is used to control pre-tag checks.\n",
>      "# The filter on the right is invoked with the following arguments\n",
> @@ -652,6 +671,10 @@
>      {CVSROOTADM_WRITERS,
>       "a %s file specifies read/write users",
>       NULL},
> +    {CVSROOTADM_LOCKINFO,
> +     "a %s file can be used to control the creation of read locks",
> +     lockinfo_contents},
> +     
>  
>      /* Some have suggested listing CVSROOTADM_PASSWD here too.  This
>         would mean that CVS commands which operate on the
> Only in cvs_my/: test

        -- Mark
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3 (FreeBSD)

iD8DBQFCf6sB3x41pRYZE/gRAvEYAJ9J+hIvtUZ+b0JSdqzFW8ezOBz7eQCeMNuI
6wYSvkyGrt47MKUhtRxsc1Q=
=feyg
-----END PGP SIGNATURE-----