| Message ID | 20200623205659.14297-1-mforney@mforney.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ea3f7e598c8f1171f0bd02da777dc526cdb8424d |
| Headers | show |
| Series | [1/2] revision: use repository from rev_info when parsing commits | expand |
On Tue, Jun 23, 2020 at 5:02 PM Michael Forney <mforney@mforney.org> wrote: > This is needed when repo_init_revisions() is called with a repository > that is not the_repository to ensure appropriate repository is used > in repo_parse_commit_internal(). If the wrong repository is used, > a fatal error is the commit-graph machinery occurs: s/is/in/ > fatal: invalid commit position. commit-graph is likely corrupt > > Since revision.c was the only user of the parse_commit_gently > compatibility define, remove it from commit.h. > > Signed-off-by: Michael Forney <mforney@mforney.org>
On 6/23/2020 4:56 PM, Michael Forney wrote: > This is needed when repo_init_revisions() is called with a repository > that is not the_repository to ensure appropriate repository is used > in repo_parse_commit_internal(). If the wrong repository is used, > a fatal error is the commit-graph machinery occurs: > > fatal: invalid commit position. commit-graph is likely corrupt > > Since revision.c was the only user of the parse_commit_gently > compatibility define, remove it from commit.h. Is this demonstrable in a test case, to prevent regressions? Notably, you are _not_ dropping parse_commit(), and it would be easy for another call to that shim to slip into revision.c. > Signed-off-by: Michael Forney <mforney@mforney.org> > --- > commit.h | 1 - > revision.c | 18 +++++++++--------- > 2 files changed, 9 insertions(+), 10 deletions(-) > > diff --git a/commit.h b/commit.h > -#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet) I'm happy we can drop this shim! > diff --git a/revision.c b/revision.c > index ebb4d2a0f2..2b6bf47c81 100644 > --- a/revision.c > +++ b/revision.c > @@ -439,7 +439,7 @@ static struct commit *handle_commit(struct rev_info *revs, > if (object->type == OBJ_COMMIT) { > struct commit *commit = (struct commit *)object; > > - if (parse_commit(commit) < 0) > + if (repo_parse_commit(revs->repo, commit) < 0) I counted 9 copies of parse_commit[_gently]() in my version of revision.c, so it looks like you caught them all. Thanks! -Stolee
Derrick Stolee <stolee@gmail.com> writes: > On 6/23/2020 4:56 PM, Michael Forney wrote: >> This is needed when repo_init_revisions() is called with a repository >> that is not the_repository to ensure appropriate repository is used >> in repo_parse_commit_internal(). If the wrong repository is used, >> a fatal error is the commit-graph machinery occurs: >> >> fatal: invalid commit position. commit-graph is likely corrupt >> >> Since revision.c was the only user of the parse_commit_gently >> compatibility define, remove it from commit.h. > > Is this demonstrable in a test case, to prevent regressions? It appears that Michael tried and failed. Even if we do not currently have a caller that asks these functions in revision.c to work on a repository that is not the primary one (i.e. in a submodule), in which case these patches may not be fixing any bug that can be triggered in the current code, it is quite obvious that these functions misbehave once a caller starts asking them to work on a repository other than the primary one. So, given that ... > > I counted 9 copies of parse_commit[_gently]() in my version > of revision.c, so it looks like you caught them all. ... we should be able to proceed with the code as-is, I guess. Thanks.
On 9/3/2020 5:58 PM, Junio C Hamano wrote: > Derrick Stolee <stolee@gmail.com> writes: > >> On 6/23/2020 4:56 PM, Michael Forney wrote: >>> This is needed when repo_init_revisions() is called with a repository >>> that is not the_repository to ensure appropriate repository is used >>> in repo_parse_commit_internal(). If the wrong repository is used, >>> a fatal error is the commit-graph machinery occurs: >>> >>> fatal: invalid commit position. commit-graph is likely corrupt >>> >>> Since revision.c was the only user of the parse_commit_gently >>> compatibility define, remove it from commit.h. >> >> Is this demonstrable in a test case, to prevent regressions? > > It appears that Michael tried and failed. Even if we do not > currently have a caller that asks these functions in revision.c to > work on a repository that is not the primary one (i.e. in a > submodule), in which case these patches may not be fixing any bug > that can be triggered in the current code, it is quite obvious that > these functions misbehave once a caller starts asking them to work > on a repository other than the primary one. > > So, given that ... > >> >> I counted 9 copies of parse_commit[_gently]() in my version >> of revision.c, so it looks like you caught them all. > > ... we should be able to proceed with the code as-is, I guess Yes, I think this is an improvement regardless. Thanks, for the reminder. -Stolee
diff --git a/commit.h b/commit.h index 1b2dea5d85..a2e8ca99a2 100644 --- a/commit.h +++ b/commit.h @@ -97,7 +97,6 @@ static inline int parse_commit_no_graph(struct commit *commit) #ifndef NO_THE_REPOSITORY_COMPATIBILITY_MACROS #define parse_commit_internal(item, quiet, use) repo_parse_commit_internal(the_repository, item, quiet, use) -#define parse_commit_gently(item, quiet) repo_parse_commit_gently(the_repository, item, quiet) #define parse_commit(item) repo_parse_commit(the_repository, item) #endif diff --git a/revision.c b/revision.c index ebb4d2a0f2..2b6bf47c81 100644 --- a/revision.c +++ b/revision.c @@ -439,7 +439,7 @@ static struct commit *handle_commit(struct rev_info *revs, if (object->type == OBJ_COMMIT) { struct commit *commit = (struct commit *)object; - if (parse_commit(commit) < 0) + if (repo_parse_commit(revs->repo, commit) < 0) die("unable to parse commit %s", name); if (flags & UNINTERESTING) { mark_parents_uninteresting(commit); @@ -992,7 +992,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) ts->treesame[0] = 1; } } - if (parse_commit(p) < 0) + if (repo_parse_commit(revs->repo, p) < 0) die("cannot simplify commit %s (because of %s)", oid_to_hex(&commit->object.oid), oid_to_hex(&p->object.oid)); @@ -1037,7 +1037,7 @@ static void try_to_simplify_commit(struct rev_info *revs, struct commit *commit) * IOW, we pretend this parent is a * "root" commit. */ - if (parse_commit(p) < 0) + if (repo_parse_commit(revs->repo, p) < 0) die("cannot simplify commit %s (invalid %s)", oid_to_hex(&commit->object.oid), oid_to_hex(&p->object.oid)); @@ -1105,7 +1105,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit, parent = parent->next; if (p) p->object.flags |= UNINTERESTING; - if (parse_commit_gently(p, 1) < 0) + if (repo_parse_commit_gently(revs->repo, p, 1) < 0) continue; if (p->parents) mark_parents_uninteresting(p); @@ -1136,7 +1136,7 @@ static int process_parents(struct rev_info *revs, struct commit *commit, struct commit *p = parent->item; int gently = revs->ignore_missing_links || revs->exclude_promisor_objects; - if (parse_commit_gently(p, gently) < 0) { + if (repo_parse_commit_gently(revs->repo, p, gently) < 0) { if (revs->exclude_promisor_objects && is_promisor_object(&p->object.oid)) { if (revs->first_parent_only) @@ -3295,7 +3295,7 @@ static void explore_walk_step(struct rev_info *revs) if (!c) return; - if (parse_commit_gently(c, 1) < 0) + if (repo_parse_commit_gently(revs->repo, c, 1) < 0) return; if (revs->sort_order == REV_SORT_BY_AUTHOR_DATE) @@ -3333,7 +3333,7 @@ static void indegree_walk_step(struct rev_info *revs) if (!c) return; - if (parse_commit_gently(c, 1) < 0) + if (repo_parse_commit_gently(revs->repo, c, 1) < 0) return; explore_to_depth(revs, c->generation); @@ -3414,7 +3414,7 @@ static void init_topo_walk(struct rev_info *revs) for (list = revs->commits; list; list = list->next) { struct commit *c = list->item; - if (parse_commit_gently(c, 1)) + if (repo_parse_commit_gently(revs->repo, c, 1)) continue; test_flag_and_insert(&info->explore_queue, c, TOPO_WALK_EXPLORED); @@ -3476,7 +3476,7 @@ static void expand_topo_walk(struct rev_info *revs, struct commit *commit) if (parent->object.flags & UNINTERESTING) continue; - if (parse_commit_gently(parent, 1) < 0) + if (repo_parse_commit_gently(revs->repo, parent, 1) < 0) continue; if (parent->generation < info->min_generation) {
This is needed when repo_init_revisions() is called with a repository that is not the_repository to ensure appropriate repository is used in repo_parse_commit_internal(). If the wrong repository is used, a fatal error is the commit-graph machinery occurs: fatal: invalid commit position. commit-graph is likely corrupt Since revision.c was the only user of the parse_commit_gently compatibility define, remove it from commit.h. Signed-off-by: Michael Forney <mforney@mforney.org> --- commit.h | 1 - revision.c | 18 +++++++++--------- 2 files changed, 9 insertions(+), 10 deletions(-)