Was https://ccvs.cvshome.org/issues/show_bug.cgi?id=157:

We have a big problem with people accidentally
running the "import" command (unfortunately,
WinCVS puts "checkout" and "import" right next to
each other in its menu, and people end up
clicking the wrong menu item from time to time.

What I've done locally is to modify the import
command to look for a file
named "CVSROOT/importallow" and verify that the
current user is in this list (effectively
restricting import permissions to a few dozen
users).  If the "importallow" file is not
present, then anybody is presumed to be allowed
to perform imports.

I'd like to submit this patch for inclusion in
mainline CVS.  Does this sound like a useful
inclusion?



------- Additional comments from Joshua Davies Thu Feb 19 10:06:17 -0700 2004 -------

Created an attachment (id=67)
"importallow" patch



------- Additional comments from Joshua Davies Thu Feb 19 10:07:28 -0700 2004 -------

Sorry it took so long to include this patch; I ran into some
technical difficulties.  This was tested against source release
1.12.5.  This patch has been running in our site for over six months
now and has helped us tremendously.



------- Additional comments from Derek Price Mon Mar 15 16:44:58 -0700 2004 -------

I looked over this patch and have several comments:

1.  It needs src/sanity.sh test cases and documentation in
doc/cvs.texinfo.  Please read the HACKING file in the top level of the
CVS source distribution for more if you don't understand this.

2.  I think the comparison of the current user name to the readers,
writers, or importallowed file should be consolidated into a single
function that accepts the file the current username is being
authenticated against as an argument.  This is way too much
duplication of code.

3.  The default comment in the importallowed file created by
mkmodules.c should be more complete.  Plese see the comments in the
readers, writers, and *info file for examples.

4.  How about renaming the file "importers" for consistency with the
"readers" and "writers" files?

5.  The "not authorized to use this command" error message needs at
least a comment in the code about it being easy to miss the "checkout"
menu option and hit "import" with WinCVS.  In the context of a command
line client this error message is much too cryptic.

I am also going to send a message off to -email is unavailable- in hopes of
starting a more general discussion about this patch.  If you can
update your patch with regards to the above criticisms and any further
valid criticisms and the bug-cvs discussion turns out ok, then I will
commit this patch.



------- Additional comments from Derek Price Mon Mar 15 16:45:40 -0700 2004 -------

Incidentally, thanks for the patch submission!



------- Additional comments from Derek Price Mon Mar 15 16:48:25 -0700 2004 -------

General questions this patch raises for me:

1.  This makes me itch for a more general permissions scheme, but
nothing comes instantly to mind.  My current inclination is to allow
this patch (assuming my earlier criticisms are answered), but I would
love to hear discussion of a more general solution first.



------- Additional comments from Derek Price Mon Mar 15 16:52:33 -0700 2004 -------

Adding myself to the CC list.



------- Additional comments from Derek Price Mon Mar 15 18:01:47 -0700 2004 -------

Mark D. Baushke <mdb@cvshome.org> writes:

I would rather see a CVSROOT/importinfo file with script that get run to
determine if the files being imported meet with the cvs administrator
critera for new imports.

Such a script could use something like cvs_acls to determine that a
given user was not allowed to do the import as well as determining that
the files being imported were not acceptable for other criteria.

One of the biggest things I'd like to see such a script be able to
handle is to disallow vendor and version tags that do not match the
criteria provided by the administrator. Right now taginfo is fairly
useless if someone can easily wipe-out an existing tag thru use of
a 'cvs import'...

Having the special-purpose CVSROOT/importers seems less flexible to me.

For that matter, I think it might be useful to have trigger scripts for
'cvs checkout' and 'cvs update' to allow a cvs administrator to provide
a finer grained policy on what users may do with files in a repository.

-- Mark



------- Additional comments from Derek Price Mon Mar 15 18:05:05 -0700 2004 -------

This is exactly the sort of thing I was fishing for.  An importinfo
file should be fairly easy to implement now with the new
format_cmdline() hooks.



------- Additional comments from Ingolf Steinbach Tue Mar 16 05:16:02 -0700 2004 -------

Derek wrote on 2004-03-15:

> 5.  The "not authorized to use this command" error message needs at
> least a comment in the code about it being easy to miss the "checkout"
> menu option and hit "import" with WinCVS.  In the context of a command
> line client this error message is much too cryptic.

 
I am not sure if comments in the cvs code should deal with mis-usage
and/or pitfalls with certain versions of certain external tools.
IMO, an improved error message like "you are not authorized to use the
'import' command" (again without a reference to WinCVS or something like
that) would be better.
 
Kind regards
    Ingolf



------- Additional comments from Derek Price Tue Mar 16 08:39:26 -0700 2004 -------

Larry Jones <lawrence.jones@ugsplm.com> writes:

>Mark D. Baushke writes:
>
>>>
>>> I would rather see a CVSROOT/importinfo file with script that get

run to

>>> determine if the files being imported meet with the cvs administrator
>>> critera for new imports.
>
>I'll second that, with the caveat that I would still prefer to extend
>commitinfo rather than creating a new administrative file.

------- Additional comments from Derek Price Tue Mar 16 08:41:50 -0700 2004 -------

Extending commitinfo should be even easier to add with the new API.  A
single new field type containing the commit type would need to be
added, and the current commitinfo call placed in a function accessible
to both commit.c & import.c.



------- Additional comments from Derek Price Tue Mar 16 09:14:21 -0700 2004 -------

Mark D. Baushke Writes:

Extending the commitinfo would be acceptable to me provided that access
to both the vendor and version tags to be used for the import were also
somehow made available to make the decision to allow the import to
happen or not.



------- Additional comments from Derek Price Tue Mar 16 09:31:22 -0700 2004 -------

Good point again, Mark.  It should be useful and easy enough to extend
the current commitinfo hook to include a format string for the
destination branch name anyhow, and this could be reused for import's
vendor branch.  Even including the old revision for each file would be
easy.  Destination revision would be harder - it currently isn't set
for use by loginfo until it is retrieved after the actual commit.

As for the list of release tags (don't forget it is a list), this
could be done, but would be a little more compilcated.  Perhaps if it
were simply left empty for calls to commit and filled in for calls to
import, that would be sufficient.  Using some characters forbidden in
tags, users could specify something like the following so that their
scripts could handle the double lists:

ALL $CVSROOT/CVSROOT/myscript %r %p $T %{t} -- %{sV}



------- Additional comments from Mark D. Baushke Tue Mar 16 09:45:39 -0700 2004 -------

Derek Robert Price <derek@ximbiot.com> writes:

> Good point again, Mark. It should be useful and easy
> enough to extend the current commitinfo hook to include a
> format string for the destination branch name anyhow, and
> this could be reused for import's vendor branch. Even
> including the old revision for each file would be easy.
> Destination revision would be harder - it currently isn't
> set for use by loginfo until it is retrieved after the
> actual commit.

Agreed. The destination revision is a harder problem.
However, it might be useful to have another flag that could
be passed into the script to indicate that the operation is
either commit or import.

Would the commitinfo script be called once for the entire
tree as loginfo is called at present or once for each
directory in the import tree? This has implications for how
the contrib/commit_prep.in and contrib/log_accum examples
presently operate.

It might also be useful for the 'cvs add' and 'cvs import'
commands to share logic with 'cvs commit' as to validation
that the creation of a new directory in the repository.

> As for the list of release tags (don't forget it is a
> list), this could be done, but would be a little more
> compilcated. Perhaps if it were simply left empty for
> calls to commit and filled in for calls to import, that
> would be sufficient. Using some characters forbidden in
> tags, users could specify something like the following so
> that their scripts could handle the double lists:
>
> ALL $CVSROOT/CVSROOT/myscript %r %p $T %{t} -- %{sV}

I do not understand the $T in the above line. Where is it
set?

> I've annotated Issuezilla with our last few comments again.

For a 'cvs import' I would be under the impression that
there are two active tags in the operation: 1) the vendor
tag, 2) the version tag. It is still not clear to me if you
intend to integrate commitinfo with the taginfo operation by
calling the taginfo script during import or not...



------- Additional comments from Derek Price Tue Mar 16 10:17:51 -0700 2004 -------

> Would the commitinfo script be called once for the entire
> tree as loginfo is called at present or once for each
> directory in the import tree? This has implications for how
> the contrib/commit_prep.in and contrib/log_accum examples
> presently operate.

Good question.  I would have said that it should be called for each
directory if you hadn't mentioned that loginfo wasn't being called
separately.  Perhaps it should be kept in sync?  As long as
commit_prep is called only once, I think log_accum will still work
properly.  I'm not sure I understand all the issues involved, but you
are probably right that a once-per-directory/once-per-commit
combination would break the examples.

> It might also be useful for the 'cvs add' and 'cvs import'
> commands to share logic with 'cvs commit' as to validation
> that the creation of a new directory in the repository.

Hrm.  Yes, probably, but beyond the scope of my voting for acceptance
of this patch, I think.

> >ALL $CVSROOT/CVSROOT/myscript %r %p $T %{t} -- %{sV}
>
> I do not understand the $T in the above line. Where is it
> set?

Er, it should have been %T, and I meant it to be the destination
branch/vendor branch.  Maybe %B would be better?  Anyhow, taginfo is
using %t for the name of the tag being created and %b as a
branch/non-branch flag, and I was trying to remain somewhat
consistent.  I thought %t/%T was somewhat consistent with the %{Vv}
being used for old revision number/new revision number model being
used for loginfo & taginfo.

And %T isn't being set yet.  I was just suggesting it.

> For a 'cvs import' I would be under the impression that
> there are two active tags in the operation: 1) the vendor
> tag, 2) the version tag.

Actually, there can be more than one release tag (what you are calling
a vesion tag):

Usage: cvs import [-d] [-k subst] [-I ign] [-m msg] [-b branch]
    [-W spec] repository vendor-tag release-tags...

I don't know how often people use that functionality, but as long as
it is enabled, it would need to be accounted for.

> It is still not clear to me if you
> intend to integrate commitinfo with the taginfo operation by
> calling the taginfo script during import or not...

Ouch.  I hadn't thought of that.  It's a thought, but I was still
thinking of leaving that to the commitinfo script...  if a user really
wanted, it shouldn't be too hard to call their own taginfo script from
their commitinfo script on an import, if that's what they wanted.  I
think I will vote for leaving taginfo out of the import until it looks
like it is really necessary, or at least useful, to separate the
functionality.



------- Additional comments from Derek Price Tue Mar 16 14:05:56 -0700 2004 -------

Larry Jones writes:

>Ouch.  I hadn't thought of that.  It's a thought, but I was still
>> thinking of leaving that to the commitinfo script...  if a user really
>> wanted, it shouldn't be too hard to call their own taginfo script from
>> their commitinfo script on an import.  I think I would vote for leaving
>> taginfo out of the import until it looks like it is really

necessary, or

>> at least useful, to separate the functionality.

It seems to me that import is basically a shorthand for commit and tag
operations and thus should use the existing mechanisms.  That way users
don't have to put the same stuff in two different places (which always
leads to synchronization problems).



------- Additional comments from Joshua Davies Thu Mar 18 10:14:25 -0700 2004 -------

Wow - I've sparked quite a debate, here.  I'd be more than happy to
apply the five changes suggested by Derek Price on 3/15, but it looks
like the consensus is that a new "importallow" script isn't the most
flexible way to handle this issue.  It seems like the consensus is
that import should invoke "commitinfo" - I'll have to do some
research to understand how the "commitinfo" script is invoked to see
how it can be extended.

Minor comment from an FNG, though - if, as Mark Baushke suggests,
commitinfo should be invoked as part of add, commit & import...
should the name of the file be changed?  (With a "new" file name
searched for and "commitinfo" called as a fallback for migration -
sort of like the XF86Config-4 migration)?  Just a thought.



------- Additional comments from Derek Price Thu Mar 18 11:00:30 -0700 2004 -------

I wouldn't think a filename change is necessary, per se.  Add of a
directory and import are both performing commits, basically.

Neither would I consider the name change a big deal if you want to
change it.  I suppose it might be a trifle less confusing to newbies
to see a more general name not so directly associated with commit.



------- Additional comments from Joshua Davies Sat Mar 27 09:40:57 -0700 2004 -------

Ok, I've made some progress on this - and dug much deeper into the CVS
source code than I ever thought I would.  One observation I wouldn't
mind getting some feedback on - if the "import" command invokes the
"commitinfo" script, obviously none of the modules will ever match, so
if you want to control import behavior, you'd need to add a script
under DEFAULT or ALL.

Would it make sense to add another keyword, IMPORT, to be invoked on
import?  So you're commitinfo script might look like this:

module1  $CVSROOT/CVSROOT/sanity-check.sh %p %r %{s}
DEFAULT  $CVSROOT/CVSROOT/verify.sh %p %r %{s}
IMPORT   $CVSROOT/CVSROOT/check-user.sh %r %p $T %{t} -- %{sV}
ALL      xxx



------- Additional comments from Derek Price Sat Mar 27 11:36:09 -0700 2004 -------

Do you have a good reason for wanting to differentiate the import from
the creation of other directories and files?  I can't think of a good
reason.

If there is a good reason, what advantage does the IMPORT flag have
over passing a string that resolves to import/checkout/add into the
called script as an argument if requested by the user?  I think I see
disadvantages given that putting this functionality into commitinfo
was a way to consolidate the new functionality where it made sense.



------- Additional comments from Joshua Davies Sun Mar 28 08:21:06 -0700 2004 -------

Well, remember that my original use-case was to restrict the
import command entirely - so I'm going to end up writing a "DEFAULT"
script like this:

#!/bin/sh

if [ "$1" == "import" ]
then
# restrict access
else
# do some style-checking and strip off dos-style newlines, etc.
fi

Let me do this - I'm almost done with the change to make the import
command invoke the commitinfo scriptset (without the special "IMPORT"
keyword); I'll submit another patch, let you look at it, and if you
think I should add the IMPORT keyword to the commitinfo script, I'll
do so and submit another patch.



------- Additional comments from Derek Price Sun Mar 28 09:47:15 -0700 2004 -------

Sounds good.



------- Additional comments from Derek Price Mon Jun 7 06:20:52 -0700 2004 -------

* Issue 184 has been marked as a duplicate of this issue. *



------- Additional comments from Joshua Davies Wed Jul 28 13:10:32 -0700 2004 -------

In case you're curious, I haven't given up on this... it's just a lot trickier
than I ever really anticipated.  I've finally gotten some time to come back and
look at this, and my original use case (trigger the "commitinfo" script as part
of each "import_descend" action and allow the commitinfo default script to
accept or reject an import) is working fine.  Of course, CVS has gone through 3
minor revisions while I was messing around with it ;), but I'm current with
1.12.9 at the moment.

Adding the branch & version info is a bit trickier, though, and this seems to
be what issue #184 wants (we've ad-hoc'ed this at our site, so this would
probably be beneficial to us as well).  I'm not entirely sure how to tackle
this, since the branch name could vary from file to file (for all practical
purposes, it never does, but CVS does allows this), so it seems like each
file in the %{s} list on comminfo should be followed by the branch name, the
old revision, and the new revision (so that the new format string would be %
{sbvV} or something like that).