Allow git cp to create destination folder automatically#1091
Allow git cp to create destination folder automatically#1091spacewander merged 4 commits intotj:mainfrom
git cp to create destination folder automatically#1091Conversation
hyperupcall
left a comment
hyperupcall
left a comment
There was a problem hiding this comment.
It looks like there is a race and an extra subshell here. What about something like this?:
DESTINATION_FILENAME=${DESTINATION_FILENAME%/}
if ! mkdir -p "${DESTINATION_FILENAME%/*}"; then
echo 1>&2 "Failed to create destination directory: $DESTINATION_DIR"
exit 80
fi
Thanks! Removed the subprocess command. PTAL. (BTW my knowledge with shell script is very limited, so open to further suggestions) |
| exit 80 | ||
| fi | ||
| DESTINATION_DIR="${DESTINATION_FILENAME%/*}" | ||
| if [ ! -d "$DESTINATION_DIR" ]; then |
There was a problem hiding this comment.
We can use mkdir -p directly as the cmd won't return error if the dir exists?
There was a problem hiding this comment.
Make sense. Removed this check.
| echo 1>&2 "$DESTINATION_FILENAME is not a file path." | ||
| exit 80 | ||
| fi | ||
| DESTINATION_DIR="${DESTINATION_FILENAME%/*}" |
There was a problem hiding this comment.
What if the DESTINATION_FILENAME doesn't contain the directory, like ./foo?
There was a problem hiding this comment.
Good question! Although the code still works for ./foo (because DESTINATION_DIR will simply be ./ and creating it is a harmless no-op), another case that I tested did lead to incorrect behavior which is when the DESTINATION_FILENAME is just a filename. The previous revision would create a folder of that name, and make a copy of the source file into that folder (rather than copy to a file of that name).
I've now switched the condition to be checking whether DESTINATION_FILENAME contains any slash characters - if not the folder creation is skipped.
… path contains slash(s)
|
@hyperupcall |
|
The new revision looks good to me - fixed that big. My only comment is that it seems |
|
@spacewander @hyperupcall Thanks for the review! Would either of you be able to click the merge button for me? I don't have write access to this repo. |
|
@weiw005 @spacewander will be able to do it |
|
Merged. Thanks! |
3 participants
fixed #1089