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

Do not enable auto fetch by default #343

Open
diego-santacruz opened this issue Jun 27, 2023 · 4 comments
Open

Do not enable auto fetch by default #343

diego-santacruz opened this issue Jun 27, 2023 · 4 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug
Milestone

Comments

@diego-santacruz
Copy link

Recently the Github and Azure repos extensions started to automatically doing a fetch in the background.

This is controlled via the "Github Repositories › Auto Fetch: Enabled" and "Azure Repos › Auto Fetch: Enabled" settings, which default to enabled.

This is problematic for people that use git push --force-with-lease because the local view of the remote changes without knowledge of the user and it is all too easy to trample over someone else's commits in a PR without noticing it.

I do not know if this is because the extension gained that capability recently, or the setting was changed, but I think it should be changed to default to disabled.

Also, the Git extension has an equivalent setting, called "Git: Autofetch", which defaults to false. So it is really surprising to have different defaults.

In my opinion, I think that the defaults for Github Repositories and Azure Repos should inherit from the setting of "Git: Autofetch", with the possibility to override it for these extensions, and same for the associated periods.

@joyceerhl
Copy link
Contributor

Those settings' default values haven't changed since they were introduced over a year ago.

the local view of the remote changes without knowledge of the user and it is all too easy to trample over someone else's commits in a PR without noticing it.

We should not auto-pull changes if you have uncommitted changes locally. Instead we should be showing you that you have incoming changes in the status bar item. If you are experiencing that the file contents change from under you, can you please share a screen recording or some repro steps? Thanks!

@joyceerhl joyceerhl self-assigned this Jun 27, 2023
@joyceerhl joyceerhl added the info-needed Issue requires more information from poster label Jun 27, 2023
@diego-santacruz
Copy link
Author

Those settings' default values haven't changed since they were introduced over a year ago.
Ah, OK, then for some reason they started having an effect in my setup not long ago.

We should not auto-pull changes if you have uncommitted changes locally. Instead we should be showing you that you have incoming changes in the status bar item.
My working copy is not being modified, so no pulls are being done automatically, which is a good thing. What happens is that a fetch is being done and this updates the local view of the remote. This may seem innocuous at first sight, but it is not, because it breaks the --force-with-lease option of git push, so workflows that rely on force pushes to PR branches become much more risky. It is also surprising since the default for the Git extension, is exactly the inverse (i.e., disabled).

I understand the value of having an auto fetch enabled for some workflows (as you point out), but it would be best to not have contradicting defaults on the various extensions (Git extension vs. Github repositories and Azure repo extensions) and follow the principle of least surprise.

On a related note, I find the action done by the sync button (i.e. pull) to be dangerous too, and people I know just avoid it, because it will do a pull merging upstream changes with local changes without asking, and it is usually not what one wants. One wants to have control over the git commit history and not just blindly merge. But that is outside the Github repositories and Azure repo extensions.

@joyceerhl
Copy link
Contributor

Apologies, I don't understand the issue that you run into here

a fetch is being done and this updates the local view of the remote

Can you provide a concrete example of a workflow that is broken by the auto fetch behavior? Do you mean that you have local uncommitted changes in a virtual GitHub/Azure Repos repository, someone else force pushes to the branch you're on, and your local view of those files is updated while you still have uncommitted changes?

@diego-santacruz
Copy link
Author

I've found this blog post which explains the use of git push --force-with-lease much better than what I can do: https://itnext.io/git-force-vs-force-with-lease-9d0e753e8c41

The thing is that force-with-lease will compare the commit id of the remote branch HEAD that it knows of locally (e.g., remote/origin/mybranch), which is only updated on a fetch, with the current one at the remote git repo. If they do not match then the push errors out. This allows to detect if someone else pushed to the branch upstream since the last fetch, and avoid losing those new commits otherwise.

Workflow can be like this:

  • Create branch "mybranch" from main
  • I create some commits on that branch and I push upstream
  • I ask a colleague to give it a look, while I still work on it.
  • Colleague pushes some extra commit to that branch
  • I rework my commits, rebasing, for instance by squashing a fix up commit.
  • I push, since I rebased I need to do a force push. If I do a plain "--force" then my colleague's commits would be lost. With a --force-with-lease push it would error out since the expected upstream HEAD does not match the actual one.
  • I rework my commits to pull my colleagues commits
  • I push again, this time succeeding and not losing any commits

If there is something doing periodic fetches in the background it would break this workflow, because the expected remote HEAD would be updated without me knowing, losing the value of the --force-with-lease option.

That is why I think that defaulting auto fetch to enabled is a dangerous thing.

Let me know if this explanation was clear enough.

@joyceerhl joyceerhl added bug Issue identified by VS Code Team member as probable bug and removed info-needed Issue requires more information from poster labels Dec 5, 2023
@joyceerhl joyceerhl added this to the Backlog milestone Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue identified by VS Code Team member as probable bug
Projects
None yet
Development

No branches or pull requests

2 participants