<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>