On Fri, Feb 16, 2024 at 01:09:43PM -0800, Junio C Hamano wrote:
> Modeling after how the `--no-replace-objects` option is made usable
> across subprocess spawning (e.g., cURL based remote helpers are
> spawned as a separate process while running "git fetch"), allow the
> `--no-lazy-fetch` option to be passed across process boundary.
>
> Do not model how the value of GIT_NO_REPLACE_OBJECTS environment
> variable is ignored, though. Just use the usual git_env_bool() to
> allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
> to be equivalents.
Nice. I wondered if we would need to hide fetch_if_missing behind a
function that lazy-evaluates the environment variable, but it looks like
setup_git_env() is central enough to cover us here.
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 2f1cb3ef4e..be2829003d 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -183,6 +183,8 @@ If you just want to run git as if it was started in `<path>` then use
> Do not fetch missing objects from the promisor remote on
> demand. Useful together with `git cat-file -e <object>` to
> see if the object is locally available.
> + This is equivalent to setting the `GIT_NO_LAZY_FETCH`
> + environment variable to `1`.
As with the other patch, I'd suggest adding an entry to the list of
environment variables later in the manpage.
> --- a/environment.h
> +++ b/environment.h
> @@ -35,6 +35,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
> #define EXEC_PATH_ENVIRONMENT "GIT_EXEC_PATH"
> #define CEILING_DIRECTORIES_ENVIRONMENT "GIT_CEILING_DIRECTORIES"
> #define NO_REPLACE_OBJECTS_ENVIRONMENT "GIT_NO_REPLACE_OBJECTS"
> +#define NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
> #define GIT_REPLACE_REF_BASE_ENVIRONMENT "GIT_REPLACE_REF_BASE"
> #define GITATTRIBUTES_FILE ".gitattributes"
> #define INFOATTRIBUTES_FILE "info/attributes"
A small nit, but maybe worth keeping the two replace-related variables
next to each other, rather than sticking the new one in the middle?
> diff --git a/git.c b/git.c
> index 28e8bf7497..d11d4dc77b 100644
> --- a/git.c
> +++ b/git.c
> @@ -189,6 +189,9 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
> *envchanged = 1;
> } else if (!strcmp(cmd, "--no-lazy-fetch")) {
> fetch_if_missing = 0;
> + setenv(NO_LAZY_FETCH_ENVIRONMENT, "1", 1);
> + if (envchanged)
> + *envchanged = 1;
> } else if (!strcmp(cmd, "--no-replace-objects")) {
> disable_replace_refs();
> setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
I _suspect_ this makes the fetch_if_missing call redundant, since we
should be parsing these before doing any repo setup that would trigger
the code that reads the environment variable.
This should probably also be xsetenv(), though as you can see in the
context we are not very consistent about using it. :) But certainly if
we failed to set it I would prefer to see an error rather than
accidentally lazy-fetching.
> diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh
> index 5b7bee888d..59629cea1f 100755
> --- a/t/t0410-partial-clone.sh
> +++ b/t/t0410-partial-clone.sh
> @@ -665,6 +665,15 @@ test_expect_success 'lazy-fetch when accessing object not in the_repository' '
> git -C partial.git rev-list --objects --missing=print HEAD >out &&
> grep "[?]$FILE_HASH" out &&
>
> + # The no-lazy-fetch mechanism prevents Git from fetching
> + test_must_fail env GIT_NO_LAZY_FETCH=1 \
> + git -C partial.git cat-file -e "$FILE_HASH" &&
> + test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
> +
> + # Sanity check that the file is still missing
> + git -C partial.git rev-list --objects --missing=print HEAD >out &&
> + grep "[?]$FILE_HASH" out &&
OK, we exercise it by setting the variable directly. A more interesting
one might be:
git -c alias.foo='!git cat-file' --no-lazy-fetch ...
which should fail without the patch.
I also wondered why we were testing the direct-builtin invocation here,
since we are building on top of the original --no-lazy-fetch patch, but
it looks like it is because that patch didn't add any tests at all). I
think they would make more sense as a single patch, but I guess the
original is already in 'next' (though I suppose it is about to be
rewound). I am OK with it either way.
-Peff
