does not understand "yes" as boolean true value for autoenable

Please describe the problem.

git takes "Boolean true literals are yes, on, true, and 1."

IMHO git-annex should centrally/uniformly handle all boolean values for all configuration and remote settings.

majority of the boolean variable settings e.g. for special remotes in examples etc use yes. But examples with autoenable seems to use true and having yes causes git-annex to not autoenable it:

$> (set -x ; ds=ds002396; rm -rf $ds; git clone https://github.com/OpenNeuroDatasets/$ds.git && git -C $ds show origin/git-annex:remote.log && git -C $ds annex init && git -C $ds annex info | grep -i s3-public )
+zsh:194> ds=ds002396 
+zsh:194> rm -rf ds002396
+zsh:194> git clone https://github.com/OpenNeuroDatasets/ds002396.git
Cloning into 'ds002396'...
remote: Enumerating objects: 93, done.
remote: Counting objects: 100% (93/93), done.
remote: Compressing objects: 100% (56/56), done.
remote: Total 93 (delta 19), reused 93 (delta 19), pack-reused 0
Receiving objects: 100% (93/93), 9.30 KiB | 414.00 KiB/s, done.
Resolving deltas: 100% (19/19), done.
+zsh:194> git -C ds002396 show origin/git-annex:remote.log
6b554bb8-47cc-4f88-b3f0-b4becdb80272 autoenable=yes bucket=openneuro.org datacenter=US encryption=none exporttree=yes fileprefix=ds002396/ host=s3.amazonaws.com name=s3-PUBLIC partsize=1GiB port=80 public=yes publicurl=http://openneuro.org.s3.amazonaws.com/ storageclass=STANDARD type=S3 versioning=yes timestamp=1579150706.611377263s
+zsh:194> git -C ds002396 annex init
init  (merging origin/git-annex into git-annex...)
(recording state in git...)

  Remote origin not usable by git-annex; setting annex-ignore
ok
(recording state in git...)
+zsh:194> git -C ds002396 annex info
+zsh:194> grep '--color=auto' -d skip -i s3-public
    6b554bb8-47cc-4f88-b3f0-b4becdb80272 -- s3-PUBLIC

whenever it autoenables if I explicitly replace yes with true here:

$> ds=ds002396; rm -rf $ds*; ( git clone https://github.com/OpenNeuroDatasets/$ds.git && cd $ds && git checkout git-annex && sed -i -e 's,autoenable=yes,autoenable=true,g' remote.log && git commit -m "fix yes to true" -a && git checkout master; ) && git clone $ds $ds-clone && git -C $ds-clone annex init
Cloning into 'ds002396'...
remote: Enumerating objects: 93, done.
remote: Counting objects: 100% (93/93), done.
remote: Compressing objects: 100% (56/56), done.
remote: Total 93 (delta 19), reused 93 (delta 19), pack-reused 0
Receiving objects: 100% (93/93), 9.30 KiB | 4.65 MiB/s, done.
Resolving deltas: 100% (19/19), done.
CHANGES  dataset_description.json  sub-01/  sub-02/
Branch 'git-annex' set up to track remote branch 'git-annex' from 'origin'.
Switched to a new branch 'git-annex'
[git-annex 3770aed] fix yes to true
 1 file changed, 1 insertion(+), 1 deletion(-)
Switched to branch 'master'
Your branch is up to date with 'origin/master'.
Cloning into 'ds002396-clone'...
done.
init  (merging origin/git-annex into git-annex...)
(recording state in git...)
(Auto enabling special remote s3-PUBLIC...)
ok
(recording state in git...)

FWIW -- above examples are with git annex 7.20190819+git2-g908476a9b-1~ndall+1 but initially investigated using 8.20200330+git28-g7ebc11877-1~ndall+1

References: initial report on neurostars

RSS Atom

comment 1

As of 7.20200202.7, initremote and enableremote will refuse to accept unknown values:

$ git annex initremote d type=directory directory=/$PWD../dir/ autoenable=yes encryption=none
initremote d                                         
git-annex: Bad value for autoenable (expected true or false)
failed
git-annex: initremote: 1 failed

Here's your todo related to those changes.

Rejecting invalid values isn't the more liberal casting of boolean values that you're asking for here, but the above behavior would stop an initremote caller from misconfiguring the remote, preventing cases like the report you link to.

Assuming that taking multiple values for a boolean is a good thing in general, it seems like it'd be problematic to switch special remote parameters over to this behavior. The stored config is used when enabling it in other repos, so an older version of git-annex would see, say, autoenable=yes and not treat it as autoenable=true. To avoid that, I suppose git-annex could map the value onto the currently accepted form when storing the config, but I'd guess at that point support for multiple ways to say true isn't really worth the trade-off.

Comment by kyle — Tue Apr 7 15:30:54 2020

Remove comment

comment 1

Yes, true, git-annex has some inconsistent parsing of these.

It's certianly at least a bug worth fixing that the Git.Config parser only supports "true" and not "1" and "on". (And IIRC git-config also has a special case for true boolean settings that has only the setting in the config file, without a value -- which git-annex also may not parse.)

On special remotes, Kyle's right. Same reasoning also applies to booleans set by git-annex config. I don't immediately see any good way to add a translation layer to either when things are set, it would be an ugly addition in both cases.

I don't feel git-annex necessarily needs to mimic git here in accepting all those things. It's well known that not all of git's UI choices are good, and git-annex does not mimic all of them, eg git has some very nasty positional --switch parsing.

But readonly and autoenable using true/false while all other special remote configs uses yes/no is not good UI either.

Comment by joey — Tue Apr 7 17:28:52 2020

Remove comment

comment 3

I don't feel git-annex necessarily needs to mimic git here in accepting all those things. It's well known that not all of git's UI choices are good, and git-annex does not mimic all of them, eg git has some very nasty positional --switch parsing.

I would not over-generalize this issue to all possible git UI bad decisions... Talking about configuration boolean options, given the:

But readonly and autoenable using true/false while all other special remote configs uses yes/no is not good UI either.

IMHO staying inline with git would bring desired consistent (even if undesired flexibility, which I do not like either) handling.

FWIW, in DataLad, in reading the bool config values we have

 def anything2bool(val):
    if hasattr(val, 'lower'):
        val = val.lower()
    if val in {"off", "no", "false", "0"} or not bool(val):
        return False
    elif val in {"on", "yes", "true", True} \
            or (hasattr(val, 'isdigit') and val.isdigit() and int(val)) \
            or isinstance(val, int) and val:
        return True
    else:
        raise TypeError(
            "Got value %s which could not be interpreted as a boolean"
            % repr(val))

so for better or for worse, any non-0 positive integer is considered True as well... it seems we are ALMOST inline with git:

$> for v in true yes 1 200 '' false no 0 -1 0.00 0.1 string; do echo -n "$v="; git -c "sec.blah=$v" config --bool sec.blah; python -c "from datalad.config import anything2bool; print(anything2bool('$v'))"; echo; done
true=true
True

yes=true
True

1=true
True

200=true
True

=false
False

false=false
False

no=false
False

0=false
False

-1=true
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-maint/datalad/config.py", line 132, in anything2bool
    % repr(val))
TypeError: Got value '-1' which could not be interpreted as a boolean

0.00=fatal: bad numeric config value '0.00' for 'sec.blah': invalid unit
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-maint/datalad/config.py", line 132, in anything2bool
    % repr(val))
TypeError: Got value '0.00' which could not be interpreted as a boolean

0.1=fatal: bad numeric config value '0.1' for 'sec.blah': invalid unit
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-maint/datalad/config.py", line 132, in anything2bool
    % repr(val))
TypeError: Got value '0.1' which could not be interpreted as a boolean

string=fatal: bad numeric config value 'string' for 'sec.blah': invalid unit
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/yoh/proj/datalad/datalad-maint/datalad/config.py", line 132, in anything2bool
    % repr(val))
TypeError: Got value 'string' which could not be interpreted as a boolean

so only the negative integer is tolerated by git but not by us. I will send a quick PR

Comment by yarikoptic — Tue Apr 7 18:25:46 2020

Remove comment

comment 4

Surely values that git does not even document can be considered to have undefined behavior.

Comment by joey — Tue Apr 7 18:48:18 2020

Remove comment

comment 5

Surely values that git does not even document can be considered to have undefined behavior.

yeah. integers which aren't 0 or 1 are indeed a dark (undocumented) zone. Might be worth contributing to git and making them documented (probably better to not change behavior at this point).

Comment by yarikoptic — Wed Apr 8 01:08:10 2020

Remove comment

comment 6

I've made the git-config parser support all the ways of indicating true and false that git documents, including the strange distinction between an empty config setting (false) and no config setting (true). I'm not going to chase undocumented git behavior that could vary between versions of git.

That also made it support values like "on" that are set by git-annex config --set. It was always possible to set such things. Older git-annex will display an error message if used with a repo that has such values set. That seems acceptable.

I have not done anything about making special remote configs consistent.

Comment by joey — Mon Apr 13 17:45:51 2020

Remove comment

comment 7

Looking closer at the affected special remote configs:

readonly=true is only used by external special remotes. It used to be stored in the remote.log, but after examining it, I have changed that.

So, it could be safely changed to also accept readonly=yes with no back-compat issues.
When git-annex init is run, any autoenable= that is stored never results in an error no matter what the value is; if it doesn't recognise it, it assumes false. (And more generally, no value already stored in remote.log causes a parse error, even if it cannot be parsed.) So autoenable=yes would only result in older git-annex init not enabling the remote. Which I guess is not a hard problem for a user to recover from, the remote clearly won't be enabled when they try to use it. Two arguments in favor of not worrying about backwards compatability for autoenable=yes:
1. When autoenable=true was added originally, we obviously didn't worry that older git-annex's would ignore that config and not autoenable.
2. If the user does anything other than git-annex init, autoenabling doesn't happen currently, which seems like rather more foot shooting potential for ending up w/o a remote autoenabled, but I'm not seeing much complaint about that. Though IIRC there might be a todo about it.

So I'm leaning a bit toward it would be ok to accept yes/no as well as true/false for those two, without worrying about translation to values old git-annex will understand.

But.. Allowing yes/no for these two while still also allowing true/false does not remove the special case, it just buries it a bit. All the other initremote booleans are yes/no and not true/false. And changing them would need a translation layer. I don't feel that bit of uniformity is worth that added complexity.

Comment by joey — Thu Apr 23 16:39:16 2020

Remove comment

Add a comment