mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <>
To: Johannes Schindelin via GitGitGadget <>
	"Konstantin Ryabitsev" <>,
	"SZEDER Gábor" <>,
	"Junio C Hamano" <>,
	"Johannes Schindelin" <>
Subject: Re: [PATCH 1/1] bundle verify: error out if called without an object database
Date: Mon, 27 May 2019 21:51:33 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

> From: Johannes Schindelin <>
> 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)

  /* 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".


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


  parent reply	other threads:[~2019-05-28  1:51 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-27 19:59 [PATCH 0/1] bundle verify: improve the user experience when called without a .git/ directory Johannes Schindelin via GitGitGadget
2019-05-27 19:59 ` [PATCH 1/1] bundle verify: error out if called without an object database Johannes Schindelin via GitGitGadget
2019-05-27 20:08   ` brian m. carlson
2019-05-27 20:20     ` Johannes Schindelin
2019-05-28  1:51   ` Jeff King [this message]
2019-05-28 11:17     ` Johannes Schindelin
2019-05-28 20:58       ` Jeff King
2019-05-28 20:03     ` Junio C Hamano
2019-05-28 21:04       ` Jeff King
2019-05-28 21:09         ` Junio C Hamano
     [not found] ` <>
     [not found]   ` <>
2019-05-27 20:25     ` [PATCH v2 " brian m. carlson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).