This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7521

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

Authored by spectral on Nov 26 2019, 7:29 PM.

Details

Reviewers
mharbison72
pulkit
Group Reviewers
hg-reviewers
Commits
rHG5558e3437872: amend: check for file modifications when updating dirstate (issue6233)
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

spectral created this revision.Nov 26 2019, 7:29 PM
mharbison72 requested changes to this revision.Dec 2 2019, 9:47 AM
mharbison72 added a subscriber: mharbison72.
mharbison72 added inline comments.
tests/test-amend.t
493

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

This revision now requires changes to proceed.Dec 2 2019, 9:47 AM
spectral updated this revision to Diff 18425.Dec 2 2019, 1:40 PM
spectral marked an inline comment as done.Dec 2 2019, 1:43 PM
spectral added inline comments.
tests/test-amend.t
493

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

496

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

mharbison72 accepted this revision.Dec 2 2019, 6:51 PM

Windows is happy with this test

pulkit accepted this revision.Dec 6 2019, 9:00 AM
This revision is now accepted and ready to land.Dec 6 2019, 9:00 AM
pulkit added a comment.Dec 6 2019, 9:55 AM

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
spectral marked an inline comment as done.Dec 6 2019, 10:37 AM
spectral added a commit: rHG5558e3437872: amend: check for file modifications when updating dirstate (issue6233).
Closed by commit rHG5558e3437872: amend: check for file modifications when updating dirstate (issue6233) (authored by spectral). · Explain Why
This revision was automatically updated to reflect the committed changes.
pulkit added a comment.Dec 6 2019, 1:05 PM

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

Revision Contents
Changeset List

PathPackages
Mmercurial/cmdutil.py (8 lines)
Mtests/test-amend.t (30 lines)

Diff 18490

mercurial/cmdutil.py

# Fixing the dirstate because localrepo.commitctx does not update # Fixing the dirstate because localrepo.commitctx does not update
# it. This is rather convenient because we did not need to update # it. This is rather convenient because we did not need to update
# the dirstate for all the files in the new commit which commitctx # the dirstate for all the files in the new commit which commitctx
# could have done if it updated the dirstate. Now, we can # could have done if it updated the dirstate. Now, we can
# selectively update the dirstate only for the amended files. # selectively update the dirstate only for the amended files.
dirstate = repo.dirstate dirstate = repo.dirstate
# Update the state of the files which were added and # Update the state of the files which were added and modified in the
# and modified in the amend to "normal" in the dirstate. # 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 normalfiles = set(wctx.modified() + wctx.added()) & filestoamend
for f in normalfiles: for f in normalfiles:
dirstate.normal(f) dirstate.normallookup(f)
# Update the state of files which were removed in the amend # Update the state of files which were removed in the amend
# to "removed" in the dirstate. # to "removed" in the dirstate.
removedfiles = set(wctx.removed()) & filestoamend removedfiles = set(wctx.removed()) & filestoamend
for f in removedfiles: for f in removedfiles:
dirstate.drop(f) dirstate.drop(f)
return newid return newid

tests/test-amend.t

$ hg diff --stat -c . $ hg diff --stat -c .
a | 2 +- a | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-) 1 files changed, 1 insertions(+), 1 deletions(-)
$ echo a > a; echo b2 > b; hg amend -q b $ echo a > a; echo b2 > b; hg amend -q b
$ hg diff --stat -c . $ hg diff --stat -c .
a | 2 +- a | 2 +-
b | 2 +- b | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-) 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.sh <<EOF
> echo hi > "\$1"
> sleep 3
> EOF
$ HGEDITOR="sh $TESTTMP/sleepy_editor.sh" hg commit --amend &
$ sleep 1
mharbison72Unsubmitted
Done

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

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

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

spectral: Replaced this with what other tests do: call sh directly (example: test-rebase-collapse.t, L37).
$ echo delta >> foo
$ sleep 3
$ if (hg diff -c . | grep 'delta' >/dev/null) || [[ -n "$(hg status)" ]]; then
spectralAuthorUnsubmitted
Done

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

spectral: Also had to actually amend to include this fix, I'd left it in my working directory. Oops.
> 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.