Skip to content

Conversation

@nullishamy
Copy link
Contributor

This adds checks for bare repositories, which are unsupported, and errors gracefully if we encounter one.

Related: #2066

@jesseduffield
Copy link
Owner

Rather than exit the program, it would be better to ask the user if they want to open lazygit anyway, to the most recently opened repo. E.g.:

You've attempted to open Lazygit in a bare repo but Lazygit does not yet support bare repos. Open most recent repo? Y/N

If the user presses Y, then we'll do the same thing we currently do when the user opens lazygit in a non-repo and chooses not to initialise a new one. Otherwise, we'll exit the program.

Another point: we should move the message into pkg/i18n/english.go so that it can be translated into other languages (don't worry, you don't need to actually do any translating in this PR, it just makes things easier for others who want to add translations).

Lemme know if you need any pointers :)

@nullishamy
Copy link
Contributor Author

Yeah i wasnt sure how to handle the case where we need to exit, but just left it for review because its not a huge deal to change. Also wasnt sure how to operate the i18n, but judging from existing usage it shouldn't be too hard. Will work on this soon.

@nullishamy
Copy link
Contributor Author

I'm not sure if we should factor out the "switch to a recent repo" logic or not, thoughts?

@mark2185
Copy link
Collaborator

mark2185 commented Aug 1, 2022

I'm not sure if we should factor out the "switch to a recent repo" logic or not, thoughts?

Does seem reasonable, and since you're already in that neck of the woods...

@nullishamy
Copy link
Contributor Author

Will implement that, where should it go? Unsure if we'd need to use it in other files and whatnot, so not sure what place would be best.

@mark2185
Copy link
Collaborator

mark2185 commented Aug 1, 2022

All three references to isDirectoryAGitRepository are in app.go, so I guess that is as good place as any.

But Jesse has a better mental model of the entire project so I'd rather hear his two cents clink.

@nullishamy
Copy link
Contributor Author

Right, I've put it in there for now, will wait for Jesse's opinion on it and alter if needed.

pkg/app/app.go Outdated
}

func isBareRepo(osCommand *oscommands.OSCommand) (bool, error) {
res, err := osCommand.Cmd.New("git rev-parse --is-bare-repository").DontLog().RunWithOutput()
Copy link
Collaborator

@mark2185 mark2185 Aug 1, 2022

Choose a reason for hiding this comment

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

@jesseduffield should we somehow conflate this isBareRepo and this one?

Currently I think the one from recent_repos_panel isn't reachable from app.go, but maybe it's better to make it public rather than have a duplicate?

Copy link
Owner

Choose a reason for hiding this comment

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

Good idea, let's make the existing one public like you say

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would that look, and do you want that implemented in this PR? Also how do I go about accessing the existing function?

Copy link
Collaborator

Choose a reason for hiding this comment

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

How would that look, and do you want that implemented in this PR?

It seems like a trivial change, I see no reason why to postpone it.

Also how do I go about accessing the existing function?

Should be accessible through app.IsBareRepo once you make it public (capitalize it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll give it a go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I say we wait for Jesse to see if he can deus ex machina this pickle.

I also say we extract it to something like pkg/commands/git_commands/misc.go, throw in IsDirectoryAGitRepository as well and call it a day.

We'll see!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meaaaaaan that's not even a bad idea per-se. But we can wait for Jesse to comment on this one. Go is a real pain sometimes.

Copy link
Owner

Choose a reason for hiding this comment

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

the app package can depend on the commands package (which contains git.go) but not vice-versa. I see two approaches we can take here:

  1. make it so that we can instantiate the git struct (in git.go) without having to do the validation stuff that currently happens there.
  2. Move the code we've got here into pkg/commands/git_commands/status.go Like so:
func (self *StatusCommands) IsBareRepo() (bool, error) {
	return IsBareRepo(self.os)
}

func IsBareRepo(osCommand *oscommands.OSCommand) (bool, error) {
	res, err := osCommand.Cmd.New("git rev-parse --is-bare-repository").DontLog().RunWithOutput()
	if err != nil {
		return false, err
	}

	// The command returns output with a newline, so we need to strip
	return strconv.ParseBool(strings.TrimSpace(res))
}

Then in app.go we call IsBareRepo, passing in our os struct.

I would prefer option 1 but that deserves its own PR, so I'm happy to go with option 2 for now.

Copy link
Owner

Choose a reason for hiding this comment

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

the go-git method is very fast btw because it's just checking if a value is nil, but this function is only called when you want to view recent repos so it's not a hot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should be that sorted, took me a while to figure out how to import the function lol.

@jesseduffield
Copy link
Owner

@nullishamy code looks good. You'll need to run gofumpt i.e.

go install mvdan.cc/gofumpt@latest && gofumpt -l -w .

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

see comment

@nullishamy
Copy link
Contributor Author

Never saw your comment, I believe that's what you wanted?

@nullishamy nullishamy requested review from jesseduffield and removed request for mark2185 August 18, 2022 17:35
@jesseduffield jesseduffield merged commit 014daf7 into jesseduffield:master Aug 18, 2022
@jesseduffield
Copy link
Owner

nice work @nullishamy !

@nullishamy nullishamy deleted the feat/detect-bare-repo branch August 18, 2022 23:44
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.

3 participants