Feature lighter init #44
Conversation
| import java.util.Date; | ||
| import java.util.List; |
There was a problem hiding this comment.
Can you please remove unneeded imports?
| @@ -12,7 +12,7 @@ private Utils() { | |||
| } | |||
|
|
|||
| public static File createCheckoutDirectory(String path) throws GitException { | |||
|
|
|||
| // FIXME use predefined folder | |||
| @@ -27,6 +27,7 @@ | |||
| private final boolean debug = Utils.getBooleanEnvironmentProperty("wagon.git.debug"); | |||
| private final boolean safeCheckout = Utils.getBooleanEnvironmentProperty("wagon.git.safe.checkout"); | |||
| private final boolean skipEmptyCommit = Utils.getBooleanEnvironmentProperty("wagon.git.skip.empty.commit"); | |||
| private final boolean enableShallowFetch = Utils.getBooleanEnvironmentProperty("wagon.git.enable.shallow.fetch"); | |||
There was a problem hiding this comment.
Can we invert logic here? What about disableShallowFetch thus we would assume it's enabled by default?
|
So here are some fixes as you requested :
(I'll squash the commits once we agree on the code =P) |
| dir = new File(System.getProperty("java.io.tmpdir"), "wagon-git-" + hashPath(path)); | ||
| } else { | ||
| dir = new File(path); |
There was a problem hiding this comment.
I'm not totally happy with this change. Let's forget path for a while, path is just a text transformation of the repo URI. What if the new argument just replaces the new File(java.io.tempdir) if present (not null)?
There was a problem hiding this comment.
I'm not sure of what you mean =/
Remote is not passed to Utils (it is defined inside GitBackend which calls Utils). So without passing more arguments to the function, apart from the current proposition, i don't see how i can compute the temp file (from remote) and override it with the new argument.
Do you mean something like this :
public static File createCheckoutDirectory(String remote, String overridenPath) throws GitException {
File dir;
if (overridenPath != null && !"".equals(overridenPath)) {
dir = new File(overridenPath);
} else {
dir = new File(System.getProperty("java.io.tmpdir"), "wagon-git-" + hashPath(remote));
}
dir.mkdirs();
return dir;
}
There was a problem hiding this comment.
This new feature of providing a directory, interferes somehow, in a "semantic" way, with the "safe checkout" flag (http://synergian.github.io/wagon-git/troubleshooting.html), which wouldn't have any sense any more.
I don't like to have features that interfere one with each other. That's what makes me unhappy :)
I think it's much more elegant to provide a path, like you did. But I would like to remove the old flag so the features don't overlap. And also, as I spent quite some time writing the documentation, I'd like to have it updated to these changes.
I'm not asking you to make those changes, I can. Sadly, I don't have the time right now, so they would have to wait.
There was a problem hiding this comment.
Oh I forgot about this flag, you're totally right.
Don't worry, i will do the changes. Also, I was going to update the documentation as well to reflect all the changes. I want to be sure that we agree on the code before getting to deep into documentation =)
There was a problem hiding this comment.
So, about this flag, should we keep it and enable it only when wagon.git.permanent.root is not set ? (ie. when wagon.git.permanent.root is set, wagon.git.safe.checkout is considered as false).
There was a problem hiding this comment.
I'm sorry. I'm messing things up. I wrote that part of the code like 4 or 5 years ago. Forget that. Although, the name of the property should be wagon.git.cleanup.directory instead of wagon.git.safe.checkout. I probably changed that at some point. I honestly don't remember.
Change that if you want. I'm okay if you don't.
I'll just await for the documentation of the two new flags.
Here is a PR for #43.
It's a work in progress (it needs testing), but i want to show you where I was going as soon as possible.
There is 2 new properties :
temporary) for git clones