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

Stop advertising broken datalad -c :<key> to unset config #7589

Open
mih opened this issue May 7, 2024 · 1 comment
Open

Stop advertising broken datalad -c :<key> to unset config #7589

mih opened this issue May 7, 2024 · 1 comment

Comments

@mih
Copy link
Member

mih commented May 7, 2024

datalad/datalad-next#677 highlights some problems from the perspective of implementations in datalad-next.

However, this feature is broken and broken in a way that cannot be addressed cleanly and comprehensively, because Git itself does not support temporary unsetting of config items (only temporary overrides).

The current promise is:

  -c (:name|name=value)
     specify configuration setting overrides. They override any
     configuration read from a file. A configuration can also be unset
     temporarily by prefixing its name with a colon (':'), e.g.
     ':user.name'. Overrides specified here may be overridden themselves
     by configuration settings declared as environment variables.

But the correct description is more like:

A configuration can also be unset temporarily by prefixing its name
with a colon (':'), e.g. ':user.name'. However, this is only in effect
for configuration queries via DataLad's `ConfigManager`, and only in the
root process. Therefore this approach will generally not be effective
for impacting the behavior of Git, git-annex, or DataLad subprocesses
(special remotes).  

I suggest to adjust the documentation now and also deprecate this feature because of the limited scope and large potential to cause confusion and undesired behavior.

@christian-monch
Copy link
Contributor

That is a good suggestion from my point of view

mih pushed a commit to mih/datalad-next that referenced this issue May 14, 2024
This violates the promises made in the type annotation of this
function. However, it is necessary (at least for now) in order
to regain compatibility with an (anti-)feature of the DataLad
CLI.

The underlying issue is documented in
datalad/datalad#7589

TL;DR: DataLad promises to be able to unset a git config
item -- but this is not supported by Git, and also DataLad
can only achieve this within a single, top-level process.

With this commit `None` value (e.g. coming from the CLI)
is treated as an empty string. It also ensures that all
values are strings instead of, for example,
`int`.

It adds a paragraph to the doc-string of
`datalad_next.config.utils.set_gitconfig_items_in_env`
to document its behavior if a configuration item
has a `None`-value.

This isssue has been present since the introduction of this
function, but has only been discovered now, because test
setup did not reliably load extensions, hence this datalad-next
code did not always run.

Ping datalad#677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants