Merge pull request #102 from dartmouth-pbs/bf-chmod

BF: do not chmod files using hardcoded mode number

Author: yarikoptic

bin/heudiconv

@@ -53,6 +53,14 @@ from random import sample
 MIN_VERSIONS = {
     'datalad': '0.7'
 }
+
+# Some variables for reuse
+import stat
+
+ALL_CAN_WRITE = (stat.S_IWUSR | stat.S_IWGRP | stat.S_IWOTH)
+ALL_CAN_READ = (stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH)
+assert ALL_CAN_READ >> 1 == ALL_CAN_WRITE  # Assumption in the code
+
 PY3 = sys.version_info[0] >= 3
 
 import logging
@@ -306,6 +314,35 @@ def find_files(regex, topdir=curdir, exclude=None, exclude_vcs=True, dirs=False)
 find_files.__doc__ %= (_VCS_REGEX,)
 
 
+def set_readonly(path, read_only=True):
+    """Make file read only or writeable while preserving "access levels"
+
+    So if file was not readable by "others" it should remain not readable
+    by others
+
+    Parameters
+    ----------
+    path : str
+    read_only : bool, optional
+      If True (default) - would make it read-only. If False, would make it
+      writeable for levels where it is readable
+    """
+    # get current permissions
+    perms = stat.S_IMODE(os.lstat(path).st_mode)
+    # set new permissions
+    if read_only:
+        new_perms = perms & (~ALL_CAN_WRITE)
+    else:
+        # need to set only for those which had read bit set
+        # read bit is <<1 away from write bit
+        whocanread = perms & ALL_CAN_READ
+        thosecanwrite = whocanread >> 1
+        new_perms = perms | thosecanwrite
+    # apply and return those target permissions
+    os.chmod(path, new_perms)
+    return new_perms
+
+
 def group_dicoms_into_seqinfos(
         files, file_filter=None, dcmfilter=None, grouping='studyUID'
 ):
@@ -959,7 +996,7 @@ def convert(items, symlink=True, converter=None,
                 if exists(scaninfo):
                     lgr.info("Post-treating %s file", scaninfo)
                     treat_infofile(scaninfo)
-                os.chmod(outname, 0o0440)
+                set_readonly(outname)
 
         if custom_callable is not None:
             custom_callable(*item)
@@ -1143,9 +1180,9 @@ def tuneup_bids_json_files(json_files):
                     lgr.error("Failed to open magnitude file: %s", exc)
 
             # might have been made R/O already
-            os.chmod(json_phasediffname, 0o0664)
+            set_readonly(json_phasediffname, False)
             json.dump(json_, open(json_phasediffname, 'w'), indent=2)
-            os.chmod(json_phasediffname, 0o0444)
+            set_readonly(json_phasediffname)
 
         # phasediff one should contain two PhaseDiff's
         #  -- one for original amplitude and the other already replicating what is there
@@ -1219,15 +1256,15 @@ in that one though
         if global_options['overwrite'] and os.path.lexists(scaninfo):
             # TODO: handle annexed file case
             if not os.path.islink(scaninfo):
-                os.chmod(scaninfo, 0o0660)
+                set_readonly(scaninfo, False)
         res = embedfunc.run()
-        os.chmod(scaninfo, 0o0444)
+        set_readonly(scaninfo)
         if with_prov:
             g = res.provenance.rdf()
             g.parse(prov_file,
                     format='turtle')
             g.serialize(prov_file, format='turtle')
-            os.chmod(prov_file, 0o0440)
+            set_readonly(prov_file)
     except Exception as exc:
         lgr.error("Embedding failed: %s", str(exc))
         os.chdir(cwd)
@@ -1244,10 +1281,10 @@ def treat_infofile(filename):
     j_slim = slim_down_info(j)
     j_pretty = json_dumps_pretty(j_slim, indent=2, sort_keys=True)
 
-    os.chmod(filename, 0o0664)
+    set_readonly(filename, False)
     with open(filename, 'wt') as fp:
         fp.write(j_pretty)
-    os.chmod(filename, 0o0444)
+    set_readonly(filename)
 
 
 def convert_dicoms(sid,

tests/test_heuristics.py

@@ -11,7 +11,10 @@
 
 import pytest
 
-from datalad.api import Dataset
+try:
+    from datalad.api import Dataset
+except ImportError:  # pragma: no cover
+    Dataset = None
 
 
 def test_smoke_converall(tmpdir):
@@ -28,6 +31,7 @@ def test_smoke_converall(tmpdir):
         "-d tests/data/{subject}/* -s 01-fmap_acq-3mm" # "old" way specifying subject
         # should produce the same results
     ])
+@pytest.mark.skipif(Dataset is None, reason="no datalad")
 def test_dbic_bids_largely_smoke(tmpdir, heuristic, invocation):
     is_bids = True if heuristic == 'dbic_bids' else False
     arg = "-f heuristics/%s.py -c dcm2niix -o %s" % (heuristic, tmpdir)

tests/test_main.py

@@ -1,6 +1,7 @@
 import csv
 import os
 import pytest
+import stat
 import sys
 
 from mock import patch
@@ -194,4 +195,23 @@ def test__find_subj_ses():
     assert heudiconv._find_subj_ses('950_bids_test4/sub-phantom1sid1/fmap/sub-phantom1sid1_acq-3mm_phasediff.json') == ('phantom1sid1', None)
     assert heudiconv._find_subj_ses('sub-s1/ses-s1/fmap/sub-s1_ses-s1_acq-3mm_phasediff.json') == ('s1', 's1')
     assert heudiconv._find_subj_ses('sub-s1/ses-s1/fmap/sub-s1_ses-s1_acq-3mm_phasediff.json') == ('s1', 's1')
-    assert heudiconv._find_subj_ses('fmap/sub-01-fmap_acq-3mm_acq-3mm_phasediff.nii.gz') == ('01', None)
+    assert heudiconv._find_subj_ses('fmap/sub-01-fmap_acq-3mm_acq-3mm_phasediff.nii.gz') == ('01', None)
+
+
+def test_make_readonly(tmpdir):
+    # we could test it all without torturing a poor file, but for going all
+    # the way, let's do it on a file
+    path = tmpdir.join('f')
+    pathname = str(path)
+    with open(pathname, 'w'):
+        pass
+    for orig, ro, rw in [
+        (0o600, 0o400, 0o600),  # fully returned
+        (0o624, 0o404, 0o606),  # it will not get write bit where it is not readable
+        (0o1777, 0o1555, 0o1777),  # and other bits should be preserved
+    ]:
+        os.chmod(pathname, orig)
+        assert heudiconv.set_readonly(pathname) == ro
+        assert stat.S_IMODE(os.lstat(pathname).st_mode) == ro
+        # and it should go back if we set it back to non-read_only
+        assert heudiconv.set_readonly(pathname, read_only=False) == rw