Expected behavior when both copyAllScoresOnTrivialRebase & copyAllScoresOnNoCodeChange = true

[atav...@yext.com]

Hi all,

Suppose I have my Code-Review label configured like so:

[label "Code-Review"]
    function = MaxWithBlock
    defaultValue = 0
    copyMinScore = true
    copyAllScoresOnTrivialRebase = true
    copyAllScoresIfNoCodeChange = true
    value = -2 This shall not be merged
    value = -1 I'd prefer if this were not merged
    value =  0 No score
    value = +1 Looks good, but someone else needs to approve
    value = +2 LGTM, approved

If a commit is rebased and its commit-message is changed, but there is no code delta, and then pushed as a new patchset? Will the score be copied?

It seems that if either of those two (rebase w/ no commit-message change or code delta; commit-message change w/ no parent change or code delta) are pushed independently, the scores are copied; but when combined into one patchset, they are not. I can't think of a situation where pushing these two changes separately vs together results in a different final patchset, so I'm wondering if this is expected/desired behavior -- or is there a case I'm missing that justifies not copying the scores over?

Thanks!

[atav...@yext.com]

Forgot to mention: I'm currently running stable release 2.15.2

[Jonathan Nieder]

Sounds worth a feature request at https://crbug.com/gerrit/new.

Thanks and hope that helps,

Jonathan

пт, 29 июн. 2018 г. в 13:02, <atav...@yext.com>:

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Edwin Kempin]

On Sat, Jun 30, 2018 at 12:06 AM 'Jonathan Nieder' via Repo and Gerrit Discussion <repo-d...@googlegroups.com> wrote:

Sounds worth a feature request at https://crbug.com/gerrit/new.

Thanks and hope that helps,

Jonathan

пт, 29 июн. 2018 г. в 13:02, <atav...@yext.com>:

Forgot to mention: I'm currently running stable release 2.15.2

On Friday, June 29, 2018 at 4:01:15 PM UTC-4, atav...@yext.com wrote:

Hi all,

Suppose I have my Code-Review label configured like so:

[label "Code-Review"]
    function = MaxWithBlock
    defaultValue = 0
    copyMinScore = true
    copyAllScoresOnTrivialRebase = true
    copyAllScoresIfNoCodeChange = true
    value = -2 This shall not be merged
    value = -1 I'd prefer if this were not merged
    value =  0 No score
    value = +1 Looks good, but someone else needs to approve
    value = +2 LGTM, approved

If a commit is rebased and its commit-message is changed, but there is no code delta, and then pushed as a new patchset? Will the score be copied?

It seems that if either of those two (rebase w/ no commit-message change or code delta; commit-message change w/ no parent change or code delta) are pushed independently, the scores are copied; but when combined into one patchset, they are not. I can't think of a situation where pushing these two changes separately vs together results in a different final patchset, so I'm wondering if this is expected/desired behavior -- or is there a case I'm missing that justifies not copying the scores over?

It's expected that Gerrit cannot detect a combination of these change kinds (rebase w/ no commit-message change or code delta; commit-message change w/ no parent change or code delta) because it's not implemented. I agree that it would be nice if it was able to detect it. Please file a feature request.

[atav...@yext.com]

Thanks Edwin & Jonathan. I've opened feature request 9379.

Looking at the relevant code (ChangeKindCacheImpl and ApprovalsCopier), it seems like these types of commits are classified as REWORKs, which means neither of those copyAllScores apply by design. So, either these types of commits will need to be classified as an existing type, or a new type.

Existing ChangeKinds to consider:

• TRIVIAL_REBASE
• This type of commit technically fits with the description of "Conflict-free merge between the new parent and the prior patch set" in the patchSet documentation, as commit message deltas can't create conflicts
• It may break some plugins/integrations/etc. that assume this ChangeType implies commit-message is unchanged
• NO_CODE_CHANGE
• This type of commit technically fits with the name, if you consider commit-messages not to be code
• It doesn't fit the documented description of "Same tree and same parent-tree"

New ChangeKind (e.g. REBASE_NO_CODE_CHANGE):

• This can also be problematic for existing plugins/integrations/etc. that have handlers for the current 5 kinds & assume these cover all types of commits
  

I'm not sure if any one of these options are preferable (or if these are the only ones), but in any case I hope this helps in fleshing out this feature.

Thanks!