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 lookup of tags in the presence of lightweight tags #1029

Merged
merged 3 commits into from
Feb 3, 2024

Conversation

Rob-Hague
Copy link
Contributor

Fixes #1026

I've not validated this against my problematic repo (I'm not sure how), but I think I've worked out a decent enough understanding of the problem to be confident that this will fix it.

- Add GitRepositoryTests.TryGetObjectByShaAndWrongTypeTest, which fails
  because of the exception thrown in TryGetObjectByPath
  (Nerdbank.GitVersioning.GitException : Got a comm instead of a tree when
  opening object d13d221382b251e13f23d0e32062bb2354d2c340)

- Extend VersionOracleTests.Tags to add another commit and check that
  the new commit does not point to the tag. This fails for the lightweight
  tag case because we enter the TryGetObjectBySha(pointsAt, "tag", ...)
  path which tries to retrieve the tag object assuming it is of type "tag"
  when in fact it is of type "commit", and throws the exception.

  The reason it doesn't fail without the extension to the test (i.e. when
  the lightweight tag points to HEAD) is that the objectId.Equals(pointsAt)
  check passes (for the same reason: the tag "is" the commit) so the
  TryGetObjectBySha path is not entered.

  That's also why there is no additional fix needed for lightweight tags
  in HandleCandidate: its purpose is to decide whether a tag points to
  the given object. For lightweight tags it's a simple equality check on
  the object ids. It's only for annotated tags where we need to dig in
  to find the actual commit id.

- Extend VersionOracleTests.Tags_Annotated for symmetry. It still passes
  either way.
Copy link
Collaborator

@AArnott AArnott left a comment

Choose a reason for hiding this comment

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

Thanks!

@AArnott AArnott merged commit fe36dc1 into dotnet:main Feb 3, 2024
16 checks passed
@AArnott AArnott added this to the v3.7 milestone Feb 3, 2024
@filipnavara
Copy link
Member

Thanks a lot!

@Rob-Hague Rob-Hague deleted the lightweighttags branch February 3, 2024 15:56
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.

Error in ManagedGit when dealing with non-packed lightweight tags
3 participants