(Bug report + fix) gitk "IgnCase" search doesn't ignore case

(Bug report + fix) gitk "IgnCase" search doesn't ignore case

[Juan Navarro]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

Hi,

this question was originally posted on the Google Groups list, trying to 
get help 
(https://groups.google.com/d/topic/git-users/QAFKOQU4eUo/discussion).
Now that it's confirmed as a bug and I have a proposed solution, I'm 
posting here.

Gitk "find commit" search function doesn't follow the "IgnCase" option 
that is selectable with a combo selector on the right side of the 
window; it should be searching in a case-insensitive way, but it doesn't.

Steps to reproduce:
1. Clone any repo. I'm right now using 
https://github.com/Kurento/kms-core.git 
<https://github.com/Kurento/kms-core.git>
2. cd into the repo and launch gitk
3. In the "Find commit" bar, select "changing lines matching"
4. In the right side of the same bar, select "IgnCase"
5. Search for a term that you know exists in uppercase, but write the 
search in lowercase. In my example, I'm searching for "leaky_time".
6. No search results appear.
7. Change your search word to uppercase.
8. Some search results appear, thus proving that the search is being 
done case-sensitive.

The command that gets called when searching is in lines 4804 & 4809 
(https://github.com/git/git/blob/master/gitk-git/gitk#L4804 
<https://github.com/git/git/blob/master/gitk-git/gitk#L4804>):
git diff-tree -r -s --stdin -G<SearchTerm>

Proposed solution is almost trivial: check if the "IgnCase" option is 
enabled, and in that case add the flag "-i" to the git command. Now that 
we are at it, it's probably correct to add that option to all search modes.
A diff is attached to this email, hoping that someone is able to apply 
it (sorry I'm not familiarized with contributing with patch files, but 
the diff is pretty small anyways).

Regards,
Juan

[-- Attachment #2: gitk-igncase.diff --]
[-- Type: text/x-patch, Size: 267 bytes --]

4809c4809,4815
<     set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
---
> 
>     set igncase_arg ""
>     if {$findtype eq [mc "IgnCase"]} {
>         set igncase_arg "-i"
>     }
> 
>     set cmd [concat | git diff-tree $igncase_arg -r -s --stdin $gdtargs]

Re: (Bug report + fix) gitk "IgnCase" search doesn't ignore case

[Eric Sunshine]

On Thu, Jun 14, 2018 at 02:53:03PM +0200, Juan Navarro wrote:
> Gitk "find commit" search function doesn't follow the "IgnCase" option that
> is selectable with a combo selector on the right side of the window; it
> should be searching in a case-insensitive way, but it doesn't.
> 
> Steps to reproduce:
> [...]
> 3. In the "Find commit" bar, select "changing lines matching"
> 4. In the right side of the same bar, select "IgnCase"
> [...]
> 
> Proposed solution is almost trivial: check if the "IgnCase" option is
> enabled, and in that case add the flag "-i" to the git command. Now that we
> are at it, it's probably correct to add that option to all search modes.
> A diff is attached to this email, hoping that someone is able to apply it
> (sorry I'm not familiarized with contributing with patch files, but the diff
> is pretty small anyways).

Thanks for reporting this.

A different way to interpret the situation is that the user-interface
is being inconsistent. For instance, the "fields" pop-up next to the
"exact/ignore-case/regexp" pop-up does not seem to make sense for
search types other than "containing", so it probably ought to be
disabled for anything other than "containing". By extension, one could
argue that the "exact/ignore-case/regexp" pop-up also ought be
disabled for non-"containing" searches. The fact that they are not
disabled confuses people into thinking that they should be functional
for all searches, not just for "containing" searches, even though such
functionality was never implemented (and indeed, may be difficult to
implement fully).

Your proposed fix handles only the "ignore case" item; it does not
implement "regexp" functionality, so it could be considered
incomplete. A more complete fix would also disable the "regexp" item
to avoid misleading users, and to head off future bug reports similar
to this one saying that "regexp" doesn't work for non-"containing"
searches. (Bonus points for also disabling the "fields" pop-up for
non-"containing" searches when it's not applicable.)

Below is your fix wrapped up as a proper patch and sent in-line rather
than as an attachment. It's also slightly simplified by dropping the
unnecessary extra variable. You'll need to sign-off on the patch if it
is ever to be accepted. You can do so by adding it after my sign-off.
If you don't feel like re-sending the patch with your sign-off, you
can alternately reply to this email with a "Signed-off-by: Your Name
<your@email>" line.

Note, however, that the gitk project is, at best, deeply slumbering,
so it's not clear when or even if patches will be incorporated.
(Indeed, other recent gitk-related patches sent to the mailing list
have not yet been picked up.)

--- >8 ---
From: Juan Navarro <juan.navarro@gmx.es>
Subject: [PATCH] gitk: support "ignore case" for non-"containing" searches

The "Exact/Ignore Case/Regexp" pop-up control only affects "containing"
searches. Other types of searches ("touching paths", "adding/removing
strings", "changing lines matching") ignore this option. Improve the
user experience by also recognizing "ignore case" for non-"containing"
searches.

Note: This change only implements the "ignore case" option for these
other search types; it does not add support for the "regexp" option
(which still only affects "containing" searches). A more complete "fix"
would improve the user experience even more by making the UI more
consistent; namely, by disabling options which don't make sense or are
not easily implemented for non-"containing" searches. In particular, the
"regexp" pop-up item and the neighboring "fields" pop-up control ought
perhaps be disabled when a non-"containing" search is selected.

[es: wrote commit message; slightly simplified proposed "fix"]

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
---
diff --git a/gitk-git/gitk b/gitk-git/gitk
index a14d7a16b2..fbb75f7390 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -4806,6 +4806,9 @@ proc do_file_hl {serial} {
 	# must be "containing:", i.e. we're searching commit info
 	return
     }
+    if {$findtype eq [mc "IgnCase"]} {
+	set gdtargs [linsert $gdtargs 0 "-i"]
+    }
     set cmd [concat | git diff-tree -r -s --stdin $gdtargs]
     set filehighlight [open $cmd r+]
     fconfigure $filehighlight -blocking 0
-- 
2.18.0.rc1.256.g331a1db143