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.

Sign up for GitHub

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

Fixes #299 #300

Merged
merged 1 commit into from
Mar 17, 2023
Merged

Fixes #299 #300

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion get_git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,9 @@ func setupGitEnv(cmd *exec.Cmd, sshKeyFile string) {
sshKeyFile = strings.Replace(sshKeyFile, `\`, `/`, -1)
}
sshCmd = append(sshCmd, "-i", sshKeyFile)
env = append(env, strings.Join(sshCmd, " "))
}

env = append(env, strings.Join(sshCmd, " "))
cmd.Env = env
}
Copy link
Contributor Author

@nl-brett-stime nl-brett-stime Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please expand/unfold the lines above so that you can see the whole setupGitEnv function--it's not long.

The only place sshCmd is modified, and would therefore need to be written back to the environment, is if sshKeyFile != "" (line 263). Apart from that case, there's currently no reason to initialize a default sshCmd as in these lines from above (259-261):

	if len(sshCmd) == 0 {
		sshCmd = []string{gitSSHCommand + "ssh"}
	}

If you feel like there might be other types of modifications/mutations in the future (besides sshKeyFile), and you want to make it relatively clear/easy how to add them, then I'd recommend adding a variable like hasSshCmdBeenModified bool. E.g.,

var hasSshCmdBeenModified = false
if len(sshCmd) == 0 {
	// Ensure a default command is initialized in case there are any subsequent modifications
	sshCmd = []string{gitSSHCommand + "ssh"}
}
if sshKeyFile != "" {
	sshCmd = addKeyModification(sshCmd, sshKeyFile)
	hasSshCmdBeenModified = true
}
if shouldDoModificationB {
	sshCmd = doModificationB(sshCmd)
	hasSshCmdBeenModified = true
}
if shouldDoModificationC {
	sshCmd = doModificationC(sshCmd)
	hasSshCmdBeenModified = true
}
if hasSshCmdBeenModified {
	// Don't append unless there was an actual modification.
	// The value of sshCmd might be the temporary default that was prepared
	// for modifications but didn't end up being needed.
	env = append(env, strings.Join(sshCmd, " "))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...or maybe:

baseSshCmdForModification = []string{gitSSHCommand + "ssh"}
if len(sshCmd) == 0 {
	// Ensure a default command is initialized in case there are any subsequent modifications
	sshCmd = baseSshCmdForModification
}
if sshKeyFile != "" {
	sshCmd = addKeyModification(sshCmd, sshKeyFile)
}
if shouldDoModificationB {
	sshCmd = doModificationB(sshCmd)
}
if shouldDoModificationC {
	sshCmd = doModificationC(sshCmd)
}
if sshCmd != baseSshCmdForModification {
	env = append(env, strings.Join(maybeModifiedSshCmd, " "))
}


Expand Down