Skip to content

handle edge case with setting default repo - #6810

Closed
vilmibm wants to merge 2 commits into
trunkfrom
fix-6808
Closed

handle edge case with setting default repo#6810
vilmibm wants to merge 2 commits into
trunkfrom
fix-6808

Conversation

@vilmibm

@vilmibm vilmibm commented Jan 4, 2023

Copy link
Copy Markdown
Contributor

Fixes #6808

gnarly hack for gnarly times

@vilmibm
vilmibm requested a review from a team as a code owner January 4, 2023 23:55
@vilmibm
vilmibm requested review from samcoe and removed request for a team January 4, 2023 23:55
Comment on lines +198 to +202
if currentDefaultRepo.Remote != nil {
if currentDefaultRepo.Remote.Resolved != "base" {
current = currentDefaultRepo.Remote.Resolved
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we need to make the check that the remote is not nil? Seems like that is already guaranteed by virtue of currentDefaultRepo being set from ResolvedRemote().

Additionally if the resolved value is "base" should we be keeping the previous behavior of current = ghrepo.FullName(currentDefaultRepo)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do we need to make the check that the remote is not nil? Seems like that is already guaranteed by virtue of currentDefaultRepo being set from ResolvedRemote().

I suppose not but I felt like being defensive.

Additionally if the resolved value is "base" should we be keeping the previous behavior of current = ghrepo.FullName(currentDefaultRepo)?

no; that is precisely the case that resulted in the panic of #6808

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of this workaround, which is not very self-evident to me even with the added explanatory comment, could the workaround be to add a guard around Prompter.Select to ensure that current exists within repoNames and make the default blank if not?

Comment on lines +198 to +202
if currentDefaultRepo.Remote != nil {
if currentDefaultRepo.Remote.Resolved != "base" {
current = currentDefaultRepo.Remote.Resolved
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of this workaround, which is not very self-evident to me even with the added explanatory comment, could the workaround be to add a guard around Prompter.Select to ensure that current exists within repoNames and make the default blank if not?

@vilmibm

vilmibm commented Jan 9, 2023

Copy link
Copy Markdown
Contributor Author

closing in favor of #6831

@vilmibm vilmibm closed this Jan 9, 2023
@nahnicholas

This comment was marked as spam.

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.

gh repo set-default produces panic: runtime error: index out of range [-1]

5 participants