bundle verify: improve the user experience when called without a .git/ directory

From: Johannes Schindelin <[email protected]>

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.

Reported by Konstantin Ryabitsev.

Signed-off-by Johannes Schindelin <[email protected]>

---
 bundle.c                | 3 +++
 t/t5607-clone-bundle.sh | 6 ++++++
 2 files changed, 9 insertions(+)

bundle.c +3 -0

index b45666c49b..b5d21cd80f 100644
@@ -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"));
+
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;

t/t5607-clone-bundle.sh +0 -0

index cf39e9e243..2a0fb15cf1 100755

Quoted 3 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

The deal with bundles is: they really are thin packs, with very little

Generally a colon can only follow what could be a complete sentence, so maybe we want to say something like "The deal with bundles is that they really are…" or "The deal with bundles is this: they really are…".

Quoted 36 lines

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.

Reported by Konstantin Ryabitsev.

Signed-off-by Johannes Schindelin <[email protected]>

---
 bundle.c                | 3 +++
 t/t5607-clone-bundle.sh | 6 ++++++
 2 files changed, 9 insertions(+)

bundle.c +3 -0

index b45666c49b..b5d21cd80f 100644
@@ -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"));
+
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;

t/t5607-clone-bundle.sh +0 -0

index cf39e9e243..2a0fb15cf1 100755
@@ -14,6 +14,12 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 

t/t5607-clone-bundle.sh test_expect_success 'setup' ' · line 17

Quoted 1 line

t/t5607-clone-bundle.sh +1 -0

+test_expect_success '"verify" needs a worktree' '

Did you want to say "needs a repository"? Or do we really need a worktree?

Other than that, this looks fine to me. This is a much better experience than a BUG and a nice improvement overall.

From: Johannes Schindelin <[email protected]>

The deal with bundles is that 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.

Reported by Konstantin Ryabitsev.

Signed-off-by Johannes Schindelin <[email protected]>

From: Johannes Schindelin <[email protected]>

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.

Quoted 8 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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:

bundle.c int verify_bundle(struct repository *r, · line 146

Quoted 10 lines

bundle.c +2 -0

index b45666c49b..b5d21cd80f 100644
@@ -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).

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.

Quoted 5 lines

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:

Quoted 7 lines

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.

I am not so sure that I feel comfortable with optimizing for the no-op case. Because that's essentially what this boils down to: if there are no prerequisites, there is not a whole lot to do.

And I'd rather have the command be consistent about its demands, to abide by the Law of Least Surprise.

Quoted 5 lines · Jeff King

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:

Quoted 7 lines

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.

It depends on how important we consider the use of bundles that can be cloned from. If it is important enough, what you suggest is an improvement worth doing over what Dscho did.

A bundle that can be cloned from (i.e. no prerequisite) is meant to be used without having any repository by definition, so it is a grave regression to require object store when verifying such a bundle.

On the other hand, a bundle that cannot be cloned from but only usabel for an incremental sneaker-net update needs receiving repository anyway, so it is perfectly fine to require object store.

So I guess you are asking only about the part that I dismissed above as "not all that interesting". I.e., do people actually run:

git bundle verify foo.bundle &&
git clone foo.bundle

That is not nonsense, per-se, but it is somewhat pointless since the clone will verify the bundle itself anyway. But then, I guess I do not see much point in anyone calling "bundle verify" in the first place.