[PATCH 3 of 6] dirstate: read from `.pending` file under HG_PENDING mode
Pierre-Yves David
pierre-yves.david at ens-lyon.org
Fri May 22 16:57:50 UTC 2015
On 05/22/2015 11:34 AM, FUJIWARA Katsunori wrote:
>
> At Thu, 21 May 2015 16:59:47 -0500,
> Pierre-Yves David wrote:
>>
>> On 05/19/2015 11:42 AM, FUJIWARA Katsunori wrote:
>>> # HG changeset patch
>>> # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
>>> # Date 1432051569 -32400
>>> # Wed May 20 01:06:09 2015 +0900
>>> # Node ID e830ef506b9aaa01c5406d100a385961c4c0dff0
>>> # Parent 2d2eff9ee053025e8e0267b8d6c6d07326f607ee
>>> dirstate: read from `.pending` file under HG_PENDING mode
>>>
>>> True/False value of `_pendingmode` means whether `dirstate.pending` is
>>> used to initialize own `_map` and so on. When it is None, neither
>>> `dirstate` nor `dirstate.pending` is read in yet.
>>>
>>> This is used to keep consistent view between `_pl()` and `_read()`.
>>>
>>> Once `_pendingmode` is determined by reading one of `dirstate` or
>>> `dirstate.pending` in, `_pendingmode` is kept even if `invalidate()`
>>> is invoked. This should be reasonable, because:
>>>
>>> - effective `invalidate()` invocation should occur only in wlock scope, and
>>> - wlock can't be gotten under HG_PENDING mode
>>>
>>> For review-ability, this patch focuses only on reading `.pending` file
>>> in. Then, `dirstate.write()` can write changes out even under
>>> HG_PENDING mode. But it is still safe enough, because there is no code
>>> path writing `.pending` file out yet (= `_pendingmode` never be True).
>>>
>>> `_trypending()` is defined as a normal function to factor similar code
>>> path (in bookmarks and phases) out in the future easily.
>>
>> This series is poking in the right direction, but it is failing to
>> acknowledge the following list of principle and will not result in a
>> working behavior.
>>
>> 1) '.pending' files are made necessary by the existence of a transaction,
>> 2) HG_PENDING (and .pending files usage) must -only- occured in the
>> context of a transaction.
>> 3) The transaction object is the one handle the '.pending' files and
>> associated non-pending file. This includes:
>>
>> 3.1) The transaction must purge any existing '.pending' when it open/close
>> 3.2) generation of .pending files is done through the transaction
>> 3.3) writing of the final files is also done through the transaction
>>
>> *) We now have a developer warning feature, we should use it more.
>>
>> Let me details this more.
>
> Thank you for detailed explanation !
>
> And, perhaps, should I post the patch below not as the first one of
> next series but as the last one of this series ?
>
> I put this into next series, because this seemed to increase
> review-ability of client code paths (= making in-memory changes
> visible to external process) of these functions in next series.
>
> ====================
> diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
> --- a/mercurial/dirstate.py
> +++ b/mercurial/dirstate.py
> @@ -632,6 +632,35 @@ class dirstate(object):
> return
> self._writedirstate(self._filename)
>
> + def delayupdate(self, tr):
> + '''Delay visibility of dirstate to other readers
> + '''
> + category = 'dirstate-%i' % id(self)
> +
> + tr.addpending(category, self._writepending)
> +
> + # pending file should be fixed up regardless of outcome of transaction
> + tr.addfinalize(category, self._fixuppending)
> + tr.addabort(category, self._fixuppending)
> +
> + def makependingvisible(self, repo):
> + '''Make pending disrtate changes visible to external process
> +
> + This is an utility to treat pending changes regardless of
> + transaction activity.
> +
> + This returns the value to be set to 'HG_PENDING' environment
> + variable for external process.
> + '''
> + tr = repo.currenttransaction()
> + if tr:
> + self.delayupdate(tr)
> + if tr.writepending():
> + return repo.root # set 'HG_PENDING' only in this case
> + else:
> + self.write()
> + return ""
> +
> def _removepending(self):
> try:
> self._opener.unlink(self._pendingfilename)
> ====================
>
> This assumes that:
>
> - `dirstate.delayupdate()` will be used in certain transaction scope
> (e.g. for `pretxncommit` hook), like `changelog.delayupdate()`
>
> - `dirstate.makependingvisible()` will be used in other code paths
> (e.g. for commit editor, `precommit` hook and so on), where
> transaction activity is determined only at runtime.
>
> I think that using these functions can satisfy rules you described
> above. How about this ?
>
>
> I chose `addpending()` instead of `addfilegenerator()` to write
> dirstate pending file out, because assumption below of the latter
> seems not suitable for dirstate characteristic:
>
> - changes will be written out at successful `transaction.close()`
> - pending file will be discarded at transaction failure
>
> As described in patch #4 of this series, dirstate changes should be
> written out (at releasing wlock) regardless of outcome of transaction
> in almost all cases.
This assumption are a surprise to me. Can you explain why we do not want
change to the dirstate that at "done withing a transaction" to be
rollbacked during that transaction.
Practical example will probably help to transfer your knowledge in my
direction :)
>> Developer Warning:
>> -----------------
>>
>> To work your code (will) make multiple assumption:
>>
> [snip]
>> - write are always done in a dirstate guard context,
>
> Even though `dirstateguard` is typical `dirstate.write()` caller, I
> don't assume above. But are there something to indicate or suggest so ?
> I may overllook them :-<
I'm far less familiar with this topic than you. But I cannot think of a
case were we would want to update the dirstate content without some sort
of transactional protect that guard is offering.
Do you have an example ?
--
Pierre-Yves David
More information about the Mercurial-devel
mailing list