feat(git-bulk): add new option to no follow symlinks #1194
Conversation
hyperupcall
left a comment
There was a problem hiding this comment.
Thanks! I think the feature is good, had a few comments on the implementation.
bin/git-bulk
Outdated
| find_flags+=(-L) | ||
| fi | ||
| # find all git repositories under the workspace on which we want to operate | ||
| allGitFolders=( $(eval find $find_flags . -name ".git") ) |
There was a problem hiding this comment.
| allGitFolders=( $(eval find $find_flags . -name ".git") ) | |
| readarray allGitFolders <(find "${find_flags[@]}" . -name ".git") |
Possible to do this instead? This won't break on filepaths that include a newline and removes the (unecessary?) eval
There was a problem hiding this comment.
Thank you for the improvement. It is fixed with my new commit 5a7c33d.
A few comments:
- Compared to your proposition, I added a missing
<(the first one is thestdinredirection, the second one is the process substitution). I tested this implementation against paths with spaces, which is working. Note that I assume you meant "filepaths that include a space" and not "filepaths that include a newline", right? - I removed the unnecessary
eval, which was introduced here since the very first commit57c0eb7ofgit-bulk, for a non obvious reason to me.
There was a problem hiding this comment.
I assume you meant "filepaths that include a space" and not "filepaths that include a newline", right?
Yeah that's what I meant haha
| if [[ "$no_follow_symlinks" == true ]]; then | ||
| find_flags+=(-P) | ||
| else | ||
| find_flags+=(-L) |
There was a problem hiding this comment.
This codepath changes the default. According to the manpage:
Since it is the default, the -P option should be considered to be in effect unless either -H or -L is specified.
I don't think changing the default behavior is a good idea. Maybe the flag should be --follow-symlinks to make this clearer?
There was a problem hiding this comment.
Indeed, this codepath change the default of find... but not the default of the git-bulk behavior, where the -L flag was added in commit 3730339 in 2018.
I did this in this way to not change the default git-bulk behavior. But I agree with you that it may have been better to not change find default in the first place.
So, from this, what do you prefer? On my side, I don't have any strong opinion about this. :)
There was a problem hiding this comment.
Oops! Yeah you're right, my mistake. I agree we don't want to change the default git-bulk behavior
1. Use `readarray` such that we can handle paths with spaces 2. Remove the unnecessary `eval`
5a7c33d to
f646ff6
Compare
|
FYI, I rebased on |
|
@hyperupcall |
hyperupcall
left a comment
There was a problem hiding this comment.
Thanks! With the new changes everything looks great
| if [[ "$no_follow_symlinks" == true ]]; then | ||
| find_flags+=(-P) | ||
| else | ||
| find_flags+=(-L) |
There was a problem hiding this comment.
Oops! Yeah you're right, my mistake. I agree we don't want to change the default git-bulk behavior
bin/git-bulk
Outdated
| find_flags+=(-L) | ||
| fi | ||
| # find all git repositories under the workspace on which we want to operate | ||
| allGitFolders=( $(eval find $find_flags . -name ".git") ) |
There was a problem hiding this comment.
I assume you meant "filepaths that include a space" and not "filepaths that include a newline", right?
Yeah that's what I meant haha
|
Merged. Thanks! |
Add a new option
--no-follow-symlinksforgit-bulkto not traverse symlinks under the workspace directories when searching for git repositories. This PR does not change the default behavior, it only add the option for the user.