search

Murmele / Gittyup

Understand your Git history!
https://murmele.github.io/Gittyup
MIT License
1.37k stars 107 forks source link

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

Closed siforrer closed 2 months ago

siforrer commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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.