On Mon, Jul 16, 2018 at 6:00 AM Derrick Stolee via GitGitGadget <gitgitgad...@gmail.com> wrote: > > From: Derrick Stolee <dsto...@microsoft.com> > > Signed-off-by: Derrick Stolee <dsto...@microsoft.com>
This looks good, apart from nits below. Thanks, Stefan > diff --git a/commit-reach.c b/commit-reach.c > new file mode 100644 > index 000000000..f2e2f7461 > --- /dev/null > +++ b/commit-reach.c > @@ -0,0 +1,359 @@ > +#include "cache.h" > +#include "prio-queue.h" > +#include "commit-reach.h" and commit.h (see discussion below) ? > diff --git a/commit-reach.h b/commit-reach.h > new file mode 100644 > index 000000000..244f48c5f > --- /dev/null > +++ b/commit-reach.h > @@ -0,0 +1,41 @@ > +#ifndef __COMMIT_REACH_H__ > +#define __COMMIT_REACH_H__ > + > +#include "commit.h" Do we really need to include the header file in another header file? I'd think forward declarations would work fine here? (The benefit of forward declaring the structs commits, commit_list is purely for the poor saps of developers that we are, as then touching commit.h would not trigger a compilation of files that only include this header but not commit.h. That are not many in this particular case, but I consider it good practice that we should follow) > + > +struct commit_list *get_merge_bases_many(struct commit *one, > + int n, > + struct commit **twos); > +struct commit_list *get_merge_bases_many_dirty(struct commit *one, > + int n, > + struct commit **twos); > +struct commit_list *get_merge_bases(struct commit *one, struct commit *two); > +struct commit_list *get_octopus_merge_bases(struct commit_list *in); > + > +/* To be used only when object flags after this call no longer matter */ > +struct commit_list *get_merge_bases_many_dirty(struct commit *one, int n, > struct commit **twos); > + > +int is_descendant_of(struct commit *commit, struct commit_list *with_commit); > +int in_merge_bases_many(struct commit *commit, int nr_reference, struct > commit **reference); > +int in_merge_bases(struct commit *commit, struct commit *reference); > + > + > +/* > + * Takes a list of commits and returns a new list where those > + * have been removed that can be reached from other commits in > + * the list. It is useful for, e.g., reducing the commits > + * randomly thrown at the git-merge command and removing > + * redundant commits that the user shouldn't have given to it. > + * > + * This function destroys the STALE bit of the commit objects' > + * flags. > + */ > +struct commit_list *reduce_heads(struct commit_list *heads); > + > +/* > + * Like `reduce_heads()`, except it replaces the list. Use this > + * instead of `foo = reduce_heads(foo);` to avoid memory leaks. > + */ > +void reduce_heads_replace(struct commit_list **heads); Thanks for the docs! Bonus points for also documenting the other functions (is_descendant_of etc. For example is is_descendant_of destroying some bit state?) > +#endif > diff --git a/commit.c b/commit.c > index 39b80bd21..32d1234bd 100644 > --- a/commit.c > +++ b/commit.c > @@ -843,364 +843,6 @@ void sort_in_topological_order(struct commit_list > **list, enum rev_sort_order so > clear_author_date_slab(&author_date); > } > > -/* merge-base stuff */ This is the only line that did not make it to the other file. :-) I don't think it is needed in commit-reach.c
