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

Add pr update command #8953

Open
wants to merge 17 commits into
base: trunk
Choose a base branch
from

Conversation

babakks
Copy link
Contributor

@babakks babakks commented Apr 10, 2024

This PR adds the new pr update command, to update a PR branch with the latest changes of its base. Under the hood, we use the updatePullRequestBranch mutation.

Fixes #8426

Details

The command doc is:

Update a pull request branch with latest changes of the base branch.

Without an argument, the pull request that belongs to the current branch is selected.

The default behavior is to update with a merge commit (i.e., merging the base branch
into the the PR's branch). To reconcile the changes with rebasing on top of the base
branch, the `--rebase` option should be provided.


USAGE
  gh pr update [<number> | <url> | <branch>] [flags]

FLAGS
  --rebase   Update PR branch by rebasing on top of latest base branch

INHERITED FLAGS
      --help                     Show help for command
  -R, --repo [HOST/]OWNER/REPO   Select another repository using the [HOST/]OWNER/REPO format

EXAMPLES
  $ gh pr update 23
  $ gh pr update 23 --rebase

LEARN MORE
  Use `gh <command> <subcommand> --help` for more information about a command.
  Read the manual at https://cli.github.com/manual

QA

I was able to QA the changes using the script below. This should be run at the repo's root directory, with the binary at bin/gh. Note that the script will nevertheless clean up on exit.

#!/usr/bin/env sh

set -e

_tmp=/tmp
_repo=gh-some-repo
_gh=$(pwd)/bin/gh
_pwd=$(pwd)

cleanup () {
    cd $_pwd
    ARG=$?
    rm -rf "$_tmp/$_repo"
    gh repo delete --yes $_repo
    exit $ARG
} 
trap cleanup EXIT

cd $_tmp
$_gh repo create --private --add-readme $_repo
$_gh repo clone $_repo
cd $_repo
git checkout -b pr-branch
echo "updated in pr branch" > README.md
git commit -am 'change in pr'
git push -u origin pr-branch
$_gh pr create --fill --title "test" --body ''
git checkout main
touch NEW.md
echo "new file on main" > NEW.md
git add NEW.md
git commit -am 'change on main'
git push -u origin main
git checkout pr-branch
$_gh pr update

Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
@babakks babakks requested a review from a team as a code owner April 10, 2024 18:51
@cliAutomation cliAutomation added the external pull request originating outside of the CLI core team label Apr 10, 2024
@cliAutomation cliAutomation added this to Needs review 🤔 in The GitHub CLI Apr 10, 2024
Signed-off-by: Babak K. Shandiz <[email protected]>
@babakks
Copy link
Contributor Author

babakks commented Apr 10, 2024

@williammartin This PR is actually my second (and simpler) approach to add the pr update command. My initial approach included a second step where we git pull the latest PR branch updates. You can find that implementation here on this branch on my fork.

Unfortunately, this wasn't easily possible because the updated PullRequest instance returned by the API endpoint, does not yet reflect the changes after the merge/rebase. To be precise, the returned PullRequests's HeadRefOid remains unchanged (in comparison with what it was before calling the endpoint). I even tried to exlicitly fetch the PR again in a separate request, but since the merging takes a bit of time, the returned PR is still at the same ref. The solution is to wait for a few seconds to ensure the merge has been done, but I'm not sure about the systematic way do to this (I mean, instead of waiting for a random amount of time, which is a weird and unreliable behavior). If you have any solution for this, I'll be more than happy to apply it and push the solution because that provides a nicer UX.

@babakks babakks changed the title 8426 add pr update cmd no local update Add pr update command Apr 10, 2024
Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

Generally this looks excellent. Really easy to read top to bottom and great test coverage. However, there is some surprising behaviour that I think we need to address somehow.

Surprising Behaviour

Merging

There's some surprising behaviour here in my mind around updating branches that are already up to date. For example,
#9033, which was 1 commit ahead and 0 commits behind trunk when I ran gh pr update 9033 3 times with the result being:

image

I fixed this with gh pr update --rebase 9033 which removed the merge commits (but also did something unexpected documented below):

image

Rebase

Now for the strange --rebase behaviour:

image

After rebasing with gh pr update --rebase 9033, I would not anticipate changes to the commit sha either:

image

What's going on?

For some reason that I will have to ask about internally, the API that powers these operations acts differently than git merge or git rebase which say Already up to date and no-op.

What should we do?

The Web UI only shows this button when the branch is out of date:

image

Although it's an extra network hop, it seems like the right thing to do is to check whether our PR branch is actually behind and only then request the mutation.

In the meantime I'm going to see if I can get an answer from the team that owns the API.

@williammartin
Copy link
Member

I spoke to the API team internally and they are going to look at whether there should be some changes here. It appears this behaviour could also occur if you clicked the Update branch on a stale browser window with the issue being resolved in the background using git.

Let's proceed with adding a check that there is indeed work to be done before sending the mutation, thanks.

@babakks
Copy link
Contributor Author

babakks commented May 6, 2024

@williammartin Thanks for the investigation. I added the check by using the compare method in the PR type's baseRef object. This checks whether the PR branch (head) is behind the base branch, and if yes, we'll proceed with the update. I'll soon provide you with the QA scripts.

Signed-off-by: Babak K. Shandiz <[email protected]>
@babakks
Copy link
Contributor Author

babakks commented May 9, 2024

@williammartin I just QA-ed the PR with the following script and it worked with the merge (not rebase) approach. That is, the first call updated the branch and second call resulted in an "already up-to-date" message. But when I tried with the --rebase option, I got this API error for the first gh pr update invocation, which I'm not sure what it means:

GraphQL: rebase not prepared (updatePullRequestBranch)

UPDATE I think it's a timing issue and the backend needed some time to process the changes. When I added a 30-second sleep before calling the gh pr update commands, the --rebase option worked well, too. So, seems like things are working as expected.

QA

To run this script, you need to build the gh binary (which goes to bin/gh) and then run the script from the root of the repo. As you can see, the gh pr update is called at the end, twice. You can add the --rebase option to both to get the error I mentioned above.

#!/usr/bin/env sh

set -e

_tmp=/tmp
_repo=gh-some-repo
_gh=$(pwd)/bin/gh
_pwd=$(pwd)

cleanup () {
    cd $_pwd
    ARG=$?
    rm -rf "$_tmp/$_repo"
    gh repo delete --yes $_repo
    exit $ARG
} 
trap cleanup EXIT

cd $_tmp
$_gh repo create --private --add-readme $_repo
$_gh repo clone $_repo
cd $_repo
git checkout -b pr-branch
echo "updated in pr branch" > README.md
git commit -am 'change in pr'
git push -u origin pr-branch
$_gh pr create --fill --title "test" --body ''
git checkout main
touch NEW.md
echo "new file on main" > NEW.md
git add NEW.md
git commit -am 'change on main'
git push -u origin main
git checkout pr-branch

# This sleep helps with the "rebase not prepared" API error.
sleep 30

# This should result in "PR branch update"
$_gh pr update

# This should result in "PR branch already up-to-date"
$_gh pr update

@babakks
Copy link
Contributor Author

babakks commented May 9, 2024

@williammartin Also, when there's a merge conflict, we just print the API error, which is:

GraphQL: merge conflict between base and head (updatePullRequestBranch)

I'm not sure how we can detect this specific error and display it in a nicer way.

UPDATE I checked the Status field of the returned Comparison object, and it just says "DIVERGED", which is not conclusive for this case; because the same status could happen when both branches have changed but not in a conflicting way (like in the QA script).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external pull request originating outside of the CLI core team
Projects
No open projects
The GitHub CLI
  
Needs review 🤔
Development

Successfully merging this pull request may close these issues.

Add a gh pr update command corresponding to the API call / button on the Web UI
3 participants