fix: always prune on git fetch (#10664)

[ADustyOldMuffin]

This should clean up any old branches and save on disk space, and fix any errors around bad branch names.

fixes #10664

Most initial tests of timing the git fetch command with and without prune found that differences in amount of time on large repositories with many changes were within 10-50ms of each other, and with smaller repositories with low amounts of changes the difference was closer to un-measurable if not the same.

I think this combined with possibly saving space and removing old refs adds up that it makes sense to auto prune on fetch so we can avoid random issues with git branches.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • (b) bug fix
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Nothing to change
• Does this PR require documentation updates?
  • I don't think so.
• I've updated documentation as required by this PR.
  • N/A
• Optional. My organization is added to USERS.md.
  • Not at this time
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • Tests for this case are already written, but git error message was different. Existing test should still work/pass for this.
• My build is green (troubleshooting builds).

[crenshaw-dev]

@ADustyOldMuffin do you think you can design a unit test to demonstrate that this change works? Might be able to use a local directory as the "remote" for a git repo.

[ADustyOldMuffin]

I can add more tests if needed, I was hoping these tests added with the previous change would cover this change as well to ensure we're also still fixing the previous issue. After looking at them I can add some extra tests/validation to ensure we're cleaning up old branches possibly.

[crenshaw-dev]

Yeah, I woulda hoped those tests would have caught this as well. But they must be insufficient, since you're still hitting the cannot lock ref error. I wonder under what conditions one of those error messages is produced vs. the other.

[ADustyOldMuffin]

I took a second look and I don't believe those tests actually covered anything since the fetch is being ran in the same local repo. I'll write some tests that add a local remote and actually attempt to fetch. Our issue stemmed from the same issue being tested.

[codecov]

Codecov Report

Base: 45.66% // Head: 45.64% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (016c172) compared to base (ca5e4c5).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10761      +/-   ##
==========================================
- Coverage   45.66%   45.64%   -0.02%     
==========================================
  Files         236      236              
  Lines       28690    28695       +5     
==========================================
- Hits        13102    13099       -3     
- Misses      13794    13803       +9     
+ Partials     1794     1793       -1     

Impacted Files	Coverage Δ
util/git/client.go	48.64% <66.66%> (-0.53%)	⬇️
util/argo/argo.go	63.45% <0.00%> (-0.99%)	⬇️
cmd/argocd/commands/app.go	18.70% <0.00%> (-0.05%)	⬇️
util/settings/settings.go	51.36% <0.00%> (ø)
util/dex/dex.go	58.62% <0.00%> (+1.47%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ADustyOldMuffin]

@crenshaw-dev I removed the prune and walked through the tests and it throws the same error we had in the issue, but ours didn't include the magic words.

I did some digging and found a multitude of errors that require pruning to remove the error and some of them having the same messaging but the try running 'git remote prune origin' messaging being inconsistent. I'm willing to change back, but I'd vote to prune by default to remove the possibility of hitting any of them. It also seems this is an issue in other repos as well.

[crenshaw-dev]

Awesome research! I love the phrase "a multitude of errors." :-P

I'll go ahead and and merge. If people see noticeable performance issues in 2.5.0-rcX, then we can revisit the possibility of a "laundry list of error messages" approach.

[crenshaw-dev]

Thanks @ADustyOldMuffin!

[crenshaw-dev]

If we want to consider cherry-picking this back, I think we'll want some metrics demonstrating that there's no performance regression. Maybe the initial tests you did would be sufficient, but it would be nice to have some stats for this code running in a live instance.