Patchwork D7521: amend: check for file modifications when updating dirstate (issue6233)

login
register
mail settings
Submitter phabricator
Date Nov. 27, 2019, 12:30 a.m.
Message ID <differential-rev-PHID-DREV-agrdvxkm5plliyxgu3og-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43525/
State Superseded
Headers show

Comments

phabricator - Nov. 27, 2019, 12:30 a.m.
spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Previously, we called dirstate.normal(f), which would put information into the
  dirstate claiming that the file on disk is what it "should be" for the current
  checkout, and it would have the size and timestamp of the most recent
  modification to the file (which is not necessarily the one we just committed).
  If the file was modified while the commit message editor was open, we would put
  incorrect information into the dirstate.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  mercurial/cmdutil.py
  tests/test-amend.t

CHANGE DETAILS




To: spectral, #hg-reviewers
Cc: mercurial-devel
phabricator - Dec. 2, 2019, 2:47 p.m.
This revision now requires changes to proceed.
mharbison72 added inline comments.
mharbison72 requested changes to this revision.

INLINE COMMENTS

> test-amend.t:493
> +  > EOF
> +  $ chmod +x $TESTTMP/sleepy_editor
> +  $ HGEDITOR=$TESTTMP/sleepy_editor hg commit --amend &

This whole test needs to be wrapped in `#if exebit` to keep Windows happy.

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - Dec. 2, 2019, 6:43 p.m.
spectral added inline comments.
spectral marked an inline comment as done.

INLINE COMMENTS

> mharbison72 wrote in test-amend.t:493
> This whole test needs to be wrapped in `#if exebit` to keep Windows happy.

Replaced this with what other tests do: call sh directly (example: test-rebase-collapse.t, L37).

> test-amend.t:496
> +  $ sleep 3
> +  $ if (hg diff -c . | grep 'delta' >/dev/null) || [[ -n "$(hg status)" ]]; then
> +  >   echo "OK."

Also had to actually amend to include this fix, I'd left it in my working directory. Oops.

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - Dec. 2, 2019, 11:51 p.m.
mharbison72 added a comment.
mharbison72 accepted this revision.


  Windows is happy with this test

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - Dec. 6, 2019, 2:55 p.m.
pulkit added a comment.


  I get the following test failure:
  
    --- /home/pulkit/repo/hgpush/tests/test-amend.t                                          
    +++ /home/pulkit/repo/hgpush/tests/test-amend.t#obsstore-off.err              
    @@ -505,4 +505,23 @@                                                                                                                                           
       >   hg diff                                                                                                                                                 
       >   hg status                                                                                                                                               
       > fi                                                                                                                                                      
    -  OK.                                                                              
    +  $TESTTMP.sh: 292: $TESTTMP.sh: [[: not found                                                              
    +  Bug detected. 'delta' is not part of the commit OR the wdir                                                       
    +  Diff and status before rebuild:                                                 
    +  diff --git a/foo b/foo                                                       
    +  --- a/foo                                                                       
    +  +++ b/foo                                                                   
    +  @@ -1,2 +1,3 @@                                                                                              
    +   alpha                                                                          
    +   beta                                                                          
    +  +delta                                                                             
    +  M foo                                                                           
    +  Diff and status after rebuild:                                                  
    +  diff --git a/foo b/foo                                                                
    +  --- a/foo                                                                             
    +  +++ b/foo                                                                                                                                                   
    +  @@ -1,2 +1,3 @@                                                                                                                                             
    +   alpha                                                                                                                                                      
    +   beta                                                                                                                                                     
    +  +delta                                                                                                                                                      
    +  M foo         
                                                                                                                         
    ERROR: test-amend.t#obsstore-off output changed                                    
    !.                                                                              
    --- /home/pulkit/repo/hgpush/tests/test-amend.t                                    
    +++ /home/pulkit/repo/hgpush/tests/test-amend.t#obsstore-on.err                
    @@ -505,4 +505,23 @@                                                                                            
       >   hg diff                                                                     
       >   hg status                                                                  
       > fi                                                                               
    -  OK.                                                                             
    +  $TESTTMP.sh: 317: $TESTTMP.sh: [[: not found                                    
    +  Bug detected. 'delta' is not part of the commit OR the wdir                           
    +  Diff and status before rebuild:                                                       
    +  diff --git a/foo b/foo                                                                                                                                      
    +  --- a/foo                                                                                                                                                   
    +  +++ b/foo                                                                                                                                                   
    +  @@ -1,2 +1,3 @@                                                                                                                                           
    +   alpha                                                                                                                                                     
    +   beta                                                                                                     
    +  +delta
    +  M foo
    +  Diff and status after rebuild:
    +  diff --git a/foo b/foo
    +  --- a/foo
    +  +++ b/foo
    +  @@ -1,2 +1,3 @@
    +   alpha
    +   beta
    +  +delta
    +  M foo

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

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

To: spectral, #hg-reviewers, mharbison72, pulkit
Cc: mharbison72, mercurial-devel
phabricator - Dec. 6, 2019, 6:05 p.m.
pulkit added a comment.


  Looks like I forgot to drop this from my queue and pushed it. :(

REPOSITORY
  rHG Mercurial

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

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

To: spectral, #hg-reviewers, mharbison72, pulkit
Cc: mharbison72, mercurial-devel

Patch

diff --git a/tests/test-amend.t b/tests/test-amend.t
--- a/tests/test-amend.t
+++ b/tests/test-amend.t
@@ -476,3 +476,35 @@ 
    a |  2 +-
    b |  2 +-
    2 files changed, 2 insertions(+), 2 deletions(-)
+
+Modifying a file while the editor is open can cause dirstate corruption
+(issue6233)
+
+  $ cd $TESTTMP
+  $ hg init modify-during-amend; cd modify-during-amend
+  $ echo r0 > foo; hg commit -qAm "r0"
+  $ echo alpha > foo; hg commit -qm "alpha"
+  $ echo beta >> foo
+  $ cat > $TESTTMP/sleepy_editor <<EOF
+  > #!/bin/bash
+  > echo hi > "\$1"
+  > sleep 3
+  > EOF
+  $ chmod +x $TESTTMP/sleepy_editor
+  $ HGEDITOR=$TESTTMP/sleepy_editor hg commit --amend &
+  $ sleep 1
+  $ echo delta >> foo
+  $ sleep 3
+  $ if (hg diff -c . | grep -q 'delta') || [[ -n "$(hg status)" ]]; then
+  >   echo "OK."
+  > else
+  >   echo "Bug detected. 'delta' is not part of the commit OR the wdir"
+  >   echo "Diff and status before rebuild:"
+  >   hg diff
+  >   hg status
+  >   hg debugrebuilddirstate
+  >   echo "Diff and status after rebuild:"
+  >   hg diff
+  >   hg status
+  > fi
+  OK.
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -3054,11 +3054,13 @@ 
         # selectively update the dirstate only for the amended files.
         dirstate = repo.dirstate
 
-        # Update the state of the files which were added and
-        # and modified in the amend to "normal" in the dirstate.
+        # Update the state of the files which were added and modified in the
+        # amend to "normal" in the dirstate. We need to use "normallookup" since
+        # the files may have changed since the command started; using "normal"
+        # would mark them as clean but with uncommitted contents.
         normalfiles = set(wctx.modified() + wctx.added()) & filestoamend
         for f in normalfiles:
-            dirstate.normal(f)
+            dirstate.normallookup(f)
 
         # Update the state of files which were removed in the amend
         # to "removed" in the dirstate.