Add additional validation for Git and SVN repos.

Review Request #13223 — Created Aug. 18, 2023 and submitted

Information

Review Board
release-6.x

Reviewers

We have two cases that have come up semi-frequently, tripping up users
when they're trying to configure their repositories:

  1. Users were putting in a raw file path to an SVN repository, where it
    needs a file:// URL.
  2. When configuring a Git repo, they'd enter their SVN clone address but
    leave out the raw file URL. This one is especially confusing because
    a lot of people expect Git to just work, and don't read our
    documentation about why it doesn't.

This change adds new repository form subclasses that can validate these
conditions.

  • Verified that I wasn't able to save an SVN repo pointing to a bare
    filesystem path, but could with file:// and https://
  • Verified that trying to save a Git repo with a path but no raw file
    URL would fail.
  • Made sure that saving with a raw file URL or Git repos on a hosting
    service still worked correctly.
  • Ran unit tests.
Summary ID
Add additional validation for Git and SVN repos.
We have two cases that have come up semi-frequently, tripping up users when they're trying to configure their repositories: 1. Users were putting in a raw file path to an SVN repository, where it needs a `file://` URL. 2. When configuring a Git repo, they'd enter their SVN clone address but leave out the raw file URL. This one is especially confusing because a lot of people expect Git to just work, and don't read our documentation about why it doesn't. This change adds new repository form subclasses that can validate these conditions. Testing Done: - Verified that I wasn't able to save an SVN repo pointing to a bare filesystem path, but could with file:// and https:// - Verified that trying to save a Git repo with a path but no raw file URL would fail. - Made sure that saving with a raw file URL or Git repos on a hosting service still worked correctly. - Ran unit tests.
5c5dc96592a51b47e86c245a192aa15e3e05de85
Description From Last Updated

continuation line unaligned for hanging indent Column: 23 Error code: E131

reviewbotreviewbot

Missing Version Added.

chipx86chipx86

This is missing typing.

chipx86chipx86

We should also allow file: schemes.

chipx86chipx86

Let's link to the relevant docs here.

chipx86chipx86

Missing Version Added.

chipx86chipx86

This is missing typing.

chipx86chipx86

Missing blank line.

chipx86chipx86

Should add a -> None so we can opt into typing. Also, let's add tests for supported schemes, just to …

chipx86chipx86

Should add a -> None so we can opt into typing. Also, let's add tests for other schemes, just to …

chipx86chipx86

No . in unit test docstrings.

chipx86chipx86

Leftover debug output.

chipx86chipx86

line too long (83 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

local variable 'x' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'y' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (80 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

This should be typed as Dict[str, Any].

chipx86chipx86

This should be typed as Dict[str, Any].

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/scmtools/git.py (Diff revision 2)
     
     
    Show all issues

    Missing Version Added.

  3. reviewboard/scmtools/git.py (Diff revision 2)
     
     
    Show all issues

    This is missing typing.

  4. reviewboard/scmtools/git.py (Diff revision 2)
     
     
    Show all issues

    We should also allow file: schemes.

  5. reviewboard/scmtools/git.py (Diff revision 2)
     
     
    Show all issues

    Let's link to the relevant docs here.

  6. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
    Show all issues

    Missing Version Added.

  7. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
    Show all issues

    This is missing typing.

  8. reviewboard/scmtools/svn/__init__.py (Diff revision 2)
     
     
     
    Show all issues

    Missing blank line.

  9. reviewboard/scmtools/tests/test_git.py (Diff revision 2)
     
     
    Show all issues

    Should add a -> None so we can opt into typing.

    Also, let's add tests for supported schemes, just to make sure we don't ever regress.

  10. reviewboard/scmtools/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    Should add a -> None so we can opt into typing.

    Also, let's add tests for other schemes, just to make sure we don't ever regress that.

  11. reviewboard/scmtools/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    No . in unit test docstrings.

  12. reviewboard/scmtools/tests/test_svn.py (Diff revision 2)
     
     
    Show all issues

    Leftover debug output.

  13. 
      
david
Review request changed
Commits:
Summary ID
Add additional validation for Git and SVN repos.
We have two cases that have come up semi-frequently, tripping up users when they're trying to configure their repositories: 1. Users were putting in a raw file path to an SVN repository, where it needs a `file://` URL. 2. When configuring a Git repo, they'd enter their SVN clone address but leave out the raw file URL. This one is especially confusing because a lot of people expect Git to just work, and don't read our documentation about why it doesn't. This change adds new repository form subclasses that can validate these conditions. Testing Done: - Verified that I wasn't able to save an SVN repo pointing to a bare filesystem path, but could with file:// and https:// - Verified that trying to save a Git repo with a path but no raw file URL would fail. - Made sure that saving with a raw file URL or Git repos on a hosting service still worked correctly. - Ran unit tests.
e8e46b1bfd3ee093f855ba77e0f88d7880a04464
Add additional validation for Git and SVN repos.
We have two cases that have come up semi-frequently, tripping up users when they're trying to configure their repositories: 1. Users were putting in a raw file path to an SVN repository, where it needs a `file://` URL. 2. When configuring a Git repo, they'd enter their SVN clone address but leave out the raw file URL. This one is especially confusing because a lot of people expect Git to just work, and don't read our documentation about why it doesn't. This change adds new repository form subclasses that can validate these conditions. Testing Done: - Verified that I wasn't able to save an SVN repo pointing to a bare filesystem path, but could with file:// and https:// - Verified that trying to save a Git repo with a path but no raw file URL would fail. - Made sure that saving with a raw file URL or Git repos on a hosting service still worked correctly. - Ran unit tests.
ac2dc56a105bce0353bc5a0474b9bba953c374dd

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/scmtools/git.py (Diff revision 4)
     
     
    Show all issues

    This should be typed as Dict[str, Any].

  3. reviewboard/scmtools/svn/__init__.py (Diff revision 4)
     
     
    Show all issues

    This should be typed as Dict[str, Any].

  4. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to release-6.x (bcc85a0)