Github forge problems

[arxanas]

I recently landed #1184 to implement Github as a backend for git submit, but it has a lot of problems. For posterity, I'm reproducing this message from private correspondence.

---

There are three main problems:

• Landing a stack can't be done well, as landing the bottom-most commit rewrites it in such a way that GitHub can't automatically update dependent commits. You can manually rebase every commit in the stack (and wait for CI), but it's quite tedious. Presumably this is why spr and many similar solutions have their own land commands. I'm not opposed to adding a --land mode of some sort to git submit, but it would also only work for repositories that you have push access to.
• When updating a PR, if a dependency PR was landed, then the PR message is updated to remove the dependency from the stack, which loses useful contextual information and churns the message for no reason. I think this has to be handled by including metadata in the PR message and preserving any historical dependency PRs if they appear to have been landed.
• There seem to be several bugs with things like reordering a stack where PRs lose track of the proper ancestry. (in such cases, it often resolves itself by running git submit again, but that command should be idempotent and this shouldn't happen). I may have chosen an altogether wrong algorithmic approach to handle updates, not sure. You might be able to suss some out with dedicated testing or regular usage.

Also:

• The local branch names are very long. It would be nice to update the local branch name to the form pr/123.

[hauleth]

Ok, I have tested it out.

So far I have noticed:

1. In most cases I want to apply commits independently, so each of the created PRs should target origin/HEAD, not PR for previous commit
2. I sometimes want to have PRs constructed from more than one commit so with tree like

◇ 57ccf2b 14h (main) chore: prod secrets update (#2002)
┃
◯ 1d17d01 12h (ft/use-dynamic-repos-for-pg-adaptor) ft: use dynamic repos for PG adaptor
┃
◯ 7ea5cc6 12h test: use better way to retry tests
┃
● 45baef1 4s (ᐅ test/reduce-flakiness) ft: another stuff

I want it to result in 2 PRs, not 3.
3. [a/b] prefix is irritating IMHO. Either it should not be there at all or should be applied as a suffix (preferably not applied at all if it will work as mentioned in 1.).

[willruggiano]

(1) I think this should be supported but not the default. IMO if you are stacking commits then those commits (by definition) build on each other, and therefore its counter intuititive to cherry-pick them onto the main branch. spr, for example, takes this approach and provides a --cherry-pick option to support this exact case. You can always git prev and then git record -d to rearrange your commits such that they don't descend from each other. In combination with something like (2), it would then be really easy to create a PR for each commit, cherry-picked onto the main branch.

(2) Revsets, maybe? For example:

# create a pr for the main branch
$ git submit 'main()'

# create prs for each draft branch
$ git submit 'draft() & branches()'

# create prs for each draft commit
$ git submit 'draft()'

# create prs for each draft commit in the current stack
# (this is likely the default)
$ git submit 'stack() & draft()'

(3) I like the prefix lol. But should probably gate it with a config variable.

[hauleth]

1. The problem with rearranging them is that I still end with working tree where I do not have some changes without some additional acrobatics.
2. See above, because the same applies there. Even when you submit commits via git submit <revset> it will treat all commits as a whole, it will not treat them independently and will create commit stacks.

Technically I could currently create "dummy merge" locally and then do some tango on rewriting commits higher in the stack to support that, but that is highly unergonomic. I am thinking about switching to jj for that reason, but jj has a problem that it do not support email-based workflow, which is a little bit of dealbreaker for me currently.

[arxanas]

Re 2, it was intended to allow multiple commits per PR:

git-branchless/git-branchless-submit/tests/test_github_forge.rs

Lines 507 to 598 in 3faa074

	fn test_github_forge_multiple_commits_in_pull_request() -> eyre::Result<()> {
	let GitWrapperWithRemoteRepo {
	temp_dir: _temp_dir,
	original_repo: remote_repo,
	cloned_repo: local_repo,
	} = make_git_with_remote_repo()?;
	if remote_repo.get_version()? < MIN_VERSION {
	return Ok(());
	}
	remote_repo.init_repo()?;
	remote_repo.clone_repo_into(&local_repo, &[])?;
	local_repo.detach_head()?;
	local_repo.commit_file("test1", 1)?;
	local_repo.commit_file("test2", 2)?;
	local_repo.commit_file("test3", 3)?;
	{
	let stdout = local_repo.smartlog()?;
	insta::assert_snapshot!(stdout, @r###"
	O f777ecc (master) create initial.txt
	|
	o 62fc20d create test1.txt
	|
	o 96d1c37 create test2.txt
	|
	@ 70deb1e create test3.txt
	"###);
	}
	{
	let (stdout, _stderr) = local_repo.branchless_with_options(
	"submit",
	&[&"--forge", "github", "--create", "HEAD"],
	&GitRunOptions {
	env: mock_env(&remote_repo),
	..Default::default()
	},
	)?;
	insta::assert_snapshot!(stdout, @r###"
	branchless: running command: <git-executable> push --set-upstream origin mock-github-username/create-test3-txt
	branch 'mock-github-username/create-test3-txt' set up to track 'origin/mock-github-username/create-test3-txt'.
	Updating pull request (title, body) for commit 70deb1e create test3.txt
	branchless: running command: <git-executable> push --force-with-lease origin mock-github-username/create-test3-txt
	Submitted 1 commit: mock-github-username/create-test3-txt
	"###);
	}
	{
	let state = dump_state(&local_repo, &remote_repo)?;
	insta::assert_snapshot!(state, @r###"
	Local state:
	O f777ecc (master) create initial.txt
	|
	o 62fc20d create test1.txt
	|
	o 96d1c37 create test2.txt
	|
	@ 70deb1e (mock-github-username/create-test3-txt) create test3.txt
	Remote state:
	@ f777ecc (> master) create initial.txt
	|
	o 62fc20d create test1.txt
	|
	o 96d1c37 create test2.txt
	|
	o 70deb1e (mock-github-username/create-test3-txt) create test3.txt
	Pull request info:
	{
	"pull_request_index": 1,
	"pull_requests": {
	"mock-github-username/create-test3-txt": {
	"number": 1,
	"url": "https://example.com/mock-github-username/mock-github-repo/pulls/1",
	"headRefName": "mock-github-username/create-test3-txt",
	"headRefOid": "70deb1e28791d8e7dd5a1f0c871a51b91282562f",
	"baseRefName": "master",
	"closed": false,
	"isDraft": false,
	"title": "[1/1] create test3.txt",
	"body": "**Stack:**\n\n* https://example.com/mock-github-username/mock-github-repo/pulls/1\n\n\n---\n\ncreate test3.txt\n\n"
	}
	}
	}
	"###);
	}
	Ok(())
	}

[arxanas]

Re 1, git-branchstack and jj parallelize might support this workflow. Under git-branchless, a command to cherry-pick in-memory could be useful, like #551, plus some additional scripting on your end. I don't plan on having git branchless submit itself support a non-stacked workflow for stacked commits.

I am thinking about switching to jj for that reason, but jj has a problem that it do not support email-based workflow, which is a little bit of dealbreaker for me currently.

I don't know the details, but perhaps you can use jj in a Git repo and use the Git tools for the email-based workflows?

[mhammerly]

The local branch names are very long. It would be nice to update the local branch name to the form pr/123.

i think the trick here is to create an issue first and then convert it to a PR. creating the issue assigns you an ID which you can use in your branch name and it'll stay the same when you convert the issue to a PR

$ gh issue create --title foo --body bar
https://github.com/$OWNER/$REPO/issues/19
$ git branch pr19
$ git push origin pr19
$ gh api /repos/$OWNER/$REPO/pulls -X POST -F issue=19 -F head=pr19 -F base=main --jq .url
"https://api.github.com/repos/$OWNER/$REPO/pulls/19"

i spent a little time reading the current GitHub forge implementation this evening and i'll see if i get anywhere with a little more time this weekend. sapling and jj are very nice but i need hooks, submodules, and LFS juuuust often enough that i am interested in improving the GH forge + GPG support in git-branchless haha

[mhammerly]

i've put up my experiment so far in a branch master...mhammerly:git-branchless:gh-forge-improvements. it's currently implemented as a new forge you can select with -F github2

main changes so far:

• the branch for PR 30 is named branchless/pr30 (the prefix is configurable)
• displays stack context in a table and can handle stacks that fork (see screenshots below)
• from light testing, it appears to handle reorders correctly

there's more to do, but first, @arxanas some questions if you don't mind:

• what's your appetite for this? would you have bandwidth for review if i keep going with this?
• do you have examples in git-branchless of concurrent operations with progress tracking? thinking of parallelizing gh pr edit calls and the like and want to follow convention
• i see you're pretty active in the jj discord/community. i'm not a jj user but if you have direction on how to make this easier to share with them i can keep it in mind (e.g. common dependencies, behaviors that aren't available in jj)

example of the stack info table:

the forked stack that the above PR is part of ("second" is before "first" because i reordered and updated the stack to test):

(my testing repo is https://github.com/mhammerly/stack-test which has some helper scripts for generating/nuking stacks)

[arxanas]

@mhammerly I'm generally interested, but I haven't had time to review code lately, so I'd be unlikely to have time to review+merge the changes 🙁

do you have examples in git-branchless of concurrent operations with progress tracking? thinking of parallelizing gh pr edit calls and the like and want to follow convention

git-branchless test uses concurrent calls, and I think the progress framework should support concurrent progress meters, etc. You may even find it helpful to use/modify the code as a library, since it generally handles "run a command on each commit". I think the Phabricator forge does that in some way.

[arxanas]

I recently learned about a new potential issue when integrating with GitHub, which Graphite / gt attempts to handle: when a PR is updated, some systems check the changed files with respect to the PR's merge-base with the target branch to assign reviewers:

• If you're not careful, after a rebase (or potentially other stack operations, like reordering?), you can briefly have a merge-base that includes a bunch of commits from the main branch, which will cause a bunch of unrelated people to be added as reviewers.
• By that point, the damage is done, since they'll be subscribed/get notifications, even if you were to remove them all (if I understand correctly).
• It's tricky because one PR's head branch may be another PR's base branch, so you can't necessarily update the PRs in isolation.

[ppwwyyxx]

ghstack avoids this issue, by letting each PR's base NOT point to another PR's head. Of course, this could lead to other complexities.

I implemented a gitlab version of submit and found it very hard to make everything work perfectly when the platform itself does not know the concept of stack.