Support wrapped clone in current directory #1478
Conversation
ttaylorr
force-pushed
the
clone-in-current-directory
branch
from
1d59cce
to
365d615
Compare
| Run: cleanCommand, | ||
| Use: "clean", | ||
| Run: cleanCommand, | ||
| PreRun: resolveLocalStorage, |
There was a problem hiding this comment.
I wonder if we could remove all the duplication of setting PreRun (and potentially forgetting to include it) by standardising on a wrapper function like NewLfsCommand("clean", cleanCommand) or similar?
There was a problem hiding this comment.
I think I am going to leave this as-is, and only give the PreRun option to commands that need it.
There was a problem hiding this comment.
Isn't that all of them except clone? I expect all new commands will need it too, would be easier to remember with a utility function.
There was a problem hiding this comment.
I would rather be explicit about which commands are using it up front. If it ends up being the majority, then perhaps it makes sense to add a helper function, but not now.
There was a problem hiding this comment.
I think there are a few commands that don't need it, like env, but I'd bet that the majority of commands will need it.
|
That's a fascinating edge case 😄 👍 from me |
In commit 365d615 of PR git-lfs#1478 a pair of tests in what is now our t/t-clone.sh test script were enhanced to check that the "git lfs clone" command did not create a directory named "lfs" within the top level ofa cloned repository's working tree, as the "lfs" directory should instead be created within the ".git" directory. This check has since been replicated in a number of other tests in the t/t-clone.sh script, but not all of them, so we add this check in the relevant sections of the tests where it is not yet performed. We also adjust one check which dates from commit 365d615 to use the same -e shell test as all the other checks, rather than the -f test. These changes are not especially important, but in conjunction with the revisions we have made in other commits in this PR, will help synchonize the tests in the t/t-clone.sh script so they are more consistent and complete.
In commit 365d615 of PR git-lfs#1478 a pair of tests in what is now our t/t-clone.sh test script were enhanced to check that the "git lfs clone" command did not create a directory named "lfs" within the top level ofa cloned repository's working tree, as the "lfs" directory should instead be created within the ".git" directory. This check has since been replicated in a number of other tests in the t/t-clone.sh script, but not all of them, so we add this check in the relevant sections of the tests where it is not yet performed. We also adjust one check which dates from commit 365d615 to use the same -e shell test as all the other checks, rather than the -f test. These changes are not especially important, but in conjunction with the revisions we have made in other commits in this PR, will help synchonize the tests in the t/t-clone.sh script so they are more consistent and complete.
This PR supports cloning into the current directory,
".", a-la:$ git lfs clone git@github.com:foo/bar.git .Previously, this was broken because an invocation to the
git-lfs clonecommand would create alocalstoragedirectory due to a call to thelocalstorage.ResolveDirsfunction causing both of these files to be created. The presence of those files violated agit-cloneinvariant that the clone target must be empty. That source of that call is from theinit()function in thelfspackage, here.In e447be2, I removed that call, and instead replaced it with a
PreRunhook on all of our commands, exceptclone.PreRunhooks, as dictated by thecobrapackage will run before the actualRunfunction of the command.In a further PR, we should definitely take a look at which commands actually need to have the
localstoragestuff initialized./cc @technoweenie @rubyist @sinbad for review
/cc @tarka #1431 for a heads-up