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

nix-prefetch-git: Run git-init with --initial-branch=master #113313

Merged

Conversation

primeos
Copy link
Member

@primeos primeos commented Feb 16, 2021

Edit: Please cast your vote on master vs. main here: #113313 (comment)


The reason for this change is simply to avoid the following messages
that are unnecessary and can be confusing (and these messages will be
repeated for each submodule):

hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint:
hint:   git config --global init.defaultBranch <name>
hint:
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint:
hint:   git branch -m <name>

This shouldn't cause any hashes to change and nix-prefetch-git uses the
"fetchgit" branch anyway:

Switched to a new branch 'fetchgit'

For that reason the initial branch name doesn't really matter anyway and
since we're not relying on / hardcoding "master" we can simply switch to
"main" (which seems most common nowadays).

Motivation for this change

Without this change (I'm using an image to showcase the yellow color that makes it look like a warning):
image

With this change:
image

Tested using (edit: --check doesn't work to test this btw, which I only found out later - luckily I ran additional tests though):

nix-build -A iwd.src --check # Doesn't use submodules
nix-build -A tev.src --check # Uses submodules, even recursively

cc @bjornfor

We could (/should?) also test this on staging first.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Feb 16, 2021

Result of nixpkgs-review pr 113313 at 894b1551 run on aarch64-linux 1

11 packages built:

Result of nixpkgs-review pr 113313 at 894b1551 run on x86_64-linux 1

12 packages built:

@SuperSandro2000
Copy link
Member

which seems most common nowadays

Please use master anyway because it was the previous default and this way we can be sure nothing else in the ecosystem breaks.

pkgs/build-support/fetchgit/nix-prefetch-git Outdated Show resolved Hide resolved
pkgs/build-support/fetchgit/nix-prefetch-git Outdated Show resolved Hide resolved
@primeos
Copy link
Member Author

primeos commented Feb 17, 2021

Please use master anyway because it was the previous default and this way we can be sure nothing else in the ecosystem breaks.

I thought about that too but given that Git will AFAIK switch to main as new default and using master is now controversial it seemed better to switch to main right away (before anyone complains that we're explicitly using master).

And again: "[AFAIK] the initial branch name doesn't really matter anyway and since we're not relying on / hardcoding master we can simply switch to main". From the code the only difference should be in the console output (doesn't affect the hash!) if branchName is set to master or main (the console output is Switched to a new branch '...' vs. Already on '...').

nix-prefetch-git will always switch to a branch and the only trace of the initial branch name will be in .git/logs/HEAD [1] and even if leaveDotGit=true all reflogs will be deleted by the make_deterministic_repo function. So I'm pretty certain that the initial branch name cannot have any effect on the output-hash (and I tested multiple cases).

That being said the current Git default branch name is still master and I'm not sure if it's already official that the new name will be main (like on GitHub). So we could indeed use master for now.

Anyway, I don't care about master vs. main [0] but I'd like to have a quick vote (not sure if that'll work but I don't have a better idea :o) on this so that we can hopefully reach some rough consensus.

[0]: Git's master branch is fine for me because there is no "slave" branch, etc. So Git's switch to another name seems like a bad/unnecessary idea but I'd like to leave that decision to people that are affected by this (so basically I see myself as neutral on this matter - but regarding this PR I'm for main because of GitHub's and Git's decisions).
[1]: Example (refs/heads/XXX123456789XXX will be deleted in this case during git-checkout):

$ git init --initial-branch=XXX123456789XXX test
Initialized empty Git repository in /tmp/test/.git/
$ cd test
$ git remote add origin https://git.kernel.org/pub/scm/network/wireless/iwd.git
$ git fetch origin
[...]
$ grep -R XXX123456789XXX
.git/HEAD:ref: refs/heads/XXX123456789XXX
$ git checkout -b test 1.9
Switched to a new branch 'test'
$ grep -R XXX123456789XXX
.git/logs/HEAD:0000000000000000000000000000000000000000 aa3dc1b95348dea177e9d8c2c3063b29e20fe2e9 Michael Weiss <dev.primeos@gmail.com> 1613571661 +0100	checkout: moving from XXX123456789XXX to test

@primeos
Copy link
Member Author

primeos commented Feb 17, 2021

Could those in favor of main react to this issue with 👍 and those in favor of master react with 👎?
(Note: This is only for the initial branch name in nix-prefetch-git and shouldn't matter at all apart from the fact that one console output line changes if branchName was set to master or main. So this vote is pretty silly but let's try to decide this once and for all.)
Commenting on this issue is obviously fine as well (and better ideas are welcome).

@SuperSandro2000
Copy link
Member

I thought about that too but given that Git will AFAIK switch to main as new default and using master is now controversial it seemed better to switch to main right away (before anyone complains that we're explicitly using master).

Which will probably be overwritten back to master if it can cause issues. This PR is about hiding a message we don't need to see everytime not changing the defaults of tooling in nixpkgs which could break a lot of things and I just don't want to risk that.

Anyway, I don't care about master vs. main [0] but I'd like to have a quick vote (not sure if that'll work but I don't have a better idea :o) on this so that we can hopefully reach some rough consensus.

Could those in favor of main react to this issue with 👍 and those in favor of master react with 👎?

No, this is not about politics or opinion. This is about if it breaks anything around the nix tooling. If it does break anything than we keep master to not break anything. If it does not matter then it is whatever and the default branch could even be homeless-shelter.

If you are just hiding the message and don't change anything I feel comfortable to merge this without breaking anything or causing regressions. If you change the default branch I don't feel comfortable to merge it.

The reason for this change is simply to avoid the following messages
that are unnecessary and can be confusing (and these messages will be
repeated for each submodule):
hint: Using 'master' as the name for the initial branch. This default branch name
hint: is subject to change. To configure the initial branch name to use in all
hint: of your new repositories, which will suppress this warning, call:
hint:
hint:   git config --global init.defaultBranch <name>
hint:
hint: Names commonly chosen instead of 'master' are 'main', 'trunk' and
hint: 'development'. The just-created branch can be renamed via this command:
hint:
hint:   git branch -m <name>

With this change the behaviour remains unchanged (apart from the
suppressed "warning" in the console output of course) and therefore this
doesn't cause any hashes to change and by default nix-prefetch-git uses
the "fetchgit" branch anyway (branchName can be set to override the
default):
Switched to a new branch 'fetchgit'

For that reason the initial branch name doesn't matter anyway and since
we're not relying on / hardcoding "master" we could simply switch to
"main" (which seems most common nowadays). See [0] for more details on
why this wouldn't break anything.
However, since the initial branch name doesn't matter and to avoid any
additional risks it was "decided" to keep using "master" (s. NixOS#113313).

[0]: NixOS#113313 (comment)
@primeos primeos force-pushed the nix-prefetch-git-avoid-initial-branch-warnings branch from 894b155 to 2aadb9a Compare February 18, 2021 10:46
@primeos
Copy link
Member Author

primeos commented Feb 18, 2021

No, this is not about politics or opinion.

Tell that to Git and GitHub ;)
(BTW: Part of why I pushed for main is also that I don't want to become a target of some bad SJWs, which unfortunately can be a real problem, even though for a Nixpkgs PR with such minor impact it seems super unlikely to get that much attention.)

This is about if it breaks anything around the nix tooling.

But I explained why it doesn't...
Anyway, since there where not votes I'll go with your personal opinion. I consider Nixpkgs a democracy though.

If you change the default branch I don't feel comfortable to merge it.

That is of course fine. (I would've obviously been confident enough to merge it as is since I tested it but this PR was mainly for feedback regarding the master -> main change and since nobody seems to really care about it we can stick to master.)

@primeos primeos merged commit eecade6 into NixOS:master Feb 18, 2021
@primeos primeos changed the title nix-prefetch-git: Run git-init with --initial-branch=main nix-prefetch-git: Run git-init with --initial-branch=master Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants