| Message ID | f36712f5-28bd-42d7-3ea1-f4afa328be07@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 92d92345ce5996933f5cfc357dce1e1744487b6a |
| Headers | show |
| Series | ds/sparse-checkout-requires-per-worktree-config (was Re: What's cooking in git.git (Feb 2022, #02; Wed, 9)) | expand |
Derrick Stolee <stolee@gmail.com> writes: > On 2/9/2022 7:12 PM, Junio C Hamano wrote: > >> * ds/sparse-checkout-requires-per-worktree-config (2022-02-08) 6 commits >> ... >> Will merge to 'next'? >> cf. <20220204081336.3194538-1-newren@gmail.com> >> cf. <CAPig+cRrRxuTeByhKkLs_KDaWY8-r4+jrwT83A-r+sBQsmebMw@mail.gmail.com> >> source: <pull.1101.v6.git.1644269583.gitgitgadget@gmail.com> > > You and I have had a good discussion about the latest version. I > think we've mostly landed on finding ways to improve documentation > in other ways (including the patch you submitted), but here are > the things that I see as still outstanding: Thanks for a clearly written summary. Very much appreciated. With the maintainer hat on, I agree that all are good points. From a reviewer's point of view, I do not care too deeply about 1 or 2 myself though. > 0. Eric mentioned earlier that he was interested in looking again, > but has not signaled that his review is complete. > > 1. You and Eric disagree about the use of "worktree" and "working > tree" in the documentation. I could revert the change to these > docs from v5 to v6. > > 2. You mention that the changes in config.c could be split into > two (first, create repo_config_set_multivar_gently() and then > create repo_config_set_worktree_gently()). > > 3. Jean-Noël noticed an improvement to reduce work on translators. > The diff below could be squashed into patch 5 OR I could submit > it as a forward fix. > > diff --git a/builtin/worktree.c b/builtin/worktree.c > index c6eb636329a..7c272078dc9 100644 > --- a/builtin/worktree.c > +++ b/builtin/worktree.c > @@ -384,11 +384,13 @@ static int add_worktree(const char *path, const char *refname, > 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); > + error(_("failed to unset '%s' in '%s'"), > + "core.bare", to_file); > 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); > + error(_("failed to unset '%s' in '%s'"), > + "core.worktree", to_file); > > git_configset_clear(&cs); > } > > Thanks, > -Stolee
On Fri, Feb 11, 2022 at 11:51 AM Junio C Hamano <gitster@pobox.com> wrote: > Derrick Stolee <stolee@gmail.com> writes: > > On 2/9/2022 7:12 PM, Junio C Hamano wrote: > >> * ds/sparse-checkout-requires-per-worktree-config (2022-02-08) 6 commits > >> Will merge to 'next'? > > > > You and I have had a good discussion about the latest version. I > > think we've mostly landed on finding ways to improve documentation > > in other ways (including the patch you submitted), but here are > > the things that I see as still outstanding: > > With the maintainer hat on, I agree that all are good points. From > a reviewer's point of view, I do not care too deeply about 1 or 2 > myself though. Agreed. > > 0. Eric mentioned earlier that he was interested in looking again, > > but has not signaled that his review is complete. I have now read through the most recent version and left one or two minor comments, but probably nothing actionable. > > 1. You and Eric disagree about the use of "worktree" and "working > > tree" in the documentation. I could revert the change to these > > docs from v5 to v6. I wouldn't go so far as to characterize it as a disagreement. In my review, I only pointed out that this series was introducing some new instances of inconsistency which some earlier efforts (started by Michael Haggerty) had eliminated. As mentioned in [2], I personally prefer the term "worktree". [1]: https://lore.kernel.org/git/CAPig+cS-3CxxyPGcy_vkeN_WYTRo1b-ZhJNdPy8ARZSNKkF1Xg@mail.gmail.com/ [2]: https://lore.kernel.org/git/CAPig+cSY7E_XQC1gHzDJxoDGGmgWDmNz9Ys=CwbhLkCt+DQf-A@mail.gmail.com/ > > 2. You mention that the changes in config.c could be split into > > two (first, create repo_config_set_multivar_gently() and then > > create repo_config_set_worktree_gently()). > > > > 3. Jean-Noël noticed an improvement to reduce work on translators. > > The diff below could be squashed into patch 5 OR I could submit > > it as a forward fix.
Eric Sunshine <sunshine@sunshineco.com> writes: > I wouldn't go so far as to characterize it as a disagreement. In my > review, I only pointed out that this series was introducing some new > instances of inconsistency which some earlier efforts (started by > Michael Haggerty) had eliminated. As mentioned in [2], I personally > prefer the term "worktree". These two words mean different things, so I am not sure there is much room for personal preference. If what a documentation refers to is the working tree plus its administrative files, referring to the whole as a "worktree" would be more precise and concise. If the discussion is only about the thing above .git/ that the editors and the compilers see, "working tree" is the right term to use.
diff --git a/builtin/worktree.c b/builtin/worktree.c index c6eb636329a..7c272078dc9 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -384,11 +384,13 @@ static int add_worktree(const char *path, const char *refname, 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); + error(_("failed to unset '%s' in '%s'"), + "core.bare", to_file); 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); + error(_("failed to unset '%s' in '%s'"), + "core.worktree", to_file); git_configset_clear(&cs); }