Re: Patch: Add support for CVS_USER environment variable

[Mark D. Baushke]

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

M.E.O'Neill <oneill@cs.hmc.edu> writes:

> I wrote:
> >> If you don't want to accept the patch, it's not a huge loss to me.
> 
> ... and Derek Price <derek@ximbiot.com> replied:
> > Great!  Then you won't mind resubmitting your patch
> 
> Interesting logic.  I gave you my patches as-is, in the hope they
> would be useful.  I have zero interest in being a CVS developer.

And with no real understanding of how the patch you have provided is
intended to be use or may be tested, how is it useful to add your code
to the network release?

Your patch to subr.c tells cvs to read the CVS_USER environment variable
and do commits as that user instead of as the user LOGNAME. There are no
configuration changes for this even though the CVS_USER variable is
documented as being something that is part of the :pserver: protocol.

You patch did not contain any changes to documentation and the few lines
of description were somewhat ambiguous as to the intended application.

So, the patch seems more like a one-off hack than a general solution
that may have wider applicability to the general user populace.

I am not saying that it might not be useful on occasion to have such a
patch, just that the general applicability and behavior is not well
defined in the patch given it lacks documentation and test scripts to
verify correct performance.

> > so that it only works in pserver mode as you specified
> 
> No, I never said my patch was intended for cvs servers running in `cvs
> pserver' mode (which does authentication and knows what the usernames
> are supposed to be).  I said my patch was intended for cvs servers
> running in `cvs server' mode, which always assumes that the username
> it runs as is the correct username for commits.

And yet, the implication was that you were somehow running a
client/server cvs with the server side running as a set-uid process and
having some kind of a wrapper script setting CVS_USER in the environment
so that the author= section of the RCS delta would use the ${CVS_USER}
value.

A more 'common' approach would be to use :pserver: and have the mapping
of multiple external users to some local committer user done view the
existing cvs :pserver: authentication methods.

> Moreover, I am not convinced that the illusion of "security" provided
> by limiting this patch to that case is worth anything whatsoever.

Nor am I convinced that the patch itself is inherently useful without a
better understanding of how sites other than yours are to use this
patch. I'm not saying that someone else might not find it useful, just
that what has been written so far does not seem to be unambiguous in
how you are using it youself not to mention how you would expect other
folks to use it.

> Similarly, about my other patch (honoring -q/-Q), Derek wrote:
> > would you mind updating the sanity.sh test cases to take account of
> > your changes, possibly answer Mark's other concerns below, and
> > resubmit your patch?
> 
> Yes, I would mind.  I'd deleted my copy of the CVS sources before I
> sent my patches.  I'm done.

Fine. Thanks for the suggestion. It is a pity you didn't want to help
out more, but not everyone has the time to do such. So, thanks for the
idea. It will either be acted on or not as folks have time to do
something with it. If you want to make sure the idea does not die for
lack of immediate attention, feel free to open a change-request bug for
it.

If you wish to engage in the production of cvs patches that you find
useful and don't want to keep porting forward, it might be useful to get
the cvshome team to actually get your patches implemented sooner rather
than later.

Feel free to submit patches here on bug-cvs@gnu.org for peer review and
once consensus is reached and the change gets committed, it will show up
in some future release of cvs. The other path is to open a defect or
enhancement request and hope that some other volunteer has the time to
do the work you would appear to wish see gets done.

Just hacking the source is the 'easy' part of the problem. Making sure
to understand the impacts to documentation and testing is not as much
fun, but still needs to be done.

> > and with test cases and documentation, as requested in the HACKING
> > file in the top level of the CVS source distribution!
> 
> If I broke cvs hacking etiquette by just sending a patch, my
> apologies. 

No apology is needed. Your patch was a nice proof-of-concept. It is just
less than the full amount of work that needs to be done before it can be
integrated into the fairly mature product known as cvs.

> The message I'm getting here is that because I have neither the time
> nor inclination to act as a champion and maintainer for my patches, I
> shouldn't have bothered sending them in at all.

Hmmm... I am sorry that the message was somehow distorted and that you
feel so frustrated by the process.

I wouldn't say that you need to be a champion, but it would be 'easier'
to get your patches added into the code base if all of the needed
details were done up-front by you instead of possibly attempted, but not
fully understood by someone else along the line.

> Perhaps I should just submit bug reports and feature requests without
> any code?

If you wish to go with that approach, by all means do so. 
Such bugs do get worked on by folks as time permits.

If you wish to help to improve the cvs program you may feel free to
submit patches. You can submit patches to either the tracking system or
to the bug-cvs@gnu.org mailing list.

Patches with test cases (either already in the format needed for
src/sanity.sh are best, but a simple script that demonstrates proper
behavior is 'good enough' as we can convert to sanity.sh format) and
documentation updates are more likely to be quickly adopted and accepted
into the source base than are just hacks to the code.

Bug reports that show things like core dumps or incorrect behavior are
hopefully being handled quickly. Requests for enhancements sometimes
require discussion to understand the implications of the direction
implied by the change and to determine if it will cause current cvs
users to have significant problems with their next upgrade or not.

Of your two patch suggestions, I am not sure I understand why the
CVS_USER hack is desirable, but if others want it and it can be
documented and tested so that users of the cvs server are not surprised
by the new feature, then it might be adopted.

I can see more merit in the -q/-Q patch, but it did not seem complete in
that there are some other messages that should probably also be made
more quiet and this change will also require some changes to the
sanity.sh tests as well as to the documentation.

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

iD8DBQFASaOG3x41pRYZE/gRAn1WAJ45ntwR6wqvaw1neFr+RVhoT3PGvwCffmVQ
6nMng3SeuUVKrny0mp/FdW0=
=yIrX
-----END PGP SIGNATURE-----