commit: use `dirstate.change_files` to scope the associated `addremove`

This was significantly more complicated than I expected, because multiple
extensions get in the way.

I introduced a context that lazily open the transaction and associated context
to work around these complication. See the inline documentation for details.

Introducing the wrapping transaction remove the need for dirstate-guard (one of
the ultimate goal of all this), and slightly affect the result of a `hg
rollback` after a `hg commit --addremove`. That last part is deemed fine. It
aligns the behavior with what happens after a failed `hg commit --addremove` and
nobody should be using `hg rollback` anyway.

The small output change in the test come from the different transaction timing
and fact the transaction now backup the dirstate before the addremove, which
might mean "no file to backup" when the repository starts from an empty state.

hgext/largefiles/overrides.py

@@ -1545,11 +1545,19 b' def overridesummary(orig, ui, repo, *pat'
 
 
 @eh.wrapfunction(scmutil, b'addremove')
-def scmutiladdremove(orig, repo, matcher, prefix, uipathfn, opts=None):
+def scmutiladdremove(
+    orig,
+    repo,
+    matcher,
+    prefix,
+    uipathfn,
+    opts=None,
+    open_tr=None,
+):
     if opts is None:
         opts = {}
     if not lfutil.islfilesrepo(repo):
-        return orig(repo, matcher, prefix, uipathfn, opts)
+        return orig(repo, matcher, prefix, uipathfn, opts, open_tr=open_tr)
     # Get the list of missing largefiles so we can remove them
     lfdirstate = lfutil.openlfdirstate(repo.ui, repo)
     unsure, s, mtime_boundary = lfdirstate.status(
@@ -1560,6 +1568,10 b' def scmutiladdremove(orig, repo, matcher'
         unknown=False,
     )
 
+    # open the transaction and changing_files context
+    if open_tr is not None:
+        open_tr()
+
     # Call into the normal remove code, but the removing of the standin, we want
     # to have handled by original addremove.  Monkey patching here makes sure
     # we don't remove the standin in the largefiles code, preventing a very
@@ -1592,7 +1604,8 b' def scmutiladdremove(orig, repo, matcher'
     # function to take care of the rest.  Make sure it doesn't do anything with
     # largefiles by passing a matcher that will ignore them.
     matcher = composenormalfilematcher(matcher, repo[None].manifest(), added)
-    return orig(repo, matcher, prefix, uipathfn, opts)
+
+    return orig(repo, matcher, prefix, uipathfn, opts, open_tr=open_tr)
 
 
 # Calling purge with --all will cause the largefiles to be deleted.

mercurial/cmdutil.py

@@ -29,7 +29,6 b' from . import ('
     changelog,
     copies,
     crecord as crecordmod,
-    dirstateguard,
     encoding,
     error,
     formatter,
@@ -2789,21 +2788,114 b' def cat(ui, repo, ctx, matcher, basefm, '
     return err
 
 
+class _AddRemoveContext:
+    """a small (hacky) context to deal with lazy opening of context
+
+    This is to be used in the `commit` function right below. This deals with
+    lazily open a `changing_files` context inside a `transaction` that span the
+    full commit operation.
+
+    We need :
+    - a `changing_files` context to wrap the dirstate change within the
+      "addremove" operation,
+    - a transaction to make sure these change are not written right after the
+      addremove, but when the commit operation succeed.
+
+    However it get complicated because:
+    - opening a transaction "this early" shuffle hooks order, especially the
+      `precommit` one happening after the `pretxtopen` one which I am not too
+      enthusiastic about.
+    - the `mq` extensions + the `record` extension stacks many layers of call
+      to implement `qrefresh --interactive` and this result with `mq` calling a
+      `strip` in the middle of this function. Which prevent the existence of
+      transaction wrapping all of its function code. (however, `qrefresh` never
+      call the `addremove` bits.
+    - the largefile extensions (and maybe other extensions?) wraps `addremove`
+      so slicing `addremove` in smaller bits is a complex endeavour.
+
+    So I eventually took a this shortcut that open the transaction if we
+    actually needs it, not disturbing much of the rest of the code.
+
+    It will result in some hooks order change for `hg commit --addremove`,
+    however it seems a corner case enough to ignore that for now (hopefully).
+
+    Notes that None of the above problems seems insurmountable, however I have
+    been fighting with this specific piece of code for a couple of day already
+    and I need a solution to keep moving forward on the bigger work around
+    `changing_files` context that is being introduced at the same time as this
+    hack.
+
+    Each problem seems to have a solution:
+    - the hook order issue could be solved by refactoring the many-layer stack
+      that currently composes a commit and calling them earlier,
+    - the mq issue could be solved by refactoring `mq` so that the final strip
+      is done after transaction closure. Be warned that the mq code is quite
+      antic however.
+    - large-file could be reworked in parallel of the `addremove` to be
+      friendlier to this.
+
+    However each of these tasks are too much a diversion right now. In addition
+    they will be much easier to undertake when the `changing_files` dust has
+    settled."""
+
+    def __init__(self, repo):
+        self._repo = repo
+        self._transaction = None
+        self._dirstate_context = None
+        self._state = None
+
+    def __enter__(self):
+        assert self._state is None
+        self._state = True
+        return self
+
+    def open_transaction(self):
+        """open a `transaction` and `changing_files` context
+
+        Call this when you know that change to the dirstate will be needed and
+        we need to open the transaction early
+
+        This will also open the dirstate `changing_files` context, so you should
+        call `close_dirstate_context` when the distate changes are done.
+        """
+        assert self._state is not None
+        if self._transaction is None:
+            self._transaction = self._repo.transaction(b'commit')
+            self._transaction.__enter__()
+        if self._dirstate_context is None:
+            self._dirstate_context = self._repo.dirstate.changing_files(
+                self._repo
+            )
+            self._dirstate_context.__enter__()
+
+    def close_dirstate_context(self):
+        """close the change_files if any
+
+        Call this after the (potential) `open_transaction` call to close the
+        (potential) changing_files context.
+        """
+        if self._dirstate_context is not None:
+            self._dirstate_context.__exit__(None, None, None)
+            self._dirstate_context = None
+
+    def __exit__(self, *args):
+        if self._dirstate_context is not None:
+            self._dirstate_context.__exit__(*args)
+        if self._transaction is not None:
+            self._transaction.__exit__(*args)
+
+
 def commit(ui, repo, commitfunc, pats, opts):
     '''commit the specified files or all outstanding changes'''
     date = opts.get(b'date')
     if date:
         opts[b'date'] = dateutil.parsedate(date)
 
-    dsguard = None
-    # extract addremove carefully -- this function can be called from a command
-    # that doesn't support addremove
-    if opts.get(b'addremove'):
-        dsguard = dirstateguard.dirstateguard(repo, b'commit')
-    with dsguard or util.nullcontextmanager():
+    with repo.wlock(), repo.lock():
         message = logmessage(ui, opts)
         matcher = scmutil.match(repo[None], pats, opts)
-        if True:
+
+        with _AddRemoveContext(repo) as c:
             # extract addremove carefully -- this function can be called from a
             # command that doesn't support addremove
             if opts.get(b'addremove'):
@@ -2818,11 +2910,12 b' def commit(ui, repo, commitfunc, pats, o'
                     b"",
                     uipathfn,
                     opts,
+                    open_tr=c.open_transaction,
                 )
                 m = _(b"failed to mark all new/missing files as added/removed")
                 if r != 0:
                     raise error.Abort(m)
-
+            c.close_dirstate_context()
             return commitfunc(ui, repo, message, matcher, opts)
 
 

mercurial/scmutil.py

@@ -1219,7 +1219,7 b' def cleanupnodes('
                 )
 
 
-def addremove(repo, matcher, prefix, uipathfn, opts=None):
+def addremove(repo, matcher, prefix, uipathfn, opts=None, open_tr=None):
     if opts is None:
         opts = {}
     m = matcher
@@ -1279,7 +1279,9 b' def addremove(repo, matcher, prefix, uip'
         repo, m, added + unknown, removed + deleted, similarity, uipathfn
     )
 
-    if not dry_run:
+    if not dry_run and (unknown or forgotten or deleted or renames):
+        if open_tr is not None:
+            open_tr()
         _markchanges(repo, unknown + forgotten, deleted, renames)
 
     for f in rejected:

tests/test-fncache.t

@@ -103,12 +103,10 b' Non store repo:'
   .hg/phaseroots
   .hg/requires
   .hg/undo
-  .hg/undo.backup.dirstate
   .hg/undo.backupfiles
   .hg/undo.bookmarks
   .hg/undo.branch
   .hg/undo.desc
-  .hg/undo.dirstate
   .hg/undo.phaseroots
   .hg/wcache
   .hg/wcache/checkisexec (execbit !)
@@ -147,11 +145,9 b' Non fncache repo:'
   .hg/store/undo
   .hg/store/undo.backupfiles
   .hg/store/undo.phaseroots
-  .hg/undo.backup.dirstate
   .hg/undo.bookmarks
   .hg/undo.branch
   .hg/undo.desc
-  .hg/undo.dirstate
   .hg/wcache
   .hg/wcache/checkisexec (execbit !)
   .hg/wcache/checklink (symlink !)

tests/test-inherit-mode.t

@@ -95,11 +95,9 b' new directories are setgid'
   00660 ./.hg/store/undo
   00660 ./.hg/store/undo.backupfiles
   00660 ./.hg/store/undo.phaseroots
-  00660 ./.hg/undo.backup.dirstate
   00660 ./.hg/undo.bookmarks
   00660 ./.hg/undo.branch
   00660 ./.hg/undo.desc
-  00660 ./.hg/undo.dirstate
   00770 ./.hg/wcache/
   00711 ./.hg/wcache/checkisexec
   007.. ./.hg/wcache/checklink (re)

tests/test-racy-mutations.t

@@ -106,10 +106,10 b' happen for the changelog (the linkrev sh'
 
 #if fail-if-detected
   $ cat .foo_commit_out
+  note: commit message saved in .hg/last-message.txt
+  note: use 'hg commit --logfile .hg/last-message.txt --edit' to reuse it
   transaction abort!
   rollback completed
-  note: commit message saved in .hg/last-message.txt
-  note: use 'hg commit --logfile .hg/last-message.txt --edit' to reuse it
   abort: 00changelog.i: file cursor at position 249, expected 121
 And no corruption in the changelog.
   $ hg debugrevlogindex -c