patchgit: --no-lazy-fetch option

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(+)

Junio C Hamano <gitster@pobox.com> writes:

Nit: perhaps s/locally/already locally/ is better?

Linus Arver <linusa@google.com> writes:

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.

?

Linus Arver <linusa@google.com> writes:

Wonderful.  Thanks for an extra set of eyes.

On Thu, Feb 08, 2024 at 03:17:31PM -0800, Junio C Hamano wrote:

That seems like a good feature, but...

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

Jeff King <peff@peff.net> writes:

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

Junio C Hamano <gitster@pobox.com> writes:

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(-)

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * And this comes on top of the original one plus the documentation
   update for GIT_NO_REPLACE_OBJECTS.

 Documentation/git.txt    | 2 ++
 environment.c            | 4 ++++
 environment.h            | 1 +
 git.c                    | 3 +++
 t/t0410-partial-clone.sh | 9 +++++++++
 5 files changed, 19 insertions(+)

FWIW, I see some typos. Otherwise this patch along with your "git: document GIT_NO_REPLACE_OBJECTS environment variable" one both LGTM for wording/readability. I must defer to Peff and others for "is this patch actually doing the right thing?" ;).

Junio C Hamano <gitster@pobox.com> writes:

s/boundary/boundaries

s/equivalents/equivalent

Linus Arver <linusa@google.com> writes:

Right; thanks.

I meant to say that these two are "equivalents" (noun, plural).

I can rephrase to

	Just use git_env_bool() to make "export GIT_NO_LAZY_FETCH=0"
	an equivalent to "unset GIT_NO_LAZY_FETCH".

though, of course.

Junio C Hamano <gitster@pobox.com> writes:

Ah, I see.

SGTM either way. Thanks.

On Fri, Feb 16, 2024 at 01:09:43PM -0800, Junio C Hamano wrote:

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.

As with the other patch, I'd suggest adding an entry to the list of environment variables later in the manpage.

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?

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.

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

Jeff King <peff@peff.net> writes:

Thanks, done.

We'll have to make a separate patch to add GIT_NO_REPLACE_OBJECTS to the list, by the way (#leftoverbits), which I used as a model to find what needs to be updated.

OK.

True. Again, we'd need to clean-up the NO_REPLACE_OBJECTS codepath as well---the disable_replace_refs() call should also be redundant, which I mimicked to add the no-lazy-fetch codepath (#leftoverbits).

I'd probably leave it outside the scope of this series; as nobody uses xsetenv() in git.c currently, it would be a clean-up topic on its own (#leftoverbits).

True, again. I'll test all three variants (i.e. environment, command line option to "git", forced subprocess turning command line option to "git" into environment).

Thanks.

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH v2 3/3] git: extend --no-lazy-fetch to work across subprocesses

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

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1:  00e202b679 ! 1:  e0764f2e21 git: extend --no-lazy-fetch to work across subprocesses
    @@ Documentation/git.txt: If you just want to run git as if it was started in `<pat
      
      --literal-pathspecs::
      	Treat pathspecs literally (i.e. no globbing, no pathspec magic).
    +@@ Documentation/git.txt: for full details.
    + 	Setting this Boolean environment variable to true will cause Git to treat all
    + 	pathspecs as case-insensitive.
    + 
    ++`GIT_NO_LAZY_FETCH`::
    ++	Setting this Boolean environment variable to true tells Git
    ++	not to lazily fetch missing objects from the promisor remote
    ++	on demand.
    ++
    + `GIT_REFLOG_ACTION`::
    + 	When a ref is updated, reflog entries are created to keep
    + 	track of the reason why the ref was updated (which is
     
      ## environment.c ##
     @@ environment.c: const char * const local_repo_env[] = {
    @@ environment.c: void setup_git_env(const char *git_dir)
     
      ## environment.h ##
     @@ environment.h: 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 NO_LAZY_FETCH_ENVIRONMENT "GIT_NO_LAZY_FETCH"
      #define GITATTRIBUTES_FILE ".gitattributes"
      #define INFOATTRIBUTES_FILE "info/attributes"
    + #define ATTRIBUTE_MACRO_PREFIX "[attr]"
     
      ## git.c ##
     @@ git.c: static int handle_options(const char ***argv, int *argc, int *envchanged)
    @@ t/t0410-partial-clone.sh: test_expect_success 'lazy-fetch when accessing object
     +	# 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" &&
    ++
    ++	# The same with command line option to "git"
     +	test_must_fail git --no-lazy-fetch -C partial.git cat-file -e "$FILE_HASH" &&
     +
    ++	# The same, forcing a subprocess via an alias
    ++	test_must_fail git --no-lazy-fetch -C partial.git \
    ++		-c alias.foo="!git cat-file" foo -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 &&

 Documentation/git.txt    |  7 +++++++
 environment.c            |  4 ++++
 environment.h            |  1 +
 git.c                    |  3 +++
 t/t0410-partial-clone.sh | 15 +++++++++++++++
 5 files changed, 30 insertions(+)

On Mon, Feb 26, 2024 at 10:04:54PM -0800, Junio C Hamano wrote:

This looks pretty reasonable to me, and the lines you drew for #leftoverbits all seemed liked good spots.

The only thing I noticed in the patch was this (which I think is not even new in this round):

This will clear the environment variable when we move between repositories. I can see an argument for it, and certainly that's how GIT_NO_REPLACE_OBJECTS works.

But I can also see an argument that this is not a property of the _repository_, but of the request. For example, if I run "git --no-lazy-fetch log" and we cross into a submodule to do a diff, should that submodule also avoid lazy-fetching? I'd think so, but I think your patch would restore the defaults when we "enter" the submodule repo.

There's some prior art there, I think, in how GIT_CEILING_DIRECTORIES works, or even something like "git --literal-pathspecs", neither of which appear in local_repo_env.

-Peff

Jeff King <peff@peff.net> writes:

Unlike GIT_CEILING_DIRECTORIES that is more or less permanent and a part of configuring an everyday environment for real work, I view this --no-lazy-fetch thing as solely a debugging aid. A repository with promisor wouldn't be able to "fill the gap" by forbidding on-demand fetching of promised objects while running say "git fetch" from elsewhere and it just will die with "some objects we are supposed to have are missing from this repository".

So I do not have a strong opinion either way, if it is more convenient to propagate the request out to other repositories when we run processes in two or more repositories (e.g. "git clone --local"), or if it is more convenient to make sure that the request is limited to the target repository. Here is a version without the local_repo_env[] change.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH v3 3/3] git: extend --no-lazy-fetch to work across subprocesses

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

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.

Also do not model how the request is not propagated to subprocesses we spawn (e.g. "git clone --local" that spawns a new process to work in the origin repository, while the original one working in the newly created one) by the "--no-replace-objects" option, as this "do not lazily fetch from the promisor" is more about a per-request debugging aid, not "this repository's promisor should not be relied upon" property specific to a repository.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Range-diff:
1:  5fa7ddbf68 ! 1:  ea40bb9a1d git: extend --no-lazy-fetch to work across subprocesses
    @@ Commit message
         allow "export GIT_NO_LAZY_FETCH=0" and "unset GIT_NO_LAZY_FETCH"
         to be equivalents.
     
    +    Also do not model how the request is not propagated to subprocesses
    +    we spawn (e.g. "git clone --local" that spawns a new process to work
    +    in the origin repository, while the original one working in the
    +    newly created one) by the "--no-replace-objects" option, as this "do
    +    not lazily fetch from the promisor" is more about a per-request
    +    debugging aid, not "this repository's promisor should not be relied
    +    upon" property specific to a repository.
    +
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## Documentation/git.txt ##
    @@ Documentation/git.txt: for full details.
      	track of the reason why the ref was updated (which is
     
      ## environment.c ##
    -@@ environment.c: const char * const local_repo_env[] = {
    - 	GRAFT_ENVIRONMENT,
    - 	INDEX_ENVIRONMENT,
    - 	NO_REPLACE_OBJECTS_ENVIRONMENT,
    -+	NO_LAZY_FETCH_ENVIRONMENT,
    - 	GIT_REPLACE_REF_BASE_ENVIRONMENT,
    - 	GIT_PREFIX_ENVIRONMENT,
    - 	GIT_SHALLOW_FILE_ENVIRONMENT,
     @@ environment.c: void setup_git_env(const char *git_dir)
      	shallow_file = getenv(GIT_SHALLOW_FILE_ENVIRONMENT);
      	if (shallow_file)

 Documentation/git.txt    |  7 +++++++
 environment.c            |  3 +++
 environment.h            |  1 +
 git.c                    |  3 +++
 t/t0410-partial-clone.sh | 15 +++++++++++++++
 5 files changed, 29 insertions(+)

On Tue, Feb 27, 2024 at 08:48:29AM -0800, Junio C Hamano wrote:

Yeah, GIT_CEILING_DIRECTORIES is maybe a bad example. But I do think LITERAL_PATHSPECS is a better one, and the submodule-fetch example I gave would be genuinely surprising if it behaved differently than the superproject, I'd think.

I do agree this is probably going to mostly be a debugging aid, so it might not matter much. But once in the wild these things tend to take on a life of their own. ;)

So anyway, this version seems good to me.

-Peff

Jeff King <peff@peff.net> writes:

Thanks.

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

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

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

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.

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

Jeff King <peff@peff.net> writes:

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?

Jeff King <peff@peff.net> writes:

Thanks for the helpful explanation, very much appreciated.