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: remove a submodule with a remote without refs + misc fixes around #521

Merged

Conversation

yarikoptic
Copy link
Contributor

now if there is a remote without any refs (this happens e.g. with git-annex repo with special remotes), it would refuse to remove the submodule
(with 2.0.8 it also didn't work but with a different msg "AssertionError: Remote datalad did not have any references")

@ankostis
Copy link
Contributor

ankostis commented Oct 2, 2016

@yarikoptic would it be possible to add a test-case into the PR with the annex command, replicating the situation you just described?

@yarikoptic
Copy link
Contributor Author

ok @ankostis -- but along the way had to fix 2 other bugs... apparently some tests (which used @with_rw_directory) were not run at all! so you can expect failures of those tests which were effected


parent.index.commit("Added submodule")

assert sm.repo is parent # yoh was surprised since expected sm repo!!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

and I am not sure why that is the case -- I was expecting submodules repo to be the repo of the submodule ... but I guess it can make sense to refer to parent which contains the submodule, but then is there an attribute for submodules repo (if present)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Byron any comment? I'm not familiar with git-modules.

@yarikoptic
Copy link
Contributor Author

reincarnating @with_rw_directory lead to one failing test

======================================================================

FAIL: test_lock_reentry (git.test.test_config.TestBase)

----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/gitpython-developers/GitPython/git/test/lib/helper.py", line 95, in wrapper
    return func(self, path)
  File "/home/travis/build/gitpython-developers/GitPython/git/test/test_config.py", line 100, in test_lock_reentry
    self.failUnlessRaises(IOError, GitConfigParser, fpl, read_only=False)
AssertionError: IOError not raised
-------------------- >> begin captured logging << --------------------

git.util: INFO: Test TestBase.test_lock_reentry failed, output is at '/tmp/test_lock_reentryfSovNi'

I am not sure on state of locking/context manager expectations atm and see recent work touching locking... so I will need to leave this to you guys (@ankostis ?) to figure out

@yarikoptic yarikoptic changed the title BF: Allow to remove a submodule with a remote without refs BF: remove a submodule with a remote without refs + misc fixes around Oct 2, 2016
yarikoptic added a commit to yarikoptic/GitPython that referenced this pull request Oct 2, 2016
codecov in our (datalad, etc) experience provides a better service,
great support, and super-nice intergration with chromium and firefox
for reviewing coverage of pull requests.  In light of the
@with_rw_directory fiasco detected/fixed in gitpython-developers#521 I would strongly
recommend to (re-)enable and use coverage reports
yarikoptic added a commit to yarikoptic/GitPython that referenced this pull request Oct 2, 2016
codecov in our (datalad, etc) experience provides a better service,
great support, and super-nice intergration with chromium and firefox
for reviewing coverage of pull requests.  In light of the
@with_rw_directory fiasco detected/fixed in gitpython-developers#521 I would strongly
recommend to (re-)enable and use coverage reports
@ankostis
Copy link
Contributor

ankostis commented Oct 2, 2016

@yarikoptic so you mean that git.test.test_config:TestBase.test_lock_reentry() that is failing is included in the CI for the first time?

@yarikoptic
Copy link
Contributor Author

Yes, at least since some time in the past

@yarikoptic
Copy link
Contributor Author

If your really want to know, I could check later since when

@ankostis
Copy link
Contributor

ankostis commented Oct 2, 2016

Indeed @yarikoptic your 31fd955 fixed an important regression introduced by #519,
where I had practically disabled 19 TCs!
(so the ALL GREEN was an illusion :-/, check #519 for the actual list of TC errors on Windows)

So, I'm cherry picking your 2528d11 and 31fd955 but modified to include only your helper.py fixes and pushing them into my own repo, to try to fix the failing TC; for the test_submodles.py changes, they have to wait, and I will collect them and re-synthesize a new branch to merge.

@ankostis ankostis merged commit f284a4e into gitpython-developers:master Oct 2, 2016
ankostis added a commit that referenced this pull request Oct 2, 2016
+ The actual commits have been re-written and rebased previously.
@ankostis
Copy link
Contributor

ankostis commented Oct 2, 2016

On Travis, PY3.3 failed due to file-handles; increased ulimit 96-->100 solved the problem.
On Appveyor, this is a drama...

@ankostis
Copy link
Contributor

ankostis commented Oct 2, 2016

Forgot to thank you.
@yarikoptic If you may, add yourself on the Authors, on this branch, and I will cherry-pick it, just add a comment here to notify me.

@yarikoptic
Copy link
Contributor Author

That is OK, if you like to see me there - add, otherwise I am just fine. Cheers. Thanks for attending to my PRs quickly

@yarikoptic yarikoptic deleted the bf-rsubmodule-remove branch October 15, 2016 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants