Skip to content

Conversation

@jesseduffield
Copy link
Owner

@jesseduffield jesseduffield commented Jul 15, 2023

  • PR Description

If the user has gh installed and it's on version 2 or greater, we'll refresh pull requests at the same time we run a background git fetch.

There are a lot of touch points here:

  • gh is written in Go and we could avoid an external dependency by using the gh code directly, however doing so would mean taking on the burden of handling things like authentication, which would increase the scope quite a bit. Given that pulling pull requests takes a while (3.5s for me), the extra cost of shelling out to gh is negligible in comparison
  • There's no way to say 'only fetch the PRs for specific branches' so we instead pull the latest 500 PRs (looking at the code this may be limited to 100 anyway). This unfortunately takes some time. Filtering to only open PRs makes no difference.
  • We show PRs by default, but it can be opted out of
  • If the user does not have gh installed we won't attempt to fetch anything
  • If the user has gh installed but on an old version (<2) we log a message to the user and don't attempt to retry
  • If there is an authentication failure we log and then retry later on
  • We refresh PRs at the same rate as we do a git fetch
  • If the user hasn't configured the base repo of the current git repo, we prompt them to do so.
  • Currently we determine that we're in a git repo by looking at the url of the first remote, to see whether it contains github.com. Gh itself looks at the configured hosts file to see if there is an authentication setup for any host. We should do the same.
  • Please check if the PR fulfills these requirements
  • Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • Docs (specifically docs/Config.md) have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@jesseduffield jesseduffield mentioned this pull request Jul 15, 2023
6 tasks
@jesseduffield jesseduffield changed the title Add github support Show pull requests against branches Jul 15, 2023
@stefanhaller
Copy link
Collaborator

Oooh, very nice!

A few thoughts after trying it (very briefly though, so I might be missing a lot of things):

  • It looks like it's fetching the PRs only after it's done fetching the remote(s). Is there a reason for this? For me, fetching often takes a lot of time, so I have to wait quite a while until the PR numbers show up.
  • I'm not sure the 500 limit works well. For our monorepo at work this amounts to roughly one month; sometimes we have PRs open for longer than that, so some of my open branches don't show PR numbers. Also, it would be really nice if you would also see PR numbers for very old branches. I haven't thought at all about how to achieve that, so sorry if this is not very constructive input.
  • It looks like PR numbers are only shown in the local branches panel. I'm thinking they could be even more useful in the remote branches list. (Not totally sure about that, it's just a feeling for now.)
  • Do I understand it right that at the moment the only thing this does is add the PR numbers to the display? I somehow expected you would be able to do more with this; the obvious wish is to open a PR in the web browser (I have a custom command for this). Maybe the o command could be overloaded for this? Show the existing PR if we know there is one, and open a new one if not.

@jesseduffield
Copy link
Owner Author

It looks like it's fetching the PRs only after it's done fetching the remote(s). Is there a reason for this? For me, fetching often takes a lot of time, so I have to wait quite a while until the PR numbers show up.

It needs remotes to map from PRs back to branches i.e. the PR has an owner e.g. 'jesseduffield') and the remote has a URL (e.g. github.com/jesseduffield/lazygit) and the branch has the name of the remote. So we need to piece those together to know that a particular PR should be assigned to a particular branch. imo getting remotes should be really fast because it's just a matter of reading a file so if it's currently a bottleneck it's worth looking into.

I'm not sure the 500 limit works well. For our monorepo at work this amounts to roughly one month; sometimes we have PRs open for longer than that, so some of my open branches don't show PR numbers. Also, it would be really nice if you would also see PR numbers for very old branches. I haven't thought at all about how to achieve that, so sorry if this is not very constructive input.

No need to apologise, I've been thinking the same thing. I wish there was a way to pass multiple branch heads to gh or to the graphql endpoint (gh is just a way of talking to graphql) but from what I've seen and tried, there's not. We could try a parallelised approach of shelling out to a bunch of gh processes at once but it would probably be rate limited.

It looks like PR numbers are only shown in the local branches panel. I'm thinking they could be even more useful in the remote branches list. (Not totally sure about that, it's just a feeling for now.)

I agree, and it shouldn't be hard to support that.

Do I understand it right that at the moment the only thing this does is add the PR numbers to the display? I somehow expected you would be able to do more with this; the obvious wish is to open a PR in the web browser (I have a custom command for this). Maybe the o command could be overloaded for this? Show the existing PR if we know there is one, and open a new one if not.

Yep currently it's just for display but I like your idea.

@jesseduffield jesseduffield force-pushed the gh-integration-3 branch 2 times, most recently from 134a144 to 319ca04 Compare June 3, 2024 12:42
@jesseduffield jesseduffield changed the title Show pull requests against branches WIP: Show pull requests against branches Jun 3, 2024

func fetchPullRequestsQuery(branches []string, owner string, repo string) string {
var queries []string
for i, branch := range branches {
Copy link

@dlvhdr dlvhdr Jun 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this in one request (not sure about the length limits for the query) with:

query {
  search(first:3, query:"head:gh-integration-3 head:integration-tests-on-windows repo:jesseduffield/lazygit", type: ISSUE) {
    nodes {
      ... on PullRequest {
        url
      }
    }
  }
}

which returns:

{
  "data": {
    "search": {
      "nodes": [
        {
          "url": "https://github.com/jesseduffield/lazygit/pull/2781"
        },
        {
          "url": "https://github.com/jesseduffield/lazygit/pull/2648"
        }
      ]
    }
  }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dlvhdr Interesting, playing with this now. I'm new to GraphQL and all this stuff.

May I ask a few questions?

  • What are some criteria for which is better? When I tried both versions using a list of 10 branches, they both took roughly the same time (a bit over 500ms).
  • When you say "one request", then the original version is also just one request (i.e. one http call). It just has a bunch of subqueries, and I couldn't find a lot of information about what these even are or how they work. (But I didn't try hard.) Ok, so this wasn't a question. 😄
  • Why does this even work? The github documentation says that when you put multiple things into a search query, it ANDs them together by default, so I would have expected that you need query:"(head:gh-integration-3 OR head:integration-tests-on-windows) repo:jesseduffield/lazygit". Does github have the DWIM logic to synthesize the right boolean operator based on what field types you search for?

Copy link

@dlvhdr dlvhdr Oct 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh then I must have been mistaken.. Seems pretty much identical.
Regarding the 3rd point I couldn't find the documentation for it, but I swear it exists.
Anyway if you use https://docs.github.com/en/graphql/overview/explorer and provide this query, it works:

query FetchSponsors {
   search(query:"is:pr repo:dlvhdr/gh-dash repo:jesseduffield/lazygit", type:ISSUE, first: 10) {
    nodes {
      ... on PullRequest {
        title
        repository {
          nameWithOwner
        }
      }
    }
  }
}

Maybe github does an implicit OR if an AND doesn't make sense.

@Daemoen
Copy link

Daemoen commented Jul 10, 2024

Is there a plan to make gh pr usable in LG, or only the external view? I would love to see it allow us to use 'gh pr create/edit/etc'.. if lg could add in support for the gh command for prs... it would be amazing and make it so i dont even have to leave my nvim/lg setup xD

@mpasa
Copy link

mpasa commented Jul 12, 2024

@Daemoen You can easily do this as a customCommand. For example, for creating a PR on the current branch:

customCommands:
  - key: "o"
    command: "gh pr create"
    subprocess: true
    context: "localBranches"

@OliverJAsh
Copy link

OliverJAsh commented Aug 9, 2024

Could we show a list of pull requests and their titles? I would actually use this instead of the branch view most of the time. This way I wouldn't need to remember the name of the branch or the PR number. I just need the (human readable) title.

This would also be useful for code review. Currently I have to go to GitHub, copy the branch name, and checkout the branch, whereas with this workflow I could just directly checkout the PR inside of lazygit. The branch name is an implementation detail.

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.

7 participants