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: allow for fetch to fail due to HEAD not yet been available on a remote for annex #4200

Merged
merged 2 commits into from Feb 29, 2020

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Feb 26, 2020

This is an alternative solution to #4199
which is lfs specific. In this solution which would probably behave "correctly" for any
special remote which has URL and for which we never managed to fetch HEAD.

yarikoptic added a commit to yarikoptic/book that referenced this issue Feb 26, 2020
Unfortunately I could not specify --publish-depends on use
datalad publish due to a crash which would be resolved if either (or both)
of

 datalad/datalad#4200
 datalad/datalad#4199

get merged
@codecov
Copy link

@codecov codecov bot commented Feb 26, 2020

Codecov Report

No coverage uploaded for pull request base (maint@09faea2). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             maint    #4200   +/-   ##
========================================
  Coverage         ?   89.04%           
========================================
  Files            ?      275           
  Lines            ?    36853           
  Branches         ?        0           
========================================
  Hits             ?    32815           
  Misses           ?     4038           
  Partials         ?        0
Impacted Files Coverage Δ
datalad/distribution/publish.py 87.93% <100%> (ø)
datalad/distribution/tests/test_publish.py 100% <100%> (ø)

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 09faea2...1dc0fdc. Read the comment docs.

@yarikoptic yarikoptic added the merge-if-ok label Feb 26, 2020
@yarikoptic yarikoptic added this to the 0.12.3 milestone Feb 26, 2020
ds.repo.merge_annex(remote)
try:
ds.repo.fetch(remote=remote)
ds.repo.merge_annex(remote)
Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: You're only concerned with catching an exception from fetch, so in my view it'd be clearer/better to have merge_annex as part of trys else, though it is unlikely to matter in practice given the handling should almost certainly reraise if the error came from merge_annex.

# possibly special (e.g. LFS) annex special remote,
# and no HEAD was known
if ("Couldn't find remote ref HEAD" in exc.stderr) and \
('%s/HEAD' % remote not in ds.repo.get_remote_branches()):
Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing to find HEAD on the remote and the existence of the local refs/remotes/<remote>/HEAD are distinct things. If a user doesn't like recording a default branch for the remote, they can git remote set-head <remote> --delete, and it won't be created again.

While it may seem unlikely that the remote was successfully fetched from, then locally the remote's HEAD was removed, and then for a later fetch the remote HEAD couldn't be found, I don't see a benefit of making this assumption. Would just checking whether any refs are under refs/remotes/<remote> be a reliable enough indicator that we shouldn't raise the error in this case? Something like the (untested)

diff --git a/datalad/distribution/publish.py b/datalad/distribution/publish.py
index ed2ad6cec8..5aa6250790 100644
--- a/datalad/distribution/publish.py
+++ b/datalad/distribution/publish.py
@@ -436,10 +436,7 @@ def _publish_dataset(ds, remote, refspec, paths, annex_copy_options, force=False
                 # We might be publishing for the first time to this
                 # possibly special (e.g. LFS) annex special remote,
                 # and no HEAD was known
-                if ("Couldn't find remote ref HEAD" in exc.stderr) and \
-                   ('%s/HEAD' % remote not in ds.repo.get_remote_branches()):
-                    pass
-                else:
+                if next(ds.repo.for_each_ref_(pattern="refs/remotes/" + remote):
                     raise
 
         # Note: git's push.default is 'matching', which doesn't work for first

As a tangent, I wonder if a non-zero exit code for an empty repository is specific to the hosting service or git version. With git 2.25.1, if I add an empty gitlab repo or regular ssh target as a remote and fetch, the fetch call doesn't fail; it just doesn't bring anything down.

Copy link
Member Author

@yarikoptic yarikoptic Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds reasonable, I will add the change, but will delegate to get_remote_branches use of refs/remotes/ ;-)
FWIW -- system git I have is 2.25, while the one bundled with git annex is (and probably was) 2.24. So, in your case if you run the snippet in #4199 (sorry I did not make it totally cut/pasteable) -- it works on master (or ideally 0.12.2) version?

Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but will delegate to get_remote_branches use of refs/remotes/ ;-)

Okay. The advantage of using refs/remotes/<remote> is that we have potentially less output to process, but, given that we're working within an exception that should not be triggered often, it's unlikely that it matters.

Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in your case if you run the snippet in #4199 (sorry I did not make it totally cut/pasteable) -- it works on master (or ideally 0.12.2) version?

Sorry, I haven't taken the time to put datalad into the mix. I'm just talking about when I create an empty repo on gitlab or an plain ssh server, add that as a remote, and git fetch it. I tried with an empty repo on github, and git fetch (via ssh and https) exits with status of 0.

Based on the description of the problem, I think that's the state things are in when this call to fetch in publish fails, but perhaps it's some other in between state, and that is the reason for the difference.

Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stepping through fetch.c, the essential piece to trigger "fatal: Couldn't find remote ref HEAD" is that the remote does not have remote.*.fetch set.

Copy link
Member Author

@yarikoptic yarikoptic Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha. So may be we shouldn't even fetch for such remotes?

Copy link
Contributor

@kyleam kyleam Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So may be we shouldn't even fetch for such remotes?

Given both clone and remote add will populate .fetch values, I think that'd be a reasonable approach, though I don't mind sticking with the current one either.

Copy link
Contributor

@kyleam kyleam Feb 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So may be we shouldn't even fetch for such remotes?

I've updated the PR to go with this approach.

range-diff
1:  19854724e1 = 1:  ac1871c47e BF: allow for fetch to fail due to HEAD not yet been available on a remote for annex
-:  ---------- > 2:  676d360692 BF: publish: Don't fetch if remote.*.fetch is not set

…emote for annex

This is an alternative solution to datalad#4199
which is lfs specific.  In this solution which would probably behave "correctly" for any
special remote which has URL and for which we never managed to fetch HEAD.
@kyleam kyleam changed the base branch from master to maint Feb 28, 2020
kyleam added a commit to yarikoptic/datalad that referenced this issue Feb 28, 2020
To deal with a situation where fetching an empty repository (in
particular an git-lfs special annex remote) failed with

    fatal: Couldn't find remote ref HEAD" if no branch

the previous commit updated one of the fetch() calls in
_publish_dataset() to catch a CommandError and swallow it if it looks
like it was due to the above failure.

The core path to this error is fetching from an empty repository that
has neither remote.*.fetch nor an upstream configured.  Not having the
former is rare because 'clone' and 'remote add' both set
remote.*.fetch automatically.  Having remote.*.fetch unset but a
configured upstream would be a hard state to get into [*], but, in
that case, fetching doesn't serve any purpose for publish() because
nothing in refs/remotes/<remote> would be updated, only FETCH_HEAD.

Given publish() can't do anything useful without a fetch refspec,
let's instead avoid the above error by guarding the fetch() calls in
publish() with a check that a fetch refspec is set.

A minor though nice benefit of this approach is that it works with
both master (which raises a CommandError in the above case) and maint
(where the GitPython-based fetch raises an AssertionError when no
fetch refspec is configured).

Re: datalad#4200 (comment)

[*] `git branch --set-upstream-to` would reject it, so it'd have to be
     set up directly via .git/config values.
kyleam added a commit to yarikoptic/datalad that referenced this issue Feb 29, 2020
To deal with a situation where fetching an empty repository (in
particular an git-lfs special annex remote) failed with

    fatal: Couldn't find remote ref HEAD" if no branch

the previous commit updated one of the fetch() calls in
_publish_dataset() to catch a CommandError and swallow it if it looks
like it was due to the above failure.

The core path to this error is fetching from an empty repository that
has neither remote.*.fetch nor an upstream configured.  Not having the
former is rare because 'clone' and 'remote add' both set
remote.*.fetch automatically.  Having remote.*.fetch unset but a
configured upstream would be a hard state to get into [*], but, in
that case, fetching doesn't serve any purpose for publish() because
nothing in refs/remotes/<remote> would be updated, only FETCH_HEAD.

Given publish() can't do anything useful without a fetch refspec,
let's instead avoid the above error by guarding the fetch() calls in
publish() with a check that a fetch refspec is set.

A minor though nice benefit of this approach is that it works with
both master (which raises a CommandError in the above case) and maint
(where the GitPython-based fetch raises an AssertionError when no
fetch refspec is configured).

Re: datalad#4200 (comment)

[*] `git branch --set-upstream-to` would reject it, so it'd have to be
     set up directly via .git/config values.
To deal with a situation where fetching an empty repository (in
particular an git-lfs special annex remote) failed with

    fatal: Couldn't find remote ref HEAD" if no branch

the previous commit updated one of the fetch() calls in
_publish_dataset() to catch a CommandError and swallow it if it looks
like it was due to the above failure.

The core path to this error is fetching from an empty repository that
has neither remote.*.fetch nor an upstream configured.  Not having the
former is rare because 'clone' and 'remote add' both set
remote.*.fetch automatically.  Having remote.*.fetch unset but a
configured upstream would be a hard state to get into [*], but, in
that case, fetching doesn't serve any purpose for publish() because
nothing in refs/remotes/<remote> would be updated, only FETCH_HEAD.

Given publish() can't do anything useful without a fetch refspec,
let's instead avoid the above error by guarding the fetch() calls in
publish() with a check that a fetch refspec is set.

A minor though nice benefit of this approach is that it works with
both master (which raises a CommandError in the above case) and maint
(where the GitPython-based fetch raises an AssertionError when no
fetch refspec is configured).

Re: datalad#4200 (comment)

[*] `git branch --set-upstream-to` would reject it, so it'd have to be
     set up directly via .git/config values.
@kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 29, 2020

AppVeyor failure is the familiar Invoke-WebRequestissue.

Copy link
Member Author

@yarikoptic yarikoptic left a comment

Left a comment about the test, but not sure if it is worthwhile to test assumption - smoke test is probably sufficient. Ideally though it should have mimicked original problem better - there same source repo is used as both regular and special remote (eg directory), with dependence for publishing. Then test could check that things got published correctly.

But I am ok with it as is since it fixes the issue

ds.repo.config.unset("remote.origin.fetch", where="local")
(ds.repo.pathobj / "foo").write_text("a")
ds.save()
ds.publish(to="origin")
Copy link
Member Author

@yarikoptic yarikoptic Feb 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is worth to explicitly note that it is just a smoke test. it could have been an explicit mock for .fetch verifying that it was not called

Copy link
Contributor

@kyleam kyleam Feb 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is worth to explicitly note that it is just a smoke test.

I don't find "smoke test" a useful description. Its specific purpose is to make sure that the fetches do not fail in this situation. If you revert the changes from this PR, you see the failure that the changes here are trying to prevent: the "fatal: Couldn't find remote ref HEAD" error on master or the GitPython failure on maint.

it could have been an explicit mock for .fetch verifying that it was not called

Ew

@kyleam
Copy link
Contributor

@kyleam kyleam commented Feb 29, 2020

Ideally though it should have mimicked original problem better - there same source repo is used as both regular and special remote (eg directory), with dependence for publishing. Then test could check that things got published correctly.

In my opinion the added test covers the core issue well. But please feel free to take back over your PR and make whatever changes you'd prefer.

@yarikoptic
Copy link
Member Author

@yarikoptic yarikoptic commented Feb 29, 2020

But I am ok with it as is since it fixes the issue

Thank you Kyle for taking care about this pr!

@yarikoptic yarikoptic merged commit 43c671d into datalad:maint Feb 29, 2020
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants