Patchwork [STABLE] record: prevent commits that don't pick up dirty subrepo changes (issue6102)

login
register
mail settings
Submitter Matt Harbison
Date March 16, 2019, 10:20 p.m.
Message ID <0d48bef566bf86ade809.1552774837@Envy>
Download mbox | patch
Permalink /patch/39295/
State Superseded
Headers show

Comments

Matt Harbison - March 16, 2019, 10:20 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1552761621 14400
#      Sat Mar 16 14:40:21 2019 -0400
# Branch stable
# Node ID 0d48bef566bf86ade809225d7e8b08c37914beb4
# Parent  25fc5b96d1c30468417ee0d690c2979db362edd0
record: prevent commits that don't pick up dirty subrepo changes (issue6102)

This path covers interactive mode for commit, amend, and shelve, as well as the
deprecated record extension.  Since shelf creation uses commit without -S in the
non-interactive case, aborting here should be OK.  (I didn't check what happens
to non interactive shelve creation if `ui.commitsubrepos=True` is set.)

subrepoutil.precommit() will abort on a dirty subrepo if the config option isn't
set, but the hint recommends using --subrepos to commit.  Since only the commit
command currently supports that option, the error has to be raised here to omit
the hint.

Doing the check before asking about all of the hunks in the MQ test seems like
an improvement on its own.  There's probably an additional check on this path
that can be removed.
Yuya Nishihara - March 17, 2019, 2:01 a.m.
On Sat, 16 Mar 2019 18:20:37 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1552761621 14400
> #      Sat Mar 16 14:40:21 2019 -0400
> # Branch stable
> # Node ID 0d48bef566bf86ade809225d7e8b08c37914beb4
> # Parent  25fc5b96d1c30468417ee0d690c2979db362edd0
> record: prevent commits that don't pick up dirty subrepo changes (issue6102)

> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -248,6 +248,21 @@ def dorecord(ui, repo, commitfunc, cmdsu
>      if not opts.get('user'):
>          ui.username() # raise exception, username not provided
>  
> +    wctx = repo[None]
> +    match = scmutil.match(wctx, pats, opts)
> +    status = repo.status(match=match)
> +
> +    overrides = {(b'ui', b'commitsubrepos'): True}
> +
> +    with repo.ui.configoverride(overrides, b'record'):
> +        # Force allows -X subrepo.
> +        subs, commitsubs, newstate = subrepoutil.precommit(
> +            repo.ui, wctx, status, match, force=True)
> +        for s in subs:
> +            if s in commitsubs:
> +                dirtyreason = wctx.sub(s).dirtyreason(True)
> +                raise error.Abort(dirtyreason)

Can we move these into recordfunc() where locks should be held properly?

>      def recordfunc(ui, repo, message, match, opts):
>          """This is generic record driver.

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -248,6 +248,21 @@  def dorecord(ui, repo, commitfunc, cmdsu
     if not opts.get('user'):
         ui.username() # raise exception, username not provided
 
+    wctx = repo[None]
+    match = scmutil.match(wctx, pats, opts)
+    status = repo.status(match=match)
+
+    overrides = {(b'ui', b'commitsubrepos'): True}
+
+    with repo.ui.configoverride(overrides, b'record'):
+        # Force allows -X subrepo.
+        subs, commitsubs, newstate = subrepoutil.precommit(
+            repo.ui, wctx, status, match, force=True)
+        for s in subs:
+            if s in commitsubs:
+                dirtyreason = wctx.sub(s).dirtyreason(True)
+                raise error.Abort(dirtyreason)
+
     def recordfunc(ui, repo, message, match, opts):
         """This is generic record driver.
 
diff --git a/tests/test-amend-subrepo.t b/tests/test-amend-subrepo.t
--- a/tests/test-amend-subrepo.t
+++ b/tests/test-amend-subrepo.t
@@ -121,9 +121,22 @@  add new commit to be amended
   $ echo a >> a
   $ hg ci -m3
 
+  $ echo 't = t' > .hgsub
+
+--interactive won't silently ignore dirty subrepos
+
+  $ echo modified > t/b
+  $ hg amend --interactive --config ui.interactive=True
+  abort: uncommitted changes in subrepository "t"
+  [255]
+  $ hg amend --interactive --config ui.interactive=True --config ui.commitsubrepos=True
+  abort: uncommitted changes in subrepository "t"
+  [255]
+
+  $ hg -R t revert -q --all --no-backup
+
 amend with one subrepo dropped
 
-  $ echo 't = t' > .hgsub
   $ hg amend
   saved backup bundle to * (glob) (obsstore-off !)
   $ hg status --change .
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
@@ -295,16 +295,6 @@  handle subrepos safely on qrecord
   A .hgsub
   A sub/a
   % qrecord --config ui.interactive=1 -m0 0.diff
-  diff --git a/.hgsub b/.hgsub
-  new file mode 100644
-  examine changes to '.hgsub'? [Ynesfdaq?] y
-  
-  @@ -0,0 +1,1 @@
-  +sub = sub
-  record this change to '.hgsub'? [Ynesfdaq?] y
-  
-  warning: subrepo spec file '.hgsub' not found
-  warning: subrepo spec file '.hgsub' not found
   abort: uncommitted changes in subrepository "sub"
   [255]
   % update substate when adding .hgsub w/clean updated subrepo
@@ -333,15 +323,6 @@  handle subrepos safely on qrecord
   M .hgsub
   A sub2/a
   % qrecord --config ui.interactive=1 -m1 1.diff
-  diff --git a/.hgsub b/.hgsub
-  1 hunks, 1 lines changed
-  examine changes to '.hgsub'? [Ynesfdaq?] y
-  
-  @@ -1,1 +1,2 @@
-   sub = sub
-  +sub2 = sub2
-  record this change to '.hgsub'? [Ynesfdaq?] y
-  
   abort: uncommitted changes in subrepository "sub2"
   [255]
   % update substate when modifying .hgsub w/clean updated subrepo