[1/2] revision: use repository from rev_info when parsing commits
diff mbox series

Message ID 20200623205659.14297-1-mforney@mforney.org
State New
Headers show
Series
  • [1/2] revision: use repository from rev_info when parsing commits
Related show

Commit Message

Michael Forney June 23, 2020, 8:56 p.m. UTC
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(-)

Comments

Eric Sunshine June 23, 2020, 9:24 p.m. UTC | #1
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>
Derrick Stolee June 24, 2020, 2:29 p.m. UTC | #2
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

Patch
diff mbox series

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