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 - chip...@chipx86.com
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> 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+unsubscr...@googlegroups.com
> 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


Reply via email to