Community
Participate
Working Groups
3.6 RC4 and also 3.5.2. The 'Consider file contents in comparison' option on Team > CVS > Synchronize/Compare is completely broken: 1. it does not work (see bug 62547) 2. it is enabled by default but the doc (F1) says other: it says that by default timestamps are used
Let me start from saying that I was able to reproduce the behavior described in bug 62547, comment 6. I do admit the result surprised me, but looking at the description for the 'Consider file contents in comparison' option you can see that it explicitly says it does not apply for workspace syncs[1], so I guess the current behavior is expected. I'm not saying I don't see an issue here, but I would convert this into a clarify-the-doc bug (not major). As for the default state, in the Default column it's clearly stated the option is enabled by default, but again the description may be confusing as it contains this sentence: "Usually, time stamps are used to compare CVS files, and this is by far the fastest method." [1] Last sentence from the doc: "This option only applies to comparisons and merges but not Workspace synchronizations."
I disagree. The F1 help says for this option says: Sets the default compare method for comparing and synchronizing CVS resources. ==> It is 100% expected that this works for CVS resources. Anyway, who says that the doc is the master? ;-) For me it makes absolute no sense that this option is not working for workspace syncs.
Having bug 62547 fixed we will need a new set of reproducible steps. Those from bug 62547, comment 6 do not indicate any problem now.
(In reply to comment #3) > Having bug 62547 fixed we will need a new set of reproducible steps. Those from > bug 62547, comment 6 do not indicate any problem now. Use 3.7 M5 to create the test setup/workspace. Then exit and start with latest build or HEAD to reproduce the comparison bug. > you can see >that it explicitly says it does not apply for workspace syncs[1], so I guess >the current behavior is expected. Yes, I saw that but as noted before, e.g. in bug 62547 comment 6, it also doesn't work when doing an explicit manual compare (with) on the file. Just an idea: maybe that option got broken when compare results were moved into the Synchronize view? NOTE: I can accept that the option isn't used to synchronize the workspace though that restriction was probably only added for performance reasons. In that case we should improve the option name and use "Compare (actions)" instead of "comparisons".
(In reply to comment #4) > Yes, I saw that but as noted before, e.g. in bug 62547 comment 6, it also > doesn't work when doing an explicit manual compare (with) on the file. I looked at the code (HEAD and archived branches including 3.2) for references to the property from the summary. My conclusion is that it was never supposed to work for the kind of comparisons you mentioned. Instead, the operations expected to be faster by ignoring file contents are: * comparing two remote files - CVSCompareEditorInput * comparing two remote folders - RemoteCompareOperation * comparing two tags (Compare with Another Branch or Version), but only when "Allow models" is off > Just an idea: maybe that option got broken when compare results were moved into > the Synchronize view? This is partly true, Comparing with Another Branch or Version will display the expected result in the Sync view if you turn models off. Steps: 1. Create a project containing 2 files with random contents. 2. Share, commit and tag the project. 3. Modify both of the files, keep the original content of one of them in clipboard. 4. Commit the changes 5. Revert the changes for one of the files (undo, copy from clipboard, get contents from history) 6. Commit the file, contents of rev 1.1 should be the same as 1.3 7. Turn off "Allow Models" option 8. Compare with tag created in 2. => Only one of the files should be displayed 9. Now, turn off "Consider file contents" option 10. Compare with tag created in 2. => This time you will see both files in the Sync view > [...] In that case we should improve the option name and use "Compare (actions)" instead of > "comparisons". I totally agree the wording should be adjusted. This applies to the doc as well.
> I looked at the code (HEAD and archived branches including 3.2) for references > to the property from the summary. My conclusion is that it was never supposed > to work for the kind of comparisons you mentioned. Instead, the operations > expected to be faster by ignoring file contents are: > * comparing two remote files - CVSCompareEditorInput > * comparing two remote folders - RemoteCompareOperation > * comparing two tags (Compare with Another Branch or Version), but only when > "Allow models" is off And how about the default of the preference? Does it really compare the contents for the above mentioned cases by default or is it the other way around like mentioned in the F1 help: "By default timestamps are used to compare CVS files"?
The preference is Enabled by default, so it does compare the file contents in those cases. Please notice that I didn't touch the preference to get the expected result after 8. As for the F1 help, it says "Usually, time stamps are used to compare CVS files, [...]". IMU, this means that other CVS clients usually work like that, but in Eclipse we give you an option to go a step further and compare by contents, ignoring pseudo changes. What confuses me about the doc is the last sentence "This option only applies to comparisons and merges but not Workspace synchronizations.", which IMO needs some clarification.
(In reply to comment #7) > The preference is Enabled by default, so it does compare the file contents in > those cases. My question was rather: "is it also implemented like this"? ;-) ==> and Tomek confirmed: "yes". > As for the F1 help, it says "Usually, time stamps are used to compare CVS > files, [...]". Talked to Tomek: I looked at the F1 help of the page while he looked at the documentation of the page - another inconsistency. And I just found some more fun: if one takes my test case (bug 62547 comment 6) and then opens the History view, one can select the 1.31 tag > context menu > Compare Current with 1.31 ==> it shows the diff - and hey: it also shows it when the preference is disabled. lol. Given all this confusion and the fact that it was like that for quite some time we decided to only clarify the documentation for 3.7 (see bug 341114) and correctly implement the preference in 3.8.
Another case it does not work: follow the steps from comment 5, but select a single file and do "Compare with Another Branch or Version". A compare editor will open showing left and right side being identical. What should happen is: a message dialog with "No diffs" pops up, just like it does when comparing 1.3 and <tag> revisions of that file in CVS Repositories view.
(In reply to comment #8) > And I just found some more fun: if one takes my test case (bug 62547 comment 6) > and then opens the History view, one can select the 1.31 tag > context menu > > Compare Current with 1.31 > ==> it shows the diff - and hey: it also shows it when the preference is > disabled. lol. That's because the action opens the Compare Editor not the Sync view with a synchronize participant. I didn't know that when you filed the bug, but I've checked the code and now I can act like a smartass :). The pref in question is only respected by ComparePariticipant (for non-model comparisons) and CompareSubscriberContext (for model comparisons). While using it only in comparisons may be acceptable, we could consider consuming the pref in result of "Compare with Latest from HEAD". To my surprise, with the pref checked, the result is different when you compare with "Latest from HEAD" and "Another Branch or Version" > "HEAD". So, this is one thing I would fix. I haven't checked other compare actions like "Released". The other issue I found is ignoring the fact that the pref is off in model comparisons. Compare org.eclipse.team.internal.ccvs.ui.mappings.CompareSubscriberContext.getDiffFilter() and org.eclipse.team.internal.ccvs.ui.subscriber.CompareParticipant.setSubscriber(Subscriber) where in the latter the pref is checked for state first. I'm not sure what have we decided about respecting the pref in regular synchronizations. Another scenario worth considering is a sync showing merge preview, currently it ignores the pref. (In reply to comment #9) > Another case it does not work: follow the steps from comment 5, but select a > single file and do "Compare with Another Branch or Version". A compare editor > will open showing left and right side being identical. What should happen is: a > message dialog with "No diffs" pops up, just like it does when comparing 1.3 and > <tag> revisions of that file in CVS Repositories view. This could be a dupe of bug 318826.
Created attachment 204881 [details] Fix v01 (In reply to comment #10) > The other issue I found is ignoring the fact that the pref is off in model > comparisons. Compare [...].CompareSubscriberContext.getDiffFilter() > and [...].CompareParticipant.setSubscriber(Subscriber) A fix for this part.
Created attachment 204882 [details] mylyn/context/zip
Dani, is fixing "Compare with" actions (model and non-model) enough in your opinion to mark this bug as fixed? To be clear, these things that haven't been covered by the proposed patch, they ignore the pref from the summary: * synchronizations, results of Team > Synchronize, Synchronize from the Sync view toolbar etc * merge previews * "Compare with Latest from HEAD" which is in fact a synchronization As for the scenario from bug 62547, comment 6, mentioned here in comment 4, I'm not sure if the pref can be of any help. The sync info (sync bytes) says that the file is in sync, so we don't compare contents. However, if the info had indicated the file is out of sync, then we would have checked if this is actually true by comparing the contents (if the pref was on). In the same comment, you also suggested to rename "comparisons" to "Compare (actions)" on the pref page and in the doc. Do you still think it's necessary?
> To be clear, these things that haven't been covered by the proposed patch, they > ignore the pref from the summary: > * synchronizations, results of Team > Synchronize, Synchronize from the Sync > view toolbar etc > * merge previews > * "Compare with Latest from HEAD" which is in fact a synchronization Seriously, how would a "normal" user know this? I would assume that this is a remote comparison and hence the preference be honored. We either need to fix it so that the preference works or clarify the preference in a way that a user can deterministically know what is affected when he toggles that preference.
(In reply to comment #14) > We either need to fix it so that the preference works or clarify the preference I would go with the former, ie. honor the pref in all synchronizations regardless of the performance hit. If it hurts the user she can always turn off the pref. Twisting the doc sounds boring :) Jokes aside, I'm afraid no matter how hard I would try to clarify this in the doc, the user needs to read it first and then understand why "Compare with Latest from HEAD" is not a comparison. I doubt this would work. Green light for respecting the pref anytime? Dani, you reported the bug that's why I'm bugging you about it.
> I would go with the former, ie. honor the pref in all synchronizations > regardless of the performance hit. If it hurts the user she can always turn off > the pref. > > Green light for respecting the pref anytime? Dani, you reported the bug that's > why I'm bugging you about it. Discussed this with Tomek over Sametime: implementing this will take quite some time and also require time for testing. In addition, it could introduce performance hits for large remote synchronizations. Given the future is Git, we should only adjust the preference label and the doc. How about inverting the logic of the label ==> [ ] Only look at timestamps to detect changes That way, we don't talk about contents in the UI. We can then explain in the doc in which cases/actions we use the contents if the preference is disabled (which is the default).
(In reply to comment #16) > How about inverting the logic of the label > ==> [ ] Only look at timestamps to detect changes I like the new label, but this is going to be a new preference and I get goosebump thinking about any migration story. Even as tiny as this one. However, if I don't figure out a better label, I will go with yours.
(In reply to comment #17) > (In reply to comment #16) > > How about inverting the logic of the label > > ==> [ ] Only look at timestamps to detect changes > > I like the new label, but this is going to be a new preference and I get > goosebump thinking about any migration story. Even as tiny as this one. > However, if I don't figure out a better label, I will go with yours. I wasn't thinking of adding a new preference. Just invert the UI for it.
(In reply to comment #18) > Just invert the UI for it. You mean something like this (excerpt from ComparePreferencePage.createFieldEditors()): addField(new BooleanFieldEditor(ICVSUIConstants.PREF_CONSIDER_CONTENTS, CVSUIMessages.ComparePreferencePage_4, BooleanFieldEditor.DEFAULT, getFieldEditorParent()) { protected void doLoad() { super.doLoad(); getChangeControl(getFieldEditorParent()).setSelection(!getBooleanValue()); } protected void doLoadDefault() { super.doLoadDefault(); getChangeControl(getFieldEditorParent()).setSelection(!getBooleanValue()); } protected void doStore() { getPreferenceStore().setValue(getPreferenceName(), !getBooleanValue()); } }); where the preference value is kept intact, but the checkbox selection is inverted? Is the new label worth it?
> where the preference value is kept intact, but the checkbox selection is > inverted? Is the new label worth it? May I just cite from your comment 17: "I like the new label" ;-)
That was nothing more than the first part of the praise/criticism/praise sandwich :)
(In reply to comment #21) > That was nothing more than the first part of the praise/criticism/praise > sandwich :) But now you're doomed :-)
(In reply to comment #16) > We can then explain in the doc > in which cases/actions we use the contents if the preference is disabled (which > is the default). This is how I would put it: "For more precise comparison, the contents can be compared. If performance is an issue this can be disabled to use the much faster time stamp based comparison. This option only applies to comparisons of remote resources or when comparing a selected project or folder with a branch or version. The option is not honored in synchronizations or when the comparison has been started with Compare With > Latest From <Branch Name> or <Version>." I removed the line about the option being "only respected when displaying model elements is disabled." as this is going to be fixed here as well, see comment 11. I wanted to keep the last sentence as concise as possible but I guess I have to mention that "Compare With > Latest From HEAD" (synchronization) is different than "Compare With > Another Branch or Version > HEAD" (comparison), see comment 13 :| Or can I leave out the last sentence and say nothing about cases where the pref is not honored?
(In reply to comment #23) Proposal sounds good. I would change two things: 1) I would write Compare With > Latest from ... or Compare With > Latest from <branch/version> 2) "synchronizations": I'm not sure users know what this means
(In reply to comment #24) > 1) I would write Compare With > Latest from ... > or Compare With > Latest from <branch/version> > 2) "synchronizations": I'm not sure users know what this means I've rephrased the last sentence to "The option is not honored in synchronizations launched using the Team > Synchronize menu command, the Synchronize toolbar command or the Compare With > Latest From <branch/version> menu command.". Fixed with 3f762b893c48833cd921ed0dda5d75a9a9f5524f in eclipse.platform.team and 5033c0a5aa8a25f1163a41a9279e2526b05e4d9b in eclipse.platform.common. Available in builds >= N20111110-2000.
