> > > > I would extend the regex in the DiffParser constructor to contain " - " at > the > end to make the propability that a filename with # in it would cause issues > smaller. Also I find the usage of both Options and options in > SetP4Options() an > invitation for confusion (and possible errors). >
I'll change the regex. Although I verified it in debuggex to make sure we could correctly handle different combination of files with # and other characters in it, it makes sense to add the additional check. About the options, do you mean not using P4Options when doing an update if options for the update commands are given? I have to admit that I don't fully understand the difference between Update Options and VCS Specific options (I got that code from the GIT implementation) but Perforce has the notion of global and command specific flags. In this case, the global flags are the P4Options which are used to specify how do you connect to the server (host, port, username) and which client view to use, the "other" options are the flags you pass for the update command, which are completely different. I agree it can be confusing > Also I see a possible conflict between LoadRevisions() and > NoteOldRevision(). > The latter sets OldRevision to "0" in case it can't load any, the former > tests > the variable for being empty. Given the right order of events this could > result in different results. > You are absolutely right. That's a logic error. I made a last minute change to report 0 if there was an error getting the revision number and didn't update the other function. It's triggered only if there is an issue connecting to the server, so the sync would fail anyway, and that's why the unit test didn't catch it. > > get_filename_component(TOP "${CMAKE_CURRENT_LIST_FILE}" PATH) could be > written > as CMAKE_CURRENT_LIST_DIR. I don't remember exactly when it was introduced, > but you drive that test with the newly built CMake so this must work. And a > newline is missing at the end of that file. > Would that be preferred? I have no issues using it but that's what the other tests were using, so I wanted them to be as similar as possible. > > Otherwise, awesome work. > Thanks! And thanks for the detailed review. Pedro > > Eike
-- Powered by www.kitware.com Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Follow this link to subscribe/unsubscribe: http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers