[PATCH] resolve issue 3375: Operation not permitted: journal.phaseroots
Matt Mackall
mpm at selenic.com
Tue Apr 17 10:50:36 CDT 2012
On Tue, 2012-04-17 at 10:08 -0400, Michael Bacarella wrote:
> # HG changeset patch
> # User Michael Bacarella <mbacarella at janestreet.com>
> # Date 1334671188 14400
> # Node ID a7b53829e08f290f5e9c02bfff19c6713fb33efb
> # Parent 3387c7dc83b1c94b7ac16ed7fa152d9b19e74efe
> localrepo: fix unpushable repos when transaction aborted (issue3375)
>
> This fix is similar to the fix for issue3317.
>
> This patch makes journal file copying more consistent.
On closer inspection, everything here is wrong and has always been
wrong.
There are two patterns here:
try:
data = opener.read()
except IOError:
data = ""
opener.write(data)
and:
if os.path.exists():
util.copyfile()
else:
write("")
..and they're both wrong. The first will silently lose data when we get
an IOError other than ENOENT, for instance EPERM. If a transaction's
aborted or rolled back, the file will be lost, because write() will
actually -delete- the target file first to ensure success.
The second opens existing files and tries to write to them and/or change
their modes, which can fail if they have unfortunate modes and/or
ownership.
The first pattern was introduced by me about 7 years ago:
http://www.selenic.com/hg/diff/e8eb427c6d71/mercurial/hg.py
The second pattern was introduced here:
http://www.selenic.com/hg/rev/89e7d35e0ef0
Note that the code it deletes in the first chunk is following the first
pattern. Lesson: don't be clever when moving code around.
So what's the fix? The right fix.. is to use the first pattern but to
explicitly check for ENOENT. In fact, I suspect just about every
instance of "except IOError:" without inspecting the exception in the
code is a bug.
I'll spin up a proper patch for this, thanks.
--
Mathematics is the supreme nostalgia of our time.
More information about the Mercurial-devel
mailing list