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

From: Kristoffer Haugsbakk <redacted>

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/ (local)
• Fix useless `BUG` placements https://lore.kernel.org/git/xmqqfr0hswxm.fsf@gitster.g/ (local)
• 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