dkprice / vim-easygrep

Fast and Easy Find and Replace Across Multiple Files
The Unlicense
325 stars 47 forks source link

Miscellaneous minor changes #34

Closed hardenedapple closed 8 years ago

hardenedapple commented 8 years ago

I'd like to suggest a few changes that I think fix some bugs, but rather than open up a new issue for each one I'm creating a pull request combining them.

  1. GNU grep, without recursive flag, and using Track Extension With this configuration, the command line created is of the form grep --include= ... --exclude= ... "<pattern>" /directory/of/file GNU grep ignores the directory passed on the command line because the --recursive flag is not set. To fix this I made the command line of the form grep --include= ... --exclude= ... "<pattern>" /directory/of/file/* which appears to work.
  2. ag case-sensitive flag On my machine, ag uses --smart-case by default so your casesensitivity_ag test was failing. I added in the relevant flag to the corresponding RegisterGrepProgram call and the test seems to work now.
  3. I run Linux, so the tests were failing from mismatched path-separators in the *.ok files, I added some calls to sed in test.sh to change the separators before running the test and put them back after. Because this relies on the GNU specific '-i' flag, I only call that when running Linux, and not e.g. Solaris. This workaround isn't nice, but it should work for now.
  4. In order to pass some "simpleregex" testcases on my machine, I changed the opt_str_patternpostfix and opt_str_patternprefix values of some grep program arguments. I switched them from " to ' , which meant the backslash in the regex's aren't expanded before being passed to the command. I'm a little wary of this change (as I always am when dealing with shell-escaping) -- I have the feeling you had it that way for a reason, and that maybe I'm fixing the test cases on my machine but breaking them somewhere else, or breaking something else somewhere.
  5. I added some tests to pick up the Track Extension problem I first noticed. Currently ag is failing these tests, and I believe the reason is twofold. The first is to do with the --file-search-regex option -- I don't think you can use that multiple times. This is justified by the fact that ag --nogroup --nocolor --column -s --file-search-regex="(\.m4|\.ac|\.in|\.am|\.mk|\.mak|\.dsp)" --ignore=".svn" --ignore=".git" --ignore=".hg" 'a' ~/.vim/bundle/easygrep/tests works, while ag --nogroup --nocolor --column -s --file-search-regex="\.m4" --file-search-regex="\.ac" --file-search-regex="\.in" --file-search-regex="\.am" --file-search-regex="\.mk" --file-search-regex="\.mak" --file-search-regex="\.dsp" --ignore=".svn" --ignore=".git" --ignore=".hg" 'a' ~/.vim/bundle/easygrep/tests doesn't.

This I believe requires something like the patch

diff --git a/plugin/EasyGrep.vim b/plugin/EasyGrep.vim
index 9fa3fe1..d62c3f3 100755
--- a/plugin/EasyGrep.vim
+++ b/plugin/EasyGrep.vim
@@ -2436,7 +2436,10 @@ function! s:ConfigureGrepCommandParameters()
                 \ 'opt_bool_isinherentlyrecursive': '1',
                 \ 'opt_bool_isselffiltering': '0',
                 \ 'opt_bool_nofiletargets': '0',
-                \ 'opt_str_mapinclusionsexpression': '"--file-search-regex=\"" .substitute(v:val, "^\\*\\.", "\\\\.", "")."\""',
+                \ 'opt_str_mapinclusionsexpression': 'substitute(v:val, "^\\*\\.", "\\\\.", "")',
+                \ 'opt_str_mapinclusionsexpressionseparator': '|',
+                \ 'opt_str_mapinclusionsprefix': '--file-search-regex="(',
+                \ 'opt_str_mapinclusionspostfix': ')"',
                 \ 'opt_str_hiddenswitch': '--hidden',
                 \ })

Unfortunately I get errors like /bin/bash: -c: line 0: unexpected EOF while looking for matching `"' with this, which is why it's not in my suggested changesets.

The second I didn't manage to fix at all -- the inclusion file regex includes '*[mM]akefile', which "ag" complains about.

Also I didn't manage to find the 'pt' program, so in order to maintain symmetry I added a test for it but left the expected output as "Unknown"

I should note I'm breaking a rule I have to always sleep on a changeset before pushing, but I expect any problems are more likely to be shaken out by testing on other machines rather than me thinking it through. Hence this is less of a "please put these changesets into your repo" and more of a "please consider these changes" request, with the assumption that there are problems in here that you may see easier than I.

Cheers

dkprice commented 8 years ago

Hi! I wanted to write in a quick comment to say I really appreciate this contribution. I'll be slow to review it though because my wife just had a baby and everything else has been put to the backburner =)

hardenedapple commented 8 years ago

Congratulations!!

and yeah, I think you've got your priorities in the right order :)