<table><tr><td style="">rjvbb added inline comments.
</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/D8158" rel="noreferrer">View Revision</a></tr></table><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/D8158#inline-34338" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">CMakeLists.txt:133</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">We're not overwriting anything, we're adding an additional full version string. That's not unheard of, this is exactly what's done in Apple's Info.plist files. A "numeric" version for use in code, and a human-readable string that is to be presented to users.</p></blockquote>

<p style="padding: 0; margin: 8px;">You're basically overwriting/invalidating the <tt style="background: #ebebeb; font-size: 13px;">major.minor.patch</tt> version which was previously shown in the About dialog as set by CMake.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">I can achieve the same thing the way Krita does things, but that's more complicated (cf. the script I used in the initial release). Sven pointed out that we cannot change VERSION_PATCH because that variable is used by dependent code.</p></blockquote>

<p style="padding: 0; margin: 8px;">It's not more complicated. Please forget about Krita again, it probably only confuses you.</p>

<p style="padding: 0; margin: 8px;">And where did I talk about changing VERSION_PATCH?</p>

<p style="padding: 0; margin: 8px;"><tt style="background: #ebebeb; font-size: 13px;">set(KDevelop_FULLVERSION_STRING "${KDEVELOP_VERSION_STRING} (git ${SHA1}")</tt> doesn't "change" VERSION_PATCH.</p>

<blockquote style="border-left: 3px solid #a7b5bf; color: #464c5c; font-style: italic; margin: 4px 0 12px 0; padding: 4px 12px; background-color: #f8f9fc;"><p style="padding: 0; margin: 8px;">Whatever approach I will use I find it crucial that the output from git describe is used. Git hashes are random, so using only them will not allow checking at a glance which of two versions is older or newer (and that's part of what I want to achieve with this patch).</p></blockquote>

<p style="padding: 0; margin: 8px;">Okay, then please use:<br />
<tt style="background: #ebebeb; font-size: 13px;">set(KDevelop_FULLVERSION_STRING "${KDEVELOP_VERSION_STRING} (git ${GIT_FULL_VERSION}")</tt></p>

<p style="padding: 0; margin: 8px;">That'll show e.g. '5.1.2' twice, but that doesn't harm.</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">I interpreted <tt style="background: #ebebeb; font-size: 13px;">re-use VERSION_MAJOR, VERSION_MINOR, VERSION_PATCH</tt> to suggest that the fuller version should be obtained from them directly.</p>

<p style="padding: 0; margin: 8px;">I'll play with <tt style="background: #ebebeb; font-size: 13px;">string replace</tt>, it should be possible to remove <tt style="background: #ebebeb; font-size: 13px;">${MAJOR}.${MINOR}.${PATCH}</tt> from the <tt style="background: #ebebeb; font-size: 13px;">git describe</tt> return. That way the full version won't show 5.x.y twice if the version from the CMake version agrees with the current tag. Duplicate may not harm but avoiding them is more elegant, don't you think?</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/D8158#inline-34336" rel="noreferrer">View Inline</a><span style="color: #4b4d51; font-weight: bold;">kfunk</span> wrote in <span style="color: #4b4d51; font-weight: bold;">GetGitRevisionDescription.cmake:1</span></div>
<div style="margin: 8px 0; padding: 0 12px; color: #74777D;"><p style="padding: 0; margin: 8px;">Hm?</p>

<p style="padding: 0; margin: 8px;">I was asking you to update the copy of those GetGit*.cmake files. What does that have to do with KDevelop's 5.2 branch?</p></div></div>
<div style="margin: 8px 0; padding: 0 12px;"><p style="padding: 0; margin: 8px;">My bad, I read too quickly. But then I also don't understand why you're getting an error and I don't.</p></div></div></div></div></div><br /><div><strong>REPOSITORY</strong><div><div>R32 KDevelop</div></div></div><br /><div><strong>REVISION DETAIL</strong><div><a href="https://phabricator.kde.org/D8158" rel="noreferrer">https://phabricator.kde.org/D8158</a></div></div><br /><div><strong>To: </strong>rjvbb, KDevelop, brauch, kfunk<br /><strong>Cc: </strong>kossebau, flherne, mwolff, brauch, kdevelop-devel, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger<br /></div>