Re: [PATCH v2 4/4] promisor-remote: teach lazy-fetch in any repo

Jonathan Tan [off-list ref] writes:

 		/* Check if it is a missing object */
-		if (fetch_if_missing && has_promisor_remote() &&
-		    !already_retried && r == the_repository &&
+		if (fetch_if_missing && repo_has_promisor_remote(r) &&
+		    !already_retried &&

Turning has_promisor_remote() into repo_has_promisor_remote(r) does
make tons of sense.  Is this part of the code ready to lose "'r' must
be the_repository because has_promisor_remote() only works on the
primary in-core repository" we had before?

This is what prepare_submodule_repo_env_in_gitdir() does; it makes
me wonder if it (i.e. set up environment for that other repository,
including the GIT_DIR and possibly other per-repository environment
variable override) should be the primary API callers would want,
instead of a more limited prepare_other_repo_env() that does not
even take 'repo' parameter.  Doesn't it feel somewhat strange for a
function that is supposed to help preparing a part of child process
by filling appropriate environ[] array to be run in a repository
that is different from ours (which is "other repo" part of its name)
not to want to even know which repository the "other" repo is?

The reason why this only gives size is because it will eventually
become unnecessary once the main code starts running things in a
submodule repository properly (i.e. without doing the alternate odb
thing), and a more elaborate check is not worth your engineering
effort?  Object type and object sizes are something that you can
safely express in plain text, would be handy for testing, and would
not require too much extra code, I'd imagine.

+static void object_info(const char *gitdir, const char *oid_hex)
+{
+	struct repository r;
+	struct object_id oid;
+	unsigned long size;
+	struct object_info oi = {.sizep = &size};
+	const char *p;
+
+	if (repo_init(&r, gitdir, NULL))
+		die("could not init repo");
+	if (parse_oid_hex(oid_hex, &oid, &p))
+		die("could not parse oid");
+	if (oid_object_info_extended(&r, &oid, &oi, 0))
+		die("could not obtain object info");
+	printf("%d\n", (int) size);

Mimicking what builtin/cat-file.c::cat_one_file() does, for example, and
using

	printf("%"PRIuMAX"\n", (uintmax_t)size);

might be better (I was wondering if we can extract reusable helpers,
but I do not think that is worth doing, if this is meant to be
temporary stop-gap measure).

Thanks.

Jonathan Tan [off-list ref] writes:

quoted

 		/* Check if it is a missing object */
-		if (fetch_if_missing && has_promisor_remote() &&
-		    !already_retried && r == the_repository &&
+		if (fetch_if_missing && repo_has_promisor_remote(r) &&
+		    !already_retried &&

Turning has_promisor_remote() into repo_has_promisor_remote(r) does
make tons of sense.  Is this part of the code ready to lose "'r' must
be the_repository because has_promisor_remote() only works on the
primary in-core repository" we had before?

Yes - that is precisely what I test with the test helper (running this
code path with "r" that is not the_repository).

quoted

@@ -21,6 +22,11 @@ static int fetch_objects(const char *remote_name,
 
 	child.git_cmd = 1;
 	child.in = -1;
+	if (repo != the_repository) {
+		prepare_other_repo_env(&child.env_array);
+		strvec_pushf(&child.env_array, "%s=%s", GIT_DIR_ENVIRONMENT,
+			     repo->gitdir);
+	}

This is what prepare_submodule_repo_env_in_gitdir() does; it makes
me wonder if it (i.e. set up environment for that other repository,
including the GIT_DIR and possibly other per-repository environment
variable override) should be the primary API callers would want,
instead of a more limited prepare_other_repo_env() that does not
even take 'repo' parameter.  Doesn't it feel somewhat strange for a
function that is supposed to help preparing a part of child process
by filling appropriate environ[] array to be run in a repository
that is different from ours (which is "other repo" part of its name)
not to want to even know which repository the "other" repo is?

Good point. I'll update prepare_other_repo_env() to have a gitdir
parameter.

quoted

diff --git a/t/helper/test-partial-clone.c b/t/helper/test-partial-clone.c
new file mode 100644
index 0000000000..3f102cfddd
--- /dev/null
+++ b/t/helper/test-partial-clone.c

@@ -0,0 +1,43 @@
+#include "cache.h"
+#include "test-tool.h"
+#include "repository.h"
+#include "object-store.h"
+
+/*
+ * Prints the size of the object corresponding to the given hash in a specific
+ * gitdir. This is similar to "git -C gitdir cat-file -s", except that this
+ * exercises the code that accesses the object of an arbitrary repository that
+ * is not the_repository. ("git -C gitdir" makes it so that the_repository is
+ * the one in gitdir.)
+ */

The reason why this only gives size is because it will eventually
become unnecessary once the main code starts running things in a
submodule repository properly (i.e. without doing the alternate odb
thing),

If you mean that this code path can be tested through user-visible
commands (e.g. git grep with submodule recursion) once the main code
starts avoiding doing the alternate odb thing, so this helper will
eventually be unnecessary, then the answer is yes.

and a more elaborate check is not worth your engineering
effort?

Even now, I don't think a more elaborate check is worth the engineering
effort - I just want to check that the file indeed was lazy-fetched, so
any minor datum would suffice.

Object type and object sizes are something that you can
safely express in plain text, would be handy for testing, and would
not require too much extra code, I'd imagine.

It would, but we can already use "git cat-file -s" (or -t) for that. The
helper is meant to test a specific code path wherein we access a
submodule object during a process running in the superproject.

quoted

+	printf("%d\n", (int) size);

Mimicking what builtin/cat-file.c::cat_one_file() does, for example, and
using

	printf("%"PRIuMAX"\n", (uintmax_t)size);

might be better (I was wondering if we can extract reusable helpers,
but I do not think that is worth doing, if this is meant to be
temporary stop-gap measure).

Thanks.

Sounds good - I'll change this.