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

[FUJIWARA Katsunori]

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 at 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 at lares.dti.ne.jp