Patchwork [1,of,4] revert: small refactoring in the way backup value are handled

login
register
mail settings
Submitter Pierre-Yves David
Date Sept. 20, 2014, 1:05 a.m.
Message ID <c7bf7274d2184c47fa09.1411175100@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/5894/
State Superseded
Headers show

Comments

Pierre-Yves David - Sept. 20, 2014, 1:05 a.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1409358507 -7200
#      Sat Aug 30 02:28:27 2014 +0200
# Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
# Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
revert: small refactoring in the way backup value are handled

The current backup value may have two different values:

  1. Do not try to do backup
  2. Do backup if applicable

We are about to move to:

  1. Do not try to do backup
  2. Do backup if applicable
  3. Do backup in all cases

So we change the current values to make room for the new one.
Augie Fackler - Sept. 24, 2014, 3:31 p.m.
On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1409358507 -7200
> #      Sat Aug 30 02:28:27 2014 +0200
> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
> revert: small refactoring in the way backup value are handled
>
> The current backup value may have two different values:
>
>   1. Do not try to do backup
>   2. Do backup if applicable
>
> We are about to move to:
>
>   1. Do not try to do backup
>   2. Do backup if applicable
>   3. Do backup in all cases
>
> So we change the current values to make room for the new one.
>
> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> --- a/mercurial/cmdutil.py
> +++ b/mercurial/cmdutil.py
> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>                     'unknown': (None, _('file not managed: %s\n')),
>                    }
>
>
>          # should we do a backup?
> -        backup = not opts.get('no_backup')
> -        discard = False
> +        backup = 2

Is there something evil and magic about 2 in this context? Why isn't
it true?

(probably missing something outside the diff context, but that
probably suggests this wants a symbolic constant instead of 2 as a
magic value.)

> +        discard = 0
> +        if opts.get('no_backup'):
> +            backup = 0
>
>          disptable = (
>              # dispatch table:
>              #   file state
>              #   action
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 24, 2014, 4:36 p.m.
On 09/24/2014 08:31 AM, Augie Fackler wrote:
> On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1409358507 -7200
>> #      Sat Aug 30 02:28:27 2014 +0200
>> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
>> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
>> revert: small refactoring in the way backup value are handled
>>
>> The current backup value may have two different values:
>>
>>    1. Do not try to do backup
>>    2. Do backup if applicable
>>
>> We are about to move to:
>>
>>    1. Do not try to do backup
>>    2. Do backup if applicable
>>    3. Do backup in all cases
>>
>> So we change the current values to make room for the new one.
>>
>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>> --- a/mercurial/cmdutil.py
>> +++ b/mercurial/cmdutil.py
>> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>>                      'unknown': (None, _('file not managed: %s\n')),
>>                     }
>>
>>
>>           # should we do a backup?
>> -        backup = not opts.get('no_backup')
>> -        discard = False
>> +        backup = 2
>
> Is there something evil and magic about 2 in this context? Why isn't
> it true?

because the point is a to add a third value. That will be:

0: no backup
1: check if backup needed
2: do backup unconditionnaly

The "1" value is introduced in the patch right after this one.
Augie Fackler - Sept. 24, 2014, 5 p.m.
On Wed, Sep 24, 2014 at 12:36 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 09/24/2014 08:31 AM, Augie Fackler wrote:
>>
>> On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
>>>
>>> # HG changeset patch
>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>> # Date 1409358507 -7200
>>> #      Sat Aug 30 02:28:27 2014 +0200
>>> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
>>> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
>>> revert: small refactoring in the way backup value are handled
>>>
>>> The current backup value may have two different values:
>>>
>>>    1. Do not try to do backup
>>>    2. Do backup if applicable
>>>
>>> We are about to move to:
>>>
>>>    1. Do not try to do backup
>>>    2. Do backup if applicable
>>>    3. Do backup in all cases
>>>
>>> So we change the current values to make room for the new one.
>>>
>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>> --- a/mercurial/cmdutil.py
>>> +++ b/mercurial/cmdutil.py
>>> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>>>                      'unknown': (None, _('file not managed: %s\n')),
>>>                     }
>>>
>>>
>>>           # should we do a backup?
>>> -        backup = not opts.get('no_backup')
>>> -        discard = False
>>> +        backup = 2
>>
>>
>> Is there something evil and magic about 2 in this context? Why isn't
>> it true?
>
>
> because the point is a to add a third value. That will be:
>
> 0: no backup
> 1: check if backup needed
> 2: do backup unconditionnaly
>
> The "1" value is introduced in the patch right after this one.


Then I'd like a resend that detangles this into a symbolic constant
for clarity of code.
Pierre-Yves David - Sept. 24, 2014, 5:04 p.m.
On 09/24/2014 10:00 AM, Augie Fackler wrote:
> On Wed, Sep 24, 2014 at 12:36 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 09/24/2014 08:31 AM, Augie Fackler wrote:
>>>
>>> On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
>>>>
>>>> # HG changeset patch
>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>> # Date 1409358507 -7200
>>>> #      Sat Aug 30 02:28:27 2014 +0200
>>>> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
>>>> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
>>>> revert: small refactoring in the way backup value are handled
>>>>
>>>> The current backup value may have two different values:
>>>>
>>>>     1. Do not try to do backup
>>>>     2. Do backup if applicable
>>>>
>>>> We are about to move to:
>>>>
>>>>     1. Do not try to do backup
>>>>     2. Do backup if applicable
>>>>     3. Do backup in all cases
>>>>
>>>> So we change the current values to make room for the new one.
>>>>
>>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>>> --- a/mercurial/cmdutil.py
>>>> +++ b/mercurial/cmdutil.py
>>>> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>>>>                       'unknown': (None, _('file not managed: %s\n')),
>>>>                      }
>>>>
>>>>
>>>>            # should we do a backup?
>>>> -        backup = not opts.get('no_backup')
>>>> -        discard = False
>>>> +        backup = 2
>>>
>>>
>>> Is there something evil and magic about 2 in this context? Why isn't
>>> it true?
>>
>>
>> because the point is a to add a third value. That will be:
>>
>> 0: no backup
>> 1: check if backup needed
>> 2: do backup unconditionnaly
>>
>> The "1" value is introduced in the patch right after this one.
>
>
> Then I'd like a resend that detangles this into a symbolic constant
> for clarity of code.

This -is- the assignment of the symbolic constant. (it just happen to be 
close by the code disabling all backup)
Augie Fackler - Sept. 24, 2014, 5:11 p.m.
On Wed, Sep 24, 2014 at 10:04:59AM -0700, Pierre-Yves David wrote:
>
>
> On 09/24/2014 10:00 AM, Augie Fackler wrote:
> >On Wed, Sep 24, 2014 at 12:36 PM, Pierre-Yves David
> ><pierre-yves.david@ens-lyon.org> wrote:
> >>
> >>
> >>On 09/24/2014 08:31 AM, Augie Fackler wrote:
> >>>
> >>>On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
> >>>>
> >>>># HG changeset patch
> >>>># User Pierre-Yves David <pierre-yves.david@fb.com>
> >>>># Date 1409358507 -7200
> >>>>#      Sat Aug 30 02:28:27 2014 +0200
> >>>># Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
> >>>># Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
> >>>>revert: small refactoring in the way backup value are handled
> >>>>
> >>>>The current backup value may have two different values:
> >>>>
> >>>>    1. Do not try to do backup
> >>>>    2. Do backup if applicable
> >>>>
> >>>>We are about to move to:
> >>>>
> >>>>    1. Do not try to do backup
> >>>>    2. Do backup if applicable
> >>>>    3. Do backup in all cases
> >>>>
> >>>>So we change the current values to make room for the new one.
> >>>>
> >>>>diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
> >>>>--- a/mercurial/cmdutil.py
> >>>>+++ b/mercurial/cmdutil.py
> >>>>@@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
> >>>>                      'unknown': (None, _('file not managed: %s\n')),
> >>>>                     }
> >>>>
> >>>>
> >>>>           # should we do a backup?
> >>>>-        backup = not opts.get('no_backup')
> >>>>-        discard = False
> >>>>+        backup = 2
> >>>
> >>>
> >>>Is there something evil and magic about 2 in this context? Why isn't
> >>>it true?
> >>
> >>
> >>because the point is a to add a third value. That will be:
> >>
> >>0: no backup
> >>1: check if backup needed
> >>2: do backup unconditionnaly
> >>
> >>The "1" value is introduced in the patch right after this one.
> >
> >
> >Then I'd like a resend that detangles this into a symbolic constant
> >for clarity of code.
>
> This -is- the assignment of the symbolic constant. (it just happen to be
> close by the code disabling all backup)

backup appears to be a variable here - sometimes you assign it one
value, sometimes another. I'm expecting it to be something like

backup = _ALWAYS_BACKUP
if opts.get('no_backup'):
   backup = _SKIP_BACKUPS

(Possible I'm reading this wrong, but I really don't think so.)

>
> --
> Pierre-Yves David
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Sept. 24, 2014, 5:18 p.m.
On 09/24/2014 10:11 AM, Augie Fackler wrote:
> On Wed, Sep 24, 2014 at 10:04:59AM -0700, Pierre-Yves David wrote:
>>
>>
>> On 09/24/2014 10:00 AM, Augie Fackler wrote:
>>> On Wed, Sep 24, 2014 at 12:36 PM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>
>>>> On 09/24/2014 08:31 AM, Augie Fackler wrote:
>>>>>
>>>>> On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>>>> # Date 1409358507 -7200
>>>>>> #      Sat Aug 30 02:28:27 2014 +0200
>>>>>> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
>>>>>> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
>>>>>> revert: small refactoring in the way backup value are handled
>>>>>>
>>>>>> The current backup value may have two different values:
>>>>>>
>>>>>>     1. Do not try to do backup
>>>>>>     2. Do backup if applicable
>>>>>>
>>>>>> We are about to move to:
>>>>>>
>>>>>>     1. Do not try to do backup
>>>>>>     2. Do backup if applicable
>>>>>>     3. Do backup in all cases
>>>>>>
>>>>>> So we change the current values to make room for the new one.
>>>>>>
>>>>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>>>>> --- a/mercurial/cmdutil.py
>>>>>> +++ b/mercurial/cmdutil.py
>>>>>> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>>>>>>                       'unknown': (None, _('file not managed: %s\n')),
>>>>>>                      }
>>>>>>
>>>>>>
>>>>>>            # should we do a backup?
>>>>>> -        backup = not opts.get('no_backup')
>>>>>> -        discard = False
>>>>>> +        backup = 2
>>>>>
>>>>>
>>>>> Is there something evil and magic about 2 in this context? Why isn't
>>>>> it true?
>>>>
>>>>
>>>> because the point is a to add a third value. That will be:
>>>>
>>>> 0: no backup
>>>> 1: check if backup needed
>>>> 2: do backup unconditionnaly
>>>>
>>>> The "1" value is introduced in the patch right after this one.
>>>
>>>
>>> Then I'd like a resend that detangles this into a symbolic constant
>>> for clarity of code.
>>
>> This -is- the assignment of the symbolic constant. (it just happen to be
>> close by the code disabling all backup)
>
> backup appears to be a variable here - sometimes you assign it one
> value, sometimes another. I'm expecting it to be something like
>
> backup = _ALWAYS_BACKUP
> if opts.get('no_backup'):
>     backup = _SKIP_BACKUPS
>
> (Possible I'm reading this wrong, but I really don't think so.)

We could double the initial assignation with "symbolic constant" name 
but as it going to be lower case I do not expect this to help much more 
than a comment

discard = _discard = 0
check   = _checkbackup = 1
backup  = _alwaysbackup = 2
if opts.get('no_backup'):
    check = backup = _discard.


Having the constant outside the function would be more confusing in my 
opignions.


It that improving the comment around that section will be the simplest 
way to improve the readability of this code (a comment must be added in 
the one test making distinction between check and backup too. but we can 
probably just use if dobackup == check in taht case.)
Pierre-Yves David - Sept. 25, 2014, 2:37 a.m.
On 09/24/2014 10:11 AM, Augie Fackler wrote:
> On Wed, Sep 24, 2014 at 10:04:59AM -0700, Pierre-Yves David wrote:
>>
>>
>> On 09/24/2014 10:00 AM, Augie Fackler wrote:
>>> On Wed, Sep 24, 2014 at 12:36 PM, Pierre-Yves David
>>> <pierre-yves.david@ens-lyon.org> wrote:
>>>>
>>>>
>>>> On 09/24/2014 08:31 AM, Augie Fackler wrote:
>>>>>
>>>>> On Fri, Sep 19, 2014 at 06:05:00PM -0700, Pierre-Yves David wrote:
>>>>>>
>>>>>> # HG changeset patch
>>>>>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>>>>>> # Date 1409358507 -7200
>>>>>> #      Sat Aug 30 02:28:27 2014 +0200
>>>>>> # Node ID c7bf7274d2184c47fa0988f054b24d1b2b3416cd
>>>>>> # Parent  5e16fe6fdd32124c3295db5ec40b076084cc5bd4
>>>>>> revert: small refactoring in the way backup value are handled
>>>>>>
>>>>>> The current backup value may have two different values:
>>>>>>
>>>>>>     1. Do not try to do backup
>>>>>>     2. Do backup if applicable
>>>>>>
>>>>>> We are about to move to:
>>>>>>
>>>>>>     1. Do not try to do backup
>>>>>>     2. Do backup if applicable
>>>>>>     3. Do backup in all cases
>>>>>>
>>>>>> So we change the current values to make room for the new one.
>>>>>>
>>>>>> diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
>>>>>> --- a/mercurial/cmdutil.py
>>>>>> +++ b/mercurial/cmdutil.py
>>>>>> @@ -2620,12 +2620,14 @@ def revert(ui, repo, ctx, parents, *pats
>>>>>>                       'unknown': (None, _('file not managed: %s\n')),
>>>>>>                      }
>>>>>>
>>>>>>
>>>>>>            # should we do a backup?
>>>>>> -        backup = not opts.get('no_backup')
>>>>>> -        discard = False
>>>>>> +        backup = 2
>>>>>
>>>>>
>>>>> Is there something evil and magic about 2 in this context? Why isn't
>>>>> it true?
>>>>
>>>>
>>>> because the point is a to add a third value. That will be:
>>>>
>>>> 0: no backup
>>>> 1: check if backup needed
>>>> 2: do backup unconditionnaly
>>>>
>>>> The "1" value is introduced in the patch right after this one.
>>>
>>>
>>> Then I'd like a resend that detangles this into a symbolic constant
>>> for clarity of code.
>>
>> This -is- the assignment of the symbolic constant. (it just happen to be
>> close by the code disabling all backup)
>
> backup appears to be a variable here - sometimes you assign it one
> value, sometimes another. I'm expecting it to be something like
>
> backup = _ALWAYS_BACKUP
> if opts.get('no_backup'):
>     backup = _SKIP_BACKUPS
>
> (Possible I'm reading this wrong, but I really don't think so.)

So I agree the current thing is confusing. But I disagree that using 
symbolic constant will help readability here. If add a comment to 
clarify and remove the other reference to numeric value in the function 
would you be an happy panda?
Augie Fackler - Sept. 29, 2014, 9:37 p.m.
On Wed, Sep 24, 2014 at 10:37 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 09/24/2014 10:11 AM, Augie Fackler wrote:

>> backup = _ALWAYS_BACKUP
>> if opts.get('no_backup'):
>>     backup = _SKIP_BACKUPS
>>
>> (Possible I'm reading this wrong, but I really don't think so.)
>
>
> So I agree the current thing is confusing. But I disagree that using
> symbolic constant will help readability here. If add a comment to clarify
> and remove the other reference to numeric value in the function would you be
> an happy panda?

Maybe, but I doubt it. Can you show me what you have in mind, at least
as a sketch?

(I'm lost enough on this thread at this point I'm not honestly sure
what you're proposing.)
Pierre-Yves David - Sept. 29, 2014, 10:01 p.m.
On 09/29/2014 02:37 PM, Augie Fackler wrote:
> On Wed, Sep 24, 2014 at 10:37 PM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org> wrote:
>>
>>
>> On 09/24/2014 10:11 AM, Augie Fackler wrote:
>
>>> backup = _ALWAYS_BACKUP
>>> if opts.get('no_backup'):
>>>      backup = _SKIP_BACKUPS
>>>
>>> (Possible I'm reading this wrong, but I really don't think so.)
>>
>>
>> So I agree the current thing is confusing. But I disagree that using
>> symbolic constant will help readability here. If add a comment to clarify
>> and remove the other reference to numeric value in the function would you be
>> an happy panda?
>
> Maybe, but I doubt it. Can you show me what you have in mind, at least
> as a sketch?

The end result would looks like:

         # "constant" that convey the backup strategy.
         # All set to `discard` if `no-backup` is set do avoid checking
         # no_backup lower in the code.
         backup = 2  # unconditionnally do backup
         check = 1   # check if existing file differ from target
         discard = 0 # never do backup
         if opts.get('no_backup'):
             backup = check = discard

Patch

diff --git a/mercurial/cmdutil.py b/mercurial/cmdutil.py
--- a/mercurial/cmdutil.py
+++ b/mercurial/cmdutil.py
@@ -2620,12 +2620,14 @@  def revert(ui, repo, ctx, parents, *pats
                    'unknown': (None, _('file not managed: %s\n')),
                   }
 
 
         # should we do a backup?
-        backup = not opts.get('no_backup')
-        discard = False
+        backup = 2
+        discard = 0
+        if opts.get('no_backup'):
+            backup = 0
 
         disptable = (
             # dispatch table:
             #   file state
             #   action