scm: add config option to stage files automatically

[bobertlo]

Fixes #4330

I'd appreciate feedback on this.

• The name scm_autostage was arbitrary, but chosen to match no_scm. I wasn't sure if just autostage would be too generic or if git_autostage might not be future proof.
• Maybe scm.remind_to_track could be renamed scm.handle_changes? I didn't want to go there without feedback. It's named in tests, but not called anywhere else in the codebase outside the context of this patch.
• Alternatively, this change could be handled in scm_context. I thought altering the attribute/behavior in the scm class would be more straight-forward but, after writing out the previous point, maybe a separate maybe scm_context could read the attribute from the repo class and handle the logic there, calling a separate function if changes are to be staged.

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
  config: document new core.autostage setting dvc.org#1779

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[bobertlo]

Thinking more on this I do think it would be much better to fork in scm_context and probably add a separate method in the scm class? I'll try to work on that next but would appreciate any feedback on this feature, config variable name, or anything else.

[bobertlo]

I moved the logic to scm_context and added @skshetry 's suggestion. I also changed config variable name from scm_autostage to just autostage, because that seems more intuitive for users and documentation.

[bobertlo]

Also, I was not sure about handling of scm.files_to_track. I ventured that it might be appropriate to clear it in track_changed_files() but I am not familiar with this code-base and would appreciate input on that decision!

If noone has objections to this I'll start looking at adding documentation possibilities soon.

Edit: And look at tests. I looked at the tests before this failed now so I think I know what failed and why.

[bobertlo]

Okay, I added autostage: False for the test I broke, then added another test for autostage: True.

I'm a little lost on how to test track_changed_files but I'm slowly reading through other tests to see what you guys have going on here.

EDIT:

I'm removing WIP, because the code seems ready for review at least.

• I was unsure about calling reset_tracked_files from track_changed_files or from scm_context but took a stab at it.
• I'm pretty new to python testing but i think the scm_context tests are complete, if not pretty.
• I don't know how to test track_changed_files in the current testing framework. Advice would be highly welcome, but I will keep trying to look into this as I have time.

[efiop]

Thank you for your patience!

Looks great! 🔥 Just a few minor comments to address.

[bobertlo]

That should cover both points.

Would you prefer squashed commits in PRs? I'm used to doing that but usually on my own commits only.

[efiop]

Would you prefer squashed commits in PRs? I'm used to doing that but usually on my own commits only.

We'll squash automatically when merging, so it is up to you how to organize this PR internally 🙂

[efiop]

@bobertlo Btw, we'll need to add this new option to https://dvc.org/doc/command-reference/config . Could you submit a PR for it as well, please?

[efiop]

@pmrowla Could you take a look, please? Wonder if this will affect the experiments feature in some harmful way.

[pmrowla]

In general, this LGTM.

Regarding the experiments feature, this shouldn't affect anything as far as I can tell. Currently dvc exp checkout ... will still not stage any files even if core.autostage is enabled, since nothing is added to the scm.files_to_track list by Experiments.checkout(). Given how the experiments feature is supposed to be used, this seems like the correct behavior to me. We can consider making exp checkout support auto-staging at a later date if needed.

[skshetry]

Looks good @bobertlo. Thanks for the PR. 🎉

[efiop]

Thank you @bobertlo ! 🙏