Extend sensor: Git branch / tag

Thank you!

From the description above and discussions we did, we learned that one command execution is not always enough. (especially if we would want to check if submodules are initialized properly and more...)

Now you're passing the problem to find the right command to the user. That was not the intention...
I would have expected an optional dropdown to pick a branch or a tag.

Let's see what Berdir wants for the initial release. We should remain simple.

A bit feedback here.

1. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,314 @@
   +      if (strpos($value, '*') !== FALSE) {
   ...
   +      if (strpos($value, '*') !== FALSE) {
   ...
   +      if (strpos($branch, '*') !== FALSE) {
   ...
   +      if (strpos($tag, '*') !== FALSE) {
   

   The * should be the first posision. Not an unspecified one.

2. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,314 @@
   +  protected function checkTag($tag) {
   ...
   +      throw new GitRepositorySensorException(format_string('Unexpected tag @current expecting @expected', array('@expected' => $tag, '@current' => $current_tag)));
   ...
   +  protected function checkBranch($branch) {
   ...
   +      throw new GitRepositorySensorException(format_string('Unexpected branch @current expecting @expected', array('@expected' => $branch, '@current' => $current_branch)));
   

   This is bad design. With a check function, you expect that the check might not be successful. It should be true or false and could have an argument by ref.

   Exceptions are for unexpected function / API behavior.

3. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,314 @@
   +      '#options' => $this->getBranches(),
   ...
   +    foreach ($this->getBranches() as $key => $value) {
   ...
   +  protected function getBranches() {
   

   Agree, this "*" is the output of the shell command. But it's totally strange to have an API like that and callers need to parse it... We should return something better. Unsure: Is using local variables an option?

4. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,314 @@
   +    if ($this->getRepositoryPath($values['repo_path']) == NULL) {
   +      form_set_error('repo_path', t('The provided path %path does not exist or is not a git repository.', array('%path' => $values['repo_path'])));
   

   Before it was optional. I was talking about possibly removing it from UI. Now you make it even mandatory? Now it's clearly wrong.

5. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,314 @@
   +  protected function getRepositoryPath($path) {
   ...
   +    return $this->repositoryPath;
   ...
   +        $this->repositoryPath = $repo_path;
   ...
   +  protected $repositoryPath;
   ...
   +    if ($this->getRepositoryPath($this->info->getSetting('repo_path')) == NULL) {
   ...
   +    if ($this->getRepositoryPath($repo_path) == NULL) {
   ...
   +      $repo_path = $form_state['values'][monitoring_sensor_settings_key($this->getSensorName())]['repo_path'];
   ...
   +      $repo_path = $this->info->getSetting('repo_path');
   ...
   +    $repo_path = escapeshellarg($this->getRepositoryPath($this->info->getSetting('repo_path')));
   ...
   +  protected function execAtGitRoot($cmd) {
   ...
   +    return $this->exec("cd $repo_path\n$cmd 2>&1");
   

   Way too complex. You store it as local variable but never use it. And pathes are passed around and checked with git status like crazy.

6. +++ b/test/tests/monitoring.core.test
   @@ -687,6 +663,104 @@ class MonitoringCoreTest extends MonitoringTestBase {
   +      $dir_cmd . "git status 2>&1" => array(
   ...
   +  function testGitSensor() {
   

   We cannot be sure git is just there. This needs a check first.

   The sensor needs to deal with missing git application in general and show an error message - for instance in the settings form.

Some moore feedback. :-)

1. +++ b/lib/Drupal/monitoring/Sensor/Sensors/SensorGit.php
   @@ -0,0 +1,298 @@
   +    return trim(shell_exec($cmd));
   ...
   +    $cmd_output = $this->execAtGitRoot('git branch');
   +    if (strpos($cmd_output, 'fatal') !== FALSE) {
   ...
   +    $cmd_output = $this->execAtGitRoot('git tag');
   +    if (strpos($cmd_output, 'fatal') !== FALSE) {
   

   That's not a reliable check. Use exec and check the third $return_val argument. It should be 0 in case of success. Otherwise, it's an error and i guess throwing an exception would make sense here.

2. +++ b/monitoring.drush.inc
   @@ -44,11 +44,11 @@ function monitoring_drush_command() {
   +      'show-execAtGitRoot-time' => 'Relevant for the table output listing all results. Will expand the table with execution time info.'
   

   Bad replace. :-)

We're in beta and the sensor is not ready yet.

Thus i committed the escape fix in #2176897: Escape shell commands
Make sure to rebase / reroll the patch.