Patchwork status: add dirty() method to status type

login
register
mail settings
Submitter Martin von Zweigbergk
Date Nov. 9, 2015, 11:06 p.m.
Message ID <6c4a3f70880b72538c47.1447110405@waste.org>
Download mbox | patch
Permalink /patch/11342/
State Rejected
Delegated to: Matt Mackall
Headers show

Comments

Martin von Zweigbergk - Nov. 9, 2015, 11:06 p.m.
# HG changeset patch
# User Martin von Zweigbergk <martinvonz@google.com>
# Date 1447109202 28800
#      Mon Nov 09 14:46:42 2015 -0800
# Node ID 6c4a3f70880b72538c475336cb0349f573a9aacf
# Parent  8b2fbe3f59b1b969878691cb472369ad0067f165
status: add dirty() method to status type

We do "s.modified or s.added or s.removed{ or s.deleted}" in quite a
few places, so let's introduce a method on the status type for this
purpose.
Matt Mackall - Nov. 9, 2015, 11:33 p.m.
On Mon, 2015-11-09 at 17:06 -0600, Martin von Zweigbergk wrote:
> # HG changeset patch
> # User Martin von Zweigbergk <martinvonz@google.com>
> # Date 1447109202 28800
> #      Mon Nov 09 14:46:42 2015 -0800
> # Node ID 6c4a3f70880b72538c475336cb0349f573a9aacf
> # Parent  8b2fbe3f59b1b969878691cb472369ad0067f165
> status: add dirty() method to status type
> 
> We do "s.modified or s.added or s.removed{ or s.deleted}" in quite a
> few places,

..but we probably shouldn't, because it usually means we're forgetting
small details like "is the branch name changed" or "are there two
parents of the working directory" or "are the subrepos clean" or "do we
have a topic"[1], etc.? And those questions actually can't be asked of
the status object.

So we should instead have a nice method on wctx that knows about all
the things (and how to ignore some of them) so that we don't have
people open-coding checks. And that's basically what we have already in
wctx.dirty(), but we've apparently got lots of code that needs to be
caught up.

(First on that list is cmdutil.bailifchanged.)

-- 
Mathematics is the supreme nostalgia of our time.
Martin von Zweigbergk - Nov. 10, 2015, 12:07 a.m.
On Mon, Nov 9, 2015 at 3:31 PM Matt Mackall <mpm@selenic.com> wrote:

> On Mon, 2015-11-09 at 17:06 -0600, Martin von Zweigbergk wrote:
> > # HG changeset patch
> > # User Martin von Zweigbergk <martinvonz@google.com>
> > # Date 1447109202 28800
> > #      Mon Nov 09 14:46:42 2015 -0800
> > # Node ID 6c4a3f70880b72538c475336cb0349f573a9aacf
> > # Parent  8b2fbe3f59b1b969878691cb472369ad0067f165
> > status: add dirty() method to status type
> >
> > We do "s.modified or s.added or s.removed{ or s.deleted}" in quite a
> > few places,
>
> ..but we probably shouldn't, because it usually means we're forgetting
> small details like "is the branch name changed" or "are there two
> parents of the working directory" or "are the subrepos clean" or "do we
> have a topic"[1], etc.? And those questions actually can't be asked of
> the status object.
>
> So we should instead have a nice method on wctx that knows about all
> the things (and how to ignore some of them) so that we don't have
> people open-coding checks. And that's basically what we have already in
> wctx.dirty(), but we've apparently got lots of code that needs to be
> caught up.
>

That makes sense, thanks for the explanation.


>
> (First on that list is cmdutil.bailifchanged.)
>
> --
> Mathematics is the supreme nostalgia of our time.
>
>

Patch

diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -971,11 +971,9 @@ 
 
         actobj = actiontable[action].fromrule(state, currentnode)
 
-        s = repo.status()
-        if s.modified or s.added or s.removed or s.deleted:
+        if repo.status().dirty(missing=True):
             actobj.continuedirty()
-            s = repo.status()
-            if s.modified or s.added or s.removed or s.deleted:
+            if repo.status().dirty(missing=True):
                 raise error.Abort(_("working copy still dirty"))
 
         parentctx, replacements = actobj.continueclean()
diff --git a/hgext/keyword.py b/hgext/keyword.py
--- a/hgext/keyword.py
+++ b/hgext/keyword.py
@@ -360,7 +360,7 @@ 
     wlock = repo.wlock()
     try:
         status = _status(ui, repo, wctx, kwt, *pats, **opts)
-        if status.modified or status.added or status.removed or status.deleted:
+        if status.dirty(missing=True):
             raise error.Abort(_('outstanding uncommitted changes'))
         kwt.overwrite(wctx, status.clean, True, expand)
     finally:
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -1044,7 +1044,7 @@ 
     repo.lfstatus = True
     s = repo.status()
     repo.lfstatus = False
-    if s.modified or s.added or s.removed or s.deleted:
+    if s.dirty(missing=True):
         raise error.Abort(_('uncommitted changes'))
 
 def cmdutilforget(orig, ui, repo, match, prefix, explicitonly):
diff --git a/hgext/shelve.py b/hgext/shelve.py
--- a/hgext/shelve.py
+++ b/hgext/shelve.py
@@ -664,8 +664,7 @@ 
         # to the original pctx.
 
         # Store pending changes in a commit
-        s = repo.status()
-        if s.modified or s.added or s.removed or s.deleted:
+        if repo.status().dirty(missing=True):
             ui.status(_("temporarily committing pending changes "
                         "(restore with 'hg unshelve --abort')\n"))
             def commitfunc(ui, repo, message, match, opts):
diff --git a/hgext/strip.py b/hgext/strip.py
--- a/hgext/strip.py
+++ b/hgext/strip.py
@@ -36,7 +36,7 @@ 
     cmdutil.checkunfinished(repo)
     s = repo.status()
     if not force:
-        if s.modified or s.added or s.removed or s.deleted:
+        if s.dirty(missing=True):
             _("local changes found") # i18n tool detection
             raise error.Abort(_("local changes found" + excsuffix))
         if checksubstate(repo):
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -306,8 +306,7 @@ 
 def bailifchanged(repo, merge=True):
     if merge and repo.dirstate.p2() != nullid:
         raise error.Abort(_('outstanding uncommitted merge'))
-    modified, added, removed, deleted = repo.status()[:4]
-    if modified or added or removed or deleted:
+    if repo.status().dirty(missing=True):
         raise error.Abort(_('uncommitted changes'))
     ctx = repo[None]
     for s in sorted(ctx.substate):
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -6232,8 +6232,7 @@ 
     elif (parents[0].closesbranch() and
           pnode in repo.branchheads(branch, closed=True)):
         t += _(' (head closed)')
-    elif not (status.modified or status.added or status.removed or renamed or
-              copied or subs):
+    elif not (status.dirty() or renamed or copied or subs):
         t += _(' (clean)')
         cleanworkdir = True
     elif pnode not in bheads:
diff --git a/mercurial/context.py b/mercurial/context.py
--- a/mercurial/context.py
+++ b/mercurial/context.py
@@ -1372,8 +1372,7 @@ 
         # check current working dir
         return ((merge and self.p2()) or
                 (branch and self.branch() != self.p1().branch()) or
-                self.modified() or self.added() or self.removed() or
-                (missing and self.deleted()))
+                self._status.dirty(missing=missing))
 
     def add(self, list, prefix=""):
         join = lambda f: os.path.join(prefix, f)
diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -69,6 +69,10 @@ 
         '''files that have not been modified'''
         return self[6]
 
+    def dirty(self, missing=False):
+        return bool(self.modified or self.added or self.removed or
+                (missing and self.deleted))
+
     def __repr__(self, *args, **kwargs):
         return (('<status modified=%r, added=%r, removed=%r, deleted=%r, '
                  'unknown=%r, ignored=%r, clean=%r>') % self)