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

BF: status: Provide special treatment of "." path #3325

Merged
merged 2 commits into from Apr 13, 2019

Conversation

@kyleam
Copy link
Contributor

@kyleam kyleam commented Apr 13, 2019

When status receives "." as the path, rev_resolve_path() resolves the
path to the dataset (either to the one explicitly provided as an
argument or to the one inferred from the current directory).  So,
using "." means we get the reference dataset as the path.

If a call includes an explicitly specified dataset _and_ "." as a path
argument, whether the call succeeds or fails depends on whether the
specified dataset is a subdataset.  When the reference dataset isn't a
subdataset, status queries within the dataset.  When it is a
subdataset, the call fails with a message saying the subdataset is not
underneath itself:

  $ datalad status -d. .
  [ERROR ] dataset containing given paths is not underneath the
  reference dataset [...]

Even though the desired behavior could be achieved by dropping "." as
a path argument, let's make using "." as the path argument work
because

  * it is consistent with the non-subdataset case, and it won't be
    obvious to a caller why it works in one case but not the other

  * based on the use of "." as a path argument in other places in
    datalad, it's be reasonable for a caller to expect, say,
    subds.status(".") to work

As shown by the added run test, this fixes a regression where calling
'datalad run' failed from within a subdataset, which was introduced by
eba3ccfd6 (BF: run: Save results with rev-save, 2019-03-18) because
the call to rev_save() retained the path="." that is necessary for the
add() call.

I decided to make this change in status() because I couldn't think of a reason why the current behavior is preferable, but we could of course put the kludge in run() with something like

diff --git a/datalad/interface/run.py b/datalad/interface/run.py
index d23247c00..473e734a8 100644
--- a/datalad/interface/run.py
+++ b/datalad/interface/run.py
@@ -394,7 +394,7 @@ def _execute_command(command, pwd, expected_exit=None):
 def _save_outputs(ds, to_save, msg):
     """Helper to save results after command execution is completed"""
     return Save.__call__(
-        to_save,
+        None if to_save == "." else to_save,
         dataset=ds.path,
         recursive=True,
         message=msg,

(We could even drop the custom saver parameter in run.run_command(), and move to an inline ds.rev_save() call because saver only existed for -revolution's rev-run.)

When status receives "." as the path, rev_resolve_path() resolves the
path to the dataset (either to the one explicitly provided as an
argument or to the one inferred from the current directory).  So,
using "." means we get the reference dataset as the path.

If a call includes an explicitly specified dataset _and_ "." as a path
argument, whether the call succeeds or fails depends on whether the
specified dataset is a subdataset.  When the reference dataset isn't a
subdataset, status queries within the dataset.  When it is a
subdataset, the call fails with a message saying the subdataset is not
underneath itself:

  $ datalad status -d. .
  [ERROR ] dataset containing given paths is not underneath the
  reference dataset [...]

Even though the desired behavior could be achieved by dropping "." as
a path argument, let's make using "." as the path argument work
because

  * it is consistent with the non-subdataset case, and it won't be
    obvious to a caller why it works in one case but not the other

  * based on the use of "." as a path argument in other places in
    datalad, it's be reasonable for a caller to expect, say,
    subds.status(".") to work

As shown by the added run test, this fixes a regression where calling
'datalad run' failed from within a subdataset, which was introduced by
eba3ccf (BF: run: Save results with rev-save, 2019-03-18) because
the call to rev_save() retained the path="." that is necessary for the
add() call.
@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 13, 2019

I am yet to fully digest what is proposed exactly, but giving special treatment to what could be a legit path, possibly programmatically computed, sounds like a trouble

kyleam added a commit that referenced this issue Apr 13, 2019
does error from gh-3325 appveyor run happen on maint?

[ci skip]
@kyleam
Copy link
Contributor Author

@kyleam kyleam commented Apr 13, 2019

Hrm, I'm baffled by that AppVeyor failure. It's happening in the new run test:

https://ci.appveyor.com/project/mih/datalad/builds/23817228/job/uwqkpr8n753vh1ue#L1365

datalad.interface.tests.test_run.test_run_from_subds ... Command exited with code -1073741819

Then, if I revert all the changes from this PR except that run test, commenting the actual .run() call (because that would fail without the other changes), I see a similar but not identical failure:

https://ci.appveyor.com/project/mih/datalad/build/job/01c6var48gbjpyrw#L1398

datalad.interface.tests.test_run.test_run_from_subds ... Unhandled exception in thread started by Command exited with code -1073741819

Dunno.

@yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Apr 13, 2019

Confused: is there ability to direct output to a file on command line in Windows with > ?

@mih
Copy link
Member

@mih mih commented Apr 13, 2019

Ping #3247 where the modified code is RF'ed into a standalone function.

@mih
Copy link
Member

@mih mih commented Apr 13, 2019

Confused: is there ability to direct output to a file on command line in Windows with > ?

Yes, of course. However, the syntax is slightly different, so those examples are carefully chosen to yield equivalent results across platforms.

@mih
Copy link
Member

@mih mih commented Apr 13, 2019

Hrm, I'm baffled by that AppVeyor failure.

Sadly, this is "normal". We have a bunch of tests that crash like that. Note, it doesn't lead to a test error, but crashes the test harness itself. You will find examples of

if 'APPVEYOR' in os.environ:
    ...

throughout the code base, where I simply skip individual pieces that lead to this failure. I could not figure out why it happens, the internet only has speculation, and I could not replicate any of those failures in local VMs or real Windows machines.

mih
mih approved these changes Apr 13, 2019
Copy link
Member

@mih mih left a comment

I think this is a sensible move and I agree with the rational.

@codecov
Copy link

@codecov codecov bot commented Apr 13, 2019

Codecov Report

Merging #3325 into master will increase coverage by 45.87%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3325       +/-   ##
===========================================
+ Coverage   45.26%   91.14%   +45.87%     
===========================================
  Files         260      263        +3     
  Lines       34209    34253       +44     
===========================================
+ Hits        15486    31219    +15733     
+ Misses      18723     3034    -15689
Impacted Files Coverage Δ
datalad/core/local/tests/test_status.py 98.87% <100%> (+3.42%) ⬆️
datalad/core/local/status.py 97.91% <100%> (+4.16%) ⬆️
datalad/interface/tests/test_run.py 99.83% <100%> (+58.49%) ⬆️
datalad/metadata/extractors/__init__.py 100% <0%> (ø) ⬆️
datalad/metadata/extractors/tests/__init__.py 100% <0%> (ø) ⬆️
datalad/metadata/__init__.py 100% <0%> (ø) ⬆️
datalad/support/tests/__init__.py 100% <0%> (ø) ⬆️
datalad/tests/test_dochelpers.py 100% <0%> (ø) ⬆️
tools/coverage-bin/git-annex-remote-datalad 100% <0%> (ø)
tools/coverage-bin/datalad 100% <0%> (ø)
... and 189 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ffdca4...ce48eda. Read the comment docs.

@codecov
Copy link

@codecov codecov bot commented Apr 13, 2019

Codecov Report

Merging #3325 into master will increase coverage by 45.68%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #3325       +/-   ##
===========================================
+ Coverage   45.26%   90.95%   +45.68%     
===========================================
  Files         260      263        +3     
  Lines       34209    34235       +26     
===========================================
+ Hits        15486    31139    +15653     
+ Misses      18723     3096    -15627
Impacted Files Coverage Δ
datalad/core/local/status.py 97.91% <ø> (+4.16%) ⬆️
datalad/core/local/tests/test_status.py 98.87% <100%> (+3.42%) ⬆️
datalad/interface/tests/test_run.py 99.35% <100%> (+58.01%) ⬆️
datalad/support/due.py 48% <0%> (-28%) ⬇️
datalad/ui/utils.py 46.87% <0%> (-6.25%) ⬇️
datalad/tests/test_config.py 98.8% <0%> (-1.2%) ⬇️
datalad/metadata/extractors/__init__.py 100% <0%> (ø) ⬆️
datalad/metadata/extractors/tests/__init__.py 100% <0%> (ø) ⬆️
datalad/metadata/__init__.py 100% <0%> (ø) ⬆️
datalad/config.py 98.41% <0%> (ø) ⬆️
... and 190 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ffdca4...ce48eda. Read the comment docs.

@mih mih merged commit c44b3d7 into datalad:master Apr 13, 2019
5 checks passed
@kyleam kyleam deleted the status-subds-dot branch Apr 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants