[modules-dev] Initial repository management tool

[Stanley M. Ho]

Hi Dave,

Looks like a good start. Several comments inline:

- src/share/classes/sun/module/tools/ToolBase.java

35 public class ToolBase {

Should be package private.

- src/share/classes/sun/module/tools/JRepo.java

Remove empty line at #43 and #46.

   76             usage = "Usage: jrepo <cmd>, where cmd is follows one 
of the patterns below";

Perhaps "command" instead of "cmd".

  68     private static final Map<String, Command> commands = new 
LinkedHashMap<String, Command>();
   75         if (usage == null) {
   76             usage = "Usage: jrepo <cmd>, where cmd is follows one 
of the patterns below";
   77             new ListCommand();
   78             new FindCommand();
   79             new QueryCommand();
   80         }
  167             commands.put(name, this);
  168             String u = usage();
  169             if (u != null) {
  170                 usage += "\n\t" + usage();
  171             }
  330         for (Map.Entry<String, Command> entry : commands.entrySet()) {
  331             if (entry.getKey().startsWith(args[0])) {
  332                 cmd = entry.getValue();
  333                 break;
  334             }
  335         }
  416     void usageError() {
  417         error(usage);
  418     }

I don't like the idea of having the Command constructor to bring side 
effect to "usage" and "commands" in the outer class. I think removing 
these side effects could simply the code and make it more readable, e.g.

private state final List<Command> commands = new ArrayList<Command>();
...
     commands.add(new ListCommand());
     commands.add(new FindCommand());
     commands.add(new QueryCommand());
...
for (Command command : commands) {
    if (command.getName().startsWith(args[0])) {
         cmd = entry.getValue();
         break;
    }
}
...
private void usageError() {
    StringBuilder builder = new StringBuilder();
    builder.append("Usage: jrepo <command>, where command follows one of 
the patterns below\n");
    for (Command command : commands) {
    	builder.append(command.getUsage());
         builder.append("\n");
    }
    error(builder.toString());
}

  165     abstract class Command {
  223     class ListCommand extends Command {
  261     class FindCommand extends Command {
  303     class QueryCommand extends FindCommand {

These classes should be private static if possible.

  103         if (repo == null) {
  104             err.println("can't access repository " + 
repositoryLocation);
  105             return false;
  106         }

Is this check really necessary? I would have assumed the method has 
already returned if there was exception in #95-#100.

I'm surprised we have to configure the metadata directory in the 
repository in #139-#145. I think the URLRepository implementation should 
do this automatically by default if no metadata directory is specified.

  246                 List<ModuleArchiveInfo> installed = repo.list();
  247                 for (ModuleArchiveInfo mai : installed)

Should combine into

	for (ModuleArchiveInfo mai : repo.list())

  308         String usage() { return null; }

Should either not override the method, or return usage string specific 
for "query".

The getRepositoryText method looks like something Repository.toString() 
should provide. If not, at least we could clean it up as:

  private String getRepositoryText(Repository repo) {
  	URL u = repo.getSourceLocation();
         if (u == null) {
             return "Bootstrap repository";
         } else {
	    return "Repository " + u.toExternalForm();
	}
  }

That's it for now.

- Stanley