Hi Phillip,
On Wed, Jul 2, 2025 at 2:31 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> Hi Ayush
>
> On 02/07/2025 00:53, Ayush Chandekar wrote:
> > On Tue, Jul 1, 2025 at 6:48 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> >> On 30/06/2025 20:27, Ayush Chandekar wrote:
> >>>
> >>> void clear_skip_worktree_from_present_files(struct index_state *istate)
> >>> {
> >>> + int sparse_expect_files_outside_of_patterns = 0;
> >>> + repo_config_get_bool(istate->repo, "sparse.expectfilesoutsideofpatterns",
> >>> + &sparse_expect_files_outside_of_patterns);
> >>
> >> This changes the user facing behavior if
> >> sparse.expectfilesoutsideofpatterns is not a valid boolean value.
> >> Currently git will error out when it first starts because that config
> >> value is parsed by git_default_config() which is called by almost all
> >> git commands. This means that if someone sets an invalid value they get
> >> timely feedback that the value is invalid and git dies before doing
> >> anything. Now, if the value is invalid, git will only die if this
> >> function is called and it is likely to die in the middle of a command.
> >
> > Yes, I get your point. However, if we look at settings which are
> > shifted to `struct repo_settings`, the behaviour is to set a
> > fallback/default value in case of an invalid input, instead of
> > throwing an error. This is done inside the `prepare_repo_settings()`
> > function, which is often called in the middle of a process.
>
> I'm a bit confused by this and I'm not quite sure what you're saying for
> a couple of reasons. Firstly this patch is not adding a new member to
> struct repo_settings, it is parsing the config directly and will die()
> in git_config_bool() if the config value is invalid. Secondly
> prepare_repo_settings() ends up calling git_config_bool() and so will
> also die if the config value is invalid rather than setting a default
> value. In the case of prepare_repo_settings() commands that do not want
> to die in the middle of an operation can call that function early on
> before they start doing any real work. Looking at the output of "git
> grep prepare_repo_settings()" many do exactly that. Here there is no
> option for a command to die() early on invalid config values if it wants to.
>
> Thanks
>
> Phillip
>
Yeah, you're right about the `prepare_repo_settings()` which throws
error and since they're called early on, we are notified about it
before any heavy operation takes place. I had it confused as it also
stores a default value if there isn't a config setting set. I can move
this setting into `struct repo_settings`, and since
`prepare_repo_settings()` is already called just before the function
that uses this variable, I don't have to add any extra call to it.
Thanks!
Ayush
