Git Commit Logs

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
Review of the master branch:

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
  

  git_commit_logs.module:37:  //operation for specific node type
  git_commit_logs.module:40:    //added a fieldset
  git_commit_logs.module:59:    //disable url field once edited
  git_commit_logs.module:62:    //added a url text field
  git_commit_logs.module:163:    //check if hash is already there
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./git_commit_logs.module:55:          $data[] = array('id' => $id, 'date' => $row['date'], 'name' => $row['author'], 'email' => $row['email'], 'message' => $row['message']); // selected fields only
  ./git_commit_logs.module:100:  if ((valid_url($project_url, FALSE) && substr($project_url, -4, 4) != '.git') ) { // substr last 4 characters
  ./git_commit_logs.module:119:  $files_dir = $_SERVER['DOCUMENT_ROOT'] . $base_path . file_directory_path() . '/git_commit_logs/';  // base path incase working on localhost url could be http://localhost/project1
  ./git_commit_logs.module:128:    exec('cd ' . $folder . '; git pull'); // if project already in local, update it
  ./git_commit_logs.module:132:    exec("git clone " . $project_url .' '. $folder); //execute multiple command seperated with semi colon
  ./git_commit_logs.module:134:  exec('cd ' . $folder . '; git log ', $output); //exec will provide data in array format; read project logs
  ./git_commit_logs.views.inc:97:    'left_field' => 'nid', // base table field eg: node
  ./git_commit_logs.views.inc:98:    'field' => 'nid', // our table field
  

• ./git_commit_logs.install: all functions should have doxygen doc blocks, see http://drupal.org/node/1354#functions
  

  
  function git_commit_logs_install() {
  --
  
  function git_commit_logs_uninstall() {
  

• There should be a space before and after operators like ==, ===, && and ||. See http://drupal.org/node/318#controlstruct
  

  git_commit_logs.module:138:    if ( strpos($line, 'commit')===0 ) {
  git_commit_logs.module:145:    elseif ( strpos($line, 'Author')===0 ) {
  git_commit_logs.module:149:    elseif ( strpos($line, 'Date')===0 ) {
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  git_commit_logs.module:39:  if ( !empty($set) && $form_id == $form['#node']->type . '_node_form' ) {
  git_commit_logs.module:61:    if ( !empty($url) ) $attributes = array('readonly' => 'readonly');
  git_commit_logs.module:71:    if ( !empty($data) ) {
  git_commit_logs.module:103:  if ( preg_match('/[^A-Za-z\\-_:.\\/]/', $project_url) == TRUE ) {
  git_commit_logs.module:127:  if ( file_exists($folder) ) {
  git_commit_logs.module:137:  foreach ( $output as $line ) {
  git_commit_logs.module:138:    if ( strpos($line, 'commit')===0 ) {
  git_commit_logs.module:139:      if ( !empty($commit) ) {
  git_commit_logs.module:145:    elseif ( strpos($line, 'Author')===0 ) {
  git_commit_logs.module:149:    elseif ( strpos($line, 'Date')===0 ) {
  git_commit_logs.module:158:  foreach ( $history as $single ) {
  git_commit_logs.module:166:    if ( empty($if_exist) ) {
  git_commit_logs.module:180:  if ( $type == 'email' ) {
  

• Remove all old CVS $Id tags, they are not needed anymore.
  

  git_commit_logs.info:1:; $Id$
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

  ./git_commit_logs.views.inc
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

The above errors are straight out of coder I think .. or maybe soemthing separate. The Coding Standards are what everything is based off. http://drupal.org/coding-standards

Basically, if you have a control statement like this

if ( !empty($set) && $form_id == $form['#node']->type . '_node_form' ) {

It is fixed like this

if (!empty($set) && $form_id == $form['#node']->type . '_node_form') {

and here

if ( strpos($line, 'commit')===0 ) {
      if ( !empty($commit) ) {

to this

if (strpos($line, 'commit') === 0) {
      if (!empty($commit)) {

Updating status.

Hi,

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

Review of the 6.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/git_commit_logs.module:
   +4: [minor] Comment should be read "Implements hook_foo()."
   +23: [minor] Comment should be read "Implements hook_foo()."
   +33: [minor] Comment should be read "Implements hook_foo()."
   +102: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +126: [minor] There should be no trailing spaces
   +130: [minor] There should be no trailing spaces
   +131: [minor] There should be no trailing spaces
   +135: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
   +135: [minor] There should be no trailing spaces
   +138: [minor] There should be no trailing spaces
   +139: [minor] There should be no trailing spaces
   +147: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +150: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +151: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +154: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +160: [minor] There should be no trailing spaces
   +192: [minor] Comment should be read "Implements hook_foo()."
   +204: [minor] Comment should be read "Implements hook_foo()."
   +209: [minor] There should be no trailing spaces
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 normal warnings, 18 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• ./git_commit_logs.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

    // base path incase working on localhost url could be http://localhost/project1
  

• There should be no space after the opening "(" of a control structure, see http://drupal.org/node/318#controlstruct
  

  git_commit_logs.module:48:    if ( isset($form['#node']->nid) ) {
  

• All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting
  

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

Manual review:

• There are still files in your master branch. You should replace them as described in http://drupal.org/node/1127732 at step 5.
• The "newlines at file endings" message is empty. There is something odd in some automated reviews atm. Manually checked, there are some files with many, some with one newline. I suggest you set them all to "one newline" and push them again.
• .module file, line 5: AFAIK, in function headers for hook implementations there is no need for a summary. I suggest moving it inside the function as an inline comment (in "// " style then). Different with all non-hook functions, you should add doxygen comments there as described in http://drupal.org/node/1354#functions

hth, dave

@devtherock has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

The application has been closed. If you would like to reopen it, you are free to do so.
See http://drupal.org/node/894256#abandonedtwoweekscontact

I am co-maintainer of this project. Last days were quite busy. Now This project is under active maintaining. All Above errors are resolved. Now I am putting this issue on "needs review".

In the install file, line 33: " 'description' => 'Email address of auther',"
I think that should be "author", as in the field above it.

In the module file:

Line 84, the header labels should be wrapped in t().

Line 91, the line adding the submit handler is commented out. Why?

Line 116, "Project URL can't contains invalid character." would be better
something like this: "Project URL may not contain invalid characters."

Function function git_commit_logs_read_gitlogs(): Have you made certain that
it's not possible to alter directories other than the intended ones on the
server? This might be possible by entering an invalid git repository URL, for
example by adding "../../" in the URL. What about adding semicolons and
arbitrary commands in the URL string? In any case, code that alters the server
directory structure based on user input is something that you should triple
check.

Automated check at http://ventral.org/pareview revealed quite a few coding
standard violations:

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/git_commit_logs.module
 --------------------------------------------------------------------------------
 FOUND 23 ERROR(S) AFFECTING 7 LINE(S)
 --------------------------------------------------------------------------------
 148 | ERROR | Whitespace found at end of line
 149 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 149 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 149 | ERROR | Inline comments must start with a capital letter
 149 | ERROR | Inline comments must end in full-stops, exclamation marks, or
 | | question marks
 149 | ERROR | Comments may not appear after statements.
 150 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 150 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 151 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 151 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 151 | ERROR | Whitespace found at end of line
 151 | ERROR | Inline comments must start with a capital letter
 151 | ERROR | Inline comments must end in full-stops, exclamation marks, or
 | | question marks
 152 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 152 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 152 | ERROR | An operator statement must be followed by a single space
 152 | ERROR | There must be a single space before an operator statement
 153 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 153 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 153 | ERROR | An operator statement must be followed by a single space
 153 | ERROR | There must be a single space before an operator statement
 154 | ERROR | Spaces must be used to indent lines; tabs are not allowed
 154 | ERROR | Line indented incorrectly; expected at least 2 spaces, found 1
 --------------------------------------------------------------------------------
 

solved above errors. That was because of new code to pull logs from all branches rather than master branch.

The last one :)

FILE: ...w/sites/all/modules/pareview_temp/test_candidate/git_commit_logs.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
85 | ERROR | If the line declaring an array spans longer than 80 characters,
| | each element should be broken into its own line
--------------------------------------------------------------------------------

This looks like a very useful module, and I look forward to it becoming a full project. If/when it does, if you would like a collaborator to work on the Drupal 7 port, please let me know - I would be happy to help.

1. Language suggestion for checkbox "Check if you want to use this content type for git logs" could be just "Import Git Logs".

2. Make git_commit_logs path configurable. The risk of conflict here is lower than with some modules but it still exists.

3. There is a security risk here that you need to address. You are cloning the Git project into a public files directory. What if the project is not public code? The user might assume that by not publishing the node, or using content access control, that this information will not be available to everybody. But an anonymous user could access this directory with no protections. There are many possible solutions but all have trade-offs: I cannot say what is right here. .htaccess? Removing the directory once processed?

4. Another security risk: You are using user-generated input in very dangerous ways such as in exec commands. You must sanitize these inputs. ($folder, $project_url, etc.) It is not enough to call check_url() here. That only prepares URLs for embedding into an HTML page, not for passing to exec().

You should also review the language input filters and similar modules display to users when configuring them. This module could be just as dangerous as php_filter if not used correctly.

I see that you are doing some sanitizing in the form _validate() handler but you should also do it in the actual callback because there are many ways (like through Drush, other add-on modules, possible mistakes in code, or perhaps you improve the rules but an existing site already has a bad URL) that this check can be bypassed.

You should also note that you are doing this in your README.txt:
a. That you require "git" to be in the Web server processes' path.
b. That you use exec() to call it. Sites with safe_mode, limited lists of PHP functions, restrictions against calling out-of-directory binaries, etc. may not be able to use this module.

5. git_commit_logs_git_user_email() has a confusing name because it operates on both author and email fields. Consider renaming it. Also, it looks like it's doing double work because it's always called twice in a row. Why not return an array with both values?

      $commit['email'] = git_commit_logs_git_user_email(drupal_substr($line, drupal_strlen('Author:')), 'email');
      $commit['author'] = git_commit_logs_git_user_email(drupal_substr($line, drupal_strlen('Author:')), 'author');

becomes:

      // git_commit_logs_parse_user() can get both author and email out of the line in one call
      $commit += git_commit_logs_parse_user($line);

6. SUGGESTION: Provide a setting to pass to git clone to control --depth. On very large Git projects this tool takes a LONG time to clone. My browser timed out and the node save failed. It would help to also be able to limit the git log command to N entries.

7. SUGGESTION: Provide control over the branches tracked. It looks like you're reading them all. On a test project with 5,000 commits and 25 branches my CRON used 100% CPU for several minutes. The user should at least me warned about this possibility, or allowed control.

8. There is still one error in automated review: http://ventral.org/pareview/httpgitdrupalorgsandboxdevtherock1293404git. But actually you have many moer that the tool is not picking up because you are using so many delimiters that I think it cannot keep track. For example, in line 148 you have a block of code indented an extra level. It is below an exec with a command chain and I think this is throwing off the checker, but it should still be fixed.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.