4

I want to configure the following:

  1. When developers check-in code from their SVN client, a hook should verify whether it has a revision property "CodeReview" set along with the value of the property if it is set.
  2. If it is not set then add the revision property and set its value to "Not Done"
  3. When code review is done, update the property value to "Done".

I get error at step 1 itself. I tried by adding a pre-commit hook to check if the revision property is set. I am not able to do this in a pre-commit hook. I wrote a pre-commit.BAT file and used svn propget as below:

"C:\Program Files\Subversion\bin\svn.exe" propget "CodeReview" -t %TXN% %REPOS% >property FIND "%property%" "C:\repos\hooks\requiredproperties.txt">Null
If %ERRORLEVEL% EQU 0 goto OK1

This gives error -- complains about -t.

Can anyone help me with the scripts for the three steps?

bahrep
  • 29,961
  • 12
  • 103
  • 150
Yeshwant
  • 71
  • 5
  • Codereview status also haven't sense without relation to revision of file, which was reviewed (you have to store **revision with status** somehow) – Lazy Badger Apr 16 '13 at 22:00

3 Answers3

3

As already noted, you have to use svnlook on step 1. But situation is even more worse: it's strongly not recommended (while not prohibited) to modify transaction content in pre-commit hook in order to avoid unpredictable results, but you want to do it on your step 2

And, JFYI, property (any svn property) is attribute of file or directory, you tried to propget from transaction, which will be never successful

Lazy Badger
  • 94,711
  • 9
  • 78
  • 110
2

I have a pre-commit hook that can make sure properties are set on files when they're committed and that these properties are set to particular properties.

This will make sure that the property is set on a files when they're committed. HOWEVER, there are several issues:

File properties are revisioned just like files. If I set on my file the property CodeReview = Done on revision 14, it will be set to CodeReview = Done when the next person commits their change into Subversion. Not exactly what you want.

There isn't an easy way to query file properties to see which ones have the value of Done and which don't.

My humbly submitted recommendation: You're heading into a world of hurt. Instead, setup Jenkins as a continuous build server. Each time someone commits a change in Subversion, Jenkins will fire off a build recording what got changed and by who. Then, instead of marking individual files as being reviewed or not reviewed, you verify the Jenkins build which take place on an entire changeset (which really makes more sense anyway).

You can use the Promoted Build Plugin to mark whether a particular build has been reviewed, and whether it passed the code review. You can also attach to each build a comment on whatever issues were discovered in your code review.

Now, excuse me as I get on my soapbox here...

Most code reviews are pretty worthless. There are three major aspects of code review:

  1. Did the user follow the corporate standard practices?
  2. Is the logic embedded in the program any good?
  3. Did the developer make a silly error?

Too many code reviews focus on Issue #1 and leave little time left for the rest of code reviewing or even development. I've spent hours in a code review where a developer called a variable CustomerServiceResponseCode and not customerServiceResponseCode. We've even argued whether it should be responseCodeCustomerService. Hours of my life I will never get back doing more important things like playing Angry Birds.

As for Point #2: This should be done before any code is written. It's a bit too late when the developer spent 5 hours coding to be told You did it all wrong.

Now, let's get to Point #3.

Before code reviews:

Developer #1: Hey, there's a bug in your program!

Developer #2: Wow, yeah. I forgot to fomberbrate the verbex before I plugdix it. Boy that's a stupid mistake.

Results: One developer who feels incompetent and stupid.

After code reviews:

Developer #1: We just got a report back that there's a bug in the program!

Developer #2: Wow, We forgot to fomberbrate the verbex before we plugdix it. How did this slip by in code review?

Results: A whole team of developers who feel incompetent and stupid.

I have rarely seen a code review that actually catches a bug -- even a simple one -- before approving the code. It's someone else's code and most of the developers have other things on their minds. They do a quick look over, see the overall logic looks right, and then to show they're paying attention, complain that the indentation isn't correct.

What to do?

  • As part of your process, all new development should be discussed before the code is written. This isn't code review, this is part of your issue tracking system.

  • Use automated tools to catch things. In Java, we have Checkstyle (is the code formatted correctly), Findbugs, PMD (both tools catch potential bugs), copy/paste detectors, etc. Jenkins can run these programs and build all sorts of charts and graphs. Even better, it can be part of a Continuous Integration game. You get points for solving Checkstyle and Findbugs issues, and lose points for creating new ones. Also insist that compiler warnings and deprecation notices are fixed. Again, Jenkins can track them and give points for fixing them and handling them.

  • Use Unit testing. When I head a project, I insist that all unit tests are written before a lick of code is created. This gives developers a goal to reach when developing and the code is usually better with fewer bugs. Use coverage tools to see how well your unit tests are covering your code.

Using automated tools usually catch more errors than manual code reviews can. Automated testing can detect when code logic might have issues.

This doesn't mean code reviews are useless. It simply allows for code reviews to be overall reviews of the code. For example:

  • What did the developer think about the coding? Did they find the implementation more complex than it should have been because of poor design somewhere else?
  • Is the program still readable and understandable? Are there too many exceptions and if clauses to get around implementation issues?
  • Was the general strategy followed? Did the developer have an issue using a base class/API because of bad implementation?
  • Did the developer follow the agreed upon logic?
  • Did the developer depend on say one JSON third party library, and you've seen another third party library used in the rest of the code?

Don't do code reviews for the sake of code reviews. Don't waste too much time on the implementation of code reviews. And, for God's own sake, do not do pre-commit code reviews. This means that developers are stuck waiting for approvals before they can do any work.

Community
  • 1
  • 1
David W.
  • 105,218
  • 39
  • 216
  • 337
  • 1. I tend to disagree with *your* CR tasks vision and priorities. No 2 is most important and useful part: "Don't allow juniors to write shitty, but formally buildable code". In this aspect Jenkins != CR (because it can't detect ugly coding style of code-monkeys, and this style is **more dangerous in perspective**, than not-detected bugs) – Lazy Badger Apr 17 '13 at 00:49
  • @LazyBadger I didn't list the items in order. I listed #1 first because that's what most CR waste their time on. Yes, #2 is the most important, but should be done before code is written. CI != CR, but tools like Checkstyle can _detect ugly coding style of code-monkeys_. No need to spend hours going over formatting issues. Jenkins is good not because it's CI, but because it can integrate Issue tracking, Unit tests, code eval, etc. It becomes a dev hub for us because of all the information. We did code reviews, but focus on the points I mentioned at the end. – David W. Apr 17 '13 at 01:54
  • #2 (for me) is never-ending process (before-in-after coding), because you can't guarantee quality of code from the side of low-level coders (checkstyle only for style, not for properly styled, but terrible in implementation code). #2 (again - for me) is single reason for using CR in appropriate use-cases – Lazy Badger Apr 17 '13 at 20:50
  • Thanks David. I am thinking of the below process for CR: – Yeshwant Apr 19 '13 at 18:49
  • 1. Static analysis: Done on 100% code checked-in. It acts as gate keeper, automated via pre-commit hook. Considering legacy code will have lots of issues, baseline it code and then apply the gate keeping to only new issues. 2. Peer reviews: Do selectively and for a revision (considering all check-ins are atomic). For tracking purpose, use a revision property to record review status (Required, WIP, Done). If a tool is available to manage the peer review workflow, then configure it to update the revision property else run a script manually. – Yeshwant Apr 19 '13 at 18:56
  • For tracking/reporting/control: Write ETLs to populate peer review data to db. Run Static analysis on SVN repo also. Write BI reports based on peer review data and static analysis data to satisfy information needs of various stakeholders. – Yeshwant Apr 19 '13 at 18:58
1

You cannot use svn.exe as it does not support the -t option. You should use 

svnlook.exe
Peter Parker
  • 29,093
  • 5
  • 52
  • 80