Re: [GSOC PATCH v6 0/3] environment: remove sparse-checkout related global variables

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2025, 22:14 UTC
Message-ID: <xmqqcy9qlfm8.fsf@gitster.g>
In-Reply-To: <cover.1752882401.git.ayu.chandekar@gmail.com>

Ayush Chandekar <ayu.chandekar@gmail.com> writes:

Show 24 quoted lines

> This patch series aims to remove global variables related to
> sparse-checkout from the global scope and to remove the definition
> '#define USE_THE_REPOSITORY_VARIABLE' from a few files.
>
> It contains three patches:
>
> 1 - Remove the global variable 'core_apply_sparse_checkout' and
> move its setting to the 'struct repo_settings'. Also remove the
> definition '#define USE_THE_REPOSITORY_VARIABLE' from
> "builtin/backfill.c".
>
> 2 - Remove the global variable 'core_sparse_checkout_cone' and
> move its setting to the 'struct repo_settings'.
>
> 3 - Remove the global variable
> 'sparse_expect_files_outside_of_patterns` and move its setting to
> 'struct repo_settings'. Also remove the definition '#define
> USE_THE_REPOSITORY_VARIABLE' from "sparse-index.c"
> --
> Discussions since v5:
>
> * For 1/3 and 2/3, Junio told me that it was concerning to put so
>   many calls to `prepare_repo_settings()` so I tried to minimize the
>   calls and made sure that there's no useless calling.

I didn't mean that the number of places is the problem. What I found troubling was that this is not done in any central place, so it is hard to notice even if some random cmd_foo() failed to call the function before doing its real work. For example, shouldn't we be able to, at least for built-in commands that have RUN_SETUP bit set, centrally call prepare_repo_settings() somewhere late in git.c:run_builtin() after we figure out what should be in the_repository? Now historically, setting up a repository may never have involved opening and parsing tons of configuration files, so such a change may be incurring extra overhead we did not have to pay, so it needs a lot more thought than just trying to minimize the number of calls, but some performance measurement.

> * For 3/3, Phillip told me that it broke user-facing as it will be
>   parsed quite late in the callchain and might throw an error mid
>   operation which we do not want.

So has the behaviour change caused by 3/3 been resolved?

A meta-level comment and a half.

 * Please do not use "-- " (that is a line that has dash dash and a
   single space and nothing else on it) lightly.  It is called
   signature line and often MUA pays attention to it when responding
   to a message with such a line by omitting everything after it
   (which is supposed to be your "who I am" advertisement) when
   quoting the original.  Since you had one before the "discussions
   since v5" section and the range-diff, I had to manually resurrect
   the part after the signature line while composing this message.

 * This throws everything in repo_settings, but these settings are
   inherently per repository and they are meaningful only when you
   are working with a repository.  What makes us choose to make them
   new members in the repo_settings structure, not direct members in
   the repository structure?

   Not an objection and not a suggestion to move them out of the
   repo_settings and to the repository proper.  Just wanted to hear
   the reasoning behind it (and have the rationale clearly
   documented, preferrably in the proposed log messages).