Re: [PATCH] git: --no-lazy-fetch option

From: Jeff King <peff@peff.net>
Date: Feb 17, 2024, 05:29 UTC
Message-ID: <20240217052932.GC539459@coredump.intra.peff.net>
In-Reply-To: <xmqq1q9cl3xv.fsf@gitster.g>

On Fri, Feb 16, 2024 at 09:22:20AM -0800, Junio C Hamano wrote:

> Here is a preliminary clean-up only for Documentation.  Will not be
> queuing before the final, but just so that I won't forget.

I think this is an improvement, but two small comments...

Show 14 quoted lines

> ------- >8 ------------- >8 ------------- >8 -------
> Subject: [PATCH] git: document GIT_NO_REPLACE_OBJECTS environment variable
> 
> This variable is used as the primary way to disable the object
> replacement mechanism, with the "--no-replace-objects" command line
> option as an end-user visible way to set it, but has not been
> documented.
> 
> The original reason why it was left undocumented might be because it
> was meant as an internal implementation detail, but the thing is,
> that our tests use the environment variable directly without the
> command line option, and there certainly are folks who learned its
> use from there, making it impossible to deprecate or change its
> behaviour by now.

I agree that we should document these sorts of variables; they really are part of the public interface since it's so easy for users to see them in their own scripts, aliases, etc, when we set them.

But in fact do document this environment variable in git-replace(1), and have for a long time. So I don't think we need to even make claims about its undocumented state. :)

Show 7 quoted lines

>  --no-replace-objects::
> -	Do not use replacement refs to replace Git objects. See
> -	linkgit:git-replace[1] for more information.
> +	Do not use replacement refs to replace Git objects.
> +	This is equivalent to exporting the `GIT_NO_REPLACE_OBJECTS`
> +	environment variable with any value.
> +	See linkgit:git-replace[1] for more information.

The text both before and after your patch links to git-replace, which covers the variable. So I think the "before" state is actually not that bad. But I still think mentioning it explicitly is better still.

However, should it get an entry in the "ENVIRONMENT VARIABLES" section? We cover stuff there like GIT_LITERAL_PATHSPECS, which is triggered in the same way.

> Add documentation and note that for this variable, unlike many
> boolean-looking environment variables, only the presence matters,
> not what value it is set to.

Yuck. IMHO depending on GIT_NO_REPLACE_OBJECTS=0 is somewhat crazy, and we could consider the current behavior a bug. It's probably not _that_ big a deal either way (because I would not expect anybody to set it that way in the first place). But I wonder if being consistent across variables trumps retaining weird historical compatibility for such a far-fetched case. I dunno. I guess this is https://xkcd.com/1172/. :)

-Peff