Skip to content
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.

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

fix(forge): run dep.has_branch in correct dir #9453

Merged
merged 1 commit into from
Dec 3, 2024
Merged

Conversation

yash-atreya
Copy link
Member

@yash-atreya yash-atreya commented Dec 2, 2024

Motivation

Closes #7225.

Current while installing a dep, we are checking for the deps branch in project root, which is incorrect as the tagged branch, if it exists, will be in the directory of the dep i.e lib/dep-name

Solution

Check whether the branch exists in the correct dir i.e lib/dep-name

Tested this on @zerosnacks's repro https://github.com/zerosnacks/foundry-bug-7225-repro, correctly writes the branch = 0.8 in .gitmodules

@yash-atreya yash-atreya changed the title fix(forge): run git.has_branch in correct dir fix(forge): run dep.has_branch in correct dir Dec 2, 2024
@yash-atreya yash-atreya self-assigned this Dec 2, 2024
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

Nice catch! This won't fix the installing from tag issue but we IMO we should merge the fix and then look to extend to tag / rev, @zerosnacks wdyt?

@yash-atreya
Copy link
Member Author

Nice catch! This won't fix the installing from tag issue but we IMO we should merge the fix and then look to extend to tag / rev, @zerosnacks wdyt?

Yeah this doesn't address the tag/rev issue
Will look into it next

@yash-atreya yash-atreya enabled auto-merge (squash) December 3, 2024 05:36
@yash-atreya yash-atreya requested a review from grandizzy December 3, 2024 05:36
Copy link
Member

@zerosnacks zerosnacks left a comment

Choose a reason for hiding this comment

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

Tested against the repro, works well!

Landing this fix already should unblock users, see https://github.com/OpenZeppelin/openzeppelin-contracts?tab=readme-ov-file#foundry-git

@yash-atreya yash-atreya merged commit ade4b35 into master Dec 3, 2024
21 checks passed
@yash-atreya yash-atreya deleted the yash/fix-7225 branch December 3, 2024 09:04
@grandizzy grandizzy added T-bug Type: bug C-forge Command: forge labels Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge T-bug Type: bug
Projects
Status: Completed
Development

Successfully merging this pull request may close these issues.

Bug: forge does not add tag information into gitmodules file on install
3 participants