Codestriker: collaborative code reviewer

> - Now that both tasks have been completed, both developers would like to
> have each of their individual work reviewed.

This is the problem.  Codestriker is designed to be a "pre-commit" step.  

The usual scenario with development teams is that unreviewed code is _not_ 
allowed to be committed into the CVS repository.  Once code has been 
reviewed, then the developer is free to commit it in.

The motivation for this is that a bad commit can impact the whole development 
team, resulting in potentially a lot of wasted time for bad commits.

In your scenario, there should be no commits by developer 1 or 2.  Once they 
have finished their "work", then they produce a "cvs diff" which is then 
reviewed in Codestriker.

There is no problem with both reviewers doing this in parallel - CVS is 
designed to handle this merges and so forth.  

The challenge is to define sensible units of work, which aren't too large to 
review, but software teams become very experienced at doing this very quickly.

Cheers,
David

> -----Original Message-----
> From: David Sitsky [mailto:sits@...]
> Sent: Tuesday, February 10, 2004 4:16 AM
> To: Matthew Hailstone; codestriker-user@...
> Subject: Re: [Codestriker-user] Feature request
>=20
> > - Now that both tasks have been completed, both developers would
like to
> > have each of their individual work reviewed.
>=20
> This is the problem.  Codestriker is designed to be a "pre-commit"
step.
>=20
> The usual scenario with development teams is that unreviewed code is
_not_
> allowed to be committed into the CVS repository.  Once code has been
> reviewed, then the developer is free to commit it in.
>=20
> The motivation for this is that a bad commit can impact the whole
> development
> team, resulting in potentially a lot of wasted time for bad commits.


Hi David,
	Just FYI, we don't do development this way, nor does any
development organization that I've been a part of for the last 20 years.
We do projects in branches, have the branches reviewed, do unit test,
THEN merge it to the main line of development.  We want our developers
to be able to do a commit at any time (and urge them to do it at least
daily) to prevent mishaps due to power outages, drive failure, etc.
This has paid off more than once!  The other issue arises when you're
working on a change that affects several different platforms (aix,
solaris and windows for instance), you need to be able to commit, so
that you can check the code out on the other machines to do unit
testing.

-Kelly

>=20
> In your scenario, there should be no commits by developer 1 or 2.
Once
> they
> have finished their "work", then they produce a "cvs diff" which is
then
> reviewed in Codestriker.
>=20
> There is no problem with both reviewers doing this in parallel - CVS
is
> designed to handle this merges and so forth.
>=20
> The challenge is to define sensible units of work, which aren't too
large
> to
> review, but software teams become very experienced at doing this very
> quickly.
>=20
> Cheers,
> David
>=20
>=20
> -------------------------------------------------------
> The SF.Net email is sponsored by EclipseCon 2004
> Premiere Conference on Open Tools Development and Integration
> See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
> http://www.eclipsecon.org/osdn
> _______________________________________________
> Codestriker-user mailing list
> Codestriker-user@...
> https://lists.sourceforge.net/lists/listinfo/codestriker-user

> Hi David,
> 	Just FYI, we don't do development this way, nor does any
> development organization that I've been a part of for the last 20 years.
> We do projects in branches, have the branches reviewed, do unit test,
> THEN merge it to the main line of development.  We want our developers
> to be able to do a commit at any time (and urge them to do it at least
> daily) to prevent mishaps due to power outages, drive failure, etc.
> This has paid off more than once!  The other issue arises when you're
> working on a change that affects several different platforms (aix,
> solaris and windows for instance), you need to be able to commit, so
> that you can check the code out on the other machines to do unit
> testing.

Hi Kelly,

I guess the key message here is that with your process, each developer has 
their own separate _private_ branch, so even if they do daily commits to 
their private branch, there is no impact to the rest of the development 
team.  With your process, there is still a way once the work has been 
completed, to generate a diff that can be reviewed, and then merged with 
the main branch.

With the process described by Matthew, unreviewed code from more than one 
developer is being committed into a branch/main trunk, which I think is a 
big problem.

The basic message is unreviewed code should never be committed to a 
"public" branch that other developers are using.

-- 
Cheers,
David

> -----Original Message-----
> From: David Sitsky [mailto:sits@...]
> Sent: Tuesday, February 10, 2004 3:22 PM
> To: Kelly F. Hickel; Matthew Hailstone; codestriker-
> user@...
> Subject: Re: [Codestriker-user] Feature request
>=20
> > Hi David,
> > 	Just FYI, we don't do development this way, nor does any
> > development organization that I've been a part of for the last 20
years.
> > We do projects in branches, have the branches reviewed, do unit
test,
> > THEN merge it to the main line of development.  We want our
developers
> > to be able to do a commit at any time (and urge them to do it at
least
> > daily) to prevent mishaps due to power outages, drive failure, etc.
> > This has paid off more than once!  The other issue arises when
you're
> > working on a change that affects several different platforms (aix,
> > solaris and windows for instance), you need to be able to commit, so
> > that you can check the code out on the other machines to do unit
> > testing.
>=20
> Hi Kelly,
>=20
> I guess the key message here is that with your process, each developer
has
> their own separate _private_ branch, so even if they do daily commits
to
> their private branch, there is no impact to the rest of the
development
> team.  With your process, there is still a way once the work has been
> completed, to generate a diff that can be reviewed, and then merged
with
> the main branch.
>=20
> With the process described by Matthew, unreviewed code from more than
one
> developer is being committed into a branch/main trunk, which I think
is a
> big problem.
>=20
> The basic message is unreviewed code should never be committed to a
> "public" branch that other developers are using.

David,
	While I (in theory) agree with your last statement, we do have
multiple developers working on projects (not always, but often), and so,
they do share a branch.  They are fully authorized to beat each other
with a wet noodle if one of them breaks the other's code (;>), but it
does happen.  We do the reviews at the end, and sometimes the reviews
cause changes, so we need to review it again, requiring a new
codestriker topic, instead of just updating the existing one.

-Kelly7

>=20
> --
> Cheers,
> David

David,

I understand your suggestion. I agree with you that on the whole, code
should be reviewed before committed. Unfortunately, I am still faced
with the problem because other managers do not agree with that
philosophy in all cases. Most of the time that will be the case, but
because our processes are more time driven than event driven I'm coming
up towards a brick wall with the scenario I described earlier. I am
really excited about incorporating CodeStriker into our process, and
except for this problem, everything is really going well. :)

Here are the diff files that would be created in my scenario and my
proposal:

- Developer 1's diff file (developer1.diff) should look like the
following:

Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.1
diff -u -r1.1 append.test
--- append.test	6 Feb 2004 23:01:29 -0000	1.1
+++ append.test	6 Feb 2004 23:02:32 -0000
@@ -1,7 +1,7 @@
 #This is a test
 test =3D 0
 #To see how to append
-append =3D 0
+append =3D 1
 #Differential information from different revisions
 diffs =3D 0
 #Into one diff file
Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.3
diff -u -r1.3 append.test
--- append.test	6 Feb 2004 23:04:49 -0000	1.3
+++ append.test	6 Feb 2004 23:06:27 -0000
@@ -5,4 +5,4 @@
 #Differential information from different revisions
 diffs =3D 1
 #Into one diff file
-file =3D 0
+file =3D 1

- Developer 2's diff file (developer2.diff) should look like the
following:

Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.2
diff -u -r1.2 append.test
--- append.test	6 Feb 2004 23:03:27 -0000	1.2
+++ append.test	6 Feb 2004 23:03:53 -0000
@@ -3,6 +3,6 @@
 #To see how to append
 append =3D 1
 #Differential information from different revisions
-diffs =3D 0
+diffs =3D 1
 #Into one diff file
 file =3D 0
Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.4
diff -u -r1.4 append.test
--- append.test	6 Feb 2004 23:05:27 -0000	1.4
+++ append.test	6 Feb 2004 23:05:34 -0000
@@ -1,5 +1,5 @@
 #This is a test
-test =3D 0
+test =3D 1
 #To see how to append
 append =3D 1
 #Differential information from different revisions

- Now that both tasks have been completed, both developers would like to
have each of their individual work reviewed.

- Each developer creates a separate CodeStriker topic, and
uploads/attaches their respective diff files.

<Problem:> When a topic is created, only a single file entry is created
for the "Contents:" list which is associated with the first encounter of
the filename in the uploaded diff file. (i.e. Index: append.test)
Subsequent entries of the same file are not associated with their
respective revisions. All subsequent diff entries for an identified file
entry is associated with the revision of the first encountered file
entry. The "Parallel" and "non-Parallel?" views feature is also broken
from this kind of diff file structure.

<Possible Solution:>When parsing the uploaded diff file, create a new
entry in the "Contents:" list for each file entry encountered in the
diff file. This should also preserve revisioning and maintain the
current "Parallel" and non-"Parallel" view functionality.




If you try these kind of diff files, do you see the same problems
happening?



Thanks, :)

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

> > The basic message is unreviewed code should never be committed to a
> > "public" branch that other developers are using.
>
> David,
> 	While I (in theory) agree with your last statement, we do have
> multiple developers working on projects (not always, but often), and so,
> they do share a branch.  They are fully authorized to beat each other
> with a wet noodle if one of them breaks the other's code (;>), but it
> does happen.  We do the reviews at the end, and sometimes the reviews
> cause changes, so we need to review it again, requiring a new
> codestriker topic, instead of just updating the existing one.

Ok, but in terms of reviewing code going into the main branch, you are 
happy to review it as an ordinary diff file?

What Matthew is proposing is to create a diff file that contains separate 
sections for each developer.

-- 
Cheers,
David

> Ok, but in terms of reviewing code going into the main=20
> branch, you are=20
> happy to review it as an ordinary diff file?
>=20

Correct. Doing a diff on a modified file, producing the unified diff
output and redirecting that output to a file, then uploading that file
to a topic in Codestriker should accomplish this.

> What Matthew is proposing is to create a diff file that=20
> contains separate=20
> sections for each developer.
>=20

Your comment about a "diff file that contains separate sections for each
developer" concerns me though.

Basically, the only changes that I see that Codestriker would have to
make is how it parses the uploaded diff file.

Currently each developer needs to:
- Create a diff file
- Create a Codestriker topic (upload their own diff file)

This would remain the same with my scenario. The only difference would
be that the diff file uploaded in our scenario would contain multiple
entries of the same file with different revision diffs.

Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.1
diff -u -r1.1 append.test
--- append.test	6 Feb 2004 23:01:29 -0000	1.1
+++ append.test	6 Feb 2004 23:02:32 -0000
@@ -1,7 +1,7 @@
 #This is a test
 test =3D 0
 #To see how to append
-append =3D 0
+append =3D 1
 #Differential information from different revisions
 diffs =3D 0
 #Into one diff file
Index: append.test
=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: /cvsrep/test/append.test,v
retrieving revision 1.3
diff -u -r1.3 append.test
--- append.test	6 Feb 2004 23:04:49 -0000	1.3
+++ append.test	6 Feb 2004 23:06:27 -0000
@@ -5,4 +5,4 @@
 #Differential information from different revisions
 diffs =3D 1
 #Into one diff file
-file =3D 0
+file =3D 1

When the diff file is uploaded currently, the 1.3 diff is put under the
1.1 listing of the "append.test" file.

I am suggesting that when the diff file is uploaded and parsed, that 2
listings are created for the "append.test" file (one for each occurrence
in the diff file which will be different revision diffs).

Each developer would still have to create their own diff file and create
their own Codestriker topic.

BTW, this has been a good exercise for me to evaluate myself in how well
I'm communicating the problem.
I hope I'm making more sense and not nonsense.

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

> -----Original Message-----
> From: David Sitsky [mailto:sits@...]
> Sent: Tuesday, February 10, 2004 4:24 PM
> To: Kelly F. Hickel; Matthew Hailstone; codestriker-
> user@...
> Subject: Re: [Codestriker-user] Feature request
>=20
>=20
> > > The basic message is unreviewed code should never be committed to
a
> > > "public" branch that other developers are using.
> >
> > David,
> > 	While I (in theory) agree with your last statement, we do have
> > multiple developers working on projects (not always, but often), and
so,
> > they do share a branch.  They are fully authorized to beat each
other
> > with a wet noodle if one of them breaks the other's code (;>), but
it
> > does happen.  We do the reviews at the end, and sometimes the
reviews
> > cause changes, so we need to review it again, requiring a new
> > codestriker topic, instead of just updating the existing one.
>=20
> Ok, but in terms of reviewing code going into the main branch, you are
> happy to review it as an ordinary diff file?
>=20
> What Matthew is proposing is to create a diff file that contains
separate
> sections for each developer.

Ahh, I missed that.  No, what I want (which of course, translates to
"the right thing" ;>) is to see all the chages on the branch rolled into
one review screen.  Ideally, each time I visited it, it would refresh to
the latest set of changes on the branch.

>=20
> --
> Cheers,
> David

See ya!
-Kelly

> Basically, the only changes that I see that Codestriker would have to
> make is how it parses the uploaded diff file.
>
> Currently each developer needs to:
> - Create a diff file
> - Create a Codestriker topic (upload their own diff file)
>
> This would remain the same with my scenario. The only difference would
> be that the diff file uploaded in our scenario would contain multiple
> entries of the same file with different revision diffs.

There is an assumption in the Codestriker database that there is only a 
single diff change per filename in the patch file.  Patch files (and diffs 
files from all SCM systems) have this property.  I still maintain it would 
be confusing for a reviewer to see a diff chunk for more than one file, 
and then to see another diff chunk for the same file.  The diff files you 
are talking about would have to be created by a custom tool, they can'tbe 
created by the SCM system.

A reviewer wants to see the cumulative code changes in one place, not in 
several places which would require them to mentally "merge" them together.  
Your provided example is very simple, and mental merging here is easy, but 
in a real-life scenario, I think it would be frustrating and error prone, 
especially if the second diff chunk makes changes made on the same lines 
of the first diff chunk.

What about the process Kelly describes?  Does that suit your requirements 
better?  Another option is before a developer does a commit for unreviewed 
code, they still post a Codestriker topic, but it gets reviewed at a 
future time.  That way, if your process needs to commit code in the SCM 
now, you can still commit it, and it can get reviewed later, as the topic 
has been created.

-- 
Cheers,
David

David,

Your suggestion is what my secondary plan of action was going to be.
The developers will just need to create a topic for each time that they
commit, or do a diff between two revisions (beginning and end) and
upload that file (notwithstanding potential changes from other
developers will appear in the diff). This will have to be acceptable. :)

Kelly's process of committing to particular branches would really break
our current process. Not that your model isn't good, Kelly. It is just
different than our very established way of branching and producing
deliverables. :)

Thanks for the discussion. It's interesting to learn how other
organizations carry out the code review process.
I look forward to 1.8.0!

:)
Matthew

> There is an assumption in the Codestriker database that there=20
> is only a=20
> single diff change per filename in the patch file.  Patch=20
> files (and diffs=20
> files from all SCM systems) have this property.  I still=20
> maintain it would=20
> be confusing for a reviewer to see a diff chunk for more than=20
> one file,=20
> and then to see another diff chunk for the same file.  The=20
> diff files you=20
> are talking about would have to be created by a custom tool,=20
> they can'tbe=20
> created by the SCM system.
>=20
> A reviewer wants to see the cumulative code changes in one=20
> place, not in=20
> several places which would require them to mentally "merge"=20
> them together. =20
> Your provided example is very simple, and mental merging here=20
> is easy, but=20
> in a real-life scenario, I think it would be frustrating and=20
> error prone,=20
> especially if the second diff chunk makes changes made on the=20
> same lines=20
> of the first diff chunk.
>=20
> What about the process Kelly describes?  Does that suit your=20
> requirements=20
> better?  Another option is before a developer does a commit=20
> for unreviewed=20
> code, they still post a Codestriker topic, but it gets reviewed at a=20
> future time.  That way, if your process needs to commit code=20
> in the SCM=20
> now, you can still commit it, and it can get reviewed later,=20
> as the topic=20
> has been created.
>=20
> --=20
> Cheers,
> David
>=20
>=20
 =20
*************************************=20
This e-mail may contain privileged or confidential material intended for =
the named recipient only.=20
If you are not the named recipient, delete this message and all =
attachments.=20
Unauthorized reviewing, copying, printing, disclosing, or otherwise =
using information in this e-mail is prohibited.=20
We reserve the right to monitor e-mail sent through our network. =20
*************************************=20

Comments below...

David Sitsky wrote:

 > On Thu, 28 Jul 2005 00:26, codestriker.jafs@... wrote:
 >
 >> It would be nice to have in the config file a value to set the valid
 >> open states, so that when I click on List all open topics, it would
 >> list all valid open states.
 >
 >
 >
 > Have you seen the @readonly_states config variable?  In some sense, 
what you are asking for is the inverse of this.
 >
 >
 >> Do you think that feature would be useful to other? As of now, I
 >> modified the template to put [0, 3] to display all open topics.
 >
 >
 >
 > Maybe you can make the code check if the state is not in 
@readonly_states, to get the same/similar effect?


This would certainly be possible, but I'm not sure it is what everyone 
would want.  Some installations (probably ours, for one, though it's 
probably not a big deal) would want the 'List open topics' link to list 
only topics in state 'open' and not ones in state 'Reviewed', even 
though both are not 'read-only' states (i.e. topics in either state can 
be edited).

 >
 > Failing that - perhaps you could defined a @open_states config variable.
 >

I would vote for this option.  I'm assuming it would control only the 
states that get displayed when you click the 'List open states' link, 
and would be different from @readonly_states.