Details

Reviewers

Group Reviewers

Commits

rP9e2f263bb49c: Add repositories to fulltext search index.

Summary

This implements a simplistic PhabricatorRepositoryFulltextEngine
Currently only the repository name, description, timestamps and
status are indexed.

Note: I had to change the search index workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit

Test Plan

ran ./bin/search index --type PhabricatorRepository --force
- searched for some repositories. Saw reasonable results matching on either title or description.
Edited a repository in the web ui
- Added unique key words to the repo description.
- I was then able to find that repo by searching for the new keywords.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

20after4 created this revision.Feb 2 2017, 6:34 PM

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptFeb 2 2017, 6:34 PM

Herald added a subscriber: Korvin. · View Herald Transcript

Harbormaster completed remote builds in B15507: Diff 41609.Feb 2 2017, 6:35 PM

20after4 added projects: Search, Diffusion.Feb 2 2017, 6:36 PM

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
10	This kind of documentation is unconventional in this codebase, and the docs say `$repository` while the actual variable is `$repo`. I think this is perfectly clear from context anyway, just nuke it?
15	Newline is probably a better delimiter than colon, since users might reasonably search for `"x: y"` and get a false/confusing positive here. (In theory, they can search for a newline too, I guess, but not reasonably using the normal UI).

This revision now requires changes to proceed.Feb 3 2017, 1:55 PM

In D17300#206257, @epriestley wrote:

Looks good. Couple of minor nits inline.

Your test plan should also include something like this:

Edited a repository to have "platypus" in the description, searched for "platypus", found the repository.

Even when bin/search index works, the actual "edit stuff with the web UI" workflow may still not be fully hooked up. Usually, this is because the Editor does not override supportsSearch(), so edits don't rebuild the index. In this case it looks like you're in the clear, so I'd expect this to work correctly with no further changes, but it's worth double checking.

Ok I'll test re-indexing, I think that works fine but you're right that I should have tested more thoroughly and mentioned it in my test plan. ;)

Also, thanks for pointing out "supportsSearch" I wasn't aware of the need to override that method.

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
10	I was trying to get code intelligence to act intelligently. It didn't work and I forgot to delete the comment. Code intelligence: still not very intelligent in 2017.
15	Good point.

20after4 edited the test plan for this revision. (Show Details)Mar 26 2017, 8:28 AM

Addressed epriestley's feedback.

Harbormaster failed remote builds in B16170: Diff 42245!Mar 26 2017, 8:42 AM

resubmit with arc diff --config repository.callsign=P

For some reason, arc can't determine the repo and skips staging unless I
explicitly specify the callsign.

Harbormaster completed remote builds in B16171: Diff 42246.Mar 26 2017, 8:45 AM

20after4 added inline comments.Mar 26 2017, 11:53 AM

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php
13	I guess I could actually add the field twice instead of concatenating with a \n, at least that works fine with elasticsearch. I wonder how it would work with mysql...

In terms of figuring out which repository your working copy is, what does arc which show?

One minor inline.

(I think the MySQL schema makes the field key (body, etc) unique, so you can't add multiple fields of the same type.)

src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php

'\n' in single quotes isn't a newline in PHP. 🐈

epriestley@orbital ~ $ cat test.php 
<?php

echo "With Double Quotes: <\n>";
echo "\n";
echo 'With Single Quotes: <\n>';
echo "\n";
epriestley@orbital ~ $ php -f test.php 
With Double Quotes: <
>
With Single Quotes: <\n>

This revision is now accepted and ready to land.Mar 26 2017, 12:11 PM

20after4 added inline comments.Mar 26 2017, 12:22 PM