| 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
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
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.
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.
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)
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
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.)
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?
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.)
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