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

Prevent tag refs in $(GitBranch) for detached heads. #127

Merged
merged 1 commit into from
Sep 24, 2020
Merged

Prevent tag refs in $(GitBranch) for detached heads. #127

merged 1 commit into from
Sep 24, 2020

Conversation

andersforsgren
Copy link
Contributor

@andersforsgren andersforsgren commented Sep 23, 2020

Changes the behavior of finding a value for $(GitBranch) when in detached head, common on CI servers. Fixes #126

@kzu kzu merged commit 64fd2e4 into devlooped:master Sep 24, 2020
@kzu
Copy link
Member

kzu commented Sep 24, 2020

Awesome improvement and nice docs too! Thanks 💯

@andersforsgren
Copy link
Contributor Author

Heads up that this might need a tweak: It actually regressed to a worse state on our build servers with this version despite a fair bit of local testing. Turns out that if there are no refs (including no tags) at all in the working copy, the name-rev would return "remotes/origin/master~N". With the new default filter, remote branch names don't match the filter so it becomes "undefined" instead of "master", which is clearly worse.

The correct thing to do is obviously to always pass in /p:GitBranch=whatever to the msbuild command when building for CI. But the question is what the correct thing to do is when it's not done.

  1. The old behavior (try to grab whatever name, including a tag). As I said I don't quite understand how it some times dug up the correct name from a detached head! (basically revert this)

  2. Use the commit hash as the tag name (this is really most correct for a detached head - there is no "branch" after all!)

  3. Fail, instructing the user that when building a detached head, GitBranch must be passed in. This is effectively a
    `

@kzu
Copy link
Member

kzu commented Sep 29, 2020

Hm, I think using the hardcoded detached might be best in this case. Maybe the short sha too, but I'm not so sure.

@andersforsgren
Copy link
Contributor Author

"detached-30ac81b" would make sense

@kzu
Copy link
Member

kzu commented Sep 30, 2020

Sounds good! Could you send a PR?

Thanks!

@andersforsgren
Copy link
Contributor Author

Will do

@andersforsgren
Copy link
Contributor Author

I looked at git name-rev docs and it has an argument "--always" that makes it return the shorthash in case it can't find another refname. I suggest just using that for simplicity, even though it means one gets just "shorthash" instead of "detached-shorthash".

The alternative would involve detecting the nonzero error code or "undefined" name, and then doing another exec git rev-parse --short HEAD to get the hash. That seems a bit overkill for the purpose, so I'll do the simplest one.

PR to add "--always" to the default args incoming.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possibility to exclude tag refs from $(GitBranch)
2 participants