src: add grep utility to explore feature #178

melissawen · 2020-05-14T11:34:24Z

This patch expands the feature -explore- adding the grep utility
for searching plain-text data in files that are not under git.
Related to #61

Signed-off-by: Melissa Wen melissa.srw@gmail.com

melissawen · 2020-05-14T11:34:49Z

@rodrigosiqueira

rodrigosiqueira

First of all, thanks a lot for your patch!

I added some comments in the code, but I want to add other topics:

Remember to update the documentation at: documentation/man/kw.rst
Add a new help entry
In your commit message, replace: "Related to #61" for "Closes #61"

rodrigosiqueira · 2020-05-14T23:54:42Z

src/explore.sh

  if [[ "$1" =~ ^--log$ && ! -z "$2" ]]; then
    return 1
  fi
+


There is an extra space here

rodrigosiqueira · 2020-05-14T23:55:38Z

src/explore.sh


  cmd_manager "$flag" "git grep -e \"$regex\" -nI $path"
 }
+


You forgot to add the function documentation.

rodrigosiqueira · 2020-05-15T00:04:05Z

src/explore.sh

    return 1
  fi
+
+  if [[ "$1" =~ ^--grep$ && ! -z "$2" ]]; then


in this case, it could be nice if we also support -g, i.e., kw e -g lala or kw e --grep lala.

rodrigosiqueira · 2020-05-15T00:07:02Z

src/explore.sh

+  # If user only set regex value
+  path=${path:-"."}
+
+  cmd_manager "$flag" "grep -nrwI $path -e \"$regex\""


Since you decided to use cmd_manager you can use the flag TEST_MODE for implementing a new test that validate the command correctness. Take a look at mk_test, test mk_kernel_uninstall_Test for reference.

rodrigosiqueira · 2020-05-15T00:09:01Z

tests/explore_test.sh

    git commit -m "Commit number $commit" &> /dev/null
  done
+
+  echo "##regex_sample##" > e_grep_test.txt


To make this test a little bit more robust, I recommend you to add a full text in tests/samples and use it for validating your grep function.

rodrigosiqueira · 2020-05-15T00:26:49Z

Also, since you're working at this part of the code could you also take a look at this issue: #92

matheustavares · 2020-05-15T16:36:59Z

src/explore.sh

+  # If user only set regex value
+  path=${path:-"."}
+
+  cmd_manager "$flag" "grep -nrwI $path -e \"$regex\""


We currently don use the -w flag in the "git grepping mode". So, for consistency, I think we should remove it here or add it in the git-grep search as well. Personally, I prefer not to use -w, since we wouldn't allow users to search for "non-words". And they can always resort to something like "\bpattern\b", to simulate the same effect, without -w.

I agree with the homogeneity for the feature as a whole. So, I will remove -w in my next version

matheustavares · 2020-05-15T17:33:48Z

Hi, @melissawen and @rodrigosiqueira . First of all, thanks for the new feature :)

Two ideas came to my mind, regarding implementation. Please, let me know what you think about them:

git-grep has an option to search outside Git repositories: the --no-index flag. So we could keep using git-grep for this new case as well. One advantage is that we can ensure the same behavior among all our "explore" cases, since we would be using the same program. Also, I think (but not sure) git-grep should be faster than GNU grep for some cases.
What if, instead of providing --grep, we automatically detect when we are inside/outside a repository's working tree and use the respective grepping option, in either case? We could do this test with a function like the following:

function in_git_work_tree()
{
  local ret="$(git rev-parse --is-inside-work-tree 2>&1)"
    if [[ $? == 0 && "$ret" == "true" ]]; then
      return 0
    fi
    return 1
}

melissawen · 2020-05-18T09:09:05Z

Hi @matheustavares,

I appreciate your suggestion with --no-index. For me, I can refactor this patch (or do another one) to use this parameter, as I don't see another employment for grep utility other than searching in files that are not under git control.
@rodrigosiqueira, what do you think?

rodrigosiqueira · 2020-05-18T12:33:45Z

Hi @melissawen and @matheustavares

Great idea Matheus! I believe that in_git_work_tree is a very useful function and should be in kwlib for making it available for other parts of kw.

Additionally and IMHO it does not hurt to provide an extra option for letting users explicitly say that they want to search something not under git control. For example, some times, I want to search on some metadata in the Linux kernel (under git control) that I know that is not under git control.

melissawen · 2020-05-18T14:15:46Z

I agree. I think the user should decide to search in files under or not under git control. But I think we can handle it only using git tool, with --no-index (without adding grep tool). Is it good?

rodrigosiqueira · 2020-05-18T14:39:45Z

I recommend that you follow Matheus's suggestion, but also add the option for users to ignore git repository (optional). Finally, I also agree to use --no-index.

matheustavares · 2020-05-18T18:13:21Z

Oh, there is one caveat I just noticed: --no-index greps all files in the current directory, but if there is a .git, it ignores it. So if we want to allow users to search inside .git, as well, we might have to use GNU grep.

matheustavares · 2020-05-18T18:14:23Z

Oops, I accidentally hit "Close and Comment". Sorry for that! Reopening...

melissawen · 2020-05-19T20:54:17Z

Hi,

Just feedback:
I'm working on it; however, I saw a problem in_git_work_tree, since rev-parse returns a fatal error (instead of false) when in a folder that is not under git control (as reported at https://groups.google.com/forum/#!topic/git-users/dWc23LFhWxE). Maybe I will take a while to find a solution. :)

matheustavares · 2020-05-19T22:04:04Z

Hi @melissawen. Yes, git rev-parse will die with an exit code different than 0 when outside a Git repository. But the is_git_work_tree function should already deal with this case, in the if [[ $? == 0 .. section, right? Or is this check falling in your tests?

melissawen · 2020-05-19T22:17:42Z

Hi @matheustavares, the original function failed my tests. However, I adjusted it to deal with the problem :) I will send everything after applying all the suggestions. Thanks :)

rodrigosiqueira

Hi @melissawen ,

First of all, thanks for updating your patch! You had a lot of progress :)

You can see all my comments per change in your commit, please take a careful look in all of them; I tried to provide a detailed explanation. Aside from the inline comments, I want to add:

If you use vim, try to enable the option set list to see extra spaces in your code. Alternatively, you can use git log -p after commit your change and double-check for extra space at the end of lines.
Add extra tests that exercise the following cases:

Search in a non-git directory: In the setup phase, create a folder without git repo and write a test that validates kw e something. This test going to exercise the git grep --no-index -e
Search in a git repository project: You can create a setup case in which you create a git repository, commit a change, and try to search for something. This will validate git grep -e

Hint: When you write a test, try to think about how you going to pass through each line of your code. How you going to pass in each if, elif, else, and so forth.

rodrigosiqueira · 2020-05-23T15:09:37Z

src/explore.sh

-# This function manages the search inside files and under the git repository
-# control.
+# This function is responsible for checking if the folder is under git control.
+function in_git_work_tree


See the comments about this function implementation below.

rodrigosiqueira · 2020-05-23T15:27:50Z

src/explore.sh


+  # If user only set regex value
+  path=${path:-"."}
+  if [[ $(in_git_work_tree) ]]; then


This condition is always false because $(in_git_work_tree) is empty. Inside in_git_work_tree you did not return anything, exit 1 or exit 0 only change the value inside the variable $? and with $(in_git_work_tree) you're not checking for this value. If you want to test what I mean, just create tags (make tags) in your kernel repository and try to use your implementation for searching vkms_vblank_simulate; in my case I got:

kw e vkms_vblank_simulate drivers/gpu/drm/vkms/ drivers/gpu/drm/vkms/vkms_crtc.c:10:static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) drivers/gpu/drm/vkms/vkms_crtc.c:65: out->vblank_hrtimer.function = &vkms_vblank_simulate; tags:4871065:vkms_vblank_simulate drivers/gpu/drm/vkms/vkms_crtc.c /^static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)$/;" f typeref:enum:hrtimer_restart file: signature:(struct hrtimer * timer) rtimer_restart file: signature:(struct hrtimer * timer)

My suggestion for fixing this issue:

# This function is responsible for checking if the folder is under git control. function in_git_work_tree { output=$(git rev-parse --is-inside-work-tree 2>/dev/null) ret="$?" # If git rev-parse --is-inside-work-tree executes without issue it will # return 0, however, we can get false as an output. if [[ "$ret" -eq 0 && "$output" -eq "true" ]]; then return 0 fi return 1 } # This function detects if the files are under the git repository # control and properly handles the search using git grep tool. # # @regex Specifies the regex that we want to search in the files # @path Narrow down the search # @flag How to display a command, the default value is "SILENT". For more # options see `src/kwlib.sh` function `cmd_manager` function explore_files_under_git() { local regex="$1" local path="$2" local flag="$3" # Silent by default flag=${flag:-"SILENT"} # If user only set regex value path=${path:-"."} output=$(in_git_work_tree) if [[ "$?" != 0 ]]; then cmd_manager "$flag" "git grep --no-index -e \"$regex\" -nI $patch" else cmd_manager "$flag" "git grep -e \"$regex\" -nI $path" fi }

rodrigosiqueira · 2020-05-23T15:28:46Z

src/explore.sh


-# This function manages the search inside files and under the git repository
-# control.
+# This function is responsible for checking if the folder is under git control.


Document the return value, for example

... # Returns: # Explain the return

rodrigosiqueira · 2020-05-23T15:31:52Z

src/explore.sh

  path=${path:-"."}

-  cmd_manager "$flag" "git grep -e \"$regex\" -nI $path"
+  cmd_manager "$flag" "grep -nrI $path -e \"$regex\""


Add the flag --color to make the final results a little bit better.

rodrigosiqueira · 2020-05-23T15:33:49Z

documentation/man/kw.rst

 directory.

-e, explore [--log] [*EXPRESSION*] [-p] [*DIRECTORY|FILE*]
+e, explore [--log | --grep, -g] [*EXPRESSION*] [-p] [*DIRECTORY|FILE*]


You should extend the below line with ~~~~~, otherwise you will break the documentation style. For example, try kw man after installing your changes and you will see that this section is broken.

rodrigosiqueira · 2020-05-23T15:51:14Z

tests/explore_test.sh

  suite_addTest "explore_files_under_git_repo_Test"
  suite_addTest "explore_git_log_Test"
  suite_addTest "explore_parser_Test"
+  suite_addTest "explore_grep_Test"


In your code you used cmd_manager, this feature provides support for TEST_MODE which enables us to test the command sequence. I recommend you to add extra tests taking advantage of this feature, I wrote the below example for helping you to understand how to do it:

function explore_grep_cmd_Test { local ID local -r current_path="$PWD" local expected_cmd="grep --color -nrI . -e \"GNU grep\"" cd "$test_path" ID=1 output=$(explore --grep "GNU grep" "." "TEST_MODE") assertEquals "($ID)" "$expected_cmd" "$output" cd "$current_path" }

rodrigosiqueira · 2020-05-23T15:51:52Z

tests/explore_test.sh

+  local file_name
+  local regex_sample


You did not use these variables, you can drop it.

This patch detects if a directory isn't under git control to expand coverage of git grep and also adds gnu grep option for catching files in hidden directories Closes kworkflow#61 Co-developed-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br> Signed-off-by: Melissa Wen <melissa.srw@gmail.com>

rodrigosiqueira · 2020-06-07T13:50:09Z

Hi @melissawen

Thanks a lot for your patch! It is already merged, see at:

d131585

rodrigosiqueira requested changes May 15, 2020

View reviewed changes

matheustavares reviewed May 15, 2020

View reviewed changes

matheustavares closed this May 18, 2020

matheustavares reopened this May 18, 2020

melissawen force-pushed the add_grep_to_explore branch 2 times, most recently from ca6099e to 01c1841 Compare May 21, 2020 16:41

rodrigosiqueira requested changes May 23, 2020

View reviewed changes

melissawen force-pushed the add_grep_to_explore branch 2 times, most recently from 632c264 to 8d00c2c Compare May 25, 2020 14:14

melissawen force-pushed the add_grep_to_explore branch from 8d00c2c to 986a1b1 Compare May 25, 2020 21:46

rodrigosiqueira closed this Jun 7, 2020

src: add grep utility to explore feature #178

src: add grep utility to explore feature #178

Uh oh!

Conversation

melissawen commented May 14, 2020

Uh oh!

melissawen commented May 14, 2020

Uh oh!

rodrigosiqueira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigosiqueira commented May 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheustavares commented May 15, 2020

Uh oh!

melissawen commented May 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigosiqueira commented May 18, 2020

Uh oh!

melissawen commented May 18, 2020

Uh oh!

rodrigosiqueira commented May 18, 2020

Uh oh!

matheustavares commented May 18, 2020

Uh oh!

matheustavares commented May 18, 2020

Uh oh!

melissawen commented May 19, 2020

Uh oh!

matheustavares commented May 19, 2020

Uh oh!

melissawen commented May 19, 2020

Uh oh!

rodrigosiqueira left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodrigosiqueira commented Jun 7, 2020

Uh oh!

Uh oh!

melissawen commented May 18, 2020 •

edited

Loading