Git Tag

I think you need a better description of the module, so dear d.o. people can actually understand what it is about.
Here's an example, please correct me if I'm wrong:

This module adds an admin page where a developer can pull latest changes from git. This is ideal to environments where several devs are pushing changes a single environment (which is a very common situation...), and the sysadmin doesn't want to let them ssh in the server only to do git-pull (which is also a common situation... :) ).

This way, developers can update the website staging/production environment, without exposing the server to unnecessary risks.

Interesting, but why does it have to be to specific git tags -- why not a generic git pull?

Marking as critical as per http://drupal.org/node/894256
Sorry for the long wait.

exec('sudo ...') ??? I don't think that's a real good idea ...

Menu titles shouldn't be in t().

I'm generally against writing a module like this which is just a thin wrapper around OS-specific commands. Why don't you communicate directly to the remote repository using the Git HTTP protocol? I realize that's a LOT more ambitious...

If this module used glip instead of exec() it would be acceptable.

This is still pretty insecure since it requires the ability to sudo without password to a user that has write access to the docroot.

Even if sudo was limited to "git", you could still do a remote add to a completely different repo and git pull from there.

Also, many code standard violations. Please run through coder module.

Quick scan, this not a thorough review:

1) Menu titles should not be run through t(). See docs at http://api.drupal.org/api/drupal/modules--system--system.api.php/functio...

2) GIT is properly spelled/capitalized as Git according to http://git-scm.com/

3) In git_tag_form() you are putting form data about available tags into drupal_set_message(). The theme may not put this near the form, or may not display messages at all. This data should be part of the form, either using '#type' => 'markup' or something along those lines. This appears that it would duplicate the data in the form select element.

3) in git_gat_form() the confirm element has a string that is using line breaks and does not use a continuous t(). This is incorrect. You should not be determining line break positions for your form, but leave that up to the theme by allowing it to wrap.

4) As a whole the git_tag_form has some inconsistencies that don't make a lot of sense. There's the confirm element that has a title that is a question without a question mark, which is redundant with the page title. Is there a reason the user would be on this page if they didn't want to pull, or is there another action they can perform from this page? The second item lacks a title, but includes a description. I would highly recommend combining those two form elements into a single select element with a title of "Select a tag" and using the #value of 'confirm' as the description for that form element.

5) As a whole, I think it'd make more sense to either call drupal_set_message more times rather than using
tags excessively in status messages.

6) This is a fairly destructive action, and as a whole, it would be good to require confirmation from the user before performing file system level actions. This would also give you a change to present to the user a list of changes, and ideally display the actual shell command that would be run after confirming the action. This should be easy to add with http://api.drupal.org/api/drupal/modules--system--system.module/function...

7) There is no UI to set the variable gittag_git_user.

Several others have raised concerns over the security of this module, and I would concur with them, it does not include enough validation of various items, such as the gittar_git_user variable, and that the commands being run are actual git commands, and not attempts to subvert the system security. If these concerns aren't addressed, or can't be addressed reasonably, this module may not be a good fit for a full project. While it works well for you, it's hard to communicate those kind of tradeoffs through a project page on d.o.

I should follow up here, that the issue isn't whether it uses OS shell commands, or glip PHP library, but that the webserver itself is able to write to the application files itself. This is why the D7 module installer uses FTP/SSH to write files to the system, not shell commands, and it does not ever store FTP/SSH credentials.

We're using this module constantly as part of our deployment process. It works as expected.
Please promote it to be a first-class module.

1) There are still many coding standards errors. You haven't yet run the module through Coder as requested, and haven't fixed the specific coding standards problems that were pointed out above.

2) Your module is named gittag, so all your functions should be prefixed by "gittag". Sometimes you use "gittag", sometimes you use "git_tag", and sometimes you just use "git".

3) You don't delete your system variables on uninstall. This should be done in hook_uninstall().

4) There's no need to have the git_tag_page() function at all. In your hook_menu(), just use:

    'page callback'    => 'drupal_get_form',
    'page arguments'   => array('git_tag_form'),

5) You haven't addressed several of the concerns raised above. For instance, you don't provide a way to set the gittag_git_user variable. What's the point of having a system variable here if it can't be changed? Also, your menu titles are still wrapped in t().

I still don't think the way this module is implemented is a good idea. By using such a potentially dangerous command, I think the burden is on you to show that you've taken all appropriate measures to protect the innocent module user from unnecessary risks.

Thanks @TR

1) Coding standards errors reported by Coder are fixed

2) Fixed naming to follow 'gittag' all across.

3) Variables are now added to $conf in settings.php, and README.txt was modified accordingly, so no variables need to deleted upon uninstall.

4) Fixed :).

5) Fixed the variable issue and the hook_menu one.

In general, this module is part of a concept and a workflow of working in a Drupal developing company.
In any case, the module is not for "innocent module user" but for system administrators and need some settings on the server.

@z.stolar - every time you make comments like "We're using this module constantly as part of our deployment process. It works as expected." it has the opposite effect in my mind. A review is a review. You can't just say you like something and move it to RTBC. Note how many more things (appropriate to be fixed first) were fixed after your rtbc?

Personally I would really like to see a decoupling of the webserver being able to write to its own code. This could be achieved by having Drupal update the database or a file on the filesystem and a cron job running as a different user that runs every 5 minutes and updates the code.

That said, I appreciate that there are some environments where this is appropriate. I think the best thing would just be to explain in more depth how this can be a problem. The project page says "use at your own risk" but it would be great if it provided more detail on the pros/cons of using a system like this and what risks someone is exposing themself to.

Setting to RTBC, though I'd really like to see the project page change prior to fixing.

@greggles: You are perfectly right about my status change to RTBC.

Changes to the project page or README will be made to explain in greater details what may be the implications of misuse of the module, and it should be used best.

@shaiw, then a "further decoupling" ;)

every system which updates the code of the site can be destructive, if its done in drush/cron or any other external system.

You and I both know this, but it would be great to explain those risks to the users of the module.

Thanks for your contribution, shaiw! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Forgot to change the status...