[v4,5/7] commit/revisions: bookkeeping before refactoring
diff mbox series

Message ID f3e291665dae94b7347621ec78721f7e0dcc86d8.1539729393.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • Use generation numbers for --topo-order
Related show

Commit Message

Elijah Newren via GitGitGadget Oct. 16, 2018, 10:36 p.m. UTC
From: Derrick Stolee <dstolee@microsoft.com>

There are a few things that need to move around a little before
making a big refactoring in the topo-order logic:

1. We need access to record_author_date() and
   compare_commits_by_author_date() in revision.c. These are used
   currently by sort_in_topological_order() in commit.c.

2. Moving these methods to commit.h requires adding the author_slab
   definition to commit.h.

3. The add_parents_to_list() method in revision.c performs logic
   around the UNINTERESTING flag and other special cases depending
   on the struct rev_info. Allow this method to ignore a NULL 'list'
   parameter, as we will not be populating the list for our walk.
   Also rename the method to the slightly more generic name
   process_parents() to make clear that this method does more than
   add to a list (and no list is required anymore).

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 commit.c   | 11 +++++------
 commit.h   |  8 ++++++++
 revision.c | 18 ++++++++++--------
 3 files changed, 23 insertions(+), 14 deletions(-)

Comments

Jakub Narębski Oct. 21, 2018, 9:17 p.m. UTC | #1
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Derrick Stolee <dstolee@microsoft.com>
>
> There are a few things that need to move around a little before
> making a big refactoring in the topo-order logic:
>
> 1. We need access to record_author_date() and
>    compare_commits_by_author_date() in revision.c. These are used
>    currently by sort_in_topological_order() in commit.c.
>
> 2. Moving these methods to commit.h requires adding the author_slab
>    definition to commit.h.

Those two changes are connected, and must be kept together.

> 3. The add_parents_to_list() method in revision.c performs logic
>    around the UNINTERESTING flag and other special cases depending
>    on the struct rev_info. Allow this method to ignore a NULL 'list'
>    parameter, as we will not be populating the list for our walk.
>    Also rename the method to the slightly more generic name
>    process_parents() to make clear that this method does more than
>    add to a list (and no list is required anymore).

But as far as I can understand, this change is independent, and it could
be put into a separate commmit.

The change of function name to process_parents() and allowing for 'list'
parameter to be NULL are related, though.

>
> Helped-by: Jeff King <peff@peff.net>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>

No need to split, unless there would be v5 anyway, in my opinion.

> ---
>  commit.c   | 11 +++++------
>  commit.h   |  8 ++++++++
>  revision.c | 18 ++++++++++--------
>  3 files changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index d0f199e122..861a485e93 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -655,11 +655,10 @@ struct commit *pop_commit(struct commit_list **stack)
>  /* count number of children that have not been emitted */
>  define_commit_slab(indegree_slab, int);
>  
> -/* record author-date for each commit object */
> -define_commit_slab(author_date_slab, timestamp_t);
> +implement_shared_commit_slab(author_date_slab, timestamp_t);

I see that the comment got moved to the site with
define_shared_commit_slab(), i.e. to commit.h, instead of duplicting
it.  All right.

Sidenote: Ugh, small_caps preprocessor macros [trickery].

>  
> -static void record_author_date(struct author_date_slab *author_date,
> -			       struct commit *commit)
> +void record_author_date(struct author_date_slab *author_date,
> +			struct commit *commit)
>  {
>  	const char *buffer = get_commit_buffer(commit, NULL);
>  	struct ident_split ident;
> @@ -684,8 +683,8 @@ fail_exit:
>  	unuse_commit_buffer(commit, buffer);
>  }
>  
> -static int compare_commits_by_author_date(const void *a_, const void *b_,
> -					  void *cb_data)
> +int compare_commits_by_author_date(const void *a_, const void *b_,
> +				   void *cb_data)

All right, this is straighforward changing record_author_date() and
compare_commits_by_author_date() from static (file-local) functions to
exported functions.

>  {
>  	const struct commit *a = a_, *b = b_;
>  	struct author_date_slab *author_date = cb_data;
> diff --git a/commit.h b/commit.h
> index 2b1a734388..977d397356 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -8,6 +8,7 @@
>  #include "gpg-interface.h"
>  #include "string-list.h"
>  #include "pretty.h"
> +#include "commit-slab.h"
>  
>  #define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
>  #define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
> @@ -328,6 +329,13 @@ extern int remove_signature(struct strbuf *buf);
>   */
>  extern int check_commit_signature(const struct commit *commit, struct signature_check *sigc);
>  
> +/* record author-date for each commit object */
> +define_shared_commit_slab(author_date_slab, timestamp_t);

All right, this is needed for record_author_date() function, which is
now exported.

> +
> +void record_author_date(struct author_date_slab *author_date,
> +			struct commit *commit);
> +
> +int compare_commits_by_author_date(const void *a_, const void *b_, void *unused);

O.K., this is simply exporting previously static functions.

>  int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
>  int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
>  
> diff --git a/revision.c b/revision.c
> index 2dcde8a8ac..36458265a0 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -768,8 +768,8 @@ static void commit_list_insert_by_date_cached(struct commit *p, struct commit_li
>  		*cache = new_entry;
>  }
>  
> -static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
> -		    struct commit_list **list, struct commit_list **cache_ptr)
> +static int process_parents(struct rev_info *revs, struct commit *commit,
> +			   struct commit_list **list, struct commit_list **cache_ptr)

All right, straighforward rename.

>  {
>  	struct commit_list *parent = commit->parents;
>  	unsigned left_flag;
> @@ -808,7 +808,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
>  			if (p->object.flags & SEEN)
>  				continue;
>  			p->object.flags |= SEEN;
> -			commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
> +			if (list)
> +				commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
>  		}
>  		return 0;
>  	}
> @@ -847,7 +848,8 @@ static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
>  		p->object.flags |= left_flag;
>  		if (!(p->object.flags & SEEN)) {
>  			p->object.flags |= SEEN;
> -			commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
> +			if (list)
> +				commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);

All right, both of those is about allowing 'list' parameter to be NULL,
and invoking commit_list_insert_by_date_cached() only if it's not NULL.

>  		}
>  		if (revs->first_parent_only)
>  			break;
> @@ -1091,7 +1093,7 @@ static int limit_list(struct rev_info *revs)
>  
>  		if (revs->max_age != -1 && (commit->date < revs->max_age))
>  			obj->flags |= UNINTERESTING;
> -		if (add_parents_to_list(revs, commit, &list, NULL) < 0)
> +		if (process_parents(revs, commit, &list, NULL) < 0)
>  			return -1;
>  		if (obj->flags & UNINTERESTING) {
>  			mark_parents_uninteresting(commit);
> @@ -2913,7 +2915,7 @@ static struct commit *next_topo_commit(struct rev_info *revs)
>  
>  static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
>  {
> -	if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {
> +	if (process_parents(revs, commit, &revs->commits, NULL) < 0) {
>  		if (!revs->ignore_missing_links)
>  			die("Failed to traverse parents of commit %s",
>  			    oid_to_hex(&commit->object.oid));
> @@ -2979,7 +2981,7 @@ static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
>  	for (;;) {
>  		struct commit *p = *pp;
>  		if (!revs->limited)
> -			if (add_parents_to_list(revs, p, &revs->commits, &cache) < 0)
> +			if (process_parents(revs, p, &revs->commits, &cache) < 0)
>  				return rewrite_one_error;
>  		if (p->object.flags & UNINTERESTING)
>  			return rewrite_one_ok;
> @@ -3312,7 +3314,7 @@ static struct commit *get_revision_1(struct rev_info *revs)
>  				try_to_simplify_commit(revs, commit);
>  			else if (revs->topo_walk_info)
>  				expand_topo_walk(revs, commit);
> -			else if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {
> +			else if (process_parents(revs, commit, &revs->commits, NULL) < 0) {
>  				if (!revs->ignore_missing_links)
>  					die("Failed to traverse parents of commit %s",
>  						oid_to_hex(&commit->object.oid));

All those is just changing the calling convention due to function
rename.

(I wonder if such simple refactoring could have been done via Coccinelle
patch).


Anyway, looks good to me.
--
Jakub Narębski

Patch
diff mbox series

diff --git a/commit.c b/commit.c
index d0f199e122..861a485e93 100644
--- a/commit.c
+++ b/commit.c
@@ -655,11 +655,10 @@  struct commit *pop_commit(struct commit_list **stack)
 /* count number of children that have not been emitted */
 define_commit_slab(indegree_slab, int);
 
-/* record author-date for each commit object */
-define_commit_slab(author_date_slab, timestamp_t);
+implement_shared_commit_slab(author_date_slab, timestamp_t);
 
-static void record_author_date(struct author_date_slab *author_date,
-			       struct commit *commit)
+void record_author_date(struct author_date_slab *author_date,
+			struct commit *commit)
 {
 	const char *buffer = get_commit_buffer(commit, NULL);
 	struct ident_split ident;
@@ -684,8 +683,8 @@  fail_exit:
 	unuse_commit_buffer(commit, buffer);
 }
 
-static int compare_commits_by_author_date(const void *a_, const void *b_,
-					  void *cb_data)
+int compare_commits_by_author_date(const void *a_, const void *b_,
+				   void *cb_data)
 {
 	const struct commit *a = a_, *b = b_;
 	struct author_date_slab *author_date = cb_data;
diff --git a/commit.h b/commit.h
index 2b1a734388..977d397356 100644
--- a/commit.h
+++ b/commit.h
@@ -8,6 +8,7 @@ 
 #include "gpg-interface.h"
 #include "string-list.h"
 #include "pretty.h"
+#include "commit-slab.h"
 
 #define COMMIT_NOT_FROM_GRAPH 0xFFFFFFFF
 #define GENERATION_NUMBER_INFINITY 0xFFFFFFFF
@@ -328,6 +329,13 @@  extern int remove_signature(struct strbuf *buf);
  */
 extern int check_commit_signature(const struct commit *commit, struct signature_check *sigc);
 
+/* record author-date for each commit object */
+define_shared_commit_slab(author_date_slab, timestamp_t);
+
+void record_author_date(struct author_date_slab *author_date,
+			struct commit *commit);
+
+int compare_commits_by_author_date(const void *a_, const void *b_, void *unused);
 int compare_commits_by_commit_date(const void *a_, const void *b_, void *unused);
 int compare_commits_by_gen_then_commit_date(const void *a_, const void *b_, void *unused);
 
diff --git a/revision.c b/revision.c
index 2dcde8a8ac..36458265a0 100644
--- a/revision.c
+++ b/revision.c
@@ -768,8 +768,8 @@  static void commit_list_insert_by_date_cached(struct commit *p, struct commit_li
 		*cache = new_entry;
 }
 
-static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
-		    struct commit_list **list, struct commit_list **cache_ptr)
+static int process_parents(struct rev_info *revs, struct commit *commit,
+			   struct commit_list **list, struct commit_list **cache_ptr)
 {
 	struct commit_list *parent = commit->parents;
 	unsigned left_flag;
@@ -808,7 +808,8 @@  static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 			if (p->object.flags & SEEN)
 				continue;
 			p->object.flags |= SEEN;
-			commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
+			if (list)
+				commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
 		}
 		return 0;
 	}
@@ -847,7 +848,8 @@  static int add_parents_to_list(struct rev_info *revs, struct commit *commit,
 		p->object.flags |= left_flag;
 		if (!(p->object.flags & SEEN)) {
 			p->object.flags |= SEEN;
-			commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
+			if (list)
+				commit_list_insert_by_date_cached(p, list, cached_base, cache_ptr);
 		}
 		if (revs->first_parent_only)
 			break;
@@ -1091,7 +1093,7 @@  static int limit_list(struct rev_info *revs)
 
 		if (revs->max_age != -1 && (commit->date < revs->max_age))
 			obj->flags |= UNINTERESTING;
-		if (add_parents_to_list(revs, commit, &list, NULL) < 0)
+		if (process_parents(revs, commit, &list, NULL) < 0)
 			return -1;
 		if (obj->flags & UNINTERESTING) {
 			mark_parents_uninteresting(commit);
@@ -2913,7 +2915,7 @@  static struct commit *next_topo_commit(struct rev_info *revs)
 
 static void expand_topo_walk(struct rev_info *revs, struct commit *commit)
 {
-	if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {
+	if (process_parents(revs, commit, &revs->commits, NULL) < 0) {
 		if (!revs->ignore_missing_links)
 			die("Failed to traverse parents of commit %s",
 			    oid_to_hex(&commit->object.oid));
@@ -2979,7 +2981,7 @@  static enum rewrite_result rewrite_one(struct rev_info *revs, struct commit **pp
 	for (;;) {
 		struct commit *p = *pp;
 		if (!revs->limited)
-			if (add_parents_to_list(revs, p, &revs->commits, &cache) < 0)
+			if (process_parents(revs, p, &revs->commits, &cache) < 0)
 				return rewrite_one_error;
 		if (p->object.flags & UNINTERESTING)
 			return rewrite_one_ok;
@@ -3312,7 +3314,7 @@  static struct commit *get_revision_1(struct rev_info *revs)
 				try_to_simplify_commit(revs, commit);
 			else if (revs->topo_walk_info)
 				expand_topo_walk(revs, commit);
-			else if (add_parents_to_list(revs, commit, &revs->commits, NULL) < 0) {
+			else if (process_parents(revs, commit, &revs->commits, NULL) < 0) {
 				if (!revs->ignore_missing_links)
 					die("Failed to traverse parents of commit %s",
 						oid_to_hex(&commit->object.oid));