Patchwork [4,of,4] doc: unify various "uncommitted changes in subrepo" like messages

login
register
mail settings
Submitter Katsunori FUJIWARA
Date March 13, 2015, 3:35 p.m.
Message ID <3efda1632e264ec13ea8.1426260903@feefifofum>
Download mbox | patch
Permalink /patch/8053/
State Changes Requested
Headers show

Comments

Katsunori FUJIWARA - March 13, 2015, 3:35 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1426260651 -32400
#      Sat Mar 14 00:30:51 2015 +0900
# Node ID 3efda1632e264ec13ea80c7f06b91b879a58228f
# Parent  7ee7e04d5fab55bea85c5e41751585720d254f9b
doc: unify various "uncommitted changes in subrepo" like messages

This patch chooses "uncommitted changes in subrepository '%s'" used in
"merge.update()".

This unification reduces cost of translation.
Matt Mackall - March 13, 2015, 10:44 p.m.
On Sat, 2015-03-14 at 00:35 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1426260651 -32400
> #      Sat Mar 14 00:30:51 2015 +0900
> # Node ID 3efda1632e264ec13ea80c7f06b91b879a58228f
> # Parent  7ee7e04d5fab55bea85c5e41751585720d254f9b
> doc: unify various "uncommitted changes in subrepo" like messages

Hrm. I don't think this is right, because we still have multiple
dirty-checking functions that don't agree with largefiles.

I think we should aim to have:

- one place where the abort messages exist
- one smaller function that largefiles can wrap

Here's a strawman proposal:

- add an abort flag to dirty()
- redefine bailifchanged on dirty
- pass flags through
- break the working copy check into a _dirtystatus()
- wrap that in largefiles
- consider nuking bailifchanged

Relatedly, dirty() probably wants to be reordered for speed:

- parents
- branch
- status
- subrepos

..because checking subrepos first is silly.
Katsunori FUJIWARA - March 17, 2015, 1:33 p.m.
At Fri, 13 Mar 2015 17:44:37 -0500,
Matt Mackall wrote:
> 
> On Sat, 2015-03-14 at 00:35 +0900, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1426260651 -32400
> > #      Sat Mar 14 00:30:51 2015 +0900
> > # Node ID 3efda1632e264ec13ea80c7f06b91b879a58228f
> > # Parent  7ee7e04d5fab55bea85c5e41751585720d254f9b
> > doc: unify various "uncommitted changes in subrepo" like messages
> 
> Hrm. I don't think this is right, because we still have multiple
> dirty-checking functions that don't agree with largefiles.
> 
> I think we should aim to have:
> 
> - one place where the abort messages exist
> - one smaller function that largefiles can wrap
> 
> Here's a strawman proposal:
> 
> - add an abort flag to dirty()
> - redefine bailifchanged on dirty
> - pass flags through
> - break the working copy check into a _dirtystatus()
> - wrap that in largefiles
> - consider nuking bailifchanged

In fact, I have pending patches to do similar. But they aren't good
enough around subrepositories :-<

I'll post revised ones.

And please let me confirm the direction of changes:

(1) use specific dirty-checking function instead of "workingctx.dirty()"

  (it is assumed that Matt supposes to centralize all dirty-checking
  into "workingctx.dirty()")

  According to current implementation:

    - "workingctx.dirty()" depends on "workingctx._status"

    - "workingctx._status" may be cached before "workingctx.dirty()"
      (e.g. by "f in wctx" or such manifest accessing)

  For consitency between "dirty()" and others of workingctx,
  "workingctx._status" should be always cooked by largefiles.

  But, on the other hand:

    - result of "repo.status()" depends on "workingctx._status"

    - some code paths expect that "repo.status()" result is RAW
      (= treating standins instead of largefiles, for example)

  So, "workingctx.dirty()" should use independent dirty-checking
  function (like "_dirtystatus()" in Matt's reply) instead of
  examining "self.status" related fields for consistency.

  In addition to it, in many of dirty-checking code paths:

    - "repo.status()" is used instead of "workingctx.dirty()", and
    - there isn't any other reasons to get workingctx

  Then, specific function for dirty-checking seems to be better than
  "repo[None].dirty()" or so at unification of existing dirty-checking.

  How about it ?

  BTW, in my plan, dirty-checking function will be:

    - placed in scmutil.py
      (some dirty-checking code paths can't use cmdutil.py for cyclic problem)

    - named as "workingdirty" or so
      (just "dirty" in scmutil.py may cause confusion about "what is dirty")

(2) separate examination (by "dirty") and aborting (by "bailifchanged")

  For readability, varying "dirty()" and "bailifchanged()" seems to be
  better than passing "abort" flag to "dirty()". How about it ?

    xxxx.bailifchanged(merge=False)

    xxxx.dirty(merge=False, abort=True)

  BTW, I'm planning to make "dirty()" return reason string when
  changes are detected. So, "bailifchanged()" can be kept without
  code duplication and inefficiency.

    def bailifchanged(....):
        reason = dirty(....)
        if reason:
            raise util.Abort(reason)


> Relatedly, dirty() probably wants to be reordered for speed:
> 
> - parents
> - branch
> - status
> - subrepos
> 
> ..because checking subrepos first is silly.

You mean that there is no reasonable reason to "check subrepos first"
even though "workingctx.dirty" has such comment, don't you ?


> -- 
> Mathematics is the supreme nostalgia of our time.
> 
> 
> 

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Matt Mackall - March 17, 2015, 9:12 p.m.
On Tue, 2015-03-17 at 22:33 +0900, FUJIWARA Katsunori wrote:
> At Fri, 13 Mar 2015 17:44:37 -0500,
> Matt Mackall wrote:
> > 
> > On Sat, 2015-03-14 at 00:35 +0900, FUJIWARA Katsunori wrote:
> > > # HG changeset patch
> > > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > > # Date 1426260651 -32400
> > > #      Sat Mar 14 00:30:51 2015 +0900
> > > # Node ID 3efda1632e264ec13ea80c7f06b91b879a58228f
> > > # Parent  7ee7e04d5fab55bea85c5e41751585720d254f9b
> > > doc: unify various "uncommitted changes in subrepo" like messages
> > 
> > Hrm. I don't think this is right, because we still have multiple
> > dirty-checking functions that don't agree with largefiles.
> > 
> > I think we should aim to have:
> > 
> > - one place where the abort messages exist
> > - one smaller function that largefiles can wrap
> > 
> > Here's a strawman proposal:
> > 
> > - add an abort flag to dirty()
> > - redefine bailifchanged on dirty
> > - pass flags through
> > - break the working copy check into a _dirtystatus()
> > - wrap that in largefiles
> > - consider nuking bailifchanged
> 
> In fact, I have pending patches to do similar. But they aren't good
> enough around subrepositories :-<
> 
> I'll post revised ones.
> 
> And please let me confirm the direction of changes:
> 
> (1) use specific dirty-checking function instead of "workingctx.dirty()"
> 
>   (it is assumed that Matt supposes to centralize all dirty-checking
>   into "workingctx.dirty()")
> 
>   According to current implementation:
> 
>     - "workingctx.dirty()" depends on "workingctx._status"
> 
>     - "workingctx._status" may be cached before "workingctx.dirty()"
>       (e.g. by "f in wctx" or such manifest accessing)
> 
>   For consitency between "dirty()" and others of workingctx,
>   "workingctx._status" should be always cooked by largefiles.
> 
>   But, on the other hand:
> 
>     - result of "repo.status()" depends on "workingctx._status"
> 
>     - some code paths expect that "repo.status()" result is RAW
>       (= treating standins instead of largefiles, for example)
> 
>   So, "workingctx.dirty()" should use independent dirty-checking
>   function (like "_dirtystatus()" in Matt's reply) instead of
>   examining "self.status" related fields for consistency.
> 
>   In addition to it, in many of dirty-checking code paths:
> 
>     - "repo.status()" is used instead of "workingctx.dirty()", and
>     - there isn't any other reasons to get workingctx
> 
>   Then, specific function for dirty-checking seems to be better than
>   "repo[None].dirty()" or so at unification of existing dirty-checking.
> 
>   How about it ?

Seems good.

>   BTW, in my plan, dirty-checking function will be:
> 
>     - placed in scmutil.py
>       (some dirty-checking code paths can't use cmdutil.py for cyclic problem)
> 
>     - named as "workingdirty" or so
>       (just "dirty" in scmutil.py may cause confusion about "what is dirty")
> 
> (2) separate examination (by "dirty") and aborting (by "bailifchanged")
> 
>   For readability, varying "dirty()" and "bailifchanged()" seems to be
>   better than passing "abort" flag to "dirty()". How about it ?
> 
>     xxxx.bailifchanged(merge=False)
> 
>     xxxx.dirty(merge=False, abort=True)

Ok.

>   BTW, I'm planning to make "dirty()" return reason string when
>   changes are detected. So, "bailifchanged()" can be kept without
>   code duplication and inefficiency.
> 
>     def bailifchanged(....):
>         reason = dirty(....)
>         if reason:
>             raise util.Abort(reason)
> 
> 
> > Relatedly, dirty() probably wants to be reordered for speed:
> > 
> > - parents
> > - branch
> > - status
> > - subrepos
> > 
> > ..because checking subrepos first is silly.
> 
> You mean that there is no reasonable reason to "check subrepos first"
> even though "workingctx.dirty" has such comment, don't you ?

The "reason" appears to be "programmer too lazy to refactor existing
return statement".

Patch

diff --git a/hgext/strip.py b/hgext/strip.py
--- a/hgext/strip.py
+++ b/hgext/strip.py
@@ -25,7 +25,7 @@ 
     for s in sorted(wctx.substate):
         if wctx.sub(s).dirty(True):
             raise util.Abort(
-                _("uncommitted changes in subrepository %s") % s)
+                _("uncommitted changes in subrepository '%s'") % s)
         elif s not in bctx.substate or bctx.sub(s).dirty():
             inclsubs.append(s)
     return inclsubs
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -262,7 +262,8 @@ 
     ctx = repo[None]
     for s in sorted(ctx.substate):
         if ctx.sub(s).dirty():
-            raise util.Abort(_("uncommitted changes in subrepo %s") % s)
+            raise util.Abort(_("uncommitted changes in subrepository '%s'")
+                             % s)
 
 def logmessage(ui, opts):
     """ get the log message according to -m and -l option """
diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1327,7 +1327,8 @@ 
                     if wctx.sub(s).dirty(True):
                         if not self.ui.configbool('ui', 'commitsubrepos'):
                             raise util.Abort(
-                                _("uncommitted changes in subrepo %s") % s,
+                                _("uncommitted changes in subrepository '%s'")
+                                % s,
                                 hint=_("use --subrepos for recursive commit"))
                         subs.append(s)
                         commitsubs.add(s)
diff --git a/tests/test-largefiles-misc.t b/tests/test-largefiles-misc.t
--- a/tests/test-largefiles-misc.t
+++ b/tests/test-largefiles-misc.t
@@ -248,7 +248,7 @@ 
   commit: 1 subrepos
   update: (current)
   $ hg ci -m "this commit should fail without -S"
-  abort: uncommitted changes in subrepo subrepo
+  abort: uncommitted changes in subrepository 'subrepo'
   (use --subrepos for recursive commit)
   [255]
 
diff --git a/tests/test-mq-subrepo-svn.t b/tests/test-mq-subrepo-svn.t
--- a/tests/test-mq-subrepo-svn.t
+++ b/tests/test-mq-subrepo-svn.t
@@ -50,7 +50,7 @@ 
   $ cd ..
   $ hg status -S        # doesn't show status for svn subrepos (yet)
   $ hg qnew -m1 1.diff
-  abort: uncommitted changes in subrepository sub
+  abort: uncommitted changes in subrepository 'sub'
   [255]
 
   $ cd ..
diff --git a/tests/test-mq-subrepo.t b/tests/test-mq-subrepo.t
--- a/tests/test-mq-subrepo.t
+++ b/tests/test-mq-subrepo.t
@@ -102,7 +102,7 @@ 
   A .hgsub
   A sub/a
   % qnew -X path:no-effect -m0 0.diff
-  abort: uncommitted changes in subrepository sub
+  abort: uncommitted changes in subrepository 'sub'
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
   A .hgsub
@@ -117,7 +117,7 @@ 
   M .hgsub
   A sub2/a
   % qnew --cwd .. -R repo-2499-qnew -X path:no-effect -m1 1.diff
-  abort: uncommitted changes in subrepository sub2
+  abort: uncommitted changes in subrepository 'sub2'
   [255]
   % update substate when modifying .hgsub w/clean updated subrepo
   M .hgsub
@@ -161,7 +161,7 @@ 
   A .hgsub
   A sub/a
   % qrefresh
-  abort: uncommitted changes in subrepository sub
+  abort: uncommitted changes in subrepository 'sub'
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
   A .hgsub
@@ -177,7 +177,7 @@ 
   M .hgsub
   A sub2/a
   % qrefresh
-  abort: uncommitted changes in subrepository sub2
+  abort: uncommitted changes in subrepository 'sub2'
   [255]
   % update substate when modifying .hgsub w/clean updated subrepo
   M .hgsub
@@ -300,7 +300,7 @@ 
   record this change to '.hgsub'? [Ynesfdaq?] y
   
   warning: subrepo spec file .hgsub not found
-  abort: uncommitted changes in subrepository sub
+  abort: uncommitted changes in subrepository 'sub'
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
   A .hgsub
@@ -335,7 +335,7 @@ 
   +sub2 = sub2
   record this change to '.hgsub'? [Ynesfdaq?] y
   
-  abort: uncommitted changes in subrepository sub2
+  abort: uncommitted changes in subrepository 'sub2'
   [255]
   % update substate when modifying .hgsub w/clean updated subrepo
   M .hgsub
diff --git a/tests/test-subrepo-recursion.t b/tests/test-subrepo-recursion.t
--- a/tests/test-subrepo-recursion.t
+++ b/tests/test-subrepo-recursion.t
@@ -59,7 +59,7 @@ 
 Commits:
 
   $ hg commit -m fails
-  abort: uncommitted changes in subrepo foo
+  abort: uncommitted changes in subrepository 'foo'
   (use --subrepos for recursive commit)
   [255]
 
diff --git a/tests/test-subrepo.t b/tests/test-subrepo.t
--- a/tests/test-subrepo.t
+++ b/tests/test-subrepo.t
@@ -91,7 +91,7 @@ 
 
   $ echo b >> s/a
   $ hg backout tip
-  abort: uncommitted changes in subrepo s
+  abort: uncommitted changes in subrepository 's'
   [255]
   $ hg revert -C -R s s/a
 
@@ -141,7 +141,7 @@ 
 
   $ echo c > s/a
   $ hg --config ui.commitsubrepos=no ci -m4
-  abort: uncommitted changes in subrepo s
+  abort: uncommitted changes in subrepository 's'
   (use --subrepos for recursive commit)
   [255]
   $ hg id
diff --git a/tests/test-update-branches.t b/tests/test-update-branches.t
--- a/tests/test-update-branches.t
+++ b/tests/test-update-branches.t
@@ -161,7 +161,7 @@ 
   M foo
 
   $ revtest '-c dirtysub linear'   dirtysub 1 2 -c
-  abort: uncommitted changes in subrepo sub
+  abort: uncommitted changes in subrepository 'sub'
   parent=1
   M sub/suba