Bug 4592 - revert --interactive may leave the dirstate in a corrupted state
Summary: revert --interactive may leave the dirstate in a corrupted state
Status: RESOLVED FIXED
Alias: None
Product: Mercurial
Classification: Unclassified
Component: Mercurial (show other bugs)
Version: default branch
Hardware: PC Linux
: critical bug
Assignee: Bugzilla
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-09 16:06 UTC by Pierre-Yves David
Modified: 2015-05-01 01:00 UTC (History)
4 users (show)

See Also:
Python Version: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Yves David 2015-04-09 16:06 UTC
After a rebase -i I ended up with a file marked as clean while it still had uncommitted changes, I'll try to build a repro soon
Comment 1 Pierre-Yves David 2015-04-09 16:07 UTC
Critical because invalide dirstate lead to data loss
Comment 2 Pierre-Yves David 2015-04-11 03:53 UTC
I can reproduce in a shell, but not in the test suite.

  $ cat >> $HGRCPATH << EOF
  > [ui]
  > interactive=True
  > EOF

  $ hg init issue4592
  $ cd issue4592
  $ echo a > a
  $ hg add a
  $ hg commit -m a
  $ echo e > a
  $ hg st
  M a
  $ hg revert -i a << EOF
  > y
  > n
  > EOF
  diff -r cb9a9f314b8b a
  1 hunks, 1 lines changed
  examine changes to 'a'? [Ynesfdaq?] y
  
  @@ -1,1 +1,1 @@
  -e
  +a
  record this change to 'a'? [Ynesfdaq?] n
  


st and diff are buggy

  $ hg st
  ? a.orig
  $ hg diff

The rest is fine

  $ cat a
  e

Touching file fixes it.

  $ touch a
  $ hg st
  M a
  ? a.orig

There is either timing issue, that my shell hits but not the test, or something
is weird in my interactive setup. Can someone try the same step manual on their
own ends?
Comment 3 Pierre-Yves David 2015-04-11 04:04 UTC
Looking a the code, we appears to be marking everything as "normal" even if we
actually only partially revert them. I'm not sure why the test pass at all.

Blindly falling back to the more prudent "normallookup" fixes it for me. I'll
prepare a proper patch tomorrow.

  diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
  --- a/mercurial/cmdutil.py
  +++ b/mercurial/cmdutil.py
  @@ -3099,10 +3099,13 @@ def _performrevert(repo, parents, ctx, a
               normal = repo.dirstate.normallookup
           else:
               normal = repo.dirstate.normal
   
       if interactive:
  +        normallookup = None
  +        if normal:
  +            normallookup = repo.dirstate.normallookup
           # Prompt the user for changes to revert
           torevert = [repo.wjoin(f) for f in actions['revert'][0]]
           m = scmutil.match(ctx, torevert, {})
           diff = patch.diff(repo, None, ctx.node(), m)
           originalchunks = patch.parsepatch(diff)
  @@ -3123,12 +3126,12 @@ def _performrevert(repo, parents, ctx, a
               except patch.PatchError, err:
                   raise util.Abort(str(err))
           del fp
   
           for f in actions['revert'][0]:
  -            if normal:
  -                normal(f)
  +            if normallookup:
  +                normallookup(f)
   
       else:
           for f in actions['revert'][0]:
               checkout(f)
               if normal:
Comment 4 Martin von Zweigbergk 2015-04-11 12:06 UTC
FYI, I could reproduce the problem on my machine. Since we don't check if all or no chunks of a file are reverted, I agree that normallookup seems right. And it's probably not worth checking for those cases either.
Comment 5 Pierre-Yves David 2015-04-11 12:35 UTC
I've actually dropped all the normal call, This is what happen when reverting againts an arbitrary rev and just lets the status logic do the work.
Comment 6 HG Bot 2015-04-14 14:31 UTC
Fixed by http://selenic.com/repo/hg/rev/ab6e3729747e
Pierre-Yves David <pierre-yves.david@fb.com>
revert: stop marking files clean after interactive revert (issue4592)

The goal of 'hg revert --interactive' is usually to keep some change in the
revert file, so the files -must-not- be marked as clean. We want the status
logic to do its usual job here.

For unclear reasons (probably timing related), I was unable to build an automated
test that reproduced issue4592 but manual testing shows this is fixed.

(please test the fix)
Comment 7 Bugzilla 2015-04-22 01:00 UTC
Bug was set to TESTING for 7 days, resolving
Comment 8 HG Bot 2015-04-23 14:30 UTC
Fixed by http://selenic.com/repo/hg/rev/45bd336e3991
Martin von Zweigbergk <martinvonz@google.com>
revert: accept just -I/-X without paths or -a/-i (issue4592)

'hg revert -I foo' currently fails with

  abort: no files or directories specified
  (use --all to revert all files, or 'hg update 1' to update)

It doesn't seem intentional that -I/-X without other paths or
--all/--interactive should fail, and it doesn't seem that harmful to
allow it either, so let's just do that.

(please test the fix)
Comment 9 Martin von Zweigbergk 2015-04-23 17:27 UTC
Sorry, 45bd336e3991 did not fix this issue, but issue 4618.
Comment 10 Bugzilla 2015-05-01 01:00 UTC
Bug was set to TESTING for 7 days, resolving