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: refetching lightweight tags from origin #349

Merged
merged 2 commits into from
Jun 20, 2024
Merged

fix: refetching lightweight tags from origin #349

merged 2 commits into from
Jun 20, 2024

Conversation

miki725
Copy link
Contributor

@miki725 miki725 commented Jun 19, 2024

Issue

fixes #345

Description

If repo is checked out incorrectly locally, a tag can be lightweight but annotated in origin. To guarantee chalk sees up-to-date tags, chalk refetches lightweight tags to ensure it reports correct metadata about the "latest" tag.

This was noticed via checkout github action which if triggered from a tag build it looses any tag annotations.

Testing

➜ make tests args="test_git.py::test_refetch_tag --logs --pdb"

Base automatically changed from ms/tag to main June 19, 2024 17:23
If repo is checked out incorrectly locally, a tag can be lightweight
but annotated in origin. To guarantee chalk sees up-to-date tags,
chalk refetches lightweight tags to ensure it reports correct metadata
about the "latest" tag.

This was noticed via checkout github action which if triggered from a
tag build it looses any tag annotations.
@miki725 miki725 marked this pull request as ready for review June 19, 2024 18:16
@miki725 miki725 requested a review from viega as a code owner June 19, 2024 18:16
@miki725 miki725 requested a review from ee7 June 20, 2024 05:28
ee7
ee7 previously approved these changes Jun 20, 2024
Copy link
Contributor

@ee7 ee7 left a comment

Choose a reason for hiding this comment

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

If the same commit has two tags:

  • an annotated tag 0.1.0
  • and a lightweight tag 0.1.1

I guess we do want to report the details for 0.1.0? That's the current behavior, and I'm OK with that.

@miki725
Copy link
Contributor Author

miki725 commented Jun 20, 2024

so far annotated tags have a preference over lightweight tags as they have a concrete ordering without doing any tag name comparisons as tag formats are wildly different with semver, dates, etc so nothing concrete to sort by. if there are no annotated tags then it falls back to regular tags

return
var args = @[
"fetch",
"origin",
Copy link
Contributor

@ee7 ee7 Jun 20, 2024

Choose a reason for hiding this comment

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

For later: maybe we should think about not hard-coding origin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm let me see if we figure out the correct remote name in the parsing although we might still want to hardcode to origin as thats standard in CI and locally people might have all sort of remotes. not sure. your thoughts?

Copy link
Contributor

@ee7 ee7 Jun 20, 2024

Choose a reason for hiding this comment

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

Might be worth parsing e.g. git remote or git remote -v and, when both:

  • there is only one remote
  • the only remote is not named origin

we use that remote.

For the case where there are multiple remotes and none are named origin, I don't know. But that's probably not very important.

Copy link
Contributor

Choose a reason for hiding this comment

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

As an n=1, personally, I prefer to name every remote explicitly. So if I have a fork on GitHub of repo foo from org, I typically have:

  • A remote named ee7 that tracks https://github.com/ee7/foo
  • A remote named org that tracks https://github.com/org/foo

and when I don't maintain a fork, there's only one remote:

  • A remote named org that tracks https://github.com/org/foo

That is, no origin, and it's always clear where I'm pushing.

And furthermore, in my git config I have:

[push]
    default = nothing

So that:

$ git push
fatal: You didn't specify any refspecs to push, and push.default is "nothing".

But yes, in most situations for chalk, assuming origin is probably OK most of the time. But it's not guaranteed to be correct.

Probably better to think about this later - there's probably things that are affected outside this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. if we make that generic prolly worth adding additional keys like GIT_REMOTE_NAME or something as we currently have ORIGIN_URI which I guess presumes origin although the git config parsing uses first remote if origin is not found

for (sec, subsec, kvpairs) in conf:
if sec == ghRemote:
if subsec == ghOrigin:
for (k, v) in kvpairs:
if k == ghUrl:
self.origin = v
return v
elif firstFound == "":
for (k, v) in kvpairs:
if k == ghUrl:
firstFound = v
break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually you can pass a url to git fetch command directly so will use that for now. this way it will use whatever remote chalk detects otherwise...

@miki725
Copy link
Contributor Author

miki725 commented Jun 20, 2024

for reference:

let sortedTags = info.tags.values().toSeq().sortedByIt((it.unixTime, it.name))

@miki725 miki725 merged commit 12a968d into main Jun 20, 2024
4 checks passed
@miki725 miki725 deleted the ms/fetch branch June 20, 2024 14:45
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.

annotatated/signed tags are misreported as regular tags
2 participants