[PATCH 0/8] reflog: migrate fully to parse_options()

[PATCH 0/8] reflog: migrate fully to parse_options()

[Ævar Arnfjörð Bjarmason]

This is a follow-up to the recently landed 1b4d9b4512e (Merge branch
'jc/reflog-parse-options', 2022-02-05). It converted a couple of
subcommands in builtin/reflog.c to use parse_options(), this converts
the rest.

The 1/8 is a technically unrelated indentation fix for the recently
landed 7d3d226e700 (reflog: libify delete reflog function and helpers,
2022-03-02), but since it's also in the reflog code I figured it was
OK and that it didn't deserve its own topic.

Ævar Arnfjörð Bjarmason (8):
  reflog.c: indent argument lists
  reflog: refactor cmd_reflog() to "if" branches
  reflog tests: add missing "git reflog exists" tests
  reflog: move "usage" variables and use macros
  git reflog [expire|delete]: make -h output consistent with SYNOPSIS
  reflog exists: use parse_options() API
  reflog: convert to parse_options() API
  reflog [show]: display sensible -h output

 builtin/reflog.c         | 146 +++++++++++++++++++++++++--------------
 reflog.c                 |  20 +++---
 t/t1410-reflog.sh        |  22 ++++++
 t/t1411-reflog-show.sh   |   5 --
 t/t1418-reflog-exists.sh |  37 ++++++++++
 t/test-lib-functions.sh  |   2 +-
 6 files changed, 165 insertions(+), 67 deletions(-)
 create mode 100755 t/t1418-reflog-exists.sh

-- 
2.35.1.1384.g7d2906948a1

[PATCH 1/8] reflog.c: indent argument lists

[Ævar Arnfjörð Bjarmason]

When reflog.c was lib-ified in 7d3d226e700 (reflog: libify delete
reflog function and helpers, 2022-03-02) these previously "static"
functions were made non-"static", but the argument lists were not
correspondingly indented according to our usual coding style. Let's do
that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 reflog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/reflog.c b/reflog.c
index 333fd8708fe..82e5a935117 100644
--- a/reflog.c
+++ b/reflog.c
@@ -240,8 +240,8 @@ static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit
  * Return true iff the specified reflog entry should be expired.
  */
 int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
-				    const char *email, timestamp_t timestamp, int tz,
-				    const char *message, void *cb_data)
+			     const char *email, timestamp_t timestamp, int tz,
+			     const char *message, void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit *old_commit, *new_commit;
@@ -273,10 +273,10 @@ int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
 }
 
 int should_expire_reflog_ent_verbose(struct object_id *ooid,
-					    struct object_id *noid,
-					    const char *email,
-					    timestamp_t timestamp, int tz,
-					    const char *message, void *cb_data)
+				     struct object_id *noid,
+				     const char *email,
+				     timestamp_t timestamp, int tz,
+				     const char *message, void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	int expire;
@@ -323,8 +323,8 @@ static int is_head(const char *refname)
 }
 
 void reflog_expiry_prepare(const char *refname,
-				  const struct object_id *oid,
-				  void *cb_data)
+			   const struct object_id *oid,
+			   void *cb_data)
 {
 	struct expire_reflog_policy_cb *cb = cb_data;
 	struct commit_list *elem;
@@ -377,8 +377,8 @@ void reflog_expiry_cleanup(void *cb_data)
 }
 
 int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
-		const char *email, timestamp_t timestamp, int tz,
-		const char *message, void *cb_data)
+		     const char *email, timestamp_t timestamp, int tz,
+		     const char *message, void *cb_data)
 {
 	struct cmd_reflog_expire_cb *cb = cb_data;
 	if (!cb->expire_total || timestamp < cb->expire_total)
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 1/8] reflog.c: indent argument lists

[John Cai]

Hi Ævar

On 17 Mar 2022, at 14:08, Ævar Arnfjörð Bjarmason wrote:

> When reflog.c was lib-ified in 7d3d226e700 (reflog: libify delete
> reflog function and helpers, 2022-03-02) these previously "static"
> functions were made non-"static", but the argument lists were not
> correspondingly indented according to our usual coding style. Let's do
> that.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  reflog.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/reflog.c b/reflog.c
> index 333fd8708fe..82e5a935117 100644
> --- a/reflog.c
> +++ b/reflog.c
> @@ -240,8 +240,8 @@ static int unreachable(struct expire_reflog_policy_cb *cb, struct commit *commit
>   * Return true iff the specified reflog entry should be expired.
>   */
>  int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
> -				    const char *email, timestamp_t timestamp, int tz,
> -				    const char *message, void *cb_data)
> +			     const char *email, timestamp_t timestamp, int tz,
> +			     const char *message, void *cb_data)
>  {
>  	struct expire_reflog_policy_cb *cb = cb_data;
>  	struct commit *old_commit, *new_commit;
> @@ -273,10 +273,10 @@ int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
>  }
>
>  int should_expire_reflog_ent_verbose(struct object_id *ooid,
> -					    struct object_id *noid,
> -					    const char *email,
> -					    timestamp_t timestamp, int tz,
> -					    const char *message, void *cb_data)
> +				     struct object_id *noid,
> +				     const char *email,
> +				     timestamp_t timestamp, int tz,
> +				     const char *message, void *cb_data)
>  {
>  	struct expire_reflog_policy_cb *cb = cb_data;
>  	int expire;
> @@ -323,8 +323,8 @@ static int is_head(const char *refname)
>  }
>
>  void reflog_expiry_prepare(const char *refname,
> -				  const struct object_id *oid,
> -				  void *cb_data)
> +			   const struct object_id *oid,
> +			   void *cb_data)
>  {
>  	struct expire_reflog_policy_cb *cb = cb_data;
>  	struct commit_list *elem;
> @@ -377,8 +377,8 @@ void reflog_expiry_cleanup(void *cb_data)
>  }
>
>  int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
> -		const char *email, timestamp_t timestamp, int tz,
> -		const char *message, void *cb_data)
> +		     const char *email, timestamp_t timestamp, int tz,
> +		     const char *message, void *cb_data)
>  {
>  	struct cmd_reflog_expire_cb *cb = cb_data;
>  	if (!cb->expire_total || timestamp < cb->expire_total)

Just wanted to say thanks for fixing these :)

> -- 
> 2.35.1.1384.g7d2906948a1

[PATCH 2/8] reflog: refactor cmd_reflog() to "if" branches

[Ævar Arnfjörð Bjarmason]

Refactor the "if" branches in cmd_reflog() to use "else if" instead,
and remove the whitespace between them.

As with 92f480909f7 (multi-pack-index: refactor "goto usage" pattern,
2021-08-23) this makes this code more consistent with how
builtin/{bundle,stash,commit-graph,multi-pack-index}.c look and
behave. Their top-level commands are all similar sub-command routing
functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 9407f835cb6..c864f276308 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -384,14 +384,11 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 
 	if (!strcmp(argv[1], "show"))
 		return cmd_log_reflog(argc - 1, argv + 1, prefix);
-
-	if (!strcmp(argv[1], "expire"))
+	else if (!strcmp(argv[1], "expire"))
 		return cmd_reflog_expire(argc - 1, argv + 1, prefix);
-
-	if (!strcmp(argv[1], "delete"))
+	else if (!strcmp(argv[1], "delete"))
 		return cmd_reflog_delete(argc - 1, argv + 1, prefix);
-
-	if (!strcmp(argv[1], "exists"))
+	else if (!strcmp(argv[1], "exists"))
 		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
 
 	return cmd_log_reflog(argc, argv, prefix);
-- 
2.35.1.1384.g7d2906948a1

[PATCH 3/8] reflog tests: add missing "git reflog exists" tests

[Ævar Arnfjörð Bjarmason]

There were a few "git reflog exists" tests scattered over the test
suite, but let's consolidate the testing of the main functionality
into a new test file. This makes it easier to run just these tests
during development.

To do that amend and extend an existing test added in
afcb2e7a3b8 (git-reflog: add exists command, 2015-07-21). Let's use
"test_must_fail" instead of "!" (in case it segfaults), and test for
basic usage, an unknown option etc.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1411-reflog-show.sh   |  5 -----
 t/t1418-reflog-exists.sh | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 5 deletions(-)
 create mode 100755 t/t1418-reflog-exists.sh

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 0bb319b944a..975c4ea83a8 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -169,9 +169,4 @@ test_expect_success 'git log -g -p shows diffs vs. parents' '
 	test_cmp expect actual
 '
 
-test_expect_success 'reflog exists works' '
-	git reflog exists refs/heads/main &&
-	! git reflog exists refs/heads/nonexistent
-'
-
 test_done
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
new file mode 100755
index 00000000000..60c6411ce3c
--- /dev/null
+++ b/t/t1418-reflog-exists.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='Test reflog display routines'
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	test_commit A
+'
+
+test_expect_success 'usage' '
+	test_expect_code 129 git reflog exists &&
+	test_expect_code 129 git reflog exists -h
+'
+
+test_expect_success 'usage: unknown option' '
+	test_expect_code 129 git reflog exists --unknown-option
+'
+
+test_expect_success 'reflog exists works' '
+	git reflog exists refs/heads/main &&
+	test_must_fail git reflog exists refs/heads/nonexistent
+'
+
+test_expect_success 'reflog exists works with a "--" delimiter' '
+	git reflog exists -- refs/heads/main &&
+	test_must_fail git reflog exists -- refs/heads/nonexistent
+'
+
+test_done
-- 
2.35.1.1384.g7d2906948a1

[PATCH 4/8] reflog: move "usage" variables and use macros

[Ævar Arnfjörð Bjarmason]

Move the "usage" variables in builtin/reflog.c to the top of the file,
in preparation for later commits defining a common "reflog_usage" in
terms of some of these strings, as was done in
8757b35d443 (commit-graph: define common usage with a macro,
2021-08-23).

While we're at it let's make them "const char *const", as is the
convention with these "usage" variables.

The use of macros here is a bit odd, but in subsequent commits we'll
make these use the same pattern as builtin/commit-graph.c uses since
8757b35d443 (commit-graph: define common usage with a macro,
2021-08-23).

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index c864f276308..25313d504a9 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -5,8 +5,31 @@
 #include "worktree.h"
 #include "reflog.h"
 
+#define BUILTIN_REFLOG_EXPIRE_USAGE \
+	N_("git reflog expire [--expire=<time>] " \
+	   "[--expire-unreachable=<time>] " \
+	   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] " \
+	   "[--verbose] [--all] <refs>...")
+
+#define BUILTIN_REFLOG_DELETE_USAGE \
+	N_("git reflog delete [--rewrite] [--updateref] " \
+	   "[--dry-run | -n] [--verbose] <refs>...")
+
+#define BUILTIN_REFLOG_EXISTS_USAGE \
+	N_("git reflog exists <ref>")
+
+static const char *const reflog_expire_usage[] = {
+	BUILTIN_REFLOG_EXPIRE_USAGE,
+	NULL
+};
+
+static const char *const reflog_delete_usage[] = {
+	BUILTIN_REFLOG_DELETE_USAGE,
+	NULL
+};
+
 static const char reflog_exists_usage[] =
-N_("git reflog exists <ref>");
+	BUILTIN_REFLOG_EXISTS_USAGE;
 
 static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
@@ -147,14 +170,6 @@ static void set_reflog_expiry_param(struct cmd_reflog_expire_cb *cb, const char
 		cb->expire_unreachable = default_reflog_expire_unreachable;
 }
 
-static const char * reflog_expire_usage[] = {
-	N_("git reflog expire [--expire=<time>] "
-	   "[--expire-unreachable=<time>] "
-	   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] "
-	   "[--verbose] [--all] <refs>..."),
-	NULL
-};
-
 static int expire_unreachable_callback(const struct option *opt,
 				 const char *arg,
 				 int unset)
@@ -304,12 +319,6 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
-static const char * reflog_delete_usage[] = {
-	N_("git reflog delete [--rewrite] [--updateref] "
-	   "[--dry-run | -n] [--verbose] <refs>..."),
-	NULL
-};
-
 static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 {
 	int i, status = 0;
-- 
2.35.1.1384.g7d2906948a1

[PATCH 5/8] git reflog [expire|delete]: make -h output consistent with SYNOPSIS

[Ævar Arnfjörð Bjarmason]

Make use of the guaranteed pretty alignment of "-h" output added in my
4631cfc20bd (parse-options: properly align continued usage output,
2021-09-21) and wrap and format the "git reflog [expire|delete] -h"
usage output. Also add the missing "--single-worktree" option, as well
as adding other things that were in the SYNOPSIS output, but not in
the "-h" output.

This was last touched in 33d7bdd6459 (builtin/reflog.c: use
parse-options api for expire, delete subcommands, 2022-01-06), but in
that commit the previous usage() output was faithfully
reproduced. Let's follow-up on that and make this even easier to read.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 25313d504a9..458764400b5 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -6,14 +6,13 @@
 #include "reflog.h"
 
 #define BUILTIN_REFLOG_EXPIRE_USAGE \
-	N_("git reflog expire [--expire=<time>] " \
-	   "[--expire-unreachable=<time>] " \
-	   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] " \
-	   "[--verbose] [--all] <refs>...")
+	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_DELETE_USAGE \
-	N_("git reflog delete [--rewrite] [--updateref] " \
-	   "[--dry-run | -n] [--verbose] <refs>...")
+	N_("git reflog delete [--rewrite] [--updateref]\n" \
+	   "                  [--dry-run | -n] [--verbose] <ref>@{<specifier>}...")
 
 #define BUILTIN_REFLOG_EXISTS_USAGE \
 	N_("git reflog exists <ref>")
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 5/8] git reflog [expire|delete]: make -h output consistent with SYNOPSIS

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Make use of the guaranteed pretty alignment of "-h" output added in my
> 4631cfc20bd (parse-options: properly align continued usage output,
> 2021-09-21) and wrap and format the "git reflog [expire|delete] -h"

"(expire|delete) -h", I think.  Does it matter who wrote an earlier
commit, by the way?  You do not name who did 33d7bdd6459 you refer
to in the later part of the proposed log message, for example, and
it feels simpler to consider all the past commits as just "ours".

> usage output. Also add the missing "--single-worktree" option, as well
> as adding other things that were in the SYNOPSIS output, but not in
> the "-h" output.
>
> This was last touched in 33d7bdd6459 (builtin/reflog.c: use
> parse-options api for expire, delete subcommands, 2022-01-06), but in
> that commit the previous usage() output was faithfully
> reproduced. Let's follow-up on that and make this even easier to read.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/reflog.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 25313d504a9..458764400b5 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -6,14 +6,13 @@
>  #include "reflog.h"
>  
>  #define BUILTIN_REFLOG_EXPIRE_USAGE \
> -	N_("git reflog expire [--expire=<time>] " \
> -	   "[--expire-unreachable=<time>] " \
> -	   "[--rewrite] [--updateref] [--stale-fix] [--dry-run | -n] " \
> -	   "[--verbose] [--all] <refs>...")
> +	N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
> +	   "                  [--rewrite] [--updateref] [--stale-fix]\n" \
> +	   "                  [--dry-run | -n] [--verbose] [--all [--single-worktree] | <refs>...]")

OK.  This makes the line folding match what we write in the synopsis
section of the documentation.

>  #define BUILTIN_REFLOG_DELETE_USAGE \
> -	N_("git reflog delete [--rewrite] [--updateref] " \
> -	   "[--dry-run | -n] [--verbose] <refs>...")
> +	N_("git reflog delete [--rewrite] [--updateref]\n" \
> +	   "                  [--dry-run | -n] [--verbose] <ref>@{<specifier>}...")

Likewise.  Looking good.


>  #define BUILTIN_REFLOG_EXISTS_USAGE \
>  	N_("git reflog exists <ref>")

[PATCH 6/8] reflog exists: use parse_options() API

[Ævar Arnfjörð Bjarmason]

Change the "reflog exists" command added in afcb2e7a3b8 (git-reflog:
add exists command, 2015-07-21) to use parse_options() instead of its
own custom command-line parser. This continues work started in
33d7bdd6459 (builtin/reflog.c: use parse-options api for expire,
delete subcommands, 2022-01-06).

As a result we'll understand the --end-of-options synonym for "--", so
let's test for that.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c         | 38 ++++++++++++++++----------------------
 t/t1418-reflog-exists.sh |  5 +++++
 2 files changed, 21 insertions(+), 22 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 458764400b5..9847e9db3de 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -27,8 +27,10 @@ static const char *const reflog_delete_usage[] = {
 	NULL
 };
 
-static const char reflog_exists_usage[] =
-	BUILTIN_REFLOG_EXISTS_USAGE;
+static const char *const reflog_exists_usage[] = {
+	BUILTIN_REFLOG_EXISTS_USAGE,
+	NULL,
+};
 
 static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
@@ -350,28 +352,20 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)
 
 static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
 {
-	int i, start = 0;
-
-	for (i = 1; i < argc; i++) {
-		const char *arg = argv[i];
-		if (!strcmp(arg, "--")) {
-			i++;
-			break;
-		}
-		else if (arg[0] == '-')
-			usage(_(reflog_exists_usage));
-		else
-			break;
-	}
-
-	start = i;
+	struct option options[] = {
+		OPT_END()
+	};
+	const char *refname;
 
-	if (argc - start != 1)
-		usage(_(reflog_exists_usage));
+	argc = parse_options(argc, argv, prefix, options, reflog_exists_usage,
+			     0);
+	if (!argc)
+		usage_with_options(reflog_exists_usage, options);
 
-	if (check_refname_format(argv[start], REFNAME_ALLOW_ONELEVEL))
-		die(_("invalid ref format: %s"), argv[start]);
-	return !reflog_exists(argv[start]);
+	refname = argv[0];
+	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
+		die(_("invalid ref format: %s"), refname);
+	return !reflog_exists(refname);
 }
 
 /*
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
index 60c6411ce3c..d51ecd5e925 100755
--- a/t/t1418-reflog-exists.sh
+++ b/t/t1418-reflog-exists.sh
@@ -29,4 +29,9 @@ test_expect_success 'reflog exists works with a "--" delimiter' '
 	test_must_fail git reflog exists -- refs/heads/nonexistent
 '
 
+test_expect_success 'reflog exists works with a "--end-of-options" delimiter' '
+	git reflog exists --end-of-options refs/heads/main &&
+	test_must_fail git reflog exists --end-of-options refs/heads/nonexistent
+'
+
 test_done
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 6/8] reflog exists: use parse_options() API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the "reflog exists" command added in afcb2e7a3b8 (git-reflog:
> add exists command, 2015-07-21) to use parse_options() instead of its
> own custom command-line parser. This continues work started in
> 33d7bdd6459 (builtin/reflog.c: use parse-options api for expire,
> delete subcommands, 2022-01-06).
>
> As a result we'll understand the --end-of-options synonym for "--", so
> let's test for that.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/reflog.c         | 38 ++++++++++++++++----------------------
>  t/t1418-reflog-exists.sh |  5 +++++
>  2 files changed, 21 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 458764400b5..9847e9db3de 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -27,8 +27,10 @@ static const char *const reflog_delete_usage[] = {
>  	NULL
>  };
>  
> -static const char reflog_exists_usage[] =
> -	BUILTIN_REFLOG_EXISTS_USAGE;
> +static const char *const reflog_exists_usage[] = {
> +	BUILTIN_REFLOG_EXISTS_USAGE,
> +	NULL,
> +};

Good.

The discrepanthy in the postimage of [PATCH 4/8] bothered me a lot.

This would immediately help by making it possible to feed it to
parse_options(), but the uniformity will also make it easier to
concatenate common and subcommand specific usage later, I would
presume, if we wanted to.

> +	struct option options[] = {
> +		OPT_END()
> +	};
> +	const char *refname;
>  
> +	argc = parse_options(argc, argv, prefix, options, reflog_exists_usage,
> +			     0);
> +	if (!argc)
> +		usage_with_options(reflog_exists_usage, options);
>  
> +	refname = argv[0];
> +	if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
> +		die(_("invalid ref format: %s"), refname);
> +	return !reflog_exists(refname);
>  }

Quite straight-forward.  Looking good.

[PATCH 7/8] reflog: convert to parse_options() API

[Ævar Arnfjörð Bjarmason]

Continue the work started in 33d7bdd6459 (builtin/reflog.c: use
parse-options api for expire, delete subcommands, 2022-01-06) and
convert the cmd_reflog() function itself to use the parse_options()
API.

Let's also add a test which would fail if we forgot
PARSE_OPT_NO_INTERNAL_HELP here, as well as making sure that we'll
still pass through "--" by supplying PARSE_OPT_KEEP_DASHDASH. For that
test we need to change "test_commit()" to accept files starting with
"--".

The "git reflog -h" usage will now show the usage for all of the
sub-commands, rather than a terse summary which wasn't
correct (e.g. "git reflog exists" is not a valid command). See my
8757b35d443 (commit-graph: define common usage with a macro,
2021-08-23) for prior art.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c        | 40 ++++++++++++++++++++++++++++++++--------
 t/t1410-reflog.sh       | 17 +++++++++++++++++
 t/test-lib-functions.sh |  2 +-
 3 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 9847e9db3de..3971921fc14 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -32,6 +32,14 @@ static const char *const reflog_exists_usage[] = {
 	NULL,
 };
 
+static const char *const reflog_usage[] = {
+	N_("git reflog [show] [<log-options>] [<ref>]"),
+	BUILTIN_REFLOG_EXPIRE_USAGE,
+	BUILTIN_REFLOG_DELETE_USAGE,
+	BUILTIN_REFLOG_EXISTS_USAGE,
+	NULL
+};
+
 static timestamp_t default_reflog_expire;
 static timestamp_t default_reflog_expire_unreachable;
 
@@ -372,17 +380,28 @@ static int cmd_reflog_exists(int argc, const char **argv, const char *prefix)
  * main "reflog"
  */
 
-static const char reflog_usage[] =
-"git reflog [ show | expire | delete | exists ]";
-
 int cmd_reflog(int argc, const char **argv, const char *prefix)
 {
-	if (argc > 1 && !strcmp(argv[1], "-h"))
-		usage(_(reflog_usage));
+	struct option options[] = {
+		OPT_END()
+	};
 
-	/* With no command, we default to showing it. */
-	if (argc < 2 || *argv[1] == '-')
-		return cmd_log_reflog(argc, argv, prefix);
+	argc = parse_options(argc, argv, prefix, options, reflog_usage,
+			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
+			     PARSE_OPT_KEEP_UNKNOWN |
+			     PARSE_OPT_NO_INTERNAL_HELP);
+
+	/*
+	  * With "git reflog" we default to showing it. !argc is
+	  * impossible with PARSE_OPT_KEEP_ARGV0.
+	  */
+	if (argc == 1)
+		goto log_reflog;
+
+	if (!strcmp(argv[1], "-h"))
+		usage_with_options(reflog_usage, options);
+	else if (*argv[1] == '-')
+		goto log_reflog;
 
 	if (!strcmp(argv[1], "show"))
 		return cmd_log_reflog(argc - 1, argv + 1, prefix);
@@ -393,5 +412,10 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 	else if (!strcmp(argv[1], "exists"))
 		return cmd_reflog_exists(argc - 1, argv + 1, prefix);
 
+	/*
+	 * Fall-through for e.g. "git reflog -1", "git reflog master",
+	 * as well as the plain "git reflog" above goto above.
+	 */
+log_reflog:
 	return cmd_log_reflog(argc, argv, prefix);
 }
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 68f69bb5431..0dc36d842b0 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -106,6 +106,23 @@ test_expect_success setup '
 	test_line_count = 4 output
 '
 
+test_expect_success 'correct usage on sub-command -h' '
+	test_expect_code 129 git reflog expire -h >err &&
+	grep "git reflog expire" err
+'
+
+test_expect_success 'pass through -- to sub-command' '
+	test_when_finished "rm -rf repo" &&
+	git init repo &&
+	test_commit -C repo message --a-file contents dash-tag &&
+
+	git -C repo reflog show -- --does-not-exist >out &&
+	test_must_be_empty out &&
+	git -C repo reflog show >expect &&
+	git -C repo reflog show -- --a-file >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success rewind '
 	test_tick && git reset --hard HEAD~2 &&
 	test -f C &&
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0f439c99d61..1d004744589 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -329,7 +329,7 @@ test_commit () {
 	else
 		$echo "${3-$1}" >"$indir$file"
 	fi &&
-	git ${indir:+ -C "$indir"} add "$file" &&
+	git ${indir:+ -C "$indir"} add -- "$file" &&
 	if test -z "$notick"
 	then
 		test_tick
-- 
2.35.1.1384.g7d2906948a1

Re: [PATCH 7/8] reflog: convert to parse_options() API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  int cmd_reflog(int argc, const char **argv, const char *prefix)
>  {
> -	if (argc > 1 && !strcmp(argv[1], "-h"))
> -		usage(_(reflog_usage));
> +	struct option options[] = {
> +		OPT_END()
> +	};
>  
> -	/* With no command, we default to showing it. */
> -	if (argc < 2 || *argv[1] == '-')
> -		return cmd_log_reflog(argc, argv, prefix);
> +	argc = parse_options(argc, argv, prefix, options, reflog_usage,
> +			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
> +			     PARSE_OPT_KEEP_UNKNOWN |
> +			     PARSE_OPT_NO_INTERNAL_HELP);
> +
> +	/*
> +	  * With "git reflog" we default to showing it. !argc is
> +	  * impossible with PARSE_OPT_KEEP_ARGV0.
> +	  */

Funny indentation?

> +	if (argc == 1)
> +		goto log_reflog;
> +
> +	if (!strcmp(argv[1], "-h"))
> +		usage_with_options(reflog_usage, options);
> +	else if (*argv[1] == '-')
> +		goto log_reflog;

Unfortunate.  KEEP_UNKNOWN is unwieldy, but it is no worse than the
original.  We do not have options that are common to all "git reflog"
subcommands, so the first token after "git reflog" being anything
that begins with "-" is a sign that the default "show" command is
being asked for.

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 0f439c99d61..1d004744589 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -329,7 +329,7 @@ test_commit () {
>  	else
>  		$echo "${3-$1}" >"$indir$file"
>  	fi &&
> -	git ${indir:+ -C "$indir"} add "$file" &&
> +	git ${indir:+ -C "$indir"} add -- "$file" &&

OK.

Looking good.

[PATCH 8/8] reflog [show]: display sensible -h output

[Ævar Arnfjörð Bjarmason]

Change the "git reflog show -h" output to show the usage summary
relevant to it, rather than displaying the same output that "git log
-h" would show.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/reflog.c  | 25 +++++++++++++++++++++++--
 t/t1410-reflog.sh |  5 +++++
 2 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 3971921fc14..aaf65ed31c6 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -5,6 +5,9 @@
 #include "worktree.h"
 #include "reflog.h"
 
+#define BUILTIN_REFLOG_SHOW_USAGE \
+	N_("git reflog [show] [<log-options>] [<ref>]")
+
 #define BUILTIN_REFLOG_EXPIRE_USAGE \
 	N_("git reflog expire [--expire=<time>] [--expire-unreachable=<time>]\n" \
 	   "                  [--rewrite] [--updateref] [--stale-fix]\n" \
@@ -17,6 +20,11 @@
 #define BUILTIN_REFLOG_EXISTS_USAGE \
 	N_("git reflog exists <ref>")
 
+static const char *const reflog_show_usage[] = {
+	BUILTIN_REFLOG_SHOW_USAGE,
+	NULL,
+};
+
 static const char *const reflog_expire_usage[] = {
 	BUILTIN_REFLOG_EXPIRE_USAGE,
 	NULL
@@ -33,7 +41,7 @@ static const char *const reflog_exists_usage[] = {
 };
 
 static const char *const reflog_usage[] = {
-	N_("git reflog [show] [<log-options>] [<ref>]"),
+	BUILTIN_REFLOG_SHOW_USAGE,
 	BUILTIN_REFLOG_EXPIRE_USAGE,
 	BUILTIN_REFLOG_DELETE_USAGE,
 	BUILTIN_REFLOG_EXISTS_USAGE,
@@ -207,6 +215,19 @@ static int expire_total_callback(const struct option *opt,
 	return 0;
 }
 
+static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
+{
+	struct option options[] = {
+		OPT_END()
+	};
+
+	parse_options(argc, argv, prefix, options, reflog_show_usage,
+		      PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
+		      PARSE_OPT_KEEP_UNKNOWN);
+
+	return cmd_log_reflog(argc - 1, argv + 1, prefix);
+}
+
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 {
 	struct cmd_reflog_expire_cb cmd = { 0 };
@@ -404,7 +425,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 		goto log_reflog;
 
 	if (!strcmp(argv[1], "show"))
-		return cmd_log_reflog(argc - 1, argv + 1, prefix);
+		return cmd_reflog_show(argc, argv, prefix);
 	else if (!strcmp(argv[1], "expire"))
 		return cmd_reflog_expire(argc - 1, argv + 1, prefix);
 	else if (!strcmp(argv[1], "delete"))
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index 0dc36d842b0..3f469353ec7 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -111,6 +111,11 @@ test_expect_success 'correct usage on sub-command -h' '
 	grep "git reflog expire" err
 '
 
+test_expect_success 'correct usage on "git reflog show -h"' '
+	test_expect_code 129 git reflog show -h >err &&
+	grep -F "git reflog [show]" err
+'
+
 test_expect_success 'pass through -- to sub-command' '
 	test_when_finished "rm -rf repo" &&
 	git init repo &&
-- 
2.35.1.1384.g7d2906948a1

[PATCH] reflog: fix 'show' subcommand's argv

[SZEDER Gábor]

cmd_reflog() invokes parse_options() with PARSE_OPT_KEEP_ARGV0, but it
doesn't account for the retained argv[0] before invoking
cmd_reflog_show() to handle the 'git reflog show' subcommand.
Consequently, cmd_reflog_show() always gets an 'argv' array starting
with elements argv[0]="reflog" and argv[1]="show".

Strip the name of the git command from the 'argv' array before passing
it to the function handling the 'show' subcommand.

There is no user-visible bug here, because cmd_reflog_show() doesn't
have any options or parameters of its own.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/reflog.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 6c4fe1af40..c943c2aabe 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -225,7 +225,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
 		      PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
 		      PARSE_OPT_KEEP_UNKNOWN);
 
-	return cmd_log_reflog(argc - 1, argv + 1, prefix);
+	return cmd_log_reflog(argc, argv, prefix);
 }
 
 static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
@@ -425,7 +425,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
 		goto log_reflog;
 
 	if (!strcmp(argv[1], "show"))
-		return cmd_reflog_show(argc, argv, prefix);
+		return cmd_reflog_show(argc - 1, argv + 1, prefix);
 	else if (!strcmp(argv[1], "expire"))
 		return cmd_reflog_expire(argc - 1, argv + 1, prefix);
 	else if (!strcmp(argv[1], "delete"))
-- 
2.35.1.1272.g0ee9eb8209

Re: [PATCH] reflog: fix 'show' subcommand's argv

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> cmd_reflog() invokes parse_options() with PARSE_OPT_KEEP_ARGV0, but it
> doesn't account for the retained argv[0] before invoking
> cmd_reflog_show() to handle the 'git reflog show' subcommand.
> Consequently, cmd_reflog_show() always gets an 'argv' array starting
> with elements argv[0]="reflog" and argv[1]="show".
>
> Strip the name of the git command from the 'argv' array before passing
> it to the function handling the 'show' subcommand.
>
> There is no user-visible bug here, because cmd_reflog_show() doesn't
> have any options or parameters of its own.

These two changes cancel out as far as cmd_log_reflog() is concerned
but makes a difference inside cmd_reflog_show(), if that function
cared.

There is parse_options() call that uses argc and argv supplied by
the caller, and it is not adjusted, so it is curious why there is no
externally observable behaviour change.  But argv[0] and argv[1] are
not dashed options and because we do not say STOP_AT_NON_OPTION,
parse_options() will skip such an extra non-option argument, so is
that the reason why there is no behaviour change?

As this is an "oops, that is not quite right" fix for
ab/reflog-parse-options topic, let's queue it diretly on top of the
topic.

Thanks.

> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>  builtin/reflog.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 6c4fe1af40..c943c2aabe 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -225,7 +225,7 @@ static int cmd_reflog_show(int argc, const char **argv, const char *prefix)
>  		      PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_ARGV0 |
>  		      PARSE_OPT_KEEP_UNKNOWN);
>  
> -	return cmd_log_reflog(argc - 1, argv + 1, prefix);
> +	return cmd_log_reflog(argc, argv, prefix);
>  }
>  
>  static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
> @@ -425,7 +425,7 @@ int cmd_reflog(int argc, const char **argv, const char *prefix)
>  		goto log_reflog;
>  
>  	if (!strcmp(argv[1], "show"))
> -		return cmd_reflog_show(argc, argv, prefix);
> +		return cmd_reflog_show(argc - 1, argv + 1, prefix);
>  	else if (!strcmp(argv[1], "expire"))
>  		return cmd_reflog_expire(argc - 1, argv + 1, prefix);
>  	else if (!strcmp(argv[1], "delete"))