Conversation
| } | ||
| } | ||
|
|
||
| fn get_commit_message_body(commit: &Commit<'_>) -> eyre::Result<String> { |
There was a problem hiding this comment.
suggestion: Instead, can we expose a wrapper for https://docs.rs/git2/latest/git2/struct.Commit.html#method.body? That should be more direct (and less likely to be buggy in case of invalid UTF-8, etc.).
fd89fe9 to
0066918
Compare
|
Done, thanks. I added a thin Local verification: TEST_GIT=$(command -v git) TEST_GIT_EXEC_PATH=$(git --exec-path) cargo test -p git-branchless-submit --tests
cargo fmt --check
git diff --check |
When submitting to GitHub, the commit summary is already used as the pull request title. Use the remaining commit message body for both initial creation and stack metadata updates so squash-merge title/body output does not repeat the subject. Add a forge regression for a subject/body commit and update existing snapshots for subject-only commits. Fixes arxanas#1680.
0066918 to
b29868c
Compare
|
|
||
| /// Get the commit message body. | ||
| #[instrument] | ||
| pub fn get_body(&self) -> Option<&str> { |
There was a problem hiding this comment.
Ah, sorry, looking at the other wrapper methods, this should also return a BString (and call body_bytes internally). Then, downstream, we should call String::from_utf8_lossy to handle invalid UTF-8 and be sure that we still render something in the PR body.
If body_bytes returns None, then you can propagate the error using the same pattern as for get_summary above.
| fn get_commit_message_body(commit: &Commit<'_>) -> String { | ||
| let mut body = commit.get_body().unwrap_or_default().to_owned(); | ||
| if !body.is_empty() && !body.ends_with('\n') { | ||
| body.push('\n'); |
There was a problem hiding this comment.
I don't think this \n logic is necessary —
- The docs specify that trailing whitespace is removed, so the check for
body.ends_with('\n')is redundant (the only case where this is not true is already handled bybody.is_empty()). - It might not be obvious, but the calling code uses a multi-line string literal, so it already adds a newline after
{commit_message_body}. - We can see in the current snapshot tests that the
bodyends with\n\n, indicating that there's an extra trailing newline.
I think we can remove this function altogether and write a line similar to this one at the call-sites:
let title = String::from_utf8_lossy(&commit.get_summary()?).into_owned();but for the body instead.
2 participants
Fixes #1680.
Problem
git branchless submit --forge githubuses the commit summary as the GitHub pull request title, but the create and update paths also include the full commit message in the pull request body. For a conventional subject/body commit message, that repeats the subject in the PR description. Projects that squash-merge with title plus body then get the title twice in the final commit message.Changes
Verification
cargo build -p git-branchless -p git-branchless-submitTEST_GIT="$(which git)" TEST_GIT_EXEC_PATH="$(git --exec-path)" cargo test -p git-branchless-submit test_github_forge --test test_github_forge -- --nocapturecargo fmt --all -- --check