Re: [PATCH v3] documentation: add tutorial for revision walking
- From
Emily Shaffer <emilyshaffer@google.com>
- Date
- Aug 6, 2019, 23:10 UTC
- Message-ID
- <20190806231050.GA130824@google.com>
- In-Reply-To
- <20190724233253.208318-1-jonathantanmy@google.com>
On Wed, Jul 24, 2019 at 04:32:53PM -0700, Jonathan Tan wrote:
Show 14 quoted lines
> 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.
Show 8 quoted lines
> > > +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.
Show 9 quoted lines
> > > +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!
Show 8 quoted lines
> > > +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.
Show 24 quoted lines
>
> > +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.