Skip to content

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

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/libvcs/sync/git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,9 +418,9 @@ def update_repo(self, set_remotes: bool = False, *args: Any, **kwargs: Any) -> N
# to be able to perform git pull --rebase
if need_stash:
# If Git < 1.7.6, uses --quiet --all
git_stash_save_options = "--quiet"
git_stash_save_options = ["--quiet", "--include-untracked"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tony tony Sep 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Yes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

Copy link
Member

@tony tony Sep 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

try:
process = self.run(["stash", "save", git_stash_save_options])
process = self.run(["stash", "save"] + git_stash_save_options)
except exc.CommandError:
self.log.error("Failed to stash changes")

Expand Down
29 changes: 28 additions & 1 deletion tests/sync/test_git.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ def test_repo_git_obtain_full(

@pytest.mark.parametrize(
# Postpone evaluation of options so fixture variables can interpolate
"constructor,lazy_constructor_options",
"constructor,lazy_constructor_options,has_untracked_files",
[
[
GitSync,
Expand All @@ -121,6 +121,16 @@ def test_repo_git_obtain_full(
"dir": tmp_path / "myrepo",
"vcs": "git",
},
False,
],
[
GitSync,
lambda git_remote_repo, tmp_path, **kwargs: {
"url": f"file://{git_remote_repo}",
"dir": tmp_path / "myrepo",
"vcs": "git",
},
True,
],
[
create_project,
Expand All @@ -129,6 +139,16 @@ def test_repo_git_obtain_full(
"dir": tmp_path / "myrepo",
"vcs": "git",
},
False,
],
[
create_project,
lambda git_remote_repo, tmp_path, **kwargs: {
"url": f"git+file://{git_remote_repo}",
"dir": tmp_path / "myrepo",
"vcs": "git",
},
True,
],
],
)
Expand All @@ -138,9 +158,16 @@ def test_repo_update_handle_cases(
mocker: MockerFixture,
constructor: ProjectTestFactory,
lazy_constructor_options: ProjectTestFactoryLazyKwargs,
has_untracked_files: bool,
) -> None:
git_repo: GitSync = constructor(**lazy_constructor_options(**locals()))
git_repo.obtain() # clone initial repo

if has_untracked_files:
some_file = git_repo.dir.joinpath("some_file")
with open(some_file, "w") as untracked_file:
untracked_file.write("some content")

mocka = mocker.spy(git_repo, "run")
git_repo.update_repo()

Expand Down