<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 8, 2015 at 1:15 AM FUJIWARA Katsunori <<a href="mailto:foozy@lares.dti.ne.jp">foozy@lares.dti.ne.jp</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"># HG changeset patch<br>
# User FUJIWARA Katsunori <<a href="mailto:foozy@lares.dti.ne.jp" target="_blank">foozy@lares.dti.ne.jp</a>><br>
# Date 1436342865 -32400<br>
#      Wed Jul 08 17:07:45 2015 +0900<br>
# Node ID 17ca03748ee914f9696dcec623bc2bc3977d8c38<br>
# Parent  a3f47d16a56c9c64f45b928749de85386b8a5dc9<br>
cmdutil: apply dirstate.normallookup on (maybe partially) committed files<br>
<br>
To detect change of a file without redundant comparison of file<br>
content, dirstate recognizes a file as certainly clean, if:<br>
<br>
  (1) it is already known as "normal",<br>
  (2) dirstate entry for it has valid (= not "-1") timestamp, and<br>
  (3) mode, size and timestamp of it on the filesystem are as same as<br>
      ones expected in dirstate<br>
<br>
This works as expected in many cases, but doesn't in the corner case<br>
that changing a file keeps mode, size and timestamp of it on the<br>
filesystem.<br>
<br>
The timetable below shows steps in one of typical such situations:<br>
<br>
  ---- ----------------------------------- ----------------<br>
                                           timestamp of "f"<br>
                                           ----------------<br>
                                           dirstate   file-<br>
  time          action                     mem  file  system<br>
  ---- ----------------------------------- ---- ----- -----<br>
  N                                              ***   ***<br>
       - change "f"                                     N<br>
<br>
       - execute `hg commit -i`<br>
         - backup "f" with timestamp N<br>
         - revert "f" by `merge.update()`               N<br>
           with `partially`<br>
         - apply selected hunks                         N<br>
           by `patch.patch()`<br>
<br>
         - `repo.commit()`<br>
           - `dirstate.normal("f")`         N<br>
  N+1<br>
           - `dirstate.write()`             N    N<br>
<br>
         - restore "f"                                  N+1<br>
         - restore timestamp of "f"                     N<br>
<br>
       - `hg status` shows "f" as "clean"   N    N      N<br>
  ---- ----------------------------------- ---- ----- -----<br>
<br>
The most important point is that `dirstate.write()` is executed at N+1<br>
or later. This causes writing dirstate timestamp N of "f" out<br>
successfully. If it is executed at N, `parsers.pack_dirstate()`<br>
replaces timestamp N with "-1" before actual writing dirstate out.<br>
<br>
This issue can occur when `hg commit -i` satisfies conditions below:<br>
<br>
  - the file is committed partially, and<br>
  - mode and size of the file aren't changed before and after committing<br>
<br>
The root cause of this issue is that (maybe partially changed) files<br>
are restored with original timestamp but dirstate isn't updated for<br>
them.<br>
<br>
To detect changes of files correctly, this patch applies<br>
`dirstate.normallookup()` on restored files. Status check is needed<br>
before `dirstate.normallookup()`, because status other than "n(ormal)"<br>
should be kept at failure of committing.<br>
<br>
This patch doesn't examine whether each files are committed fully or<br>
partially, because interactive hunk selection makes it difficult.<br>
<br>
After this change, timetable is changed as below:<br>
<br>
  ---- ----------------------------------- ----------------<br>
                                           timestamp of "f"<br>
                                           ----------------<br>
                                           dirstate   file-<br>
  time          action                     mem  file  system<br>
  ---- ----------------------------------- ---- ----- -----<br>
  N                                              ***   ***<br>
       - change "f"                                     N<br>
<br>
       - execute `hg commit -i`<br>
         - backup "f" with timestamp N<br>
         - revert "f" by `merge.update()`               N<br>
           with `partially`<br>
         - apply selected hunks                         N<br>
           by `patch.internalpatch()`<br>
<br>
         - `repo.commit()`<br>
           - `dirstate.normal("f")`         N<br>
  N+1<br>
           - `dirstate.write()`             N    N<br>
<br>
         - restore "f"                                  N+1<br>
         - restore timestamp of "f"                     N<br>
       ----------------------------------- ---- ----- -----<br>
         - normallookup("f")                -1<br>
         - release wlock<br>
           - `dirstate.write()`             -1   -1     N<br>
       ----------------------------------- ---- ----- -----<br>
<br>
       - `hg status` shows "f" as "clean"   -1   -1     N<br>
  ---- ----------------------------------- ---- ----- -----<br>
<br>
To reproduce this issue in tests certainly, this patch emulates some<br>
timing critical actions as below:<br>
<br>
  - change "f" at N<br>
<br>
    `touch -t 200001010000` before command invocation changes mtime of<br>
    "f" to "2000-01-01 00:00" (= N).<br>
<br>
  - apply selected hunks at N<br>
<br>
    `patch.internalpatch()` with `fakepatchtime.py` explicitly changes<br>
    mtime of patched files to "2000-01-01 00:00" (= N).<br>
<br>
  - `dirstate.write()` at N+1 (or "not at N")<br>
<br>
    `pack_dirstate()` uses actual timestamp at runtime as "now", and<br>
    it should be different from the "2000-01-01 00:00" of "f".<br>
<br>
BTW, in `test-commit-interactive.t`, files are sometimes treated as<br>
modified , even though they are just committed fully via `hg commit<br>
-i` and `hg diff` shows nothing for them.<br>
<br>
Enabling win32text causes EOL style mismatching below:<br>
<br>
  - files are changed in LF style EOL<br>
<br>
    => files restored after committing uses LF style EOL (1)<br>
<br>
  - `merge.update()` reverts files in CRLF style EOL<br>
  - `patch.internalpatch()` changes files in CRLF style EOL<br>
<br>
    => `dirstate.normal()` via `repo.commit()` uses the size of files<br>
       in CRLF style EOL (2)<br>
<br>
Therefore, fully committed files are treated as "modified", because<br>
`lstat()` returns size of (1) restored files in LF style EOL, but<br>
dirstate expects size of (2) committed files in CRLF style EOL.<br>
<br>
After this patch, `dirstate.normallookup()` on committed files forces<br>
subsequent `hg status` to examine changes exactly, and fully committed<br>
files are treated as clean as expected.<br>
<br>
This is reason why this patch also does:<br>
<br>
  - add some `hg status` checking status of fully committed files<br>
  - clear win32text configuration before size/timestamp sensitive examination<br>
<br>
diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py<br>
--- a/mercurial/cmdutil.py<br>
+++ b/mercurial/cmdutil.py<br>
@@ -203,8 +203,16 @@<br>
         finally:<br>
             # 5. finally restore backed-up files<br>
             try:<br>
+                dirstate = repo.dirstate<br>
                 for realname, tmpname in backups.iteritems():<br>
                     ui.debug('restoring %r to %r\n' % (tmpname, realname))<br>
+<br>
+                    if dirstate[realname] == 'n':<br>
+                        # without normallookup, restoring timestamp<br>
+                        # may cause that partially committed files<br>
+                        # aren't treated as modified<br>
+                        dirstate.normallookup(realname)<br>
+<br></blockquote><div><br></div><div>Should this be done after writing the file as we do elsewhere? Maybe it doesn't matter since the file will get an old timestamp anyway (because of shutil.copystat)?</div><div><br></div><div>Besides that question, this series looks good to me. I haven't reviewed all of it, but I like the new direction of it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
                     util.copyfile(tmpname, repo.wjoin(realname))<br>
                     # Our calls to copystat() here and above are a<br>
                     # hack to trick any editors that have f open that<br>
diff --git a/tests/test-commit-interactive.t b/tests/test-commit-interactive.t<br>
--- a/tests/test-commit-interactive.t<br>
+++ b/tests/test-commit-interactive.t<br>
@@ -1381,6 +1381,8 @@<br>
   record this change to 'subdir/f1'? [Ynesfdaq?] y<br>
<br>
<br>
+  $ hg status -A subdir/f1<br>
+  C subdir/f1<br>
   $ hg tip -p<br>
   changeset:   28:* (glob)<br>
   tag:         tip<br>
@@ -1417,6 +1419,8 @@<br>
   +e<br>
   record this change to 'subdir/f1'? [Ynesfdaq?] y<br>
<br>
+  $ hg status -A subdir/f1<br>
+  C subdir/f1<br>
   $ hg log --template '{author}\n' -l 1<br>
   xyz<br>
   $ HGUSER="test"<br>
@@ -1426,7 +1430,7 @@<br>
 Moving files<br>
<br>
   $ hg update -C .<br>
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved<br>
+  0 files updated, 0 files merged, 0 files removed, 0 files unresolved<br>
   $ hg mv plain plain3<br>
   $ echo somechange >> plain3<br>
   $ hg commit -i -d '23 0' -mmoving_files << EOF<br>
@@ -1447,6 +1451,8 @@<br>
   record this change to 'plain3'? [Ynesfdaq?] y<br>
<br>
 The #if execbit block above changes the hash here on some systems<br>
+  $ hg status -A plain3<br>
+  C plain3<br>
   $ hg tip<br>
   changeset:   30:* (glob)<br>
   tag:         tip<br>
@@ -1526,3 +1532,98 @@<br>
   +foo<br>
<br>
   $ cd ..<br>
+<br>
+  $ hg status -A folder/bar<br>
+  C folder/bar<br>
+<br>
+Clear win32text configuration before size/timestamp sensitive test<br>
+<br>
+  $ cat >> .hg/hgrc <<EOF<br>
+  > [extensions]<br>
+  > win32text = !<br>
+  > [decode]<br>
+  > ** = !<br>
+  > [encode]<br>
+  > ** = !<br>
+  > [patch]<br>
+  > eol = strict<br>
+  > EOF<br>
+  $ hg update -q -C null<br>
+  $ hg update -q -C tip<br>
+<br>
+Test that partially committed file is still treated as "modified",<br>
+even if none of mode, size and timestamp is changed on the filesystem<br>
+(see also issue4583).<br>
+<br>
+  $ cat > subdir/f1 <<EOF<br>
+  > A<br>
+  > a<br>
+  > a<br>
+  > b<br>
+  > c<br>
+  > d<br>
+  > E<br>
+  > EOF<br>
+  $ hg diff --git subdir/f1<br>
+  diff --git a/subdir/f1 b/subdir/f1<br>
+  --- a/subdir/f1<br>
+  +++ b/subdir/f1<br>
+  @@ -1,7 +1,7 @@<br>
+  -a<br>
+  +A<br>
+   a<br>
+   a<br>
+   b<br>
+   c<br>
+   d<br>
+  -e<br>
+  +E<br>
+<br>
+  $ touch -t 200001010000 subdir/f1<br>
+<br>
+  $ cat >> .hg/hgrc <<EOF<br>
+  > # emulate invoking patch.internalpatch() at 2000-01-01 00:00<br>
+  > [fakepatchtime]<br>
+  > fakenow = 200001010000<br>
+  ><br>
+  > [extensions]<br>
+  > fakepatchtime = $TESTDIR/fakepatchtime.py<br>
+  > EOF<br>
+  $ hg commit -i -m 'commit subdir/f1 partially' <<EOF<br>
+  > y<br>
+  > y<br>
+  > n<br>
+  > EOF<br>
+  diff --git a/subdir/f1 b/subdir/f1<br>
+  2 hunks, 2 lines changed<br>
+  examine changes to 'subdir/f1'? [Ynesfdaq?] y<br>
+<br>
+  @@ -1,6 +1,6 @@<br>
+  -a<br>
+  +A<br>
+   a<br>
+   a<br>
+   b<br>
+   c<br>
+   d<br>
+  record change 1/2 to 'subdir/f1'? [Ynesfdaq?] y<br>
+<br>
+  @@ -2,6 +2,6 @@<br>
+   a<br>
+   a<br>
+   b<br>
+   c<br>
+   d<br>
+  -e<br>
+  +E<br>
+  record change 2/2 to 'subdir/f1'? [Ynesfdaq?] n<br>
+<br>
+  $ cat >> .hg/hgrc <<EOF<br>
+  > [extensions]<br>
+  > fakepatchtime = !<br>
+  > EOF<br>
+<br>
+  $ hg debugstate | grep ' subdir/f1$'<br>
+  n   0         -1 unset               subdir/f1<br>
+  $ hg status -A subdir/f1<br>
+  M subdir/f1<br>
_______________________________________________<br>
Mercurial-devel mailing list<br>
<a href="mailto:Mercurial-devel@selenic.com" target="_blank">Mercurial-devel@selenic.com</a><br>
<a href="https://selenic.com/mailman/listinfo/mercurial-devel" rel="noreferrer" target="_blank">https://selenic.com/mailman/listinfo/mercurial-devel</a><br>
</blockquote></div></div>