sitter added a comment.

  Hm, how about separate functions though? With a single stat any given build 
still needs N process forks even when they only want 1 value.
  
  In D24641#548394 <https://phabricator.kde.org/D24641#548394>, @thomasfischer 
wrote:
  
  > To clarify `ECM_SOURCE_VERSION_CONTROL_COMMIT_COUNT`: it counts the number 
of commits in direct line of succession from the repository's initialization to 
the current commit. It does not include commits in other branches. Basically 
the number of commits listed in a plain `git log`. The commit count gives a 
quick indication of the progress of a repository (or branch) without requiring 
to look up the repo's commit messages or hashes.
  
  
  I understand. This is super heavy though. I have just run it on all clones I 
have on this PC and for some repos that takes upwards of half a second to 
complete... on an SSD, so I also ran a quick check on our CI servers, which use 
HDDs and there it takes at least half a second with some repos (e.g. kdevelop) 
going up to 26 seconds (uncached)! I wouldn't mind terribly if the various 
things got split into their own functions (ecm_source_version_control_revison, 
ecm_source_version_control_branch, ecm_source_version_control_commit_count... 
or some names like that) but even then I have to question the use case behind 
the commit count information. So, what is the use case? What do you do with 
this information? It occurs to me that if you know the hash you could look up 
the commit count of that hash should you need it, but I struggle to imagine a 
scenario where that number is relevant.

INLINE COMMENTS

> ECMSourceVersionControl.cmake:70
> +        message(STATUS "Source directory '${CMAKE_SOURCE_DIR}' is under 
> version control by Git.")
> +        find_program(GIT_EXECUTABLE
> +            NAMES git.bat git # for Windows, 'git.bat' must be found before 
> 'git'

I think you are leaking this variable into the parent scope. I am not super 
sure how to deal with this but I think I've seen `_`prefixes, or you use a 
function and explicitly forward into the PARENT_SCOPE 
(https://cmake.org/cmake/help/v3.0/command/set.html).

Alternatively with a multi-function approach I'd probably just make it 
ECM_SOURCE_VERSION_CONTROL_EXECUTABLE so it only needs finding on the first 
call.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D24641

To: thomasfischer, sitter, kossebau
Cc: kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, 
michaelh, ngraham, bruns

Reply via email to