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

cmd/go: go get needs some way to set missing user.email (maybe user.name) information in repos it pulls #16516

Closed
dakotahawkins opened this issue Jul 27, 2016 · 12 comments

Comments

@dakotahawkins
Copy link

dakotahawkins commented Jul 27, 2016

  1. What version of Go are you using (go version)?

    go version go1.6.2 windows/amd64
    
  2. What operating system and processor architecture are you using (go env)?

    set GOARCH=amd64
    set GOBIN=
    set GOEXE=.exe
    set GOHOSTARCH=amd64
    set GOHOSTOS=windows
    set GOOS=windows
    set GOPATH=E:\src\Go
    set GORACE=
    set GOROOT=C:\Go
    set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
    set GO15VENDOREXPERIMENT=1
    set CC=gcc
    set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
    set CXX=g++
    set CGO_ENABLED=1
    
  3. What did you do?

    • Global .gitconfig has user.name set and user.useConfigOnly = true, while user.email is purposefully unset.
      • This is to support using a personal address for personal repos, and a work address for work repos.
  4. What did you expect to see?

    • Some mechanism for setting the missing information ahead of time
  5. What did you see instead?

    • go get pulls from git remotes (e.g. github) fail because their individual repo configs require user.email to be set
      • Especially annoying when another tool is trying to do this for you (e.g. vscode)
@quentinmit
Copy link
Contributor

Why does the pull fail? Can you please provide the exact output of the command? The Git documentation says that user.useConfigOnly affects only commits, so it shouldn't interfere with the clone that go get is doing.

@dakotahawkins
Copy link
Author

dakotahawkins commented Jul 28, 2016

(Github went down for a few minutes, I replied to my email notification about your comment after that but I don't know if it will go anywhere or show up here later or what)

Sure. It seems to let you clone but not pull updates. I can't really explain (based on the documentation) why it does this, but it does. Perhaps the note about commits is just intended as an example, I don't know.

It is easiest to replicate without involving go directly (I'll use this repo). In any case, the error is identical:

$ git clone https://github.com/golang/go.git
Cloning into 'go'...
remote: Counting objects: 264675, done.
remote: Compressing objects: 100% (11/11), done.
remote: Total 264675 (delta 2), reused 1 (delta 1), pack-reused 264663
Receiving objects: 100% (264675/264675), 104.84 MiB | 23.51 MiB/s, done.
Resolving deltas: 100% (210291/210291), done.
Checking connectivity... done.
Checking out files: 100% (5650/5650), done.

$ cd go

$ git pull

*** 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.

fatal: no email was given and auto-detection is disabled
You need to set your committer info first

@dakotahawkins
Copy link
Author

In this specific case it's not repos I want to look at or contribute to, just ones vscode's golang extension wants to update to provide its IDE functionality. To do this it tries to go get them. I'm not really in the loop, except to say "sure install/update those things" and then to observe the failures.

Presumably anything else trying to use go get to pull in updates to dependencies would be susceptible to the same thing, and so in this case it would be nice to at least be able to tell go what I'd like that default to be in repos it clones, or to have it work around that requirement somehow.

@dakotahawkins
Copy link
Author

dakotahawkins commented Jul 29, 2016

Ah ha!

This is because I have pull set to rebase by default:

[pull]
        rebase = preserve

git pull --rebase=false doesn't ask me to identify myself.

Still, that's a sensible setting for my day to day git use, and I'm not going to turn it off in my global config (like I'm not going to set my e-mail in my global config).

If go is going to do pulls for me, perhaps it should turn rebase off for them.

@quentinmit
Copy link
Contributor

I'm afraid this feels like somewhere between WAI and a big in Git to me. You have configured Git is such a way that it can't make commits, and then you're asking go get to do a thing that might involve making commits. It's not up togo get to fix your broken configuration. If there were a reasonable default value for user.email that go get could set, you would have it in your global config already.

That said, the bug in Git is that it would treat them differently. Both rebase=true and rebase=false can make commits, and IIUC in the same situations.

I'm going to close this issue as WAI as far as Go is concerned. If you think there is some specific behavior Go could have to make this better, please comment, but AFAICT there isn't.

@dakotahawkins
Copy link
Author

It may be that it is a bug in Git, though while I can't think of a great reason for it to behave that way I'd suspect it's for some valid reason.

Either way, it's absolutely not a "broken configuration". Those two settings are the way that they are for good reason, and in working directly with a git repo it's no problem (in fact it's preferable) to set whichever email I need to set in the local config. It's probably somewhat common to do that with this setting -- I don't want to accidentally commit to a personal or public repo with my work e-mail address and I similarly don't want to commit to work repos with my personal address. So, if it works as intended then it wasn't intended to work very well.

I think there are at least four different ways Go could behave to make this better:

  1. It could make sure it knows user.name and user.email prior to attempting operations that may require them, and request whatever is missing ahead of time interactively, or
  2. It could alter the way it pulls code to enforce the default configuration it's currently just expecting to exist (--rebase=false for pull and I think --checkout for submodule update), or
  3. It could behave more or less like it does now but provide preferences (environment variables, I guess?) that users could set that would then be set in local git configs after an initial clone, or
  4. It could just look for a "template" config file in $GOPATH to merge into local configs

If Go is just going to run git commands on my behalf, and (compounding the issue) if things that work with Go are going to run those commands that run the git commands on my behalf, it's just extra annoying due to the additional steps I'm removed from using git directly.

I'm not saying none of those options have potential downsides, but I think we could probably find one with more net positives than negatives.

It's also obviously not a critical problem and it has (annoying) workarounds, but at the moment it doesn't make using Go very pleasant some of the time, and I think it would be a shame if the consensus was that that's the intent.

At the very least I think it would be worth mentioning this in the docs somewhere if we've identified a (valid) git configuration that doesn't work seamlessly with Go's usage of git.

@dakotahawkins
Copy link
Author

Bonus 5th way, if it's not possible already, would be to somehow allow relatively easy overriding of the vcs commands as a preference, similar to picking up an optional config template but perhaps more generic.

@paranoiacblack
Copy link
Contributor

Wouldn't it be easier to make a local .gitconfig in somewhere like your $GOPATH and just run go get from there so you don't have to worry about your global .gitconfig interfering? All git commands on package repos would then have the pull rebasing turned off. I don't think that's an annoying workaround.

We already do submodule update (https://github.com/golang/go/blob/master/src/cmd/go/vcs.go#L136).
If you'd like to add a comment to the go get, that's fine, but I think go get does not need more complicated vcs support.

@dakotahawkins
Copy link
Author

dakotahawkins commented Jul 29, 2016

It would be easier to do that, but I don't think git will just walk up the directory tree until it finds one (or at least the docs say it doesn't): https://git-scm.com/docs/git-config#FILES

With respect to submodules, my point is that like pull you can configure submodule update to rebase by default, however re-reading that part of the git-config doc it looks like that might have to be specified per-specific-submodule, in which case I think it would be OK here. If that's not the case for whatever reason adding --checkout to the submodule update command would force it to use its default behavior and should override your config setting:

This option is only valid for the update command. Checkout the commit recorded in the superproject on a detached HEAD in the submodule. This is the default behavior, the main use of this option is to override submodule.$name.update when set to a value other than checkout. If the key submodule.$name.update is either not explicitly set or set to checkout, this option is implicit.

@quentinmit
Copy link
Contributor

It sounds like your goal is to have different Git configuration in the repositories that go get clones, compared to the other Git repositories on your system. I've tried to explain below why that's not a typical case, but in any event, I think it should be doable by writing a post-checkout hook:

git config --global core.hooksPath E:\hooks
#!/bin/sh
if [[ "$GIT_DIR" = E:\src\Go* ]]; then
git config user.email "goemail@corp.com"
fi

It could make sure it knows user.name and user.email prior to attempting operations that may require them, and request whatever is missing ahead of time interactively, or

Git is already doing this check, as you pointed out by showing the output of "git pull". I don't think it's worth duplicating this check in go get. There are probably a hundred other configurations that cause errors too, should we duplicate all of Git's logic in go get?

It could alter the way it pulls code to enforce the default configuration it's currently just expecting to exist (--rebase=false for pull and I think --checkout for submodule update), or

Setting --rebase=false doesn't prevent a commit from being created, and even if it did, people have preferences for rebase vs. merge. (You yourself, even, since you have pull.rebase set.) I think perhaps there's a disconnect in what go get is for. If you think of it solely as a package manager, then yeah, it makes a lot of sense to enforce a single, working, Git configuration. But go get is not just a package manager - it's a tool for managing your GOPATH, where user-written code also lives. Consider, for instance, if a user go gets a package, edits the code and commits locally, and then tries to update. Clearly, we should be using the user's preference for rebase vs. merge here - we shouldn't force them to use a merge (by passing --rebase=false). It is very common for users to directly manipulate the Git repositories in GOPATH.

It could behave more or less like it does now but provide preferences (environment variables, I guess?) that users could set that would then be set in local git configs after an initial clone, or

I don't see why those preferences need to be anywhere other than where they are now, .git/config. As I tried to explain above, the Git repositories that go clones are not special in any way.

It could just look for a "template" config file in $GOPATH to merge into local configs

Ditto, this is already the purpose of .git/config, why invent another config file?

@dakotahawkins
Copy link
Author

dakotahawkins commented Jul 29, 2016

I think it should be doable by writing a post-checkout hook

That's actually a really good idea, I'll see if it works!

Git is already doing this check, as you pointed out by showing the output of "git pull". I don't think it's worth duplicating this check in go get. There are probably a hundred other configurations that cause errors too, should we duplicate all of Git's logic in go get?

My argument that it might be worth it is go keeps those repos pretty deeply nested, likely inconveniently far away from whatever your cwd is or whatever you're trying to actually work on (again, even worse if it's something else running go get outside of a terminal, but that's not go's fault). It's an annoying context switch, and I think it would be reasonably easy to avoid in this case.

Setting --rebase=false doesn't prevent a commit from being created, and even if it did, people have preferences for rebase vs. merge.

Agreed, what it does do is let you fast-forward if that's possible, and you can do that without becoming an author or committer. If I only got this failure when fast-forward wasn't possible in package repos, it probably wouldn't have bothered me enough to think about it.

If you think of it solely as a package manager, then yeah, it makes a lot of sense to enforce a single, working, Git configuration. But go get is not just a package manager - it's a tool for managing your GOPATH, where user-written code also lives.

Sure, I understand that. This problem just happens to be with the packages for me. For a repo I'm developing in I'm just using git like normal, and it's unlikely anything would trigger a pull on that except for me doing it manually because I want to pull it. I'm much less aware of what's pulling which package when, where, and why. They're third-party packages, something requires them, I'd prefer they just worked however they need to work. I admit I do not know (being new to go) if there is any distinction made in go between working repos and dependencies that get pulled to act as packages. Either way would probably seem reasonable to me, for different reasons.

I don't see why those preferences need to be anywhere other than where they are now, .git/config. As I tried to explain above, the Git repositories that go clones are not special in any way.

Yeah I like that idea the least. They're not special, they're just (packages anyway) far removed from whatever somebody is actually trying to work on when one gets cloned or updated.

Ditto, this is already the purpose of .git/config, why invent another config file?

This would not be my first choice either, but if you have something that wants to clone or update a bunch of packages, it's annoying to go update a bunch of .git/config files every time.

If go is trying to manage this work behind the scenes on behalf of the user, which it is, I don't think it's a good idea for it to assume that everybody's global git config by itself will allow go to manage clone/pull/whatever operations by itself on a bunch of repositories. I don't think it's a good idea precisely because git is designed to have tiered configurations. The global one is settings that apply to all of your user's repos machine-wide, the local one is for settings that need to be different in different repositories. If go wants to more-or-less-seamlessly manage some git repos, it should let me help it do that somehow.

So, I think beyond the scope of this specific problem, it's not great that go doesn't provide some way to say "hey, here's how I want the repositories you go get configured, please."

With regards to this specific issue, I've also pinged the git mailing list to ask why rebase doesn't avoid this check when it should know it won't make new commits, and/or why it doesn't detect this and fast-forward instead of rebasing in this case since the end-result should be identical AFAIK. If and when they respond, I'll let you guys know what they say in case you're interested.

@dakotahawkins
Copy link
Author

They started responding, and they seem to mostly agree that an early ident check could probably (and should probably) be avoided in this specific case, but they are not sure why the line responsible for it exists since it was added along with the whole of the interactive rebase feature.

@golang golang locked and limited conversation to collaborators Jul 29, 2017
@mikioh mikioh changed the title go get needs some way to set missing user.email (maybe user.name) information in repos it pulls cmd/go: go get needs some way to set missing user.email (maybe user.name) information in repos it pulls Aug 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants