Skip to content

Cached PR template does not update after remote file changed #369

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
messense opened this issue Aug 30, 2024 · 10 comments · Fixed by #371
Closed

Cached PR template does not update after remote file changed #369

messense opened this issue Aug 30, 2024 · 10 comments · Fixed by #371
Labels
bug Something isn't working

Comments

@messense
Copy link

messense commented Aug 30, 2024 •
edited
Loading

I've updated pull_request_template.md in trunk of remote git repository, but git-spice still uses the old cached template when submitting stacks.
@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024

Hey, just to narrow this down: Is the branch you're submitting from restacked on top of trunk?

@abhinav abhinav added the bug Something isn't working label Aug 30, 2024
@messense
Copy link
Author

Yes.

@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024

Follow up: Is trunk fully synced?
If not, that would explain why it's using the old cache key because right now, it checks $trunk, not origin/$trunk, so even if an update to the template is merged to trunk, and you've fetched trunk, the local ref $trunk is behind and the cache is seeing the wrong version.
Easily fixable, but I want to make sure that's what you're seeing.

abhinav added a commit that referenced this issue Aug 30, 2024
@abhinav
submit: Invalidate template cache if origin/main is updated
0c54ec5 
We calculate the template cache key based on the hashes of the files
inside the locations in the repository where tempates are stored,
but we check their state at `$trunk` instead of `$remote/$trunk`.

This results in the cache not being invalidated
if the remote has been updated and fetched, but the local ref is behind.

Ref #369
@abhinav abhinav mentioned this issue Aug 30, 2024
Merged
@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024

If the above is the issue, then #370 should fix this.

abhinav added a commit that referenced this issue Aug 30, 2024
@abhinav
submit: Invalidate template cache if origin/main is updated (#370)
8bdfcaa 
We calculate the template cache key based on the hashes of the files
inside the locations in the repository where tempates are stored,
but we check their state at `$trunk` instead of `$remote/$trunk`.

This results in the cache not being invalidated
if the remote has been updated and fetched, but the local ref is behind.

Ref #369
@messense
Copy link
Author

messense commented Aug 30, 2024 •
edited
Loading

Is trunk fully synced?

I think so, I've ran git pull on my local checkout of trunk branch and also ran gs repo sync again.

@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024 •
edited
Loading

Oh, origin/$trunk was in sync? That doesn't sound right.
Would you be able to try with #370? It's merged on main, but you'd have to build from source.

And if the issue still persists: what is the path to the template file?
The other thing I'm guessing could be wrong here is that the template file name is lower case, and right now, git-spice only checks upper-case names (PULL_REQUEST_TEMPLATE).

@messense
Copy link
Author

The other thing I'm guessing could be wrong here is that the template file name is lower case, and right now, git-spice only checks upper-case names (PULL_REQUEST_TEMPLATE).

yes, the file name is indeed lower case: .github/pull_request_template.md

@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024

Aha! That's it! Fix incoming!

abhinav added a commit that referenced this issue Aug 30, 2024
@abhinav
github: Fix outdated PR templates with lower-case names
58b543a 
We use the contents of possible PR template sites
as the hash key for caching the PR templates we get from the remote.
This was only looking at PULL_REQUEST_TEMPLATE.md names,
even though pull_request_template.md is valid and accepted.

This change ensures that lower-cased versions are also considered
when calculating the hash key.

The contract of ChangeTemplatePaths has been adjusted
to state that all paths are case-sensitive, not the other way around,
since we don't have a guarantee that all forges will treat these
as case-insensitive.

Resolves #369
@abhinav abhinav mentioned this issue Aug 30, 2024
Merged
@abhinav
Copy link
Owner

abhinav commented Aug 30, 2024

Alright, #371 should take care of it.

abhinav added a commit that referenced this issue Aug 30, 2024
@abhinav
submit: Fix outdated PR templates with lower-case names (#371)
bccac00 
We use the contents of possible PR template sites
as the hash key for caching the PR templates we get from the remote.
This was only looking at `PULL_REQUEST_TEMPLATE.md` names,
even though `pull_request_template.md` is valid and accepted.

This change ensures that lower-cased versions are also considered
when calculating the hash key.

To do this, we consider upper-case and lower-case variants
of configured Forge template paths when calculating the cache key.
Case-insensitivity of this path is considered part of the Forge contract
because GitHub and GitLab both treat these as case-insensitive-ish.
We can adjust this assumption if we add a forge that doesn't.

Resolves #369
@messense
Copy link
Author

Thanks for the quick fix! v0.5.2 works fine now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

submit: Fix outdated PR templates with lower-case names abhinav/git-spice
2 participants
@abhinav @messense