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

Compiler specs fail if git is configured with a different defaultBranch #11753

Closed
yxhuvud opened this issue Jan 18, 2022 · 11 comments · Fixed by #11754
Closed

Compiler specs fail if git is configured with a different defaultBranch #11753

yxhuvud opened this issue Jan 18, 2022 · 11 comments · Fixed by #11754
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs

Comments

@yxhuvud
Copy link
Contributor

yxhuvud commented Jan 18, 2022

Bug Report

How to reproduce:

Have a .gitconfig that includes

[init]
	defaultBranch = main

In checked out crystal repo, run the compiler specs:

make clean
make
make compiler_spec

This results in 3 faileures:

crystal spec spec/compiler/crystal/tools/doc/project_info_spec.cr:93 # Crystal::Doc::ProjectInfo #fill_with_defaults with shard.yml git non-tagged commit
crystal spec spec/compiler/crystal/tools/doc/project_info_spec.cr:105 # Crystal::Doc::ProjectInfo #fill_with_defaults with shard.yml git non-tagged commit dirty
crystal spec spec/compiler/crystal/tools/doc/project_info_spec.cr:142 # Crystal::Doc::ProjectInfo .find_git_version

All three are failing due to master being hardcoded.

  • Include Crystal compiler version (crystal -v) and OS. If possible, try to see if the bug still reproduces on master.

I had 1.3.1 checked out when I ran into the issue.

@yxhuvud yxhuvud added the kind:bug A bug in the code. Does not apply to documentation, specs, etc. label Jan 18, 2022
@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 18, 2022

Do we have any version requirement on git? It turns out defaultBranch was added in 2.28.0. Would it be ok to add --initial-branch=master to the git initialization in those specs? I believe (but havn't verified) that option was also added then.

@HertzDevil
Copy link
Contributor

Should we do the opposite and query for init.defaultbranch in the specs?

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 19, 2022

Right, that is probably safer. I was initially under the impression that fetching unknown keys gave an error message, but that is not the case.

@straight-shoota
Copy link
Member

I would rather prefer to eliminate influence of local configuration from the spec code instead of adapting to it. We're better off using the same well defined behaviour everyhwere.

That means adding --initial-branch=master to git init.

@lbguilherme
Copy link
Contributor

lbguilherme commented Jan 19, 2022

Another approach could be to ignore global and system configs by setting the env variables GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to point to empty files.

Ref: https://git-scm.com/docs/git-config#Documentation/git-config.txt-GITCONFIGGLOBAL

@straight-shoota
Copy link
Member

That might even be better because other settings might influence git behaviour as well 👍
It only works as long as git doesn't change the default, though.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 19, 2022

@straight-shoota Adding --initial-branch=master means adding a requirement on git 2.28.0 to run the test suite. Is that old enough that it is ok?

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 19, 2022

@lbguilherme perhaps even GIT_CONFIG_NOSYSTEM could be pretty nice. Then there wouldn't be any need to set up any temporary files.

@lbguilherme
Copy link
Contributor

Unfortunately GIT_CONFIG_NOSYSTEM doesn't stop git from using the global config at your home, only the system-wide one.

@yxhuvud
Copy link
Contributor Author

yxhuvud commented Jan 19, 2022

An alternative could be to simply checkout some directory with
git checkout -b master
Would add an extra line but would perhaps be a bit more explicit and clearer, while avoiding the version issues?

@straight-shoota
Copy link
Member

straight-shoota commented Jan 21, 2022

We should go with git checkout -b master after git init in the affected specs.
I suppose it's even an improvement for reading the spec code because it explicitly defines the value.

And it's a smaller diff and less introduced complexity than the current proposal in #11754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:specs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants