search

aviator-co / av

A command line tool to manage stacked PRs with Aviator
https://aviator.co
MIT License
266 stars 22 forks source link

Updating a PR metadata before git-push is not possible in some cases #395

Open draftcode opened 3 months ago

draftcode commented 3 months ago

In https://github.com/aviator-co/av/pull/352, we switched to call GitHub API to update the PR metadata first, then doing a git-push. This is to make sure that a CI service sees the base branch data correctly.

However, if a branch order is swapped, the PR metadata cannot be updated. When a stack is like main <- feature1 <- feature2 originally, and then swapped the feature1 and feature2 (main <- feature2 <- feature1), without a git-push beforehand, GitHub recognizes that there's no new commit between feature2 and feature1 (because GH is still recognizing them based on the pre-reorder state), rejecting the API request.

draftcode commented 3 months ago

I guess we can check if the branch order is the same as the current local branch order. This won't detect all the cases that GH would recognize there's no new commit, but should cover some.

wahyudibo commented 1 month ago

Hi @draftcode, i'm pleased to meet you. I've been assigned to this issue by @jainankit and trying to replicate the issue on my end. I have followed the video in https://docs.aviator.co/aviator-cli and here's the result by running av stack tree

$ av stack tree

  * frontend-change
  │ https://github.com/wahyudibo/test-aviator-av/pull/2
  │
  * backend-1 (HEAD)
  │ https://github.com/wahyudibo/test-aviator-av/pull/1
  │
  * main

I run these commands to swap the branches order

$ av stack reorder
Starting branch frontend-change at c8ba99a
  - applied commit 50332e7 without conflict (HEAD is now at 2dd3ce4)
Starting branch backend-1 at 2dd3ce4
  - applied commit c54ea93 without conflict (HEAD is now at 1fbbf80)
  - applied commit 934a4d3 without conflict (HEAD is now at 8d8b41d)
Reorder complete!

The stack was reordered successfully.

$ av stack tree

  * backend-1 (HEAD)
  │ https://github.com/wahyudibo/test-aviator-av/pull/1
  │
  * frontend-change
  │ https://github.com/wahyudibo/test-aviator-av/pull/2
  │
  * main

Then i run av stack sync and choose Yes when it asked me to push to github and i got this result

$ av stack sync

  ✓ GitHub fetch is done
  ✓ Restack is done

    * ✓ backend-1 80669e1
    │
    * frontend-change eb81b3e
    │
    * main 312a571

  ⡿ Pushing to GitHub...

    Following branches are being pushed...

      frontend-change
        Remote: 8c68447 frontend change 2024-10-06 05:06:31 +0700 +0700 (3 minutes ago)
        Local:  eb81b3e frontend change 2024-10-06 05:09:15 +0700 +0700 (53 seconds ago)
        PR:     https://github.com/wahyudibo/test-aviator-av/pull/2

      backend-1
        Remote: 376f2d8 more backend changes 2024-10-06 05:06:15 +0700 +0700 (3 minutes ago)
        Local:  80669e1 more backend changes 2024-10-06 05:09:15 +0700 +0700 (53 seconds ago)
        PR:     https://github.com/wahyudibo/test-aviator-av/pull/1

error: failed to update pull request: github error: There are no new commits between base branch 'frontend-change' and head branch 'backend-1'

Please kindly confirm if i reproduce the issue correctly. Thanks!

draftcode commented 1 month ago

Yeah. That's the issue.

wahyudibo commented 1 month ago

Thank you for your confirmation @draftcode. I have working on the solution but i just want to confirm if my approach is correct. In your comment above

I guess we can check if the branch order is the same as the current local branch order. This won't detect all the cases that GH would recognize there's no new commit, but should cover some.

I wonder if after we find out that the orders are different between local and remote, then what is the suggested next step?

My thought based on my experiment at https://github.com/wahyudibo/test-aviator-av/pull/1: since we're updating PR before pushing it to github,

In my testing repo, i create 2 branch stack (main > backend-1 > frontend-change), reorder them using av stack reorder so the order changes (main > frontend-change > backend-1) then run av stack sync using modified binary where the above error is skipped so the cli is not stopped. The result is both frontend-change and backend-1 base branches in the PR are pointed to main. This condition can be fixed if we run av stack sync once again. This time backend-1 branch in the PR correctly points to frontend-change instead of main.

Please let me know if this makes sense or not and if you have further suggestion.

jainankit commented 1 month ago

Hi @wahyudibo your strategy sounds good to me. I suspect that this will be a very rare scenario, so handling the failure gracefully, and fixing the PR metadata after push is a decent fallback.

Just want to make sure that you are not expecting the user to run av stack sync twice? We should fix the PR metadata in the same command execution.

wahyudibo commented 1 month ago

Thank you @jainankit for your feedback.

Just want to make sure that you are not expecting the user to run av stack sync twice? We should fix the PR metadata in the same command execution.

Yes, i just want to highlight that the process of running av stack sync twice can actually fix it but in the implementation, I'll make it run in the same command execution.