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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 10, 2019, 20:49 UTC
Message-ID: <xmqq7e9tmahm.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20190607010708.46654-1-emilyshaffer@google.com>

Emily Shaffer <emilyshaffer@google.com> writes:

Show 10 quoted lines

> +My First Revision Walk
> +======================
> +
> +== What's a Revision Walk?
> +
> +The revision walk is a key concept in Git - this is the process that underpins
> +operations like `git log`, `git blame`, and `git reflog`. Beginning at HEAD, the
> +list of objects is found by walking parent relationships between objects. The
> +revision walk can also be usedto determine whether or not a given object is
> +reachable from the current HEAD pointer.

s/usedto/used to/;

Show 11 quoted lines

> +We'll put our fiddling into a new command. For fun, let's name it `git walken`.
> +Open up a new file `builtin/walken.c` and set up the command handler:
> +
> +----
> +/*
> + * "git walken"
> + *
> + * Part of the "My First Revision Walk" tutorial.
> + */
> +
> +#include <stdio.h>

Bad idea. In the generic part of the codebase, system headers are supposed to be supplied by including git-compat-util.h (or cache.h or builtin.h, that are common header files that begin by including it and are allowed by CodingGuidelines to be used as such).

Show 8 quoted lines

> +#include "builtin.h"
> +
> +int cmd_walken(int argc, const char **argv, const char *prefix)
> +{
> +        printf(_("cmd_walken incoming...\n"));
> +        return 0;
> +}
> +----

I wonder if it makes sense to use trace instead of printf, as our reader has already seen the psuh example for doing the above.

> +Add usage text and `-h` handling, in order to pass the test suite:

It is not wrong per-se, and it indeed is a very good practice to make sure that our subcommands consistently gives usage text and short usage. Encouraging them early is a good idea.

But "in order to pass the test suite" invites "eh, the test suite does not pass without usage and -h? why?".

Either drop the mention of "the test suite", or perhaps say something like

	Add usage text and `-h` handling, like all the subcommands
	should consistently do (our test suite will notice and
	complain if you fail to do so).

i.e. the real purpose is consistency and usability; test suite is merely an enforcement mechanism.

Show 5 quoted lines

> +----
> +{ "walken", cmd_walken, RUN_SETUP },
> +----
> +
> +Add it to the `Makefile` near the line for `builtin\worktree.o`:

Backslash intended?