<table><tr><td style="">sitter added a comment.
</td><a style="text-decoration: none; padding: 4px 8px; margin: 0 8px 8px; float: right; color: #464C5C; font-weight: bold; border-radius: 3px; background-color: #F7F7F9; background-image: linear-gradient(to bottom,#fff,#f1f0f1); display: inline-block; border: 1px solid rgba(71,87,120,.2);" href="https://phabricator.kde.org/D24641">View Revision</a></tr></table><br /><div><div><p>This is starting to look really good. All functions will need documenting in the header of that file so they show up on api.kde.org, see other modules for examples.</p></div></div><br /><div><strong>INLINE COMMENTS</strong><div><div style="margin: 6px 0 12px 0;"><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24641#inline-142971">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ECMSourceVersionControl.cmake:61</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">set</span><span class="p">(</span><span style="color: #766510">_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_NOT_VCS</span> <span style="color: #766510">FALSE</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);"><span style="color: #304a96">set</span><span class="p">(</span><span style="color: #766510">_ECM_SOURCE_VERSION_CONTROL_ALREADY_WARNED_MISSING_GIT_EXEC</span> <span style="color: #766510">FALSE</span><span class="p">)</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">Do we really need this? It seems to me we could just spam it for every call, in the grand scheme of things it makes no difference but is less logic one has to worry about when extending this module.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24641#inline-142972">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ECMSourceVersionControl.cmake:75</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #74777d"># Git</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #304a96">if</span><span class="p">(</span><span style="color: #766510">NOT</span> <span style="color: #766510">_ECM_SOURCE_VERSION_GIT_EXECUTABLE</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span style="color: #304a96">find_program</span><span class="p">(</span><span style="color: #766510">_ECM_SOURCE_VERSION_GIT_EXECUTABLE</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I'd move the exec detection and missing reporting into its own helper which gets called by all "public" functions. Currently the logic is duped in both functions.</p></div></div><br /><div style="border: 1px solid #C7CCD9; border-radius: 3px;"><div style="padding: 0; background: #F7F7F7; border-color: #e3e4e8; border-style: solid; border-width: 0 0 1px 0; margin: 0;"><div style="color: #74777d; background: #eff2f4; padding: 6px 8px; overflow: hidden;"><a style="float: right; text-decoration: none;" href="https://phabricator.kde.org/D24641#inline-142974">View Inline</a><span style="color: #4b4d51; font-weight: bold;">ECMSourceVersionControl.cmake:87</span></div>
<div style="font: 11px/15px "Menlo", "Consolas", "Monaco", monospace; white-space: pre-wrap; clear: both; padding: 4px 0; margin: 0;"><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">            <span style="color: #304a96">set</span><span class="p">(</span><span style="color: #766510">ECM_SOURCE_VERSION_CONTROL_REVISION</span> <span style="color: #766510">"${ECM_SOURCE_VERSION_CONTROL_REVISION}"</span> <span style="color: #766510">PARENT_SCOPE</span><span class="p">)</span>
</div><div style="padding: 0 8px; margin: 0 4px; background: rgba(151, 234, 151, .6);">        <span style="color: #304a96">else</span><span class="p">()</span>
</div></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I think it's more idiomatic if you accept the variable name as a function argument instead of hardcoding one.</p>

<p style="padding: 0; margin: 8px;">For example <tt style="background: #ebebeb; font-size: 13px;">find_program</tt>, but really most helpers work like that off the top of my head.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R240 Extra CMake Modules</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D24641">https://phabricator.kde.org/D24641</a></div></div><br /><div><strong>To: </strong>thomasfischer, sitter, kossebau<br /><strong>Cc: </strong>kde-frameworks-devel, kde-buildsystem, LeGast00n, GB_2, bencreasy, michaelh, ngraham, bruns<br /></div>