Re: [PATCH v2 5/7] reflog: improve and update documentation

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 2, 2015, 22:04 UTC
Message-ID: <xmqqfv9n9htg.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1425288597-20547-6-git-send-email-mhagger@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 7 quoted lines

> +Reference logs, or "reflogs", record when the tips of branches and
> +other references were updated in the local repository. Reflogs are
> +useful in various Git commands, to specify the old value of a
> +reference. For example, `HEAD@{2}` means "where HEAD used to be two
> +moves ago", `master@{one.week.ago}` means "where master used to point
> +to one week ago", and so on. See linkgit:gitrevisions[7] for more
> +details.

Looks very good, especially the part that mentions "in the local repository". It seems to be a common novice misunderstanding what `master@{one.week.ago}` means, and it might be beneficial to be more verbose by saying "where master used to point to one week ago in this local repository".

Show 9 quoted lines

> +The "expire" subcommand prunes older reflog entries. Entries older
> +than `expire` time, or entries older than `expire-unreachable` time
> +and not reachable from the current tip, are removed from the reflog.
> +This is typically not used directly by end users -- instead, see
> +linkgit:git-gc[1].
> +
> +The "delete" subcommand deletes single entries from the reflog. Its
> +argument must be an _exact_ entry (e.g. "`git reflog delete
> +master@{2}`").

Just like "expire", "delete" should be accompanied by the same "typically not". I do not think it is even worth mentioning that it exists merely as an implementation detail for likgit:git-stash[1] and for no other reason.

> +Options for `expire` and/or `delete`
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I think this started from a hope that these two share many, but looking at the result I notice the shared ones are a tiny and trivial minority. It probably makes sense to retitle this section "Options for expire" (and remove "For the expire command only"), and add an "Options for delete" section immediately after it that looks like:

	Options for `delete`
        ~~~~~~~~~~~~~~~~~~~~

	--updateref::
        --rewrite::
        --dry-run::
        	The `delete` command takes these options whose
                meanings are the same as those for `expire`.

Show 16 quoted lines

> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 49c64f9..dd68a72 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -13,9 +13,9 @@
>   */
>  
>  static const char reflog_expire_usage[] =
> -"git reflog expire [--verbose] [--dry-run] [--stale-fix] [--expire=<time>] [--expire-unreachable=<time>] [--all] <refs>...";
> +"git reflog expire [--expire=<time>] [--expire-unreachable=<time>] [--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] [--verbose] [--all] <refs>...";
>  static const char reflog_delete_usage[] =
> -"git reflog delete [--verbose] [--dry-run] [--rewrite] [--updateref] <refs>...";
> +"git reflog delete [--rewrite] [--updateref] [--dry-run | -n] [--verbose] <refs>...";
>  
>  static unsigned long default_reflog_expire;
>  static unsigned long default_reflog_expire_unreachable;

Thanks for being complete, but I sense that it may be time we switched to parse-options here, which gives us the help string for free. Perhaps throw in a comment line before this hunk

	/* NEEDSWORK: switch to parse-options */

or something to leave hint for other people?

Thanks.