| Message ID | pull.1101.v6.git.1644269583.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Sparse checkout: fix bug with worktree of bare repo | expand |
On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > This series is now based on v2.35.0 since that contains all of the necessary > topics. > > This patch series includes a fix to the bug reported by Sean Allred [1] and > diagnosed by Eric Sunshine [2]. > > The root cause is that 'git sparse-checkout init' writes to the worktree > config without checking that core.bare or core.worktree are set in the > common config file. This series fixes this, but also puts in place some > helpers to prevent this from happening in the future. > > ATTENTION: I have significantly redesigned the series since previous > versions, so most of this cover letter is new. > > * Patch 1 updates documentation around extensions.worktreeConfig in a few > places to improve discoverability. Several cross links are added to make > it easy to find the related areas. (The documentation for the changes to > 'git sparse-checkout' are delayed to patch 4.) > > * Patch 2 introduces the init_worktree_config() helper which follows the > documented instructions to enable extensions.worktreeConfig as well as > move the core.bare and core.worktree config values. This update does not > modify core.repositoryFormatVersion, since this is not needed > specifically for extensions.worktreeConfig. > > * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we > can internally adjust a config value within a worktree, at least if > extensions.worktreeConfig is enabled. (It will write to the common config > file if the extension is not enabled.) > > * Patch 4 modifies the sparse-checkout builtin to use > init_worktree_config() and repo_config_set_worktree_gently() in ways that > fix the reported bug. The behavior change here is that it will no longer > upgrade the repository format version, since that is not needed for > extensions.worktreeConfig. > > * Patch 5 updates 'git worktree add' to copy the worktree config from the > current worktree to the new one (while unsetting core.bare=true and > core.worktree=*) along with copying the sparse-checkout patterns file. > > [1] > https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/ > [2] > https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/ > > > Updates in v6 > ============= > > * Updated documentation to use "working tree" over "worktree" and "" over > "" Not sure what "" over "" means. > * Delay some allocations to avoid leaking memory in error conditions. > * Use "main worktree" over "base worktree" in comments. > * Was the empty bullet point meant to cover the new patch 6? Anyway, comments on the cover letter aside, the patches themselves are: Reviewed-by: Elijah Newren <newren@gmail.com>
On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote: > On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > Updates in v6 > > * Updated documentation to use "working tree" over "worktree" and "" over > > "" > > Not sure what "" over "" means. Probably related to my review comment[1] about spelling it "$GIT_DIR/worktrees/<id>/" rather than "$GIT_DIR/worktrees/<worktree-name>/". > > * Delay some allocations to avoid leaking memory in error conditions. > > * Use "main worktree" over "base worktree" in comments. > > * > > Was the empty bullet point meant to cover the new patch 6? The "Updates in v6" section was botched a bit. If you look closely, the remaining bullet points actually ended up in the "Updates in v5" section. The complete "Updates in v6" section should have been (approximately): * Updated documentation to use "working tree" over "worktree" and "<id>" over "<worktree-name>" * Delay some allocations to avoid leaking memory in error conditions. * Use "main worktree" over "base worktree" in comments. * Removed use of git_configset_get_string_tmp() and added a patch that removes its public declaration. * Fragile variables are replaced with better ones. * Variable names and code style improved. * Several test cleanups in patch 5. [1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24
On Mon, Feb 7, 2022 at 9:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote: > > On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget > > <gitgitgadget@gmail.com> wrote: > > > Updates in v6 > > > * Updated documentation to use "working tree" over "worktree" and "" over > > > "" > > > > Not sure what "" over "" means. > > Probably related to my review comment[1] about spelling it > "$GIT_DIR/worktrees/<id>/" rather than > "$GIT_DIR/worktrees/<worktree-name>/". > > > > * Delay some allocations to avoid leaking memory in error conditions. > > > * Use "main worktree" over "base worktree" in comments. > > > * > > > > Was the empty bullet point meant to cover the new patch 6? > > The "Updates in v6" section was botched a bit. If you look closely, > the remaining bullet points actually ended up in the "Updates in v5" > section. The complete "Updates in v6" section should have been > (approximately): > > * Updated documentation to use "working tree" over "worktree" and > "<id>" over "<worktree-name>" > * Delay some allocations to avoid leaking memory in error conditions. > * Use "main worktree" over "base worktree" in comments. > * Removed use of git_configset_get_string_tmp() and added a patch that > removes its public declaration. > * Fragile variables are replaced with better ones. > * Variable names and code style improved. > * Several test cleanups in patch 5. > > [1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24 So, you clearly also read the patches in this round. Do they also look good to you? :-)
On Tue, Feb 8, 2022 at 12:18 AM Elijah Newren <newren@gmail.com> wrote: > On Mon, Feb 7, 2022 at 9:03 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote: > > > Was the empty bullet point meant to cover the new patch 6? > > > > The "Updates in v6" section was botched a bit. If you look closely, > > the remaining bullet points actually ended up in the "Updates in v5" > > section. The complete "Updates in v6" section should have been > > (approximately): > > > > * Updated documentation to use "working tree" over "worktree" and > > "<id>" over "<worktree-name>" > > * Delay some allocations to avoid leaking memory in error conditions. > > * Use "main worktree" over "base worktree" in comments. > > * Removed use of git_configset_get_string_tmp() and added a patch that > > removes its public declaration. > > * Fragile variables are replaced with better ones. > > * Variable names and code style improved. > > * Several test cleanups in patch 5. > > So, you clearly also read the patches in this round. Do they also > look good to you? :-) I have not yet looked at either the patches or the range-diff, and I only scanned my eye quickly over the cover letter, but the empty bullet point made me stop and look a bit more carefully at that part (and only that part) of the cover letter. Not sure yet when I'll have time to carefully read this round.
On 2/8/2022 12:02 AM, Eric Sunshine wrote: > On Mon, Feb 7, 2022 at 11:14 PM Elijah Newren <newren@gmail.com> wrote: >> On Mon, Feb 7, 2022 at 1:33 PM Derrick Stolee via GitGitGadget >> <gitgitgadget@gmail.com> wrote: >>> Updates in v6 >>> * Updated documentation to use "working tree" over "worktree" and "" over >>> "" >> >> Not sure what "" over "" means. > > Probably related to my review comment[1] about spelling it > "$GIT_DIR/worktrees/<id>/" rather than > "$GIT_DIR/worktrees/<worktree-name>/". > >>> * Delay some allocations to avoid leaking memory in error conditions. >>> * Use "main worktree" over "base worktree" in comments. >>> * >> >> Was the empty bullet point meant to cover the new patch 6? > > The "Updates in v6" section was botched a bit. If you look closely, > the remaining bullet points actually ended up in the "Updates in v5" > section. The complete "Updates in v6" section should have been > (approximately): Whoops on mixing them up. Sorry about that. > * Updated documentation to use "working tree" over "worktree" and > "<id>" over "<worktree-name>" this is the issue for the empty quotes. GGG thought these were HTML tags, so made them disappear. I should have used `<id>` and `<worktree-name>`. > * Delay some allocations to avoid leaking memory in error conditions. > * Use "main worktree" over "base worktree" in comments. > * Removed use of git_configset_get_string_tmp() and added a patch that > removes its public declaration. > * Fragile variables are replaced with better ones. > * Variable names and code style improved. > * Several test cleanups in patch 5. > > [1]: https://lore.kernel.org/git/pull.1101.v4.git.1643136134.gitgitgadget@gmail.com/T/#m4926177771017bbea82c33e9e03e6a9a004ebf24 Thanks for cleaning up my cover letter! -Stolee
On Tue, Feb 8, 2022 at 12:42 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > On Tue, Feb 8, 2022 at 12:18 AM Elijah Newren <newren@gmail.com> wrote: > > So, you clearly also read the patches in this round. Do they also > > look good to you? :-) > > I have not yet looked at either the patches or the range-diff, and I > only scanned my eye quickly over the cover letter, but the empty > bullet point made me stop and look a bit more carefully at that part > (and only that part) of the cover letter. Not sure yet when I'll have > time to carefully read this round. I've now read through the series and, I think, left only one or two minor not-actionable comments.
This series is now based on v2.35.0 since that contains all of the necessary topics. This patch series includes a fix to the bug reported by Sean Allred [1] and diagnosed by Eric Sunshine [2]. The root cause is that 'git sparse-checkout init' writes to the worktree config without checking that core.bare or core.worktree are set in the common config file. This series fixes this, but also puts in place some helpers to prevent this from happening in the future. ATTENTION: I have significantly redesigned the series since previous versions, so most of this cover letter is new. * Patch 1 updates documentation around extensions.worktreeConfig in a few places to improve discoverability. Several cross links are added to make it easy to find the related areas. (The documentation for the changes to 'git sparse-checkout' are delayed to patch 4.) * Patch 2 introduces the init_worktree_config() helper which follows the documented instructions to enable extensions.worktreeConfig as well as move the core.bare and core.worktree config values. This update does not modify core.repositoryFormatVersion, since this is not needed specifically for extensions.worktreeConfig. * Patch 3 adds a new repo_config_set_worktree_gently() helper method so we can internally adjust a config value within a worktree, at least if extensions.worktreeConfig is enabled. (It will write to the common config file if the extension is not enabled.) * Patch 4 modifies the sparse-checkout builtin to use init_worktree_config() and repo_config_set_worktree_gently() in ways that fix the reported bug. The behavior change here is that it will no longer upgrade the repository format version, since that is not needed for extensions.worktreeConfig. * Patch 5 updates 'git worktree add' to copy the worktree config from the current worktree to the new one (while unsetting core.bare=true and core.worktree=*) along with copying the sparse-checkout patterns file. [1] https://lore.kernel.org/git/CABceR4bZmtC4rCwgxZ1BBYZP69VOUca1f_moJoP989vTUZWu9Q@mail.gmail.com/ [2] https://lore.kernel.org/git/CAPig+cQ6U_yFw-X2OWrizB1rbCvc4bNxuSzKFzmoLNnm0GH8Eg@mail.gmail.com/ Updates in v6 ============= * Updated documentation to use "working tree" over "worktree" and "" over "" * Delay some allocations to avoid leaking memory in error conditions. * Use "main worktree" over "base worktree" in comments. * Updates in v5 ============= Responded to most of Eric's suggestions. I pushed back on one about a comment including information from the commit message, but everything else should be as Eric suggested. * Cleaned up documentation as per Elijah's suggestions. * Removed unnecessary conflicting change in git-sparse-checkout.txt * Fixed an ambiguous comment about moving config values. * Removed use of git_configset_get_string_tmp() and added a patch that removes its public declaration. * Fragile variables are replaced with better ones. * Variable names and code style improved. * Several test cleanups in patch 5. Updates in v4 ============= * Rebased to v2.35.0 * Fixed memory leak (was leaking repo_git_path() result) * Added additional documentation updates so curious users can discover the intricacies of extensions.worktreeConfig from multiple entry points. * Significantly reduced the amount of changes to config.c. * 'git sparse-checkout' no longer upgrades the repository format. * Dropped the update to upgrade_repository_format(), since it is not needed. * Dropped the 'git worktree init-worktree-config' subcommand in favor of a helper method called by 'git sparse-checkout' * Many others because of the significant changes required by the above items. Thanks, -Stolee Derrick Stolee (6): Documentation: add extensions.worktreeConfig details worktree: create init_worktree_config() config: add repo_config_set_worktree_gently() sparse-checkout: set worktree-config correctly worktree: copy sparse-checkout patterns and config on add config: make git_configset_get_string_tmp() private Documentation/config/extensions.txt | 31 ++++++++++++ Documentation/git-config.txt | 8 ++- Documentation/git-sparse-checkout.txt | 16 ++++-- Documentation/git-worktree.txt | 11 ++-- builtin/sparse-checkout.c | 28 +++++----- builtin/worktree.c | 63 +++++++++++++++++++++++ config.c | 39 ++++++++++++-- config.h | 9 +++- sparse-index.c | 10 ++-- t/t1091-sparse-checkout-builtin.sh | 35 +++++++++---- t/t2400-worktree-add.sh | 58 ++++++++++++++++++++- worktree.c | 73 +++++++++++++++++++++++++++ worktree.h | 21 ++++++++ 13 files changed, 353 insertions(+), 49 deletions(-) base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1101%2Fderrickstolee%2Fsparse-checkout%2Fbare-worktree-bug-v6 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1101/derrickstolee/sparse-checkout/bare-worktree-bug-v6 Pull-Request: https://github.com/gitgitgadget/git/pull/1101 Range-diff vs v5: 1: 1bd5f26271c ! 1: 0260ff6cac0 Documentation: add extensions.worktreeConfig details @@ Documentation/config/extensions.txt: extensions.objectFormat:: + If enabled, then worktrees will load config settings from the + `$GIT_DIR/config.worktree` file in addition to the + `$GIT_COMMON_DIR/config` file. Note that `$GIT_COMMON_DIR` and -+ `$GIT_DIR` are the same for the main worktree, while other -+ worktrees have `$GIT_DIR` equal to -+ `$GIT_COMMON_DIR/worktrees/<worktree-name>/`. The settings in the ++ `$GIT_DIR` are the same for the main working tree, while other ++ working trees have `$GIT_DIR` equal to ++ `$GIT_COMMON_DIR/worktrees/<id>/`. The settings in the + `config.worktree` file will override settings from any other + config files. ++ +When enabling `extensions.worktreeConfig`, you must be careful to move -+certain values from the common config file to the main worktree's ++certain values from the common config file to the main working tree's +`config.worktree` file, if present: ++ +* `core.worktree` must be moved from `$GIT_COMMON_DIR/config` to @@ Documentation/git-config.txt: from all available files. read from or written to if `extensions.worktreeConfig` is - present. If not it's the same as `--local`. + enabled. If not it's the same as `--local`. Note that `$GIT_DIR` -+ is equal to `$GIT_COMMON_DIR` for the main worktree, but is of the -+ form `$GIT_DIR/worktrees/<worktree-name>/` for other worktrees. See ++ is equal to `$GIT_COMMON_DIR` for the main working tree, but is of ++ the form `$GIT_DIR/worktrees/<id>/` for other working trees. See + linkgit:git-worktree[1] to learn how to enable + `extensions.worktreeConfig`. 2: 2a2c350112e ! 2: 5d0cc242d92 worktree: create init_worktree_config() @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha + int bare = 0; + struct config_set cs = { { 0 } }; + const char *core_worktree; -+ char *common_config_file = xstrfmt("%s/config", r->commondir); -+ char *main_worktree_file = xstrfmt("%s/config.worktree", r->commondir); ++ char *common_config_file; ++ char *main_worktree_file; + + /* + * If the extension is already enabled, then we can skip the @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha + if ((res = git_config_set_gently("extensions.worktreeConfig", "true"))) + return error(_("failed to set extensions.worktreeConfig setting")); + ++ common_config_file = xstrfmt("%s/config", r->commondir); ++ main_worktree_file = xstrfmt("%s/config.worktree", r->commondir); ++ + git_configset_init(&cs); + git_configset_add_file(&cs, common_config_file); + + /* + * If core.bare is true in the common config file, then we need to -+ * move it to the base worktree's config file or it will break all ++ * move it to the main worktree's config file or it will break all + * worktrees. If it is false, then leave it in place because it + * _could_ be negating a global core.bare=true. + */ @@ worktree.c: int should_prune_worktree(const char *id, struct strbuf *reason, cha + goto cleanup; + } + /* -+ * If core.worktree is set, then the base worktree is located ++ * If core.worktree is set, then the main worktree is located + * somewhere different than the parent of the common Git dir. + * Relocate that value to avoid breaking all worktrees with this + * upgrade to worktree config. + */ -+ if (!git_configset_get_string_tmp(&cs, "core.worktree", &core_worktree)) { ++ if (!git_configset_get_value(&cs, "core.worktree", &core_worktree)) { + if ((res = move_config_setting("core.worktree", core_worktree, + common_config_file, + main_worktree_file))) 3: 802b28a9510 = 3: cf9e86fe3a4 config: add repo_config_set_worktree_gently() 4: 08b89d17ccf ! 4: 5b5924eab49 sparse-checkout: set worktree-config correctly @@ Metadata ## Commit message ## sparse-checkout: set worktree-config correctly - The previous change added repo_config_set_worktree_gently() to assist - writing config values into the config.worktree file, if enabled. An - earlier change added init_worktree_config() as a helper to initialize - extensions.worktreeConfig if not already enabled. + `git sparse-checkout set/init` enables worktree-specific + configuration[*] by setting extensions.worktreeConfig=true, but neglects + to perform the additional necessary bookkeeping of relocating + `core.bare=true` and `core.worktree` from $GIT_COMMON_DIR/config to + $GIT_COMMON_DIR/config.worktree, as documented in git-worktree.txt. As a + result of this oversight, these settings, which are nonsensical for + secondary worktrees, can cause Git commands to incorrectly consider a + worktree bare (in the case of `core.bare`) or operate on the wrong + worktree (in the case of `core.worktree`). Fix this problem by taking + advantage of the recently-added init_worktree_config() which enables + `extensions.worktreeConfig` and takes care of necessary bookkeeping. - Let the sparse-checkout builtin use these helpers instead of attempting to - initialize the worktree config on its own. This changes behavior of 'git - sparse-checkout set' in a few important ways: + While at it, for backward-compatibility reasons, also stop upgrading the + repository format to "1" since doing so is (unintentionally) not + required to take advantage of `extensions.worktreeConfig`, as explained + by 11664196ac ("Revert "check_repository_format_gently(): refuse + extensions for old repositories"", 2020-07-15). - 1. Git will no longer upgrade the repository format, since this is not - a requirement for understanding extensions.worktreeConfig. - - 2. If the main worktree is bare, then this command will not put the - worktree in a broken state. - - The main reason to use worktree-specific config for the sparse-checkout - builtin was to avoid enabling sparse-checkout patterns in one and - causing a loss of files in another. If a worktree does not have a - sparse-checkout patterns file, then the sparse-checkout logic will not - kick in on that worktree. + [*] The main reason to use worktree-specific config for the + sparse-checkout builtin was to avoid enabling sparse-checkout patterns + in one and causing a loss of files in another. If a worktree does not + have a sparse-checkout patterns file, then the sparse-checkout logic + will not kick in on that worktree. Reported-by: Sean Allred <allred.sean@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> 5: 85779dfaed3 ! 5: c51cb3714e7 worktree: copy sparse-checkout patterns and config on add @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam + */ + if (core_apply_sparse_checkout) { + char *from_file = git_pathdup("info/sparse-checkout"); -+ char *to_file = xstrfmt("%s/worktrees/%s/info/sparse-checkout", -+ realpath.buf, name); ++ char *to_file = xstrfmt("%s/info/sparse-checkout", ++ sb_repo.buf); + + if (file_exists(from_file)) { + if (safe_create_leading_directories(to_file) || @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam + */ + if (repository_format_worktree_config) { + char *from_file = git_pathdup("config.worktree"); -+ char *to_file = xstrfmt("%s/worktrees/%s/config.worktree", -+ realpath.buf, name); ++ char *to_file = xstrfmt("%s/config.worktree", ++ sb_repo.buf); + + if (file_exists(from_file)) { -+ struct config_set cs = { { 0 }}; -+ const char *str_value; -+ int bool_value; ++ struct config_set cs = { { 0 } }; ++ const char *core_worktree; ++ int bare; + + if (safe_create_leading_directories(to_file) || -+ copy_file(to_file, from_file, 0666)) -+ die(_("failed to copy worktree config from '%s' to '%s'"), -+ from_file, to_file); ++ copy_file(to_file, from_file, 0666)) { ++ error(_("failed to copy worktree config from '%s' to '%s'"), ++ from_file, to_file); ++ goto worktree_copy_cleanup; ++ } + + git_configset_init(&cs); + git_configset_add_file(&cs, from_file); + -+ if (!git_configset_get_bool(&cs, "core.bare", &bool_value) && -+ bool_value && ++ if (!git_configset_get_bool(&cs, "core.bare", &bare) && ++ bare && + git_config_set_multivar_in_file_gently( + to_file, "core.bare", NULL, "true", 0)) + error(_("failed to unset 'core.bare' in '%s'"), to_file); -+ if (!git_configset_get_value(&cs, "core.worktree", &str_value) && ++ if (!git_configset_get_value(&cs, "core.worktree", &core_worktree) && + git_config_set_in_file_gently(to_file, + "core.worktree", NULL)) + error(_("failed to unset 'core.worktree' in '%s'"), to_file); @@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam + git_configset_clear(&cs); + } + ++worktree_copy_cleanup: + free(from_file); + free(to_file); + } @@ t/t1091-sparse-checkout-builtin.sh: test_expect_success 'add to sparse-checkout' + git -C repo sparse-checkout set --no-cone "/*" && + git -C repo worktree add --quiet ../worktree 2>err && + test_must_be_empty err && -+ new=repo/.git/worktrees/worktree/info/sparse-checkout && -+ test_path_is_file $new && -+ test_cmp repo/.git/info/sparse-checkout $new && ++ new="$(git -C worktree rev-parse --git-path info/sparse-checkout)" && ++ test_path_is_file "$new" && ++ test_cmp repo/.git/info/sparse-checkout "$new" && + git -C worktree sparse-checkout set --cone && + test_cmp_config -C worktree true core.sparseCheckoutCone && + test_must_fail git -C repo core.sparseCheckoutCone @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep - ) + git worktree add ../there3 main && + cd ../there3 && ++ # Simple check that a Git command does not ++ # immediately fail with the current setup + git status + ) && + cat >expect <<-EOF && @@ t/t2400-worktree-add.sh: test_expect_success '"add" default branch of a bare rep + git clone --bare . bare3 && + cd bare3 && + git config extensions.worktreeconfig true && ++ ++ # Add config values that are erroneous to have in ++ # a config.worktree file outside of the main ++ # working tree, to check that Git filters them out ++ # when copying config during "git worktree add". + git config --worktree core.bare true && + git config --worktree core.worktree "$(pwd)" && ++ ++ # We want to check that bogus.key is copied + git config --worktree bogus.key value && + git config --unset core.bare && + git worktree add ../there4 main && + cd ../there4 && ++ ++ # Simple check that a Git command does not ++ # immediately fail with the current setup + git status && + git worktree add --detach ../there5 && + cd ../there5 && -: ----------- > 6: f687a0bfa16 config: make git_configset_get_string_tmp() private