[PATCH 0/4] format-rev: add --abbrev, --color, and --date

[PATCH 0/4] format-rev: add --abbrev, --color, and --date

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Topic name: kh/format-rev-more-options

Topic summary: Add three more options for controlling the formatting. Also
do some minor refactoring and text fixes as preparatory steps.

[1/4] format-rev: use lower case for opts description
[2/4] format-rev: factor option variables into a struct
[3/4] doc: rev-list-options.adoc: factor out --date alts
[4/4] format-rev: learn --abbrev, --color, and --date

 Documentation/git-format-rev.adoc             | 44 +++++++++-
 .../rev-list-option-date-alternatives.adoc    | 55 ++++++++++++
 Documentation/rev-list-options.adoc           | 56 +-----------
 builtin/name-rev.c                            | 88 ++++++++++++-------
 t/t6120-describe.sh                           | 44 ++++++++++
 5 files changed, 195 insertions(+), 92 deletions(-)
 create mode 100644 Documentation/rev-list-option-date-alternatives.adoc


base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1
-- 
2.54.0.22.g9e26862b904

[PATCH 1/4] format-rev: use lower case for opts description

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

The option descriptions use a mix of initial capital and lower case
letters. Lower case is the correct style.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/name-rev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 60cbbfb4b7d..254c88199fd 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -833,12 +833,12 @@ int cmd_format_rev(int argc,
 		OPT_STRING_LIST(0, "notes", &notes, N_("notes"),
 				N_("display notes for pretty format")),
 		OPT_CALLBACK_F('z', "null", &nul_data, N_("z"),
-			       N_("Use NUL for input and output termination"),
+			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
 		OPT_BOOL(0, "null-input", &nul_data.nul_input,
-			 N_("Use NUL for input termination")),
+			 N_("use NUL for input termination")),
 		OPT_BOOL(0, "null-output", &nul_data.nul_output,
-			 N_("Use NUL for output termination")),
+			 N_("use NUL for output termination")),
 		OPT_END(),
 	};
 
-- 
2.54.0.22.g9e26862b904

[PATCH 2/4] format-rev: factor option variables into a struct

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

We will in two commits add three more options to this command.
Let’s prepare for that by moving option variables into a struct
so that we get less local variables.

This allows us to inline `format_nul_data` into this new
structure. Let’s also rename `stdin_mode_arg` to `stdin_mode`.
(We couldn’t use `stdin_mode` before because of the enumeration
with the same name.)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/name-rev.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 254c88199fd..7d824aa1c5d 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -772,16 +772,19 @@ int cmd_name_rev(int argc,
 	return 0;
 }
 
-struct format_nul_data {
+struct format_rev_data {
+	const char *format;
+	const char *stdin_mode;
 	bool nul_input;
 	bool nul_output;
+	struct string_list notes;
 };
 
 static int format_nul_cb(const struct option *option,
 			 const char *arg,
 			 int unset)
 {
-	struct format_nul_data *data = option->value;
+	struct format_rev_data *data = option->value;
 	data->nul_input = 1;
 	data->nul_output = 1;
 	BUG_ON_OPT_NEG(unset);
@@ -813,31 +816,30 @@ int cmd_format_rev(int argc,
 		   const char *prefix,
 		   struct repository *repo UNUSED)
 {
-	const char *format = NULL;
+	struct format_rev_data data = {
+		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
+	};
 	enum stdin_mode stdin_mode;
-	const char *stdin_mode_arg = NULL;
-	struct format_nul_data nul_data = { 0, 0 };
 	char output_terminator;
 	strbuf_getline_fn getline_fn;
 	struct display_notes_opt format_notes_opt;
 	struct rev_info format_rev = REV_INFO_INIT;
 	struct pretty_format format_pp = { 0 };
-	struct string_list notes = STRING_LIST_INIT_NODUP;
 	struct strbuf scratch_buf = STRBUF_INIT;
 	struct command cmd;
 	struct option opts[] = {
-		OPT_STRING(0, "format", &format, N_("format"),
+		OPT_STRING(0, "format", &data.format, N_("format"),
 			   N_("pretty format to use")),
-		OPT_STRING(0, "stdin-mode", &stdin_mode_arg, N_("stdin-mode"),
+		OPT_STRING(0, "stdin-mode", &data.stdin_mode, N_("stdin-mode"),
 			   N_("how revs are processed")),
-		OPT_STRING_LIST(0, "notes", &notes, N_("notes"),
+		OPT_STRING_LIST(0, "notes", &data.notes, N_("notes"),
 				N_("display notes for pretty format")),
-		OPT_CALLBACK_F('z', "null", &nul_data, N_("z"),
+		OPT_CALLBACK_F('z', "null", &data, N_("z"),
 			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
-		OPT_BOOL(0, "null-input", &nul_data.nul_input,
+		OPT_BOOL(0, "null-input", &data.nul_input,
 			 N_("use NUL for input termination")),
-		OPT_BOOL(0, "null-output", &nul_data.nul_output,
+		OPT_BOOL(0, "null-output", &data.nul_output,
 			 N_("use NUL for output termination")),
 		OPT_END(),
 	};
@@ -849,18 +851,18 @@ int cmd_format_rev(int argc,
 		usage_with_options(format_rev_usage, opts);
 	}
 
-	if (!format)
+	if (!data.format)
 		die(_("'%s' is required"), "--format");
-	if (!stdin_mode_arg)
+	if (!data.stdin_mode)
 		die(_("'%s' is required"), "--stdin-mode");
 
-	getline_fn = nul_data.nul_input ? strbuf_getline_nul : strbuf_getline_lf;
-	output_terminator = nul_data.nul_output ? '\0' : '\n';
+	getline_fn = data.nul_input ? strbuf_getline_nul : strbuf_getline_lf;
+	output_terminator = data.nul_output ? '\0' : '\n';
 
 	init_display_notes(&format_notes_opt);
-	stdin_mode = parse_stdin_mode(stdin_mode_arg);
+	stdin_mode = parse_stdin_mode(data.stdin_mode);
 
-	get_commit_format(format, &format_rev);
+	get_commit_format(data.format, &format_rev);
 	format_pp.ctx.rev = &format_rev;
 	format_pp.ctx.fmt = format_rev.commit_format;
 	format_pp.ctx.abbrev = format_rev.abbrev;
@@ -868,13 +870,13 @@ int cmd_format_rev(int argc,
 	format_pp.ctx.date_mode = format_rev.date_mode;
 	format_pp.ctx.color = GIT_COLOR_AUTO;
 
-	userformat_find_requirements(format,
+	userformat_find_requirements(data.format,
 				     &format_pp.want);
 	if (format_pp.want.notes) {
 		int ignore_show_notes = 0;
 		struct string_list_item *n;
 
-		for_each_string_list_item(n, &notes)
+		for_each_string_list_item(n, &data.notes)
 			enable_ref_display_notes(&format_notes_opt,
 						 &ignore_show_notes,
 						 n->string);
@@ -934,7 +936,7 @@ int cmd_format_rev(int argc,
 	}
 
 	strbuf_release(&scratch_buf);
-	string_list_clear(&notes, 0);
+	string_list_clear(&data.notes, 0);
 	release_display_notes(&format_notes_opt);
 	return 0;
 }
-- 
2.54.0.22.g9e26862b904

[PATCH 3/4] doc: rev-list-options.adoc: factor out --date alts

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

We will introduce `--date` to git-format-rev(1) in the next commit and
will need to add it to the documentation. Let’s factor out the option
alternatives so that it can be included in git-format-rev(1).

The initial paragraph of this option mentions things like git-log(1).
We could make it fit in git-format-rev(1) while not changing it for
git-rev-list(1) and related commands with some conditionals like
`ifndef`, but writing a dedicated paragraph is simple enough.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 .../rev-list-option-date-alternatives.adoc    | 55 ++++++++++++++++++
 Documentation/rev-list-options.adoc           | 56 +------------------
 2 files changed, 56 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/rev-list-option-date-alternatives.adoc

diff --git a/Documentation/rev-list-option-date-alternatives.adoc b/Documentation/rev-list-option-date-alternatives.adoc
new file mode 100644
index 00000000000..141570b1059
--- /dev/null
+++ b/Documentation/rev-list-option-date-alternatives.adoc
@@ -0,0 +1,55 @@
+--
+`--date=relative` shows dates relative to the current time,
+e.g. ``2 hours ago''. The `-local` option has no effect for
+`--date=relative`.
+
+`--date=local` is an alias for `--date=default-local`.
+
+`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
+The differences to the strict ISO 8601 format are:
+
+	- a space instead of the `T` date/time delimiter
+	- a space between time and time zone
+	- no colon between hours and minutes of the time zone
+
+`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict
+ISO 8601 format.
+
+`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822
+format, often found in email messages.
+
+`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.
+
+`--date=raw` shows the date as seconds since the epoch (1970-01-01
+00:00:00 UTC), followed by a space, and then the timezone as an offset
+from UTC (a `+` or `-` with four digits; the first two are hours, and
+the second two are minutes). I.e., as if the timestamp were formatted
+with `strftime("%s %z")`).
+Note that the `-local` option does not affect the seconds-since-epoch
+value (which is always measured in UTC), but does switch the accompanying
+timezone value.
+
+`--date=human` shows the timezone if the timezone does not match the
+current time-zone, and doesn't print the whole date if that matches
+(ie skip printing year for dates that are "this year", but also skip
+the whole date itself if it's in the last few days and we can just say
+what weekday it was).  For older dates the hour and minute is also
+omitted.
+
+`--date=unix` shows the date as a Unix epoch timestamp (seconds since
+1970).  As with `--raw`, this is always in UTC and therefore `-local`
+has no effect.
+
+`--date=format:<format>` feeds the _<format>_ to your system `strftime`,
+except for `%s`, `%z`, and `%Z`, which are handled internally.
+Use `--date=format:%c` to show the date in your system locale's
+preferred format.  See the `strftime`(3) manual for a complete list of
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:<format>`.
+
+`--date=default` is the default format, and is based on ctime(3)
+output.  It shows a single line with three-letter day of the week,
+three-letter month, day-of-month, hour-minute-seconds in "HH:MM:SS"
+format, followed by 4-digit year, plus timezone information, unless
+the local time zone is used, e.g. `Thu Jan 1 00:00:00 1970 +0000`.
+--
diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index fd831f0ec64..6e6093f4747 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -1132,61 +1132,7 @@ include::pretty-options.adoc[]
 	author's). If `-local` is appended to the format (e.g.,
 	`iso-local`), the user's local time zone is used instead.
 +
---
-`--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''. The `-local` option has no effect for
-`--date=relative`.
-
-`--date=local` is an alias for `--date=default-local`.
-
-`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
-The differences to the strict ISO 8601 format are:
-
-	- a space instead of the `T` date/time delimiter
-	- a space between time and time zone
-	- no colon between hours and minutes of the time zone
-
-`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict
-ISO 8601 format.
-
-`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822
-format, often found in email messages.
-
-`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.
-
-`--date=raw` shows the date as seconds since the epoch (1970-01-01
-00:00:00 UTC), followed by a space, and then the timezone as an offset
-from UTC (a `+` or `-` with four digits; the first two are hours, and
-the second two are minutes). I.e., as if the timestamp were formatted
-with `strftime("%s %z")`).
-Note that the `-local` option does not affect the seconds-since-epoch
-value (which is always measured in UTC), but does switch the accompanying
-timezone value.
-
-`--date=human` shows the timezone if the timezone does not match the
-current time-zone, and doesn't print the whole date if that matches
-(ie skip printing year for dates that are "this year", but also skip
-the whole date itself if it's in the last few days and we can just say
-what weekday it was).  For older dates the hour and minute is also
-omitted.
-
-`--date=unix` shows the date as a Unix epoch timestamp (seconds since
-1970).  As with `--raw`, this is always in UTC and therefore `-local`
-has no effect.
-
-`--date=format:<format>` feeds the _<format>_ to your system `strftime`,
-except for `%s`, `%z`, and `%Z`, which are handled internally.
-Use `--date=format:%c` to show the date in your system locale's
-preferred format.  See the `strftime`(3) manual for a complete list of
-format placeholders. When using `-local`, the correct syntax is
-`--date=format-local:<format>`.
-
-`--date=default` is the default format, and is based on ctime(3)
-output.  It shows a single line with three-letter day of the week,
-three-letter month, day-of-month, hour-minute-seconds in "HH:MM:SS"
-format, followed by 4-digit year, plus timezone information, unless
-the local time zone is used, e.g. `Thu Jan 1 00:00:00 1970 +0000`.
---
+include::rev-list-option-date-alternatives.adoc[]
 
 ifdef::git-rev-list[]
 `--header`::
-- 
2.54.0.22.g9e26862b904

[PATCH 4/4] format-rev: learn --abbrev, --color, and --date

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Add three more options for controlling the formatting.

This does not complete all the pretty formatting knobs for this command
relative to e.g. git-log(1), but it does add the most important ones, in
my opinion. We can see which are missing by taking a look at
`Documentation/pretty-options.adoc`:

• `--encoding=<encoding>`
• `--show-signature`
• `--expand-tabs=<n>`

***

We could add these options to the command synopsis, but let’s instead
simplify the synopsis to just mention the mandatory options and stuff
the other ones into `[<options>]`. I don’t think a long command synopsis
line is useful. And this way the two mandatory options stand out more.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    > We can see which are missing by taking a look at
    
    Or am I missing some?

 Documentation/git-format-rev.adoc | 44 ++++++++++++++++++++++++++++---
 builtin/name-rev.c                | 42 ++++++++++++++++++++---------
 t/t6120-describe.sh               | 44 +++++++++++++++++++++++++++++++
 3 files changed, 115 insertions(+), 15 deletions(-)

diff --git a/Documentation/git-format-rev.adoc b/Documentation/git-format-rev.adoc
index 505a52feccd..1a06ccbf9b8 100644
--- a/Documentation/git-format-rev.adoc
+++ b/Documentation/git-format-rev.adoc
@@ -9,7 +9,7 @@ git-format-rev - EXPERIMENTAL: Pretty format revisions on demand
 SYNOPSIS
 --------
 [synopsis]
-(EXPERIMENTAL!) git format-rev --stdin-mode=<mode> --format=<pretty> [--[no-]notes=<ref>] [-z] [--[no-]null-output] [--[no-]null-input]
+(EXPERIMENTAL!) git format-rev [<options>] --stdin-mode=<mode> --format=<pretty>
 
 DESCRIPTION
 -----------
@@ -33,8 +33,8 @@ OPTIONS
 The argument `rev` is also accepted.
 
 `text`;; Formats all commit object names found in freeform text. These
-	must be full object names, i.e. abbreviated hexadecimal object
-	names will not be interpreted.
+	must be full object names, i.e. abbreviated hexadecimal (_hex_)
+	object names will not be interpreted.
 +
 Anything that is parsed as an object name but that is not found to be a
 commit object name is left alone (echoed).
@@ -76,6 +76,44 @@ This is useful if the output could contain newlines, for example if the
 +
 This is useful if the input revision expressions could contain newlines.
 
+`--color[=<when>]`::
+`--no-color`::
+	Respect color formatting. The default color behavior is
+	`auto`. Bare `--color` is the same as `--color=always`.
++
+Giving `--no-color` is the same as `--color=never`.
++
+_<when>_ must be one of:
++
+--
+`always`;;
+	Always use color, even if the output is something like a file.
+`never`;;
+	Never use color.
+`auto`;;
+	Use color when the output is a terminal but not when the output
+	is something like a file.
+--
+
+`--abbrev[=<n>]`::
+`--no-abbrev`::
+	Abbreviate the commit hex output. Without _<n>_ it will find the
+	minimum length which can describe the commit uniquely, with some
+	extra slack. Giving _<n>_ specifies the minimum length; a longer
+	length will be used if needed.
++
+Giving `--no-abbrev` will turn off abbreviation, showing the full commit
+hex output.
++
+Note that some pretty formats use `--abbrev`. This behavior can be
+controlled with these two options.
+
+`--date=<format>`::
+	Date format for pretty formats. Note that date atoms like `%aI`
+	are not affected. This option cannot be negated.
++
+include::rev-list-option-date-alternatives.adoc[]
+
 [[io]]
 INPUT AND OUTPUT FORMAT
 -----------------------
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 7d824aa1c5d..0c9014ca594 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -21,6 +21,7 @@
 #include "revision.h"
 #include "notes.h"
 #include "write-or-die.h"
+#include "date.h"
 
 /*
  * One day.  See the 'name a rev shortly after epoch' test in t6120 when
@@ -778,6 +779,8 @@ struct format_rev_data {
 	bool nul_input;
 	bool nul_output;
 	struct string_list notes;
+	struct rev_info rev;
+	int color;
 };
 
 static int format_nul_cb(const struct option *option,
@@ -792,6 +795,17 @@ static int format_nul_cb(const struct option *option,
 	return 0;
 }
 
+static int date_cb(const struct option *option,
+		   const char *arg,
+		   int unset)
+{
+	struct rev_info *data = option->value;
+	parse_date_format(arg, &data->date_mode);
+	data->date_mode_explicit = 1;
+	BUG_ON_OPT_NEG(unset);
+	return 0;
+}
+
 static enum stdin_mode parse_stdin_mode(const char *stdin_mode)
 {
 	if (!strcmp(stdin_mode, "text"))
@@ -805,9 +819,8 @@ static enum stdin_mode parse_stdin_mode(const char *stdin_mode)
 }
 
 static char const *const format_rev_usage[] = {
-	N_("(EXPERIMENTAL!) git format-rev --stdin-mode=<mode> "
-	   "--format=<pretty> [--[no-]notes=<ref>] "
-	   "[-z] [--[no-]null-output] [--[no-]null-input]"),
+	N_("(EXPERIMENTAL!) git format-rev [<options>] "
+	   "--stdin-mode=<mode> --format=<pretty>"),
 	NULL
 };
 
@@ -817,13 +830,13 @@ int cmd_format_rev(int argc,
 		   struct repository *repo UNUSED)
 {
 	struct format_rev_data data = {
-		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
+		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP,
+		REV_INFO_INIT, GIT_COLOR_AUTO
 	};
 	enum stdin_mode stdin_mode;
 	char output_terminator;
 	strbuf_getline_fn getline_fn;
 	struct display_notes_opt format_notes_opt;
-	struct rev_info format_rev = REV_INFO_INIT;
 	struct pretty_format format_pp = { 0 };
 	struct strbuf scratch_buf = STRBUF_INIT;
 	struct command cmd;
@@ -834,6 +847,11 @@ int cmd_format_rev(int argc,
 			   N_("how revs are processed")),
 		OPT_STRING_LIST(0, "notes", &data.notes, N_("notes"),
 				N_("display notes for pretty format")),
+		OPT__ABBREV(&data.rev.abbrev),
+		OPT__COLOR(&data.color, N_("use colored output")),
+		OPT_CALLBACK_F(0, "date", &data.rev, N_("date"),
+			       N_("date format"),
+			       PARSE_OPT_NONEG, date_cb),
 		OPT_CALLBACK_F('z', "null", &data, N_("z"),
 			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
@@ -862,13 +880,13 @@ int cmd_format_rev(int argc,
 	init_display_notes(&format_notes_opt);
 	stdin_mode = parse_stdin_mode(data.stdin_mode);
 
-	get_commit_format(data.format, &format_rev);
-	format_pp.ctx.rev = &format_rev;
-	format_pp.ctx.fmt = format_rev.commit_format;
-	format_pp.ctx.abbrev = format_rev.abbrev;
-	format_pp.ctx.date_mode_explicit = format_rev.date_mode_explicit;
-	format_pp.ctx.date_mode = format_rev.date_mode;
-	format_pp.ctx.color = GIT_COLOR_AUTO;
+	get_commit_format(data.format, &data.rev);
+	format_pp.ctx.rev = &data.rev;
+	format_pp.ctx.fmt = data.rev.commit_format;
+	format_pp.ctx.abbrev = data.rev.abbrev;
+	format_pp.ctx.date_mode_explicit = data.rev.date_mode_explicit;
+	format_pp.ctx.date_mode = data.rev.date_mode;
+	format_pp.ctx.color = data.color;
 
 	userformat_find_requirements(data.format,
 				     &format_pp.want);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 7a7c46658a3..2621edb5937 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -1017,4 +1017,48 @@ do
 	'
 done <stdin-modes
 
+format_rev_cmp_log () {
+	opts="$1"
+	format=reference
+	cat >input <<-\EOF &&
+	third
+	second
+	first
+	EOF
+	git -C repo-format log --stdin --no-walk \
+		"$opts" --format="$format" >expect <input &&
+	git -C repo-format format-rev "$opts" \
+		--stdin-mode=revs --format="$format" >actual <input &&
+	test_cmp expect actual
+}
+
+format_rev_err_cmp_log () {
+	opts="$1"
+	format=reference
+	# No input since we ought to fail while parsing options
+	test_must_fail git -C repo-format log --stdin --no-walk \
+		"$opts" --format="$format" 2>expect &&
+	test_must_fail git -C repo-format format-rev "$opts" \
+		--stdin-mode=revs --format="$format" 2>actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'format-rev --color' '
+	format_rev_cmp_log --color=always &&
+	format_rev_cmp_log --color &&
+	format_rev_cmp_log --no-color &&
+	format_rev_err_cmp_log --color=not-valid
+'
+
+test_expect_success 'format-rev --abbrev' '
+	format_rev_cmp_log --abbrev=31 &&
+	format_rev_cmp_log --no-abbrev
+'
+
+test_expect_success 'format-rev --date' '
+	format_rev_cmp_log --date=relative &&
+	format_rev_cmp_log --date=iso-strict &&
+	format_rev_err_cmp_log --date=not-valid
+'
+
 test_done
-- 
2.54.0.22.g9e26862b904

Re: [PATCH 2/4] format-rev: factor option variables into a struct

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>
> We will in two commits add three more options to this command.
> Let’s prepare for that by moving option variables into a struct
> so that we get less local variables.
>
> This allows us to inline `format_nul_data` into this new
> structure. Let’s also rename `stdin_mode_arg` to `stdin_mode`.
> (We couldn’t use `stdin_mode` before because of the enumeration
> with the same name.)
>
> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> ---
>  builtin/name-rev.c | 44 +++++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 21 deletions(-)
>
> diff --git a/builtin/name-rev.c b/builtin/name-rev.c
> index 254c88199fd..7d824aa1c5d 100644
> --- a/builtin/name-rev.c
> +++ b/builtin/name-rev.c
> @@ -772,16 +772,19 @@ int cmd_name_rev(int argc,
>  	return 0;
>  }
>  
> -struct format_nul_data {
> +struct format_rev_data {
> +	const char *format;
> +	const char *stdin_mode;
>  	bool nul_input;
>  	bool nul_output;
> +	struct string_list notes;
>  };
>  
>  static int format_nul_cb(const struct option *option,
>  			 const char *arg,
>  			 int unset)
>  {
> -	struct format_nul_data *data = option->value;
> +	struct format_rev_data *data = option->value;
>  	data->nul_input = 1;
>  	data->nul_output = 1;
>  	BUG_ON_OPT_NEG(unset);
> @@ -813,31 +816,30 @@ int cmd_format_rev(int argc,
>  		   const char *prefix,
>  		   struct repository *repo UNUSED)
>  {
> -	const char *format = NULL;
> +	struct format_rev_data data = {
> +		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
> +	};

It will make it easier to maintain if you used designated
initializer here, i.e.,

	struct format_rev_data data = {
		.notes = STRING_LIST_INIT_NODUP,
	};

The other members not explicitly mentioned by the initializer will
be zero-initialized.

Other parts of the patch look good.

Thanks.

Re: [PATCH 2/4] format-rev: factor option variables into a struct

[Kristoffer Haugsbakk]

On Thu, Aug 13, 2026, at 20:21, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>> From: Kristoffer Haugsbakk <code@khaugsbakk.name>
>>[snip]
>> -	const char *format = NULL;
>> +	struct format_rev_data data = {
>> +		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
>> +	};
>
> It will make it easier to maintain if you used designated
> initializer here, i.e.,
>
> 	struct format_rev_data data = {
> 		.notes = STRING_LIST_INIT_NODUP,
> 	};
>

That is excellent. Thanks!

>[snip]

Re: [PATCH 4/4] format-rev: learn --abbrev, --color, and --date

[Junio C Hamano]

kristofferhaugsbakk@fastmail.com writes:

> +static int date_cb(const struct option *option,
> +		   const char *arg,
> +		   int unset)
> +{
> +	struct rev_info *data = option->value;
> +	parse_date_format(arg, &data->date_mode);
> +	data->date_mode_explicit = 1;
> +	BUG_ON_OPT_NEG(unset);
> +	return 0;
> +}

This BUG_ON_OPT_NEG(unset) is a bit curious and confusing to me.  If
the caller could pass unset==1 (e.g., "--no-date"), option->value
would be NULL, and we would already have dereferenced data->date_mode
when preparing to call parse_date_format().

On the other hand, ...

> +		OPT_CALLBACK_F(0, "date", &data.rev, N_("date"),
> +			       N_("date format"),
> +			       PARSE_OPT_NONEG, date_cb),

... because we mark the option entry with PARSE_OPT_NONEG,
"--no-date" would not cause date_cb() to be called with unset==1.

I guess, from existing uses of BUG_ON_OPT_NEG() elsewhere (like
apply.c), that the intention is to notice when this callback
function is broken by future changes, i.e., somebody careless makes
the calling parse_options(), or an additional side caller that calls
this callback directly, pass unset==1 and option->value==NULL
combinations.  But then the assertion should come before the first
potentially problematic use, i.e., in this order:

	struct rev_info *data = option->value;

	BUG_ON_OPT_NEG(unset);
	parse_date_format(arg, &data->date_mode);
	data->date_mode_explicit = 1;
	return 0;

or the assertion will not trigger before the code segfaults, no?

Thanks.

Re: [PATCH 4/4] format-rev: learn --abbrev, --color, and --date

[Kristoffer Haugsbakk]

On Sat, Aug 15, 2026, at 04:17, Junio C Hamano wrote:
> kristofferhaugsbakk@fastmail.com writes:
>
>> +static int date_cb(const struct option *option,
>> +		   const char *arg,
>> +		   int unset)
>> +{
>> +	struct rev_info *data = option->value;
>> +	parse_date_format(arg, &data->date_mode);
>> +	data->date_mode_explicit = 1;
>> +	BUG_ON_OPT_NEG(unset);
>> +	return 0;
>> +}
>
> This BUG_ON_OPT_NEG(unset) is a bit curious and confusing to me.  If
> the caller could pass unset==1 (e.g., "--no-date"), option->value
> would be NULL, and we would already have dereferenced data->date_mode
> when preparing to call parse_date_format().

Well spotted.

>
> On the other hand, ...
>
>> +		OPT_CALLBACK_F(0, "date", &data.rev, N_("date"),
>> +			       N_("date format"),
>> +			       PARSE_OPT_NONEG, date_cb),
>
> ... because we mark the option entry with PARSE_OPT_NONEG,
> "--no-date" would not cause date_cb() to be called with unset==1.
>
> I guess, from existing uses of BUG_ON_OPT_NEG() elsewhere (like
> apply.c), that the intention is to notice when this callback
> function is broken by future changes, i.e., somebody careless makes
> the calling parse_options(), or an additional side caller that calls
> this callback directly, pass unset==1 and option->value==NULL
> combinations.  But then the assertion should come before the first
> potentially problematic use, i.e., in this order:

This is totally a monkey see and moneky do situation. Mirroring
parse-options flags as `BUG` statements. Down to the outright wrong
assertion/BUG placement. So this needs to be changed

I have these same statements on the existing callback, for `--null`:


    static int format_nul_cb(const struct option *option,
                             const char *arg,
                             int unset)
    {
            struct format_rev_data *data = option->value;
            data->nul_input = 1;
            data->nul_output = 1;
            BUG_ON_OPT_NEG(unset);
            BUG_ON_OPT_ARG(arg);
            return 0;
    }

But this does not have the `NULL` deref. problem since we just
unconditionally set two boolean values. Still, for readability it’s
better for these two statements to go at the start. Since they are
preconditions. I will add this as a patch/commit to the series.

>
> 	struct rev_info *data = option->value;
>
> 	BUG_ON_OPT_NEG(unset);
> 	parse_date_format(arg, &data->date_mode);
> 	data->date_mode_explicit = 1;
> 	return 0;
>
> or the assertion will not trigger before the code segfaults, no?

Re: [PATCH 4/4] format-rev: learn --abbrev, --color, and --date

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>     static int format_nul_cb(const struct option *option,
>                              const char *arg,
>                              int unset)
>     {
>             struct format_rev_data *data = option->value;
>             data->nul_input = 1;
>             data->nul_output = 1;
>             BUG_ON_OPT_NEG(unset);
>             BUG_ON_OPT_ARG(arg);
>             return 0;
>     }
>
> But this does not have the `NULL` deref. problem since we just
> unconditionally set two boolean values. Still, for readability it’s
> better for these two statements to go at the start. Since they are
> preconditions. I will add this as a patch/commit to the series.

Yeah, it would be a good idea to establish the "validate before
doing anything with the parameters" pattern.

Thanks.

Re: [PATCH 4/4] format-rev: learn --abbrev, --color, and --date

[Kristoffer Haugsbakk]

On Thu, Aug 13, 2026, at 19:24, kristofferhaugsbakk@fastmail.com wrote:
>[snip]
> +static int date_cb(const struct option *option,
> +		   const char *arg,
> +		   int unset)
> +{
> +	struct rev_info *data = option->value;
> +	parse_date_format(arg, &data->date_mode);

The documentation for this function says that we need to call a release
function in case a custom format was used. That is currently missing.

> +	data->date_mode_explicit = 1;
> +	BUG_ON_OPT_NEG(unset);
> +	return 0;
> +}
> +
>[snip]

[PATCH v2 0/5] format-rev: add --abbrev, --color, and --date

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Topic name (applied): kh/format-rev-more-options

Topic summary: Add three more options for controlling the formatting. Also
do some minor refactoring and text fixes as preparatory steps.

§ Changes in v2

See the patch notes for details.

• Use designated initializer syntax. That’s more readable since you pair
  the field with the value and you can omit zero-value fields.

  https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/
• Fix useless `BUG` placements https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/
• Add preliminary patch “place BUG calls first in callback” for existing
  `BUG` statement placement
  • Based on the previous point
• Patch “learn --abbrev, --color, and --date”: test a few more options

[1/5] format-rev: use lower case for opts description
[2/5] format-rev: place BUG calls first in callback
[3/5] format-rev: factor option variables into a struct
[4/5] doc: rev-list-options.adoc: factor out --date alts
[5/5] format-rev: learn --abbrev, --color, and --date

 Documentation/git-format-rev.adoc             | 44 ++++++++-
 .../rev-list-option-date-alternatives.adoc    | 55 +++++++++++
 Documentation/rev-list-options.adoc           | 56 +----------
 builtin/name-rev.c                            | 92 ++++++++++++-------
 t/t6120-describe.sh                           | 58 ++++++++++++
 5 files changed, 212 insertions(+), 93 deletions(-)
 create mode 100644 Documentation/rev-list-option-date-alternatives.adoc

Interdiff against v1:
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 0c9014ca594..fa20a2774be 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -788,10 +788,10 @@ static int format_nul_cb(const struct option *option,
 			 int unset)
 {
 	struct format_rev_data *data = option->value;
-	data->nul_input = 1;
-	data->nul_output = 1;
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
+	data->nul_input = 1;
+	data->nul_output = 1;
 	return 0;
 }
 
@@ -800,9 +800,9 @@ static int date_cb(const struct option *option,
 		   int unset)
 {
 	struct rev_info *data = option->value;
+	BUG_ON_OPT_NEG(unset);
 	parse_date_format(arg, &data->date_mode);
 	data->date_mode_explicit = 1;
-	BUG_ON_OPT_NEG(unset);
 	return 0;
 }
 
@@ -830,8 +830,9 @@ int cmd_format_rev(int argc,
 		   struct repository *repo UNUSED)
 {
 	struct format_rev_data data = {
-		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP,
-		REV_INFO_INIT, GIT_COLOR_AUTO
+		.notes = STRING_LIST_INIT_NODUP,
+		.rev = REV_INFO_INIT,
+		.color = GIT_COLOR_AUTO,
 	};
 	enum stdin_mode stdin_mode;
 	char output_terminator;
@@ -953,6 +954,7 @@ int cmd_format_rev(int argc,
 		BUG("uncovered case: %d", stdin_mode);
 	}
 
+	date_mode_release(&data.rev.date_mode);
 	strbuf_release(&scratch_buf);
 	string_list_clear(&data.notes, 0);
 	release_display_notes(&format_notes_opt);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 2621edb5937..a15da979abf 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -1026,9 +1026,9 @@ format_rev_cmp_log () {
 	first
 	EOF
 	git -C repo-format log --stdin --no-walk \
-		"$opts" --format="$format" >expect <input &&
-	git -C repo-format format-rev "$opts" \
-		--stdin-mode=revs --format="$format" >actual <input &&
+		--format="$format" "$opts" >expect <input &&
+	git -C repo-format format-rev --stdin-mode=revs \
+		--format="$format" "$opts" >actual <input &&
 	test_cmp expect actual
 }
 
@@ -1037,9 +1037,9 @@ format_rev_err_cmp_log () {
 	format=reference
 	# No input since we ought to fail while parsing options
 	test_must_fail git -C repo-format log --stdin --no-walk \
-		"$opts" --format="$format" 2>expect &&
-	test_must_fail git -C repo-format format-rev "$opts" \
-		--stdin-mode=revs --format="$format" 2>actual &&
+		--format="$format" "$opts" 2>expect &&
+	test_must_fail git -C repo-format format-rev \
+		--stdin-mode=revs --format="$format" "$opts" 2>actual &&
 	test_cmp expect actual
 }
 
@@ -1051,6 +1051,7 @@ test_expect_success 'format-rev --color' '
 '
 
 test_expect_success 'format-rev --abbrev' '
+	format_rev_cmp_log --abbrev &&
 	format_rev_cmp_log --abbrev=31 &&
 	format_rev_cmp_log --no-abbrev
 '
@@ -1058,7 +1059,20 @@ test_expect_success 'format-rev --abbrev' '
 test_expect_success 'format-rev --date' '
 	format_rev_cmp_log --date=relative &&
 	format_rev_cmp_log --date=iso-strict &&
-	format_rev_err_cmp_log --date=not-valid
+	# This also tests the only case where we need to release
+	# the data for the parsed format
+	format_rev_cmp_log --date="format:%c" &&
+	format_rev_err_cmp_log --date=not-valid &&
+	# Test --date (no arg) next
+	# We cannot compare the output to git-log(1)
+	# because that command uses a slightly different
+	# error message (different library)
+	cat >expect <<-EOF &&
+	error: option \`date${SQ} requires a value
+	EOF
+	test_must_fail git -C repo-format format-rev \
+		--stdin-mode=revs --format="$format" --date 2>actual &&
+	test_cmp expect actual
 '
 
 test_done
Range-diff against v1:
1:  eb84b1b6341 = 1:  eb84b1b6341 format-rev: use lower case for opts description
-:  ----------- > 2:  2cb12e3ce48 format-rev: place BUG calls first in callback
2:  278eb852121 ! 3:  0b653b1d218 format-rev: factor option variables into a struct
    @@ builtin/name-rev.c: int cmd_name_rev(int argc,
      {
     -	struct format_nul_data *data = option->value;
     +	struct format_rev_data *data = option->value;
    - 	data->nul_input = 1;
    - 	data->nul_output = 1;
      	BUG_ON_OPT_NEG(unset);
    + 	BUG_ON_OPT_ARG(arg);
    + 	data->nul_input = 1;
     @@ builtin/name-rev.c: int cmd_format_rev(int argc,
      		   const char *prefix,
      		   struct repository *repo UNUSED)
      {
     -	const char *format = NULL;
     +	struct format_rev_data data = {
    -+		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
    ++		.notes = STRING_LIST_INIT_NODUP,
     +	};
      	enum stdin_mode stdin_mode;
     -	const char *stdin_mode_arg = NULL;
3:  cb2cc772b31 = 4:  7556bf04462 doc: rev-list-options.adoc: factor out --date alts
4:  e6d3e14c692 ! 5:  d1bcad06e24 format-rev: learn --abbrev, --color, and --date
    @@ builtin/name-rev.c: static int format_nul_cb(const struct option *option,
     +		   int unset)
     +{
     +	struct rev_info *data = option->value;
    ++	BUG_ON_OPT_NEG(unset);
     +	parse_date_format(arg, &data->date_mode);
     +	data->date_mode_explicit = 1;
    -+	BUG_ON_OPT_NEG(unset);
     +	return 0;
     +}
     +
    @@ builtin/name-rev.c: static enum stdin_mode parse_stdin_mode(const char *stdin_mo
      };
      
     @@ builtin/name-rev.c: int cmd_format_rev(int argc,
    - 		   struct repository *repo UNUSED)
      {
      	struct format_rev_data data = {
    --		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP
    -+		NULL, NULL, 0, 0, STRING_LIST_INIT_NODUP,
    -+		REV_INFO_INIT, GIT_COLOR_AUTO
    + 		.notes = STRING_LIST_INIT_NODUP,
    ++		.rev = REV_INFO_INIT,
    ++		.color = GIT_COLOR_AUTO,
      	};
      	enum stdin_mode stdin_mode;
      	char output_terminator;
    @@ builtin/name-rev.c: int cmd_format_rev(int argc,
      
      	userformat_find_requirements(data.format,
      				     &format_pp.want);
    +@@ builtin/name-rev.c: int cmd_format_rev(int argc,
    + 		BUG("uncovered case: %d", stdin_mode);
    + 	}
    + 
    ++	date_mode_release(&data.rev.date_mode);
    + 	strbuf_release(&scratch_buf);
    + 	string_list_clear(&data.notes, 0);
    + 	release_display_notes(&format_notes_opt);
     
      ## t/t6120-describe.sh ##
     @@ t/t6120-describe.sh: do
    @@ t/t6120-describe.sh: do
     +	first
     +	EOF
     +	git -C repo-format log --stdin --no-walk \
    -+		"$opts" --format="$format" >expect <input &&
    -+	git -C repo-format format-rev "$opts" \
    -+		--stdin-mode=revs --format="$format" >actual <input &&
    ++		--format="$format" "$opts" >expect <input &&
    ++	git -C repo-format format-rev --stdin-mode=revs \
    ++		--format="$format" "$opts" >actual <input &&
     +	test_cmp expect actual
     +}
     +
    @@ t/t6120-describe.sh: do
     +	format=reference
     +	# No input since we ought to fail while parsing options
     +	test_must_fail git -C repo-format log --stdin --no-walk \
    -+		"$opts" --format="$format" 2>expect &&
    -+	test_must_fail git -C repo-format format-rev "$opts" \
    -+		--stdin-mode=revs --format="$format" 2>actual &&
    ++		--format="$format" "$opts" 2>expect &&
    ++	test_must_fail git -C repo-format format-rev \
    ++		--stdin-mode=revs --format="$format" "$opts" 2>actual &&
     +	test_cmp expect actual
     +}
     +
    @@ t/t6120-describe.sh: do
     +'
     +
     +test_expect_success 'format-rev --abbrev' '
    ++	format_rev_cmp_log --abbrev &&
     +	format_rev_cmp_log --abbrev=31 &&
     +	format_rev_cmp_log --no-abbrev
     +'
    @@ t/t6120-describe.sh: do
     +test_expect_success 'format-rev --date' '
     +	format_rev_cmp_log --date=relative &&
     +	format_rev_cmp_log --date=iso-strict &&
    -+	format_rev_err_cmp_log --date=not-valid
    ++	# This also tests the only case where we need to release
    ++	# the data for the parsed format
    ++	format_rev_cmp_log --date="format:%c" &&
    ++	format_rev_err_cmp_log --date=not-valid &&
    ++	# Test --date (no arg) next
    ++	# We cannot compare the output to git-log(1)
    ++	# because that command uses a slightly different
    ++	# error message (different library)
    ++	cat >expect <<-EOF &&
    ++	error: option \`date${SQ} requires a value
    ++	EOF
    ++	test_must_fail git -C repo-format format-rev \
    ++		--stdin-mode=revs --format="$format" --date 2>actual &&
    ++	test_cmp expect actual
     +'
     +
      test_done

base-commit: 010afd3166ddc64c9863b1506f12cbcdda0d4ea1
-- 
2.55.0.13.g85d2d65e389

[PATCH v2 1/5] format-rev: use lower case for opts description

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

The option descriptions use a mix of initial capital and lower case
letters. Lower case is the correct style.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 builtin/name-rev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 60cbbfb4b7d..254c88199fd 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -833,12 +833,12 @@ int cmd_format_rev(int argc,
 		OPT_STRING_LIST(0, "notes", &notes, N_("notes"),
 				N_("display notes for pretty format")),
 		OPT_CALLBACK_F('z', "null", &nul_data, N_("z"),
-			       N_("Use NUL for input and output termination"),
+			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
 		OPT_BOOL(0, "null-input", &nul_data.nul_input,
-			 N_("Use NUL for input termination")),
+			 N_("use NUL for input termination")),
 		OPT_BOOL(0, "null-output", &nul_data.nul_output,
-			 N_("Use NUL for output termination")),
+			 N_("use NUL for output termination")),
 		OPT_END(),
 	};
 
-- 
2.55.0.13.g85d2d65e389

[PATCH v2 2/5] format-rev: place BUG calls first in callback

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

I added these parse-options `BUG` statements based on existing examples;
one `BUG` check per flag. Now, of course the code as-is will not call
this callback with `unset` set to `0`, or with an argument
string. Rather, these preconditions defend against `opts[]` getting
changed *without* changing this callback.

And I copied the existing examples that I found down to
the placement. And the placement doesn’t matter here; we just
unconditionally set two variables. Failing on `BUG` before or after
that makes no difference to the user. Still, it is better style to
test function preconditions as early as possible. So let’s move them
to the start.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2: • [new]
        • https://lore.kernel.org/git/0bd9c642-9e88-4c82-81ee-20fdeb3c2797@app.fastmail.com/

 builtin/name-rev.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 254c88199fd..d6686bbdbb9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -782,10 +782,10 @@ static int format_nul_cb(const struct option *option,
 			 int unset)
 {
 	struct format_nul_data *data = option->value;
-	data->nul_input = 1;
-	data->nul_output = 1;
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
+	data->nul_input = 1;
+	data->nul_output = 1;
 	return 0;
 }
 
-- 
2.55.0.13.g85d2d65e389

[PATCH v2 3/5] format-rev: factor option variables into a struct

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

We will in two commits add three more options to this command.
Let’s prepare for that by moving option variables into a struct
so that we get less local variables.

This allows us to inline `format_nul_data` into this new
structure. Let’s also rename `stdin_mode_arg` to `stdin_mode`.
(We couldn’t use `stdin_mode` before because of the enumeration
with the same name.)

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    • Designated initializers: https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/

 builtin/name-rev.c | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index d6686bbdbb9..c8cb2f2d520 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -772,16 +772,19 @@ int cmd_name_rev(int argc,
 	return 0;
 }
 
-struct format_nul_data {
+struct format_rev_data {
+	const char *format;
+	const char *stdin_mode;
 	bool nul_input;
 	bool nul_output;
+	struct string_list notes;
 };
 
 static int format_nul_cb(const struct option *option,
 			 const char *arg,
 			 int unset)
 {
-	struct format_nul_data *data = option->value;
+	struct format_rev_data *data = option->value;
 	BUG_ON_OPT_NEG(unset);
 	BUG_ON_OPT_ARG(arg);
 	data->nul_input = 1;
@@ -813,31 +816,30 @@ int cmd_format_rev(int argc,
 		   const char *prefix,
 		   struct repository *repo UNUSED)
 {
-	const char *format = NULL;
+	struct format_rev_data data = {
+		.notes = STRING_LIST_INIT_NODUP,
+	};
 	enum stdin_mode stdin_mode;
-	const char *stdin_mode_arg = NULL;
-	struct format_nul_data nul_data = { 0, 0 };
 	char output_terminator;
 	strbuf_getline_fn getline_fn;
 	struct display_notes_opt format_notes_opt;
 	struct rev_info format_rev = REV_INFO_INIT;
 	struct pretty_format format_pp = { 0 };
-	struct string_list notes = STRING_LIST_INIT_NODUP;
 	struct strbuf scratch_buf = STRBUF_INIT;
 	struct command cmd;
 	struct option opts[] = {
-		OPT_STRING(0, "format", &format, N_("format"),
+		OPT_STRING(0, "format", &data.format, N_("format"),
 			   N_("pretty format to use")),
-		OPT_STRING(0, "stdin-mode", &stdin_mode_arg, N_("stdin-mode"),
+		OPT_STRING(0, "stdin-mode", &data.stdin_mode, N_("stdin-mode"),
 			   N_("how revs are processed")),
-		OPT_STRING_LIST(0, "notes", &notes, N_("notes"),
+		OPT_STRING_LIST(0, "notes", &data.notes, N_("notes"),
 				N_("display notes for pretty format")),
-		OPT_CALLBACK_F('z', "null", &nul_data, N_("z"),
+		OPT_CALLBACK_F('z', "null", &data, N_("z"),
 			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
-		OPT_BOOL(0, "null-input", &nul_data.nul_input,
+		OPT_BOOL(0, "null-input", &data.nul_input,
 			 N_("use NUL for input termination")),
-		OPT_BOOL(0, "null-output", &nul_data.nul_output,
+		OPT_BOOL(0, "null-output", &data.nul_output,
 			 N_("use NUL for output termination")),
 		OPT_END(),
 	};
@@ -849,18 +851,18 @@ int cmd_format_rev(int argc,
 		usage_with_options(format_rev_usage, opts);
 	}
 
-	if (!format)
+	if (!data.format)
 		die(_("'%s' is required"), "--format");
-	if (!stdin_mode_arg)
+	if (!data.stdin_mode)
 		die(_("'%s' is required"), "--stdin-mode");
 
-	getline_fn = nul_data.nul_input ? strbuf_getline_nul : strbuf_getline_lf;
-	output_terminator = nul_data.nul_output ? '\0' : '\n';
+	getline_fn = data.nul_input ? strbuf_getline_nul : strbuf_getline_lf;
+	output_terminator = data.nul_output ? '\0' : '\n';
 
 	init_display_notes(&format_notes_opt);
-	stdin_mode = parse_stdin_mode(stdin_mode_arg);
+	stdin_mode = parse_stdin_mode(data.stdin_mode);
 
-	get_commit_format(format, &format_rev);
+	get_commit_format(data.format, &format_rev);
 	format_pp.ctx.rev = &format_rev;
 	format_pp.ctx.fmt = format_rev.commit_format;
 	format_pp.ctx.abbrev = format_rev.abbrev;
@@ -868,13 +870,13 @@ int cmd_format_rev(int argc,
 	format_pp.ctx.date_mode = format_rev.date_mode;
 	format_pp.ctx.color = GIT_COLOR_AUTO;
 
-	userformat_find_requirements(format,
+	userformat_find_requirements(data.format,
 				     &format_pp.want);
 	if (format_pp.want.notes) {
 		int ignore_show_notes = 0;
 		struct string_list_item *n;
 
-		for_each_string_list_item(n, &notes)
+		for_each_string_list_item(n, &data.notes)
 			enable_ref_display_notes(&format_notes_opt,
 						 &ignore_show_notes,
 						 n->string);
@@ -934,7 +936,7 @@ int cmd_format_rev(int argc,
 	}
 
 	strbuf_release(&scratch_buf);
-	string_list_clear(&notes, 0);
+	string_list_clear(&data.notes, 0);
 	release_display_notes(&format_notes_opt);
 	return 0;
 }
-- 
2.55.0.13.g85d2d65e389

[PATCH v2 4/5] doc: rev-list-options.adoc: factor out --date alts

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

We will introduce `--date` to git-format-rev(1) in the next commit and
will need to add it to the documentation. Let’s factor out the option
alternatives so that it can be included in git-format-rev(1).

The initial paragraph of this option mentions things like git-log(1).
We could make it fit in git-format-rev(1) while not changing it for
git-rev-list(1) and related commands with some conditionals like
`ifndef`, but writing a dedicated paragraph is simple enough.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---
 .../rev-list-option-date-alternatives.adoc    | 55 ++++++++++++++++++
 Documentation/rev-list-options.adoc           | 56 +------------------
 2 files changed, 56 insertions(+), 55 deletions(-)
 create mode 100644 Documentation/rev-list-option-date-alternatives.adoc

diff --git a/Documentation/rev-list-option-date-alternatives.adoc b/Documentation/rev-list-option-date-alternatives.adoc
new file mode 100644
index 00000000000..141570b1059
--- /dev/null
+++ b/Documentation/rev-list-option-date-alternatives.adoc
@@ -0,0 +1,55 @@
+--
+`--date=relative` shows dates relative to the current time,
+e.g. ``2 hours ago''. The `-local` option has no effect for
+`--date=relative`.
+
+`--date=local` is an alias for `--date=default-local`.
+
+`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
+The differences to the strict ISO 8601 format are:
+
+	- a space instead of the `T` date/time delimiter
+	- a space between time and time zone
+	- no colon between hours and minutes of the time zone
+
+`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict
+ISO 8601 format.
+
+`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822
+format, often found in email messages.
+
+`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.
+
+`--date=raw` shows the date as seconds since the epoch (1970-01-01
+00:00:00 UTC), followed by a space, and then the timezone as an offset
+from UTC (a `+` or `-` with four digits; the first two are hours, and
+the second two are minutes). I.e., as if the timestamp were formatted
+with `strftime("%s %z")`).
+Note that the `-local` option does not affect the seconds-since-epoch
+value (which is always measured in UTC), but does switch the accompanying
+timezone value.
+
+`--date=human` shows the timezone if the timezone does not match the
+current time-zone, and doesn't print the whole date if that matches
+(ie skip printing year for dates that are "this year", but also skip
+the whole date itself if it's in the last few days and we can just say
+what weekday it was).  For older dates the hour and minute is also
+omitted.
+
+`--date=unix` shows the date as a Unix epoch timestamp (seconds since
+1970).  As with `--raw`, this is always in UTC and therefore `-local`
+has no effect.
+
+`--date=format:<format>` feeds the _<format>_ to your system `strftime`,
+except for `%s`, `%z`, and `%Z`, which are handled internally.
+Use `--date=format:%c` to show the date in your system locale's
+preferred format.  See the `strftime`(3) manual for a complete list of
+format placeholders. When using `-local`, the correct syntax is
+`--date=format-local:<format>`.
+
+`--date=default` is the default format, and is based on ctime(3)
+output.  It shows a single line with three-letter day of the week,
+three-letter month, day-of-month, hour-minute-seconds in "HH:MM:SS"
+format, followed by 4-digit year, plus timezone information, unless
+the local time zone is used, e.g. `Thu Jan 1 00:00:00 1970 +0000`.
+--
diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index fd831f0ec64..6e6093f4747 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -1132,61 +1132,7 @@ include::pretty-options.adoc[]
 	author's). If `-local` is appended to the format (e.g.,
 	`iso-local`), the user's local time zone is used instead.
 +
---
-`--date=relative` shows dates relative to the current time,
-e.g. ``2 hours ago''. The `-local` option has no effect for
-`--date=relative`.
-
-`--date=local` is an alias for `--date=default-local`.
-
-`--date=iso` (or `--date=iso8601`) shows timestamps in a ISO 8601-like format.
-The differences to the strict ISO 8601 format are:
-
-	- a space instead of the `T` date/time delimiter
-	- a space between time and time zone
-	- no colon between hours and minutes of the time zone
-
-`--date=iso-strict` (or `--date=iso8601-strict`) shows timestamps in strict
-ISO 8601 format.
-
-`--date=rfc` (or `--date=rfc2822`) shows timestamps in RFC 2822
-format, often found in email messages.
-
-`--date=short` shows only the date, but not the time, in `YYYY-MM-DD` format.
-
-`--date=raw` shows the date as seconds since the epoch (1970-01-01
-00:00:00 UTC), followed by a space, and then the timezone as an offset
-from UTC (a `+` or `-` with four digits; the first two are hours, and
-the second two are minutes). I.e., as if the timestamp were formatted
-with `strftime("%s %z")`).
-Note that the `-local` option does not affect the seconds-since-epoch
-value (which is always measured in UTC), but does switch the accompanying
-timezone value.
-
-`--date=human` shows the timezone if the timezone does not match the
-current time-zone, and doesn't print the whole date if that matches
-(ie skip printing year for dates that are "this year", but also skip
-the whole date itself if it's in the last few days and we can just say
-what weekday it was).  For older dates the hour and minute is also
-omitted.
-
-`--date=unix` shows the date as a Unix epoch timestamp (seconds since
-1970).  As with `--raw`, this is always in UTC and therefore `-local`
-has no effect.
-
-`--date=format:<format>` feeds the _<format>_ to your system `strftime`,
-except for `%s`, `%z`, and `%Z`, which are handled internally.
-Use `--date=format:%c` to show the date in your system locale's
-preferred format.  See the `strftime`(3) manual for a complete list of
-format placeholders. When using `-local`, the correct syntax is
-`--date=format-local:<format>`.
-
-`--date=default` is the default format, and is based on ctime(3)
-output.  It shows a single line with three-letter day of the week,
-three-letter month, day-of-month, hour-minute-seconds in "HH:MM:SS"
-format, followed by 4-digit year, plus timezone information, unless
-the local time zone is used, e.g. `Thu Jan 1 00:00:00 1970 +0000`.
---
+include::rev-list-option-date-alternatives.adoc[]
 
 ifdef::git-rev-list[]
 `--header`::
-- 
2.55.0.13.g85d2d65e389

[PATCH v2 5/5] format-rev: learn --abbrev, --color, and --date

[kristofferhaugsbakk]

From: Kristoffer Haugsbakk <code@khaugsbakk.name>

Add three more options for controlling the formatting.

This does not complete all the pretty formatting knobs for this command
relative to e.g. git-log(1), but it does add the most important ones, in
my opinion. We can see which are missing by taking a look at
`Documentation/pretty-options.adoc`:

• `--encoding=<encoding>`
• `--show-signature`
• `--expand-tabs=<n>`

***

We could add these options to the command synopsis, but let’s instead
simplify the synopsis to just mention the mandatory options and stuff
the other ones into `[<options>]`. I don’t think a long command synopsis
line is useful. And this way the two mandatory options stand out more.

Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
---

Notes (series):
    v2:
    • Designated initializers: https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/
    • Fix useless `BUG` placements https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/
    • Add a few more tests for different option arguments (or no args)
    • And the new tests revealed that I needed to change the helper
      functions so that the option (`opts`) goes last. Or else we
      couldn’t test bare `--date` (no arg, error) because of
      inconsistent ordering between log/format-rev and this:
    
          $ git format-rev --date --stdin-mode=revs
          fatal: unknown date format --stdin-mode=revs
    • Release `date_mode`: https://lore.kernel.org/git/3a55c58f-1ada-414c-a35d-40590c635b82@app.fastmail.com/
    
      I wondered if I would need a `goto cleanup` in order to deal with cases
      like this:
    
          ... --date=format:%c --format
    
      In other words, the command fails because we are missing an
      argument to `--format` but we have at that point already called
      the parse function. But my leakcheck setup didn’t call out any
      errors:
    
          CC = clang
          SANITIZE = address
          CFLAGS = -O1 -g3 -fno-omit-frame-pointer
          NO_GETTEXT = 1
    ---
    v1:
    > We can see which are missing by taking a look at
    
    Or am I missing some?

 Documentation/git-format-rev.adoc | 44 +++++++++++++++++++++--
 builtin/name-rev.c                | 42 ++++++++++++++++------
 t/t6120-describe.sh               | 58 +++++++++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-format-rev.adoc b/Documentation/git-format-rev.adoc
index 505a52feccd..1a06ccbf9b8 100644
--- a/Documentation/git-format-rev.adoc
+++ b/Documentation/git-format-rev.adoc
@@ -9,7 +9,7 @@ git-format-rev - EXPERIMENTAL: Pretty format revisions on demand
 SYNOPSIS
 --------
 [synopsis]
-(EXPERIMENTAL!) git format-rev --stdin-mode=<mode> --format=<pretty> [--[no-]notes=<ref>] [-z] [--[no-]null-output] [--[no-]null-input]
+(EXPERIMENTAL!) git format-rev [<options>] --stdin-mode=<mode> --format=<pretty>
 
 DESCRIPTION
 -----------
@@ -33,8 +33,8 @@ OPTIONS
 The argument `rev` is also accepted.
 
 `text`;; Formats all commit object names found in freeform text. These
-	must be full object names, i.e. abbreviated hexadecimal object
-	names will not be interpreted.
+	must be full object names, i.e. abbreviated hexadecimal (_hex_)
+	object names will not be interpreted.
 +
 Anything that is parsed as an object name but that is not found to be a
 commit object name is left alone (echoed).
@@ -76,6 +76,44 @@ This is useful if the output could contain newlines, for example if the
 +
 This is useful if the input revision expressions could contain newlines.
 
+`--color[=<when>]`::
+`--no-color`::
+	Respect color formatting. The default color behavior is
+	`auto`. Bare `--color` is the same as `--color=always`.
++
+Giving `--no-color` is the same as `--color=never`.
++
+_<when>_ must be one of:
++
+--
+`always`;;
+	Always use color, even if the output is something like a file.
+`never`;;
+	Never use color.
+`auto`;;
+	Use color when the output is a terminal but not when the output
+	is something like a file.
+--
+
+`--abbrev[=<n>]`::
+`--no-abbrev`::
+	Abbreviate the commit hex output. Without _<n>_ it will find the
+	minimum length which can describe the commit uniquely, with some
+	extra slack. Giving _<n>_ specifies the minimum length; a longer
+	length will be used if needed.
++
+Giving `--no-abbrev` will turn off abbreviation, showing the full commit
+hex output.
++
+Note that some pretty formats use `--abbrev`. This behavior can be
+controlled with these two options.
+
+`--date=<format>`::
+	Date format for pretty formats. Note that date atoms like `%aI`
+	are not affected. This option cannot be negated.
++
+include::rev-list-option-date-alternatives.adoc[]
+
 [[io]]
 INPUT AND OUTPUT FORMAT
 -----------------------
diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index c8cb2f2d520..fa20a2774be 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -21,6 +21,7 @@
 #include "revision.h"
 #include "notes.h"
 #include "write-or-die.h"
+#include "date.h"
 
 /*
  * One day.  See the 'name a rev shortly after epoch' test in t6120 when
@@ -778,6 +779,8 @@ struct format_rev_data {
 	bool nul_input;
 	bool nul_output;
 	struct string_list notes;
+	struct rev_info rev;
+	int color;
 };
 
 static int format_nul_cb(const struct option *option,
@@ -792,6 +795,17 @@ static int format_nul_cb(const struct option *option,
 	return 0;
 }
 
+static int date_cb(const struct option *option,
+		   const char *arg,
+		   int unset)
+{
+	struct rev_info *data = option->value;
+	BUG_ON_OPT_NEG(unset);
+	parse_date_format(arg, &data->date_mode);
+	data->date_mode_explicit = 1;
+	return 0;
+}
+
 static enum stdin_mode parse_stdin_mode(const char *stdin_mode)
 {
 	if (!strcmp(stdin_mode, "text"))
@@ -805,9 +819,8 @@ static enum stdin_mode parse_stdin_mode(const char *stdin_mode)
 }
 
 static char const *const format_rev_usage[] = {
-	N_("(EXPERIMENTAL!) git format-rev --stdin-mode=<mode> "
-	   "--format=<pretty> [--[no-]notes=<ref>] "
-	   "[-z] [--[no-]null-output] [--[no-]null-input]"),
+	N_("(EXPERIMENTAL!) git format-rev [<options>] "
+	   "--stdin-mode=<mode> --format=<pretty>"),
 	NULL
 };
 
@@ -818,12 +831,13 @@ int cmd_format_rev(int argc,
 {
 	struct format_rev_data data = {
 		.notes = STRING_LIST_INIT_NODUP,
+		.rev = REV_INFO_INIT,
+		.color = GIT_COLOR_AUTO,
 	};
 	enum stdin_mode stdin_mode;
 	char output_terminator;
 	strbuf_getline_fn getline_fn;
 	struct display_notes_opt format_notes_opt;
-	struct rev_info format_rev = REV_INFO_INIT;
 	struct pretty_format format_pp = { 0 };
 	struct strbuf scratch_buf = STRBUF_INIT;
 	struct command cmd;
@@ -834,6 +848,11 @@ int cmd_format_rev(int argc,
 			   N_("how revs are processed")),
 		OPT_STRING_LIST(0, "notes", &data.notes, N_("notes"),
 				N_("display notes for pretty format")),
+		OPT__ABBREV(&data.rev.abbrev),
+		OPT__COLOR(&data.color, N_("use colored output")),
+		OPT_CALLBACK_F(0, "date", &data.rev, N_("date"),
+			       N_("date format"),
+			       PARSE_OPT_NONEG, date_cb),
 		OPT_CALLBACK_F('z', "null", &data, N_("z"),
 			       N_("use NUL for input and output termination"),
 			       PARSE_OPT_NOARG | PARSE_OPT_NONEG, format_nul_cb),
@@ -862,13 +881,13 @@ int cmd_format_rev(int argc,
 	init_display_notes(&format_notes_opt);
 	stdin_mode = parse_stdin_mode(data.stdin_mode);
 
-	get_commit_format(data.format, &format_rev);
-	format_pp.ctx.rev = &format_rev;
-	format_pp.ctx.fmt = format_rev.commit_format;
-	format_pp.ctx.abbrev = format_rev.abbrev;
-	format_pp.ctx.date_mode_explicit = format_rev.date_mode_explicit;
-	format_pp.ctx.date_mode = format_rev.date_mode;
-	format_pp.ctx.color = GIT_COLOR_AUTO;
+	get_commit_format(data.format, &data.rev);
+	format_pp.ctx.rev = &data.rev;
+	format_pp.ctx.fmt = data.rev.commit_format;
+	format_pp.ctx.abbrev = data.rev.abbrev;
+	format_pp.ctx.date_mode_explicit = data.rev.date_mode_explicit;
+	format_pp.ctx.date_mode = data.rev.date_mode;
+	format_pp.ctx.color = data.color;
 
 	userformat_find_requirements(data.format,
 				     &format_pp.want);
@@ -935,6 +954,7 @@ int cmd_format_rev(int argc,
 		BUG("uncovered case: %d", stdin_mode);
 	}
 
+	date_mode_release(&data.rev.date_mode);
 	strbuf_release(&scratch_buf);
 	string_list_clear(&data.notes, 0);
 	release_display_notes(&format_notes_opt);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 7a7c46658a3..a15da979abf 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -1017,4 +1017,62 @@ do
 	'
 done <stdin-modes
 
+format_rev_cmp_log () {
+	opts="$1"
+	format=reference
+	cat >input <<-\EOF &&
+	third
+	second
+	first
+	EOF
+	git -C repo-format log --stdin --no-walk \
+		--format="$format" "$opts" >expect <input &&
+	git -C repo-format format-rev --stdin-mode=revs \
+		--format="$format" "$opts" >actual <input &&
+	test_cmp expect actual
+}
+
+format_rev_err_cmp_log () {
+	opts="$1"
+	format=reference
+	# No input since we ought to fail while parsing options
+	test_must_fail git -C repo-format log --stdin --no-walk \
+		--format="$format" "$opts" 2>expect &&
+	test_must_fail git -C repo-format format-rev \
+		--stdin-mode=revs --format="$format" "$opts" 2>actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'format-rev --color' '
+	format_rev_cmp_log --color=always &&
+	format_rev_cmp_log --color &&
+	format_rev_cmp_log --no-color &&
+	format_rev_err_cmp_log --color=not-valid
+'
+
+test_expect_success 'format-rev --abbrev' '
+	format_rev_cmp_log --abbrev &&
+	format_rev_cmp_log --abbrev=31 &&
+	format_rev_cmp_log --no-abbrev
+'
+
+test_expect_success 'format-rev --date' '
+	format_rev_cmp_log --date=relative &&
+	format_rev_cmp_log --date=iso-strict &&
+	# This also tests the only case where we need to release
+	# the data for the parsed format
+	format_rev_cmp_log --date="format:%c" &&
+	format_rev_err_cmp_log --date=not-valid &&
+	# Test --date (no arg) next
+	# We cannot compare the output to git-log(1)
+	# because that command uses a slightly different
+	# error message (different library)
+	cat >expect <<-EOF &&
+	error: option \`date${SQ} requires a value
+	EOF
+	test_must_fail git -C repo-format format-rev \
+		--stdin-mode=revs --format="$format" --date 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.55.0.13.g85d2d65e389