On Wed, Jul 24, 2019 at 04:32:53PM -0700, Jonathan Tan wrote: > Thanks - I think this is a useful guide to what can be a complicated > topic. It looks good overall; I just have some minor comments below. > > > diff --git a/Documentation/Makefile b/Documentation/Makefile > > index 76f2ecfc1b..91e5da67c4 100644 > > --- a/Documentation/Makefile > > +++ b/Documentation/Makefile > > @@ -78,6 +78,7 @@ SP_ARTICLES += $(API_DOCS) > > > > TECH_DOCS += MyFirstContribution > > TECH_DOCS += SubmittingPatches > > +TECH_DOCS += MyFirstRevWalk > > Any reason why this is not in alphabetical order? No reason, will fix. > > > +Also add the relevant line in `builtin.h` near `cmd_whatchanged()`: > > + > > +---- > > +extern int cmd_walken(int argc, const char **argv, const char *prefix); > > +---- > > builtin.h no longer has "extern", so we can delete it. Done. > > > +Add it to the `Makefile` near the line for `builtin\worktree.o`: > > + > > +---- > > +BUILTIN_OBJS += builtin/walken.o > > +---- > > In the first line, change the backslash to a slash. (The line in > Makefile for "builtin/worktree.o" uses a forward slash as expected.) Done, not sure how this got in there. Thanks! > > > +NOTE: For a more exhaustive overview of the new command process, take a look at > > +`Documentation/MyFirstContribution.txt`. > > + > > +NOTE: A reference implementation can be found at TODO LINK. > > I think you have a reference implementation at > https://github.com/nasamuffin/git/tree/revwalk? Yep, although it's not very fresh. I was hoping to wait for a way for us to check in the reference implementation to Git source, although that can wait and the off-project branch is maybe OK for now. > > > +We'll start by enabling all types of objects in the `struct rev_info`. Unless > > +you cloned or fetched your repository earlier with a filter, > > +`exclude_promisor_objects` is unlikely to make a difference, but we'll turn it > > +on just to make sure our lives are simple. We'll also turn on > > +`tree_blobs_in_commit_order`, which means that we will walk a commit's tree and > > +everything it points to immediately after we find each commit, as opposed to > > +waiting for the end and walking through all trees after the commit history has > > +been discovered. With the appropriate settings configured, we are ready to call > > +`prepare_revision_walk()`. > > + > > +---- > > +static void walken_object_walk(struct rev_info *rev) > > +{ > > + rev->tree_objects = 1; > > + rev->blob_objects = 1; > > + rev->tag_objects = 1; > > + rev->tree_blobs_in_commit_order = 1; > > + rev->exclude_promisor_objects = 1; > > Optional: I think we should not bother with exclude_promisor_objects. If > the user really cloned with a filter, then every object would be a > promisor object and the revision walk should output nothing, which is > very confusing. Sure, that makes sense. Ok, I removed it. Thanks for looking - and for the patience with the latency on the reply.
