Summary:

Create filename using bug numbers when downloading the diff

Review Request #694 — Created Jan. 13, 2009 and submitted

Information
Owner:	onkarshinde
Repository:	Review Board SVN (deprecated)
Branch:
Bugs:	815
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

The filename used while downloading the diff should be created using the bugs closed. This makes the file name unique for every diff and save the trouble of changing the name every time the diff is downloaded.

Testing Done

/trunk/reviewboard/reviews/views.py (Diff revision 1)

Maybe do this if the filename is just "diff"? If someone uploads a diff, it will use that filename, and it may be more descriptive (including, say, a changeset number or date or something as an identifier). In that case, I'd hate to override it with something we generate.

Change Summary:

Added if block to use generated filename only when diffset.name is "diff".

Diff:

Revision 2 (+4 -1)

Show changes

/trunk/reviewboard/reviews/views.py

/trunk/reviewboard/reviews/views.py (Diff revision 2)

It'd be nicer to store the generated filename in a 'filename' variable, and then set the 'Context-Disposition' to this after you've figured out what the filename is.

A couple issues:

1) bugs_closed may contain bugs separated by ", " instead of just ",". Whitespace should be taken into account. You should use review_request.get_bug_list(), which does all this.

2) If there's no bugs listed, this will be "bug.diff", which isn't very helpful. We'd need a better default.

3) Space after the comma in replace().

4) Make sure this is wrapped to less than 80 lines.

david Jan. 18, 2009, 3:04 p.m.

Forgive me, but isn't bugs-closed not necessarily unique? Multiple reviews may all address the same larger bug, and different revisions of diffs on the same review will definitely conflict.

Wouldn't <review request id>-<diff revision>.diff be a better format?

chipx86 Jan. 18, 2009, 3:08 p.m.

Probably. I wasn't completely comfortable with bug numbers either. Seems it would be useful in some installs, but something like the ID + revision, or somehow including the summary (though that could be pretty long) would be better.

It would also be nicer to have post-review come up with smarter filenames. But that's a separate thing.

onkarshinde Jan. 19, 2009, 3:47 p.m.

My reason for using bug numbers was to make it as unique as possible and easy to find when going through bunch of patches saved in a directory. Because I am more likely to remember a bug number when searching a patch than remembering review id.
I agree that having more than one review requests using same bug number will cause conflicts. Please let me know what approach should I follow in this case.

Meanwhile I am addressing Christian's comments.

david March 11, 2009, 7:14 p.m.

Christian and I discussed this, and I think both of us believe that <review id>-<diff id>.diff (or .patch) is the best option. Bug numbers are tempting, but given their non-uniqueness, especially with multiple diff revisions, I prefer something that maps more directly to what's in review board.

Admittedly, bug numbers may be easier to remember if you're looking at your bug list, but it's also not easy to get from patch -> review request with them, which I think is probably important if you're digging through a directory of patches.

chipx86 March 11, 2009, 7:36 p.m.

Nod. And the diff ID should be the revision, rather than the actual ID of the entry in the database.

Diff:

Revision 3 (+7 -4)

Show changes

/trunk/reviewboard/reviews/views.py