Repository: repobee/repobee-sanitizer

Issue:
Enable pull request workflow
It would be a nice feature to be able to enable a pull request workflow, such that we can lock the master/main branch of the template repositories. I'm thinking that sooner or later, someone will push to the master branch in a template repository unless we lock it down.

The neatest thing would be if there was simply a `--pull-request` argument on the `sanitize repo` command, which instead of pushing to the target branch opens a PR to it. This will require some expansion of the API, but we can implement a prototype here first, and then figure out what we need the API to do.

@tohanss thoughts?

Discussion:
tohanss: The specific error we get is:

```python
 FileNotFoundError: [Errno 2] No such file or directory: '/tmp/pytest-of-wagmode/pytest-149/test_target_branch_not_modifie0/some-other-file.txt'
```
codecov[bot]: # [Codecov](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
> Merging [#137](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (a49e1b1) into [master](https://codecov.io/gh/repobee/repobee-sanitizer/commit/0bb30c7403eccb931649c2c7133d17d9ada5800a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) (0bb30c7) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137/graphs/tree.svg?width=650&height=150&src=pr&token=jndwxVbuCt&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

```diff
@@            Coverage Diff            @@
##            master      #137   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          253       268   +15     
  Branches        43        44    +1     
=========================================
+ Hits           253       268   +15     
```


| [Impacted Files](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [repobee\_sanitizer/\_sanitize\_repo.py](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cmVwb2JlZV9zYW5pdGl6ZXIvX3Nhbml0aXplX3JlcG8ucHk=) | `100.00% <100.00%> (ø)` | |
| [repobee\_sanitizer/sanitizer.py](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cmVwb2JlZV9zYW5pdGl6ZXIvc2FuaXRpemVyLnB5) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Last update [0bb30c7...a49e1b1](https://codecov.io/gh/repobee/repobee-sanitizer/pull/137?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).

tohanss: @slarse Ready for review
tohanss: Great feedback, just what i wanted :)
tohanss: @slarse Code and tests are finished, I made some comments you can look at, I am also going to go through the documentation and update that
tohanss: I also realized we should probably give the user the name of the PR branch so that they can checkout to it and push it
tohanss: @slarse I just realised the PR flag can now be used without the targetbranch option, can we make targetbranch required by the CLI or do we manually check that in our code?
tohanss: Also, i knew this was going to become a bit whacky because theres a chance that the tests create the timestamp for thr PR faster than the code executes, and  that just happened, i will find a way to optimise that a little bit
slarse: > The time stamp right now is the time in seconds since the "epoch" aka when your device started counting time from. I think seconds is what we want since someone can create multiple PRs within the same minute

I would make it a human-readable timestamp. Something like `str(datetime.datetime.now()).replace(" ", "_")`.

Is this ready for a looksie or are you still making changes?
tohanss: @slarse Yes! I will change the timestamp real quick right now first, please look at my comments along with the review

we can change the time to this but replace space with underscore
```python
datetime.now.strftime("%Y/%m/%d %H:%M:%S")
2021/07/15 10:28:56
```
tohanss: @slarse Checking input first fixed so many nuisances I had with the code, the main `command` function is so much cleaner now and I was able to structure everything the way i wanted it

The only remaining thing now that I'm not sure about is if we should push the pr branch for the user or tell them to checkout to the branch manually (which we do now)

Patch:
diff --git a/README.md b/README.md
index 55955f0..7605d56 100644
--- a/README.md
+++ b/README.md
@@ -241,6 +241,9 @@ $ repobee sanitize repo --target-branch main
 
 will sanitize the currently checked out branch (in this case `solutions`) and commit the result to the specified branch, in this case `main`. Successfully using the `--target-branch` feature allows us to essentially retain two concurrent repositories while only having to update one of them should any changes or improvements be made to our course tasks.
 
+#### Pull Requests
+If your target branch is the `main` branch of your repository, it can be quite intimidating to force commit any changes to that branch. Therefore, we have added the `--create-pr-branch` (or `-p`) flag to `Sanitizer` that will create a new branch from the one specified by `--target-branch` and sanitize to the new branch from where a pull request can be created. This way, you can lock your main branch on your git platform, to ensure that your code is safe when using `Sanitizer`
+
 #### Errors / Force
 `Sanitizer` does its best to ensure that nothing breaks while sanitizing. Therefore, when sanitizing a repo, `Sanitizer` will always make sure you have no uncommitted files in the repo as well as no syntax errors in your `Sanitizer` syntax. If you do have an error, `Sanitizer` will _not_ sanitize _anything_ until you fix any errors and run the command again. `Sanitizer` even prevents you from committing if no changes will be made to the repo!
 
diff --git a/repobee_sanitizer/_gitutils.py b/repobee_sanitizer/_gitutils.py
new file mode 100644
index 0000000..a83b32d
--- /dev/null
+++ b/repobee_sanitizer/_gitutils.py
@@ -0,0 +1,72 @@
+import pathlib
+import git
+
+from datetime import datetime
+
+import repobee_plug as plug
+
+
+class EmptyCommitError(plug.PlugError):
+    pass
+
+
+def create_pr_branch(repo_path: pathlib.Path, target_branch: str) -> str:
+    """Create a new branch from the target_branch that we sanitize to
+    and a pull request can be created from.
+
+    Args:
+        repo_path: Path to the repository
+        target_branch: The branch to create the pull request branch from
+
+    Returns:
+        The target branch name with an added timestamp
+    """
+    pr_branch_name = (
+        target_branch + "-pr-" + datetime.now().strftime("%Y/%m/%d_%H.%M.%S")
+    )
+
+    repo = git.Repo(str(repo_path))
+    repo.git.branch(pr_branch_name, target_branch)
+
+    return pr_branch_name
+
+
+def branch_exists(repo_path: pathlib.Path, target_branch: str) -> bool:
+    repo = git.Repo(str(repo_path))
+
+    existing_branches = [branch.name for branch in repo.branches]
+
+    return target_branch in existing_branches
+
+
+def _git_commit_on_branch(
+    repo_root: pathlib.Path, target_branch: str, commit_message: str
+):
+    repo = git.Repo(str(repo_root))
+    repo.git.symbolic_ref("HEAD", f"refs/heads/{target_branch}")
+    repo.git.add(".", "--force")
+    try:
+        repo.git.commit("-m", commit_message)
+    except git.GitCommandError as exc:
+        assert "nothing to commit, working tree clean" in str(exc)
+        raise EmptyCommitError() from exc
+
+
+def _git_fetch(
+    src_repo_path: pathlib.Path,
+    src_branch: str,
+    dst_repo_path: pathlib.Path,
+    dst_branch: str,
+):
+    dst_repo = git.Repo(str(dst_repo_path))
+    src_repo_uri = f"file://{src_repo_path.absolute()}"
+    dst_repo.git.fetch(src_repo_uri, f"{src_branch}:{dst_branch}")
+
+
+def _clean_repo(repo_path: pathlib.Path):
+    """Resets working tree and index to HEAD. This is to untracked files as
+    well as uncommitted changes.
+    """
+    repo = git.Repo(str(repo_path))
+    repo.git.reset("--hard")
+    repo.git.clean("-dfx")
diff --git a/repobee_sanitizer/_sanitize_repo.py b/repobee_sanitizer/_sanitize_repo.py
index 487d769..e0fd11b 100644
--- a/repobee_sanitizer/_sanitize_repo.py
+++ b/repobee_sanitizer/_sanitize_repo.py
@@ -13,17 +13,19 @@ import repobee_plug as plug
 import daiquiri
 import git
 
-from repobee_sanitizer import _sanitize, _fileutils, _syntax, _format
+from repobee_sanitizer import (
+    _sanitize,
+    _fileutils,
+    _syntax,
+    _format,
+    _gitutils,
+)
 
 PLUGIN_NAME = "sanitizer"
 
 LOGGER = daiquiri.getLogger(__file__)
 
 
-class EmptyCommitError(plug.PlugError):
-    pass
-
-
 def check_repo_state(repo_root) -> Optional[str]:
     try:
         repo = git.Repo(repo_root)
@@ -108,16 +110,19 @@ def sanitize_to_target_branch(
     Returns:
         List of errors if any errors are found, otherwise an empty list.
     """
+
     with tempfile.TemporaryDirectory() as tmpdir:
         repo_copy_path = pathlib.Path(tmpdir) / "repo"
         shutil.copytree(src=repo_path, dst=repo_copy_path)
-        _clean_repo(repo_copy_path)
+        _gitutils._clean_repo(repo_copy_path)
         file_relpaths = discover_dirty_files(repo_copy_path)
         errors = sanitize_files(repo_copy_path, file_relpaths)
         if errors:
             return errors
-        _git_commit_on_branch(repo_copy_path, target_branch, commit_message)
-        _git_fetch(
+        _gitutils._git_commit_on_branch(
+            repo_copy_path, target_branch, commit_message
+        )
+        _gitutils._git_fetch(
             src_repo_path=repo_copy_path,
             src_branch=target_branch,
             dst_repo_path=repo_path,
@@ -125,36 +130,3 @@ def sanitize_to_target_branch(
         )
 
     return []
-
-
-def _clean_repo(repo_path: pathlib.Path):
-    """Resets working tree and index to HEAD. This is to untracked files as
-    well as uncommitted changes.
-    """
-    repo = git.Repo(str(repo_path))
-    repo.git.reset("--hard")
-    repo.git.clean("-dfx")
-
-
-def _git_commit_on_branch(
-    repo_root: pathlib.Path, target_branch: str, commit_message: str
-):
-    repo = git.Repo(str(repo_root))
-    repo.git.symbolic_ref("HEAD", f"refs/heads/{target_branch}")
-    repo.git.add(".", "--force")
-    try:
-        repo.git.commit("-m", commit_message)
-    except git.GitCommandError as exc:
-        assert "nothing to commit, working tree clean" in str(exc)
-        raise EmptyCommitError() from exc
-
-
-def _git_fetch(
-    src_repo_path: pathlib.Path,
-    src_branch: str,
-    dst_repo_path: pathlib.Path,
-    dst_branch: str,
-):
-    dst_repo = git.Repo(str(dst_repo_path))
-    src_repo_uri = f"file://{src_repo_path.absolute()}"
-    dst_repo.git.fetch(src_repo_uri, f"{src_branch}:{dst_branch}")
diff --git a/repobee_sanitizer/sanitizer.py b/repobee_sanitizer/sanitizer.py
index 15d81ea..9791d9c 100644
--- a/repobee_sanitizer/sanitizer.py
+++ b/repobee_sanitizer/sanitizer.py
@@ -16,6 +16,7 @@ from repobee_sanitizer import (
     _format,
     _sanitize_repo,
     _fileutils,
+    _gitutils,
 )
 
 PLUGIN_NAME = "sanitizer"
@@ -62,14 +63,20 @@ class SanitizeRepo(plug.Plugin, plug.cli.Command):
         ),
         __required__=True,
     )
+    create_pr_branch = plug.cli.flag(
+        short_name="-p",
+        help=(
+            "create and commit to a special branch 'sanitizer-pull-request'"
+            "to safely merge using pull requests"
+        ),
+    )
 
     def command(self) -> Optional[plug.Result]:
         repo_root = self.repo_root.absolute()
-        message = _sanitize_repo.check_repo_state(repo_root)
-        if message and not self.force:
-            return plug.Result(
-                name="sanitize-repo", msg=message, status=plug.Status.ERROR
-            )
+
+        input_error = self._validate_input(repo_root)
+        if input_error:
+            return input_error
 
         if self.no_commit:
             LOGGER.info("Executing dry run")
@@ -77,11 +84,14 @@ class SanitizeRepo(plug.Plugin, plug.cli.Command):
             errors = _sanitize_repo.sanitize_files(repo_root, file_relpaths)
         else:
             LOGGER.info(f"Sanitizing repo and updating {self.target_branch}")
+            effective_target_branch = self._resolve_effective_target_branch(
+                repo_root
+            )
             try:
                 errors = _sanitize_repo.sanitize_to_target_branch(
-                    repo_root, self.target_branch, self.commit_message
+                    repo_root, effective_target_branch, self.commit_message
                 )
-            except _sanitize_repo.EmptyCommitError:
+            except _gitutils.EmptyCommitError:
                 return plug.Result(
                     name="sanitize-repo",
                     msg="No diff between target branch and sanitized output. "
@@ -96,12 +106,47 @@ class SanitizeRepo(plug.Plugin, plug.cli.Command):
                 status=plug.Status.ERROR,
             )
 
+        result_message = "Successfully sanitized repo" + (
+            f" to pull request branch\n\nrun 'git switch "
+            f"{effective_target_branch}' to checkout the branch"
+            if self.create_pr_branch
+            else ""
+        )
+
         return plug.Result(
             name="sanitize-repo",
-            msg="Successfully sanitized repo",
+            msg=result_message,
             status=plug.Status.SUCCESS,
         )
 
+    def _resolve_effective_target_branch(self, repo_root: pathlib.Path) -> str:
+        if self.create_pr_branch:
+            return _gitutils.create_pr_branch(repo_root, self.target_branch)
+        return self.target_branch
+
+    def _validate_input(self, repo_root) -> plug.Result:
+        message = _sanitize_repo.check_repo_state(repo_root)
+        if message and not self.force:
+            return plug.Result(
+                name="sanitize-repo", msg=message, status=plug.Status.ERROR
+            )
+
+        if self.create_pr_branch:
+            if not self.target_branch:
+                return plug.Result(
+                    name="sanitize-repo",
+                    msg="Can not create a pull request without a target "
+                    "branch, please specify --target-branch",
+                    status=plug.Status.ERROR,
+                )
+            elif not _gitutils.branch_exists(repo_root, self.target_branch):
+                return plug.Result(
+                    name="sanitize-repo",
+                    msg=f"Can not create a pull request branch from "
+                    f"non-existing target branch {self.target_branch}",
+                    status=plug.Status.ERROR,
+                )
+
 
 class SanitizeFile(plug.Plugin, plug.cli.Command):
     __settings__ = plug.cli.command_settings(


Test patch:
diff --git a/tests/test_ext_commands.py b/tests/test_ext_commands.py
index 2c9246a..5e385a1 100644
--- a/tests/test_ext_commands.py
+++ b/tests/test_ext_commands.py
@@ -7,6 +7,8 @@ import shlex
 import pytest
 import git
 
+from datetime import datetime
+
 import repobee
 from repobee_sanitizer import sanitizer, _fileutils
 import repobee_plug as plug
@@ -123,6 +125,105 @@ class TestSanitizeRepo:
         fake_repo.repo.git.reset("--hard")
         assert_expected_text_in_files(fake_repo.file_infos)
 
+    def test_sanitize_to_pull_request_branch(
+        self, sanitizer_config, fake_repo
+    ):
+        """Test that sanitizer will commit to a secondary branch from where a
+        pull request can be created
+        """
+        target_branch = "student-version"
+
+        fake_repo.repo.git.checkout(b=target_branch)
+        fake_repo.repo.git.commit("--allow-empty", "-m", "initial commit")
+        fake_repo.repo.git.checkout(fake_repo.default_branch)
+
+        pr_branch_name = (
+            target_branch
+            + "-pr-"
+            + datetime.now().strftime("%Y/%m/%d_%H.%M.%S")
+        )
+
+        result = run_repobee(
+            f"sanitize repo --target-branch {target_branch} -p".split(),
+            workdir=fake_repo.path,
+        )
+
+        fake_repo.repo.git.checkout(pr_branch_name)
+        fake_repo.repo.git.reset("--hard")
+
+        assert_expected_text_in_files(fake_repo.file_infos)
+
+        assert (
+            result.status == plug.Status.SUCCESS
+            and f"Successfully sanitized repo to pull request branch\n\nrun "
+            f"'git switch {pr_branch_name}' to checkout the branch"
+            in result.msg
+        )
+
+    def test_target_branch_not_modified_by_pr(
+        self, sanitizer_config, fake_repo
+    ):
+        """Test that sanitizer doesnt modify the target branch when
+        sanitizing to PR branch
+        """
+        target_branch = "student-version"
+
+        fake_repo.repo.git.checkout(b=target_branch)
+
+        other_file_contents = (
+            "Some boring\ncontents in a non-interesting\nfile"
+        )
+        other_file = fake_repo.path / "some-other-file.txt"
+        other_file.write_text(other_file_contents)
+        fake_repo.repo.git.add(str(other_file))
+        fake_repo.repo.git.commit("-m", "'Add other file'")
+
+        fake_repo.repo.git.checkout(fake_repo.default_branch)
+
+        run_repobee(
+            f"sanitize repo --target-branch {target_branch} -p".split(),
+            workdir=fake_repo.path,
+        )
+
+        fake_repo.repo.git.checkout(target_branch)
+        other_file_contents_after = other_file.read_text(encoding="utf8")
+        assert other_file_contents == other_file_contents_after
+
+    def test_pr_from_empty_target_branch_return_error(
+        self, sanitizer_config, fake_repo
+    ):
+        """Test that sanitizer returns a plugResult error if there are no
+        commits on the target_branch.
+        """
+        target_branch = "student-version"
+
+        result = run_repobee(
+            f"sanitize repo --target-branch {target_branch} -p".split(),
+            workdir=fake_repo.path,
+        )
+
+        assert (
+            result.status == plug.Status.ERROR
+            and f"Can not create a pull request branch from non-existing "
+            f"target branch {target_branch}" in result.msg
+        )
+
+    def test_pr_without_target_branch_fails(self, sanitizer_config, fake_repo):
+        """Test that sanitizer returns the correct error if the user tries
+        using --create-pull-request without specifying a --target-branch,
+        right now this only happens if the user specifies --no-commit
+        """
+        result = run_repobee(
+            "sanitize repo --no-commit -p".split(),
+            workdir=fake_repo.path,
+        )
+
+        assert (
+            result.status == plug.Status.ERROR
+            and "Can not create a pull request without a target "
+            "branch, please specify --target-branch" in result.msg
+        )
+
     def test_no_commit_default_root(self, sanitizer_config, fake_repo):
 
         run_repobee(