Patchwork D6882: histedit: sniff-test for untracked file conflicts before prompting for rules

login
register
mail settings
Submitter phabricator
Date Sept. 25, 2019, 5:54 p.m.
Message ID <differential-rev-PHID-DREV-oaqkorowkrtezflbna6k-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/41767/
State Superseded
Headers show

Comments

phabricator - Sept. 25, 2019, 5:54 p.m.
durin42 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This bug is as old as histedit, which is more than 10 years! I'm a
  little sad about the extra calculations here that we're just going to
  throw out, but I don't see any better way to look for untracked file
  conflicts and I want the bug fixed.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

AFFECTED FILES
  hgext/histedit.py
  tests/test-histedit-non-commute-abort.t

CHANGE DETAILS




To: durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - Sept. 25, 2019, 6:03 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> histedit.py:1974-1993
> +    wctx = repo[None]
> +    # Please don't ask me why `ancestors` is this value. I figured it
> +    # out with print-debugging, not by actually understanding what the
> +    # merge code is doing. :(
> +    ancs = [repo['.']]
> +    # Sniff-test to make sure we won't collide with untracked files in
> +    # the working directory. If we don't do this, we can get a

Would it be enough to check if any untracked file is in any of those commits? That should be a lot simpler and cheaper.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:10 p.m.
durin42 added inline comments.

INLINE COMMENTS

> martinvonz wrote in histedit.py:1974-1993
> Would it be enough to check if any untracked file is in any of those commits? That should be a lot simpler and cheaper.

Maybe? The tricky bit would be if the user has a gazillion untracked or ignored files, since both of those can come into play.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:24 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> durin42 wrote in histedit.py:1974-1993
> Maybe? The tricky bit would be if the user has a gazillion untracked or ignored files, since both of those can come into play.

Won't they (correctly) cause a conflict with your approach too?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:34 p.m.
durin42 added inline comments.

INLINE COMMENTS

> martinvonz wrote in histedit.py:1974-1993
> Won't they (correctly) cause a conflict with your approach too?

They'll correctly cause a conflict, but I trust the merge code to efficiently figure out what paths to look up, rather than (eg) enumerating a large build/ hierarchy that's wholly ignored.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:39 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> durin42 wrote in histedit.py:1974-1993
> They'll correctly cause a conflict, but I trust the merge code to efficiently figure out what paths to look up, rather than (eg) enumerating a large build/ hierarchy that's wholly ignored.

Ah, now I see what you mean. There are also config options for `merge.checkunknown`, `merge.checkignored`, and `experimental.merge.checkpathconflicts` (and maybe others) that my proposal would miss. So I agree with doing it the way you have.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:39 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> histedit.py:1989
> +                # true?
> +                False, False, False, False)
> +        except error.Abort:

could you pass these as keyword arguments?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 6:43 p.m.
durin42 added inline comments.

INLINE COMMENTS

> martinvonz wrote in histedit.py:1989
> could you pass these as keyword arguments?

Maybe, but I'll do what happens at the other callsite.

I also cleaned up the TODO, as I figured it out with print-debugging.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 9:04 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> histedit.py:1975-1978
> +    # Please don't ask me why `ancestors` is this value. I figured it
> +    # out with print-debugging, not by actually understanding what the
> +    # merge code is doing. :(
> +    ancs = [repo['.']]

Does `None` (the default value for that argument) work? It seems like it should. If it does, then that seems preferable.

> histedit.py:1987-1988
> +                repo, wctx, c, ancs,
> +                # These parameters were determined by print-debugging
> +                # what happens later on inside histedit.
> +                False, # branchmerge

They all seem right to me. I don't think `followcopies` should make a difference for untracked/ignored files.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 9:07 p.m.
durin42 added inline comments.
durin42 marked 2 inline comments as done.

INLINE COMMENTS

> martinvonz wrote in histedit.py:1975-1978
> Does `None` (the default value for that argument) work? It seems like it should. If it does, then that seems preferable.

No, because that gives me the workingctx, and I need p1 of the workingctx. Again, I figured that out by printf debugging powers, it wasn't what I would have guessed (I actually expected `[c]` to be the right argument to `calculateupdates` - but the check didn't do what I needed until I was getting p1 of the workingctx like this. Sigh.)

> martinvonz wrote in histedit.py:1987-1988
> They all seem right to me. I don't think `followcopies` should make a difference for untracked/ignored files.

Agreed. I'm just not wanting to provide a warranty that this makes sense in the future, thus the hedging.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Sept. 25, 2019, 9:21 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> durin42 wrote in histedit.py:1975-1978
> No, because that gives me the workingctx, and I need p1 of the workingctx. Again, I figured that out by printf debugging powers, it wasn't what I would have guessed (I actually expected `[c]` to be the right argument to `calculateupdates` - but the check didn't do what I needed until I was getting p1 of the workingctx like this. Sigh.)

Oh, I confused `calculateupdates()` arguments with `update()` -- `calculateupdates()` does not calculate a default ancestor, so you have to pass it in as you do here.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6882/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6882

To: durin42, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/tests/test-histedit-non-commute-abort.t b/tests/test-histedit-non-commute-abort.t
--- a/tests/test-histedit-non-commute-abort.t
+++ b/tests/test-histedit-non-commute-abort.t
@@ -162,55 +162,26 @@ 
      summary:     a
   
 
-Early tree conflict doesn't leave histedit in a wedged state.
+Early tree conflict doesn't leave histedit in a wedged state. Note
+that we don't specify --commands here: we catch the problem before we
+even prompt the user for rules, sidestepping any dataloss issues.
+
   $ hg rm c
   $ hg ci -m 'remove c'
   $ echo collision > c
 
-  $ hg histedit e860deea161a --commands - 2>&1 <<EOF
-  > edit e860deea161a
-  > pick 652413bf663e
-  > pick bfa474341cc9
-  > pick 1b0954ff00fc
-  > EOF
+  $ hg histedit e860deea161a
   c: untracked file differs
-  abort: untracked files in working directory differ from files in requested revision
+  abort: untracked files in working directory conflict with files in 055a42cdd887
   [255]
 
-BUG: we didn't actually change p1 of the working copy, but we're in a
-histedit state. This confuses the process very badly and leads to
-histedit stripping things it shouldn't (in obsmarker mode it inserts
-bogus prune markers in this case.)
+We should have detected the collision early enough we're not in a
+histedit state, and p1 is unchanged.
 
   $ hg log -r 'p1()' -T'{node}\n'
   1b0954ff00fccb15a37b679e4a35e9b01dfe685e
   $ hg status --config ui.tweakdefaults=yes
   ? c
   ? e.orig
-  # The repository is in an unfinished *histedit* state.
-  
-  # To continue:    hg histedit --continue
-  # To abort:       hg histedit --abort
-  
-  $ hg histedit --continue
-  652413bf663e: skipping changeset (no changes)
-  bfa474341cc9: skipping changeset (no changes)
-  1b0954ff00fc: skipping changeset (no changes)
-  saved backup bundle to $TESTTMP/r/.hg/strip-backup/e860deea161a-a0738322-histedit.hg
-
-  $ hg log -GTcompact
-  warning: ignoring unknown working parent 1b0954ff00fc!
-  o  3[tip]   055a42cdd887   1970-01-01 00:00 +0000   test
-  |    d
-  |
-  o  2   177f92b77385   1970-01-01 00:00 +0000   test
-  |    c
-  |
-  o  1   d2ae7f538514   1970-01-01 00:00 +0000   test
-  |    b
-  |
-  o  0   cb9a9f314b8b   1970-01-01 00:00 +0000   test
-       a
-  
 
   $ cd ..
diff --git a/hgext/histedit.py b/hgext/histedit.py
--- a/hgext/histedit.py
+++ b/hgext/histedit.py
@@ -1970,6 +1970,28 @@ 
                          node.short(root))
 
     ctxs = [repo[r] for r in revs]
+
+    wctx = repo[None]
+    # Please don't ask me why `ancestors` is this value. I figured it
+    # out with print-debugging, not by actually understanding what the
+    # merge code is doing. :(
+    ancs = [repo['.']]
+    # Sniff-test to make sure we won't collide with untracked files in
+    # the working directory. If we don't do this, we can get a
+    # collision after we've started histedit and backing out gets ugly
+    # for everyone, especially the user.
+    for c in [ctxs[0].p1()] + ctxs:
+        try:
+            mergemod.calculateupdates(
+                repo, wctx, c, ancs,
+                # TODO: this last false is followcopies. Should it be
+                # true?
+                False, False, False, False)
+        except error.Abort:
+            raise error.Abort(
+       _("untracked files in working directory conflict with files in %s") % (
+           c))
+
     if not rules:
         comment = geteditcomment(ui, node.short(root), node.short(topmost))
         actions = [pick(state, r) for r in revs]