Diff not sent when review requested

[Guilherme Salgado]

I was asked to review https://code.edge.launchpad.net/~julian-edwards/launchpad/ppa-binary-expiry-bug-316317/+merge/3071 and got an email saying so, but the diff I see in the m-p was not included in that email. I think it should.

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Guilherme Salgado wrote:
> Public bug reported:
>
> I was asked to review https://code.edge.launchpad.net/~julian-
> edwards/launchpad/ppa-binary-expiry-bug-316317/+merge/3071 and got an
> email saying so, but the diff I see in the m-p was not included in that
> email. I think it should.

If it included that, wouldn't it also logically include the cover
letter, i.e. the first comment? And if it includes one comment,
shouldn't it include all of them?

It just seems like a can of worms. They don't send you all messages to
date when you join a mailing list, just further mail.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkl5y10ACgkQ0F+nu1YWqI3X8QCfQ7YV5Gkr5NUSC0yYKSSFO9lx
sEEAnjOz7IBEJUz66jbKwO485x26uaYN
=9Jio
-----END PGP SIGNATURE-----

[Guilherme Salgado (salgado)]

On Fri, 2009-01-23 at 13:51 +0000, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Guilherme Salgado wrote:
> > Public bug reported:
> >
> > I was asked to review https://code.edge.launchpad.net/~julian-
> > edwards/launchpad/ppa-binary-expiry-bug-316317/+merge/3071 and got an
> > email saying so, but the diff I see in the m-p was not included in that
> > email. I think it should.
>
> If it included that, wouldn't it also logically include the cover
> letter, i.e. the first comment? And if it includes one comment,
> shouldn't it include all of them?

Yeah, the cover letter would be good too. But I don't see any reason
for including other comments as well.

>
> It just seems like a can of worms. They don't send you all messages to
> date when you join a mailing list, just further mail.

Indeed, but I get a bug's description when I'm subscribed/assigned to
it. And being asked to review a branch looks more like being subscribed
to a bug than to a mailing list.

--
Guilherme Salgado <email address hidden>

[Tim Penhey (thumper)]

On Sat, 24 Jan 2009 03:21:52 Guilherme Salgado wrote:
> On Fri, 2009-01-23 at 13:51 +0000, Aaron Bentley wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > Guilherme Salgado wrote:
> > > Public bug reported:
> > >
> > > I was asked to review https://code.edge.launchpad.net/~julian-
> > > edwards/launchpad/ppa-binary-expiry-bug-316317/+merge/3071 and got an
> > > email saying so, but the diff I see in the m-p was not included in that
> > > email. I think it should.
> >
> > If it included that, wouldn't it also logically include the cover
> > letter, i.e. the first comment? And if it includes one comment,
> > shouldn't it include all of them?
>
> Yeah, the cover letter would be good too. But I don't see any reason
> for including other comments as well.
>
> > It just seems like a can of worms. They don't send you all messages to
> > date when you join a mailing list, just further mail.
>
> Indeed, but I get a bug's description when I'm subscribed/assigned to
> it. And being asked to review a branch looks more like being subscribed
> to a bug than to a mailing list.

This is a dup of the bug Barry reported. Barry did in fact want the entire
conversation sent when a review was requested.

[Jonathan Lange (jml)]

IMO, Barry is wrong and Salgado is right. Sending the diff and the cover letter is a) what we used to do and b) the way Bazaar does things right now.

If there are more comments than just the first, the email should say "N more comments".

[Björn Tillenius (bjornt)]

Getting the diff and cover letter when I'm requested to review a branch would make me review the branch quicker. Going to a web UI to fetch the information breaks my workflow most of the time, so I tend to put it off for later.

[Tim Penhey (thumper)]

Fixed in db-devel 7726.