Skip to content

Alert unpushed commits when merging a pull request#2255

Merged
samcoe merged 2 commits intocli:trunkfrom
quiye:alert-merging-with-unpushed
Jan 25, 2021
Merged

Alert unpushed commits when merging a pull request#2255
samcoe merged 2 commits intocli:trunkfrom
quiye:alert-merging-with-unpushed

Conversation

@quiye
Copy link
Contributor

@quiye quiye commented Oct 22, 2020

Fixes #969

I added comparing the last local commit with the pull request's last commit to see if the unpushed commit is on the local branch.

  • For strictly checking, we should check the last local commit hash for every commit pushed. However, this seems to be expensive to search. So I implemented simple commit check and branch check (the alert message says may have).
  • I added many cs.Stub("") to tests. It looks like verbose code, but I can't simplify these ...
    • It looks weird that some tests in merge_test.go don't need to add stub codes 🤔

@cli cli deleted a comment Nov 4, 2020
@vilmibm vilmibm requested a review from samcoe January 20, 2021 18:10
@vilmibm
Copy link
Contributor

vilmibm commented Jan 20, 2021

giving this to @samcoe to:

  • fix conflicts
  • disable this check if -R was used

@samcoe samcoe force-pushed the alert-merging-with-unpushed branch from 8010279 to 80b579d Compare January 25, 2021 21:13
@samcoe samcoe force-pushed the alert-merging-with-unpushed branch from 80b579d to 3f172ad Compare January 25, 2021 21:27
return err
}

if opts.SelectorArg == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

When selectorArg is empty we use the current local git branch to retrieve the PR so there is no need to recheck if the PR branch name matches the local git branch name.

Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

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

@quiye Thanks for the contribution.

I rebased against trunk, added a conditional so that we only check if there are diverging branches when no PR selector argument is provided, and wrote a test for this specific use case. I think it ready to 🚢

@samcoe samcoe requested a review from vilmibm January 25, 2021 21:33
@samcoe samcoe merged commit c71dc87 into cli:trunk Jan 25, 2021
@quiye quiye deleted the alert-merging-with-unpushed branch January 26, 2021 08:55
@quiye
Copy link
Contributor Author

quiye commented Jan 26, 2021

@samcoe Thanks for reviewing and fixing !

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.

Alert user if there are unpushed commits on the branch they are merging.

4 participants