[github] exclude firstline from body in new pr

[bolinfest]

Currently, when I create a PR via Sapling, I inevitably end up deleting the duplicated first line from the body of the commit message in the generated pull request.

It appears that create_pull_request_title_and_body() in pull_request_body.py already has logic like what this PR introduces:

https://github.com/facebook/sapling/blob/5aff04c89e3473b0d7a95f1dc223858528e4a575/eden/scm/sapling/ext/github/pull_request_body.py#L92-L94

so this updates create_pull_requests_serially() to work the same way.

[facebook-github-bot]

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[bolinfest]

I wouldn't be surprised if there are tests I need to update. I see that "Writing Tests" was added recently, which is great:

https://sapling-scm.com/docs/dev/process/writing_tests

but external contributors still need a command to run to verify we haven't broken anything.

Could we maintain a file that lists the tests that aren't expected to run in OSS and a script that runs everything except those tests? Then perhaps those of us in the OSS side can help whittle that list down over time?

[zzl0]

Thanks @bolinfest for PR, and sorry for the late reply.

For this change, I think running github extension tests should be enough.

$ ls *.t | grep github
test-ext-github-ghrevset.t
test-ext-github-pr-submit-closed.t
test-ext-github-pr-submit-overlap.t
test-ext-github-pr-submit-placeholder-issue.t
test-ext-github-pr-submit-single.t
test-ext-github.t

https://github.com/facebook/sapling/blob/5aff04c89e3473b0d7a95f1dc223858528e4a575/eden/scm/sapling/ext/github/pull_request_body.py#L92-L95

The logic in pull_request_body.py might have a "bug", we might need to change it to something like below:

     if title is None:
         title = firstline(commit_msg)
-        body = commit_msg[len(title) + 1 :]
-    body = _strip_stack_information(commit_msg)
+    body = commit_msg[len(title) + 1 :]
+    body = _strip_stack_information(body)
     extra = []
     if len(pr_numbers_and_num_commits) > 1:

Could we maintain a file that lists the tests that aren't expected to run in OSS and a script that runs everything except those tests?

This is a good idea, I will get back to this after some investigation.

[facebook-github-bot]

@zzl0 merged this pull request in facebook/sapling@868e9499db9f291ee1cc674c1bf8730cd888b2f2.

[bolinfest]

@zzl0 thanks for merging!

Though based on your comment: do you think I should move the title_and_body() function to a utility file so it can be used in both submit.py and pull_request_body.py?

[bolinfest]

Oh wait, I just saw that the function is new in your commit: https://github.com/facebook/sapling/commit/aec081c47b71d239b695cfb3666709850d180f64

[bolinfest]

Ah, I see. Looking at the code now, I feel like it might be clearer if instead of create_pull_request_title_and_body() taking commit_msg and title as Optional[str], it should have one param that has to be either commit_msg: str or (title, body): tuple[str, str].

What do you think?

[zzl0]

Though based on your comment: do you think I should move the title_and_body() function to a utility file so it can be used in both submit.py and pull_request_body.py?

Yeah, I have done it.

it should have one param that has to be either commit_msg: str or (title, body): tuple[str, str]. What do you think?

@bolinfest sounds good! Done in https://github.com/facebook/sapling/pull/902

[bolinfest]

Nice: thanks @zzl0!