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 initial commit when git user email is note defined. #207

Merged
merged 3 commits into from
Jul 15, 2017

Conversation

knight-bubble
Copy link
Contributor

I am sure the initial message shouldn't be suppressed, and if you know how to display it please tell me.
But that commit at least fixes #202

@rkostrzewski
Copy link
Collaborator

rkostrzewski commented Jul 8, 2017

Hi @knight-bubble thanks for raising the issue & making a PR 🎉.

I just found this on stack overflow. Do you think we could use this to set commit email and username if it isn't set ? I was wondering if doing sth along these lines would work:

let defaultUserEmail = 'developit@users.noreply.github.com'
let defaultUserName = 'Preact CLI'
let isUserSet = true

try {
  // Both throw error when they are not set 
  await spawn('git', ['config', 'user.name'])
  await spawn('git', ['config', 'user.email'])
} catch (err) {
  isUserSet = false
}

if (!isUserSet) {
  await spawn('git', ['commit', '-c', `user.name=${defaultUserName}`, '-c', `user.email=${defaultUserEmail}`, '-m', 'Initial commit from Preact CLI'], { cwd });
} else {
  await spawn('git', ['commit', '-m', 'Initial commit from Preact CLI'], { cwd });
}

@knight-bubble
Copy link
Contributor Author

@rkostrzewski I've tried using the "-c" option too, but it seems to not work also it gives a different error, but couldn't figured out which one 😢.
But I like your solution so I'll refactor the PR :)

@knight-bubble
Copy link
Contributor Author

@rkostrzewski Running you command gives this kind of output:
fatal: Option -m cannot be combined with -c/-C/-F/--fixup.
So i think we should keep setting of global user and unset it.

@knight-bubble knight-bubble force-pushed the fix-git-init-unset-email branch from a96a56e to a7f6e32 Compare July 8, 2017 16:20
@rkostrzewski
Copy link
Collaborator

@knight-bubble oh that could be because in the original StackOverflow post there was git commit -c .. -m 'message'

await spawn('git', ['-c', `user.name=${defaultUserName}`, '-c', `user.email=${defaultUserEmail}`, 'commit', '-m', 'Initial commit from Preact CLI'], { cwd });

I'm kida afraid that if anything throws an error for some reason the global user won't be unset.

@knight-bubble
Copy link
Contributor Author

@rkostrzewski You are right. What if instead of using git command, we will set GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL? There should be a way to call spawn with specific env variables.

@knight-bubble
Copy link
Contributor Author

@rkostrzewski This seems to work

await spawn('git', ['commit', '-m', 'initial commit from Preact CLI'], {
	cwd,
	env: {
		...(!isUserNameSet ? { GIT_COMMITTER_NAME: gitUser} : {}),
		...(!isUserEmailSet ? { GIT_COMMITTER_EMAIL: gitEmail} : {}),
		GIT_AUTHOR_NAME: gitUser,
		GIT_AUTHOR_EMAIL: gitEmail
	}
});

@developit
Copy link
Member

Maybe can simplify that to:

await spawn('git', ['commit', '-m', 'initial commit from Preact CLI'], {
	cwd,
	env: {
		GIT_AUTHOR_NAME: gitUser || defaultGitUser,
		GIT_AUTHOR_EMAIL: gitEmail || defaultGitEmail
	}
});

@knight-bubble
Copy link
Contributor Author

knight-bubble commented Jul 8, 2017

@developit Just tested without GIT_COMMITTER_NAME and GIT_COMMITTER_EMAIL. And it doesn't work.

@rkostrzewski
Copy link
Collaborator

@knight-bubble sounds great.
@developit I think GIT_COMMITER is a must and GIT_AUTHOR is not required (and would be taken from GIT_AUTHOR)

@lukeed
Copy link
Member

lukeed commented Jul 9, 2017

I'm not sure I follow --- why do we need to specify a Git author name/email? If you just removed that, it should work fine, no?

I have a few yeoman generators, and in them I just spawn git with a commit message, and my/user's git config info is picked up automatically.

@knight-bubble
Copy link
Contributor Author

@lukeed You are right. But if the user email is not specified, the git initialization function fails.
Try to unset user.email in git config an try to commit.
You should get the following message in the terminal:

*** Please tell me who you are.
Run
  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"
to set your account's default identity.
Omit --global to set the identity only in this repository.

@lukeed
Copy link
Member

lukeed commented Jul 9, 2017

Ah, okay. Does it throw? If so, then a try/catch sounds right. No point in committing if user can't commit on their own anyways~

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

LGTM

@developit
Copy link
Member

developit commented Jul 10, 2017

^ I was a dolt and didn't notice the different vars, ignore me 😅

@prateekbh
Copy link
Member

@developit @rkostrzewski @knight-bubble what exactly is stopping us from merging this right away?

@reznord
Copy link
Member

reznord commented Jul 12, 2017

This PR might be effected based on the discussions in #216

@prateekbh
Copy link
Member

oh no, This PR mainly fixes the commit author, whereas #216 mainly will just make the auto --git flag as default false.

if user still passes --git, running these commands would make sense

@prateekbh
Copy link
Member

@rkostrzewski @developit is this good to go? another PR is pending on this

@prateekbh prateekbh merged commit 32b31e8 into preactjs:master Jul 15, 2017
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.

Git initialization error
6 participants