diff mbox series

git: --no-lazy-fetch option

Message ID xmqq1q9mmtpw.fsf@gitster.g (mailing list archive)
State New, archived
Headers show
Series git: --no-lazy-fetch option | expand

Commit Message

Junio C Hamano Feb. 8, 2024, 11:17 p.m. UTC
Sometimes, especially during tests of low level machinery, it is
handy to have a way to disable lazy fetching of objects.  This
allows us to say, for example, "git cat-file -e <object-name>", to
see if the object is locally available.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Linus Arver Feb. 13, 2024, 8:23 p.m. UTC | #1
Junio C Hamano <gitster@pobox.com> writes:

> Sometimes, especially during tests of low level machinery, it is
> handy to have a way to disable lazy fetching of objects.  This
> allows us to say, for example, "git cat-file -e <object-name>", to
> see if the object is locally available.

Nit: perhaps s/locally/already locally/ is better?
Linus Arver Feb. 13, 2024, 8:37 p.m. UTC | #2
Linus Arver <linusa@google.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sometimes, especially during tests of low level machinery, it is
>> handy to have a way to disable lazy fetching of objects.  This
>> allows us to say, for example, "git cat-file -e <object-name>", to
>> see if the object is locally available.
>
> Nit: perhaps s/locally/already locally/ is better?

I forgot to mention that the new flag is missing a documentation entry
in Documentation/git.txt. Perhaps something like

    --no-lazy-fetch::
        Do not fetch missing objects. Useful together with `git cat-file -e
        <object-name>`, for example, to see if the object is already
        locally available.

?
Junio C Hamano Feb. 13, 2024, 8:49 p.m. UTC | #3
Linus Arver <linusa@google.com> writes:

> Linus Arver <linusa@google.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sometimes, especially during tests of low level machinery, it is
>>> handy to have a way to disable lazy fetching of objects.  This
>>> allows us to say, for example, "git cat-file -e <object-name>", to
>>> see if the object is locally available.
>>
>> Nit: perhaps s/locally/already locally/ is better?
>
> I forgot to mention that the new flag is missing a documentation entry
> in Documentation/git.txt. Perhaps something like
>
>     --no-lazy-fetch::
>         Do not fetch missing objects. Useful together with `git cat-file -e
>         <object-name>`, for example, to see if the object is already
>         locally available.
>
> ?

Wonderful.  Thanks for an extra set of eyes.
Jeff King Feb. 15, 2024, 5:30 a.m. UTC | #4
On Thu, Feb 08, 2024 at 03:17:31PM -0800, Junio C Hamano wrote:

> Sometimes, especially during tests of low level machinery, it is
> handy to have a way to disable lazy fetching of objects.  This
> allows us to say, for example, "git cat-file -e <object-name>", to
> see if the object is locally available.

That seems like a good feature, but...

> @@ -186,6 +187,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			use_pager = 0;
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--no-lazy-fetch")) {
> +			fetch_if_missing = 0;
>  		} else if (!strcmp(cmd, "--no-replace-objects")) {
>  			disable_replace_refs();
>  			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);

This will only help builtin commands, and even then only the top-level
one. If I run "git --no-lazy-fetch foo" and "foo" is a script or an
alias, I'd expect it to still take effect. Ditto for sub-commands kicked
off by a builtin (say, a "rev-list" connectivity check caused by a
fetch).

So this probably needs to be modeled after --no-replace-objects, etc,
where we set an environment variable that makes it to child processes.

-Peff
Junio C Hamano Feb. 15, 2024, 5:04 p.m. UTC | #5
Jeff King <peff@peff.net> writes:

> On Thu, Feb 08, 2024 at 03:17:31PM -0800, Junio C Hamano wrote:
>
>> Sometimes, especially during tests of low level machinery, it is
>> handy to have a way to disable lazy fetching of objects.  This
>> allows us to say, for example, "git cat-file -e <object-name>", to
>> see if the object is locally available.
>
> That seems like a good feature, but...
>
>> @@ -186,6 +187,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>>  			use_pager = 0;
>>  			if (envchanged)
>>  				*envchanged = 1;
>> +		} else if (!strcmp(cmd, "--no-lazy-fetch")) {
>> +			fetch_if_missing = 0;
>>  		} else if (!strcmp(cmd, "--no-replace-objects")) {
>>  			disable_replace_refs();
>>  			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
>
> This will only help builtin commands, and even then only the top-level
> one. If I run "git --no-lazy-fetch foo" and "foo" is a script or an
> alias, I'd expect it to still take effect. Ditto for sub-commands kicked
> off by a builtin (say, a "rev-list" connectivity check caused by a
> fetch).
>
> So this probably needs to be modeled after --no-replace-objects, etc,
> where we set an environment variable that makes it to child processes.

Yuck, I was hoping that we can get away with the tiny change only
for builtins,but you're right.
Linus Arver Feb. 15, 2024, 8:59 p.m. UTC | #6
Jeff King <peff@peff.net> writes:

> On Thu, Feb 08, 2024 at 03:17:31PM -0800, Junio C Hamano wrote:
>
>> Sometimes, especially during tests of low level machinery, it is
>> handy to have a way to disable lazy fetching of objects.  This
>> allows us to say, for example, "git cat-file -e <object-name>", to
>> see if the object is locally available.
>
> That seems like a good feature, but...
>
>> @@ -186,6 +187,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>>  			use_pager = 0;
>>  			if (envchanged)
>>  				*envchanged = 1;
>> +		} else if (!strcmp(cmd, "--no-lazy-fetch")) {
>> +			fetch_if_missing = 0;
>>  		} else if (!strcmp(cmd, "--no-replace-objects")) {
>>  			disable_replace_refs();
>>  			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
>
> This will only help builtin commands, and even then only the top-level
> one. If I run "git --no-lazy-fetch foo" and "foo" is a script or an
> alias, I'd expect it to still take effect. Ditto for sub-commands kicked
> off by a builtin (say, a "rev-list" connectivity check caused by a
> fetch).
>
> So this probably needs to be modeled after --no-replace-objects, etc,
> where we set an environment variable that makes it to child processes.

Thanks for the helpful explanation, very much appreciated.
Junio C Hamano Feb. 16, 2024, 5:22 p.m. UTC | #7
Junio C Hamano <gitster@pobox.com> writes:

> Yuck, I was hoping that we can get away with the tiny change only
> for builtins,but you're right.

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

------- >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.

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git c/Documentation/git.txt i/Documentation/git.txt
index 95f451b22b..2f1cb3ef4e 100644
--- c/Documentation/git.txt
+++ i/Documentation/git.txt
@@ -174,8 +174,10 @@ If you just want to run git as if it was started in `<path>` then use
 	directory.
 
 --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.
 
 --no-lazy-fetch::
 	Do not fetch missing objects from the promisor remote on
Jeff King Feb. 17, 2024, 5:29 a.m. UTC | #8
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...

> ------- >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. :)

>  --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
Linus Arver March 9, 2024, 1:57 a.m. UTC | #9
Jeff King <peff@peff.net> writes:

> On Fri, Feb 16, 2024 at 09:22:20AM -0800, Junio C Hamano wrote:
>
> [...]
>> 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/. :)

I totally agree with your take on this. Would such cleanup patches
(e.g., changing the behavior of GIT_NO_REPLACE_OBJECTS=0 to be "false"
instead of "true") be acceptable as #leftoverbits?
diff mbox series

Patch

diff --git a/git.c b/git.c
index 7068a184b0..fd9b0e6a9e 100644
--- a/git.c
+++ b/git.c
@@ -4,6 +4,7 @@ 
 #include "exec-cmd.h"
 #include "gettext.h"
 #include "help.h"
+#include "object-file.h"
 #include "pager.h"
 #include "read-cache-ll.h"
 #include "run-command.h"
@@ -186,6 +187,8 @@  static int handle_options(const char ***argv, int *argc, int *envchanged)
 			use_pager = 0;
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--no-lazy-fetch")) {
+			fetch_if_missing = 0;
 		} else if (!strcmp(cmd, "--no-replace-objects")) {
 			disable_replace_refs();
 			setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);