<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Thu, Jul 9, 2015 at 8:45 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">At Wed, 08 Jul 2015 20:28:36 +0000,<br>
Martin von Zweigbergk wrote:<br>
<br>
> On Wed, Jul 8, 2015 at 1:15 AM FUJIWARA Katsunori <<a href="mailto:foozy@lares.dti.ne.jp" target="_blank">foozy@lares.dti.ne.jp</a>><br>
> wrote:<br>
><br>
> > # 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>
(snip)<br>
<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>
> ><br>
><br>
> Should this be done after writing the file as we do elsewhere? Maybe it<br>
> doesn't matter since the file will get an old timestamp anyway (because of<br>
> shutil.copystat)?<br>
<br>
Yes, exactly speaking, 'lookupnormal()' should be invoked after<br>
writing the file out.<br></blockquote><div><br></div><div>IIUC, the only effect it might have is when lookupnormal() would give it timestamp N and the file got timestamp N+1, in which case the file would have to be unnecessarily checked for dirtiness. But since the timestamp gets replaced anyway, it doesn't matter. So leave it as is, I think that's fine.</div><div><br></div><div>Btw, I suppose this hack (the existing copystat() hack) means that all involved files would have to be checked for dirtiness.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On the other hand, IMHO, splitting below code block to place<br>
'lookupnormal()' after 'util.copyfile()' seemed to weaken logical<br>
meaning "restore file with original timestamp" of it.<br>
<br>
                    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>
                    # we haven't modified them.<br>
                    #<br>
                    # Also note that this racy as an editor could<br>
                    # notice the file's mtime before we've finished<br>
                    # writing it.<br>
                    shutil.copystat(tmpname, repo.wjoin(realname))<br>
                    os.unlink(tmpname)<br>
<br>
Should I move 'lookupnormal()' invocation ?<br>
<br>
----------------------------------------------------------------------<br>
[FUJIWARA Katsunori]                             <a href="mailto:foozy@lares.dti.ne.jp" target="_blank">foozy@lares.dti.ne.jp</a><br>
</blockquote></div></div>