Patchwork [2,of,4] bisect: extract the 'reset' logic into its own function

login
register
mail settings
Submitter Pierre-Yves David
Date Oct. 3, 2016, 2:37 p.m.
Message ID <efd8013b1521399d07ca.1475505438@nodosa.octopoid.net>
Download mbox | patch
Permalink /patch/16829/
State Accepted
Headers show

Comments

Pierre-Yves David - Oct. 3, 2016, 2:37 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
# Date 1472004833 -7200
#      Wed Aug 24 04:13:53 2016 +0200
# Node ID efd8013b1521399d07ca956c1cba7bd9f7dfc6e0
# Parent  ec6cb977f8e62bf13f86c3a7ebbee182ae50422e
# EXP-Topic bisect
bisect: extract the 'reset' logic into its own function

This is part of small clean up movement. The bisect module seem more appropriate
to host the bisect logic. The cleanup itself is motivated by some higher level
cleanup around vfs and locking.
Ryan McElroy - Oct. 5, 2016, 3:56 p.m.
On 10/3/2016 3:37 PM, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
> # Date 1472004833 -7200
> #      Wed Aug 24 04:13:53 2016 +0200
> # Node ID efd8013b1521399d07ca956c1cba7bd9f7dfc6e0
> # Parent  ec6cb977f8e62bf13f86c3a7ebbee182ae50422e
> # EXP-Topic bisect
> bisect: extract the 'reset' logic into its own function
>
> This is part of small clean up movement. The bisect module seem more appropriate
> to host the bisect logic. The cleanup itself is motivated by some higher level
> cleanup around vfs and locking.
>
> diff --git a/mercurial/commands.py b/mercurial/commands.py
> --- a/mercurial/commands.py
> +++ b/mercurial/commands.py
> @@ -902,8 +902,7 @@ def bisect(ui, repo, rev=None, extra=Non
>       cmdutil.checkunfinished(repo)
>   
>       if reset:
> -        if repo.vfs.exists("bisect.state"):
> -            repo.vfs.unlink("bisect.state")
> +        hbisect.resetstate(repo)
>           return
>   
>       state = hbisect.load_state(repo)
> diff --git a/mercurial/hbisect.py b/mercurial/hbisect.py
> --- a/mercurial/hbisect.py
> +++ b/mercurial/hbisect.py
> @@ -159,6 +159,11 @@ def save_state(repo, state):
>                   f.write("%s %s\n" % (kind, hex(node)))
>           f.close()
>   
> +def resetstate(repo):
> +    """remove any bisect state from the repository"""
> +    if repo.vfs.exists("bisect.state"):
> +        repo.vfs.unlink("bisect.state")

Generally, do we care about the race condition here? Would it be better 
to do something like the below (In another patch)?

+ try:
+     repo.vfs.unlink("bisect.state")
+ except OSError:
+    if repo.vfs.exists("bisect.state"):
+        raise


> +
>   def get(repo, status):
>       """
>       Return a list of revision(s) that match the given status:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
Pierre-Yves David - Oct. 8, 2016, 10:10 a.m.
On 10/05/2016 05:56 PM, Ryan McElroy wrote:
> On 10/3/2016 3:37 PM, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@ens-lyon.org>
>> # Date 1472004833 -7200
>> #      Wed Aug 24 04:13:53 2016 +0200
>> # Node ID efd8013b1521399d07ca956c1cba7bd9f7dfc6e0
>> # Parent  ec6cb977f8e62bf13f86c3a7ebbee182ae50422e
>> # EXP-Topic bisect
>> bisect: extract the 'reset' logic into its own function
>>
>> This is part of small clean up movement. The bisect module seem more
>> appropriate
>> to host the bisect logic. The cleanup itself is motivated by some
>> higher level
>> cleanup around vfs and locking.
>>
>> diff --git a/mercurial/commands.py b/mercurial/commands.py
>> --- a/mercurial/commands.py
>> +++ b/mercurial/commands.py
>> @@ -902,8 +902,7 @@ def bisect(ui, repo, rev=None, extra=Non
>>       cmdutil.checkunfinished(repo)
>>         if reset:
>> -        if repo.vfs.exists("bisect.state"):
>> -            repo.vfs.unlink("bisect.state")
>> +        hbisect.resetstate(repo)
>>           return
>>         state = hbisect.load_state(repo)
>> diff --git a/mercurial/hbisect.py b/mercurial/hbisect.py
>> --- a/mercurial/hbisect.py
>> +++ b/mercurial/hbisect.py
>> @@ -159,6 +159,11 @@ def save_state(repo, state):
>>                   f.write("%s %s\n" % (kind, hex(node)))
>>           f.close()
>>   +def resetstate(repo):
>> +    """remove any bisect state from the repository"""
>> +    if repo.vfs.exists("bisect.state"):
>> +        repo.vfs.unlink("bisect.state")
>
> Generally, do we care about the race condition here? Would it be better
> to do something like the below (In another patch)?

Generally it does, however, we are supposed to have locking before 
touching this things (which we actually do not, but that will get fixed 
soon). We could add that double security in another patches.

Cheers,

Patch

diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -902,8 +902,7 @@  def bisect(ui, repo, rev=None, extra=Non
     cmdutil.checkunfinished(repo)
 
     if reset:
-        if repo.vfs.exists("bisect.state"):
-            repo.vfs.unlink("bisect.state")
+        hbisect.resetstate(repo)
         return
 
     state = hbisect.load_state(repo)
diff --git a/mercurial/hbisect.py b/mercurial/hbisect.py
--- a/mercurial/hbisect.py
+++ b/mercurial/hbisect.py
@@ -159,6 +159,11 @@  def save_state(repo, state):
                 f.write("%s %s\n" % (kind, hex(node)))
         f.close()
 
+def resetstate(repo):
+    """remove any bisect state from the repository"""
+    if repo.vfs.exists("bisect.state"):
+        repo.vfs.unlink("bisect.state")
+
 def get(repo, status):
     """
     Return a list of revision(s) that match the given status: