| Message ID | e86f30408240da12a52858f836134b037e85f7ae.1537551564.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use generation numbers for --topo-order | expand |
On Fri, Sep 21, 2018 at 10:39:33AM -0700, Derrick Stolee via GitGitGadget wrote: > 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. The overall goal makes sense. Do we really need to define the whole slab in the header file? We're going to end up with multiple copies of the functions, since they're declared static in each file that includes commit.h. From what's here, I think you could get away with just: struct author_date_slab; void record_author_date(struct author_date_slab *author_date, struct commit *commit); in the header file. But presumably callers would eventually want to allocate their own author dates. If that's all we need, then these days you can do: declare_commit_slab(author_date, timestamp_t); to get the type declaration. If they really do need the functions accessible outside of commit.c, then perhaps: define_shared_commit_slab(author_date, timestamp_t); in commit.h, and: implement_shared_commit_slab(author_date, timestamp_t); in commit.c (the type repetition is not too bad, as the compiler would catch any mistakes). The only downside of this approach is that we're less likely to be able to inline element access (though "peek" is big enough that I'm not sure it ends up inlined anyway). > 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. So now you can add_parents_to_list() without a list? That sounds confusing. :) Is it possible to split the function into two? Some handle_uninteresting_parents() logic, and then an add_parents_to_list() that calls that, but also adds to the list? A cursory look at the function suggests it's actually kind of tricky. Perhaps as an alternative, add_parents_to_list() could just get a more descriptive name? > --- > commit.c | 11 ++++------- > commit.h | 8 ++++++++ > revision.c | 6 ++++-- > 3 files changed, 16 insertions(+), 9 deletions(-) The patch itself seems straight-forward based on those explanations. -Peff
diff --git a/commit.c b/commit.c index d0f199e122..f68e04b2f1 100644 --- a/commit.c +++ b/commit.c @@ -655,11 +655,8 @@ 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); - -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 +681,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..ff0eb5f8ef 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_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..92012d5f45 100644 --- a/revision.c +++ b/revision.c @@ -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;