Skip to content

use --follow without -r for getting history of renamed files in Mercu… #1469

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Mar 15, 2017
Merged

use --follow without -r for getting history of renamed files in Mercu… #1469

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3758ba5
use --follow without -r for getting history of renamed files in Mercu…
vladak Mar 1, 2017
deeaeb7
reverse
vladak Mar 15, 2017
003bb9b
copyright
vladak Mar 15, 2017
b68e3b0
add comment
vladak Mar 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 78 additions & 64 deletions src/org/opensolaris/opengrok/history/FileHistoryCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

/*
* Copyright (c) 2008, 2016, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2008, 2017, Oracle and/or its affiliates. All rights reserved.
*/

package org.opensolaris.opengrok.history;
Expand Down Expand Up @@ -80,40 +80,42 @@ public boolean isHistoryIndexDone() {

Copy link
Contributor

@tulinkry tulinkry Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copyright

Copy link
Member Author

@vladak vladak Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

/**
* Generate history for single file.
* @param map_entry entry mapping filename to list of history entries
* @param filename name of the file
* @param historyEntries list of HistoryEntry objects forming the (incremental) history of the file
* @param env runtime environment
* @param repository repository object in which the file belongs
* @param test file object
* @param srcFile file object
* @param root root of the source repository
* @param renamed true if the files was renamed in the past
* @param renamed true if the file was renamed in the past
*/
private void doFileHistory(Map.Entry<String, List<HistoryEntry>> map_entry,
private void doFileHistory(String filename, List<HistoryEntry> historyEntries,
RuntimeEnvironment env, Repository repository,
File test, File root, boolean renamed) throws HistoryException {
File srcFile, File root, boolean renamed) throws HistoryException {

History hist = null;

/*
* Certain files require special handling - this is mainly for
* files which have been renamed in Mercurial repository.
* This ensures that their complete history (follow) will be
* saved.
* If the file was renamed (in the changesets that are being indexed),
* its history is not stored in the historyEntries so it needs to be acquired
* directly from the repository.
* This ensures that complete history of the file (across renames)
* will be saved.
*/
if (renamed) {
hist = repository.getHistory(test);
hist = repository.getHistory(srcFile);
}

if (hist == null) {
hist = new History();

// File based history cache does not store files for individual
// changesets so strip them.
for (HistoryEntry ent : map_entry.getValue()) {
for (HistoryEntry ent : historyEntries) {
ent.strip();
}

// add all history entries
hist.setHistoryEntries(map_entry.getValue());
hist.setHistoryEntries(historyEntries);
} else {
for (HistoryEntry ent : hist.getHistoryEntries()) {
ent.strip();
Expand All @@ -125,20 +127,19 @@ private void doFileHistory(Map.Entry<String, List<HistoryEntry>> map_entry,
repository.assignTagsInHistory(hist);
}

File file = new File(root, map_entry.getKey());
File file = new File(root, filename);
if (!file.isDirectory()) {
storeFile(hist, file, repository);
}
}

private boolean isRenamedFile(Map.Entry<String,
List<HistoryEntry>> map_entry, RuntimeEnvironment env,
private boolean isRenamedFile(String filename,
RuntimeEnvironment env,
Repository repository, History history) throws IOException {

String fullfile = map_entry.getKey();
String repodir = env.getPathRelativeToSourceRoot(
new File(repository.getDirectoryName()), 0);
String shortestfile = fullfile.substring(repodir.length() + 1);
String shortestfile = filename.substring(repodir.length() + 1);

return (history.isRenamed(shortestfile));
}
Expand Down Expand Up @@ -211,6 +212,55 @@ private static History readCache(File file) throws IOException {
}
}

/**
* Store history in file on disk.
* @param dir directory where the file will be saved
* @param history history to store
* @param cacheFile the file to store the history to
* @throws HistoryException
*/
private void writeHistoryToFile(File dir, History history, File cacheFile) throws HistoryException {
// We have a problem that multiple threads may access the cache layer
// at the same time. Since I would like to avoid read-locking, I just
// serialize the write access to the cache file. The generation of the
// cache file would most likely be executed during index generation, and
// that happens sequencial anyway....
// Generate the file with a temporary name and move it into place when
// I'm done so I don't have to protect the readers for partially updated
// files...
final File output;
try {
output = File.createTempFile("oghist", null, dir);
try (FileOutputStream out = new FileOutputStream(output);
XMLEncoder e = new XMLEncoder(
new BufferedOutputStream(
new GZIPOutputStream(out)))) {
e.setPersistenceDelegate(File.class,
new FilePersistenceDelegate());
e.writeObject(history);
}
} catch (IOException ioe) {
throw new HistoryException("Failed to write history", ioe);
}
synchronized (lock) {
if (!cacheFile.delete() && cacheFile.exists()) {
if (!output.delete()) {
LOGGER.log(Level.WARNING,
"Failed to remove temporary history cache file");
}
throw new HistoryException(
"Cachefile exists, and I could not delete it.");
}
if (!output.renameTo(cacheFile)) {
if (!output.delete()) {
LOGGER.log(Level.WARNING,
"Failed to remove temporary history cache file");
}
throw new HistoryException("Failed to rename cache tmpfile.");
}
}
}

/**
* Store history object (encoded as XML and compressed with gzip) in a file.
*
Expand All @@ -221,10 +271,10 @@ private static History readCache(File file) throws IOException {
*/
private void storeFile(History histNew, File file, Repository repo) throws HistoryException {

File cache = getCachedFile(file);
File cacheFile = getCachedFile(file);
History history = histNew;

File dir = cache.getParentFile();
File dir = cacheFile.getParentFile();
if (!dir.isDirectory() && !dir.mkdirs()) {
throw new HistoryException(
"Unable to create cache directory '" + dir + "'.");
Expand All @@ -233,7 +283,7 @@ private void storeFile(History histNew, File file, Repository repo) throws Histo
// Incremental update of the history for this file.
History histOld;
try {
histOld = readCache(cache);
histOld = readCache(cacheFile);
// Merge old history with the new history.
List<HistoryEntry> listOld = histOld.getHistoryEntries();
if (!listOld.isEmpty()) {
Expand Down Expand Up @@ -264,45 +314,7 @@ private void storeFile(History histNew, File file, Repository repo) throws Histo
// the data to do it here.
}

// We have a problem that multiple threads may access the cache layer
// at the same time. Since I would like to avoid read-locking, I just
// serialize the write access to the cache file. The generation of the
// cache file would most likely be executed during index generation, and
// that happens sequencial anyway....
// Generate the file with a temporary name and move it into place when
// I'm done so I don't have to protect the readers for partially updated
// files...
final File output;
try {
output = File.createTempFile("oghist", null, dir);
try (FileOutputStream out = new FileOutputStream(output);
XMLEncoder e = new XMLEncoder(
new BufferedOutputStream(
new GZIPOutputStream(out)))) {
e.setPersistenceDelegate(File.class,
new FilePersistenceDelegate());
e.writeObject(history);
}
} catch (IOException ioe) {
throw new HistoryException("Failed to write history", ioe);
}
synchronized (lock) {
if (!cache.delete() && cache.exists()) {
if (!output.delete()) {
LOGGER.log(Level.WARNING,
"Failed to remove temporary history cache file");
}
throw new HistoryException(
"Cachefile exists, and I could not delete it.");
}
if (!output.renameTo(cache)) {
if (!output.delete()) {
LOGGER.log(Level.WARNING,
"Failed to remove temporary history cache file");
}
throw new HistoryException("Failed to rename cache tmpfile.");
}
}
writeHistoryToFile(dir, history, cacheFile);
}

private void finishStore(Repository repository, String latestRev) {
Expand Down Expand Up @@ -391,15 +403,16 @@ public void store(History history, Repository repository)
for (Map.Entry<String, List<HistoryEntry>> map_entry : map.entrySet()) {
try {
if (env.isHandleHistoryOfRenamedFiles() &&
isRenamedFile(map_entry, env, repository, history)) {
isRenamedFile(map_entry.getKey(), env, repository, history)) {
continue;
}
} catch (IOException ex) {
LOGGER.log(Level.WARNING,
"isRenamedFile() got exception " , ex);
}

doFileHistory(map_entry, env, repository, null, root, false);
doFileHistory(map_entry.getKey(), map_entry.getValue(),
env, repository, null, root, false);
}

if (!env.isHandleHistoryOfRenamedFiles()) {
Expand All @@ -414,7 +427,7 @@ public void store(History history, Repository repository)
new HashMap<>();
for (final Map.Entry<String, List<HistoryEntry>> map_entry : map.entrySet()) {
try {
if (isRenamedFile(map_entry, env, repository, history)) {
if (isRenamedFile(map_entry.getKey(), env, repository, history)) {
renamed_map.put(map_entry.getKey(), map_entry.getValue());
}
} catch (IOException ex) {
Expand Down Expand Up @@ -442,7 +455,8 @@ public void store(History history, Repository repository)
@Override
public void run() {
try {
doFileHistory(map_entry, env, repositoryF,
doFileHistory(map_entry.getKey(), map_entry.getValue(),
env, repositoryF,
new File(env.getSourceRootPath() + map_entry.getKey()),
root, true);
} catch (Exception ex) {
Copy link
Contributor

@tulinkry tulinkry Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes here are just refactoring?

Copy link
Member Author

@vladak vladak Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, using map_entry to pass the repo name and history made the code hard to read.

Expand Down
74 changes: 44 additions & 30 deletions src/org/opensolaris/opengrok/history/MercurialRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.io.InputStream;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.TreeSet;
Expand Down Expand Up @@ -143,7 +144,8 @@ String determineBranch() throws IOException {
*
* @param file The file or directory to retrieve history for
* @param sinceRevision the oldest changeset to return from the executor, or
* {@code null} if all changesets should be returned
* {@code null} if all changesets should be returned.
* For files this does not apply and full history is returned.
* @return An Executor ready to be started
*/
Executor getHistoryLogExecutor(File file, String sinceRevision)
Expand All @@ -161,28 +163,35 @@ Executor getHistoryLogExecutor(File file, String sinceRevision)
cmd.add(RepoCommand);
cmd.add("log");

// For plain files we would like to follow the complete history
// (this is necessary for getting the original name in given revision
// when handling renamed files)
if (!file.isDirectory()) {
cmd.add("-f");
}

// If this is non-default branch we would like to get the changesets
// on that branch and also any changesets from the parent branch(es).
if (sinceRevision != null) {
cmd.add("-r");
String[] parts = sinceRevision.split(":");
if (parts.length == 2) {
cmd.add("reverse(" + parts[0] + "::'" + getBranch() + "')");
if (file.isDirectory()) {
// If this is non-default branch we would like to get the changesets
// on that branch and also follow any changesets from the parent branch.
if (sinceRevision != null) {
cmd.add("-r");
String[] parts = sinceRevision.split(":");
if (parts.length == 2) {
cmd.add("reverse(" + parts[0] + "::'" + getBranch() + "')");
} else {
throw new HistoryException(
"Don't know how to parse changeset identifier: "
+ sinceRevision);
}
} else {
throw new HistoryException(
"Don't know how to parse changeset identifier: "
+ sinceRevision);
cmd.add("-r");
cmd.add("reverse(0::'" + getBranch() + "')");
}
} else {
cmd.add("-r");
cmd.add("reverse(0::'" + getBranch() + "')");
// For plain files we would like to follow the complete history
// (this is necessary for getting the original name in given revision
// when handling renamed files)
// It is not needed to filter on a branch as 'hg log' will follow
// the active branch.
// Due to behavior of recent Mercurial versions, it is not possible
// to filter the changesets of a file based on revision.
// For files this does not matter since if getHistory() is called
// for a file, the file has to be renamed so we want its complete history.
Copy link
Contributor

@tulinkry tulinkry Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For files this does not matter since if getHistory() is called
for a file, the file has to be renamed so we want its complete history.

This should be part of the javadoc for this function

Copy link
Member Author

@vladak vladak Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added.

cmd.add("--follow");
cmd.add(filename);
Copy link
Contributor

@tulinkry tulinkry Mar 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the whole trick here was just about using only --follow instead of -f -r reverse ... ?

Copy link
Member Author

@vladak vladak Mar 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. Using --follow by itself will guarantee that:

  • the changesets are displayed in correct order (important for history log parser)
  • changesets in the active branch plus all parent branches are displayed

At the same time, it is not necessary to strip the revisions since sinceRevision since these are not needed for renamed file and getHistory for file (as opposed to directory) is used only for that.

}

cmd.add("--template");
Expand All @@ -192,9 +201,6 @@ Executor getHistoryLogExecutor(File file, String sinceRevision)
/* JDBC requires complete list of files. */
cmd.add(env.storeHistoryCacheInDB() ? FILE_TEMPLATE_LIST : FILE_TEMPLATE);
}
if (!filename.isEmpty()) {
cmd.add(filename);
}

return new Executor(cmd, new File(directoryName), sinceRevision != null);
}
Expand Down Expand Up @@ -264,7 +270,7 @@ private InputStream getHistoryRev(String fullpath, String rev) {
}

/**
* Get the name of file in given revision
* Get the name of file in given revision.
*
* @param fullpath file path
* @param full_rev_to_find revision number (in the form of
Expand All @@ -289,20 +295,20 @@ private String findOriginalName(String fullpath, String full_rev_to_find)
/*
* Get the list of file renames for given file to the specified
* revision. We need to get them from the newest to the oldest
* (hence the reverse()) so that we can follow the renames down
* to the revision we are after.
* so that we can follow the renames down to the revision we are after.
*/
argv.add(ensureCommand(CMD_PROPERTY_KEY, CMD_FALLBACK));
argv.add("log");
argv.add("-f");
argv.add("--follow");
/*
* hg log -f -r behavior has changed since Mercurial 3.4 so filtering
* the changesets of a file no longer works with -f.
* hg log --follow -r behavior has changed since Mercurial 3.4
* so filtering the changesets of a file no longer works with --follow.
* This is tracked by https://bz.mercurial-scm.org/show_bug.cgi?id=4959
* Once this is fixed and Mercurial versions with the fix are prevalent,
* we can revert to the old behavior.
*/
// argv.add("-r");
// Use reverse() to get the changesets from newest to oldest.
// argv.add("reverse(" + rev_to_find + ":)");
argv.add("--template");
argv.add("{rev}:{file_copies}\\n");
Expand Down Expand Up @@ -576,9 +582,17 @@ History getHistory(File file) throws HistoryException {
History getHistory(File file, String sinceRevision)
throws HistoryException {
RuntimeEnvironment env = RuntimeEnvironment.getInstance();
// Note that the filtering of revisions based on sinceRevision is done
// in the history log executor by passing appropriate options to
// the 'hg' executable.
// This is done only for directories since if getHistory() is used
// for file, the file is renamed and its complete history is fetched
// so no sinceRevision filter is needed.
// See findOriginalName() code for more details.
History result = new MercurialHistoryParser(this).parse(file,
sinceRevision);
// Assign tags to changesets they represent

// Assign tags to changesets they represent.
// We don't need to check if this repository supports tags,
// because we know it :-)
if (env.isTagsEnabled()) {
Expand Down
Loading