Christian, It's great to hear this issue is being taken seriously.
While we wait for UI improvements, could the documentation be updated to
explain the correct usage?
Specifically, could the information in my "how to submit diffs" attachment
be added to this page:
http://www.reviewboard.org/docs/manual/dev/users/review-requests/creating/#review-requests-for-diffs
Brian, I'm also attaching a list of the changes I made.
I created the diff manually just to illustrate what was modified.
Thanks
Will
On Tuesday, 4 December 2012 07:29:41 UTC, Christian Hammond wrote:
>
> Hi Brian,
>
> I'm sorry you took the answer of "use post-review" as a kind of "this is
> the only solution because we don't want to work on this page" answer. I do
> think there's much room for improvement on this page.
>
> I'd like to go over what I see as a couple of the main challenges we face
> that prevents this from being as smooth today as we all like. I hope you'll
> understand that this isn't always trivial and requires some thought. (Plus
> there's lots of room for improvement throughout the product, and we're
> trying to get to what we can.)
>
> 1) Depending on the revision control system, diffs need to be generated
> from post-review anyway. Not everything generates a diff with enough
> information to be able to locate the correct revision to fetch, which is
> needed for generating the side-by-side diff.
>
> Now, for things like Git, we're (generally) good, assuming --full-index
> was passed to 'git diff'. Subversion works okay too, so long as you know
> what to put for the base diff path (another issue we have to work around).
> For others, like Perforce, we actually have to generate a diff on the
> user's behalf, because a standard Perforce diff isn't good enough.
>
> This is one of the main reasons we strongly encourage using post-review.
> We have a better chance of being able to separate user error
> from compatibility problems or something else. So we always recommend it.
>
> 2) It would be awesome to verify a diff first. We can't reliably do that
> right now.
>
> A diff can be large, and many are. A diff touching 30 files means we have
> to fetch 30 files and attempt a patch. If the repository is at all being
> slow, or the files are large, this can easily take longer than the diff
> timeout period. So an upload would appear to be slow, and a would sometimes
> just fail. Furthermore, it would block a server thread for a while.
>
> In the early days, we had this problem with the diff viewer. We had to
> change things to dynamically fetch each diff, one at a time.
>
> Down the road, what we would like to do is be able to offload diff
> processing, but that still won't solve this problem directly. We may want
> to consider a whole new UI at that point, because then, we won't even be
> able to verify a diff right away (but we could present some diff patching
> error as some banner on the review request before it's published). Things
> to consider.
>
>
> Now, we all want this page to be better, and I try to think about it on
> occasion. Honestly, I do think the only consistent solution is better tools
> and integration, because we can't always guarantee a user's diff will work
> otherwise (see #1 above).
>
> Better education and per-repository usage information would help a lot,
> and I'd like to do this for 1.7.x (it won't happen for 1.7.0). For example,
> select Git and it'll tell you how to generate the diff. Select Perforce,
> and it'll tell you you must use post-review. Things like that.
>
> One of our big next tasks is good DVCS support, which will mean better
> integration with forks of repositories. I'm hoping some of that will make
> this nicer, but that work hasn't started yet.
>
> Christian
>
>
> --
> Christian Hammond - chi...@chipx86.com <javascript:>
> Review Board - http://www.reviewboard.org
> VMware, Inc. - http://www.vmware.com
>
>
> On Mon, Dec 3, 2012 at 10:52 PM, Brian Armstrong
> <kf7...@gmail.com<javascript:>
> > wrote:
>
>> I think the interface for uploading diffs is very minimalistic and vague
>> about what to do. The first time, without fail, any of our developers have
>> tried to use the upload diff interface it has failed to work properly and
>> results in an error about not being able to apply the diff or being unable
>> to find the parent revision. This has led to me spending time with each of
>> these developers to train them how to properly upload diffs. I'd say
>> something there is very broken if it doesn't work like people are expecting
>> it to work and it doesn't have a clear explanation of what it is expecting.
>>
>> Also, if the diff can't be applied properly, it should show an error on
>> the upload screen about it. They shouldn't have to click "view diff" every
>> time to make sure it uploaded correctly.
>>
>> I think saying "the fix for the confusing interface is to not use the
>> interface and to use the command line tool instead" is not actually a
>> solution to the problem. It is also not an option for everyone (like our
>> Skins team who just does CSS and Javascript and is scared of the command
>> line).
>>
>> We have them using reviewboard, but they will only upload their diffs
>> manually. They have run into this same kind of problem, and it just makes
>> the tool "overly complicated" and "not worth the time to learn".
>>
>> Giving this kind of "solution" to a problem is not only insufficient, but
>> it also shows that the problem is real and the tool is lacking in
>> functionality and clearness. If it's not understandable and usable, then
>> it's broken. If it's broken, then it needs to be fixed. The correct "fix"
>> should not be "use something else".
>>
>> That being said, it is open source, so I'd love to see the diff of
>> changes that Will made. They'd probably be helpful for our users too.
>>
>>
>> On Friday, November 30, 2012 3:53:20 PM UTC-7, David Trowbridge wrote:
>>
>>> It's correct to upload full diffs. The best way to do this is to use the
>>> post-review tool, which (with no arguments) will do the right thing.
>>>
>>> -David
>>>
>>>
>>> -David
>>>
>>>
>>> On Fri, Nov 30, 2012 at 2:49 PM, Will <ultra...@gmail.com> wrote:
>>>
>>>> So... don't any users know how whether it's correct to upload full
>>>> diffs or partial diffs?
>>>>
>>>>
>>>>
>>>> On Thursday, 22 November 2012 12:44:25 UTC, Will wrote:
>>>>>
>>>>> Where is it described in the documentation the correct way to make
>>>>> several diffs and upload them incrementally?
>>>>>
>>>>> We had a situation where one of our developers only uploaded partial
>>>>> diffs each time (just the changes since the last time he uploaded a
>>>>> diff),
>>>>> meaning there was no way to see his complete set of diffs together.
>>>>> I don't blame him because how was he supposed to know not to do that?
>>>>> We ended up hacking some extra instructions into the "add review"
>>>>> template and marking them bold red to try and prevent people doing this.
>>>>>
>>>>> ReviewBoard assumes all diffs are complete (from first commit to
>>>>> last), and it figures out the rest, allowing reviewers to easily drill
>>>>> down
>>>>> by revision if they want.
>>>>> If any of the uploaded diffs are not complete, then reviewers can
>>>>> completely miss changes that were made.
>>>>>
>>>>> a) where is the "right" way to add diffs documented?
>>>>>
>>>>> b) shouldn't reviewboard make it a lot more difficult to upload diffs
>>>>> "wrongly"
>>>>>
>>>>> --
>>>> Want to help the Review Board project? Donate today at
>>>> http://www.reviewboard.org/**donate/<http://www.reviewboard.org/donate/>
>>>> Happy user? Let us know at
>>>> http://www.reviewboard.org/**users/<http://www.reviewboard.org/users/>
>>>> -~----------~----~----~----~--**----~----~------~--~---
>>>> To unsubscribe from this group, send email to reviewboard...@**
>>>> googlegroups.com
>>>>
>>>> For more options, visit this group at http://groups.google.com/**
>>>> group/reviewboard?hl=en<http://groups.google.com/group/reviewboard?hl=en>
>>>>
>>>>
>>>>
>>>
>>> --
>> Want to help the Review Board project? Donate today at
>> http://www.reviewboard.org/donate/
>> Happy user? Let us know at http://www.reviewboard.org/users/
>> -~----------~----~----~----~------~----~------~--~---
>> To unsubscribe from this group, send email to
>> reviewboard...@googlegroups.com <javascript:>
>> For more options, visit this group at
>> http://groups.google.com/group/reviewboard?hl=en
>>
>>
>>
>
>
--
Want to help the Review Board project? Donate today at
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~----------~----~----~----~------~----~------~--~---
To unsubscribe from this group, send email to
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/reviewboard?hl=en
How to submit diffs to ReviewBoard.
IMPORTANT: The diff you upload must be a complete diff every time, from your
first commit to your last (not just the changes since the last diff).
ReviewBoard takes care of the rest. This method allows reviewers to drill down
and split up diffs the way they want, but keeps the default view as the
complete set of changes from first to last.
First diff:
O----O-----------master------------------------>
|
|
O--feature-branch---O----O
^ ^
| |
From here To here
Second diff:
O----O-----------master------------------------>
|
|
O--feature-branch---O----O----O----O
^ ^
| |
From here To here
Third diff:
O----O-----------master------------------------>
|
|
O--feature-branch---O----O----O----O----O----O
^ ^
| |
From here To here
diff reviewboard/templates/reviews/new_review_request.html
60a61,64
> <tr>
> <td></td>
> <td class="help" colspan="2">Currently only admins can add new
> repositories. Ask on #dc2a</td>
> </tr>
92c96,97
< <td class="help" colspan="2">{{form.diff_path.help_text}}</td>
---
> <td class="help" colspan="2">{{form.diff_path.help_text}}<br>
> <span style="color: black; font-weight: bold; text-decoration:
> underline;">IMPORTANT: The diff you upload must be a complete diff every
> time, from your first commit to your last (not just the changes since the
> last diff). ReviewBoard takes care of the rest.</span></td>
96c101
< <td>{{form.parent_diff_path}}</td>
---
> <td>{{form.parent_diff_path}}<br>If you don't know what this is, you
> probably don't need it</td>
diff reviewboard/templates/reviews/review_request_box.html
39c39,44
< <td class="value" id="branch-value-cell"><span id="branch"
class="editable">{{review_request_details.branch|safe}}</span></td>
---
> <td class="value" id="branch-value-cell"><span id="branch"
> class="editable">{{review_request_details.branch|safe}}</span>
> {% if request.user == review_request.submitter or
> perms.reviews.can_edit_reviewrequest %}
> <span class="docs">(Please fill in the branch so people can check out
> your code and run the tests)</span></td>
> {% endif %}
45c50
< {% endspaceless %}</span></td>
---
> {% endspaceless %}</span> <span class="docs">(Groups/people are used only for
> notifications, not access control: Anyone can see any review)</span></td>
61c66
< {% endspaceless %}</span></td>
---
> {% endspaceless %}</span> <span class="docs">(Currently only admins can
> create new groups or add people to a group)</span></td>
75c80
< <label for="testing_done">{% trans "Testing Done" %}:</label>
---
> <label for="testing_done">{% trans "Testing Done" %}:</label> <span
> class="docs">(You did run some tests, didn't you?)</span>
diff reviewboard/reviews/forms.py.orig
102c102,103
< help_text=_("The new diff to upload."),
---
> help_text=_("The new diff to upload (use 'git diff [your first
> commit]' to generate this)."),
