Patchwork [1,of,2,STABLE] qnew: omit meaningless and harmful putting subrepositories into target list

login
register
mail settings
Submitter Katsunori FUJIWARA
Date March 19, 2014, 3:57 p.m.
Message ID <d5f18cd442838281ab0c.1395244623@feefifofum>
Download mbox | patch
Permalink /patch/3990/
State Accepted
Commit 7f7c8ef31c5d280cc0f0ecc823db675d3c6e60ee
Headers show

Comments

Katsunori FUJIWARA - March 19, 2014, 3:57 p.m.
# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1395241845 -32400
#      Thu Mar 20 00:10:45 2014 +0900
# Branch stable
# Node ID d5f18cd442838281ab0c4c052d691d60fc519813
# Parent  03774a2b6991b451bde7095238fde9ce98380d28
qnew: omit meaningless and harmful putting subrepositories into target list

Before this patch, qnew puts updated subrepositories into target list
forcibly, if any of -I, -X or patterns are specified.

But this is meaningless and harmful, because:

  - putting subrepositories into target list doesn't affect the result
    of "localrepository.status()"

    "dirstate.status()" invoked via "localrepository.status()" always
    omits subrepositories from the result of it

  - any -I/-X opts and empty "pats" causes unexpected failure

    when any -I/-X opts are specified, "inclsubs" are always added to
    "pats", even if "pats" is empty.

    but this changes meaning of "pats" from "including all to be
    included" to "including only listed subrepositories"

    this may exclude ".hgsub" and cause unexpected exception raising
    ("can't commit subrepos without .hgsub" ).

  - qnew at other than repository root (with -I, -X or any patterns)
    causes unexpected failure

    "scmutil.match()" treats pattern without syntax type as 'relpath'
    type (= one rooted at cwd).

    but qnew puts subrepository paths rooted at the repository root,
    and it causes unexpected exception raising ("SUBREPO not under
    root ROOT" in "pathutil.canonpath()"), if "hg qnew" is executed at
    other than repository root with -I, -X or any patterns.

This patch omits meaningless and harmful putting subrepositories into
target list.

This omitting doesn't miss including updated subrepositories, because
subrepositories are specified to "scmutil.matchfiles()" directly, to
get "match" object for "localrepository.commit()".
Katsunori FUJIWARA - March 20, 2014, 2:42 a.m.
At Wed, 19 Mar 2014 16:02:45 -0500,
Kevin Bullock wrote:
> 
> On Mar 19, 2014, at 10:57 AM, FUJIWARA Katsunori <foozy@lares.dti.ne.jp> wrote:
> 
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> > # Date 1395241845 -32400
> > #      Thu Mar 20 00:10:45 2014 +0900
> > # Branch stable
> > # Node ID d5f18cd442838281ab0c4c052d691d60fc519813
> > # Parent  03774a2b6991b451bde7095238fde9ce98380d28
> > qnew: omit meaningless and harmful putting subrepositories into target list
> 
> Yup, nice analysis. Crewed these, thanks.

Thanks, but can I withdraw #2 (the patch to exclude ".hgsubstate") of
this series ? After posting, I noticed that I should check (sorry,
still not yet) also for "hg commit"/"hg commit --amend" route, and fix
this in another way if needed.

----------------------------------------------------------------------
[FUJIWARA Katsunori]                             foozy@lares.dti.ne.jp
Katsunori FUJIWARA - March 20, 2014, 3:26 a.m.
On Thu, 20 Mar 2014 11:42:44 +0900, FUJIWARA Katsunori <foozy@lares.dti.ne.jp> wrote:
> At Wed, 19 Mar 2014 16:02:45 -0500,
> Kevin Bullock wrote:
>> 
>> On Mar 19, 2014, at 10:57 AM, FUJIWARA Katsunori <foozy@lares.dti.ne.jp> wrote:
>> 
>> > # HG changeset patch
>> > # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
>> > # Date 1395241845 -32400
>> > #      Thu Mar 20 00:10:45 2014 +0900
>> > # Branch stable
>> > # Node ID d5f18cd442838281ab0c4c052d691d60fc519813
>> > # Parent  03774a2b6991b451bde7095238fde9ce98380d28
>> > qnew: omit meaningless and harmful putting subrepositories into target list
>> 
>> Yup, nice analysis. Crewed these, thanks.
> 
> Thanks, but can I withdraw #2 (the patch to exclude ".hgsubstate") of
> this series ? After posting, I noticed that I should check (sorry,
> still not yet) also for "hg commit"/"hg commit --amend" route, and fix
> this in another way if needed.

I confirmed both that patch was imported into crew repo (thanks!) and 
that "hg commit" has also same inconsistent hash value problem.


I'll send the path to fix "hg commit" problem, too !

Patch

diff --git a/hgext/mq.py b/hgext/mq.py
--- a/hgext/mq.py
+++ b/hgext/mq.py
@@ -1038,8 +1038,6 @@ 
             inclsubs.append('.hgsubstate')
             substatestate = repo.dirstate['.hgsubstate']
         if opts.get('include') or opts.get('exclude') or pats:
-            if inclsubs:
-                pats = list(pats or []) + inclsubs
             match = scmutil.match(repo[None], pats, opts)
             # detect missing files in pats
             def badfn(f, msg):
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
@@ -94,32 +94,32 @@ 
 handle subrepos safely on qnew
 
   $ mkrepo repo-2499-qnew
-  $ testadd qnew -m0 0.diff
+  $ testadd qnew -X path:no-effect -m0 0.diff
   adding a
   % abort when adding .hgsub w/dirty subrepo
   A .hgsub
   A sub/a
-  % qnew -m0 0.diff
+  % qnew -X path:no-effect -m0 0.diff
   abort: uncommitted changes in subrepository sub
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
   A .hgsub
-  % qnew -m0 0.diff
+  % qnew -X path:no-effect -m0 0.diff
   path sub
    source   sub
    revision b2fdb12cd82b021c3b7053d67802e77b6eeaee31
 
-  $ testmod qnew -m1 1.diff
+  $ testmod qnew --cwd .. -R repo-2499-qnew -X path:no-effect -m1 1.diff
   adding a
   % abort when modifying .hgsub w/dirty subrepo
   M .hgsub
   A sub2/a
-  % qnew -m1 1.diff
+  % qnew --cwd .. -R repo-2499-qnew -X path:no-effect -m1 1.diff
   abort: uncommitted changes in subrepository sub2
   [255]
   % update substate when modifying .hgsub w/clean updated subrepo
   M .hgsub
-  % qnew -m1 1.diff
+  % qnew --cwd .. -R repo-2499-qnew -X path:no-effect -m1 1.diff
   path sub
    source   sub
    revision b2fdb12cd82b021c3b7053d67802e77b6eeaee31