Code review comment for lp:~wlxing/ecryptfs/lp1695767

Revision history for this message
Tyler Hicks (tyhicks) wrote :

Hi Jason - Thanks for the patch!

After reviewing the function that you modified, I think the check for
node->num_transitions being zero can be done much higher. Around line
465, where node->num_transitions is already being checked for being
equal to one, should work fine and then you can drop the call to free
the "prompt" string.

Also, a few comments on patch style...

1) Please put the description of the patch into the commit message
   itself instead of into the pull request description. Someone
   researching why this change was made will typically look into the bzr
   log history but may not look at the Launchpad pull request.

2) The correct syntax for referencing a Launchpad bug is (LP: #1695767).
   The syntax used in the commit message includes an unnecessary "lp".

3) The patch includes a space character between the third and fourth tab
   characters on the line that rc is being assigned.

review: Needs Fixing

« Back to merge proposal