Re: [PATCH 1/1] bundle verify: error out if called without an object database

[Jeff King <peff@peff.net>]

On Mon, May 27, 2019 at 12:59:14PM -0700, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The deal with bundles is: they really are thin packs, with very little
> sugar on top. So we really need a repository (or more appropriately, an
> object database) to work with, when asked to verify a bundle.
> 
> Let's error out with a useful error message if `git bundle verify` is
> called without such an object database to work with.

I think this is going in the right direction, but I think there are a
few subtle bits worth thinking about.

As Gábor noted in the earlier thread, if the bundle doesn't have any
prerequisites, this _used_ to work before b1ef400eec (setup_git_env:
avoid blind fall-back to ".git", 2016-10-20). I don't know if anybody
cares about that case or not, but we could do something like:

  if (p->nr)
	verify_prerequisites();

  /* otherwise, fall through to the printing portions */

and then just check for a repository in verify_prerequisites(), which is
the only part that needs to look at the repository object at all.

If we _are_ OK just forbidding the operation entirely outside of a
repository, then should we be doing this check in cmd_bundle() instead?
We already have checks there for "create" and "unbundle".

Likewise:

> diff --git a/bundle.c b/bundle.c
> index b45666c49b..b5d21cd80f 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -142,6 +142,9 @@ int verify_bundle(struct repository *r,
>  	int i, ret = 0, req_nr;
>  	const char *message = _("Repository lacks these prerequisite commits:");
>  
> +	if (!r || !r->objects || !r->objects->odb)
> +		return error(_("need a repository to verify a bundle"));

Those other checks are done with startup_info->have_repository. I don't
think that makes sense here (we were passed in a repository object to
operate on, so the global might or might not match). Doing it at that
higher level means that other callers of verify_bundle() are not
protected, but I think may be OK. The top-level commands are generally
responsible for setting up the repository and bailing if the requested
operation does not make sense without one.

If we do want to leave the check at this level, I'm a little
uncomfortable with how intimate this check is with the parts of "struct
repository". For instance, who sets of r->objects and r->objects->odb,
and is it possible for us to have a working repo struct that has those
as NULL (i.e., where they could be lazily filled in)? Even if it works
now, it seems like a subtle thing that could easily be broken during
later refactoring.

Instead, could we have cmd_bundle() pass in NULL when instead of
the_repository when there's no repo? That seems like a much easier
pattern in general for low-level code to decide when it has no repo
available (though I suspect many code paths would have to be adjusted to
handle a NULL repository argument).

-Peff