1135215 - Preserve commit history in parent review requests

Status: RESOLVED DUPLICATE of bug 1142615

(MozReview Graveyard :: General, defect)

Description

[Mark Côté [:mcote]]

For various reasons, we should keep a full commit history of all children, current, discarded, or replaced, in the parent's extra_data.

Comment 1

[Mark Côté [:mcote]]

Attachment: MozReview Request: bz://1135215/mcote

/r/4109 - Bug 1135215 - Add historical_commits to parent rr's extra_data.

Pull down this commit:

hg pull review -r 6da7370931947d52496d2a772820889e24f7a82a

Comment 2

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/4107/#review3281

I've only updated one test.  There are many others that need to be updated, but I wanted some feedback first to make sure my fix makes sense.

Comment 3

[Mark Côté [:mcote]]

Comment on attachment 8567281 [details]
MozReview Request: bz://1135215/mcote

/r/4109 - Bug 1135215 - Add historical_commits to parent rr's extra_data.

Pull down this commit:

hg pull review -r 426e9666bd336b903469a3484532eb29ad1ab574

Comment 4

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/4109/#review3453

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 2)
> +    historical_commits += [tuple(x) for x in commit_list if tuple(x) not in

Nit: += on lists is generally discouraged. Use .extend() instead.

::: pylib/reviewboardmods/reviewboardmods/pushhooks.py
(Diff revision 2)
> +    historical_commits += [tuple(x) for x in commit_list if tuple(x) not in
> +                           historical_commits]
> +    historical_commits_json = json.dumps(
> +        sorted(historical_commits, key=lambda x: x[0]))

Use sets and much of this complexity just disappears.

historical_commits = set(get_historical_commits(squashed_rr))

historical_commits |= {tuple(x) for x in commit_list}

historical_commits_json = json.dumps(sorted(historical_commits))

Sorting by commit SHA-1 is also a bit wonky. IMO I like review id sorting:

historical_commits_json = json.dumps(sorted(historical_commits, key=operator.itemgetter(1)))

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8567281 [details]
MozReview Request: bz://1135215/mcote

https://reviewboard.mozilla.org/r/4107/#review3455

Ship It!

Comment 6

[Mark Côté [:mcote]]

https://reviewboard.mozilla.org/r/4109/#review3459

> Use sets and much of this complexity just disappears.
> 
> historical_commits = set(get_historical_commits(squashed_rr))
> 
> historical_commits |= {tuple(x) for x in commit_list}
> 
> historical_commits_json = json.dumps(sorted(historical_commits))
> 
> Sorting by commit SHA-1 is also a bit wonky. IMO I like review id sorting:
> 
> historical_commits_json = json.dumps(sorted(historical_commits, key=operator.itemgetter(1)))

Huh yeah, for some reason I was thinking the tuples wouldn't easily work with a set, but I dunno what I was thinking.  Your way works perfectly. :)

Also thanks for the itemgetter() usage; I knew there must be a better way than defining a lambda.

Comment 7

[Mark Côté [:mcote]]

Comment on attachment 8567281 [details]
MozReview Request: bz://1135215/mcote

/r/4109 - Bug 1135215 - Add historical_commits to parent rr's extra_data.

Pull down this commit:

hg pull review -r 415d7aaa2de7d0085a4bfd7c75d5e164cbb5fbb0

Comment 8

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/4109/#review3479

Ship It!

Comment 9

[Steven MacLeod [:smacleod]]

https://reviewboard.mozilla.org/r/4109/#review3493

We're throwing information away here about the order of pushes and commits being introduced. Which is fine for the current use case. We're still going to need to reimplement this to use a proper field down the line though.

This looks like it gives us enough info to implement the confidential stuff, so Ship It.

Comment 10

[Steven MacLeod [:smacleod]]

Comment on attachment 8567281 [details]
MozReview Request: bz://1135215/mcote

https://reviewboard.mozilla.org/r/4107/#review3495

Ship It!

Comment 11

[Mark Côté [:mcote]]

Being handled better in bug 1142615.