Show sync status in branches list

[stefanhaller]

• PR Description

When pulling/pushing/fast-forwarding a branch, show this state in the branches list for that branch for as long as the operation takes, to make it easier to see when it's done (without having to stare at the status bar in the lower left).

This will hopefully help with making these operations feel more predictable, now that we no longer show a loader panel for them.

[jesseduffield]

Looking good, couple thoughts:

• it's cool to be able to queue up multiple fast-forwards and see the labels against each branch (thankfully the sync-mutex ensures nothing goes wrong) showing the progress
• it's cool seeing the label against the branch that the action affects. So I can select the second branch in the list, press 'p' for pull, and see the 'pulling' status is attached to the first branch in the list. That's really good feedback to the user who may have hoped that pulling could affect the selected branch rather than the current branch (compared to say fast-forwarding)
• If I start a pull, the 'pulling' status appears against the branch but if I then press shift+R to refresh from within the files view that status disappears even though I can see that in the bottom left we're still pulling.
• the lack of a loaders spinner is jarring. Is there a technical reason why we can't add one?
• This is a pattern which would be useful in other places e.g. pushing a tag or deleting a remote tag, or any action that we expect to take some time. As such, I'd like to have a more generic way of handling this e.g. having our list context trait hold a field that's a mapping from list item id to status.
• We'll also want to decide on a rule for when we should and shouldn't show an inline status as part of some action. A rule like 'only do it when we're talking to a remote server' may be fine, but we may want to expand to 'only do it when the action could take a while on large repos' which would include things like rebase operations.

[stefanhaller]

it's cool to be able to queue up multiple fast-forwards and see the labels against each branch (thankfully the sync-mutex ensures nothing goes wrong) showing the progress

Yes, that's cool; also for pushing a stack of branches. It's possible to check out the first branch, push it, and while it's being pushed check out the next branch and push it too.

Actually I'd be interested in trying whether the sync mutex is really needed. I wouldn't be surprised if git is good enough at handling concurrent operations correctly. The only thing I didn't dare trying yet is to check out a different branch while pulling a branch, but the sync mutex wouldn't help with that.

If I start a pull, the 'pulling' status appears against the branch but if I then press shift+R to refresh from within the files view that status disappears even though I can see that in the bottom left we're still pulling.

I'll have to debug this; it works correctly for pushing, so I don't understand why it doesn't for pulling.

the lack of a loaders spinner is jarring. Is there a technical reason why we can't add one?

I couldn't find an easy way to redraw just a single line. Do you think it's ok to redraw the entire view (PostRefreshUpdate) every 50ms?

[stefanhaller]

Actually I'd be interested in trying whether the sync mutex is really needed.

I played with this for a while, and I'm now convinced that not only can we get rid of the sync mutex, we actually should because it makes some things work better:

• pushing a stack of branches by checking out the first branch, pushing it, checking out the next one, pushing it too, and so on works better without the mutex. I did some testing on this, and simulated very slow pushes by putting a sleep 15 into a pre-push hook. This allows you to easily start more than two pushes in parallel. With the mutex, all these are queued up, and since all of these push the current branch at the time they start, they all end up pushing the last branch that was checked out; meaning that the ones in the middle of the stack aren't pushed. Without the mutex, each push pushes the right branch, and it's no problem for them all to run in parallel. Pretty cool. For fetching I'm even more convinced that git is good at locking things as needed, since it can always happen that some other GUI tool has a periodic background fetch running.
• we had complaints about switching repos or switching to submodules being slow right after startup of lazygit. That was because we do an initial fetch after startup (which can take a long time if you didn't have this repo open for a while), and the sync mutex prevents switching repos. And for no good reason, as far as I can see; I can't think of a reason why we shouldn't let an ongoing push, pull, or fetch run to completion in parallel to opening another repo.
• it could help with Lazygit crashes when pushing or pulling #2999, where a background fetch seems to hang forever, so issuing a new push or pull blocks forever. Granted, we should probably still figure out the root cause, but not having a sync mutex doesn't make this an issue any more.

I pushed a commit to remove the sync mutex.

If I start a pull, the 'pulling' status appears against the branch but if I then press shift+R to refresh from within the files view that status disappears even though I can see that in the bottom left we're still pulling.

I'll have to debug this; it works correctly for pushing, so I don't understand why it doesn't for pulling.

OK, that was a stupid bug. I reset the branchOperation entry too early by doing it outside of the WithWaitingStatus; this is fixed in 83a216f.

e.g. having our list context trait hold a field that's a mapping from list item id to status.

This sounds good except that sometimes I think it could be useful to look at this state for purposes other than drawing the list. For example, I think it would be a good idea to prevent the user from checking out a different branch while the current branch is being pulled; for this to be possible, we must keep the state somewhere more global than the list context.

[stefanhaller]

Oh, one more thing: when doing a manual refresh while the current branch is being pushed or pulled, you can see that the status panel updates too, because it uses FormatStatus to draw the checkmark or pushable/pullable state. I'm not sure if we want this, but actually I think it's kind of nice to see it there, too. If we do, we need to find a way to redraw the status panel at the beginning of the operation, and that needs moving some code around because it is drawn in RefreshHelper.refreshStatus which can't be called from the outside.

[jesseduffield]

I couldn't find an easy way to redraw just a single line. Do you think it's ok to redraw the entire view (PostRefreshUpdate) every 50ms?

I think it's worth trying to see how well it's handled. We show loader spinners in lazydocker against individual lines and it works fine. Two issues that come to mind:

• I don't think we update just the viewport; we update the whole view, so if a user has a couple hundred branches, it'll be slower to refresh.
• If there are expensive, non-idempotent callbacks happening in PostRefreshUpdate (e.g. rendering the current item to the main view) then we should introduce a separate method specifically for re-rendering the view. I don't acually recall if they are currently idempotent.

I played with this for a while, and I'm now convinced that not only can we get rid of the sync mutex, we actually should because it makes some things work better

Hmm. You make some very good points. What I don't like is that if there is a conflict the second command will just error e.g. background git fetch starts, then user does a git pull, but git fetch has the lock, so git pull errors.

According to this stack overflow response it seems that that's what will happen. I haven't been able to reproduce that specifically but I can get errors when hitting 'p' multiple times where it says I can't rebase onto multiple branches.

The mutex was originally added for #866. I can't reproduce a panic if I press 'p' repeatedly right as lazygit starts so maybe this is fine.

Happy to remove the mutex and see if anybody has issues with it. But if people do have issues with it I think it's more important to prevent those concurrency issues than to support concurrent commands, meaning I'd want to immediately reintroduce the mutex and then look into more sophisticated concurrency support.

This sounds good except that sometimes I think it could be useful to look at this state for purposes other than drawing the list.

I see: we want the status to be against the model, not the view (or context). That makes sense. I'm happy for it to live in the gui struct then but:

• I want a struct for holding these statuses (e.g. with a Branches field and a Tags field) so that it's centralised rather than tagging on more fields to the gui struct
• Currently it is just storing a presentational status string against the model item. If we ever want to read that we should switch to using an enum which then gets mapped to the status string. Given that we don't currently have the use case for reading the string I'm happy to leave it as a string until we do.

Oh, one more thing: when doing a manual refresh while the current branch is being pushed or pulled, you can see that the status panel updates too

I'm not a fan of that: updating three different places on the screen to show that we're pulling is overkill imo. I'd just pull the logic out of FormatStatus and have it specific to the branches view.

[stefanhaller]

What I don't like is that if there is a conflict the second command will just error e.g. background git fetch starts, then user does a git pull, but git fetch has the lock, so git pull errors.

It's true that some scenarios can cause errors like this, but I think a mutex is not the right tool to solve them. The only reason they can happen is that two concurrent operations try to update the same ref; this can only happen if you try to pull the same branch twice, or maybe push the same branch twice (because it needs to update the remote ref). We should simply prevent users from doing that, as there's no reason to do it. And we can use the new DisabledReason to prevent this. (This will not solve the case where the user pushes a branch from another git client, then quickly switches to lazygit and tries to push it from there, too. But I don't think we need to solve this, and the mutex wouldn't have helped with this either.)

The scenario you mentioned is not one of these (user does a pull while a background fetch is running): the background fetch only updates remote refs, the pull only updates a local ref. No conflict here.

I want a struct for holding these statuses (e.g. with a Branches field and a Tags field) so that it's centralised rather than tagging on more fields to the gui struct

I wonder if we could keep using a single map if we switch to storing the full refs as keys, as they are guaranteed to be distinct for branches as tags. Seems easier to me, what do you think?

we should switch to using an enum which then gets mapped to the status string.

I thought about that too, and it sounds easy enough, so I can do it as part of this PR.

[jesseduffield]

The scenario you mentioned is not one of these (user does a pull while a background fetch is running): the background fetch only updates remote refs, the pull only updates a local ref. No conflict here.

But a pull internally does a fetch + merge/rebase so I do think there's still a chance of conflict. At any rate I'm still happy to see how we go without the mutex.

I wonder if we could keep using a single map if we switch to storing the full refs as keys, as they are guaranteed to be distinct for branches as tags. Seems easier to me, what do you think?

I had in mind having support for a status against any model item but come to think of it it's really tags/branches that involve talking to the remote so I'm happy to go with that for now

[stefanhaller]

But a pull internally does a fetch + merge/rebase so I do think there's still a chance of conflict.

I don't think two fetches in parallel are a problem for git. (Maybe it takes some reading of the git source code to confirm that.) If it was, then it would be a problem to have several git clients running which all do periodic background fetches.

[jesseduffield]

It's quite possible that those conflicting fetches just silently fail and then try again later though, which is different to having a user-initiated pull fail. Or do you mean that we should expect more pulls to fail due to having background fetches from other applications running concurrently? That's a good point.

[stefanhaller]

No, I don't expect a pull to fail when there's a concurrent fetch running. Only two pulls can conflict (because they will try to update the same local ref, or rebase the local branch onto different things), but two fetches won't, and a pull and a fetch won't either. It still needs to be confirmed, but I would be very surprised if that doesn't hold.

[jesseduffield]

Makes sense

[stefanhaller]

New version:

• uses a more generic "ref operation" concept (can be used for branches and tags)
• uses an enum, and translates to strings at the presentation level
• draws spinning loader animations
• uses inline status displays for pushing tags and deleting remote tags

Also, I now disable a few commands in certain situations (e.g. pushing or pulling a branch while the same branch is already being pushed or pulled), so that we can be a bit more confident about removing the sync mutex. We can maybe think about adding more of these. (However, I don't want to disable checkout another branch while the current branch is being pushed; this works reliably, and is useful.)

There's one cosmetic problem with disabling these commands: I use the DisabledReason mechanism for this, which is a problem when you bring up the keybindings dialog, because it doesn't update when the state changes (i.e. when a branch is done pushing). Not sure how problematic we find this.

[stefanhaller]

Another cosmetic problem: when force-pushing a branch, the inline status replaces the ↑4↓9 while pushing, but when done, the ↑4↓9 briefly appears again and is then replaced with the green checkmark a moment later. I find this very ugly. The reason for this is the PostRefreshUpdate call for the branches context in refreshWorktrees; commenting this out fixes the glitch.

I'm not sure what to do about this, to be honest.

[stefanhaller]

I pushed a commit (830c48f) to fix the visual glitch with the ↑4↓9 vs. ✓. I hope that's an appropriate solution; as I don't use worktrees myself, I couldn't really test it much.

[jesseduffield]

Looking good.

I had a go refactoring this to support any arbitrary item (not just refs) and I like the result. What do you think? b726fd4

For the record I see this coming in handy when we improve the recent repos view to be a fully fledged list view that allows fetch/push/pull operations.

[jesseduffield]

thoughts on moving this stuff to a helper?

[stefanhaller]

Ha, I thought I could get away with just putting it into a source file that has "helper" in its name. 😄

[jesseduffield]

Alas, I still wanna shrink down the gui struct so that it's not so much of a 'God Struct', and the helpers in the helpers package are one way to do that, so I reckon we extract a helper out for this

[stefanhaller]

OK, I can give it a try. Just to be clear, I'd move everything that's now in StateAccessor to the helper, including the state, right?

[jesseduffield]

I was thinking moreso that you'd leave the state available in StateAccessor but move the functions specific to item operations to the helper

[stefanhaller]

Here's what I came up with: 206a4e0. Do let me know if you want something to be done differently, I'm not tired of the review yet. 😄

Also, normally I'd make an effort to squash this down somehow into earlier commits (also your itemOperations refactor) to avoid this back-and-forth in the history, but it's too much work this time, so I'd rather leave it as is.

[jesseduffield]

shouldn't this be done from within stopRenderingInlineStatus?

[stefanhaller]

No, this would come too early and cause the ugly flicker again (not for this command, but for the other ones). Most other commands need to refresh first and only redraw again at the end.

[jesseduffield]

TL;DR makes sense, happy to leave as-is

I see: what differentiates this from other situations is that in other cases we'll do a refresh at the end of the callback passed to WithItemOperation because we expect the model to change as a result of the action. With tags, we don't keep track of which remotes a tag has been pushed to, so there's no need to refresh. But in this case we can't piggyback on the fact that a refresh causes a re-render because we're not refreshing. That's why we need to do an explicit render.

This actually shines a light on the fact that we could be storing remote tags but we just aren't. Once we support that, we'll add a refresh here and then we won't need the call to HandleRender.

I was wondering whether we should re-render anyway within WithItemOperation but it's an extra render we don't need to do so I'm happy to leave this as is.

[jesseduffield]

We always defer a clear right after setting the ref and we always pass the same arguments to it. We can DRY this up: I'm thinking we have a function like WithWaitingStatus which takes a function as an argument and does the set/clear around calling that function.

[stefanhaller]

Good idea, see a0510f5.

[jesseduffield]

Would be good to add a comment here explaining when it's appropriate to set a ref operation. At the moment the rule is to use a ref operation when we need to talk to the remote

[stefanhaller]

5b64b2b

[stefanhaller]

I like the generalization from ref to item and cherry-picked it as is.

[jesseduffield]

Just two more things :)

[jesseduffield]

Can we add WithItemOperation to pkg/gui/gui_common.go? Feels like it should be just as accessible as WithWaitingStatus

[stefanhaller]

You mean like this? ca6fefd

I'm still a bit unsure sometimes with all the interfaces that we have. WithWaitingStatus is part of IPopupHandler (not really sure why to be honest), for WithItemOperation I couldn't think of anything better than IGuiCommon.

[jesseduffield]

yep!

[jesseduffield]

TL;DR makes sense, happy to leave as-is

I see: what differentiates this from other situations is that in other cases we'll do a refresh at the end of the callback passed to WithItemOperation because we expect the model to change as a result of the action. With tags, we don't keep track of which remotes a tag has been pushed to, so there's no need to refresh. But in this case we can't piggyback on the fact that a refresh causes a re-render because we're not refreshing. That's why we need to do an explicit render.

This actually shines a light on the fact that we could be storing remote tags but we just aren't. Once we support that, we'll add a refresh here and then we won't need the call to HandleRender.

I was wondering whether we should re-render anyway within WithItemOperation but it's an extra render we don't need to do so I'm happy to leave this as is.

[jesseduffield]

Alas, I still wanna shrink down the gui struct so that it's not so much of a 'God Struct', and the helpers in the helpers package are one way to do that, so I reckon we extract a helper out for this

[stefanhaller]

I added one more fixup (a6eb1e1) to rename a few more things from "refOperation" to "itemOperation".

[jesseduffield]

LGTM, great work this is a really good addition

[jesseduffield]

Actually something to consider: what are your thoughts on not showing the bottom-left loader when showing a loader in a view? Showing both at the same time is a little noisy and we don't gain anything extra from showing the loader in the bottom left

[stefanhaller]

Actually something to consider: what are your thoughts on not showing the bottom-left loader when showing a loader in a view? Showing both at the same time is a little noisy and we don't gain anything extra from showing the loader in the bottom left

Yeah good point, I'll try that and see how it feels. It definitely makes sense for the operations that you initiate from the item itself (e.g. fast-forward), because you probably have your eyes there when you do it anyway. I'm less sure for commands like push and pull which I often invoke from the files view or commits view, and in those cases I might look at the status bar more than at the branches list. But it's worth trying.

However, this made me realize another problem: the problem with push_with_credential_prompt.go hanging forever when running it slowly is back (argh), because we need to pause the new inline loader in the same way as we paused the status bar loader. Give me a bit of time to fix that...

[stefanhaller]

I reworked the branch heavily. I now have a clearer separation between the state, which only stores the item operations and knows nothing about where they are shown, and the helper which orchestrates the redrawing. I renamed it to WithInlineStatus, it is more or less a drop-in replacement for WithWaitingStatus now, it behaves basically the same.

I am a little bit embarrassed that it took me so long to arrive at this design (with your help), but I'm quite happy with the result. I also restructured the commit history a bit to try to better reflect what's happening.

It does need a thorough re-review though; I changed so many things that I may easily have made stupid mistakes.

[jesseduffield]

So far helpers have been stateless: letting the Gui or GuiRepoState struct hold any required state. I'm torn about whether we should retain that pattern here or just store the state on the helper. Having it on the helper is good for encapsulation but makes it harder to see at a glance the state of the app.

I'm happy to have it on the helper and see how that goes

[stefanhaller]

Hm, I'm torn too. We had this discussion elsewhere (can't find it right now) to maybe convert all helpers to freestanding functions, in order to make interdependencies between helpers more convenient. This would be the only reason for me to keep the state out of here. Unless we accept that some helpers stay objects.

But I really like the encapsulation here; the alternative would be to expose the contextsWithInlineStatus map publicly in GuiCommon, but it really isn't anybody else's business to mess with that state.

[stefanhaller]

@jesseduffield I was holding off of merging this one to see if there's any more input on this question. I'd like to merge now though, because it conflicts with #3058.

[jesseduffield]

Go ahead: happy to see how this approach goes

[jesseduffield]

LGTM, great work!

[peppy]

I've been tracking/running off this branch for a couple of weeks now and it's been great for me 👍 .

[peppy]

One unfortunate edge case is when there's no space to show this. Currently in this situation there's zero UI feedback on operations:

CleanShot.2023-10-13.at.04.50.41.mp4

In case it isn't obvious, I'm pushing multiple times in this video 😅.

I think this could be resolved by temporarily showing the "Pushing" progress text on top of the branch name (with a few characters of left padding).

[stefanhaller]

Good point. The pushing status is not the only thing affected by this; I also want to see the ↑4↓9 or the "upstream gone". The solution for all of these is to shorten the branch name enough (maybe by appending ...) to make the status string fit after it, whatever it is. I'll see how hard it is to do that. (We'll have to pass the available width into getDisplayStrings somehow.)

[stefanhaller]

This is a little trickier than I thought, because we need to rerender the view whenever the layout changes. For the case of changing the screen mode this is easy to do by putting an AfterLayout around the code in rerenderViewsWithScreenModeDependentContent; but for resizing the terminal window I don't have a good idea how to achieve it, and I'm not sure there are any other reasons why view sizes could change.

@jesseduffield Any advice here? I'm thinking we could keep track of whether any view widths have changed in layout, and rerender them if so. Once we have that, we could get rid of rerenderViewsWithScreenModeDependentContent altogether.

[stefanhaller]

@jesseduffield I tried the approach of rerendering views when their width has changed, and it looks very promising so far. Hope to have a PR tomorrow. Just saying so that you don't waste time thinking about solutions. 😄

[stefanhaller]

@peppy See #3075, if you feel like testing it.