[Findit] Bug fixing and implemented some feature requests.

[Findit]Bug fixing and implemented some feature requests.

Fixed various bugs, including exception on stacktrace and unnamed variable in git parser.
Changed the algorithm to use file_path only instead of file_name.
Removed URL links from the result.
Added commit message to the final result.
Updated OWNERS.

NOTRY=true

Committed: https://crrev.com/91fff9ef89202c9e3b33d4ececf648ec6839d0ab
Cr-Commit-Position: refs/heads/master@{#291698}

[jeun, 2014-08-21 22:16:46]

Hi,

I have committed the changed I made for supporting syzyasan/android and various
bug fixes. Would you mind taking a look at the code?

Thanks,
Jason

[Martin Barbella, 2014-08-22 02:24:14]

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py
File tools/findit/blame.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:127: crashed_line_number =
stack_frame.crashed_line_number[0]
I mention this in more detail in stacktrace.py, but the name crashed_line_number
is confusing here.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:134: if not parsed_blame_info or len(parsed_blame_info) !=
5:
It doesn't seem like a good practice to allow parsed_blame_info to ever return
something other than None or exactly 5 results. You can remove the length check
altogether so long as this is the case.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:74: except urllib2.HTTPError:
Please include some kind of retry logic here. Just define a constant for number
of attempts (something like 3-5 should be good enough).

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
File tools/findit/component_dictionary.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
tools/findit/component_dictionary.py:101: def
__CreateFileDictFromCallstack(self, callstack, top_n_frames=10):
Was there a specific reason that this was lowered?

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils.py
File tools/findit/crash_utils.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:175: # Strip 'r' off the range start/end. Not using
lstrip to avoid the case when
Maybe it would be best for us not to include the 'r' in the input that gets
passed to FindIt. Either way, this will work for now. Good catch.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:464: 'The CL changes line %s of file %s, which is
stack frame index %d.' %
Nit: remove "index".

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
File tools/findit/findit_for_clusterfuzz.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_clusterfuzz.py:140: if
utils.IsGitHash(component_regression_start):
This pattern seems to come up a lot. Add a helper to handle this with a name
like GetRepositoryType.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
File tools/findit/findit_for_crash.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:467: file_names = [file_name
Nit: just include all of this as a continuation on the next line (move file_name
down with the rest)

https://codereview.chromium.org/478763003/diff/20001/tools/findit/git_reposit...
File tools/findit/git_repository_parser.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/git_reposit...
tools/findit/git_repository_parser.py:99: if file_change_type == 'add':
Define a map to handle this and use it in both places where this logic is
needed.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace.py
File tools/findit/stacktrace.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:90: current_stack = CallStack(-1)
This seems pretty hacky. What was the issue here? There might be a better way to
handle this.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:116: if not (parsed_stack_frame and current_stack):
With the other change, would the "and current_stack" ever be relevant?

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:247: crashed_line_number =
[int(stack_frame_match.group(6))]
Maybe rename this here and in StackFrame to crashed_line_range to make its
purpose clearer. Also, for consistency, it might make sense to always define
these as ranges. Having different formats for this for different build types
adds a bit of complexity.

[stgao, 2014-08-22 06:50:54]

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py
File tools/findit/blame.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:134: if not parsed_blame_info or len(parsed_blame_info) !=
5:
+1

Why do we use 5 here?

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
File tools/findit/component_dictionary.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
tools/findit/component_dictionary.py:101: def
__CreateFileDictFromCallstack(self, callstack, top_n_frames=10):
On 2014/08/22 02:24:13, mbarbella wrote:
> Was there a specific reason that this was lowered?

+1

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils.py
File tools/findit/crash_utils.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:94: and a path in that component's repository.
Returns None if the component
It seems googlecode is not checked.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:98: normalized_path =
os.path.normpath(path.replace('\\','/'))
Please also update the comment.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:219: data = urllib2.urlopen(url, timeout=timeout)
Is there a specific reason to revert my change to use utils.GetHttpClient?

Forgot to update your code to include my change?

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
File tools/findit/findit_for_crash.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:15: import svn_repository_parser
duplicate

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:149: changed_file_name =
changed_file_path.split('/')[-1].lower()
use os.path.basename instead.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:359: match.min_distance,
Why we do this change?

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:611: return_message = ('Findit could not find
any suspected CLs.')
Please add a comment on why we would reach this case.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/match_set.py
File tools/findit/match_set.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/match_set.p...
tools/findit/match_set.py:36: priorities: A list of priorities for each of the
changed file.
Please add a description on how priority is defined, something like the deleted
description for |rank|.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace.py
File tools/findit/stacktrace.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:281: # For example, 655:1 has crashed lines 655 and
656.
How did we know ":1" mean a range?
Is it a function call that spread through multiple lines?

If so, please add an example of code here.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:297: file_name = file_path.split('/')[-1]
os.path.basename

https://codereview.chromium.org/478763003/diff/20001/tools/findit/svn_reposit...
File tools/findit/svn_repository_parser.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/svn_reposit...
tools/findit/svn_repository_parser.py:80: # Get path, file action and file name
from the xml.
comment is not updated.

[jeun, 2014-08-22 22:58:44]

Hi Shuotao,

I have addressed the codereview. Would you mind a quick code review?

Thanks!

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py
File tools/findit/blame.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:127: crashed_line_number =
stack_frame.crashed_line_number[0]
On 2014/08/22 02:24:13, mbarbella wrote:
> I mention this in more detail in stacktrace.py, but the name
crashed_line_number
> is confusing here.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:134: if not parsed_blame_info or len(parsed_blame_info) !=
5:
On 2014/08/22 06:50:53, Shuotao wrote:
> +1
> 
> Why do we use 5 here?

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/blame.py#ne...
tools/findit/blame.py:134: if not parsed_blame_info or len(parsed_blame_info) !=
5:
On 2014/08/22 02:24:13, mbarbella wrote:
> It doesn't seem like a good practice to allow parsed_blame_info to ever return
> something other than None or exactly 5 results. You can remove the length
check
> altogether so long as this is the case.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/chromium_de...
File tools/findit/chromium_deps.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/chromium_de...
tools/findit/chromium_deps.py:74: except urllib2.HTTPError:
On 2014/08/22 02:24:13, mbarbella wrote:
> Please include some kind of retry logic here. Just define a constant for
number
> of attempts (something like 3-5 should be good enough).

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
File tools/findit/component_dictionary.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
tools/findit/component_dictionary.py:101: def
__CreateFileDictFromCallstack(self, callstack, top_n_frames=10):
On 2014/08/22 02:24:13, mbarbella wrote:
> Was there a specific reason that this was lowered?

It was by Abhishek's request, he said use top 7 so I changed to 10 (I have seen
certain cases where something at 8-9 is responsible, but not beyond that)

https://codereview.chromium.org/478763003/diff/20001/tools/findit/component_d...
tools/findit/component_dictionary.py:101: def
__CreateFileDictFromCallstack(self, callstack, top_n_frames=10):
On 2014/08/22 06:50:53, Shuotao wrote:
> On 2014/08/22 02:24:13, mbarbella wrote:
> > Was there a specific reason that this was lowered?
> 
> +1

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils.py
File tools/findit/crash_utils.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:94: and a path in that component's repository.
Returns None if the component
On 2014/08/22 06:50:53, Shuotao wrote:
> It seems googlecode is not checked.

Even if we don't check it here, it will fail later, so I thought it would not be
necessary to check it twice.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:98: normalized_path =
os.path.normpath(path.replace('\\','/'))
On 2014/08/22 06:50:53, Shuotao wrote:
> Please also update the comment.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:175: # Strip 'r' off the range start/end. Not using
lstrip to avoid the case when
On 2014/08/22 02:24:14, mbarbella wrote:
> Maybe it would be best for us not to include the 'r' in the input that gets
> passed to FindIt. Either way, this will work for now. Good catch.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:219: data = urllib2.urlopen(url, timeout=timeout)
On 2014/08/22 06:50:53, Shuotao wrote:
> Is there a specific reason to revert my change to use utils.GetHttpClient?
> 
> Forgot to update your code to include my change?

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/crash_utils...
tools/findit/crash_utils.py:464: 'The CL changes line %s of file %s, which is
stack frame index %d.' %
On 2014/08/22 02:24:14, mbarbella wrote:
> Nit: remove "index".

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
File tools/findit/findit_for_clusterfuzz.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_clusterfuzz.py:140: if
utils.IsGitHash(component_regression_start):
On 2014/08/22 02:24:14, mbarbella wrote:
> This pattern seems to come up a lot. Add a helper to handle this with a name
> like GetRepositoryType.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
File tools/findit/findit_for_crash.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:15: import svn_repository_parser
On 2014/08/22 06:50:54, Shuotao wrote:
> duplicate

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:149: changed_file_name =
changed_file_path.split('/')[-1].lower()
On 2014/08/22 06:50:54, Shuotao wrote:
> use os.path.basename instead.

Changed to split('/') so that it would also work on windows machine.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:359: match.min_distance,
On 2014/08/22 06:50:53, Shuotao wrote:
> Why we do this change?

Abhishek suggested it.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:467: file_names = [file_name
On 2014/08/22 02:24:14, mbarbella wrote:
> Nit: just include all of this as a continuation on the next line (move
file_name
> down with the rest)

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/findit_for_...
tools/findit/findit_for_crash.py:611: return_message = ('Findit could not find
any suspected CLs.')
On 2014/08/22 06:50:54, Shuotao wrote:
> Please add a comment on why we would reach this case.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/git_reposit...
File tools/findit/git_repository_parser.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/git_reposit...
tools/findit/git_repository_parser.py:99: if file_change_type == 'add':
On 2014/08/22 02:24:14, mbarbella wrote:
> Define a map to handle this and use it in both places where this logic is
> needed.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/match_set.py
File tools/findit/match_set.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/match_set.p...
tools/findit/match_set.py:36: priorities: A list of priorities for each of the
changed file.
On 2014/08/22 06:50:54, Shuotao wrote:
> Please add a description on how priority is defined, something like the
deleted
> description for |rank|.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace.py
File tools/findit/stacktrace.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:90: current_stack = CallStack(-1)
On 2014/08/22 02:24:14, mbarbella wrote:
> This seems pretty hacky. What was the issue here? There might be a better way
to
> handle this.
This is to bypass UBSan (and possibly some other builds) for now, because I do
not know all the patterns. It is better to, instead of missing all the stack
frames because I don't know the stack start pattern, just add the frames and
compute the result.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:116: if not (parsed_stack_frame and current_stack):
On 2014/08/22 02:24:14, mbarbella wrote:
> With the other change, would the "and current_stack" ever be relevant?

No, removed.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:247: crashed_line_number =
[int(stack_frame_match.group(6))]
On 2014/08/22 02:24:14, mbarbella wrote:
> Maybe rename this here and in StackFrame to crashed_line_range to make its
> purpose clearer. Also, for consistency, it might make sense to always define
> these as ranges. Having different formats for this for different build types
> adds a bit of complexity.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:281: # For example, 655:1 has crashed lines 655 and
656.
On 2014/08/22 06:50:54, Shuotao wrote:
> How did we know ":1" mean a range?
> Is it a function call that spread through multiple lines?
> 
> If so, please add an example of code here.

Done.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/stacktrace....
tools/findit/stacktrace.py:297: file_name = file_path.split('/')[-1]
On 2014/08/22 06:50:54, Shuotao wrote:
> os.path.basename

This changed to split('/')[-1] so that the code will work on Windows.

https://codereview.chromium.org/478763003/diff/20001/tools/findit/svn_reposit...
File tools/findit/svn_repository_parser.py (right):

https://codereview.chromium.org/478763003/diff/20001/tools/findit/svn_reposit...
tools/findit/svn_repository_parser.py:80: # Get path, file action and file name
from the xml.
On 2014/08/22 06:50:54, Shuotao wrote:
> comment is not updated.

Done.

[stgao, 2014-08-23 00:44:13]

lgtm for committing.
Will fix minor issues later.

https://codereview.chromium.org/478763003/diff/40001/tools/findit/crash_utils.py
File tools/findit/crash_utils.py (right):

https://codereview.chromium.org/478763003/diff/40001/tools/findit/crash_utils...
tools/findit/crash_utils.py:390: function: A list of functions that the file
crashed on.
function -> functions

[stgao, 2014-08-25 16:50:49]

I'm gonna to commit this CL.

There are quite a few issues to be resolved.
But let's do it later.

[stgao, 2014-08-25 16:51:09]

The CQ bit was checked by stgao@chromium.org

[commit-bot: I haz the power, 2014-08-25 16:51:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jeun@chromium.org/478763003/40001

[commit-bot: I haz the power, 2014-08-25 16:52:29]

Committed patchset #3 (40001) as e2e208cf6f0ab95c676f58f88e60214eb0e9bba4

[commit-bot: I haz the power, 2014-09-10 02:35:05]

Patchset 3 (id:??) landed as
https://crrev.com/91fff9ef89202c9e3b33d4ececf648ec6839d0ab
Cr-Commit-Position: refs/heads/master@{#291698}