Re: [PATCH v4 5/7] commit/revisions: bookkeeping before refactoring

From: Jakub Narebski <jnareb@gmail.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
	Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v4 5/7] commit/revisions: bookkeeping before refactoring
Date: Sun, 21 Oct 2018 23:17:21 +0200	[thread overview]
Message-ID: <865zxvgftq.fsf@gmail.com> (raw)
In-Reply-To: <f3e291665dae94b7347621ec78721f7e0dcc86d8.1539729393.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Tue, 16 Oct 2018 15:36:43 -0700 (PDT)")

"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