Re: [PATCH v3] documentation: add tutorial for revision walking

[Emily Shaffer <emilyshaffer@google.com>]

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.