trilbymedia / grav-plugin-git-sync

Collaboratively Synchronize your Grav `user` folder hosted on GitHub, BitBucket or GitLab
Apache License 2.0
243 stars 58 forks source link

Error displayed when "saving" a page without changes, while there are untracked/removed files #101

Closed ScottHamper closed 6 years ago

ScottHamper commented 6 years ago

Summary

When an untracked/removed file exists outside of the directories synchronized by GitSync, an error message will be displayed when a user tries to save a page in the admin interface without making any changes.

Steps to Reproduce

  1. Build the site/checkout the synced repository containing all user sub-folders/files
  2. Create a file directly in the filesystem (not through the admin interface) under /user
  3. Log into the Grav admin interface
  4. Open the editor for a page, and click "Save" without making any changes to the page

Expected Result

A banner appears at the top of the page with the text "Successfully saved". No errors are displayed.

Actual Result

A banner appears at the top of the page with the text "Successfully saved". Immediately below that, an error message is displayed with the output of the git commit cli command.

Thoughts

Here's the sequence of interactions that I believe are causing this behavior:

  1. After save, a call to isWorkingCopyClean in the synchronize method returns false because of the untracked/removed files.
  2. As a result, commit in GitSync.php is called, which executes the git commit cli command.
  3. The return value of git commit is 1 because no changes have been added to the commit (because there aren't any changes we actually want to commit).
  4. A guard clause in the execute method notices the non-zero return value and throws an exception, causing the git commit output to be displayed as an error message in the admin interface.

To resolve this issue, we should implement a more robust way of determining whether or not a commit should take place. The implementation of isWorkingCopyClean is technically correct, but is really only a heuristic for whether or not there may be changes to commit. Commits should only be attempted if there has been a change in one of the $this->config['folders'] paths, as those are the only changes that will be git added by the plugin.

One option would be to modify/replace isWorkingCopyClean (or create another method similar to it) that appends all of the tracked directory paths to the end of the git status command, similar to the add method:

public function hasChangesToCommit()
{
    $folders = $this->config['folders'];
    $paths = [];

    foreach ($folders as $folder) {
        $paths[] = $folder;
    }

    $message = 'nothing to commit';
    $output = $this->execute('status ' . implode(' ', $paths));

    return (substr($output[count($output)-1], 0, strlen($message)) !== $message);
}

Disclaimer - I assembled the above in free-hand and have not tested it. It is intended for communicating an idea only; make sure to verify correctness, if you decide to use it.

w00fz commented 6 years ago

Thanks @ScottHamper i now merged this!