Community
Participate
Working Groups
Today we're having CVS server problems at Eclipse.org but I could not know this. When I created a patch of a large local changeset I only realized by accident that the resulting patch is way too small. The patch wizard did silently ignore the problems and gave me a corrupt patch. My work of two days was almost lost ;-( Here is the beginning of the wrong patch: ### Eclipse Workspace Patch 1.0 #P org.eclipse.emf.cdo #P org.eclipse.emf.cdo.common #P org.eclipse.emf.cdo.common.db #P org.eclipse.emf.cdo.net4j #P org.eclipse.emf.cdo.server #P org.eclipse.emf.cdo.server.db #P org.eclipse.emf.cdo.server.hibernate #P org.eclipse.emf.cdo.server.net4j #P org.eclipse.emf.cdo.server.objectivity #P org.eclipse.emf.cdo.tests Index: src/org/eclipse/emf/cdo/tests/DetachedCDORevisionConsistencyTest.java =================================================================== RCS file: src/org/eclipse/emf/cdo/tests/DetachedCDORevisionConsistencyTest.java diff -N src/org/eclipse/emf/cdo/tests/DetachedCDORevisionConsistencyTest.java --- /dev/null 1 Jan 1970 00:00:00 -0000 +++ src/org/eclipse/emf/cdo/tests/DetachedCDORevisionConsistencyTest.java 1 Jan 1970 00:00:00 -0000 @@ -0,0 +1,321 @@ ... I'll also attach screenshots of the sync view and the error dialog that only showed up when I forced a full resync of the workspace.
Created attachment 174117 [details] Synchronize View
Created attachment 174118 [details] Errors on Full Sync
If all projects are on the dead server then you get a 'No Difference' dialog. Nothing is written to the .log. >Today we're having CVS server problems at Eclipse.org See bug 319659.
The underlying CVS server problem seems to be tracked in bug 319659
Well, it's strange: The only changes that were written to the patch are from my tests project. And that's on the same server as the rest. Hmm...
(In reply to comment #0) > When I created a patch of a large local changeset I only realized by accident > that the resulting patch is way too small. The patch wizard did silently ignore > the problems and gave me a corrupt patch. Maybe it was not a problem with the server but you became another victim of bug 294650.
No, I tried it as well. All changes that were on a project connected to dev.eclipse.org were simply not added to the patch and no .log entry.
(In reply to comment #6) I can also confirm that I always press "Select All" since I'm aware of the selection problem.
Created attachment 179729 [details] Added description to error message The bug was mostly fixed by bug 123430, I attached a patch that adds more information to error message displayed when there is a server connection problem.
Gosia, with your fix applied I tried to create patch for two projects. For one of them I had manually removed a file from my local repo. To my surprise I got no error from the wizard. I guess I would get the 'No Difference' dialog from comment 3 if all the files were corrupted. I think we should display the same error dialog you tweaked in your patch in that case. Here is the patch I got: ### Eclipse Workspace Patch 1.0 #P bar Index: barfile.txt =================================================================== RCS file: /BUGS/bar/barfile.txt,v retrieving revision 1.13 diff -u -r1.13 barfile.txt --- barfile.txt 9 Jun 2010 12:21:01 -0000 1.13 +++ barfile.txt 28 Sep 2010 13:51:09 -0000 @@ -1,4 +1,4 @@ -line1 +line1a line2 line3 line4 #P bug319661_brokenPatch
Created attachment 181164 [details] Added error handling of server errors. The problem was that DiffListener ignored the server errors. I've added handling server errors in DiffListener and displaying them in DiffOperation. The only problem is that DiffOperation gets CVSErrors on problems that don't affect the patch content (i.e. conflicts) and they still need to be ignored. I've added displaying only the errors types that can be returned by DiffListener and did some extended testing on typical situations such as conflicts, new files etc and only meaningful errors display. BTW I think that my previous patch can be imported as well, to clarify to the user the consequences of server error.
Gosia, are you sure that when an error line is encountered while executing the diff operation it will *always* result in the patch being incomplete?
Created attachment 182398 [details] Added error handling of server errors and corrected message Tom, you're right, we may get an error from the diff operation when all the changes have been included in patch, the simplest example is when user did not made any changes in file that cannot be retried from server. However I still believe that user should be informed that errors occurred. I've modified the error message, informing that errors occurred and patch <b>may</b> not contain all the changes.
As written in comment 9, the same bug got "almost" fixed before. In order not to do the same mistake again, can you please add steps to reproduce the problem, so that the fix can be verified during the test pass. In addition, a new test case should be written.
The steps to reproduce the problem are: 1. Create a project and commit it to your local repository (or any other that you have direct access to) 2. Commit a file from your workspace and then go straight to the repository on your filesystem and delete the file just committed 3. Go back to your workspace and make some changes to the file from point 2, and to any other file in this project 4. Try to create a patch containing all your changes Result: You get the patch that does not contain changes in file from point 2. No error is displayed to the user. Expected: An error is displayed to the user.
Gosia, I tend to agree with Dani that a test case would be a nice thing to have. I'm aware that modifying the filesystem of the repository could be tricky, but I guess it can be done once property for RSH[1] is properly set before running the test. [1] org.eclipse.team.tests.ccvs.core.CVSTestSetup.executeRemoteCommand(ICVSRepositoryLocation, String)
RSH contains command to remote shell, however not every system can be connected via remote shell, it has to have rsh demon running. I noticed that that this command is not used anywhere for primary repository defined in repository.properties file. There are some tests in org.eclipse.team.tests.cvs.core.compatible.* that connect repositories defined in "repository1" and "repository2" properties, but I'm not sure if they are still supported. They don't run in standard test pass and didn't have significant updates since 2002. Kim, do our test machines have any valid repositories in "repository1" and "repository2" properties that have rsh demon running? Can i use any of those repositories while running tests from my local machine? Would tests from org.eclipse.team.tests.cvs.core.compatible.* pass on all our test machines?
All the repository* values in org.eclipse.team.tests.cvs.core\repository.properties are replaced with the appropriate values for our test server while running the tests. The test cvs server doesn't have access enabled via rshd at this time.
Thanks Kim. In this case I can't use rsh to delete the file. I'll try a different approach: maybe generating fake CVS metadata will course a failure in generating patch.
I cautiously scheduled it for M6, however it would be good to have it fixed by M5. Gosia, Tomek any chance for that?
Created attachment 188030 [details] Added test It was hard to recreate an error situation in a test case without accessin the CVS server, so finally I overwrite Session in the way that it's always returning the same server response that I load from the file. It's a hack, but I can't think of a better way to make the server return a diff with an error line.
Created attachment 188902 [details] Added test (corrected version)
I think it's heading in the right direction but I have a few doubts: * if the server_response_with_error.txt is there only to create a PrintStream and emulate a server response there is no need to copy it to the workspace * do we know what is the exact form of the status when the error occurs? Is it CVSStatus.SERVER_ERROR and CVSStatus.ERROR_LINE for it's first child as in the test? Or, are these codes CVSStatus.PROTOCOL_ERROR and CVSStatus.ERROR_LINE as checked in DiffOperation? What about their parent? Is it supposed to be CVSStatus.SERVER_ERROR? We should be very precise her and make sure we surface only that particular error as other may be expected. But at the same time, we don't need to ignore any other form of the error. * in messages.properties I would adjust the wording from "difference operation" to "CVS diff operation" * don't forget to update copyrights
Created attachment 191620 [details] Fix with test I applied Tomek's comments. > * do we know what is the exact form of the status when the error occurs? Is it > CVSStatus.SERVER_ERROR and CVSStatus.ERROR_LINE for it's first child as in the > test? Or, are these codes CVSStatus.PROTOCOL_ERROR and CVSStatus.ERROR_LINE as > checked in DiffOperation? What about their parent? Is it supposed to be > CVSStatus.SERVER_ERROR? We should be very precise her and make sure we surface > only that particular error as other may be expected. But at the same time, we > don't need to ignore any other form of the error. The connection error is recondized as CVSStatus.SERVER_ERROR with CVSStatus.ERROR_LINE in it. To be more precise in the test I changed the logic, so that if there are more server errors we check if any of them is an ERROR_LINE.
Created attachment 191691 [details] Fix with test
2 final questions: 1) In DiffListener.errorLine[1] we treat any error with a message as something we should present to the user[2]. Are we sure it's true for all errors that a server can send us in response? We've been fine with ignoring all of them so far, except for this case obviously. 2) In [2] if we find a child with any of the error codes we will display a message saying "An error occurred while performing CVS diff operation, the patch may not contain all the changes". Not sure if this is true for the case we have already covered ie the code is CVSStatus.BINARY_FILES_DIFFER. Is it actually an error? Moreover, in that case we know for sure that the patch does not contain some changes. Maybe we should be more precise here and have separate messages for these cases. What do you think? [1] org.eclipse.team.internal.ccvs.core.client.listeners.DiffListener.errorLine(String, ICVSRepositoryLocation, ICVSFolder, IProgressMonitor) [2] org.eclipse.team.internal.ccvs.ui.operations.DiffOperation.handleCVSException(CVSException)
ad 1) In DiffListener.errorLine[1] we handle errors that are messages from server. When generating patch started and in the response we got errors I think we should always present it to the user, because it always carries the risk that the diff is invalid or incomplete. At least I can't think of any destination between errors that cause the patch to invalid/incomplete and that don't. I think that we could get by with ignoring those error only because it's an uncommon situation that patch is only partially created. Most common error caused the patch to be empty and this was reported. ad 2) I think situation when user selected some binary files that can't be included in a patch should rather generate a warning than an error, however I left it because this it the way it was handled before. We can display different messaged depending on the type of an error, but I would like to avoid situation when both situation occurred: binary file diff and server error and we display two separate message dialogs. I think as long as we have "details" section the message gives user all the necessary information. To be more precise we could change the general error message depending on what detailed error occurred.
>ad 2) I think situation when user selected some binary files that can't be >included in a patch should rather generate a warning than an error, however I >left it because this it the way it was handled before. Currently it shows up as warning. That should stay as is. Only in case of real server error I'd like to see the error dialog.
Created attachment 191747 [details] Fix with test (In reply to comment #27) > ad 1) [...] > ad 2) [...] Fair enough. > To be more precise we could > change the general error message depending on what detailed error occurred. I've done it for you. Here is your patch + the change above. (In reply to comment #28) > Currently it shows up as warning. That should stay as is. The patch preserves the current behavior.
Created attachment 191748 [details] mylyn/context/zip
Created attachment 191749 [details] The previous patch - launch configuration Oops.
The latest patch has been applied to HEAD. Available in build >=N20110323-2000. Thanks Gosia!
Created attachment 192352 [details] Code hack to trigger the problem
Unfortunately the fix is not working. You can verify this easily: 1. Apply the attachment 192352 [details] to HEAD. 2. Start target. 3. Check out a project from CVS and make same changes. 4. Add a new file with some text. 4. Create a patch for that project. ==> The patch only contains the data for the new file but not the changes.
.
What Dani has pointed out is much wider issue than just Create Patch action. This applies to every CVS Operation. Javadoc to CVSOperation.isReportableStatus() says " Return whether the given status is reportable. By default, only server errors are reportable. Subclasses may override.". Ignoring some errors has been introduced in Bug 79553. I don't know the historical context to well, but it seems that since 2004 we are ignoring errors that don't come from the server, and it did not caused errors as long as CVS operations weren't overwritten. I think that this can trigger a greater discussion that has almost nothing to do with this bug, so I suggest to open a separate bug for it.
(In reply to comment #36) > What Dani has pointed out is much wider issue than just Create Patch action. > This applies to every CVS Operation. > Javadoc to CVSOperation.isReportableStatus() says " Return whether the given > status is reportable. By default, only server errors are reportable. Subclasses > may override.". Ignoring some errors has been introduced in Bug 79553. I don't > know the historical context to well, but it seems that since 2004 we are > ignoring errors that don't come from the server, and it did not caused errors > as long as CVS operations weren't overwritten. From a bird's eye view I would claim that this seems just wrong. Gosia, do you know exactly what exception got thrown that triggered this bug?
> Gosia, do you know exactly what exception got thrown that triggered this bug? With "this bug" I mean the concrete patch wizard bug we try to fix here.
(In reply to comment #37) > Gosia, do you know exactly what exception got thrown that triggered this bug? SERVER_ERROR, we even have a test verifying this situation. > From a bird's eye view I would claim that this seems just wrong. That's why I propose you to open a different bug to start a discussion about that. The "hack" you attached throws an error before creating patch even starts, and this bug is related to a more precise problem in diff operation, I don't think we should block this fix by it.
OK, let's assume(In reply to comment #39) > (In reply to comment #37) > > Gosia, do you know exactly what exception got thrown that triggered this bug? > SERVER_ERROR, we even have a test verifying this situation. I see now. Actually the diff operation even reports that error code if it finds a diffs, so the place where I added my hack was not suitable. Where would be a suitable place to throw the exception so that I can test that the fix is good? I tried org.eclipse.team.internal.ccvs.core.client.Session.readLine() but that also results in an incomplete patch plus no error dialog.
(In reply to comment #40) > Where would be a suitable place to throw the exception so that I can test that > the fix is good? I tried > org.eclipse.team.internal.ccvs.core.client.Session.readLine() but that also > results in an incomplete patch plus no error dialog. Please view how is it done in CreatePatchTest#testBug319661. You can throw SERVER_ERROR exception in DiffOpperation or include an error line in session response, how it's done in attached test. If you want to go below DiffOpperation then you should throw one of those 3 errors: CVSStatus.BINARY_FILES_DIFFER, CVSStatus.PROTOCOL_ERROR, CVSStatus.ERROR_LINE, that will be wrapped in SERVER_ERROR by DiffOpperation. The one that wasn't handled well and caused this bug was ERROR_LINE.
Thanks for the pointers. I was able to trigger the problem (see new code hack). The fix has two issues: 1. The dialog is shown for each changed file instead of a report at the end. If the (server) problem happens when you create a patch for a project with many changes you will be forced to click away that dialog many times without having any means to abort the operation. The issues should be collected and shown together, like it's done when synchronizing. 2. The error dialog is opened without a title and without a message and hence the message you set in DiffOperation does not appear (try the new code hack).
Created attachment 192455 [details] Code hack to trigger the problem
Created attachment 192525 [details] Code hack to trigger the problem with multistatus (In reply to comment #42) Actually you may have this impression from the way multistatus error is displayed: when multistatus contains only one subordinate status the dialog looks like just a single status. It uses the multistatus additional message when there is more than one subodrinate status. I modified your hack to contain more errors and you'll see that you get a multistatus error dialog with everything in order.
> I modified your hack to contain more errors and you'll see that you get a > multistatus error dialog with everything in order. Yes, I know that. However, it's still better to also set the title and the message (it doesn't hurt ;-). Also, any title is better than "Problem Occurred". In Eclipse we normally use the action/operation name as dialog title, e.g. "Create Patch".
Created attachment 193008 [details] Error dialog with title and message. > Yes, I know that. However, it's still better to also set the title and the message (it doesn't hurt ;-). Also, > any title is better than "Problem Occurred". In Eclipse we normally use the action/operation name as > dialog title, e.g. "Create Patch". This patch adds an error message and a title to the error dialog.
(In reply to comment #45) > > I modified your hack to contain more errors and you'll see that you get a > > multistatus error dialog with everything in order. > Yes, I know that. However, it's still better to also set the title and the > message (it doesn't hurt ;-). Also, any title is better than "Problem Occurred". > In Eclipse we normally use the action/operation name as dialog title, e.g. > "Create Patch". Dani, can you confirm that my last patch addressed this issue?
The patch "works" but it the implementation is not good as it puts the strings into the core plug-in. You should put and take the strings from org.eclipse.team.internal.ccvs.ui.CVSUIMessages. One thing I noticed but I'm not 100% sure whether this comes from my hack or whether there's still a problem with the fix: I first get the error dialog (good) but then I get a dialog saying: "No difference found".
Created attachment 193808 [details] Error dialog with title and message - strings from CVSUIMessages
(In reply to comment #48) > The patch "works" but it the implementation is not good as it puts the strings > into the core plug-in. You should put and take the strings from > org.eclipse.team.internal.ccvs.ui.CVSUIMessages. I corrected the patch and moved messages to CVSUIMessages > One thing I noticed but I'm not 100% sure whether this comes from my hack or > whether there's still a problem with the fix: I first get the error dialog > (good) but then I get a dialog saying: "No difference found". We display this "No difference found" message independently from error messages, because if there were errors while creating the patch it doesn't necessarily mean that patch is empty, and vice versa - there were no errors while creating the patch, but patch is empty (there were no differences in selected files). Note that "No difference found" is not an error but an information. If there were some errors while creating patch user may want to check what content is missing and thanks to this information he knows not to bother.
> We display this "No difference found" message independently from error > messages, because if there were errors while creating the patch it doesn't > necessarily mean that patch is empty, and vice versa - there were no errors > while creating the patch, but patch is empty (there were no differences in > selected files). Note that "No difference found" is not an error but an > information. If there were some errors while creating patch user may want to > check what content is missing and thanks to this information he knows not to > bother. This is too misleading and showing 2 dialogs in a row is bad style. Assume it fails to connect to 2 out of 7 projects. Then at the end I get the "No difference." dialog. Does that mean the error dialog was wrong? Is the result correct i.e. are there really no differences? The 99% case is that if someone wants to make a patch from n projects and 1 fails then the patch is useless anyway and has to be recreated. I agree that the "No difference." dialog is useful if there is no error.
Comment on attachment 193808 [details] Error dialog with title and message - strings from CVSUIMessages (In reply to comment #49) > Created attachment 193808 [details] > Error dialog with title and message - strings from CVSUIMessages This patch is good. I committed it together with also moving CVSMessages.CVSTeamProvider_errorAddingFileToDiff to CVSUIMessages.
Created attachment 193909 [details] Added handling of Empty patch information I change handling the "empty patch" information. When we have and empty patch and error occurred I added it to the error dialog. This way user is still informed that his patch is empty, but doesn't get second dialog. For situations if there were no error but patch is empty I left it as was: information dialog is displayed.
I looked at the patch. While technically OK, I still believe the additional information is useless if not confusing. Can you tell me what you think the benefit of this information is if I create a patch for projects 1..n and j and k fail but this is not shown but it says: 'No differences'. Unless I know exactly to which resources the 'no differences' applies, the information is useless to me. We should simply avoid the additional message/dialog. Given that 'No difference' dialog is nothing new, I'm also OK to mark this bug here as FIXED and open a new one which we can tackle during 3.8.
> Given that 'No difference' dialog is nothing new, I'm also OK to mark this bug > here as FIXED and open a new one which we can tackle during 3.8. I'm marking this bug FIXED, and the bug for the information message is Bug 343658.
