{"thread":{"id":"48710","subject":"(Bug report + fix) gitk \"IgnCase\" search doesn't ignore case","startedAt":"2018-06-14T12:53:09Z","lastAt":"2018-06-14T21:44:16Z","messageCount":2,"participants":["Juan Navarro","Eric Sunshine"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"350151","messageId":"fc6b89c5-f0be-97c8-b0e1-247d9dd3e1f0@gmx.es","threadId":"48710","inReplyTo":null,"subject":"(Bug report + fix) gitk \"IgnCase\" search doesn't ignore case","fromName":"Juan Navarro","fromEmail":"juan.navarro@gmx.es","sentAt":"2018-06-14T12:53:03Z","receivedAt":"2018-06-14T12:53:09Z","isPatch":false,"sender":{"key":"juan.navarro@gmx.es","avatar":null},"body":"Hi,\n\nthis question was originally posted on the Google Groups list, trying to \nget help \n(https://groups.google.com/d/topic/git-users/QAFKOQU4eUo/discussion).\nNow that it's confirmed as a bug and I have a proposed solution, I'm \nposting here.\n\nGitk \"find commit\" search function doesn't follow the \"IgnCase\" option \nthat is selectable with a combo selector on the right side of the \nwindow; it should be searching in a case-insensitive way, but it doesn't.\n\nSteps to reproduce:\n1. Clone any repo. I'm right now using \nhttps://github.com/Kurento/kms-core.git \n<https://github.com/Kurento/kms-core.git>\n2. cd into the repo and launch gitk\n3. In the \"Find commit\" bar, select \"changing lines matching\"\n4. In the right side of the same bar, select \"IgnCase\"\n5. Search for a term that you know exists in uppercase, but write the \nsearch in lowercase. In my example, I'm searching for \"leaky_time\".\n6. No search results appear.\n7. Change your search word to uppercase.\n8. Some search results appear, thus proving that the search is being \ndone case-sensitive.\n\nThe command that gets called when searching is in lines 4804 & 4809 \n(https://github.com/git/git/blob/master/gitk-git/gitk#L4804 \n<https://github.com/git/git/blob/master/gitk-git/gitk#L4804>):\ngit diff-tree -r -s --stdin -G<SearchTerm>\n\nProposed solution is almost trivial: check if the \"IgnCase\" option is \nenabled, and in that case add the flag \"-i\" to the git command. Now that \nwe are at it, it's probably correct to add that option to all search modes.\nA diff is attached to this email, hoping that someone is able to apply \nit (sorry I'm not familiarized with contributing with patch files, but \nthe diff is pretty small anyways).\n\nRegards,\nJuan\n\n\n4809c4809,4815\n<     set cmd [concat | git diff-tree -r -s --stdin $gdtargs]\n---\n> \n>     set igncase_arg \"\"\n>     if {$findtype eq [mc \"IgnCase\"]} {\n>         set igncase_arg \"-i\"\n>     }\n> \n>     set cmd [concat | git diff-tree $igncase_arg -r -s --stdin $gdtargs]\n"},{"id":"350209","messageId":"20180614214405.GA26783@flurp.local","threadId":"48710","inReplyTo":"fc6b89c5-f0be-97c8-b0e1-247d9dd3e1f0@gmx.es","subject":"Re: (Bug report + fix) gitk \"IgnCase\" search doesn't ignore case","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-06-14T21:44:05Z","receivedAt":"2018-06-14T21:44:16Z","isPatch":false,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Jun 14, 2018 at 02:53:03PM +0200, Juan Navarro wrote:\n> Gitk \"find commit\" search function doesn't follow the \"IgnCase\" option that\n> is selectable with a combo selector on the right side of the window; it\n> should be searching in a case-insensitive way, but it doesn't.\n> \n> Steps to reproduce:\n> [...]\n> 3. In the \"Find commit\" bar, select \"changing lines matching\"\n> 4. In the right side of the same bar, select \"IgnCase\"\n> [...]\n> \n> Proposed solution is almost trivial: check if the \"IgnCase\" option is\n> enabled, and in that case add the flag \"-i\" to the git command. Now that we\n> are at it, it's probably correct to add that option to all search modes.\n> A diff is attached to this email, hoping that someone is able to apply it\n> (sorry I'm not familiarized with contributing with patch files, but the diff\n> is pretty small anyways).\n\nThanks for reporting this.\n\nA different way to interpret the situation is that the user-interface\nis being inconsistent. For instance, the \"fields\" pop-up next to the\n\"exact/ignore-case/regexp\" pop-up does not seem to make sense for\nsearch types other than \"containing\", so it probably ought to be\ndisabled for anything other than \"containing\". By extension, one could\nargue that the \"exact/ignore-case/regexp\" pop-up also ought be\ndisabled for non-\"containing\" searches. The fact that they are not\ndisabled confuses people into thinking that they should be functional\nfor all searches, not just for \"containing\" searches, even though such\nfunctionality was never implemented (and indeed, may be difficult to\nimplement fully).\n\nYour proposed fix handles only the \"ignore case\" item; it does not\nimplement \"regexp\" functionality, so it could be considered\nincomplete. A more complete fix would also disable the \"regexp\" item\nto avoid misleading users, and to head off future bug reports similar\nto this one saying that \"regexp\" doesn't work for non-\"containing\"\nsearches. (Bonus points for also disabling the \"fields\" pop-up for\nnon-\"containing\" searches when it's not applicable.)\n\nBelow is your fix wrapped up as a proper patch and sent in-line rather\nthan as an attachment. It's also slightly simplified by dropping the\nunnecessary extra variable. You'll need to sign-off on the patch if it\nis ever to be accepted. You can do so by adding it after my sign-off.\nIf you don't feel like re-sending the patch with your sign-off, you\ncan alternately reply to this email with a \"Signed-off-by: Your Name\n<your@email>\" line.\n\nNote, however, that the gitk project is, at best, deeply slumbering,\nso it's not clear when or even if patches will be incorporated.\n(Indeed, other recent gitk-related patches sent to the mailing list\nhave not yet been picked up.)\n\n--- >8 ---\nFrom: Juan Navarro <juan.navarro@gmx.es>\nSubject: [PATCH] gitk: support \"ignore case\" for non-\"containing\" searches\n\nThe \"Exact/Ignore Case/Regexp\" pop-up control only affects \"containing\"\nsearches. Other types of searches (\"touching paths\", \"adding/removing\nstrings\", \"changing lines matching\") ignore this option. Improve the\nuser experience by also recognizing \"ignore case\" for non-\"containing\"\nsearches.\n\nNote: This change only implements the \"ignore case\" option for these\nother search types; it does not add support for the \"regexp\" option\n(which still only affects \"containing\" searches). A more complete \"fix\"\nwould improve the user experience even more by making the UI more\nconsistent; namely, by disabling options which don't make sense or are\nnot easily implemented for non-\"containing\" searches. In particular, the\n\"regexp\" pop-up item and the neighboring \"fields\" pop-up control ought\nperhaps be disabled when a non-\"containing\" search is selected.\n\n[es: wrote commit message; slightly simplified proposed \"fix\"]\n\nSigned-off-by: Eric Sunshine <sunshine@sunshineco.com>\n---\ndiff --git a/gitk-git/gitk b/gitk-git/gitk\nindex a14d7a16b2..fbb75f7390 100755\n--- a/gitk-git/gitk\n+++ b/gitk-git/gitk\n@@ -4806,6 +4806,9 @@ proc do_file_hl {serial} {\n \t# must be \"containing:\", i.e. we're searching commit info\n \treturn\n     }\n+    if {$findtype eq [mc \"IgnCase\"]} {\n+\tset gdtargs [linsert $gdtargs 0 \"-i\"]\n+    }\n     set cmd [concat | git diff-tree -r -s --stdin $gdtargs]\n     set filehighlight [open $cmd r+]\n     fconfigure $filehighlight -blocking 0\n-- \n2.18.0.rc1.256.g331a1db143\n"}]}