Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discontinue ConfigManager abuse for Git identity warning #7378

Merged
merged 1 commit into from
May 24, 2023

Conversation

mih
Copy link
Member

@mih mih commented Apr 29, 2023

DataLad warns about an undefined Git identity (user.name|email). It does that whether or not any Git identity is actually necessary or even anyhow considered. The ConfigManager constructor is (ab)used to trigger this warning, and a ConfigManager class member is used to prevent repeated warnings.

This changeset maintains the exact same behavior (warn on datalad import), but moves the associated code out of the ConfigManager class. This class is not involved in any Git repository manipulations. The warning is trigger in datalad/__init__.py directly, hence no protection against repeated execution is needed (Python does it automatically).

DataLad warns about an undefined Git identity (user.name|email).
It does that whether or not any Git identity is actually necessary
or even anyhow considered. The `ConfigManager` constructor is
(ab)used to trigger this warning, and a `ConfigManager` class
member is used to prevent repeated warnings.

This changeset maintains the exact same behavior (warn on `datalad
import`), but move the associated code out of the `ConfigManager`
class. This class is not involved in any Git repository
manipulations. The warning is trigger in `datalad/__init__.py`
directly, hence no protection against repeated execution is needed
(Python does it automatically).
@yarikoptic yarikoptic added the semver-internal Changes only affect the internal API label May 24, 2023
@yarikoptic
Copy link
Member

I am fine with this RFing although something nags me about it (as some scenario could cause it to trigger twice) but I can't come up with "how" so must be just wrong suspicion. Let's proceed. But if you @mih could clarify on what was the trigger/reason (besides the "desire for purity") which motivated this change -- would be appreciated.

@yarikoptic yarikoptic merged commit 68cb600 into datalad:maint May 24, 2023
@yarikoptic
Copy link
Member

dang -- I have missed that CI was not happy since

a test needed adjustment for this change
❯ python -m pytest -s -v -vv datalad/tests/test_base.py::test_git_config_warning
================================================ test session starts ================================================
platform linux -- Python 3.11.2, pytest-7.2.1, pluggy-1.0.0 -- /home/yoh/proj/datalad/datalad-maint/venvs/dev3/bin/python
cachedir: .pytest_cache
rootdir: /home/yoh/proj/datalad/datalad-maint, configfile: tox.ini
plugins: xdist-3.1.0, fail-slow-0.3.0, cov-4.0.0
collected 1 item                                                                                                    

datalad/tests/test_base.py::test_git_config_warning FAILEDVersions: annexremote=1.6.0 boto=2.49.0 cmd:7z=16.02 cmd:annex=10.20230126 cmd:bundled-git=UNKNOWN cmd:git=2.39.2 cmd:ssh=9.2p1 cmd:system-git=2.39.2 cmd:system-ssh=9.2p1 datalad=0.18.4+10.ge78c1df21 humanize=4.5.0 iso8601=1.1.0 keyring=23.13.1 keyrings.alt=UNKNOWN msgpack=1.0.4 platformdirs=2.6.2 requests=2.28.2
Obscure filename: str=b' |;&%b5{}\'"<>\xce\x94\xd0\x99\xd7\xa7\xd9\x85\xe0\xb9\x97\xe3\x81\x82 .datc ' repr=' |;&%b5{}\'"<>ΔЙקم๗あ .datc '
Encodings: default='utf-8' filesystem='utf-8' locale.prefered='UTF-8'
Environment: PATH='/home/yoh/proj/datalad/datalad-maint/venvs/dev3/bin:/home/yoh/gocode/bin:/home/yoh/gocode/bin:/home/yoh/bin:/home/yoh/.local/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/games:/usr/games:/sbin:/usr/sbin:/usr/local/sbin' LANG='en_US.UTF-8' GIT_PAGER='less --no-init --quit-if-one-screen' GIT_CONFIG_PARAMETERS="'init.defaultBranch=dl-test-branch' 'clone.defaultRemoteName=dl-test-remote'" PYTHON_KEYRING_BACKEND='keyrings.alt.file.PlaintextKeyring' GIT_CONFIG_GLOBAL='/home/yoh/.tmp/datalad_temp_1q9bmci7/.gitconfig' GIT_ASKPASS='true'


===================================================== FAILURES ======================================================
______________________________________________ test_git_config_warning ______________________________________________

path = '/home/yoh/.tmp/datalad_temp_tree_test_git_config_warningz01l_v03'

    @with_tree(tree={})
    def test_git_config_warning(path=None):
        if 'GIT_AUTHOR_NAME' in os.environ:
            raise SkipTest("Found existing explicit identity config")
    
        # Note: An easier way to test this, would be to just set GIT_CONFIG_GLOBAL
        # to point somewhere else. However, this is not supported by git before
        # 2.32. Hence, stick with changed HOME in this test, but be sure to unset a
        # possible GIT_CONFIG_GLOBAL in addition.
    
        patched_env = os.environ.copy()
        patched_env.pop('GIT_CONFIG_GLOBAL', None)
        patched_env.update(get_home_envvars(path))
        with chpwd(path), \
                patch.dict('os.environ', patched_env, clear=True), \
                swallow_logs(new_level=30) as cml:
            # no configs in that empty HOME
            from datalad.config import ConfigManager
            # reach into the class and disable the "checked" flag that
            # has already been tripped before we get here
>           with patch.object(ConfigManager, "_checked_git_identity", False):

datalad/tests/test_base.py:90: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
/usr/lib/python3.11/unittest/mock.py:1437: in __enter__
    original, local = self.get_original()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <unittest.mock._patch object at 0x7f92566b8b50>

    def get_original(self):
        target = self.getter()
        name = self.attribute
    
        original = DEFAULT
        local = False
    
        try:
            original = target.__dict__[name]
        except (AttributeError, KeyError):
            original = getattr(target, name, DEFAULT)
        else:
            local = True
    
        if name in _builtins and isinstance(target, ModuleType):
            self.create = True
    
        if not self.create and original is DEFAULT:
>           raise AttributeError(
                "%s does not have the attribute %r" % (target, name)
            )
E           AttributeError: <class 'datalad.config.ConfigManager'> does not have the attribute '_checked_git_identity'

/usr/lib/python3.11/unittest/mock.py:1410: AttributeError
============================================== short test summary info ==============================================
FAILED datalad/tests/test_base.py::test_git_config_warning - AttributeError: <class 'datalad.config.ConfigManager'> does not have the attribute '_checked_git_identity'
================================================= 1 failed in 0.26s =================================================

@yarikoptic
Copy link
Member

to reduce effect on other PRs -- I have hard-reset history killing the merge of this PR, have pushed a tentative fix to this branch, and reincarnating the PR now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-internal Changes only affect the internal API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants