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
Add git.TagIterator
#2117
Add git.TagIterator
#2117
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, seems in-line with the other iterator implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A late review, sorry!
return t.message | ||
} | ||
|
||
func parseAnnotatedTag(hash Hash, data []byte) (*annotatedTag, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a similar comment than these ones to better read the parsing logic pls?
return nil, err | ||
} | ||
case "tagger": | ||
if t.tagger, err = parseIdent([]byte(value)); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it comes from previous PR, but consider s/Ident/Identifier/g
@@ -140,6 +140,22 @@ type ListFilesAndUnstagedFilesOptions struct { | |||
IgnorePathRegexps []*regexp.Regexp | |||
} | |||
|
|||
// TagIterator ranges over tags for a git repository. | |||
// | |||
// All tags are ranged, including local (unpushed) tags. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// All tags are ranged, including local (unpushed) tags. | |
// All local tags are ranged. |
Otherwise we could be implying we'll range all remote ones.
} | ||
|
||
func (r *tagIterator) ForEachTag(f func(string, Hash) error) error { | ||
dir := path.Join(r.gitDirPath, "refs", "tags") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dir := path.Join(r.gitDirPath, "refs", "tags") | |
tagsDir := path.Join(r.gitDirPath, "refs", "tags") |
// Tags are either annotated or lightweight. Depending on the type, | ||
// they are stored differently. First, we try to load the tag | ||
// as an annnotated tag. If this fails, we try a commit. | ||
// Finally, we fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subjective, but I'd say lightweight tags are much more common than annotated ones? Consider switching order, first trying commits.
return fmt.Errorf( | ||
"failed to determine target of tag %q; it is neither a tag nor a commit", | ||
tagName, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird if we ever see this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think the intention is that we are unlikely to hit this code path, but would still need to return an error here. I guess we could add a comment to the effect of // should not hit this
. I suspect this error would mostly be to help us catch bugs and is useful to users on the off-chance there is some corruption in the .git
directory.
No description provided.