Codestriker: collaborative code reviewer

David,

I'm wondering if there might actually be a way to accomplish creating a
topic with already committed code using the tag and module fields and
cvsweb specified repository. I have a suggestion, but it's kind of a
hack maybe. :)

I'm using cvsweb. When I choose to do a unidiff between two revisions,
the URL looks like the following:

http://cvs.hostname.com/cgi-bin/cvsweb.cgi/modulename/filename.diff?r1=3Dt
ext&tr1=3D1.1&r2=3Dtext&tr2=3D1.2&f=3Du

The contents looks like the following:

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
RCS file: /cvs/modulename/filename,v
retrieving revision 1.1
retrieving revision 1.2
diff -u -r1.1 -r1.2
--- modulename/filename	2004/01/07 16:30:32	1.1
+++ modulename/filename	2004/01/07 16:33:39	1.2
@@ -1,5 +1,7 @@
=20<root_node>
=20    <node custom1=3D"something" custom2=3D"something"/>
-    <node custom1=3D"somethingelse1" custom2=3D"somethingelse1"/>
+    <node custom1=3D"somethingelse2" custom2=3D"somethingelse2"/>
+    <node custom1=3D"somethingelse3" custom2=3D"somethingelse3"/>
+    <node custom1=3D"somethingelse4" custom2=3D"somethingelse4"/>
=20    <node custom1=3D"finished" custom2=3D"final"/>
=20</root_node>

I've learned that if you copy this content into an editor and save a
file out, then use that saved out file as the Topic text upload, it
doesn't work. If you add:

Index:filename

before the

=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D

it will then work.

So, to summarize, all you would need to add would be to dynamically
create the URL:

http://cvs.hostname.com/cgi-bin/cvsweb.cgi/modulename/filename.diff?r1=3Dt
ext&tr1=3D1.1&r2=3Dtext&tr2=3D1.2&f=3Du

(the only real new parts would be
"filename.diff?r1=3Dtext&tr1=3D1.1&r2=3Dtext&tr2=3D1.2&f=3Du")

request the URL, and insert "Index:filename" as the first line of the
returned contents, then use this newly created content as the uploaded
file in the perl logic.

(BTW, I have no experience in Perl. This is only a naive notion that I'm
considering.)

The reason I suggest this is two-fold:

We are shying away from having to expose a default/anonymous type user
that would need to be defined in the CodeStriker repository list in the
codestriker.conf file.

I'm (myself) wondering if this might actually be possible/feasible to
do. :)

What do you think?
Question?

Thanks,

Matthew

> -----Original Message-----
> From: David Sitsky [mailto:sits@...
> Sent: Saturday, January 03, 2004 10:05 PM
> To: codestriker-user@...
> Cc: Matthew Hailstone; Jason Remillard
> Subject: Re: [Codestriker-user] Re: problem with CVS web integration
>=20
>=20
> Hi,
>=20
> Now that I am back from holidays, I'll try and answer any=20
> outstanding questions on the list, although it looks like=20
> Jason covered just about everything.
>=20
> As Jason mentioned below, the usual way to create new code=20
> review topics is to create a new file with "cvs diff -u" and=20
> to upload it.
>=20
> Some people requested the ability to code review already=20
> committed code from a CVS repository, by providing two tags=20
> and a CVS module name (or a single filename).  Only certain=20
> repositories have the ability to retrieve this diff=20
> information.  The viewcvs and cvsweb repository types can't=20
> do this, but a local CVS or pserver CVS can.
>=20
> If you still need this ability, modify your config file to=20
> just include the :pserver repository type you specified, and=20
> it should work fine.
>=20
> I have also modified the code so a misconfigured repository=20
> value is reported in a more user-friendly manner.
>=20
> Cheers,
> David
>=20
> On Wednesday 19 November 2003 12:57, Jason Remillard wrote:
> > Hi,
> >
> > Tracking this error message down in the code, it seems
> > that the only repositories that support label based
> > diffs are the cvs local and cvs pserver. All of the
> > other ones return this message when doing a single
> > file topic that on a label. I believe that the
> > intended use of the www integration is to be able to
> > display the full file when needed from the view topic
> > page. The way that we use codestriker is to upload an
> > unified diff file. Usually, a bug fix or a new feature
> > will span more than one file, so you want to see all
> > of the local changes together in once place. Because
> > of this reason you really want to do a diff upload
> > anyway. I am not even sure why David put in the
> > ability to review just a single file with labels.
> >
> > cvs diff -u
> >
> > So it turns out that the sort answer is that you can't
> > do this, but I think the normal mode of uploading
> > diffs is more useful anyway. I would expect that an
> > uploaded diff file would work with cvsweb.
> >
> > Jason.
> >
> > --- codestriker-user-request@...
> >
> > wrote:
> > > Send Codestriker-user mailing list submissions to
> > > 	codestriker-user@...
> > >
> > > To subscribe or unsubscribe via the World Wide Web,
> > > visit
> >
> > https://lists.sourceforge.net/lists/listinfo/codestriker-user
> >
> > > or, via email, send a message with subject or body
> > > 'help' to
> > > 	codestriker-user-request@...
> > >
> > > You can reach the person managing the list at
> > > 	codestriker-user-admin@...
> > >
> > > When replying, please edit your Subject line so it
> > > is more specific
> > > than "Re: Contents of Codestriker-user digest..."
> > >
> > >
> > > Today's Topics:
> > >
> > >    1. RE: CvsWeb integration problem (Matthew
> > > Hailstone)
> > >
> > > --__--__--
> > >
> > > Message: 1
> > > From: Matthew Hailstone
> > > <matthew.hailstone@...>
> > > To: "codestriker-user@..."
> > > <codestriker-user@...>
> > > Subject: RE: [Codestriker-user] CvsWeb integration
> > > problem
> > > Date: Mon, 17 Nov 2003 10:28:18 -0700
> > >
> > > I modified the repository list and got the following
> > > message:
> > >
> > > Repository "http://cvs.host.org/cgi-bin/cvsweb.cgi
> > > /cvsroot" doesn't
> > > support topic text tag retrieval.
> > >
> > > Does this shed more light?
> > > The view cvs repository entry seems to be different
> > > than the cvsweb
> > > implementation. I haven't seen an example of a
> > > cvsweb entry in the list.
> > >
> > > Thanks for all the suggestions thus far. :)
> > >
> > > Matthew
> > >
> > > > -----Original Message-----
> > > > From: Jason Remillard
> > >
> > > [mailto:jremillardshop@...
> > >
> > > > Sent: Sunday, November 16, 2003 6:28 PM
> > > > To: codestriker-user@...
> > > > Subject: [Codestriker-user] CvsWeb integration
> > >
> > > problem
> > >
> > > > Importance: Low
> > > >=3D20
> > > >=3D20
> > > > Hi,
> > > >=3D20
> > > > I think your repository configuration file is not =20
> correct. If you=20
> > > >look at the code in=3D20 =20
> Codestriker::Repository::RepositoryFactory=20
> > > >you
> > >
> > > will see that=3D20
> > >
> > > > is is expecting a CVS root path at the end of the
> > >
> > > config options.
> > >
> > > >=3D20
> > > > Here is the regex that is looking for the
> > >
> > > repository.
> > >
> > > >=3D20
> > > >     } elsif ($repository =3D3D~
> > >
> > > /^\s*(https?:\/\/.*cvsweb\.cgi)\/*\s+(.*?)\/*\s*$/i)
> > > {
> > >
> > > >=3D20
> > > > Note, the second set of () after it gets the
> > >
> > > cvsweb
> > >
> > > > part. Also, the example from viewcvs has a
> > >
> > > /cvsroot at
> > >
> > > > the end. It has the same regex as cvsweb, so it
> > >
> > > has
> > >
> > > > the same configuration.=3D20
> > > >=3D20
> > >
> > > =3D20
> > > 'http://cvs.sourceforge.net/cgi-bin/viewcvs.cgi
> > > /cvsroot',
> > >
> > > I would not worry about the other error messages,
> > > until you fix the [error] Codestriker: Couldn't
> > > match
> > > repository: in your apache log.=3D20
> > >
> > > Jason
> > >
> > > __________________________________
> > > Do you Yahoo!?
> > > Protect your identity with Yahoo! Mail AddressGuard=20
> > > http://antispam.yahoo.com/whatsnewfree
> >
> > -------------------------------------------------------
> >
> > > This SF. Net email is sponsored by: GoToMyPC
> > > GoToMyPC is the fast, easy and secure way to access
> > > your computer from
> > > any Web browser or wireless device. Click here to
> > > Try it Free!
> >
> >=20
> https://www.gotomypc.com/tr/OSDN/AW/Q4_2003/t/g22lp?Target=3D3Dmm/g22lp.
> > tmpl
> >
> > > _______________________________________________
> > > Codestriker-user mailing list=20
> Codestriker-user@...
> >
> > https://lists.sourceforge.net/lists/listinfo/codestriker-user
> >
> > > *************************************
> > >
> > > This e-mail may contain privileged or confidential
> > > material intended for =3D
> > > the named recipient only.
> > > If you are not the named recipient, delete this
> > > message and all =3D
> > > attachments.
> > > Unauthorized reviewing, copying, printing,
> > > disclosing, or otherwise using =3D
> > > information in this e-mail is prohibited.
> > > We reserve the right to monitor e-mail sent through
> > > our network.=3D20
> > >
> > > *************************************
> > >
> > >
> > >
> > > --__--__--
> > >
> > > _______________________________________________
> > > Codestriker-user mailing list=20
> Codestriker-user@...
> >
> > https://lists.sourceforge.net/lists/listinfo/codestriker-user
> >
> > > End of Codestriker-user Digest
> >
> > __________________________________
> > Do you Yahoo!?
> > Protect your identity with Yahoo! Mail AddressGuard=20
> > http://antispam.yahoo.com/whatsnewfree
> >
> >
> > -------------------------------------------------------
> > This SF.net email is sponsored by: SF.net Giveback Program. Does=20
> > SourceForge.net help you be more productive?  Does it help=20
> you create=20
> > better code?  SHARE THE LOVE, and help us help YOU!  Click Here:=20
> > http://sourceforge.net/donate/=20
> > _______________________________________________
> > Codestriker-user mailing list Codestriker-user@...
> > https://lists.sourceforge.net/lists/listinfo/codestriker-user
>=20

*************************************

This e-mail may contain privileged or confidential material intended for =
the named recipient only.
If you are not the named recipient, delete this message and all =
attachments.
Unauthorized reviewing, copying, printing, disclosing, or otherwise using =
information in this e-mail is prohibited.
We reserve the right to monitor e-mail sent through our network.=20

*************************************

Hi Matthew,

> I'm wondering if there might actually be a way to accomplish creating a
> topic with already committed code using the tag and module fields and
> cvsweb specified repository. I have a suggestion, but it's kind of a
> hack maybe. :)

Its curious that both cvsweb and viewcvs don't include the Index: line.  
Anyway, no big deal there.

The real problem with using cvsweb and viewcvs, is there is now way to 
obtain a diff given two tags and a module name.  It only allows you to 
retrieve diffs between two tags and only a _filename_.  

I would have thought for code reviewing, a single file at a time would have 
been too tedious and limited.

> The reason I suggest this is two-fold:
>
> We are shying away from having to expose a default/anonymous type user
> that would need to be defined in the CodeStriker repository list in the
> codestriker.conf file.

This is the real issue we need to discuss.  When you think about it, your 
cvsweb installation is not really that different - it is accessing your 
CVS repository in an anonymous fashion as well.

In fact, the last time I looked, both cvsweb and viewcvs had to be 
installed on the same machine as the CVS server.  Is this the case on your 
site?  If so, why not install Codestriker on the same machine as well?  
That way, you can access the repository directly without having to use 
pserver at all, but directly accessing the filesystem.

If not, and you set your security correctly, you should be able to make 
codestriker.conf readable only to the web browser userid, so nobody else 
can see its contents.

> I'm (myself) wondering if this might actually be possible/feasible to
> do. :)

What you have suggested is certainly possible, but I would have thought not 
that useful given the reasons stated above.

-- 
Cheers,
David

David,

> The real problem with using cvsweb and viewcvs, is there is=20
> no way to=20
> obtain a diff given two tags and a module name.  It only=20
> allows you to=20
> retrieve diffs between two tags and only a _filename_. =20
>=20
> I would have thought for code reviewing, a single file at a=20
> time would have=20
> been too tedious and limited.

ok. I think I am missing a _big_ feature here. I thought a filename was
necessary. How would I create a topic for a module or directory?

> This is the real issue we need to discuss.  When you think=20
> about it, your=20
> cvsweb installation is not really that different - it is=20
> accessing your=20
> CVS repository in an anonymous fashion as well.
>=20
> In fact, the last time I looked, both cvsweb and viewcvs had to be=20
> installed on the same machine as the CVS server.  Is this the=20
> case on your=20
> site?

yes.

> If so, why not install Codestriker on the same machine=20
> as well? =20
> That way, you can access the repository directly without=20
> having to use=20
> pserver at all, but directly accessing the filesystem.

Good suggestion. :)
Wouldn't the entry in the @valid_repositories list show up in the
Repository drop-down list when creating a new topic and expose the
directory structure of where CVS is?

Can there be an alias for the @valid_repositories entries?

Is this what the $file_viewer list is for?

> If not, and you set your security correctly, you should be=20
> able to make=20
> codestriker.conf readable only to the web browser userid, so=20
> nobody else=20
> can see its contents.
>=20

Thanks. :)

Matthew

*************************************

This e-mail may contain privileged or confidential material intended for =
the named recipient only.
If you are not the named recipient, delete this message and all =
attachments.
Unauthorized reviewing, copying, printing, disclosing, or otherwise using =
information in this e-mail is prohibited.
We reserve the right to monitor e-mail sent through our network.=20

*************************************

Hi Matthew,

> > I would have thought for code reviewing, a single file at a
> > time would have
> > been too tedious and limited.
>
> ok. I think I am missing a _big_ feature here. I thought a filename was
> necessary. How would I create a topic for a module or directory?

Lets assume you have a CVS module called "proj".  Doing a "cvs co proj" 
produces something like the following:

proj/dir1/file1.c
proj/dir2/file2.c
proj/dir2/file3.c
proj/dir3/file4.c

A developer then modifies code in file2.c, file3.c and file4.c say.   To 
get all of the differences for the prj module, the developer simply needs 
to:

cd proj
cvs diff -u > diff.txt

diff.txt can then be fed straight into Codestriker in the create topic 
page.  It contains the source code modifications made by the developer for 
the _entire_ proj module.

This is the usual way to work with Codestriker.

> > If so, why not install Codestriker on the same machine
> > as well?
> > That way, you can access the repository directly without
> > having to use
> > pserver at all, but directly accessing the filesystem.
>
> Good suggestion. :)
> Wouldn't the entry in the @valid_repositories list show up in the
> Repository drop-down list when creating a new topic and expose the
> directory structure of where CVS is?

In this situation, you should set @valid_repositories to have just a single 
entry, the cvsroot of your repository.  For example, if the CVS repository 
is located at /var/lib/cvs, simply put '/var/lib/cvs' as an entry for 
@valid_repositories.

The drop-down list doesn't expose the directory structure of CVS at all.

> Is this what the $file_viewer list is for?

file_viewer is used so that when displaying a topic, each filename being 
modified has a link to the cvsweb/viewcvs revision history.

-- 
Cheers,
David