The Trunk: Monticello-tfel.637.mcz

Hi both,

2016-07-25 17:55 GMT+02:00 Chris Muller <[hidden email]>:

Please do not use abbreviations.  What is AST?  Abstract Syntax Tree?
This takes the Monticello tool to a new level of technical
intimidation and confusion.

Why do we need a button for this?

I would say, why only one? Why a lack of symmetry?

The same AST means that some formatting has been performed.
I think AST currently contains comments, but see below...

Of course, with auto-format that we currently apply in the UI, we can't even visualize those diff (grrr! I hate those auto-format)

So anyway we are blind to these changes, we could as well throw a dice ;)

 

On Mon, Jul 25, 2016 at 8:24 AM,  <[hidden email]> wrote:
> Tim Felgentreff uploaded a new version of Monticello to project The Trunk:
> http://source.squeak.org/trunk/Monticello-tfel.637.mcz
>
> ==================== Summary ====================
>
> Name: Monticello-tfel.637
> Author: tfel
> Time: 25 July 2016, 3:24:24.996828 pm
> UUID: cf0d6af1-c703-5044-9c57-e798b0cf3abf
> Ancestors: Monticello-cmm.636
>
> add a button for rejecting all incoming conflicts that only change AST
>
> =============== Diff against Monticello-cmm.636 ===============
>
> Item was added:
> + ----- Method: MCConflict>>chooseSameAST (in category 'as yet unclassified') -----
> + chooseSameAST
> +       | fromSrc toSrc |
> +       (self definition isNil or: [self definition isMethodDefinition not])
> +               ifTrue: [^ self].
> +       fromSrc := (Parser new parse: operation fromSource class: nil class)
> +               generate decompile asString.
> +       toSrc := (Parser new parse: operation toSource class: nil class)
> +               generate decompile asString.
> +       fromSrc = toSrc ifTrue: [self chooseLocal].!
>

OK, so this does not even take the comments in account, because this is decompiled code (reconstructed AST) not original AST.

There's another problem here: decompile: sometimes fail, so we should protect ourselves...

 

> Item was changed:
>   ----- Method: MCMergeBrowser>>buttonSpecs (in category 'as yet unclassified') -----
>   buttonSpecs
>         ^ #((Merge merge 'Proceed with the merge' canMerge)
>                  (Cancel cancel 'Cancel the merge')
>                 ('All Newer' chooseAllNewerConflicts 'Choose all newer conflict versions')
>                 ('All Older' chooseAllOlderConflicts 'Choose all older conflict versions')
>                 ('Rest Reject' chooseAllUnchosenLocal 'Choose local versions of all remaining conflicts')
>                 ('Rest Accept' chooseAllUnchosenRemote 'Choose remote versions of all remaining conflicts')
> +               ('Accept same source' chooseAllSameAST 'Choose all local conflicting versions that have essentially the same code')
>   )!
>

There's another problem above:

Rest Accept is for accepting the remote changes

Accept same source is for accepting the local...

That can't be the same word, much confusing!

Why interesting?  I tend to think that people should not be committing
changes in which only the formatting was changed.  So, maybe better to
take measures to minimize that from happening in the first place than
introduce a button to "undo" it later....?

On Mon, Jul 25, 2016 at 4:43 PM, Nicolas Cellier
<[hidden email]> wrote:
> Whatever the flow of critics, I find the feature interesting.
>
>
>
> 2016-07-25 23:17 GMT+02:00 Chris Cunningham <[hidden email]>:
>>
>>
>>
>> On Mon, Jul 25, 2016 at 10:03 AM, Nicolas Cellier
>> <[hidden email]> wrote:
>>>
>>> Hi both,
>>>
>>> 2016-07-25 17:55 GMT+02:00 Chris Muller <[hidden email]>:
>>>>
>>>> Please do not use abbreviations.  What is AST?  Abstract Syntax Tree?
>>>> This takes the Monticello tool to a new level of technical
>>>> intimidation and confusion.
>>>>
>>>> Why do we need a button for this?
>>>
>>>
>>> I would say, why only one? Why a lack of symmetry?
>>> The same AST means that some formatting has been performed.
>>> I think AST currently contains comments, but see below...
>>>
>>> Of course, with auto-format that we currently apply in the UI, we can't
>>> even visualize those diff (grrr! I hate those auto-format)
>>> So anyway we are blind to these changes, we could as well throw a dice ;)
>>>
>>>>
>>>>
>>>> On Mon, Jul 25, 2016 at 8:24 AM,  <[hidden email]> wrote:
>>>> > Tim Felgentreff uploaded a new version of Monticello to project The
>>>> > Trunk:
>>>> > http://source.squeak.org/trunk/Monticello-tfel.637.mcz
>>>> >
>>>> > ==================== Summary ====================
>>>> >
>>>> > Name: Monticello-tfel.637
>>>> > Author: tfel
>>>> > Time: 25 July 2016, 3:24:24.996828 pm
>>>> > UUID: cf0d6af1-c703-5044-9c57-e798b0cf3abf
>>>> > Ancestors: Monticello-cmm.636
>>>> >
>>>> > add a button for rejecting all incoming conflicts that only change AST
>>>> >
>>>> > =============== Diff against Monticello-cmm.636 ===============
>>>> >
>>>> > Item was added:
>>>> > + ----- Method: MCConflict>>chooseSameAST (in category 'as yet
>>>> > unclassified') -----
>>>> > + chooseSameAST
>>>> > +       | fromSrc toSrc |
>>>> > +       (self definition isNil or: [self definition isMethodDefinition
>>>> > not])
>>>> > +               ifTrue: [^ self].
>>>> > +       fromSrc := (Parser new parse: operation fromSource class: nil
>>>> > class)
>>>> > +               generate decompile asString.
>>>> > +       toSrc := (Parser new parse: operation toSource class: nil
>>>> > class)
>>>> > +               generate decompile asString.
>>>> > +       fromSrc = toSrc ifTrue: [self chooseLocal].!
>>>> >
>>>
>>>
>>> OK, so this does not even take the comments in account, because this is
>>> decompiled code (reconstructed AST) not original AST.
>>> There's another problem here: decompile: sometimes fail, so we should
>>> protect ourselves...
>>>
>>>
>>>>
>>>> > Item was changed:
>>>> >   ----- Method: MCMergeBrowser>>buttonSpecs (in category 'as yet
>>>> > unclassified') -----
>>>> >   buttonSpecs
>>>> >         ^ #((Merge merge 'Proceed with the merge' canMerge)
>>>> >                  (Cancel cancel 'Cancel the merge')
>>>> >                 ('All Newer' chooseAllNewerConflicts 'Choose all newer
>>>> > conflict versions')
>>>> >                 ('All Older' chooseAllOlderConflicts 'Choose all older
>>>> > conflict versions')
>>>> >                 ('Rest Reject' chooseAllUnchosenLocal 'Choose local
>>>> > versions of all remaining conflicts')
>>>> >                 ('Rest Accept' chooseAllUnchosenRemote 'Choose remote
>>>> > versions of all remaining conflicts')
>>>> > +               ('Accept same source' chooseAllSameAST 'Choose all
>>>> > local conflicting versions that have essentially the same code')
>>>> >   )!
>>>> >
>>>
>>>
>>> There's another problem above:
>>> Rest Accept is for accepting the remote changes
>>> Accept same source is for accepting the local...
>>>
>>> That can't be the same word, much confusing!
>>>
>> Right.  If that option is added, it must be called 'Reject same source'.
>>
>> Probably call it something else: 'Reject cosmetic changes', maybe.
>>
>> In any case, let's not make it worse after recently cleaning up the
>> wording.
>>
>> -cbc
>>
>>
>>
>>
>
>
>
>