Skip to content

Conversation

@rohan-mehta
Copy link

@rohan-mehta rohan-mehta commented Dec 28, 2024

This fixes #435 (and also #427).

What/why

As noted in the issue, git spr update can overwrite changes made to the PR title/description. To fix this, we can have a dedicated block that looks like this, which the code can search for.

<!-- SPR data start: please do NOT edit this section -->
  
<!-- SPR data end -->

This way, any edits outside of this block are ignored. Since the block uses comments, it visually looks the same as before. And finally, if no block is present (e.g. on first commit or deleted by a user), it appends to the end.

Test plan

I added new test cases. In addition, I ran a manual test which you can see in https://github.com/rohan-mehta/spr-test-repo. The process was:

  1. Create 3 commits, commit_1, commit_2, commit_3, run git spr update, ensure that the spr stack description shows up.
  2. Edit the PR for commit_1 (add stuff above the block) and commit_2 (add stuff above and below the block), then re-run git spr update and ensure that nothing is overwritten.
  3. Amend commit_2, re-run git spr update and ensure that nothing is overwritten.
  4. Create commit_4 at the top of the stack, re-run git spr update and ensure that (a) a new PR is created (b) all previous PRs are updated to show the new commit (c) edits aren't overwritten


bin:
goreleaser --snapshot --skip-publish --clean
goreleaser --snapshot --skip=publish --clean
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new version of goreleaser changed this flag (See #427)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #444 for a standalone PR that addresses this. This PR has many unrelated YAML changes that make it hard to be properly reviewed IMHO.

Copy link
Contributor

@kwk kwk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this contribution. Please remove the changes in readme.md and take a look at my test suggestion. The changes to .goreleaser.yml look irrelevant for the most part, so they should not be part of this PR. I've isolated changes in #444 that address problems with gorleaser 2.7. Maybe you can review this?


bin:
goreleaser --snapshot --skip-publish --clean
goreleaser --snapshot --skip=publish --clean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #444 for a standalone PR that addresses this. This PR has many unrelated YAML changes that make it hard to be properly reviewed IMHO.

Comment on lines +829 to +873
func TestEmbedSprDescription(t *testing.T) {
existingBody := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
old spr content
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.
`

newSpr := `updated spr content with new data`

want := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.`
got := embedSprDescription(existingBody, newSpr)
if got != want {
t.Fatalf("Unexpected embedSprDescription result.\nGot:\n`%s`\n\nWant:\n`%s`\n", got, want)
}

// Test if markers are missing, we append them
existingBodyNoMarkers := `# Some User Heading
No markers here.
`
wantNoMarkers := `# Some User Heading
No markers here.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->`
gotNoMarkers := embedSprDescription(existingBodyNoMarkers, newSpr)
if gotNoMarkers != wantNoMarkers {
t.Fatalf("Unexpected embedSprDescription result when markers are missing.\nGot:\n%s\n\nWant:\n%s\n", gotNoMarkers, wantNoMarkers)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use subtests to simplify the naming of variables and be able to focus more on the task than on the naming. I've added a sub-test which I think was missing: when there trimmed body is empty. I didn't run the tests but I hope they work as expected.

Suggested change
func TestEmbedSprDescription(t *testing.T) {
existingBody := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
old spr content
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.
`
newSpr := `updated spr content with new data`
want := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.`
got := embedSprDescription(existingBody, newSpr)
if got != want {
t.Fatalf("Unexpected embedSprDescription result.\nGot:\n`%s`\n\nWant:\n`%s`\n", got, want)
}
// Test if markers are missing, we append them
existingBodyNoMarkers := `# Some User Heading
No markers here.
`
wantNoMarkers := `# Some User Heading
No markers here.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->`
gotNoMarkers := embedSprDescription(existingBodyNoMarkers, newSpr)
if gotNoMarkers != wantNoMarkers {
t.Fatalf("Unexpected embedSprDescription result when markers are missing.\nGot:\n%s\n\nWant:\n%s\n", gotNoMarkers, wantNoMarkers)
}
}
func TestEmbedSprDescription(t *testing.T) {
newSpr := `updated spr content with new data`
t.Run("Update with existing spr content", func(t *testing.T) {
existingBody := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
old spr content
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.
`
want := `# Some User Heading
User paragraph that should remain untouched.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->
Another user paragraph that should remain untouched as well.`
got := embedSprDescription(existingBody, newSpr)
if got != want {
t.Fatalf("\nGot:\n%s\n\nWant:\n%s\n", got, want)
}
})
t.Run("No spr markers but body", func(t *testing.T) {
// Test if markers are missing, we append them
existingBody := `# Some User Heading
No markers here.
`
want := `# Some User Heading
No markers here.
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->`
got := embedSprDescription(existingBody, newSpr)
if got != want {
t.Fatalf("\nGot:\n%s\n\nWant:\n%s\n", got, want)
}
})
t.Run("No spr markers and empty body", func(t *testing.T) {
// Test if markers are missing, we append them
existingBody := " "
want := `
<!-- SPR data start: please do NOT edit this section -->
updated spr content with new data
<!-- SPR data end -->`
got := embedSprDescription(existingBody, newSpr)
if got != want {
t.Fatalf("\nGot:\n%s\n\nWant:\n%s\n", got, want)
}
})
}

Comment on lines +1 to +3
# THIS FORK

This is a fork of the original [git-spr](https://github.com/ejoffe/spr) project. The main change is that it doesn't overwrite the PR title and body when updating a PR. When [this PR](https://github.com/ejoffe/spr/pull/436) is merged, this fork will be deleted. I've updated the install instructions below to install from this fork instead of the original.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why include this in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR title/body gets overwritten on updates/merges

2 participants