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?