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

[Pierre-Yves David]

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


-- 
Pierre-Yves David