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 crash in git.detached when ref is a commit ID #34372

Closed
wants to merge 2 commits into from
Closed

Fix crash in git.detached when ref is a commit ID #34372

wants to merge 2 commits into from

Conversation

erikgrinaker
Copy link
Contributor

What does this PR do?

Fix git.detached crash due to missing cwd parameter for git.describe, and subsequent failure due to missing --always parameter in git.describe. I'm not sure if the --always parameter will break any unrelated code.

What issues does this PR fix or reference?

See #34371 for details.

Tests written?

No

@erikgrinaker erikgrinaker changed the title Git.detached Fix crash in git.detached when ref is a commit ID Jun 29, 2016
@cachedout
Copy link
Contributor

@terminalmage This is your code, I believe. Could you please take a look?

@cachedout cachedout added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Jul 5, 2016
@terminalmage
Copy link
Contributor

No, this state was added in January in ca0c8ef, I've never seen it before.

It looks like a reasonable fix, but I need to check the availability of the --always parameter for git describe. We've been bitten by older git (1.7 or thereabouts, the version shipped with Ubuntu 12.04 LTS and some older Linux distro releases) not having CLI arguments, as evidenced by the liberal use of distutils.version.LooseVersion() to ensure that we're using arguments appropriate to the version of git available to the minion, so any time we make changes to the arguments used in the git execution module I try to make sure that we're doing it in a way that won't break the git module on those minions with older git.

@cachedout
Copy link
Contributor

Was referring to the exec mod, which was yours. Should have looked at the state too. My bad.

@terminalmage
Copy link
Contributor

To be honest, I'm not sure why git.detached is even here, git.latest does everything this state does when it is fed a SHA or tag instead of a remote branch.

@terminalmage
Copy link
Contributor

Actually, I read the past commentary on this and I understand now the reasoning-- to keep a revision which we know will be a detached head from having to go through all the additional overhead the git.latest state uses to determine which branch to use, whether the change is a fast-forward, etc.

@terminalmage
Copy link
Contributor

OK, so git describe gained the --always parameter in version 1.5.6. This should be fixed back in the 2015.8 release branch as the same error message would have appeared if this function were invoked there with the same input.

The thing is, git.detached doesn't exist in until the 2016.3 branch. Normally we would just backport a PR when it is submitted to develop and the fix needs to go into an earlier release branch, but the fact that the two commits should each be backported to different branches complicates things. So, I will split the two commits that @erikgrinaker has submitted here into separate pull requests, each against their appropriate branch, and then close this pull request.

@terminalmage
Copy link
Contributor

#34462 contains @erikgrinaker's first commit, made against the 2015.8 release branch, along with an additional commit I added to restrict usage of --always to compatible versions of git.

#34463 contains @erikgrinaker's second commit, made against the 2016.3 release branch.

Thanks @erikgrinaker for taking the time to submit these. And bonus points for making your changes atomic using separate commits, it made it that much easier to get these fixes submitted to the proper branches.

@erikgrinaker
Copy link
Contributor Author

Thanks for the review and merge, @terminalmage!

@terminalmage
Copy link
Contributor

No problem, thanks again for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants