[PATCH 0/1] bundle verify: improve the user experience when called without a .git/ directory

[PATCH 0/1] bundle verify: improve the user experience when called without a .git/ directory

[Johannes Schindelin via GitGitGadget]

The git bundle verify <bundle> command really needs access to a .git/ 
directory. But it did not make sure, instead erroring out with a BUG(),
making for a terrible user experience.

This patch fixes that.

Johannes Schindelin (1):
  bundle verify: error out if called without an object database

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


base-commit: 8104ec994ea3849a968b4667d072fedd1e688642
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-226%2Fdscho%2Ffix-bundle-verify-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-226/dscho/fix-bundle-verify-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/226
-- 
gitgitgadget

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

[Johannes Schindelin via GitGitGadget]

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.

Reported by Konstantin Ryabitsev.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 bundle.c                | 3 +++
 t/t5607-clone-bundle.sh | 6 ++++++
 2 files changed, 9 insertions(+)

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"));
+
 	repo_init_revisions(r, &revs, NULL);
 	for (i = 0; i < p->nr; i++) {
 		struct ref_list_entry *e = p->list + i;
diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
index cf39e9e243..2a0fb15cf1 100755
--- a/t/t5607-clone-bundle.sh
+++ b/t/t5607-clone-bundle.sh
@@ -14,6 +14,12 @@ test_expect_success 'setup' '
 	git tag -d third
 '
 
+test_expect_success '"verify" needs a worktree' '
+	git bundle create tip.bundle -1 master &&
+	test_must_fail nongit git bundle verify ../tip.bundle 2>err &&
+	test_i18ngrep "need a repository" err
+'
+
 test_expect_success 'annotated tags can be excluded by rev-list options' '
 	git bundle create bundle --all --since=7.Apr.2005.15:14:00.-0700 &&
 	git ls-remote bundle > output &&
-- 
gitgitgadget

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]

On 2019-05-27 at 19:59:14, 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

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…".

> 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 <johannes.schindelin@gmx.de>
> ---
>  bundle.c                | 3 +++
>  t/t5607-clone-bundle.sh | 6 ++++++
>  2 files changed, 9 insertions(+)
> 
> 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"));
> +
>  	repo_init_revisions(r, &revs, NULL);
>  	for (i = 0; i < p->nr; i++) {
>  		struct ref_list_entry *e = p->list + i;
> diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> index cf39e9e243..2a0fb15cf1 100755
> --- a/t/t5607-clone-bundle.sh
> +++ b/t/t5607-clone-bundle.sh
> @@ -14,6 +14,12 @@ test_expect_success 'setup' '
>  	git tag -d third
>  '
>  
> +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.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2302 bytes --]

Hi brian,

On Mon, 27 May 2019, brian m. carlson wrote:

> On 2019-05-27 at 19:59:14, 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
>
> 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…".

Thanks for educating me.

> > 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 <johannes.schindelin@gmx.de>
> > ---
> >  bundle.c                | 3 +++
> >  t/t5607-clone-bundle.sh | 6 ++++++
> >  2 files changed, 9 insertions(+)
> >
> > 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"));
> > +
> >  	repo_init_revisions(r, &revs, NULL);
> >  	for (i = 0; i < p->nr; i++) {
> >  		struct ref_list_entry *e = p->list + i;
> > diff --git a/t/t5607-clone-bundle.sh b/t/t5607-clone-bundle.sh
> > index cf39e9e243..2a0fb15cf1 100755
> > --- a/t/t5607-clone-bundle.sh
> > +++ b/t/t5607-clone-bundle.sh
> > @@ -14,6 +14,12 @@ test_expect_success 'setup' '
> >  	git tag -d third
> >  '
> >
> > +test_expect_success '"verify" needs a worktree' '
>
> Did you want to say "needs a repository"? Or do we really need a
> worktree?

Right, that was a place I meant to change after making up my mind what is
actually required, but forgot.

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

Thank you! v2 incoming.

Ciao,
Dscho

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On 2019-05-27 at 20:21:00, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> 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 <johannes.schindelin@gmx.de>

I think this looks great.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Jeff King]

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

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 3631 bytes --]

Hi Peff,

On Mon, 27 May 2019, Jeff King wrote:

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

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.

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

Hmm. If you really want to ;-)

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

That's more complication than I want to introduce. Let's just go with the
`builtin/bundle.c` patch instead, as you suggested.

Thanks,
Dscho

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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.

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

[Jeff King]

On Tue, May 28, 2019 at 01:17:41PM +0200, Johannes Schindelin wrote:

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

I'm OK with that.

Since the breakage was technically attributable to my commit (I say
technically because flushing out these kinds of bugs was exactly the
point of it), I feel slightly guilty that I might have broken somebody's
oddball workflow. So I tend to err on the side of restoring the original
behavior in that case, even if I can't conceive of it as being
particularly useful. :)

But if nobody is actually in support of it, and given that it has
already been broken for 2 years, I certainly do not mind waiting to see
if anybody screams.

-Peff

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

[Jeff King]

On Tue, May 28, 2019 at 01:03:26PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> 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.

I was thinking this was matters only for "bundle verify", which is not
all that interesting (either you can clone from it, or you cannot).

It is also used as part of unbundle(), which is called from the
transport code. But at that point of a clone, we'd already have created
the new repository we're writing into. I.e., "git clone .../foo.bundle"
still works fine either way.

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Yeah, I tend to agree that 'bundle verify' is pointless before
trying to clone from it and seeing it fail (it still would be useful
if 'bundle verify' gives more info on the breakage than 'clone', but
it is not all that interesting at that point unless the user is
working on git itself).