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

Set head/stash reference after pop/drop stash #713

Merged
merged 5 commits into from
Apr 20, 2024

Conversation

siforrer
Copy link
Contributor

Fixes the issue "Dropping multiple stashs in a row segfaults #637". The cause of the crash was that the commit list was not properly updated after pop/drop stash actions. With this change the commit list is updated or when the stash gets empty (after last pop/drop) then the head is selected as current reference.

@jensenr30
Copy link
Contributor

Tested. I can confirm this works. Nice bug fix!

One suggestion: If I do Stash->Pop Stash with 2 or more stashes, my view is set to the stash list. Instead, wouldn't it make more sense to always set the view to show Uncommitted Changes after a stash pop? I'm not sure if there is a clean way to do this currently, so maybe this would be a future enhancement to not hold up this PR.

@siforrer
Copy link
Contributor Author

Thanks for testing and the input regarding Stash->Pop Stash. I totally agree with you and did that change. I put the setReference in the refresh function.

An additional benefit of this is that fixes the following problem which I just noticed:
Initial state - stash selected and commit list contains stashes:
image
Press Refresh
=> The head revision ist selected but the commit list contains stashes:
image

@jensenr30
Copy link
Contributor

In your last commit #4cb4410, when I refresh, it automatically selects the top item in the CommitList. In my opinion, this is disorienting. When a certain commit selected, I like that commit to stay selected unless the user specifically moves to another one by clicking or using arrow keys.

I think your mCommits->setReference(mRepo.head()); is excellent in the case where the selected commit no longer exists (e.g. a popped stash, or any commit that was deleted/rebased). But doing upon every refresh event seems a bit heavy-handed.

Thoughts?

@siforrer
Copy link
Contributor Author

I agree that a refresh should only have an effect if there was actually a change (e.g. in the background by a command line action). Thus the last commit is no improvement. The simple solution would be that I put mCommits->setReference(mRepo.head()); in the popStash function. Though I will think a bit more about modifying the void RepoView::refresh(bool restoreSelection) function because somehow it feels right to handle it there.

@siforrer
Copy link
Contributor Author

siforrer commented Mar 23, 2024

I tried to modify the refresh method but I am not able to do that. Thus I reverted the changes and tried it as before in the popStashmethod directly.

Tested. I can confirm this works. Nice bug fix!

One suggestion: If I do Stash->Pop Stash with 2 or more stashes, my view is set to the stash list. Instead, wouldn't it make more sense to always set the view to show Uncommitted Changes after a stash pop? I'm not sure if there is a clean way to do this currently, so maybe this would be a future enhancement to not hold up this PR.

I was able to change it that the the view shows Uncommitted Changes after a stash pop.

siforrer and others added 5 commits April 20, 2024 16:24
Fixes the issue  "Dropping multiple stashs in a row segfaults Murmele#637".
The cause of the crash was that the commit list was not properly
updated after pop/drop stash actions.  With this change the commit
list is updated or when the stash gets empty (after last pop/drop) 
then the head is selected as current reference.
When executing "Stash->Pop Stash" from the menu bar or when popping a
specific stash from the commit list then the repo view is set to the head
revision and the commit list is updated with the content from the head
revision. 

Before the when doing a "Refresh" and being on the stash view the commit list 
stayed on the stashes but the head revision was selected. Now the commit list 
contains the commits from the head revision.
@Murmele Murmele force-pushed the fix-drop-pop-stash-crashes branch from 51d9a57 to 744dc13 Compare April 20, 2024 14:24
@Murmele Murmele merged commit 9c20e60 into Murmele:master Apr 20, 2024
11 checks passed
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.

3 participants