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: fix scm switch when going back to older revision #531

Merged
merged 1 commit into from Sep 5, 2023

Conversation

rhubert
Copy link
Contributor

@rhubert rhubert commented Aug 23, 2023

If gitCommitOnBranch is active and commit and branch are configured in the recipe changing the commit back to a older one did not update the sources back to the configured version. Instead the remained on the current version.

@codecov
Copy link

codecov bot commented Aug 23, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06% 🎉

Comparison is base (d8326d7) 88.24% compared to head (8243c29) 88.31%.

❗ Current head 8243c29 differs from pull request most recent head a08c442. Consider uploading reports for the commit a08c442 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #531      +/-   ##
==========================================
+ Coverage   88.24%   88.31%   +0.06%     
==========================================
  Files          46       46              
  Lines       14501    14494       -7     
==========================================
+ Hits        12797    12800       +3     
+ Misses       1704     1694      -10     
Files Changed Coverage Δ
pym/bob/scm/git.py 94.17% <100.00%> (+1.67%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkloetzke
Copy link
Member

I think the goal should be to remove git merge --ff-only and replace it by git reset --mixed $commit/$tag. The problem is that we must make sure that no commit of the user is lost. Whenever any remote branch holds the current commit (after switching to the branch) then we're safe. I think calling git branch --remotes --contains HEAD (before git reset) and verify that it produced some output before should be sufficient...

@rhubert
Copy link
Contributor Author

rhubert commented Sep 4, 2023

I think git reset --mixed is wrong as it would leave all changes uncommitted in the workspace.

If there are 3 commits:

- A - B - C  (HEAD)

git reset --mixed B would leave Cs-changes in the workspace (uncommitted).

I think we need to distinguish between moving forward and backwards in history.
Moving forwards git reset --mixed or keep git merge --ff-only should work.

Going backwards use git reset --hard if git status --porcelain returns nothing and C is on any other local or a remote branch. Otherwise move to attic.

@jkloetzke
Copy link
Member

Right, my bad. Obviously I was not reading the manpage carefully enough. 🙈 But git reset --hard does feel too dangerous. What about git reset --keep? It moves the branch forward and backwards while aborting the operation if a file has changes that needs to be updated. In this case Bob will move the repository into the attic as expected. OTOH unrelated files do not prohibit updates. Sounds safe to me.

But as already said, we have to make sure that the old commit is not lost. Maybe git branch --remotes --contains HEAD is a bit too pessimistic. The user might have another local branch. My current idea is to run git branch -a --format='%(refname:lstrip=2)' --contains HEAD and remove the current branch name from the list. Thoughts?

@rhubert rhubert force-pushed the rh-fix-git-switch branch 2 times, most recently from 31ed740 to 787722d Compare September 5, 2023 12:23
If gitCommitOnBranch is active and commit and branch are configured in
the recipe changing the commit back to a older one did not update the
sources back to the configured version. Instead they remained on the
current version.

To fix this use `git reset --keep` instead of `git merge --ff-only` when
updating, but make sure we do not lose any commits.
@jkloetzke jkloetzke merged commit a160bcc into BobBuildTool:master Sep 5, 2023
9 checks passed
@jkloetzke
Copy link
Member

Thanks for bearing with me. :)

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