This is a feature-branch pull-request from feature/rrangers to master
#18
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…set (#11) ## Summary: We don't want `ka-clone` to overwrite or duplicate any **local** values the user may have already set for `commit.template` and `include.path`, when running with `--repair`. So check if they are already set, and skip if they are! NOTE: There is a quirk with `include.path`. This config option can have multiple values! You can _add_ new values with the command `git config --local --add include.path <path>`. ``` global file:/Users/lilli/.gitconfig /Users/lilli/.gitconfig.khan local file:.git/config /Users/lilli/.gitconfig.khan-xtra local file:.git/config /Users/lilli/.gitconfig.khan ``` BUT (!!) if you do this twice with the same path-string, it's super annoying to change or unset. If each path-string is in there only once (locally or globally), you can unset that specific path with the command `git config --unset [--local|--global] include.path <EXACT-STRING-TO-SINGLE-VALUE>`. If you have a duplicate, you get this: ``` % git config --unset --local include.path /Users/lilli/.gitconfig.khan warning: include.path has multiple values ``` This doesn't cross local/global boundary when that flag is explicitly used. This behavior means that, for `include.path`, we can _append_ new things, but we have to be careful not to duplicate stuff. Since the behavior was different between the `commit.template` value, and the `include path` value, I split the function `_gitconfig_local_reference` into two functions. We also want to be safe when we do set these for our submodules, so I had to move the submodule loop to the outside of the check.   Issue: FEI-5987 ## Test plan: I set up a repo with a few submodules... Commit template tests - Test nothing set locally for `commit.template` → **this should set the value** - Test `~/.git_template/commit_template` is set locally → should skip [tilde path] - Test `/Users/lilli/.git_template/commit_template` is set locally → should skip [expanded path] - Test `./.git_template/commit_template` is set locally → should skip [same but local ref] - Test `<some other path>` is set locally → should skip [other value] - All the above use-cases should work the same even if we have something globally set for `commit.template` - Test various tests above for submodules too - Test when dotfile isn't installed in home dir Git config tests - Test nothing set locally for `include.path` → **this should append the value** - Test `~/.gitconfig.khan` is set locally → should skip it, because git config resolves this to the expanded value [tilde path] - Test `/Users/lilli/.gitconfig.khan` is set locally → it should skip this [expanded path] - Test `./.gitconfig.khan` is set locally → should still append it, since it's different than expanded value [same but local ref] - Test `<some other path>` is set locally → should still append it [other value] - All the above use-cases should work the same even if we have something globally set for `include.path` - Test various tests above for submodules too - Test when dotfile isn't installed in home dir Author: lillialexis Reviewers: lillialexis, csilvers, jeresig Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #11
## Summary: If you rerun `ka-clone --repair` in any repo (regardless if you select custom hooks to get installed), the tool will still copy in the ka global hooks. Backup the folder so a developer's personal hooks don't get overwritten, nor any infinite loop created. Why are we doing it this way? Why not just backup each hook individually? Why not just _copy_ the `hooks` directory? The answers to these questions lie in the way our KA global hooks function. Both the ka `commit-msg` and `pre-push` hooks search the `hooks` folder for any other hooks with that start with their name and then runs them. E.g., `pre-push` runs all hooks found in the `hooks` folder that start with `pre-push`, such as `pre-push.lint`. This enables us to have ka hooks, but still allows the developer to add their own hooks. It's possible for a user to have previously disabled a global ka hook by renaming the file to `pre-push.disabled` or `commit-msg.disabled`. Or they may just have other hooks in there that they don't want to run, that start with `<global-hook-name>.*`. If we **copy** the folder, then reinstall the global hooks, those other hooks will get called. If it was the original ka `pre-push` or `commit-msg` that was disabled, the developer will get into infinite loop. (Ask me how I know...) They will have `pre-push` call `pre-push.disabled` calling `pre-push.disabled` .... etc. That's why I moved the folder. Backing up individual hooks in the same folder would just get messy and potentially cause the same issue if not renamed correctly. Issue: FEI-5987 ## Test plan: Run `ka-clone --repair`, see the back up folder and new folder. Also try just `ka-clone`ing a new repo and observe the same Author: lillialexis Reviewers: csilvers, lillialexis, jeresig Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #12
## Summary: I wanted to indent the sub-messages so that they catch the eye better. See screenshot  It's not perfect, since interpolated substrings could, in theory, still cause wrapping, but it's not important enough to try and find a method that successfully breaks along string at the word boundaries. Issue: FEI-5987 ## Test plan: Run `ka-clone` in window that is 80 wide and see it look neat Author: lillialexis Reviewers: csilvers, jeresig Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #13
) ## Summary: I added info about the hooks steps in the gitfaq doc I'm concurrently writing, so adding links to it. (Note: Until I can deploy my webapp change that fixes the anchors on the redirect, they aren't currently working.) Issue: FEI-5987 ## Test plan: - Run ka-clone, see the link Author: lillialexis Reviewers: csilvers Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #14
## Summary: I also added some "opposite" args in arg groups (e.g., `--protect-master` and `--no-protect-master`), which users can't use at the same time. So, the defaults are now (currently): ``` Option | Is Default? ---------------------+----------- use ka email | yes protect master hooks | yes branch name hook | yes lint commit msg hook | no pre-push lint hook | yes git config linked | yes commit template | yes ``` @csilvers are these defaults good? Do you think I should toggle any of them? This is what I landed on after I started the implementation. Should I rename these args to be a little more consistent with each other and their function? I don't love how they currently are (e.g., `--no-msg` meaning "don't use the ka commit template" and `--no-lint` referring to the pre-push linter but not the commit-msg linter), but don't know who else may have hard-coded them as they currently are. (Or if it's worth doing.) Issue: FEI-6001 ## Test plan: - Run `ka-clone` with no args, sees that the defaults are what we are saying they are - Run with conflicting arguments (e.g., `--protect-master` and `--no-protect-master`), and see that it errors - Try running with all arguments that are opposite of the deep faults and see that they do what we expect them to do Author: lillialexis Reviewers: csilvers, lillialexis Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #15
…her (#17) ## Summary: The previous args were short, and that was nice, but they didn't really map to what each arg was doing inside the script, and this muddiness was adding cognitive load. Now the names are longer, but consistent with each other and clearer for the reader. Issue: FEI-6060 ## Test plan: Run ka-clone [--repair] with all args and make sure they work Author: lillialexis Reviewers: csilvers Required Reviewers: Approved By: csilvers Checks: Pull Request URL: #17
csilvers
approved these changes
Dec 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This PR includes the following commits:
commit.templateorinclude.pathif it is already set (Don't overwritecommit.templateorinclude.pathif it is already set #11)All PRs have already been reviewed.
Issue: FEI-XXXX
Test plan: