[cmake-developers] Perforce Patch for CTest

Pedro Navarro pnavarro at netflix.com
Tue Oct 22 14:51:52 EDT 2013


>
>
>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20131022/8be2a3ec/attachment.html>


More information about the cmake-developers mailing list