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

Check if name or email are present on global level before adding to git locally #106

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lechner
Copy link

@lechner lechner commented Aug 27, 2018

Untested, but is it approximately what you had in mind? Thank you!

Copy link
Collaborator

@stapelberg stapelberg left a comment

Choose a reason for hiding this comment

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

Looks like the right approach overall :)

var stdout bytes.Buffer
cmd := exec.Command("git", gitArgs...)
cmd.Stdout = &stdout
cmd.Stderr = ioutil.Discard
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems redundant — not setting cmd.Stderr has the same effect, no?

gitArgs := append([]string{"config", "--get", "--null"}, args...)
var stdout bytes.Buffer
cmd := exec.Command("git", gitArgs...)
cmd.Stdout = &stdout
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of dealing with the buffer manually, use cmd.Output() instead of cmd.Run() below.

if exitError, ok := err.(*exec.ExitError); ok {
if waitStatus, ok := exitError.Sys().(syscall.WaitStatus); ok {
if waitStatus.ExitStatus() == 1 {
return "", &ErrNotFound{Key: args[len(args)-1]}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don’t have the ErrNotFound type, so just return err here

@@ -279,6 +279,26 @@ func runGitCommandIn(dir string, arg ...string) error {
return cmd.Run()
}

func execGitConfig(args ...string) (string, error) {
gitArgs := append([]string{"config", "--get", "--null"}, args...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment as to why --null is required here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will. Per git-config manpage, it "allows for secure parsing of the output without getting confused e.g. by values that contain line breaks." Thank you!

@anthonyfok
Copy link
Member

I think adding the name and email at the local level is a good thing, so I have reservations about merging this PR. What does everyone else think?

@lechner
Copy link
Author

lechner commented Aug 26, 2021

@anthonyfok Hi, do you really do that for any of your other Git repos? What if your email address changes?

@anthonyfok anthonyfok self-requested a review August 26, 2021 14:52
@anthonyfok anthonyfok force-pushed the master branch 2 times, most recently from f0abee6 to 989837a Compare March 22, 2024 09:47
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