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

[Michael Haggerty <mhagger@alum.mit.edu>]

On 03/02/2015 11:04 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> +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".

Yes, that's good. I will change it.

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

OK, will change.

>> +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`.

Either way is a little bit awkward, because these two subcommands share
4 out of 8 of their options. But since "delete" is really quite useless
to end users, I stuck its options in a separate section as you
suggested, but inline in a paragraph, where they won't bother anybody.

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

OK.

Thanks for your review! A reroll will be coming soon.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu