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

RF: Reimplement GitRepo.merge_base() without GitPython #4170

Merged
merged 2 commits into from Feb 20, 2020

Conversation

mih
Copy link
Member

@mih mih commented Feb 20, 2020

This furthers gh-2879

@mih
Copy link
Member Author

mih commented Feb 20, 2020

Appveyor failure is only coverage submission.

Copy link
Collaborator

@kyleam kyleam left a comment

Thanks, looks good to me. I have only minor comments.

if "fatal: Not a valid object name" in str(exc):
return None
raise
bases = self.call_git_oneline(['merge-base'] + commitishes)
Copy link
Collaborator

@kyleam kyleam Feb 20, 2020

Choose a reason for hiding this comment

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

bases made sense given the gitpython return value, but I think for clarity (because we don't support --all) the rewrite should use base.

Copy link
Member Author

@mih mih Feb 20, 2020

Choose a reason for hiding this comment

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

Will do.

# we don't know what this is, and we have something to tell about it
# then log it. Without output, it could also just be "no merge base"
lgr.debug(exc_str(exc))
return None
Copy link
Collaborator

@kyleam kyleam Feb 20, 2020

Choose a reason for hiding this comment

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

There's a change in behavior here, switching to logging unknown exceptions rather than raising them. My preference would be to raise the error rather than logging in this situation. Perhaps something like this:

diff --git a/datalad/support/gitrepo.py b/datalad/support/gitrepo.py
index 30f08f652..642bfb511 100644
--- a/datalad/support/gitrepo.py
+++ b/datalad/support/gitrepo.py
@@ -1979,12 +1979,12 @@ def get_merge_base(self, commitishes):
         try:
             bases = self.call_git_oneline(['merge-base'] + commitishes)
         except CommandError as exc:
-            if "fatal: Not a valid object name" not in exc.stderr and (
-                    exc.stdout or exc.stderr):
-                # we don't know what this is, and we have something to tell about it
-                # then log it. Without output, it could also just be "no merge base"
-                lgr.debug(exc_str(exc))
-            return None
+            if exc.code == 1 and not (exc.stdout or exc.stderr):
+                # No merge base was found (unrelated commits).
+                return None
+            if "fatal: Not a valid object name" in exc.stderr:
+                return None
+            raise
 
         if not bases:
             return None

I don't have a strong objection to leaving it as is, though.

Copy link
Member Author

@mih mih Feb 20, 2020

Choose a reason for hiding this comment

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

I like this more than what I did, will change. Thx!

# we don't know what this is, and we have something to tell about it
# then log it. Without output, it could also just be "no merge base"
lgr.debug(exc_str(exc))
return None

if not bases:
Copy link
Collaborator

@kyleam kyleam Feb 20, 2020

Choose a reason for hiding this comment

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

This condition isn't relevant any more, is it?

Copy link
Member Author

@mih mih Feb 20, 2020

Choose a reason for hiding this comment

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

True, will remove.

@codecov
Copy link

codecov bot commented Feb 20, 2020

Codecov Report

Merging #4170 into master will decrease coverage by 0.22%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4170      +/-   ##
==========================================
- Coverage   89.13%   88.91%   -0.23%     
==========================================
  Files         275      275              
  Lines       35847    35846       -1     
==========================================
- Hits        31953    31872      -81     
- Misses       3894     3974      +80
Impacted Files Coverage Δ
datalad/support/gitrepo.py 90.28% <83.33%> (-0.37%) ⬇️
datalad/customremotes/base.py 69.25% <0%> (-14.89%) ⬇️
datalad/customremotes/tests/__init__.py 91.66% <0%> (-8.34%) ⬇️
datalad/support/keyring_.py 84.44% <0%> (-6.67%) ⬇️
datalad/tests/test_base.py 96.55% <0%> (-3.45%) ⬇️
datalad/customremotes/tests/test_archives.py 86.27% <0%> (-3.27%) ⬇️
datalad/downloaders/http.py 70.91% <0%> (-2.79%) ⬇️
datalad/downloaders/tests/test_http.py 58.39% <0%> (-2.19%) ⬇️
datalad/__init__.py 88.88% <0%> (-1.49%) ⬇️
datalad/cmd.py 91.91% <0%> (-1.28%) ⬇️
... and 8 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 bc05be3...0b364ff. Read the comment docs.

@mih
Copy link
Member Author

mih commented Feb 20, 2020

A pushed the changes you pointed out @kyleam

@kyleam
Copy link
Collaborator

kyleam commented Feb 20, 2020

A pushed the changes you pointed out @kyleam

Thanks, LGTM

@yarikoptic yarikoptic added this to the 0.13.0 milestone Feb 20, 2020
@mih mih merged commit c9c435a into datalad:master Feb 20, 2020
15 checks passed
@mih mih deleted the rf-mergebase branch Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants