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

git: Don't reset last active branch to wrong commit #5919

Merged

Conversation

p12tic
Copy link
Member

@p12tic p12tic commented Mar 18, 2021

When we change the checked out code using git reset --hard <rev> we also change the revision that the currently active branch points to. This leads to that branch pointing to a wrong commit as we're actually checking out a different branch (we switch to it just a little bit later). Instead of modifying the current branch we should just modify the working copy, which can be done by git checkout -f <rev>.

We need branches to point to correct commits in order for GitDiffInfo step to work.

git checkout -f documentation:

When switching branches, proceed even if the index or the working tree differs from HEAD. This is used to throw away local changes. When checking out paths from the index, do not fail upon unmerged entries; instead, unmerged entries are ignored.

I've tested how git checkout -f works just to be sure and it's equivalent to git reset --hard except that active branch is not affected. I've also tested a real Buildbot installation with the change.

@p12tic
Copy link
Member Author

p12tic commented Mar 18, 2021

cc @tardyp Could you take a look? This is one of the more risky changes that may have a significant impact if we get it wrong.

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #5919 (bd37daa) into master (03ed138) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5919   +/-   ##
=======================================
  Coverage   91.68%   91.68%           
=======================================
  Files         339      339           
  Lines       36260    36260           
=======================================
  Hits        33246    33246           
  Misses       3014     3014           
Impacted Files Coverage Δ
master/buildbot/steps/source/git.py 96.63% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03ed138...bd37daa. Read the comment docs.

When we change the checked out code using `git reset --hard <rev>` we
also change the revision that the currently active branch points to.
This leads to that branch pointing to a wrong commit as we're actually
checking out a different branch (we switch to it just a little bit
later). Instead of modifying the current branch we should just modify
the working copy, which can be done by `git checkout -f <rev>`.
@p12tic p12tic force-pushed the git-dont-reset-branch-to-wrong-commit branch from 44e14d6 to bd37daa Compare March 19, 2021 11:02
@tardyp
Copy link
Member

tardyp commented Mar 19, 2021

good finding!

@tardyp tardyp merged commit de1a41a into buildbot:master Mar 19, 2021
@tardyp
Copy link
Member

tardyp commented Mar 19, 2021

merging, to ease review of #5929

@p12tic p12tic deleted the git-dont-reset-branch-to-wrong-commit branch March 19, 2021 12:37
leoetlino added a commit to dolphin-emu/sadm that referenced this pull request Oct 3, 2021
Fixes release builds showing HEAD as the branch name now that
[Buildbot doesn't `git reset --hard` the active branch anymore](buildbot/buildbot#5919).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants