Re: [PATCH 2/5] backfill: basic functionality and tests

On 12/16/24 3:01 AM, Patrick Steinhardt wrote:
 > On Fri, Dec 06, 2024 at 08:07:15PM +0000, Derrick Stolee via GitGitGadget wrote:

 >> +The `git backfill` command provides a way for the user to request that
 >> +Git downloads the missing blobs (with optional filters) such that the
 >> +missing blobs representing historical versions of files can be downloaded
 >> +in batches. The `backfill` command attempts to optimize the request by
 >> +grouping blobs that appear at the same path, hopefully leading to good
 >> +delta compression in the packfile sent by the server.
 >
 > Hm. So we're asking the user to fix a usability issue of git-blame(1),
 > don't we? Ideally, git-blame(1) itself should know to transparently
 > batch the blobs it requires to compute its output, shouldn't it? That
 > usecase alone doesn't yet convince me that git-backfill(1) is a good
 > idea as I'd think we should rather fix the underlying issue.

I've looked into making this change for 'git blame' and it is a nontrivial change. I'm not sure on the timeline that we could expect 'git blame' to be improved.

But you're right that this is not enough justification on its own. It's an example of a kind of command that has these problems, including 'git log [-p|-L]'.

 > So are there other usecases for git-backfill(1)? I can imagine that it
 > might be helpful in the context of scripts that know they'll operate on
 > a bunch of blobs.

One major motivation is that it can essentially provide a way to do resumable clones: get the commits and trees in one go with a blobless clone, then download the blobs in batches. If something interrupts the 'git backfill' command, then restarting it will only repeat the most recent batch.

Finally, in a later patch we can see that the --sparse option allows a user to operate as if they have a full clone but where they only include blob data within their sparse-checkout, providing reduced disk space and network time to get to that state.

 >> +	if (ctx->current_batch.nr >= ctx->batch_size)
 >> +		download_batch(ctx);
 >
 > Okay, so the batch size is just "best effort". If we walk a tree that
 > makes us exceed the batch size then we wouldn't issue a fetch during the
 > tree walk. Is there any specific reason for this behaviour?
 >
 > In any case, as long as this is properly documented I think this should
 > be fine in general.

The main reason is that we expect the server to return a packfile that has many delta relationships within the objects at a given path. If we split the batch in the middle of a path batch, then we are artificially introducing breaks in the delta chains that could be wasteful.

However, this batching pattern could be problematic if there are a million versions of a single file and the batch is too large to download efficiently. This "absolute max batch size" is currently left as a future extension.

 >> +	clear_backfill_context(ctx);
 >
 > Are we leaking `revs` and `info`?

At the moment. Will fix.

 >> +	return ret;
 >> +}
 >> +
 >>   int cmd_backfill(int argc, const char **argv, const char *prefix, struct 
repository *repo)
 >>   {
 >> +	struct backfill_context ctx = {
 >> +		.repo = repo,
 >> +		.current_batch = OID_ARRAY_INIT,
 >> +		.batch_size = 50000,
 >> +	};
 >>   	struct option options[] = {
 >>   		OPT_END(),
 >>   	};
 >> @@ -23,7 +123,5 @@ int cmd_backfill(int argc, const char **argv, const char 
*prefix, struct reposit
 >>
 >>   	repo_config(repo, git_default_config, NULL);
 >>
 >> -	die(_("not implemented"));
 >> -
 >> -	return 0;
 >> +	return do_backfill(&ctx);
 >>   }
 >
 > The current iteration only backfills blobs as far as I can see. Do we
 > maybe want to keep the door open for future changes in git-backfill(1)
 > by implementing this via a "blob" subcommand?

I think that one tricky part is to ask "what could be missing?". With Git's partial clone, it seems that we could have treeless or depth- based tree restrictions. Technically, there could also be clones that restrict to a set of sparse-checkout patterns, but I'm not aware of any server that will respect those kinds of clones. At the moment, the tree walk would fault in any missing trees as they are seen, but this is extremely inefficient.

I think that the path-walk API could be adjusted to be more careful to check for the existence of an object before automatically loading it. That would allow for batched downloads of missing trees, though a second scan would be required to get the next layer of objects.

I'm not sure a subcommand is the right way to solve for this potential future, but instead later we could adjust the logic to be better for these treeless or tree-restricted clones.

Thanks, -Stolee