From f7522826094791f199b4a7adafb172739adedbb3 Mon Sep 17 00:00:00 2001 From: Peter Rowlands Date: Thu, 23 Mar 2023 17:26:26 +0900 Subject: [PATCH] stash: handle clean workspace on push() --- pyproject.toml | 1 + src/scmrepo/git/backend/base.py | 2 +- src/scmrepo/git/backend/dulwich/__init__.py | 8 +++++- src/scmrepo/git/backend/gitpython.py | 10 +++++-- src/scmrepo/git/backend/pygit2.py | 16 +++++++---- src/scmrepo/git/stash.py | 7 ++--- tests/test_stash.py | 32 ++++++++++++--------- 7 files changed, 48 insertions(+), 28 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index bee96561..17878a8f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -72,6 +72,7 @@ module = [ "pygtrie", "funcy", "git", + "gitdb.*", "fsspec.*", "pathspec.patterns", "asyncssh.*", diff --git a/src/scmrepo/git/backend/base.py b/src/scmrepo/git/backend/base.py index 41eb00c7..794e0584 100644 --- a/src/scmrepo/git/backend/base.py +++ b/src/scmrepo/git/backend/base.py @@ -261,7 +261,7 @@ def _stash_push( self, ref: str, message: Optional[str] = None, - include_untracked: Optional[bool] = False, + include_untracked: bool = False, ) -> Tuple[Optional[str], bool]: """Push a commit onto the specified stash. diff --git a/src/scmrepo/git/backend/dulwich/__init__.py b/src/scmrepo/git/backend/dulwich/__init__.py index 3a6b45dc..7bb0c9da 100644 --- a/src/scmrepo/git/backend/dulwich/__init__.py +++ b/src/scmrepo/git/backend/dulwich/__init__.py @@ -672,12 +672,18 @@ def _stash_push( self, ref: str, message: Optional[str] = None, - include_untracked: Optional[bool] = False, + include_untracked: bool = False, ) -> Tuple[Optional[str], bool]: from dulwich.repo import InvalidUserIdentity from scmrepo.git import Stash + # dulwich will silently generate an empty stash commit if there is + # nothing to stash, we check status here to get consistent behavior + # across backends + if not self.is_dirty(untracked_files=include_untracked): + return None, False + if include_untracked or ref == Stash.DEFAULT_STASH: # dulwich stash.push does not support include_untracked and does # not touch working tree diff --git a/src/scmrepo/git/backend/gitpython.py b/src/scmrepo/git/backend/gitpython.py index b3a6e8d4..c8ccdaec 100644 --- a/src/scmrepo/git/backend/gitpython.py +++ b/src/scmrepo/git/backend/gitpython.py @@ -359,11 +359,12 @@ def resolve_commit(self, rev: str) -> "GitCommit": """Return Commit object for the specified revision.""" from git.exc import BadName, GitCommandError from git.objects.tag import TagObject + from gitdb.exc import BadObject try: commit = self.repo.rev_parse(rev) - except (BadName, GitCommandError): - raise SCMError(f"Invalid commit '{rev}'") + except (BadName, BadObject, GitCommandError) as exc: + raise SCMError(f"Invalid commit '{rev}'") from exc if isinstance(commit, TagObject): commit = commit.object return GitCommit( @@ -503,10 +504,13 @@ def _stash_push( self, ref: str, message: Optional[str] = None, - include_untracked: Optional[bool] = False, + include_untracked: bool = False, ) -> Tuple[Optional[str], bool]: from scmrepo.git import Stash + if not self.is_dirty(untracked_files=include_untracked): + return None, False + args = ["push"] if message: args.extend(["-m", message]) diff --git a/src/scmrepo/git/backend/pygit2.py b/src/scmrepo/git/backend/pygit2.py index 05b5c0be..c4233d9a 100644 --- a/src/scmrepo/git/backend/pygit2.py +++ b/src/scmrepo/git/backend/pygit2.py @@ -534,15 +534,19 @@ def _stash_push( self, ref: str, message: Optional[str] = None, - include_untracked: Optional[bool] = False, + include_untracked: bool = False, ) -> Tuple[Optional[str], bool]: from scmrepo.git import Stash - oid = self.repo.stash( - self.default_signature, - message=message, - include_untracked=include_untracked, - ) + try: + oid = self.repo.stash( + self.default_signature, + message=message, + include_untracked=include_untracked, + ) + except KeyError: + # GIT_ENOTFOUND, nothing to stash + return None, False commit = self.repo[oid] if ref != Stash.DEFAULT_STASH: diff --git a/src/scmrepo/git/stash.py b/src/scmrepo/git/stash.py index f4ea575b..f352ae2a 100644 --- a/src/scmrepo/git/stash.py +++ b/src/scmrepo/git/stash.py @@ -32,14 +32,13 @@ def list(self): def push( self, message: Optional[str] = None, include_untracked: bool = False ) -> Optional[str]: - if not self.scm.is_dirty(untracked_files=include_untracked): - logger.debug("No changes to stash") - return None - logger.debug("Stashing changes in '%s'", self.ref) rev, reset = self.scm._stash_push( # pylint: disable=protected-access self.ref, message=message, include_untracked=include_untracked ) + if not rev: + logger.debug("No changes to stash") + return None if reset: self.scm.reset(hard=True) return rev diff --git a/tests/test_stash.py b/tests/test_stash.py index b749f4e3..06248bc4 100644 --- a/tests/test_stash.py +++ b/tests/test_stash.py @@ -89,20 +89,13 @@ def test_git_stash_drop(tmp_dir: TmpDir, scm: Git, ref: Optional[str]): https://github.com/iterative/dvc/pull/5286#issuecomment-792574294""" -@pytest.mark.parametrize( - "ref", - [ - pytest.param( - None, - marks=pytest.mark.xfail( - sys.platform in ("linux", "win32"), - raises=AssertionError, - reason=reason, - ), - ), - "refs/foo/stash", - ], +@pytest.mark.xfail( + sys.platform in ("linux", "win32"), + raises=AssertionError, + strict=False, + reason=reason, ) +@pytest.mark.parametrize("ref", [None, "refs/foo/stash"]) def test_git_stash_pop(tmp_dir: TmpDir, scm: Git, ref: Optional[str]): tmp_dir.gen({"file": "0"}) scm.add_commit("file", message="init") @@ -165,3 +158,16 @@ def test_git_stash_apply_index( assert dict(staged) == {"modify": ["file"]} assert not dict(unstaged) assert not dict(untracked) + + +def test_git_stash_push_clean_workspace( + tmp_dir: TmpDir, + scm: Git, + git: Git, +): + tmp_dir.gen("file", "0") + scm.add_commit("file", message="init") + assert git._stash_push("refs/stash") == ( # pylint: disable=protected-access + None, + False, + )