<blockquote>
<p>Fixing this isn't trivial</p>
</blockquote>
<p>That's why I suggested in an earlier comment that the fact <code>utils_strv_find_lcs()</code> looks generic is cute, but in practice this function does <em>not</em> do what <code>utils_strv_shorten_file_list()</code> needs.</p>
<blockquote>
<p>and without proper unit tests there is a risk to break previously working cases.</p>
</blockquote>
<p>Yeah, we should probably add tests for this.  Fortunately, it's probably one of the few things that should be easy enough to add a test for, because <code>utils.c</code> don't usually have dependencies on internal Geany state, which means it can likely be tested by a minimal test program linking libgeany.</p>
<blockquote>
<p>I suggest to accept the behavior for now (and merge) and I'll follow up with a fix + unit tests. But I would hate if this doesn't make it into the release just because of select (edge) cases because the status quo is just plain unusable for me. In fact I've been using this patch at work for ages where I have real-world cases and I never came across issues like this.</p>
</blockquote>
<p>Fair enough; I must admit I specifically crafted the test to fail, looking at the weakness of the implementation :)</p>
<p>This said, note that I have another implementation (see a message above) that doesn't have the issue.  It has arguably worse performance, and can probably use some improvement in other area, but I didn't find a bug in its results yet.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany/pull/1445#issuecomment-439360404">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ_J0BGklEStHpyhMqILHn_Un3ladks5uvpwEgaJpZM4MnYrc">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ1Wn_Nrz8s2jZT6neiYeBeKkwz1Lks5uvpwEgaJpZM4MnYrc.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany","title":"geany/geany","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n in #1445: \u003e Fixing this isn't trivial\r\n\r\nThat's why I suggested in an earlier comment that the fact `utils_strv_find_lcs()` looks generic is cute, but in practice this function does *not* do what `utils_strv_shorten_file_list()` needs.\r\n\r\n\u003e and without proper unit tests there is a risk to break previously working cases.\r\n\r\nYeah, we should probably add tests for this.  Fortunately, it's probably one of the few things that should be easy enough to add a test for, because `utils.c` don't usually have dependencies on internal Geany state, which means it can likely be tested by a minimal test program linking libgeany.\r\n\r\n\u003e I suggest to accept the behavior for now (and merge) and I'll follow up with a fix + unit tests. But I would hate if this doesn't make it into the release just because of select (edge) cases because the status quo is just plain unusable for me. In fact I've been using this patch at work for ages where I have real-world cases and I never came across issues like this.\r\n\r\nFair enough; I must admit I specifically crafted the test to fail, looking at the weakness of the implementation :)\r\n\r\nThis said, note that I have another implementation (see a message above) that doesn't have the issue.  It has arguably worse performance, and can probably use some improvement in other area, but I didn't find a bug in its results yet."}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany/pull/1445#issuecomment-439360404"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/geany/geany/pull/1445#issuecomment-439360404",
"url": "https://github.com/geany/geany/pull/1445#issuecomment-439360404",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "Re: [geany/geany] Improve goto-symbols popup (#1445)",
"sections": [
{
"text": "",
"activityTitle": "**Colomban Wendling**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@b4n",
"facts": [

]
}
],
"potentialAction": [
{
"name": "Add a comment",
"@type": "ActionCard",
"inputs": [
{
"isMultiLine": true,
"@type": "TextInput",
"id": "IssueComment",
"isRequired": false
}
],
"actions": [
{
"name": "Comment",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"IssueComment\",\n\"repositoryFullName\": \"geany/geany\",\n\"issueId\": 1445,\n\"IssueComment\": \"{{IssueComment.value}}\"\n}"
}
]
},
{
"name": "Close pull request",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"PullRequestClose\",\n\"repositoryFullName\": \"geany/geany\",\n\"pullRequestId\": 1445\n}"
},
{
"targets": [
{
"os": "default",
"uri": "https://github.com/geany/geany/pull/1445#issuecomment-439360404"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 211651292\n}"
}
],
"themeColor": "26292E"
}
]</script>