Skip to content

Conversation

@daavoo
Copy link
Contributor

@daavoo daavoo commented Nov 19, 2021

Close #6383

@daavoo daavoo requested a review from pmrowla November 19, 2021 19:07
@daavoo daavoo requested a review from a team as a code owner November 19, 2021 19:07
Copy link
Contributor

@pmrowla pmrowla left a comment

Choose a reason for hiding this comment

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

So the original issue isn't with tags in packed refs. The problem is with annotated (signed) tags. Normal/lightweight (unsigned) tags are already handled properly, even when they are in packed refs.

Lightweight tags are just regular git refs (where the ref points directly to a commit object's SHA).

Annotated tags are different in that the ref points to a (annotated) tag object SHA, and not a commit SHA. To go from the annotated tag name to the commit SHA, you have to actually read the tag object first to get the dereferenced commit SHA (this is generally referred to as peeling the tag object in git/dulwich/pygit/etc docs+code).

(see https://git-scm.com/docs/git-tag)

The underlying issue in DVC is that the current dulwich implementation for git.describe doesn't resolve annotated (signed) tags properly.
https://github.com/iterative/dvc/blob/cb941a836d77c845f612be29e27ae80f7a2a126c/dvc/scm/git/backend/dulwich/__init__.py#L599

I haven't investigated this issue deeply, but if I had to guess I would think that the actual bug is probably that dulwich get_ref(annotated_tag) is returning the SHA for the tag object itself, instead of returning the peeled commit SHA.

https://github.com/iterative/dvc/blob/cb941a836d77c845f612be29e27ae80f7a2a126c/dvc/scm/git/backend/dulwich/__init__.py#L613

(Note that follow is unrelated to tags - it's for whether or not to follow symbolic refs. For annotated tags get_ref should always return the peeled commit SHA.)

@pmrowla
Copy link
Contributor

pmrowla commented Nov 20, 2021

Also, if you are trying to test against packed refs, you can just make some tags and then force git to pack everything in your repo with

git pack-refs --all

Don't do this in non-testing repos as it will pack all of your active branch refs (which will probably end up affecting performance of normal git command usage)

https://git-scm.com/docs/git-pack-refs

@daavoo
Copy link
Contributor Author

daavoo commented Nov 20, 2021

Also, if you are trying to test against packed refs, you can just make some tags and then force git to pack everything in your repo with

git pack-refs --all

Don't do this in non-testing repos as it will pack all of your active branch refs (which will probably end up affecting performance of normal git command usage)

https://git-scm.com/docs/git-pack-refs

Thanks! Indeed, I used that for testing on local repo but was asking more in the lines of: how to properly do this inside one of our tests? I didn't find a way to do it from Python (apart from subprocess call)

@daavoo daavoo force-pushed the exp-show-tags branch 2 times, most recently from 0c1580b to 780fd23 Compare November 20, 2021 09:15
@daavoo
Copy link
Contributor Author

daavoo commented Nov 20, 2021

Also, if you are trying to test against packed refs, you can just make some tags and then force git to pack everything in your repo with

git pack-refs --all

Don't do this in non-testing repos as it will pack all of your active branch refs (which will probably end up affecting performance of normal git command usage)

https://git-scm.com/docs/git-pack-refs

Also, if you are trying to test against packed refs, you can just make some tags and then force git to pack everything in your repo with

git pack-refs --all

Don't do this in non-testing repos as it will pack all of your active branch refs (which will probably end up affecting performance of normal git command usage)
https://git-scm.com/docs/git-pack-refs

Thanks! Indeed, I used that for testing on local repo but was asking more in the lines of: how to properly do this inside one of our tests? I didn't find a way to do it from Python (apart from subprocess call)

As the problem was really about annotated tags, not packed refs, there is not really need for that

@daavoo daavoo requested a review from pmrowla November 20, 2021 09:16
@pmrowla
Copy link
Contributor

pmrowla commented Nov 20, 2021

Thanks! Indeed, I used that for testing on local repo but was asking more in the lines of: how to properly do this inside one of our tests? I didn't find a way to do it from Python (apart from subprocess call)

In tests you can use CLI git directly via the scm fixture and gitpython like

scm.gitpython.git.pack_refs(all=True)

(https://gitpython.readthedocs.io/en/stable/tutorial.html#using-git-directly)

Comment on lines +346 to +347
if isinstance(self.repo[ref], Tag):
ref = self.repo.get_peeled(name_b)
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks the case where name is a symbolic reference and follow = False, like when HEAD points to refs/heads/master. (see the current test failures)

Basically we need to catch KeyError when we do the self.repo[ref] check, and pass as long as follow is also False

@skshetry
Copy link
Collaborator

@daavoo, please open this PR on the upstream. Really sorry for the inconvenience. 🙂

@skshetry skshetry closed this Nov 26, 2021
@daavoo daavoo deleted the exp-show-tags branch February 11, 2022 19:19
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.

dvc exp show --all-tags: doesn't show the names of annotated tags.

3 participants