fix(forge): make recursive forge update optional via --recursive flag

[emo-eth]

Motivation

forge update recursively updates submodules, which causes submodules with out-of-date dependencies to become out-of-sync with their remote. This can lead to non-deterministic builds and a messy git working directory.

This PR makes the default behavior only to update root-level dependencies, while still recursively fetching any updated sub-dependencies.
It also adds a --recursive flag to mimic the old behavior, if for some reason it is desired.

Closes #5926.

Unfortunately, as I'm not familiar with this codebase or Rust generally – I'm not sure the best way to go about testing this. Would appreciate input and guidance.

Solution

• add recursive flag to Git::submodule_update
• add submodule_foreach to Git
• add recursive flag to forge update
• when recursive is false, initialize submodules non-recursively, then git submodule foreach a recursive, non-remote initialization submodule update

[emo-eth]

It looks like this is actually a regression from #2274, which fixed issue #2264.

This is due to the test being ignored:

foundry/crates/forge/tests/cli/cmd.rs

Line 978 in 163cccd

#[ignore]

[Evalir]

so supportive, albeit I do remember merging a similar PR and this inadvertently breaks forge's behavior for a lot of ppl as some expect recursive updates. @mds1 / @mattsse wdyt?

[mattsse]

I think this is reasonable, though I usually tend to use git directly myself.

but I can see how this is very useful in CI etc

I did not test this though.

I admit we've been a bit careless with all the git bindings, all of this just sucks.

[emo-eth]

@mattsse @Evalir what do you think the best way to go about testing this is? looks like old test was ignored because the example repo was deleted; introducing a new repo to test against could similarly have regressions.

[mds1]

I think this is correct that we don't want --recursive as the default, so supportive as long as this doesn't break anything and we properly handle fetching of deps on update. Not sure what the best way to test is though, sorry 😅

[emo-eth]

As for @Evalir's comment about being a breaking change – I'm not sure that it is:

Recursively updated submodules either update correctly (no recursive updates required) or they're left in a "dirty" state, which git won't/can't commit to the working tree. The current behavior means you need to manually revert the (recursive) updates that left dependencies "dirty" before you can commit the update to the root project (or, if you own the submodule commit and push the changes to the remote).

At least, AFAIK

[Evalir]

coming back to this! @jameswenzel sorry for the wait. tested and works well—I think for the test it's okay if you make a suitable repo to replace the one that the ignored test uses, and maybe you can then transfer to me? won't delete, so should be OK to use :)

[emo-eth]

@Evalir Just pushed an update to the existing ignored test. I initiated a transfer for these two repos; the path will probably have to be updated in the test once you accept ownership:

https://github.com/jameswenzel/forge-5980-test
https://github.com/jameswenzel/forge-5980-test-dep

The former has a dependency on the latter; the latter has an upstream breaking change, so the test will fail if Forge tries to update submodules recursively.

[Evalir]

Accepted the moves & updated!