From: Justin Tobler <jltobler@gmail.com>
Date: Wed, 23 Jul 2025 18:14:19 GMT
Subject: Re: [PATCH 2/8] builtin/reflog: improve grouping of subcommands
Message-ID: <32gvkjmjz6otqwpaxafmz5xuznowhn4iwcdyn52qhbvzohyih4@oeimg3xvqj5u>
In-Reply-To: <20250722-pks-reflog-append-v1-2-183e5949de16@pks.im>

On 25/07/22 01:20PM, Patrick Steinhardt wrote:
> The way subcommands of git-reflog(1) are layed out does not make any

s/layed/laid/

> immediate sense. Reorder them such that read-only subcommands precede
> writing commands for a bit more structure.
> 
> Furthermore, move the "expire" subcommand last. This prepares for a
> subsequent change where we are about to introduce a new "write" command
> to append reflog entries. Like this, the writing subcommands are ordered
> such that those affecting a single reflog come before those spanning
> across all reflogs.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  Documentation/git-reflog.adoc |  8 ++++----
>  builtin/reflog.c              | 38 +++++++++++++++++++-------------------
>  2 files changed, 23 insertions(+), 23 deletions(-)
> 
> diff --git a/Documentation/git-reflog.adoc b/Documentation/git-reflog.adoc
> index 707a9b39edb..6ae13e772b8 100644
> --- a/Documentation/git-reflog.adoc
> +++ b/Documentation/git-reflog.adoc
> @@ -11,13 +11,13 @@ SYNOPSIS
>  [synopsis]
>  git reflog [show] [<log-options>] [<ref>]
>  git reflog list
> -git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
> -	[--rewrite] [--updateref] [--stale-fix]
> -	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
> +git reflog exists <ref>
>  git reflog delete [--rewrite] [--updateref]
>  	[--dry-run | -n] [--verbose] <ref>@{<specifier>}...
>  git reflog drop [--all [--single-worktree] | <refs>...]
> -git reflog exists <ref>
> +git reflog expire [--expire=<time>] [--expire-unreachable=<time>]
> +	[--rewrite] [--updateref] [--stale-fix]
> +	[--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]
>  
>  DESCRIPTION
>  -----------
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 3acaf3e32c2..b00b3f9edc9 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -17,21 +17,21 @@
>  #define BUILTIN_REFLOG_LIST_USAGE \
>  	N_("git reflog list")
>  
> -#define BUILTIN_REFLOG_EXPIRE_USAGE \
> -	N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
> -	   "                  [--rewrite] [--updateref] [--stale-fix]\n" \
> -	   "                  [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]")
> +#define BUILTIN_REFLOG_EXISTS_USAGE \
> +	N_("git reflog exists <ref>")
>  
>  #define BUILTIN_REFLOG_DELETE_USAGE \
>  	N_("git reflog delete [--rewrite] [--updateref]\n" \
>  	   "                  [--dry-run | -n] [--verbose] <ref>@{<specifier>}...")
>  
> -#define BUILTIN_REFLOG_EXISTS_USAGE \
> -	N_("git reflog exists <ref>")
> -
>  #define BUILTIN_REFLOG_DROP_USAGE \
>  	N_("git reflog drop [--all [--single-worktree] | <refs>...]")
>  
> +#define BUILTIN_REFLOG_EXPIRE_USAGE \
> +	N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
> +	   "                  [--rewrite] [--updateref] [--stale-fix]\n" \
> +	   "                  [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]")
> +
>  static const char *const reflog_show_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	NULL,
> @@ -42,9 +42,9 @@ static const char *const reflog_list_usage[] = {
>  	NULL,
>  };
>  
> -static const char *const reflog_expire_usage[] = {
> -	BUILTIN_REFLOG_EXPIRE_USAGE,
> -	NULL
> +static const char *const reflog_exists_usage[] = {
> +	BUILTIN_REFLOG_EXISTS_USAGE,
> +	NULL,
>  };
>  
>  static const char *const reflog_delete_usage[] = {
> @@ -52,23 +52,23 @@ static const char *const reflog_delete_usage[] = {
>  	NULL
>  };
>  
> -static const char *const reflog_exists_usage[] = {
> -	BUILTIN_REFLOG_EXISTS_USAGE,
> -	NULL,
> -};
> -
>  static const char *const reflog_drop_usage[] = {
>  	BUILTIN_REFLOG_DROP_USAGE,
>  	NULL,
>  };
>  
> +static const char *const reflog_expire_usage[] = {
> +	BUILTIN_REFLOG_EXPIRE_USAGE,
> +	NULL
> +};
> +
>  static const char *const reflog_usage[] = {
>  	BUILTIN_REFLOG_SHOW_USAGE,
>  	BUILTIN_REFLOG_LIST_USAGE,
> -	BUILTIN_REFLOG_EXPIRE_USAGE,
> +	BUILTIN_REFLOG_EXISTS_USAGE,
>  	BUILTIN_REFLOG_DELETE_USAGE,
>  	BUILTIN_REFLOG_DROP_USAGE,
> -	BUILTIN_REFLOG_EXISTS_USAGE,
> +	BUILTIN_REFLOG_EXPIRE_USAGE,
>  	NULL
>  };
>  
> @@ -404,10 +404,10 @@ int cmd_reflog(int argc,
>  	struct option options[] = {
>  		OPT_SUBCOMMAND("show", &fn, cmd_reflog_show),
>  		OPT_SUBCOMMAND("list", &fn, cmd_reflog_list),
> -		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
> -		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
>  		OPT_SUBCOMMAND("exists", &fn, cmd_reflog_exists),
> +		OPT_SUBCOMMAND("delete", &fn, cmd_reflog_delete),
>  		OPT_SUBCOMMAND("drop", &fn, cmd_reflog_drop),
> +		OPT_SUBCOMMAND("expire", &fn, cmd_reflog_expire),
>  		OPT_END()
>  	};

Structing the subcommands order in such a manner seems sensible, but I'm
not sure the pattern will be recognized by others that may add
subcommands in the future. Maybe we could leave a comment that mentions
the order?

-Justin