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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Jul 24, 2019, 23:32 UTC
Message-ID: <20190724233253.208318-1-jonathantanmy@google.com>
In-Reply-To: <20190701201934.30321-1-emilyshaffer@google.com>

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.

Show 9 quoted lines

> 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?

Show 5 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.

Show 5 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.)

> +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?

Show 18 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.