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

retarget-pr and codeowners #1057

Closed
raylu opened this issue Sep 23, 2023 · 13 comments · Fixed by #1061, #1072 or #1074
Closed

retarget-pr and codeowners #1057

raylu opened this issue Sep 23, 2023 · 13 comments · Fixed by #1061, #1072 or #1074
Assignees
Labels
externally requested feature New feature or request github Relates to integration with GitHub usability Relates to user experience, clarity, learning curve, reducing confusion etc.
Milestone

Comments

@raylu
Copy link
Contributor

raylu commented Sep 23, 2023

Overview

(is there a place to ask questions about git-machete? I'm mostly filing this issue as a general question because there's no community tab for this project that I could find)

what is the intended workflow for a stacked PR?

$ git machete status 
  main
  └─branch1  PR #1
    └─branch2  PR #2

I had this setup and 1 got merged on github. so then I ran

$ git checkout main
$ git pull
$ git machete g down
$ git machete slide-out --delete

perhaps a little wordy, but ok. now I have

$ git machete status 
  main
  └─branch2  PR #2 (diverged from origin)

now what? I want to git machete github retarget-pr and git push --force-with-lease. but no matter what order I do this in, I end up in trouble

if I retarget first, PR 2 now includes changes from branch1. any codeowners on files touched by branch1 get added as reviewers to PR 2

if I force-push first, PR 2 now includes a bunch of changes from main. this pulls in ~every codeowner as a reviewer

Question

is there a less wordy way of updating-main-and-sliding-out-branch1-and-retargeting-and-force-pushing? totally ok if the answer is no; I was manually doing all that and going to my browser to retarget anyway

Suggestion

I believe ghstack and https://github.com/aviator-co/av mark the PR as draft, push, retarget, then mark the PR as ready for review as a single operation. draft PRs don't get codeowners automatically added as reviewers

git machete version

I'm running off my PR branch but it's approximately 3.18.3

@PawelLipski
Copy link
Collaborator

PawelLipski commented Sep 23, 2023

(is there a place to ask questions about git-machete? I'm mostly filing this issue as a general question because there's no community tab for this project that I could find)

I've just activated Discussions as well 😅

I had this setup and 1 got merged on github. so then I ran

Actually! this (and also the merge itself) can be done in a single command:

git machete advance

while on master branch. This will fast-forward master to branch1, then push master (which GitHub will consider to be merging the PR), then slide out branch1.

is there a less wordy way of updating-main-and-sliding-out-branch1-and-retargeting-and-force-pushing? totally ok if the answer is no; I was manually doing all that and going to my browser to retarget anyway

Wow this is a valid problem indeed, and no there is no way to do that with the current CLI... other than as you said, first running gh pr ready --undo, then git machete github retarget-pr and git push --force-with-lease (in any order), then gh pr ready.

Still, it'd definitely make sense (and should be easy) to add something like:

  • git machete github retarget-pr --draft-push-undraft
  • git machete github retarget-pr --push
  • git machete github push-retarget-pr
  • ...

Please let me know if you have any preferences/suggestion for how the CLI could look like 🤔

@PawelLipski PawelLipski added feature New feature or request github Relates to integration with GitHub usability Relates to user experience, clarity, learning curve, reducing confusion etc. externally requested labels Sep 23, 2023
@PawelLipski PawelLipski added this to the v3.20.0 milestone Sep 23, 2023
@raylu
Copy link
Contributor Author

raylu commented Sep 24, 2023

ah. for the project I'm developing on the most, the base branch is a protected branch and we use a merge queue to merge things onto the branch for us. so advance doesn't work in my case. users that are contributing to a repo via a fork will have the same problem, though

Please let me know if you have any preferences/suggestion for how the CLI could look like 🤔

maybe git machete github restack? my main feeling is that you would almost never want to retarget without also force-pushing anyway (maybe if you decide that 2 PRs should really be 1 and you slide-out and close the middle PR?). no strong opinions, though

@PawelLipski PawelLipski modified the milestones: v3.20.0, v3.19.0 Sep 24, 2023
@PawelLipski PawelLipski self-assigned this Sep 24, 2023
@PawelLipski
Copy link
Collaborator

Ok! work in progress 🛠️ I'll probably call it restack-pr for consistency with other github subcommands

@PawelLipski
Copy link
Collaborator

Almost there, soon to be released 📦

@raylu
Copy link
Contributor Author

raylu commented Oct 21, 2023

woo, thanks! I'll give it a whirl soon

@raylu
Copy link
Contributor Author

raylu commented Nov 1, 2023

ok it didn't quite work. we have this setting on in our repo
image
so after the first branch in the stack merges, it's deleted
so from branch2, I got

$ git machete github restack-pr
From github.com:orgname/reponame
 - [deleted]             (none)     -> origin/branch1
[...]
   361632230..cc1ebc6a7  main       -> origin/main
Proposed base branch 'branch1' was not found

the branch still exists locally, but I guess it's looking for origin/branch1

@PawelLipski
Copy link
Collaborator

Hmmm could you show your .git/machete at the moment of restack-pr? is branch1 still there? 🤔

@raylu
Copy link
Contributor Author

raylu commented Nov 3, 2023

ok so the actual branch names are raylu/bb2.27 and raylu/sysctls2
one interesting thing that happened is that after merging the first PR,
image
so actually, I didn't really need to use any tools to do this
(there's a separate problem that has nothing to do with machete where the first branch got squash merged by our tooling and github decided to change my base branch for me, which "added 2 commits" (the unsquashed commits))

anyway, to answer your question, my .git/machete has raylu/sysctls2 but no raylu/bb2.27. I didn't run any machete commands other than some --help stuff and git machete status; I wonder if status prompted me to remove the deleted branch? (I don't remember, sorry)

@PawelLipski
Copy link
Collaborator

PawelLipski commented Nov 5, 2023

Sorry for late response 🙄

I wonder if status prompted me to remove the deleted branch?

Yes indeed... git machete status removes the non-existent (typically: recently deleted) branches by default (it used to fail with an error on invalid branch, but this was an inferior UX). On the other hand, you say that the branch (branch1 aka raylu/bb2.27 ) still exists locally 🤔

ok it didn't quite work. we have this setting on in our repo

So back to the original question... git machete github restack-pr (same as retarget-pr) will always change the PR base to the parent in .git/machete. At the moment of running restack-pr, was raylu/bb2.27 still the parent of raylu/sysctls2? if raylu/bb2.27 was still the parent of raylu/sysctls2, then restack-pr tried to retarget the PR from main to raylu/bb2.27, which didn't exist anymore in the remote ☹️

Anyway, for the future, just run git machete slide-out on branch1 (which will also rebase branch2 onto main) and then git machete github restack-pr on branch2. We can think automating that even more (into a single command) ;)

@raylu
Copy link
Contributor Author

raylu commented Nov 6, 2023

no worries! less than a week turnaround is fine!

slide-out followed by github restack-pr worked great on a stack I tried just now

I think what happened is I habitually ran my command to prune merged branches. I guess I should stop using that if I'm gonna use machete
...alternatively, perhaps machete could add a command that checks if any local branches track remote branches that have been deleted https://gitlab.com/raylu/bin/-/blob/master/git-branchp (and, of course, not delete ones that are still part of stacks). it's pretty handy if you work in a repo that uses the auto-delete setting I screenshotted above

@PawelLipski
Copy link
Collaborator

Hmmm if it comes to branch delete automation, there's:

  • git machete delete-unmanaged (remove all local branches not present in .git/machete)
  • git machete github sync and similar git machete clean — remove all unmanaged branches AND all untracked branches that don't have any child in .git/machete

Albeit I'll probably deprecate clean & github sync soon, in favor of git machete slide-out --untracked (not yet implemented) — depending on user feedback etc. 🤔

@PawelLipski
Copy link
Collaborator

PawelLipski commented Nov 7, 2023

Also also, there's one slight downside to deleting local branches (right after merging them) when using git-machete, as explained in FAQ -> Ctrl+F not delete ☹️

@raylu
Copy link
Contributor Author

raylu commented Nov 7, 2023

continuing at #1088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
externally requested feature New feature or request github Relates to integration with GitHub usability Relates to user experience, clarity, learning curve, reducing confusion etc.
Projects
None yet
2 participants