| Message ID | pull.1526.git.git.1686771358484.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 69f4da8eadac4213fddbf68d85c992f230287001 |
| Headers | show |
| Series | setup.c: don't setup in discover_git_directory() | expand |
Hi Glen, On Wed, 14 Jun 2023, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> > > discover_git_directory() started modifying the_repository in ebaf3bcf1ae > (repository: move global r_f_p_c to repo struct, 2021-06-17), when, in > the repository setup process, we started copying members from the > "struct repository_format" we're inspecting to the appropriate "struct > repository". However, discover_git_directory() isn't actually used in > the setup process (its only caller in the Git binary is > read_early_config()), so it shouldn't be doing this setup at all! > > As explained by 16ac8b8db6 (setup: introduce the > discover_git_directory() function, 2017-03-13) and the comment on its > declaration, discover_git_directory() is intended to be an entrypoint > into setup.c machinery that allows the Git directory to be discovered > without side effects, e.g. so that read_early_config() can read > ".git/config" before the_repository has been set up. > > Fortunately, we didn't start to rely on this unintended behavior between > then and now, so we let's just remove it. It isn't harming anyone, but > it's confusing. > > Signed-off-by: Glen Choo <chooglen@google.com> As the author of the commit whose rationale was quoted above, I am delighted to provide my ACK to both commit message and diff. Thanks, Johannes > --- > setup.c: don't setup in discover_git_directory() > > This is the scissors patch I sent on Victoria's series [1], but rebased > onto "master" since that series hasn't been merged yet. The merge > conflict resolution is to delete all of the conflicting lines: > > - the_repository->repository_format_worktree_config = > - candidate.worktree_config; > - > > > IOW it's the original scissors patch if queued on top of Victoria's > series, but it might be cleaner to invert that, i.e. if we pretended > that this was in "master" already, there wouldn't be reason to add those > lines to begin with. > > [1] > https://lore.kernel.org/git/kl6llegnfccw.fsf@chooglen-macbookpro.roam.corp.google.com > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1526%2Fchooglen%2Fpush-nknkwmnkxolv-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1526/chooglen/push-nknkwmnkxolv-v1 > Pull-Request: https://github.com/git/git/pull/1526 > > setup.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/setup.c b/setup.c > index 458582207ea..bbd95f52c0f 100644 > --- a/setup.c > +++ b/setup.c > @@ -1423,11 +1423,6 @@ int discover_git_directory(struct strbuf *commondir, > return -1; > } > > - /* take ownership of candidate.partial_clone */ > - the_repository->repository_format_partial_clone = > - candidate.partial_clone; > - candidate.partial_clone = NULL; > - > clear_repository_format(&candidate); > return 0; > } > > base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09 > -- > gitgitgadget >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> As explained by 16ac8b8db6 (setup: introduce the >> discover_git_directory() function, 2017-03-13) and the comment on its >> declaration, discover_git_directory() is intended to be an entrypoint >> into setup.c machinery that allows the Git directory to be discovered >> without side effects, e.g. so that read_early_config() can read >> ".git/config" before the_repository has been set up. >> >> Fortunately, we didn't start to rely on this unintended behavior between >> then and now, so we let's just remove it. It isn't harming anyone, but >> it's confusing. >> >> Signed-off-by: Glen Choo <chooglen@google.com> > > As the author of the commit whose rationale was quoted above, I am > delighted to provide my ACK to both commit message and diff. > > Thanks, > Johannes Thanks, both, for writing and reviewing. Queued.
diff --git a/setup.c b/setup.c index 458582207ea..bbd95f52c0f 100644 --- a/setup.c +++ b/setup.c @@ -1423,11 +1423,6 @@ int discover_git_directory(struct strbuf *commondir, return -1; } - /* take ownership of candidate.partial_clone */ - the_repository->repository_format_partial_clone = - candidate.partial_clone; - candidate.partial_clone = NULL; - clear_repository_format(&candidate); return 0; }