fix(git): Fix update_repo when there are untracked files

[jfpedroza]

Fixes #395

I went with the adding --include-untracked to git stash save route. That way if there are conflicts the rebase will still be performed.

Let me know if that's how I should make the tests (my Python is quite rusty) or if you have any other comments.

I can make the backport PR once this is approved.

[CLAassistant]

All committers have signed the CLA.

[tony]

@jfpedroza Assuming this PR does the trick: Do you want me to see if this is backportable to 0.13.x and the current vcspull?

(May be easier for me to look ahead of time)

[codecov]

Codecov Report

Merging #422 (cc8be1b) into master (9fddb55) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #422      +/-   ##
==========================================
+ Coverage   62.17%   62.46%   +0.29%     
==========================================
  Files          39       39              
  Lines        2911     2915       +4     
  Branches      695      697       +2     
==========================================
+ Hits         1810     1821      +11     
+ Misses        667      662       -5     
+ Partials      434      432       -2     

Impacted Files	Coverage Δ
src/libvcs/sync/git.py	68.01% <100.00%> (+2.57%)	⬆️
tests/sync/test_git.py	96.29% <100.00%> (+0.08%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[tony]

I have a proposal:

Add a stash_untracked_files: bool=False to update_repo() and add this conditionally

The reason why is: This sounds like a good knob/option for future vcspull versions to allow configuring in the future

[tony]

P.S. if that is too tedious/ beyond scope I can also do it and we can merge this as-is (up to you!)

[jfpedroza]

Would that then be set to True in sync.py on the vcspull side?

[tony]

Would that then be set to True in sync.py on the vcspull side?

Yes

[tony]

@jfpedroza On second thought I think this is safe enough for default behavior and merging as-is

What do you think? Any adverse side effects / consequences you can think of for users?

https://stackoverflow.com/a/6818797, https://stackoverflow.com/a/12681856

One of them mentions this:

Warning, doing this will permanently delete your files if you have any directory/ entries in your gitignore file.*

I'm not fully sure what that means and how that will work at scale yet. I will look at this closer in the morning.

In regards to #395 (comment), do you think this option is safest?

[jfpedroza]

I hadn't realized git stash show doesn't show untracked files.

Alternatively, we could do option 1. It leaves the untracked file in the working directory and performs the rebase anyway. If there are any conflicts with the incoming changes and the file, the rebase will be aborted.

[tony]

@jfpedroza I haven't fully thought through this yet

Alternatively, we could do option 1. It leaves the untracked file in the working directory and performs the rebase anyway. If there are any conflicts with the incoming changes and the file, the rebase will be aborted.

Can you make a second PR (third PR) with this as an example?

[jfpedroza]

Do you want me to see if this is backportable to 0.13.x and the current vcspull?

The changed parts don't look different, so I think it's easier to back port.

I would suggest that the backport doesn't have the function option, as old versions won't get new features.

[tony]

The changed parts don't look different, so I think it's easier to back port.

I would suggest that the backport doesn't have the function option, as old versions won't get new features.

That's fine

[tony]

@jfpedroza You can rebase now. master on both vcspull and libvcs should work.

[tony]

@jfpedroza

Release imminent, but I will fast-track a new release for this

FYI: I'm prepping to do a long awaited feature release for libvcs and vcspull

Provided on being confident this won't cause data loss and there's good tests: I will fast track a release for your solution rapidly after the PRs are merged - even I make a feature-level release today.

Stashing untracked files

I am not fully thought out on if this could cause data loss for files in certain cases (where we otherwise weren't before).

The situation is we have to anticipate the user can have any kind of git config permutation (maybe we're already good as-is and that stackoverflow post is FUD)

[jfpedroza]

I'll get to this.

I am not fully thought out on if this could cause data loss for files in certain cases (where we otherwise weren't before).

I'm going to implement option 1 where we don't stash untracked files. Git will abort the operation if rebasing conflicts with the working directory.

[tony]

@jfpedroza That sounds good to me!

[tony]

@jfpedroza When you feel this is ready, can you update the changelog and ping me on github? (so I don't miss it)

I will also refresh of course :)