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

Fix creating references containing '@' #854

Open

Conversation

@tobiashenkel
Copy link

@tobiashenkel tobiashenkel commented Mar 15, 2019

References containing the '@' char are legal in git but fail with
[1]. This fails because as soon as '@' is encountered we stop
searching for a ref name, assume it is there and fail while retrieving
it. This can be fixed by catching this exception and continue with the
search for a valid ref.

[1] Trace:
Traceback (most recent call last):
  File "/opt/zuul/lib/python3.7/site-packages/zuul/merger/merger.py", line 108, in __init__
   self._ensure_cloned()
  File "/opt/zuul/lib/python3.7/site-packages/zuul/merger/merger.py", line 178, in _ensure_cloned
    repo.create_head(ref.remote_head, ref, force=True)
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/base.py", line 373, in create_head
    return Head.create(self, path, commit, force, logmsg)
  File "/opt/zuul/lib/python3.7/site-packages/git/refs/symbolic.py", line 546, in create
    return cls._create(repo, path, cls._resolve_ref_on_create, reference, force, logmsg)
  File "/opt/zuul/lib/python3.7/site-packages/git/refs/symbolic.py", line 497, in _create
    target = repo.rev_parse(str(reference))
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/fun.py", line 211, in rev_parse
    ref = name_to_object(repo, rev[:start], return_ref=True)
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/fun.py", line 142, in name_to_object
    raise BadObject("Couldn't find reference named %r" % name)
gitdb.exc.BadObject: <unprintable BadObject object>
References containing the '@' char are legal in git but fail with
[1]. This fails because as soon as '@' is encountered we stop
searching for a ref name, assume it is there and fail while retrieving
it. This can be fixed by catching this exception and continue with the
search for a valid ref.

[1] Trace:
Traceback (most recent call last):
  File "/opt/zuul/lib/python3.7/site-packages/zuul/merger/merger.py", line 108, in __init__
   self._ensure_cloned()
  File "/opt/zuul/lib/python3.7/site-packages/zuul/merger/merger.py", line 178, in _ensure_cloned
    repo.create_head(ref.remote_head, ref, force=True)
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/base.py", line 373, in create_head
    return Head.create(self, path, commit, force, logmsg)
  File "/opt/zuul/lib/python3.7/site-packages/git/refs/symbolic.py", line 546, in create
    return cls._create(repo, path, cls._resolve_ref_on_create, reference, force, logmsg)
  File "/opt/zuul/lib/python3.7/site-packages/git/refs/symbolic.py", line 497, in _create
    target = repo.rev_parse(str(reference))
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/fun.py", line 211, in rev_parse
    ref = name_to_object(repo, rev[:start], return_ref=True)
  File "/opt/zuul/lib/python3.7/site-packages/git/repo/fun.py", line 142, in name_to_object
    raise BadObject("Couldn't find reference named %r" % name)
gitdb.exc.BadObject: <unprintable BadObject object>
Copy link
Member

@Byron Byron left a comment

Thanks a lot for the contribution! Do you think it's possible to have another look?
Also I am happy to hear your thoughts, as I am (in the review) expressing a preference, not a hard requirement.

@@ -208,7 +208,14 @@ def rev_parse(repo, rev):
ref = repo.head.ref
else:
if token == '@':
ref = name_to_object(repo, rev[:start], return_ref=True)
try:

This comment has been minimized.

@Byron

Byron Jul 20, 2019
Member

A better solution might be to stop treating @ as special outside of the spots that it is allowed in.
I am a hesitant to let it make assumptions unless there absolutely is no other way. Or in other words, I believe the git or C implementation is deterministic here

This comment has been minimized.

@tobiashenkel

tobiashenkel Aug 8, 2019
Author

Thanks for your feedback, I'll try it when I have time in the next weeks.

@brondsem
Copy link
Contributor

@brondsem brondsem commented Feb 28, 2020

Ping - any updates?

The changes here work for me and the situation I've run into this with. It would be nice to see this (or a better solution) merged.

@Byron
Copy link
Member

@Byron Byron commented Mar 8, 2020

Having had a look at it again, I still feel uneasy about making an assumption warranting this comment:

                  # If this doesn't result in an object we most likely
                  # encounter a reference containing the '@' character.
                  # In this case we just to continue the search.

I wonder if it's safe that way, or if it leaves us open to causing unexpected breakage.

Maybe @Harmon758 can provide their opinion on this - generally this PR seems like a very desirable fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.