Skip to content

Allow user to initiate open pull request prior to push completing #4339

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

Open
ChrisMcD1 opened this issue Feb 28, 2025 · 6 comments
Open

Allow user to initiate open pull request prior to push completing #4339

ChrisMcD1 opened this issue Feb 28, 2025 · 6 comments
Labels
enhancement New feature or request

Comments

@ChrisMcD1
Copy link
Contributor

ChrisMcD1 commented Feb 28, 2025

Is your feature request related to a problem? Please describe.

I often find myself pushing a branch and then immediately making a PR. I hit P and then o, but then receive the warning that there is no upstream branch. This is factually correct, but one will exist in less than one second.

Describe the solution you'd like
I would like for lazygit to realize that there is a branch currently being pushed, and to wait for that to resolve prior to actually attempting to open the PR.

For my simple case, this could probably just be solved with a simple mutex or RWLock that the sync_controller sets, and the branch_controller waits to be available right when a user attempts to open a pull request. A more complicated system could be created that tracks which branches are currently being pushed, but that feels unnecessary. I don't feel like many people are pushing branch A, and then going and opening a pull request on branch B while that happens.

EDIT: ^ Actually, I think a map of locks keyed on local branch name would make a lot more sense. With stacked branches, I often actually push multiple branches at once, and then would like to be able to open pull requests on them without waiting.

Describe alternatives you've considered
The only alternative I can think of that doesn't involve coding something would be to just wait for visual confirmation that the operation has finished.

Additional context
I don't experience long push times in my daily flows, either from pre-push hooks or slow upload speeds, so I'm not sure how such a feature would impact those users.

@stefanhaller
Copy link
Collaborator

Describe the solution you'd like I would like for lazygit to realize that there is a branch currently being pushed, and to wait for that to resolve prior to actually attempting to open the PR.

While this would be convenient, it has never bothered me enough to want to do something about it. It's easy to see in the branches panel that the branch is still being pushed, so I just wait for the "Pushing..." spinner to go away and then press o. I feel this is not unreasonable.

For my simple case, this could probably just be solved with a simple mutex or RWLock that the sync_controller sets, and the branch_controller waits to be available right when a user attempts to open a pull request.

No, this is totally not an option. We shouldn't block in controllers, this would be a very bad user experience if the branch takes long to push.

We do sometimes block after performing a command, e.g. if we need to make sure that the selection is in sync again when the command has finished; for example after moving a commit up or down with ctrl-j/k. But we never block before even starting a command.

So if we wanted to do this properly, we'd have to enqueue the request somehow, and return to the main loop. Then execute the request later when the branch is done pushing. Which raises the question whether we need to somehow visualize that the request is enqueued; do we need to provide a way to cancel it if you change your mind, etc. In my opinion it's not worth the complexity.

Additional context I don't experience long push times in my daily flows, either from pre-push hooks or slow upload speeds, so I'm not sure how such a feature would impact those users.

In my work repo we have quite expensive pre-push hooks that examine commits one by one. Pushing a very long branch can take minutes there.

@ChrisMcD1
Copy link
Contributor Author

Perhaps my problem is that I have just gotten faster with lazygit. In my memory, there is some version of this app where I could seamlessly press PO to push and open a pull request. And the 2nd command would happen right after the first finished. These days I find myself doing the same muscle memory, but I get hit with the error message that there is no upstream. I was so confident this was a feature I just spent 10 mins building old releases thinking that maybe the push operation used to happen on the UI thread, and therefore the open pull request would naturally sequence after it, but that was not the case!

I don't think it's unreasonable to keep track of your branch status and wait to hit o, but it doesn't feel fluid. In my head what I logically want is a single operation "Get this branch into a PR". Perhaps this could even be implemented as a new command...

So if we wanted to do this properly, we'd have to enqueue the request somehow, and return to the main loop. Then execute the request later when the branch is done pushing.

Got a PR up that implements that idea using a map of waitgroups that keeps track of which local branches are being pushed. The enqueueing of the o request is implemented by sending it to a worker thread, which can then block until that waitgroup has finished! Care to take it for a spin?

#4340

It has no idea of cancellation or status updates, although that would be an interesting idea.

@ChrisMcD1
Copy link
Contributor Author

@stefanhaller This idea has been stuck in my head, so I went ahead and implemented a version with both cancellation and status display! Displays pending tasks in the status bar, and uses the global Escape action to cancel those tasks while they are pending.

Check it out at https://github.com/ChrisMcD1/lazygit/tree/full-background-task-capability!

Don't have as a PR yet since it requires additions to the gocui fork, and I'd rather get some more feedback on the idea before making that PR.

@stefanhaller
Copy link
Collaborator

I'll have a closer look as soon as I find enough time, which might only be next weekend.

Meanwhile, some early thoughts from a very cursory glance at the code:

  • looks like you are implementing cancellation manually. It would be better to use the standard go way of doing that, which is using a context.
  • it would be good to structure the commits a bit better, to make the branch easier to review. Break out independent changes into their own commits. For example, the change to allow using Then with an async refresh could be a commit of its own.
  • it's fine to create a (draft) PR even if it has changes to gocui. What I usually do is make the changes to gocui in the vendor directory in one or more commits at the beginning of the branch (prefixing their subjects with [gocui]). This will make CI fail, but that doesn't matter for now. Then, when the PR is reviewed and approved, I take the gocui changes, make a PR for them, merge it (no need to re-review them over in the gocui repo), and replace the [gocui] commits with a single "Bump gocui" commit. I have used that workflow lots of times in the past, and it has worked well.

@jesseduffield
Copy link
Owner

For the record I also find it annoying to wait for the branch to be pushed before opening, so a solution would be welcome, assuming it's not too complex (afraid I don't have the time right now to look at your branch @ChrisMcD1 ).

If it is too complex, another solution is to simply have a new keybinding for push-and-open, perhaps in the menu that you open with shift+o

@ChrisMcD1
Copy link
Contributor Author

Split into more atomic commits, and draft PR created #4370.

I am using a context.Context internally in this new implementation, but I had to do in what I think is a slightly nonstandard way, since we need the CancelFunc available inside the global state of the application so that the QuitAction can cancel it. So rather than trying to have the parent Context deal with the cancelling, I allowed any holder of a PendingTask to access it's child context, and cancel it. The call site just has no way to effectively communicate the context it created with the world. I left a context as a first arg based on the convention that everything interacting with Context should allow its upstreams to provide a context, in case they want to timeout or something, but the one current consumer just uses the basic context.Background().

Also, high level question about the gocui fork... Is this meant to be a full fork alternative that external projects rely on, or is just designed for the jesseduffield universe to use? Wondering how consideration for breaking changes would work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants