threads / patch / 57578

patch, 8 partsreflog: migrate fully to parse_options()

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

## tl;dr

15 messages between Mar 17, 2022 and Mar 28, 2022. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · lore

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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to reflog.c +10 −10
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
John Cai· Mar 17, 2022, 19:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Hi Ævar
On 17 Mar 2022, at 14:08, Ævar Arnfjörð Bjarmason wrote:
Show 63 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/reflog.c +3 −6
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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Show changes to 2 files +32 −5

t/t1411-reflog-show.sh, 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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/reflog.c +24 −15
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
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/reflog.c +5 −6
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
Junio C Hamano· Mar 18, 2022, 01:24 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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".

Show 29 quoted lines
> 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.

Show 5 quoted lines
>  #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>")
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +21 −22

builtin/reflog.c, t/t1418-reflog-exists.sh

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
Junio C Hamano· Mar 18, 2022, 01:30 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 29 quoted lines
> 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.

Show 15 quoted lines
> +	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.
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +50 −9

builtin/reflog.c, t/t1410-reflog.sh, t/test-lib-functions.sh

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
Junio C Hamano· Mar 18, 2022, 01:49 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 20 quoted lines
>  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?
Show 7 quoted lines
> +	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.

Show 10 quoted lines
> 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.
Ævar Arnfjörð Bjarmason· Mar 17, 2022, 18:08 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 2 files +28 −2

builtin/reflog.c, t/t1410-reflog.sh

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
SZEDER Gábor· Mar 28, 2022, 21:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/reflog.c +2 −2
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
Junio C Hamano· Mar 28, 2022, 22:45 UTC · re: SZEDER Gábor · lore

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

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 11 quoted lines
> 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.
Show 27 quoted lines
> 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"))

← back to recent threads