Re: It's too easy to upload diffs "wrongly"
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 > Review Board - http://www.reviewboard.org > VMware, Inc. - http://www.vmware.com > > > On Mon, Dec 3, 2012 at 10:52 PM, Brian Armstrong > > > 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 proble
Re: It's too easy to upload diffs "wrongly"
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 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
RE: It's too easy to upload diffs "wrongly"
You do not need them to run post-review from the command line themselves. Our users work on windows (server is on unix) and I have a script that does all the "hard" work. All the users need to do is right click on the main source folder and choose between these two options (added to the list via a registry key): - Review request - Update review request The former will create the review request without any additional action(s) required by the user, and the latter will ask for the review request number to update. Easy. At least depending on what your needs are. Regards, Elardus Elardus Erasmus DigiCore Technologies (Pty) Ltd Embedded Software Engineer [cid:imageafc275.JPG@f8006872.59244af5] Tel: +27 12 450 2272 * Fax: +27 12 450 2311 Email: elard...@digicore.co.za * Website: www.ctrack.co.za<http://www.ctrack.co.za/> * Email Disclaimer<http://www.ctrack.co.za/about_ctrack/email_signature_disclaimer.aspx> From: reviewboard@googlegroups.com [mailto:reviewboard@googlegroups.com] On Behalf Of Brian Armstrong Sent: 04 December 2012 08:52 To: reviewboard@googlegroups.com Subject: Re: It's too easy to upload diffs "wrongly" 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 > 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/ Happy user? Let us know at 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 -- Want to help the Review Board project? Donate today at ht
Re: It's too easy to upload diffs "wrongly"
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 >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/ >> Happy user? Let us know at 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 >> >> >> > > -- 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
Re: It's too easy to upload diffs "wrongly"
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 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/ > 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
Re: It's too easy to upload diffs "wrongly"
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/ 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
