Community
Participate
Working Groups
IFilePatch should include methods that return the before and after dates of a patch, with some sort of flag to indicate that dates aren't available. Perhaps this would work: /** * Special constant that will be returned by identity from get * getBeforeDate() or getAfterDate() if the date is unknown. * Equal to Midnight, Jan 1, 1970 GMT. */ public static Date DATE_UNKNOWN = new Date(0); /** * Returns the before date from the patch, or DATE_UNKNOWN (by identity) * if the date is unknown. Never returns null. */ public Date getBeforeDate(); /** * Returns the after date from the patch, or DATE_UNKNOWN (by identity) * if the date is unknown. Never returns null. */ public Date getBeforeDate();
Created attachment 84063 [details] Patch This looks like an easy to fix bug, especially as Stefan gave us most of the patch already :)
Created attachment 84069 [details] Test cases Simple test cases + additional cleanup of patch related tests
Created attachment 84070 [details] mylyn/context/zip
Created attachment 84071 [details] Patch for PatchReader Added a date format for patches from compare tests. This realized me that fixing this bug will cause us more trouble in the future :)
To precise what I meant in the previous comment by "more trouble": we could consider adding a preference page where date formats we're interested in can be added. Or at least we should start thinking about respecting locale settings. But that's another story and another bug.
Isn't there a standard (locale-independent) date format for CVS patches?
AFAIK, we cannot assume that. I've seen a couple of different date formats in my workspace. Actually, there is a wide variety of diffs produced by all the different implementations of diff (cvs diff, BSDs, gnu, solaris, etc). So, I'm afraid that the date string in diff might be locale specific. Also, we can't forget about cases where no date is present (-L option to diff). This raises the question: should the DATE_UNKNOWN be returned for this cases too? What about invalid dates? Or ones with unknown format? I think we should consider returning longs instead of Dates. This is the way they are kept in IFilePatch. Then we could use different flags to indicate that there is something wrong with the date (eg. DATE_UNKNOWN = -1; DATE_INVALID = -2 and so on). How does it sound to you Stefan?
> should the DATE_UNKNOWN be returned for this cases too? That was my intention. DATE_UNKNOWN would be returned in any situation where it couldn't obtain a correct date, regardless of the reason. > I think we should consider returning longs instead of Dates. I have no preference. Whatever you prefer. :-) > So, I'm afraid that the date string in diff might be locale specific Personally, I'd prefer to use heuristics to try to detect the date format and just return DATE_UNKNOWN if they fail, rather than adding a preference. If we want to support different locales, I'd want a way to pass the date format into the patch parser when I use it programmaticaly. The patches generated by my RCP app use a consistent date format and a special header that make it easy to identify the patch type - this way I could always pass in the correct date format when parsing a patch that I know came from my own app.
Created attachment 84722 [details] Patch Patch for both FileDiff and PatchReader. I sticked to returning longs and added get/set methods to modify accepted date formats.
Created attachment 84723 [details] Updated test cases
Stefan, would the latest patch be any good for you?
Released to HEAD. Stefan if you have any additional comments on the patch I've just released please let me know.
Hey, sorry for taking so long to comment. I haven't checked by bugzilla email in awhile. The new patch looks like an improvement. My only concern is this bit: + public static void setDateFormates(DateFormat[] dateFormats) { + DATE_FORMATS = dateFormats; + } I see a couple problems with this: 1. It doesn't look threadsafe. (One thread could change the global date formats while another is trying to use them to parse a patch) 2. Having these things as static variables could cause one parser to overload settings intended for another. My suggestion would be to make the date formats a property of a particular parser or at the very least, make them thread-local so that they're threadsafe.
Verified, but bug 216247 logged.
