Bug 116427 - [Patch] No check for patch existence when creating patch while another one is in progress
Summary: [Patch] No check for patch existence when creating patch while another one is...
Status: VERIFIED FIXED
Alias: None
Product: Platform
Classification: Eclipse Project
Component: CVS (show other bugs)
Version: 3.2   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: 3.3 M7   Edit
Assignee: Krzysztof Daniel CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2005-11-15 07:00 EST by Philipe Mulet CLA
Modified: 2007-06-05 15:17 EDT (History)
0 users

See Also:


Attachments
Solution proposal (11.66 KB, patch)
2007-03-16 08:54 EDT, Krzysztof Daniel CLA
no flags Details | Diff
Michael suggestion applied (8.38 KB, patch)
2007-03-16 13:23 EDT, Krzysztof Daniel CLA
no flags Details | Diff
Solution proposal 2 (9.02 KB, patch)
2007-03-19 05:53 EDT, Krzysztof Daniel CLA
no flags Details | Diff
Solution proposal 3 (8.17 KB, patch)
2007-03-19 10:22 EDT, Krzysztof Daniel CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philipe Mulet CLA 2005-11-15 07:00:45 EST
Build 3.2m3

I did initiate a patch creation, which was going to take a while. I told it to
output to a file: 'patch1' in the filesystem (outside workspace). 

While it performed, I noticed I had forgotten to include on project in the
patch; thus I reselected in package explorer, and reissued order to create
patch, using same output file 'patch1'.

The second operation did not prompt me about existing file with same name
(likely because first action hadn't reached the point yet where the file got
created.

It feels like the second patch should notice collision with first ongoing job,
and offer to cancel it or wait.
Comment 1 Bogdan Gheorghe CLA 2005-11-15 08:36:24 EST
>The second operation did not prompt me about existing file with same name
>(likely because first action hadn't reached the point yet where the file got
>created.

Correct. Filed under the TODO list for 3.2...
Comment 2 Krzysztof Daniel CLA 2007-03-16 08:54:33 EDT
Created attachment 61080 [details]
Solution proposal
Comment 3 Michael Valenta CLA 2007-03-16 10:58:59 EDT
The patch seems to have the right idea in the sense that it is using the destination as the means to detect conflicting jobs. However, there is already an existing mechanism for this which we refer to as Job families. On TeamOperation, there is a method called belongsTo which is passed a job family. When the IJobManager#find method is called with an object, this object is passed to the belongs of method of all Jobs and the Job associated with a TeamOperation delegates the call to the same method of TeamOperation. So, for instance, if you passed your clipboard object or the target file object to the Job#find method and implemented the belongsTo method in the DiffOperation, you should be able to find the specific job that needs to be canceled.
Comment 4 Krzysztof Daniel CLA 2007-03-16 13:23:10 EDT
Created attachment 61131 [details]
Michael suggestion applied
Comment 5 Michael Valenta CLA 2007-03-16 13:59:22 EDT
That looks much better. However, there is still a few issues. First of all, there is a new constant in the CVSUIMessages that is not used (CVSTeamProvider_errorAddingFileToDiff). Also, the prompt offers two choices: Terminate and Wait but the wait doesn't actually wait. It just continues. Perhaps it would be better to present the user with a message that says something like "There is already a Create Patch operation running that is writing the output to X. Continuing will cancel that operation." and give them the options of Yes or No (the X is either "the clipboard" or the file path). If the user chooses No, you can just throw an OperationCanceledException to get out of the new operation. Finally, I don't really like doing the prompt in the constructor. Perhaps you could put it in a separate method and invoke it after you create the diff operation but before you run it.
Comment 6 Krzysztof Daniel CLA 2007-03-19 05:53:00 EDT
Created attachment 61261 [details]
Solution proposal 2

User prompting moved to 'shouldRun method'.
Comment 7 Michael Valenta CLA 2007-03-19 09:55:41 EDT
The patch looks like it is almost complete. However, there are a couple of changes in DiffOperation that appear to be unrelated and are removing error text. Is that intentional? Also, there is a convenience method on the job manager for canceling all the jobs in a given family (IJobManager#cancel) so you could use thatinstead of looping.
Comment 8 Krzysztof Daniel CLA 2007-03-19 10:22:27 EDT
Created attachment 61283 [details]
Solution proposal 3

I hope now everything will be fine.

Michael, is  mylar context welcomed, or I should avoid putting it on bugzilla?
Comment 9 Michael Valenta CLA 2007-03-19 10:45:09 EDT
Looks good. We're entering out lock-down phase for 3.3 M6 so I'll verify and commit it once M6 is done (either the end of this week or early next week).

As for Mylar contexts, you can go ahead and attach them if you like.

Comment 10 Michael Valenta CLA 2007-03-21 11:57:54 EDT
Patch released with some updates to the messages.
Comment 11 Michael Valenta CLA 2007-05-01 10:17:40 EDT
Verified in I20070501-0010