Git handling renamed files #1345

tulinkry · 2017-01-24T16:57:42Z

This should fix #1335 and it might fix #1258.
Implements #794

There should be no harm with this. If anything does not work, it will work like it did before - no renamed files tracking.

vladak · 2017-01-25T09:15:27Z

src/org/opensolaris/opengrok/history/GitRepository.java

+ List<String> cmd = new ArrayList<>();
+ cmd.add(RepoCommand);
+ cmd.add("log");
+ cmd.add("-M8"); // similarity 80%


why not use 100% ?

also, maybe use long variant of the option (--find-renames) so that it is more readable.

The default value is 50% so I pushed that up to 80%. 100% is to much I think...one can change the location of the file while changing some lines

vladak · 2017-01-25T10:40:16Z

src/org/opensolaris/opengrok/history/GitHistoryParser.java

+
+ @Override
+ public void processStream(InputStream input) throws IOException {
+ /*


maybe add the sequence of command that lead to this particular log

vladak · 2017-01-25T10:41:57Z

src/org/opensolaris/opengrok/history/GitHistoryParser.java

 entries.add(entry);
 }
-
- history.setHistoryEntries(entries);


why the setHistoryEntries() no longer necessary ?

because the history is now created as a return value directly

return new History(entries, parser.getRenamedFiles());

vladak · 2017-01-25T10:42:54Z

src/org/opensolaris/opengrok/history/GitHistoryParser.java

- throw new HistoryException("Failed to get history for: \"" +
- file.getAbsolutePath() + "\" Exit code: " + status);
+ throw new HistoryException("Failed to get history for: \""
+ + file.getAbsolutePath() + "\" Exit code: " + status);


not sure about the style here - where do we generally keep the operator when wrapping lines ?

This is what I get when I autoformat in netbeans with default settings. I changed those lines anyway to use String.format and not string composition.

vladak · 2017-01-25T10:44:32Z

src/org/opensolaris/opengrok/history/GitHistoryParser.java

+ try (BufferedReader in = new BufferedReader(new InputStreamReader(input))) {
+ String line;
+ while ((line = in.readLine()) != null) {
+ if (line.startsWith("R")) {


be more strict here - preferably use regexp matching

vladak · 2017-01-25T11:00:13Z

src/org/opensolaris/opengrok/history/GitRepository.java

+ origpath = findOriginalName(fullpath, rev);
+ } catch (IOException exp) {
+ LOGGER.log(Level.SEVERE,
+ "Failed to get original revision: {0}",


vladak · 2017-01-25T11:03:47Z

src/org/opensolaris/opengrok/history/GitRepository.java

+ protected String expandGitRenamedOutput(String renamedFile) {
+ // the pattern should cover format like this:
+ // moved/{renamed.c => renamed2.c} (100%)
+ Pattern pattern = Pattern.compile("\\s*([^\\{]+)\\s*\\{\\s*(.+?)\\s+=>\\s+([^\\}]+)\\s*\\}(.*)");


catch PatternSyntaxException exception just to be sure ?

the pattern is hardcoded so the exception can't occur

unless the compiler changes.. better to catch it

I forgot to mention that I changed the command to obtain more predictable output format (without these {, } constructs. So this method is no longer available :-)

vladak · 2017-01-25T11:09:16Z

src/org/opensolaris/opengrok/history/GitRepository.java

+ file = split[0];
+ }
+
+ if (rev.equals(changeset)) {


not sure how this is supposed work given the same piece of code on lines 360-362

mistake. removed

vladak · 2017-01-25T11:09:34Z

src/org/opensolaris/opengrok/history/GitRepository.java

+ if (split.length < 2) {
+ throw new IOException(
+ String.format(
+ "Invalid format of the git output. Expected "


add more info about the file path

vladak · 2017-01-25T11:10:58Z

test/org/opensolaris/opengrok/history/GitRepositoryTest.java

+ @Test
+ public void testRenamedFiles() throws Exception {
+ String[][] tests = new String[][]{
+ {"moved/renamed2.c", "67dfbe26", "moved/renamed2.c"},


what about testing renames across distinct directories ?

I added a test case

vladak · 2017-01-26T15:21:19Z

Could you do some basic perf. measurements for some Git repo with renames ? Also, you should see standalone git log processes to be forked for the renamed files.

tulinkry · 2017-01-27T12:43:24Z

Ok. I created a small testing repo and did some renames. These commands I executed to create the repository:

Basically:

the file in 1/file.txt got moved twice (to 4 and back to 1)
all of the files from 1 were moved to 4
file from 3/3/file.txt was moved to root and changed a little bit

also download repo.txt

mkdir renamed cd renamed/ mkdir 1 mkdir 2 mkdir 3 tr -dc A-Za-z0-9 </dev/urandom | head -c 20kB > 1/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 25kB > 2/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 3/file.txt mkdir -p {1,2,3}/{1,2,3} tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 1/1/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 1/2/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 1/3/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 2/1/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 2/2/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 2/3/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 3/1/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 3/2/file.txt tr -dc A-Za-z0-9 </dev/urandom | head -c 30kB > 3/3/file.txt git init git add 1/ git commit -m "first" git add 2/1/ git commit -m "second" git mv 1 4 git add 1 git add 1/ git add 4/ git commit -m "first rename" git add 2/2 git commit -m "third" git add 3 git commit "fourth" git commit -m "fourth" mkdir 1 mv 4/1/file.txt 1/ git add 4 git add 1 git commit -m "second rename" git add 2/3 git commit -m "fifth" mv 3/3/file.txt . cat file.txt | head -c 20Kb > tmp && cp tmp file.txt && rm tmp git add 3 git add file.txt git commit -m "third rename" git add 2/ git commit -m "sixth" git log git status git commit

Then I ran the index process from scratch for current master (without) and this pr (with).

$ time strace -f 2>&1 ./OpenGrok index | egrep "fork|exec.*" > exec-without.log real	0m2.776s user	0m2.740s sys	0m0.960s $ time strace -f 2>&1 ./OpenGrok index | egrep "fork|exec.*" > exec-with.log real	0m2.335s user	0m2.548s sys	0m1.092s

I did that several times, once the with version had like 5s. Most of the time it was same.

These are the outputs:

download exec-without.txt

[pid 16355] execve(["--help"]) [pid 16357] execve(["remote", "-v"]) [pid 16358] execve(["branch"]) [pid 16360] execve(["log", "-1", "--pretty=%cd: %h %an %s", "--date=rfc"]) [pid 16363] execve(["--help"]) [pid 16365] execve(["remote", "-v"]) [pid 16366] execve(["branch"]) [pid 16368] execve(["log", "-1", "--pretty=%cd: %h %an %s", "--date=rfc"]) [pid 16372] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc"]) [pid 16376] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "dbe628c0.."])

download exec-with.txt

[pid 16648] execve(["--help"] [pid 16650] execve(["remote", "-v"] [pid 16651] execve(["branch"] [pid 16653] execve(["log", "-1", "--pretty=%cd: %h %an %s", "--date=rfc"]) [pid 16656] execve(["--help"]) [pid 16658] execve(["remote", "-v"]) [pid 16659] execve(["branch"]) [pid 16661] execve(["log", "-1", "--pretty=%cd: %h %an %s", "--date=rfc"]) [pid 16665] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc"]) [pid 16668] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline"]) [pid 16675] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "--follow", "4/2/file.txt"]) [pid 16679] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "--follow", "file.txt"], [/ [pid 16680] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "--follow", "4/3/file.txt"], [pid 16686] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "--follow", "1/file.txt"], [pid 16688] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline", "--follow", "--", "4/3/file.txt"], [pid 16692] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline", "--follow", "--", "4/2/file.txt"], [pid 16697] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline", "--follow", "--", "file.txt"], [pid 16699] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline", "--follow", "--", "1/file.txt"], [pid 16703] execve(["log", "--abbrev-commit", "--abbrev=8", "--name-only", "--pretty=fuller", "--date=rfc", "dbe628c0.."]) [pid 16706] execve(["log", "--find-renames=8", "--summary", "--abbrev=8", "--name-status", "--oneline", "dbe628c0.."])

It's clearly visible that there is one extra git command for finding renamed files. All of the renamed files have their dedicated call to git log command to find their renamed history.

In the 'with' version the commands are duplicated. This is caused by the fact the the history generation calls generic getHistory and this method does not know if it is for renamed file or not. So our implementation must perform the traditional log command with --follow (which gets all the changesets) and then again find all renames of the given file (which is not necessary for a file but it is for a directory)

tulinkry · 2017-01-27T14:39:14Z

opengrok-indexer/build.xml

 <zip destfile="${build.test.classes.dir}/org/opensolaris/opengrok/history/repositories.zip" 
 basedir="${test.repositories}"
- excludes="mercurial/hg/**, mercurial/hgignore, git/git"
+ excludes="mercurial/hg/**, mercurial/hgignore, git/git/**"


For me the compiled test-classes still contained the both git and .git directory which caused a null revision error for getting annotations from the git repository (as they aren't tracked by git but still are in the repository).

This exact same line is in the root build.xml with these ** at the end.

tulinkry added 2 commits January 24, 2017 17:49

handling renamed files with git

199bcc4

tests for renamed files in git

f2cf2a1

tulinkry added the enhancement label Jan 24, 2017

vladak requested changes Jan 25, 2017

View reviewed changes

vladak mentioned this pull request Jan 26, 2017

Move GitRepository to JGit #1113

Closed

cleanup

a28d1ba

adding a testcase for renaming across directories

24102fe

tulinkry force-pushed the git-renamed branch from 65d4882 to 24102fe Compare January 27, 2017 14:05

fixing ignores in maven's indexer subproject

b1f0634

tulinkry commented Jan 27, 2017

View reviewed changes

vladak approved these changes Jan 28, 2017

View reviewed changes

vladak merged commit 58fe41a into oracle:master Jan 28, 2017

tulinkry deleted the git-renamed branch February 6, 2017 13:49

tulinkry mentioned this pull request Feb 8, 2017

generate history index for renamed files in git repo separately #794

Closed

vladak mentioned this pull request Feb 10, 2017

Is there a major change in generating the git history from 13-rc4 to 13-rc7 - peformance drawback? #1402

Closed

tulinkry mentioned this pull request Mar 28, 2017

Historycache generation is extremely long #1491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Git handling renamed files #1345

Git handling renamed files #1345

Uh oh!

tulinkry commented Jan 24, 2017 •

edited

Loading

vladak Jan 25, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017 •

edited

Loading

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 26, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak Jan 25, 2017

tulinkry Jan 26, 2017

vladak commented Jan 26, 2017

tulinkry commented Jan 27, 2017 •

edited

Loading

tulinkry Jan 27, 2017

Labels

2 participants

Git handling renamed files #1345

Git handling renamed files #1345

Uh oh!

Conversation

tulinkry commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tulinkry Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladak commented Jan 26, 2017

tulinkry commented Jan 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Labels

2 participants

tulinkry commented Jan 24, 2017 •

edited

Loading

tulinkry Jan 26, 2017 •

edited

Loading

tulinkry commented Jan 27, 2017 •

edited

Loading