jensl/critic

Critic code review system.

critic's Issues

Demo

Is there a public demo hosted anywhere?

For a change with no comment, generate a summary like "2 issues resolved, 3 comments added, 8 files reviewed" as a title

The way it is today, Work Log is most often just an enormous list of "No comment" entries, which I end up never using (outside clicking the very newest one when I know there's an unread entry).

post-rename detect screen doesn't allow review

Here: https://critic.hoppipolla.co.uk/369c5b83?review=468

Hit 'm' and then select 'Any' and 'Any' and hit ok. Now you can't actually perform the review and hitting spacebar scrolls to the bottom them pops you back to the top. It's all very weird and I have no idea what I'm supposed to do here.

cc @jgraham

Show useful diff for files that are both moved and edited

When a file is moved, critic shows it as one deleted file and one added file, which makes it hard to see what (if anything) has changed apart from the move. It would be more useful if it said that the file has moved from A to B and then show the diff of the edits.

Unacceptable use of system resources

I installed critic out of curiosity and I've had the following problems with my computer since:

1. Dozens of watchdog processes run in the background
2. I sometimes receive hundreds of email every minute warning me that a file was created X seconds ago
3. Sometimes dozens of sendmail processes in the background (probably related to 2)
4. Occasionally high CPU usage
5. Eventually 100% RAM and swap usage (either from a memory leak or the dozens of backgrounded processes that may be multiplying)
6. Just hit 100% disk usage from /var/log/mail.log, /var/log/mail.info, and /var/log/syslog

These problems are most likely due to an incorrect configuration. I am planning on uninstalling critic as soon as I can figure out how because I actually have no use for it currently (I only installed it to try it out).

"Failed to validate commented lines" after rebase

After rebasing (new upstream), any attempt to add an issue to a commit (older than the rebase in this case) will result in a dialog saying "Failed to validate commented lines" (this happens as soon as you release the mouse button after marking source code lines).

The admin mail reads (some stuff cut away):

Subject: wsgi[validatecommentchain]: IndexError: list index out of range

Data: {
  "origin": "new",
  "count": 1,
  "review_id": 37,
  "parent_id": 1100,
  "file_id": 80,
  "offset": 167,
  "child_id": 1108
}

Traceback (most recent call last):
  File "/usr/share/critic/operation/__init__.py", line 100, in __call__
    return self.process(db, user, **value)
  File "/usr/share/critic/operation/createcomment.py", line 36, in process
    verdict, data = validateCommentChain(db, review, origin, parent_id, child_id, file_id, offset, count)
  File "/usr/share/critic/reviewing/comment/__init__.py", line 509, in validateCommentChain
    addressed_by = propagation.addressed_by[0]
IndexError: list index out of range

Note, this has not always been the case, but it happens more frequently lately on our setup.

Installation fails with python 2.7.3

Critic fails to install with Python 2.7.3 with an unhelpful error message. Upgrading to 2.7.5 solved the issue. 2.7.3 is the default on Ubuntu 12.04, which is the latest LTS release, so I guess supporting it would make sense. Otherwise the version check in the installation script should be updated.

$ python install.py
Traceback (most recent call last):
  File "install.py", line 44, in <module>
    import json
ImportError: No module named json
$ python --version
Python 2.7.3

cannot fetch from remote critic

I setup a critic server.
Auth by critic db and use cookie to keep session.
After then, I add critic as remote repo and fetch it.
It report an error with message "is this a git repository?"
I run tcpdump to get the exchanged with server.
git send request:

GET /myrepo.git/info/refs?service=git-upload-pack HTTP/1.1
Host: mydomain.com
Accept: */*
Pragma: no-cache

And critic server response:

HTTP/1.1 307 Temporary Redirect
Server: nginx/1.2.3
Date: Wed, 09 Jul 2014 04:52:35 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 0
Cache-Control: no-cache
Location: /login?target=/SyuTingSong/seafile.git/info/refs%3Fservice%3Dgit-upload-pack
Vary: Accept-Encoding

if send mail to one user fails, critic blocks all the mails after.

$subject.

Add uninstallation instructions

Uninstallation instructions or a script would be extremely helpful for those that installed critic just to play with it.

Critic treats moves as new files

https://critic.hoppipolla.co.uk/b429c0a3?review=2520

Critic should just mark that as a rename (GitHub does), and perhaps give an option to hide these renames.

Install script and file system permissions

Trying to run the install script (with sudo or as root) resulted in the error could not change directory to "/home/jacobr/Code/critic", originating from here. When running the install script from a directory where the postgres user had access, it worked fine. I am running Ubuntu 12.10.

Is there anything the install script can do about this?

Enable commit access by SHA-1 without requiring repository

Either /commits/COMMIT_SHA1 or /commits?sha1=COMMIT_SHA1 without the repository query being required.

Uninformative exception on pushing to repo

I can clone newly created repo. I've also created a commit and tried to push but getting this:

% g push origin HEAD:test
Counting objects: 3, done.
Writing objects: 100% (3/3), 233 bytes, done.
Total 3 (delta 0), reused 0 (delta 0)
remote: An exception was raised while processing the request.  A message has
remote: been sent to the system administrator(s).
To xxx.xxx.xxx:/var/git/test.git
 ! [remote rejected] HEAD -> test (pre-receive hook declined)
error: failed to push some refs to 'xxx.xxx.xxx:/var/git/test.git'

In the log I see this:

Request:
{
  "repository_name": "test",
  "refs": [
    {
      "new_sha1": "cff12eff12a224e8f0aa33d78ae119067538c1f3",
      "name": "refs/heads/test",
      "old_sha1": "0000000000000000000000000000000000000000"
    }
  ],
  "user_name": "rchlodnicki"
}

Traceback (most recent call last):
  File "background/githook.py", line 111, in <module>
    index.createBranches(user_name, repository_name, create_branches)
  File "/usr/share/critic/index.py", line 155, in createBranches
    for name, head in branches: processCommits(repository_name, head)
  File "/usr/share/critic/index.py", line 89, in processCommits
    commit = gitutils.Commit.fromSHA1(db, repository, sha1)
  File "/usr/share/critic/gitutils.py", line 665, in fromSHA1
    return Commit.fromGitObject(db, repository, repository.fetch(sha1), commit_id)
  File "/usr/share/critic/gitutils.py", line 259, in fetch
    stdin.write(sha1 + '\n')
IOError: [Errno 32] Broken pipe

Filed this for the record. I'll submit a fix.

Not possible to set etc_dir

Hi,

the installation script currently doesn't ask for a value of etc_dir, and a given command line argument is silently dropped. Instead only the default value /etc/critic is used. The relevant code is in installation/paths.py.

Add API to retrieve status of reviews

It would be really great to integrate tools like https://github.com/peterbe/github-pr-triage with Critic for projects like Servo. However, there doesn't seem to be any kind of API to retrieve the current status of a review (eg. how much is unreviewed, who is needed to perform said reviews, etc.).

Redirect to last viewed page after sign in

Normally, when I sign in, Critic redirects to /home.

Usually I want to be able to immediately comment on the review I was on, and /home is not of much use. Perhaps the behavior can be changed?

How can I remove a repository?

While testing ciritc I added some repositories which I want to have removed now.

Any hints about how to do that?

Python exception on installation

Creating group 'critic' ...
Creating user 'critic' ...
Creating directory '/etc/critic' ...
Creating directory '/etc/critic/main' ...
Creating directory '/usr/share/critic' ...
Creating directory '/var/lib/critic/relay' ...
Creating directory '/var/lib/critic/outbox' ...
Creating directory '/var/lib/critic/outbox/sent' ...
Creating directory '/var/cache/critic' ...
Creating directory '/var/cache/critic/main' ...
Creating directory '/var/cache/critic/main/highlight' ...
Creating directory '/var/git' ...
Creating directory '/var/log/critic' ...
Creating directory '/var/log/critic/main' ...
Creating directory '/var/run/critic' ...
Creating directory '/var/run/critic/main' ...
Creating directory '/var/run/critic/main/sockets' ...
Creating directory '/var/run/critic/main/wsgi' ...
Copied 318 files into /usr/share/critic ...
Creating database ...
Enabling module expires.
To activate the new configuration, you need to run:
  service apache2 restart
Enabling module rewrite.
To activate the new configuration, you need to run:
  service apache2 restart
Module wsgi already enabled
Enabling site critic-main.
To activate the new configuration, you need to run:
  service apache2 reload
 * Restarting web server apache2                                                                                                                                               apache2: Could not reliably determine the server's fully qualified domain name, using 127.0.1.1 for ServerName
 ... waiting .apache2: Could not reliably determine the server's fully qualified domain name, using 127.0.1.1 for ServerName
                                                                                                                                                                        [ OK ]
 Adding system startup for /etc/init.d/critic-main ...
   /etc/rc0.d/K20critic-main -> ../init.d/critic-main
   /etc/rc1.d/K20critic-main -> ../init.d/critic-main
   /etc/rc6.d/K20critic-main -> ../init.d/critic-main
   /etc/rc2.d/S20critic-main -> ../init.d/critic-main
   /etc/rc3.d/S20critic-main -> ../init.d/critic-main
   /etc/rc4.d/S20critic-main -> ../init.d/critic-main
   /etc/rc5.d/S20critic-main -> ../init.d/critic-main
FAILED: installation.initd.execute()
Traceback (most recent call last):
  File "install.py", line 151, in <module>
    if hasattr(module, "install") and not module.install(data):
  File "/home/rchlodnicki/workspace/critic/installation/initd.py", line 49, in install
    installation.process.check_call([target_path, "start"])
  File "/usr/lib/python2.7/subprocess.py", line 537, in check_call
    retcode = call(*popenargs, **kwargs)
  File "/usr/lib/python2.7/subprocess.py", line 524, in call
    return Popen(*popenargs, **kwargs).wait()
  File "/usr/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

ERROR: Installation aborted.

 Removing any system startup links for /etc/init.d/critic-main ...
   /etc/rc0.d/K20critic-main
   /etc/rc1.d/K20critic-main
   /etc/rc2.d/S20critic-main
   /etc/rc3.d/S20critic-main
   /etc/rc4.d/S20critic-main
   /etc/rc5.d/S20critic-main
   /etc/rc6.d/K20critic-main
Site critic-main disabled.
To activate the new configuration, you need to run:
  service apache2 reload
Enabling site default.
To activate the new configuration, you need to run:
  service apache2 reload
apache2: Could not reliably determine the server's fully qualified domain name, using 127.0.1.1 for ServerName

ProgrammingError: column "commit" does not exist

This was sent to my criticadmin email:

User: ronrande
Method: GET
URL: http://lys-critic.cisco.com/showcomments?review=1&filter=all

Traceback (most recent call last):
File "/usr/share/critic/critic.py", line 707, in main
for fragment in result:
File "/usr/share/critic/page/showcomment.py", line 186, in renderShowComments
cursor.execute(query, arguments)
File "/usr/share/critic/dbutils/database.py", line 79, in execute
self.__cursor.execute(query, params)
ProgrammingError: column "commit" does not exist
LINE 5: ...='draft' OR commentchains.uid=172) ORDER BY file, commit, fi...
^

-- critic

If critic works with Cetos6.5 which has httpd("apache")

Guys, sorry to interrupt you, looks like Critic is not compatible with the apache in Centos6.5 which is "httpd", some required apache-cmd is missed.

Is there some workaround?

Thx

Should be able to link to search results

The search feature is useful, but would be more useful if it was possible to pass around links to particular results. It would be fine for the search results to open in a new page instead of in a dialog to make this happen.

Critic attempts to make a commit when rebasing issue; fails with no identity

Fairly new install...

Rebasing a review, pushing to the critic server, get a wonderfully cryptic error message:

"remote: An exception was raised while processing the request. A message has
remote: been sent to the system administrator(s)."

The error log in githooks is as follows (sensitive information changed to tildas):

Traceback (most recent call last):
  File "/usr/share/critic/background/githook.py", line 114, in <module>
    index.updateBranch(user_name, repository_name, name, old, new, multiple, flags)
  File "/usr/share/critic/index.py", line 626, in updateBranch
    silent_if_empty=set([merge]), full_merges=set([merge]))
  File "/usr/share/critic/reviewing/utils.py", line 331, in addCommitsToReview
    db, user, review.repository, commit, do_highlight=False)
  File "/usr/share/critic/changeset/utils.py", line 44, in createFullMergeChangeset
    replayed = createChangeset(db, user, repository, commit, conflicts=True, **kwargs)
  File "/usr/share/critic/changeset/utils.py", line 83, in createChangeset
    replay = repository.replaymerge(db, user, commit)
  File "/usr/share/critic/gitutils.py", line 549, in replaymerge
    workcopy.run("commit", "--all", "--message=replay of merge that produced %s" % commit.sha1)
  File "/usr/share/critic/gitutils.py", line 481, in run
    os.path.join(self.path, self.name), *args, **kwargs)
  File "/usr/share/critic/gitutils.py", line 375, in runCustom
    raise GitCommandError(cmdline, output, cwd)
GitCommandError: '/usr/bin/git commit --all --message=replay of merge that produced f9dda211d570720bdda79e2ba07d5e68cb936860' failed: *** Please tell me who you are.

Run

  git config --global user.email "[email protected]"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: empty ident  <critic@~~~~~)> not allowed (in /var/lib/critic/temporary/~~~~_f9dda211d570720bdda79e2ba07d5e68cb936860__InVFn/~~~~.git)

2013-08-20 18:31:34,573 -  INFO - session ended: kraln / ~~~~

Use Markdown in review descriptions, issues and notes

When displaying review descriptions in the new frontend without <pre>, I've noticed that people already use basic formatting in descriptions, like lists. The same syntax for bulleted and ordered lists would be valid in Markdown.

If we use a decent Markdown library we could also add our own parsing for review ids and commits, and link to those.

Blame functionality is not quite like real blame

blame (b) in the changeset view works as expected for lines that were modified in this commit but for all other lines it will just point to the parent commit, instead of like real git blame, pointing to the commit that modified the line.

Using an incorrect path in a filter causes exceptions

When invalid paths like /foo are encountered, critic starts throwing exceptions e.g.

Traceback (most recent call last):
File "critic/operation/init.py", line 328, in call
try: return self.process(db, user, **value)
File "critic/src/critic/operation/manipulatefilters.py", line 296, in process
"count": counts[filter_path] })
KeyError: '/webdriver/'

and some others. It should instead handle such errors gracefully,or, if possible, not error at all e.g. by treating /foo/ as the same as foo/.

Generate rebase-equivalent merges that don't include additional new commits

It's a sucky experience when I push a branch for review, then add some more commits locally, rebase the branch, then push it for review again and rebase the review. Instead of seeing the old commits, the merge commit, and then the new commits, I see a merge commit which also includes the changes in the new commits, which makes it much harder to review.

Critic fails to work with branch name containing '#'

A user at my local Critic installation pushed a branch called "userid/bug#177099". When he selects the branch in Critic's branch list, Critic says:

'userid/bug' doesn't name a branch!

I guess Critic should either refuse the branch to be pushed in the first place, or should be fixed to work with branches containing '#'

Add API endpoint to map ("describe") a commit to a friendly name

It should work like the describe method on the Repository class, so given a commit SHA-1 sum and a repository ID, look for suitable tags and significant branches.

Perhaps it can be located at /api/v1/repositories/REPO_ID/description/COMMIT_ID.

Installation on Ubuntu 14.04

Hi,

I don't seem to get the system running after installing on Ubuntu 14.04. The quickstart will launch but Apache is not running the Critic system up. I am using the newest version updated with all the default newest packages.

Anyone here able to send me a clue how to troubleshoot this, I can't seem to see anything in the logs that is helpful either in Apache or the Critic logs.

Unicode problems in renderChains

The renderChains routine that displays the comments attached to a review (/r/N) has problems displaying unicode characters.

See the attached screenshot where the character ’ is replaced by �:

Cannot display review when adding reviewer with empty Directory

I'm playing with Critic at work, and when I created my first review, I added a colleague specifically to that review (since nobody has configured any filters yet). In the "Add Reviewer" dialog, I entered his user name, and left the Directory blank, and pressed "Add". The review was created and emails sent out, but when I tried to enter the review page, I got the "Darn! It seems we have a problem...", with the following email being sent to admin:

User: jherland
Method: GET
URL: http://lys-critic.cisco.com/r/1

Traceback (most recent call last):
File "/usr/share/critic/critic.py", line 707, in main
for fragment in result:
File "/usr/share/critic/page/showreview.py", line 654, in renderShowReview
row("Reviewers", renderReviewers, "Users responsible for reviewing the changes in this review.", right=False)
File "/usr/share/critic/page/showreview.py", line 358, in row
if callable(value): value(main_row.td('value', id=cellId, colspan=colspan).preformatted())
File "/usr/share/critic/page/showreview.py", line 465, in renderReviewers
filter_id, filter_user = filter_data[(fullname, original_path)]
KeyError: ('Morten Rolland', '/')

-- critic

I ended up logging into the database, and manually resetting the path from "" to "/", and that seems to have fixed the crash. However, I'm guessing the empty string shouldn't have been stored into the db in the first place...

criticctl deluser doesn't work

Output from a test of adding, removing, and then adding the same user again.

criticctl adduser --name test --email [email protected] --fullname Test --password changeme
test: user added
criticctl deluser --name test
test: user retired
criticctl adduser --name test --email [email protected] --fullname Test --password changeme
test: user exists

Crash when searching using Path: field

1. navigate to http://kingfish/search?path=1
2. Darn! It seems we have a problem

Traceback (most recent call last):
File "/usr/share/critic/critic.py", line 968, in main
result = pagefn(req, db, user)
File "/usr/share/critic/page/search.py", line 114, in renderSearch
file_ids = dbutils.contained_files(db, dbutils.find_directory(db, path_value))
File "/usr/share/critic/dbutils.py", line 460, in contained_files
cursor.execute("SELECT file FROM containedfiles(%s)", (directory_id,))
File "/usr/share/critic/dbutils.py", line 102, in execute
self.__cursor.execute(query, params)
ProgrammingError: column "file" does not exist
LINE 1: SELECT file FROM containedfiles(1)

Unable to reopen issue when file is removed

https://critic.hoppipolla.co.uk/showcomment?chain=2819

1. An issue is raised about a line in a file.
2. The file is removed which addresses the issue, and other files are added.
3. I click 'Reopen Issue' which prompts me to select a new line in the new diff.
4. Clicking a line in the new diff doesn't reopen the issue, instead it creates a new issue.

mis-using commit object and commit id

in log/commitset.py, this line:

self.__tails = parents - commit_set is error, since parents is a set of strs, but commit_set is a set of commit objects, these two are different.

btw, according to the comment, i think the right order is commit_set - parents.

Enable password recovery without sending emails...

A suggestion from one of the users on my Critic installation: Should be possible to reset one's password without having to send emails to the server admin.

I myself have no opinion on the request, but I figured I'd bring the issue forward in case you have not yet considered it...

Make the common rebase case easy

In my experience using Critic for Servo, the common case is that the rebased commits are just being shifted to a more recent base revision, and we could easily automate the process of rebasing the review by matching the commit messages against the old commits and finding the first one that is different. This would make the process a lot easier for our contributors - Critic is a constant pain point for any nontrivial first-time contributions from a newcomer.

Make it possible to install critic on Mac

I don't have much experience with critic installation process (only tried it on Linux) and not that much in Mac itself, but I'd really like to be able to install it there.

I have a Mac so I can contribute or maybe even do it myself, but having someone willing to help would be nice too.

Design brainstorming

I wanted to do something about the topbar design, one thing led to another, and I ended up with some more concept sketches and suggestions.

I think that some important goals for a new design are:

• Increase learnability for somewhat-familiar users
• Reduce the number of steps for common actions for familiar and expert users
• Make it more obvious for unfamiliar (non-)users how a Critic review works

These are all lo-fi sketches, mostly showing suggested placement and hierarchy between layout elements. Some of the ideas would require substantial changes, while others could be cherry-picked to the current design.

Top bar

Review info box

Commit list

All

Ability to add issues to "actual log" of a review branch

Whenever a merge or a rebase happens in a review, it becomes less convenient to review the changes as a whole (i.e. selecting all of the fixups and looking at the result).

The way to do it AFAIK is to rebase the review and then look at the "actual log", but in this view there is no way of adding new issues as it is just a plain git log view.

Therefore, it would be nice to be able to review the whole branch, with the ability to view and add issues, in an "actual log" kind of view.

Need Critic to set environment variable to co-exist with Gitolite

Hi,

I have set up Gitolite to handle incoming pushes and access control to the Critic repos. Although a bit fiddly, this works mostly well in practice. Except for this:

When Critic updates the tags and tracked branches from upstream, it seems to perform a local push from a relay repo into the "proper" repo. However, this push fails because the Gitolite update hook depends on some environment variables being set (in my case I want to set $GL_LIBDIR and $GL_BYPASS_ACCESS_CHECKS, to get Gitolite out of Critic's hair).

Where should I set these variables? Can I set them in the /etc/init.d/critic-main startup script (or rather make that script source /etc/critic/env.sh)? Or do I need to edit the relevant python code in background/branchtracker.py?

Enable commit.expandAllFiles by default

I think commit.expandAllFiles option should be enabled by default. That is because I think it's not obvious that clicking the file names in the commit view does something.
Making things more obvious would be another option.

Show change in size for binary files

When reviewing binary files, it would be nice with before and after file size.

This would be useful even for image files which actually show the diff. Sometimes the decoded image pixels isn't changed, but raw file data is (f.ex if someone ran some image crush tool, or forgot to do so).

"Create Comment" dialog is slow to open and close

When adding a review issue by clicking a line, the dialog "Create Comment" does not appear immediately. It would be nice if it did. Further, when clicking "Add issue" or "Add note", it would be nice if the dialog disappeared immediately, but with an indicator somewhere showing that it's currently syncing with the server.

Github integration makes links go to /home

e.g. http://critic.hoppipolla.co.uk/r/8 when I click the link, I end up at http://critic.hoppipolla.co.uk/home . If I navigate to the first URL again then it works.

Maybe this has something to do with the github integration.

Feature request: Use Cc from commit message to find reviewers

A colleague of mine suggested the following, which I find eminently sensible:

Some people put lines like this at the end of the commit message

Cc: My colleague [email protected]

to indicate that they want their colleague to have a look at the commit in question. git send-email will also interpret this, and add the colleague to the Cc-list of the generate email.

I think it would be good for Critic to do the same, and use Cc-lines from the commit message as an extra source of reviewers to be assigned to the corresponding review.

Now, there is a slight impedance mismatch here, in that Critic assigns reviewers per path, whereas a Cc-annotation is per-commit. Still, I think it would be useful to auto-assign the paths changed by the relevant commit to the mentioned reviewer (even if that would also match other commits where the colleague was not Cc-ed). As always being assigned as a reviewer to some path does not mean that you have to review it, especially not if others are assigned to the same path.

Another potential problem is that the "My colleague [email protected]" annotation does not necessarily map directly to a single Critic user. However, in practice I believe looking up the full name and/or email address would work very well, with very few false positives.

What do you think?

why critic requires apache?

why critic requires apache?
since critic is written in python lang, by default it can be served with some server written in python, or may be needed some options into install scripts to use nginx/cherokee/lighty/apache. please be server agnostic, and don't stick to apache as the only requirement in installation scripts.

Impossible to install without passlib support?

When answering 'no' on the "Do you want to install the 'passlib' module?" question I later get the following exception after selecting the access scheme.

How will Critic be accessed? [http] http
FAILED: installation.config.prepare()
Traceback (most recent call last):
  File "install.py", line 128, in <module>
    if hasattr(module, "prepare") and not module.prepare("install", arguments, data):
  File "/path/to/critic/installation/config.py", line 511, in prepare
    calibrate_minimum_rounds()
  File "/path/to/critic/installation/config.py", line 112, in calibrate_minimum_rounds
    import passlib.context
ImportError: No module named passlib.context

ERROR: Installation aborted.

Consider adding review progress to the pending "owned by you" reviews on the dashboard.

The actions required from me are quite different between "100%, X issues" and "0%", so this seems useful to have on the dashboard.