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

[kristofferhaugsbakk@fastmail.com]

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