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

[email protected]
Newsgroups org.kernel.vger.git
Message-ID <[email protected]>
From: Kristoffer Haugsbakk <[email protected]>

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/[email protected]/
• Fix useless `BUG` placements https://lore.kernel.org/git/[email protected]/
• 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.