Browse Source

Revert "[git_auth] Use pathless URL for cred helper"

This reverts commit ce47e785fae81261fb6b99a8ce123b37244485d3.

Reason for revert: fatal: --local can only be used inside a git
repository.

Original change's description:
> [git_auth] Use pathless URL for cred helper
>
> The credential helper rules should not have a path, while the URL
> rewrite rules should.
>
> Bug: b/401338175
> Change-Id: I79b52ab4af62367363617b2a9afa03a67f5ea0b9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6345631
> Commit-Queue: Allen Li <ayatane@chromium.org>
> Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>

Bug: b/401338175
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: Ifa887eeb7a7049665570e865431b41ac18649b90
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6357165
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Josip Sokcevic <sokcevic@chromium.org>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Josip Sokcevic 5 months ago
parent
commit
948edc4382
2 changed files with 13 additions and 45 deletions
  1. 7 41
      git_auth.py
  2. 6 4
      tests/git_auth_test.py

+ 7 - 41
git_auth.py

@@ -36,7 +36,7 @@ class ConfigChanger(object):
     #
     #
     # Increment this when making changes to the config, so that reliant
     # Increment this when making changes to the config, so that reliant
     # code can determine whether the config needs to be re-applied.
     # code can determine whether the config needs to be re-applied.
-    VERSION: int = 5
+    VERSION: int = 4
 
 
     def __init__(
     def __init__(
         self,
         self,
@@ -67,20 +67,12 @@ class ConfigChanger(object):
         return _url_shortname(parts)
         return _url_shortname(parts)
 
 
     @functools.cached_property
     @functools.cached_property
-    def _host_url(self) -> str:
-        # Example: https://chromium.googlesource.com
-        # Example: https://chromium-review.googlesource.com
-        parts: urllib.parse.SplitResult = urllib.parse.urlsplit(
-            self._remote_url)
-        return _url_host_url(parts)
-
-    @functools.cached_property
-    def _root_url(self) -> str:
+    def _base_url(self) -> str:
         # Example: https://chromium.googlesource.com/
         # Example: https://chromium.googlesource.com/
         # Example: https://chromium-review.googlesource.com/
         # Example: https://chromium-review.googlesource.com/
         parts: urllib.parse.SplitResult = urllib.parse.urlsplit(
         parts: urllib.parse.SplitResult = urllib.parse.urlsplit(
             self._remote_url)
             self._remote_url)
-        return _url_root_url(parts)
+        return parts._replace(path='/', query='', fragment='').geturl()
 
 
     @classmethod
     @classmethod
     def new_from_env(cls, cwd: str, cl: git_cl.Changelist) -> ConfigChanger:
     def new_from_env(cls, cwd: str, cl: git_cl.Changelist) -> ConfigChanger:
@@ -158,7 +150,7 @@ class ConfigChanger(object):
 
 
     def _apply_cred_helper(self, cwd: str) -> None:
     def _apply_cred_helper(self, cwd: str) -> None:
         """Apply config changes relating to credential helper."""
         """Apply config changes relating to credential helper."""
-        cred_key: str = f'credential.{self._host_url}.helper'
+        cred_key: str = f'credential.{self._base_url}.helper'
         if self.mode == ConfigMode.NEW_AUTH:
         if self.mode == ConfigMode.NEW_AUTH:
             self._set_config(cwd, cred_key, '', modify_all=True)
             self._set_config(cwd, cred_key, '', modify_all=True)
             self._set_config(cwd, cred_key, 'luci', append=True)
             self._set_config(cwd, cred_key, 'luci', append=True)
@@ -169,10 +161,6 @@ class ConfigChanger(object):
         else:
         else:
             raise TypeError(f'Invalid mode {self.mode!r}')
             raise TypeError(f'Invalid mode {self.mode!r}')
 
 
-        # Cleanup old from version 4
-        old_key: str = f'credential.{self._root_url}.helper'
-        self._set_config(cwd, old_key, None, modify_all=True)
-
     def _apply_sso(self, cwd: str) -> None:
     def _apply_sso(self, cwd: str) -> None:
         """Apply config changes relating to SSO."""
         """Apply config changes relating to SSO."""
         sso_key: str = f'url.sso://{self._shortname}/.insteadOf'
         sso_key: str = f'url.sso://{self._shortname}/.insteadOf'
@@ -184,7 +172,7 @@ class ConfigChanger(object):
             self._set_config(cwd, http_key, self._remote_url, modify_all=True)
             self._set_config(cwd, http_key, self._remote_url, modify_all=True)
         elif self.mode == ConfigMode.NEW_AUTH_SSO:
         elif self.mode == ConfigMode.NEW_AUTH_SSO:
             self._set_config(cwd, 'protocol.sso.allow', 'always')
             self._set_config(cwd, 'protocol.sso.allow', 'always')
-            self._set_config(cwd, sso_key, self._root_url, modify_all=True)
+            self._set_config(cwd, sso_key, self._base_url, modify_all=True)
             self._set_config(cwd, http_key, None, modify_all=True)
             self._set_config(cwd, http_key, None, modify_all=True)
         elif self.mode == ConfigMode.NO_AUTH:
         elif self.mode == ConfigMode.NO_AUTH:
             self._set_config(cwd, 'protocol.sso.allow', None)
             self._set_config(cwd, 'protocol.sso.allow', None)
@@ -208,7 +196,7 @@ class ConfigChanger(object):
 
 
     def _apply_global_cred_helper(self, cwd: str) -> None:
     def _apply_global_cred_helper(self, cwd: str) -> None:
         """Apply config changes relating to credential helper."""
         """Apply config changes relating to credential helper."""
-        cred_key: str = f'credential.{self._host_url}.helper'
+        cred_key: str = f'credential.{self._base_url}.helper'
         if self.mode == ConfigMode.NEW_AUTH:
         if self.mode == ConfigMode.NEW_AUTH:
             self._set_config(cwd, cred_key, '', scope='global', modify_all=True)
             self._set_config(cwd, cred_key, '', scope='global', modify_all=True)
             self._set_config(cwd, cred_key, 'luci', scope='global', append=True)
             self._set_config(cwd, cred_key, 'luci', scope='global', append=True)
@@ -223,10 +211,6 @@ class ConfigChanger(object):
         else:
         else:
             raise TypeError(f'Invalid mode {self.mode!r}')
             raise TypeError(f'Invalid mode {self.mode!r}')
 
 
-        # Cleanup old from version 4
-        old_key: str = f'credential.{self._root_url}.helper'
-        self._set_config(cwd, old_key, None, modify_all=True)
-
     def _apply_global_sso(self, cwd: str) -> None:
     def _apply_global_sso(self, cwd: str) -> None:
         """Apply config changes relating to SSO."""
         """Apply config changes relating to SSO."""
         sso_key: str = f'url.sso://{self._shortname}/.insteadOf'
         sso_key: str = f'url.sso://{self._shortname}/.insteadOf'
@@ -245,7 +229,7 @@ class ConfigChanger(object):
                              scope='global')
                              scope='global')
             self._set_config(cwd,
             self._set_config(cwd,
                              sso_key,
                              sso_key,
-                             self._root_url,
+                             self._base_url,
                              scope='global',
                              scope='global',
                              modify_all=True)
                              modify_all=True)
         elif self.mode == ConfigMode.NO_AUTH:
         elif self.mode == ConfigMode.NO_AUTH:
@@ -339,21 +323,3 @@ def _url_shortname(parts: urllib.parse.SplitResult) -> str:
     if name.endswith('-review'):
     if name.endswith('-review'):
         name = name[:-len('-review')]
         name = name[:-len('-review')]
     return name
     return name
-
-
-def _url_host_url(parts: urllib.parse.SplitResult) -> str:
-    """Format URL with host only (no path).
-
-    Example: https://chromium.googlesource.com
-    Example: https://chromium-review.googlesource.com
-    """
-    return parts._replace(path='', query='', fragment='').geturl()
-
-
-def _url_root_url(parts: urllib.parse.SplitResult) -> str:
-    """Format URL with root path.
-
-    Example: https://chromium.googlesource.com/
-    Example: https://chromium-review.googlesource.com/
-    """
-    return parts._replace(path='/', query='', fragment='').geturl()

+ 6 - 4
tests/git_auth_test.py

@@ -38,7 +38,7 @@ class TestConfigChanger(unittest.TestCase):
         ).apply('/some/fake/dir')
         ).apply('/some/fake/dir')
         want = {
         want = {
             '/some/fake/dir': {
             '/some/fake/dir': {
-                'credential.https://chromium.googlesource.com.helper':
+                'credential.https://chromium.googlesource.com/.helper':
                 ['', 'luci'],
                 ['', 'luci'],
                 'http.cookiefile': [''],
                 'http.cookiefile': [''],
                 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof':
                 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof':
@@ -89,7 +89,7 @@ class TestConfigChanger(unittest.TestCase):
         ).apply('/some/fake/dir')
         ).apply('/some/fake/dir')
         want = {
         want = {
             '/some/fake/dir': {
             '/some/fake/dir': {
-                'credential.https://chromium.googlesource.com.helper':
+                'credential.https://chromium.googlesource.com/.helper':
                 ['', 'luci'],
                 ['', 'luci'],
                 'http.cookiefile': [''],
                 'http.cookiefile': [''],
                 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof':
                 'url.https://chromium.googlesource.com/chromium/tools/depot_tools.git.insteadof':
@@ -160,7 +160,8 @@ class TestConfigChanger(unittest.TestCase):
             'https://chromium.googlesource.com/chromium/tools/depot_tools.git',
             'https://chromium.googlesource.com/chromium/tools/depot_tools.git',
         ).apply_global('/some/fake/dir')
         ).apply_global('/some/fake/dir')
         want = {
         want = {
-            'credential.https://chromium.googlesource.com.helper': ['', 'luci'],
+            'credential.https://chromium.googlesource.com/.helper':
+            ['', 'luci'],
         }
         }
         self.assertEqual(self.global_state, want)
         self.assertEqual(self.global_state, want)
 
 
@@ -190,7 +191,8 @@ class TestConfigChanger(unittest.TestCase):
         ).apply_global('/some/fake/dir')
         ).apply_global('/some/fake/dir')
         want = {
         want = {
             'protocol.sso.allow': ['always'],
             'protocol.sso.allow': ['always'],
-            'credential.https://chromium.googlesource.com.helper': ['', 'luci'],
+            'credential.https://chromium.googlesource.com/.helper':
+            ['', 'luci'],
         }
         }
         self.assertEqual(self.global_state, want)
         self.assertEqual(self.global_state, want)