Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits
On 09/12, Jonathan Nieder wrote: > From: Stefan Beller > > The check_has_commit helper uses resolves a submodule entry to a > commit, when validating its existence. As a side effect this means > tolerates a submodule entry pointing to a tag, which is not a valid > submodule entry that git commands would know how to cope with. > > Tighten the check to require an actual commit, not a tag pointing to a > commit. > > Also improve the error handling when a submodule entry points to > non-commit (e.g., a blob) to error out instead of warning and > pretending the pointed to object doesn't exist. > > Signed-off-by: Stefan Beller > Signed-off-by: Jonathan Nieder Looks good! > --- > submodule.c| 33 + > t/t5531-deep-submodule-push.sh | 10 ++ > 2 files changed, 35 insertions(+), 8 deletions(-) > > diff --git a/submodule.c b/submodule.c > index 3cea8221e0..e0da55920d 100644 > --- a/submodule.c > +++ b/submodule.c > @@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id > *oid, void *data) > return 0; > } > > +struct has_commit_data { > + int result; > + const char *path; > +}; > + > static int check_has_commit(const struct object_id *oid, void *data) > { > - int *has_commit = data; > + struct has_commit_data *cb = data; > > - if (!lookup_commit_reference(oid)) > - *has_commit = 0; > + enum object_type type = sha1_object_info(oid->hash, NULL); > > - return 0; > + switch (type) { > + case OBJ_COMMIT: > + return 0; > + case OBJ_BAD: > + /* > + * Object is missing or invalid. If invalid, an error message > + * has already been printed. > + */ > + cb->result = 0; > + return 0; > + default: > + die(_("submodule entry '%s' (%s) is a %s, not a commit"), > + cb->path, oid_to_hex(oid), typename(type)); > + } > } > > static int submodule_has_commits(const char *path, struct oid_array *commits) > { > - int has_commit = 1; > + struct has_commit_data has_commit = { 1, path }; > > /* >* Perform a cheap, but incorrect check for the existence of 'commits'. > @@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct > oid_array *commits) > > oid_array_for_each_unique(commits, check_has_commit, &has_commit); > > - if (has_commit) { > + if (has_commit.result) { > /* >* Even if the submodule is checked out and the commit is >* present, make sure it exists in the submodule's object store > @@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, > struct oid_array *commits) > cp.dir = path; > > if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len) > - has_commit = 0; > + has_commit.result = 0; > > strbuf_release(&out); > } > > - return has_commit; > + return has_commit.result; > } > > static int submodule_needs_pushing(const char *path, struct oid_array > *commits) > diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh > index 0f84a53146..39cb2c1c34 100755 > --- a/t/t5531-deep-submodule-push.sh > +++ b/t/t5531-deep-submodule-push.sh > @@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit > disabling recursion from > ) > ' > > +test_expect_success 'submodule entry pointing at a tag is error' ' > + git -C work/gar/bage tag -a test1 -m "tag" && > + tag=$(git -C work/gar/bage rev-parse test1^{tag}) && > + git -C work update-index --cacheinfo 16 "$tag" gar/bage && > + git -C work commit -m "bad commit" && > + test_when_finished "git -C work reset --hard HEAD^" && > + test_must_fail git -C work push --recurse-submodules=on-demand > ../pub.git master 2>err && > + test_i18ngrep "is a tag, not a commit" err > +' > + > test_expect_success 'push fails if recurse submodules option passed as yes' ' > ( > cd work/gar/bage && > -- > 2.14.1.690.gbb1197296e > -- Brandon Williams
Re: [PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits
On Tue, Sep 12, 2017 at 10:30:27AM -0700, Jonathan Nieder wrote: > From: Stefan Beller > > The check_has_commit helper uses resolves a submodule entry to a > commit, when validating its existence. As a side effect this means > tolerates a submodule entry pointing to a tag, which is not a valid > submodule entry that git commands would know how to cope with. > > Tighten the check to require an actual commit, not a tag pointing to a > commit. > > Also improve the error handling when a submodule entry points to > non-commit (e.g., a blob) to error out instead of warning and > pretending the pointed to object doesn't exist. > > Signed-off-by: Stefan Beller > Signed-off-by: Jonathan Nieder Looks good to me. Cheers Heiko
[PATCH 2/4] push, fetch: error out for submodule entries not pointing to commits
From: Stefan Beller The check_has_commit helper uses resolves a submodule entry to a commit, when validating its existence. As a side effect this means tolerates a submodule entry pointing to a tag, which is not a valid submodule entry that git commands would know how to cope with. Tighten the check to require an actual commit, not a tag pointing to a commit. Also improve the error handling when a submodule entry points to non-commit (e.g., a blob) to error out instead of warning and pretending the pointed to object doesn't exist. Signed-off-by: Stefan Beller Signed-off-by: Jonathan Nieder --- submodule.c| 33 + t/t5531-deep-submodule-push.sh | 10 ++ 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/submodule.c b/submodule.c index 3cea8221e0..e0da55920d 100644 --- a/submodule.c +++ b/submodule.c @@ -767,19 +767,36 @@ static int append_oid_to_argv(const struct object_id *oid, void *data) return 0; } +struct has_commit_data { + int result; + const char *path; +}; + static int check_has_commit(const struct object_id *oid, void *data) { - int *has_commit = data; + struct has_commit_data *cb = data; - if (!lookup_commit_reference(oid)) - *has_commit = 0; + enum object_type type = sha1_object_info(oid->hash, NULL); - return 0; + switch (type) { + case OBJ_COMMIT: + return 0; + case OBJ_BAD: + /* +* Object is missing or invalid. If invalid, an error message +* has already been printed. +*/ + cb->result = 0; + return 0; + default: + die(_("submodule entry '%s' (%s) is a %s, not a commit"), + cb->path, oid_to_hex(oid), typename(type)); + } } static int submodule_has_commits(const char *path, struct oid_array *commits) { - int has_commit = 1; + struct has_commit_data has_commit = { 1, path }; /* * Perform a cheap, but incorrect check for the existence of 'commits'. @@ -795,7 +812,7 @@ static int submodule_has_commits(const char *path, struct oid_array *commits) oid_array_for_each_unique(commits, check_has_commit, &has_commit); - if (has_commit) { + if (has_commit.result) { /* * Even if the submodule is checked out and the commit is * present, make sure it exists in the submodule's object store @@ -814,12 +831,12 @@ static int submodule_has_commits(const char *path, struct oid_array *commits) cp.dir = path; if (capture_command(&cp, &out, GIT_MAX_HEXSZ + 1) || out.len) - has_commit = 0; + has_commit.result = 0; strbuf_release(&out); } - return has_commit; + return has_commit.result; } static int submodule_needs_pushing(const char *path, struct oid_array *commits) diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh index 0f84a53146..39cb2c1c34 100755 --- a/t/t5531-deep-submodule-push.sh +++ b/t/t5531-deep-submodule-push.sh @@ -298,6 +298,16 @@ test_expect_success 'push succeeds if submodule commit disabling recursion from ) ' +test_expect_success 'submodule entry pointing at a tag is error' ' + git -C work/gar/bage tag -a test1 -m "tag" && + tag=$(git -C work/gar/bage rev-parse test1^{tag}) && + git -C work update-index --cacheinfo 16 "$tag" gar/bage && + git -C work commit -m "bad commit" && + test_when_finished "git -C work reset --hard HEAD^" && + test_must_fail git -C work push --recurse-submodules=on-demand ../pub.git master 2>err && + test_i18ngrep "is a tag, not a commit" err +' + test_expect_success 'push fails if recurse submodules option passed as yes' ' ( cd work/gar/bage && -- 2.14.1.690.gbb1197296e