bundle verify: improve the user experience when called without a .git/ directory
cover bundle verify: improve the user experience when called without a .git/ directory
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
Signature · 1 line
gitgitgadget
PATCH 1/1 bundle verify: error out if called without an object database
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.
--- bundle.c | 3 +++ t/t5607-clone-bundle.sh | 6 ++++++ 2 files changed, 9 insertions(+)
bundle.c +3 -0
@@ -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 +6 -0
@@ -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 &&
Signature · 1 line
gitgitgadget
Re: [PATCH 1/1] bundle verify: error out if called without an object database
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 33 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 verifyis 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@@ -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
t/t5607-clone-bundle.sh test_expect_success 'setup' ' · line 17
Quoted 4 lines
t/t5607-clone-bundle.sh +1
-0@@ -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.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204
Re: [PATCH 1/1] bundle verify: error out if called without an object database
Hi brian,
Quoted 8 lines · brian m. carlson
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…".
Thanks for educating me.
Quoted 36 lines
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 verifyis 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@@ -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@@ -14,6 +14,12 @@ test_expect_success 'setup' ' git tag -d third '
Quoted 4 lines
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?
Right, that was a place I meant to change after making up my mind what is actually required, but forgot.
Quoted 2 lines
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 1/1] bundle verify: error out if called without an object database
In reply to Johannes Schindelin via GitGitGadgetQuoted 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 verifyis 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@@ -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
Hi Peff,
Quoted 20 lines · Jeff King
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 verifyis 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:
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 3 lines
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 ;-)
Quoted 29 lines
Likewise:
bundle.c int verify_bundle(struct repository *r, · line 146
Quoted 10 lines
bundle.c +2
-0@@ -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 ofverify_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->objectsandr->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.
Quoted 5 lines
Instead, could we have
cmd_bundle()pass in NULL when instead ofthe_repositorywhen 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
Quoted 13 lines · Johannes Schindelin
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.
Quoted 6 lines
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
In reply to Jeff KingQuoted 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.
Re: [PATCH 1/1] bundle verify: error out if called without an object database
Quoted 19 lines · Junio C Hamano
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.
Quoted 8 lines
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
Quoted 9 lines · Jeff King
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.bundleThat 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).
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): 8467593c158ffac56897cf02e165173d9c0b5880.1558988458.git.gitgitgadget@gmail.com
Re: [PATCH v2 1/1] bundle verify: error out if called without an object database
Quoted 7 lines · Johannes Schindelin via GitGitGadget
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.
Quoted 6 lines
Let's error out with a useful error message if
git bundle verifyis called without such an object database to work with.Reported by Konstantin Ryabitsev.
Signed-off-by Johannes Schindelin <[email protected]>
I think this looks great.
Signature · 2 lines
brian m. carlson: Houston, Texas, US OpenPGP: https://keybase.io/bk2204