On 03/02/2015 11:04 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> 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@xxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html