From 2eda0102beb85b97bd43708176d81feabaf161e8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 10:56:10 -0400 Subject: check return value of verify_ref_format() Users of the ref-filter code must call verify_ref_format() before formatting any refs, but most ignore its return value. This means we may print an error on a syntactically bogus pattern, but keep going anyway. In most cases this results in a fatal error when we actually try to format a ref. But if you have no refs to show at all, then the behavior is confusing: git prints the error from verify_ref_format(), then exits with code 0 without showing any output. Let's instead abort immediately if we know we have a bogus format. We'll output the usage information if we have it handy (just like the existing call in cmd_for_each_ref() does), and otherwise just die(). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 4 +++- builtin/tag.c | 7 ++++--- builtin/verify-tag.c | 4 +++- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 8a0595e115..e756a5667a 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -409,7 +409,9 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (!format) format = to_free = build_format(filter, maxwidth, remote_prefix); - verify_ref_format(format); + + if (verify_ref_format(format)) + die(_("unable to parse format string")); ref_array_sort(sorting, &array); diff --git a/builtin/tag.c b/builtin/tag.c index 01154ea8dc..216629fb2c 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -53,7 +53,8 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con format = "%(refname:lstrip=2)"; } - verify_ref_format(format); + if (verify_ref_format(format)) + die(_("unable to parse format string")); filter->with_commit_tag_algo = 1; filter_refs(&array, filter, FILTER_REFS_TAGS); ref_array_sort(sorting, &array); @@ -501,8 +502,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (cmdmode == 'd') return for_each_tag_name(argv, delete_tag, NULL); if (cmdmode == 'v') { - if (format) - verify_ref_format(format); + if (format && verify_ref_format(format)) + usage_with_options(git_tag_usage, options); return for_each_tag_name(argv, verify_tag, format); } diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index f9a5f7535a..a10eca2b2d 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -51,7 +51,9 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) flags |= GPG_VERIFY_VERBOSE; if (fmt_pretty) { - verify_ref_format(fmt_pretty); + if (verify_ref_format(fmt_pretty)) + usage_with_options(verify_tag_usage, + verify_tag_options); flags |= GPG_VERIFY_OMIT_STATUS; } -- cgit v1.2.3 From 5d3d0681abb8f2bc2383ee7660cf2b02e89cdd07 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 10:56:21 -0400 Subject: docs/for-each-ref: update pointer to color syntax The documentation for the %(color) placeholder refers to the color.branch.* config for more details. But those details moved to their own section in b92c1a28f (Documentation/config.txt: describe 'color' value type in the "Values" section, 2015-03-03). Let's update our pointer. We can steal the text from 30cfe72d3 (pretty: fix document link for color specification, 2016-10-11), which fixed the same problem in a different place. While we're at it, let's give an example, which makes the syntax much more clear than just the text. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/git-for-each-ref.txt | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt index 03e187a105..cc42c12832 100644 --- a/Documentation/git-for-each-ref.txt +++ b/Documentation/git-for-each-ref.txt @@ -156,8 +156,10 @@ HEAD:: otherwise. color:: - Change output color. Followed by `:`, where names - are described in `color.branch.*`. + Change output color. Followed by `:`, where color + names are described under Values in the "CONFIGURATION FILE" + section of linkgit:git-config[1]. For example, + `%(color:bold red)`. align:: Left-, middle-, or right-align the content between -- cgit v1.2.3 From 097b681baac3a5e9455c74e3072a458249691e40 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 10:58:41 -0400 Subject: t: use test_decode_color rather than literal ANSI codes When we put literal ANSI terminal codes into our test scripts, it makes diffs on those scripts hard to read (the colors may be indistinguishable from diff coloring, or in the case of a reset, may not be visible at all). Some scripts get around this by including human-readable names and converting to literal codes with a git-config hack. This makes the actual code diffs look OK, but test_cmp output suffers from the same problem. Let's use test_decode_color instead, which turns the codes into obvious text tags. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t4207-log-decoration-colors.sh | 22 +++++++++------------ t/t6006-rev-list-format.sh | 42 +++++++++++++++++++++++++--------------- t/t6300-for-each-ref.sh | 18 ++++++++--------- t/test-lib-functions.sh | 1 + 4 files changed, 44 insertions(+), 39 deletions(-) diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh index b972296f06..60f040cab8 100755 --- a/t/t4207-log-decoration-colors.sh +++ b/t/t4207-log-decoration-colors.sh @@ -7,11 +7,6 @@ test_description='Test for "git log --decorate" colors' . ./test-lib.sh -get_color () -{ - git config --get-color no.such.slot "$1" -} - test_expect_success setup ' git config diff.color.commit yellow && git config color.decorate.branch green && @@ -20,14 +15,14 @@ test_expect_success setup ' git config color.decorate.stash magenta && git config color.decorate.HEAD cyan && - c_reset=$(get_color reset) && + c_reset="" && - c_commit=$(get_color yellow) && - c_branch=$(get_color green) && - c_remoteBranch=$(get_color red) && - c_tag=$(get_color "reverse bold yellow") && - c_stash=$(get_color magenta) && - c_HEAD=$(get_color cyan) && + c_commit="" && + c_branch="" && + c_remoteBranch="" && + c_tag="" && + c_stash="" && + c_HEAD="" && test_commit A && git clone . other && @@ -59,7 +54,8 @@ EOF # to this test since it does not contain any decoration, hence --first-parent test_expect_success 'Commit Decorations Colored Correctly' ' git log --first-parent --abbrev=10 --all --decorate --oneline --color=always | - sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" >out && + sed "s/[0-9a-f]\{10,10\}/COMMIT_ID/" | + test_decode_color >out && test_cmp expected out ' diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index a1dcdb81d7..647218b4ef 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -61,8 +61,9 @@ test_format () { # Feed to --format to provide predictable colored sequences. AUTO_COLOR='%C(auto,red)foo%C(auto,reset)' has_color () { - printf '\033[31mfoo\033[m\n' >expect && - test_cmp expect "$1" + test_decode_color <"$1" >decoded && + echo "foo" >expect && + test_cmp expect decoded } has_no_color () { @@ -170,19 +171,27 @@ $added EOF -test_format colors %Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy <expect <<-EOF && + commit $head2 + foobarbazxyzzy + EOF + format="%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy" && + git rev-list --format="$format" -1 master >actual.raw && + test_decode_color actual && + test_cmp expect actual +' -test_format advanced-colors '%C(red yellow bold)foo%C(reset)' <expect <<-EOF && + commit $head2 + foo + EOF + format="%C(red yellow bold)foo%C(reset)" && + git rev-list --format="$format" -1 master >actual.raw && + test_decode_color actual && + test_cmp expect actual +' test_expect_success '%C(auto,...) does not enable color by default' ' git log --format=$AUTO_COLOR -1 >actual && @@ -224,8 +233,9 @@ test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' ' ' test_expect_success '%C(auto) respects --color' ' - git log --color --format="%C(auto)%H" -1 >actual && - printf "\\033[33m%s\\033[m\\n" $(git rev-parse HEAD) >expect && + git log --color --format="%C(auto)%H" -1 >actual.raw && + test_decode_color actual && + echo "$(git rev-parse HEAD)" >expect && test_cmp expect actual ' diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 834a9ed168..7872a2f544 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -412,20 +412,18 @@ test_expect_success 'Check for invalid refname format' ' test_must_fail git for-each-ref --format="%(refname:INVALID)" ' -get_color () -{ - git config --get-color no.such.slot "$1" -} - cat >expected <master +$(git rev-parse --short refs/remotes/origin/master) origin/master +$(git rev-parse --short refs/tags/testtag) testtag +$(git rev-parse --short refs/tags/two) two EOF test_expect_success 'Check %(color:...) ' ' - git for-each-ref --format="%(objectname:short) %(color:green)%(refname:short)" >actual && + git for-each-ref \ + --format="%(objectname:short) %(color:green)%(refname:short)" \ + >actual.raw && + test_decode_color actual && test_cmp expected actual ' diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh index db622c3555..e09e93b38f 100644 --- a/t/test-lib-functions.sh +++ b/t/test-lib-functions.sh @@ -42,6 +42,7 @@ test_decode_color () { function name(n) { if (n == 0) return "RESET"; if (n == 1) return "BOLD"; + if (n == 7) return "REVERSE"; if (n == 30) return "BLACK"; if (n == 31) return "RED"; if (n == 32) return "GREEN"; -- cgit v1.2.3 From 51331aad69a1d89a8b6d1ff82bb5fedbdb6ccc6a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 10:58:56 -0400 Subject: ref-filter: simplify automatic color reset When the user-format doesn't add the closing color reset, we add one automatically. But we do so by parsing the "reset" string. We can just use the baked-in string literal, which is simpler. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ref-filter.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ae6ecbd1cf..6da5be3ac6 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2084,11 +2084,7 @@ void format_ref_array_item(struct ref_array_item *info, const char *format, } if (need_color_reset_at_eol) { struct atom_value resetv; - char color[COLOR_MAXLEN] = ""; - - if (color_parse("reset", color) < 0) - die("BUG: couldn't parse 'reset' as a color"); - resetv.s = color; + resetv.s = GIT_COLOR_RESET; append_atom(&resetv, &state); } if (state.stack->prev) -- cgit v1.2.3 From 4a68e36d7d106abaf44e3ac960276145b5a25723 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:01:18 -0400 Subject: ref-filter: abstract ref format into its own struct The ref-filter module provides routines for formatting a ref for output. The fundamental interface for the format is a "const char *" containing the format, and any additional options need to be passed to each invocation of show_ref_array_item. Instead, let's make a ref_format struct that holds the format, along with any associated format options. That will make some enhancements easier in the future: 1. new formatting options can be added without disrupting existing callers 2. some state can be carried in the struct rather than as global variables For now this just has the text format itself along with the quote_style option, but we'll add more fields in future patches. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 14 +++++++------- builtin/for-each-ref.c | 22 ++++++++++++---------- builtin/tag.c | 30 ++++++++++++++++-------------- builtin/verify-tag.c | 12 ++++++------ ref-filter.c | 22 ++++++++++++---------- ref-filter.h | 22 +++++++++++++++++----- 6 files changed, 70 insertions(+), 52 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index e756a5667a..036fdc9298 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -383,7 +383,7 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r return strbuf_detach(&fmt, NULL); } -static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format) +static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, struct ref_format *format) { int i; struct ref_array array; @@ -407,8 +407,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (filter->verbose) maxwidth = calc_maxwidth(&array, strlen(remote_prefix)); - if (!format) - format = to_free = build_format(filter, maxwidth, remote_prefix); + if (!format->format) + format->format = to_free = build_format(filter, maxwidth, remote_prefix); if (verify_ref_format(format)) die(_("unable to parse format string")); @@ -416,7 +416,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin ref_array_sort(sorting, &array); for (i = 0; i < array.nr; i++) { - format_ref_array_item(array.items[i], format, 0, &out); + format_ref_array_item(array.items[i], format, &out); if (column_active(colopts)) { assert(!filter->verbose && "--column and --verbose are incompatible"); /* format to a string_list to let print_columns() do its job */ @@ -551,7 +551,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) struct ref_filter filter; int icase = 0; static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; - const char *format = NULL; + struct ref_format format = REF_FORMAT_INIT; struct option options[] = { OPT_GROUP(N_("Generic options")), @@ -595,7 +595,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) N_("print only branches of the object"), 0, parse_opt_object_name }, OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")), - OPT_STRING( 0 , "format", &format, N_("format"), N_("format to use for the output")), + OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), OPT_END(), }; @@ -669,7 +669,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix) if (!sorting) sorting = ref_default_sorting(); sorting->ignore_case = icase; - print_ref_list(&filter, sorting, format); + print_ref_list(&filter, sorting, &format); print_columns(&output, colopts, NULL); string_list_clear(&output, 0); return 0; diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index 52be99cbac..f47066b42a 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -17,25 +17,25 @@ static char const * const for_each_ref_usage[] = { int cmd_for_each_ref(int argc, const char **argv, const char *prefix) { int i; - const char *format = "%(objectname) %(objecttype)\t%(refname)"; struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; - int maxcount = 0, quote_style = 0, icase = 0; + int maxcount = 0, icase = 0; struct ref_array array; struct ref_filter filter; + struct ref_format format = REF_FORMAT_INIT; struct option opts[] = { - OPT_BIT('s', "shell", "e_style, + OPT_BIT('s', "shell", &format.quote_style, N_("quote placeholders suitably for shells"), QUOTE_SHELL), - OPT_BIT('p', "perl", "e_style, + OPT_BIT('p', "perl", &format.quote_style, N_("quote placeholders suitably for perl"), QUOTE_PERL), - OPT_BIT(0 , "python", "e_style, + OPT_BIT(0 , "python", &format.quote_style, N_("quote placeholders suitably for python"), QUOTE_PYTHON), - OPT_BIT(0 , "tcl", "e_style, + OPT_BIT(0 , "tcl", &format.quote_style, N_("quote placeholders suitably for Tcl"), QUOTE_TCL), OPT_GROUP(""), OPT_INTEGER( 0 , "count", &maxcount, N_("show only matched refs")), - OPT_STRING( 0 , "format", &format, N_("format"), N_("format to use for the output")), + OPT_STRING( 0 , "format", &format.format, N_("format"), N_("format to use for the output")), OPT_CALLBACK(0 , "sort", sorting_tail, N_("key"), N_("field name to sort on"), &parse_opt_ref_sorting), OPT_CALLBACK(0, "points-at", &filter.points_at, @@ -52,16 +52,18 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) memset(&array, 0, sizeof(array)); memset(&filter, 0, sizeof(filter)); + format.format = "%(objectname) %(objecttype)\t%(refname)"; + parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); if (maxcount < 0) { error("invalid --count argument: `%d'", maxcount); usage_with_options(for_each_ref_usage, opts); } - if (HAS_MULTI_BITS(quote_style)) { + if (HAS_MULTI_BITS(format.quote_style)) { error("more than one quoting style?"); usage_with_options(for_each_ref_usage, opts); } - if (verify_ref_format(format)) + if (verify_ref_format(&format)) usage_with_options(for_each_ref_usage, opts); if (!sorting) @@ -80,7 +82,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) if (!maxcount || array.nr < maxcount) maxcount = array.nr; for (i = 0; i < maxcount; i++) - show_ref_array_item(array.items[i], format, quote_style); + show_ref_array_item(array.items[i], &format); ref_array_clear(&array); return 0; } diff --git a/builtin/tag.c b/builtin/tag.c index 216629fb2c..207c9eb035 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -32,7 +32,8 @@ static const char * const git_tag_usage[] = { static unsigned int colopts; static int force_sign_annotate; -static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, const char *format) +static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, + struct ref_format *format) { struct ref_array array; char *to_free = NULL; @@ -43,14 +44,14 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con if (filter->lines == -1) filter->lines = 0; - if (!format) { + if (!format->format) { if (filter->lines) { to_free = xstrfmt("%s %%(contents:lines=%d)", "%(align:15)%(refname:lstrip=2)%(end)", filter->lines); - format = to_free; + format->format = to_free; } else - format = "%(refname:lstrip=2)"; + format->format = "%(refname:lstrip=2)"; } if (verify_ref_format(format)) @@ -60,7 +61,7 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con ref_array_sort(sorting, &array); for (i = 0; i < array.nr; i++) - show_ref_array_item(array.items[i], format, 0); + show_ref_array_item(array.items[i], format); ref_array_clear(&array); free(to_free); @@ -106,17 +107,17 @@ static int verify_tag(const char *name, const char *ref, const struct object_id *oid, const void *cb_data) { int flags; - const char *fmt_pretty = cb_data; + const struct ref_format *format = cb_data; flags = GPG_VERIFY_VERBOSE; - if (fmt_pretty) + if (format->format) flags = GPG_VERIFY_OMIT_STATUS; if (gpg_verify_tag(oid->hash, name, flags)) return -1; - if (fmt_pretty) - pretty_print_ref(name, oid->hash, fmt_pretty); + if (format->format) + pretty_print_ref(name, oid->hash, format); return 0; } @@ -393,7 +394,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) struct strbuf err = STRBUF_INIT; struct ref_filter filter; static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting; - const char *format = NULL; + struct ref_format format = REF_FORMAT_INIT; int icase = 0; struct option options[] = { OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), @@ -432,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) N_("print only tags of the object"), PARSE_OPT_LASTARG_DEFAULT, parse_opt_object_name, (intptr_t) "HEAD" }, - OPT_STRING( 0 , "format", &format, N_("format"), N_("format to use for the output")), + OPT_STRING( 0 , "format", &format.format, N_("format"), + N_("format to use for the output")), OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")), OPT_END() }; @@ -484,7 +486,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix) run_column_filter(colopts, &copts); } filter.name_patterns = argv; - ret = list_tags(&filter, sorting, format); + ret = list_tags(&filter, sorting, &format); if (column_active(colopts)) stop_column_filter(); return ret; @@ -502,9 +504,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix) if (cmdmode == 'd') return for_each_tag_name(argv, delete_tag, NULL); if (cmdmode == 'v') { - if (format && verify_ref_format(format)) + if (format.format && verify_ref_format(&format)) usage_with_options(git_tag_usage, options); - return for_each_tag_name(argv, verify_tag, format); + return for_each_tag_name(argv, verify_tag, &format); } if (msg.given || msgfile) { diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c index a10eca2b2d..87d73e856a 100644 --- a/builtin/verify-tag.c +++ b/builtin/verify-tag.c @@ -32,11 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) { int i = 1, verbose = 0, had_error = 0; unsigned flags = 0; - char *fmt_pretty = NULL; + struct ref_format format = REF_FORMAT_INIT; const struct option verify_tag_options[] = { OPT__VERBOSE(&verbose, N_("print tag contents")), OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW), - OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")), + OPT_STRING(0, "format", &format.format, N_("format"), N_("format to use for the output")), OPT_END() }; @@ -50,8 +50,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) if (verbose) flags |= GPG_VERIFY_VERBOSE; - if (fmt_pretty) { - if (verify_ref_format(fmt_pretty)) + if (format.format) { + if (verify_ref_format(&format)) usage_with_options(verify_tag_usage, verify_tag_options); flags |= GPG_VERIFY_OMIT_STATUS; @@ -70,8 +70,8 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix) continue; } - if (fmt_pretty) - pretty_print_ref(name, sha1, fmt_pretty); + if (format.format) + pretty_print_ref(name, sha1, &format); } return had_error; } diff --git a/ref-filter.c b/ref-filter.c index 6da5be3ac6..66d234bb15 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -657,12 +657,12 @@ static const char *find_next(const char *cp) * Make sure the format string is well formed, and parse out * the used atoms. */ -int verify_ref_format(const char *format) +int verify_ref_format(struct ref_format *format) { const char *cp, *sp; need_color_reset_at_eol = 0; - for (cp = format; *cp && (sp = find_next(cp)); ) { + for (cp = format->format; *cp && (sp = find_next(cp)); ) { const char *color, *ep = strchr(sp, ')'); int at; @@ -2060,16 +2060,17 @@ static void append_literal(const char *cp, const char *ep, struct ref_formatting } } -void format_ref_array_item(struct ref_array_item *info, const char *format, - int quote_style, struct strbuf *final_buf) +void format_ref_array_item(struct ref_array_item *info, + const struct ref_format *format, + struct strbuf *final_buf) { const char *cp, *sp, *ep; struct ref_formatting_state state = REF_FORMATTING_STATE_INIT; - state.quote_style = quote_style; + state.quote_style = format->quote_style; push_stack_element(&state.stack); - for (cp = format; *cp && (sp = find_next(cp)); cp = ep + 1) { + for (cp = format->format; *cp && (sp = find_next(cp)); cp = ep + 1) { struct atom_value *atomv; ep = strchr(sp, ')'); @@ -2093,23 +2094,24 @@ void format_ref_array_item(struct ref_array_item *info, const char *format, pop_stack_element(&state.stack); } -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style) +void show_ref_array_item(struct ref_array_item *info, + const struct ref_format *format) { struct strbuf final_buf = STRBUF_INIT; - format_ref_array_item(info, format, quote_style, &final_buf); + format_ref_array_item(info, format, &final_buf); fwrite(final_buf.buf, 1, final_buf.len, stdout); strbuf_release(&final_buf); putchar('\n'); } void pretty_print_ref(const char *name, const unsigned char *sha1, - const char *format) + const struct ref_format *format) { struct ref_array_item *ref_item; ref_item = new_ref_array_item(name, sha1, 0); ref_item->kind = ref_kind_from_refname(name); - show_ref_array_item(ref_item, format, 0); + show_ref_array_item(ref_item, format); free_array_item(ref_item); } diff --git a/ref-filter.h b/ref-filter.h index 6552024f09..2bb58879d7 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -72,6 +72,17 @@ struct ref_filter { verbose; }; +struct ref_format { + /* + * Set these to define the format; make sure you call + * verify_ref_format() afterwards to finalize. + */ + const char *format; + int quote_style; +}; + +#define REF_FORMAT_INIT { NULL, 0 } + /* Macros for checking --merged and --no-merged options */ #define _OPT_MERGED_NO_MERGED(option, filter, h) \ { OPTION_CALLBACK, 0, option, (filter), N_("commit"), (h), \ @@ -93,14 +104,15 @@ void ref_array_clear(struct ref_array *array); /* Parse format string and sort specifiers */ int parse_ref_filter_atom(const char *atom, const char *ep); /* Used to verify if the given format is correct and to parse out the used atoms */ -int verify_ref_format(const char *format); +int verify_ref_format(struct ref_format *format); /* Sort the given ref_array as per the ref_sorting provided */ void ref_array_sort(struct ref_sorting *sort, struct ref_array *array); /* Based on the given format and quote_style, fill the strbuf */ -void format_ref_array_item(struct ref_array_item *info, const char *format, - int quote_style, struct strbuf *final_buf); +void format_ref_array_item(struct ref_array_item *info, + const struct ref_format *format, + struct strbuf *final_buf); /* Print the ref using the given format and quote_style */ -void show_ref_array_item(struct ref_array_item *info, const char *format, int quote_style); +void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format); /* Callback function for parsing the sort option */ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset); /* Default sort option based on refname */ @@ -117,6 +129,6 @@ void setup_ref_filter_porcelain_msg(void); * name must be a fully qualified refname. */ void pretty_print_ref(const char *name, const unsigned char *sha1, - const char *format); + const struct ref_format *format); #endif /* REF_FILTER_H */ -- cgit v1.2.3 From bf285ae6dbfa8e2b847940e6ad987ff3cbe40712 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:02:30 -0400 Subject: ref-filter: move need_color_reset_at_eol into ref_format Calling verify_ref_format() doesn't just confirm that the format is sane; it actually sets some global variables that will be used later when formatting the refs. These logically should belong to the ref_format, which would make it possible to use multiple formats within a single program invocation. Let's move one such flag into the ref_format struct. There are still others that would need to be moved before it would be safe to use multiple formats, but this commit gives a blueprint for how that should look. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ref-filter.c | 7 +++---- ref-filter.h | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 66d234bb15..178396e1f3 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -97,7 +97,6 @@ static struct used_atom { } u; } *used_atom; static int used_atom_cnt, need_tagged, need_symref; -static int need_color_reset_at_eol; static void color_atom_parser(struct used_atom *atom, const char *color_value) { @@ -661,7 +660,7 @@ int verify_ref_format(struct ref_format *format) { const char *cp, *sp; - need_color_reset_at_eol = 0; + format->need_color_reset_at_eol = 0; for (cp = format->format; *cp && (sp = find_next(cp)); ) { const char *color, *ep = strchr(sp, ')'); int at; @@ -673,7 +672,7 @@ int verify_ref_format(struct ref_format *format) cp = ep + 1; if (skip_prefix(used_atom[at].name, "color:", &color)) - need_color_reset_at_eol = !!strcmp(color, "reset"); + format->need_color_reset_at_eol = !!strcmp(color, "reset"); } return 0; } @@ -2083,7 +2082,7 @@ void format_ref_array_item(struct ref_array_item *info, sp = cp + strlen(cp); append_literal(cp, sp, &state); } - if (need_color_reset_at_eol) { + if (format->need_color_reset_at_eol) { struct atom_value resetv; resetv.s = GIT_COLOR_RESET; append_atom(&resetv, &state); diff --git a/ref-filter.h b/ref-filter.h index 2bb58879d7..9e1e89c19b 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -79,6 +79,9 @@ struct ref_format { */ const char *format; int quote_style; + + /* Internal state to ref-filter */ + int need_color_reset_at_eol; }; #define REF_FORMAT_INIT { NULL, 0 } -- cgit v1.2.3 From 18a2565016d45538345a06cd9b912040b5581fc1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:02:44 -0400 Subject: ref-filter: provide a function for parsing sort options The ref-filter module currently provides a callback suitable for parsing command-line --sort options. But since git-tag also supports the tag.sort config option, it needs a function whose implementation is quite similar, but with a slightly different interface. The end result is that builtin/tag.c has a copy-paste of parse_opt_ref_sorting(). Instead, let's provide a function to parse an arbitrary sort string, which we can then trivially wrap to make the parse_opt variant. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/tag.c | 26 +------------------------- ref-filter.c | 13 ++++++++----- ref-filter.h | 2 ++ 3 files changed, 11 insertions(+), 30 deletions(-) diff --git a/builtin/tag.c b/builtin/tag.c index 207c9eb035..66e35b823b 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -136,30 +136,6 @@ static const char tag_template_nocleanup[] = "Lines starting with '%c' will be kept; you may remove them" " yourself if you want to.\n"); -/* Parse arg given and add it the ref_sorting array */ -static int parse_sorting_string(const char *arg, struct ref_sorting **sorting_tail) -{ - struct ref_sorting *s; - int len; - - s = xcalloc(1, sizeof(*s)); - s->next = *sorting_tail; - *sorting_tail = s; - - if (*arg == '-') { - s->reverse = 1; - arg++; - } - if (skip_prefix(arg, "version:", &arg) || - skip_prefix(arg, "v:", &arg)) - s->version = 1; - - len = strlen(arg); - s->atom = parse_ref_filter_atom(arg, arg+len); - - return 0; -} - static int git_tag_config(const char *var, const char *value, void *cb) { int status; @@ -168,7 +144,7 @@ static int git_tag_config(const char *var, const char *value, void *cb) if (!strcmp(var, "tag.sort")) { if (!value) return config_error_nonbool(var); - parse_sorting_string(value, sorting_tail); + parse_ref_sorting(sorting_tail, value); return 0; } diff --git a/ref-filter.c b/ref-filter.c index 178396e1f3..4321212197 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2126,15 +2126,11 @@ struct ref_sorting *ref_default_sorting(void) return sorting; } -int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset) +void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) { - struct ref_sorting **sorting_tail = opt->value; struct ref_sorting *s; int len; - if (!arg) /* should --no-sort void the list ? */ - return -1; - s = xcalloc(1, sizeof(*s)); s->next = *sorting_tail; *sorting_tail = s; @@ -2148,6 +2144,13 @@ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset) s->version = 1; len = strlen(arg); s->atom = parse_ref_filter_atom(arg, arg+len); +} + +int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset) +{ + if (!arg) /* should --no-sort void the list ? */ + return -1; + parse_ref_sorting(opt->value, arg); return 0; } diff --git a/ref-filter.h b/ref-filter.h index 9e1e89c19b..67fa6261b4 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -116,6 +116,8 @@ void format_ref_array_item(struct ref_array_item *info, struct strbuf *final_buf); /* Print the ref using the given format and quote_style */ void show_ref_array_item(struct ref_array_item *info, const struct ref_format *format); +/* Parse a single sort specifier and add it to the list */ +void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *atom); /* Callback function for parsing the sort option */ int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset); /* Default sort option based on refname */ -- cgit v1.2.3 From aa8a5d144da4e4ec43580831ff4a741538d21db4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:02:52 -0400 Subject: ref-filter: make parse_ref_filter_atom a private function The parse_ref_filter_atom() function really shouldn't be exposed outside of ref-filter.c; its return value is an integer index into an array that is private in that file. Since the previous commit removed the sole external caller (and replaced it with a public function at a more appropriately level), we can just make this static. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ref-filter.c | 2 +- ref-filter.h | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 4321212197..ee97d7218e 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -364,7 +364,7 @@ struct atom_value { /* * Used to parse format string and sort specifiers */ -int parse_ref_filter_atom(const char *atom, const char *ep) +static int parse_ref_filter_atom(const char *atom, const char *ep) { const char *sp; const char *arg; diff --git a/ref-filter.h b/ref-filter.h index 67fa6261b4..1ffc3ca816 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -104,8 +104,6 @@ struct ref_format { int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int type); /* Clear all memory allocated to ref_array */ void ref_array_clear(struct ref_array *array); -/* Parse format string and sort specifiers */ -int parse_ref_filter_atom(const char *atom, const char *ep); /* Used to verify if the given format is correct and to parse out the used atoms */ int verify_ref_format(struct ref_format *format); /* Sort the given ref_array as per the ref_sorting provided */ -- cgit v1.2.3 From 29ef53cd361e8751f9f2f40811af0ce863e449b6 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:02:58 -0400 Subject: ref-filter: factor out the parsing of sorting atoms We parse sort strings as single formatting atoms, and just build on parse_ref_filter_atom(). Let's pull this idea into its own function, since it's about to get a little more complex. As a bonus, we can give the function a slightly more natural interface, since our single atoms are in their own strings. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ref-filter.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index ee97d7218e..080f5acebe 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2114,6 +2114,12 @@ void pretty_print_ref(const char *name, const unsigned char *sha1, free_array_item(ref_item); } +static int parse_sorting_atom(const char *atom) +{ + const char *end = atom + strlen(atom); + return parse_ref_filter_atom(atom, end); +} + /* If no sorting option is given, use refname to sort as default */ struct ref_sorting *ref_default_sorting(void) { @@ -2122,14 +2128,13 @@ struct ref_sorting *ref_default_sorting(void) struct ref_sorting *sorting = xcalloc(1, sizeof(*sorting)); sorting->next = NULL; - sorting->atom = parse_ref_filter_atom(cstr_name, cstr_name + strlen(cstr_name)); + sorting->atom = parse_sorting_atom(cstr_name); return sorting; } void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) { struct ref_sorting *s; - int len; s = xcalloc(1, sizeof(*s)); s->next = *sorting_tail; @@ -2142,8 +2147,7 @@ void parse_ref_sorting(struct ref_sorting **sorting_tail, const char *arg) if (skip_prefix(arg, "version:", &arg) || skip_prefix(arg, "v:", &arg)) s->version = 1; - len = strlen(arg); - s->atom = parse_ref_filter_atom(arg, arg+len); + s->atom = parse_sorting_atom(arg); } int parse_opt_ref_sorting(const struct option *opt, const char *arg, int unset) -- cgit v1.2.3 From ab7ded34d6a9a5d4c8bf073cb9627b8054f430cf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:06:40 -0400 Subject: ref-filter: pass ref_format struct to atom parsers The callback for parsing each formatting atom gets to see only the atom struct (which it's filling in) and the text to be parsed. This doesn't leave any room for it to behave differently based on context known only to the ref_format. We can solve this by passing in the surrounding ref_format to each parser. Note that this makes things slightly awkward for sort strings, which parse atoms without having a ref_format. We'll solve that by using a dummy ref_format with default parameters. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- ref-filter.c | 42 +++++++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/ref-filter.c b/ref-filter.c index 080f5acebe..129a636771 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -98,7 +98,7 @@ static struct used_atom { } *used_atom; static int used_atom_cnt, need_tagged, need_symref; -static void color_atom_parser(struct used_atom *atom, const char *color_value) +static void color_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *color_value) { if (!color_value) die(_("expected format: %%(color:)")); @@ -126,7 +126,7 @@ static void refname_atom_parser_internal(struct refname_atom *atom, die(_("unrecognized %%(%s) argument: %s"), name, arg); } -static void remote_ref_atom_parser(struct used_atom *atom, const char *arg) +static void remote_ref_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { struct string_list params = STRING_LIST_INIT_DUP; int i; @@ -160,28 +160,28 @@ static void remote_ref_atom_parser(struct used_atom *atom, const char *arg) string_list_clear(¶ms, 0); } -static void body_atom_parser(struct used_atom *atom, const char *arg) +static void body_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (arg) die(_("%%(body) does not take arguments")); atom->u.contents.option = C_BODY_DEP; } -static void subject_atom_parser(struct used_atom *atom, const char *arg) +static void subject_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (arg) die(_("%%(subject) does not take arguments")); atom->u.contents.option = C_SUB; } -static void trailers_atom_parser(struct used_atom *atom, const char *arg) +static void trailers_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (arg) die(_("%%(trailers) does not take arguments")); atom->u.contents.option = C_TRAILERS; } -static void contents_atom_parser(struct used_atom *atom, const char *arg) +static void contents_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (!arg) atom->u.contents.option = C_BARE; @@ -201,7 +201,7 @@ static void contents_atom_parser(struct used_atom *atom, const char *arg) die(_("unrecognized %%(contents) argument: %s"), arg); } -static void objectname_atom_parser(struct used_atom *atom, const char *arg) +static void objectname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (!arg) atom->u.objectname.option = O_FULL; @@ -218,7 +218,7 @@ static void objectname_atom_parser(struct used_atom *atom, const char *arg) die(_("unrecognized %%(objectname) argument: %s"), arg); } -static void refname_atom_parser(struct used_atom *atom, const char *arg) +static void refname_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { refname_atom_parser_internal(&atom->u.refname, arg, atom->name); } @@ -234,7 +234,7 @@ static align_type parse_align_position(const char *s) return -1; } -static void align_atom_parser(struct used_atom *atom, const char *arg) +static void align_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { struct align *align = &atom->u.align; struct string_list params = STRING_LIST_INIT_DUP; @@ -273,7 +273,7 @@ static void align_atom_parser(struct used_atom *atom, const char *arg) string_list_clear(¶ms, 0); } -static void if_atom_parser(struct used_atom *atom, const char *arg) +static void if_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { if (!arg) { atom->u.if_then_else.cmp_status = COMPARE_NONE; @@ -287,7 +287,7 @@ static void if_atom_parser(struct used_atom *atom, const char *arg) } } -static void head_atom_parser(struct used_atom *atom, const char *arg) +static void head_atom_parser(const struct ref_format *format, struct used_atom *atom, const char *arg) { struct object_id unused; @@ -297,7 +297,7 @@ static void head_atom_parser(struct used_atom *atom, const char *arg) static struct { const char *name; cmp_type cmp_type; - void (*parser)(struct used_atom *atom, const char *arg); + void (*parser)(const struct ref_format *format, struct used_atom *atom, const char *arg); } valid_atom[] = { { "refname" , FIELD_STR, refname_atom_parser }, { "objecttype" }, @@ -364,7 +364,8 @@ struct atom_value { /* * Used to parse format string and sort specifiers */ -static int parse_ref_filter_atom(const char *atom, const char *ep) +static int parse_ref_filter_atom(const struct ref_format *format, + const char *atom, const char *ep) { const char *sp; const char *arg; @@ -412,7 +413,7 @@ static int parse_ref_filter_atom(const char *atom, const char *ep) arg = used_atom[at].name + (arg - atom) + 1; memset(&used_atom[at].u, 0, sizeof(used_atom[at].u)); if (valid_atom[i].parser) - valid_atom[i].parser(&used_atom[at], arg); + valid_atom[i].parser(format, &used_atom[at], arg); if (*atom == '*') need_tagged = 1; if (!strcmp(valid_atom[i].name, "symref")) @@ -668,7 +669,7 @@ int verify_ref_format(struct ref_format *format) if (!ep) return error(_("malformed format string %s"), sp); /* sp points at "%(" and ep points at the closing ")" */ - at = parse_ref_filter_atom(sp + 2, ep); + at = parse_ref_filter_atom(format, sp + 2, ep); cp = ep + 1; if (skip_prefix(used_atom[at].name, "color:", &color)) @@ -2075,7 +2076,9 @@ void format_ref_array_item(struct ref_array_item *info, ep = strchr(sp, ')'); if (cp < sp) append_literal(cp, sp, &state); - get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep), &atomv); + get_ref_atom_value(info, + parse_ref_filter_atom(format, sp + 2, ep), + &atomv); atomv->handler(atomv, &state); } if (*cp) { @@ -2116,8 +2119,13 @@ void pretty_print_ref(const char *name, const unsigned char *sha1, static int parse_sorting_atom(const char *atom) { + /* + * This parses an atom using a dummy ref_format, since we don't + * actually care about the formatting details. + */ + struct ref_format dummy = REF_FORMAT_INIT; const char *end = atom + strlen(atom); - return parse_ref_filter_atom(atom, end); + return parse_ref_filter_atom(&dummy, atom, end); } /* If no sorting option is given, use refname to sort as default */ -- cgit v1.2.3 From 136c8c8b8fa39f1315713248473dececf20f8fe7 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:07:03 -0400 Subject: color: check color.ui in git_default_config() Back in prehistoric times, our decision on whether or not to show color by default relied on using a config callback that either did or didn't load color config like color.diff. When we introduced color.ui, we put it in the same boat: commands had to manually respect it by using git_color_config() or its git_color_default_config() convenience wrapper. But in 4c7f1819b (make color.ui default to 'auto', 2013-06-10), that changed. Since then, we default color.ui to auto in all programs, meaning that even plumbing commands like "git diff-tree --pretty" might colorize the output. Nobody seems to have complained in the intervening years, presumably because the "is stdout a tty" check does a good job of catching the right cases. But that leaves an interesting curiosity: color.ui defaults to auto even in plumbing, but you can't actually _disable_ the color via config. So if you really hate color and set "color.ui" to false, diff-tree will still show color (but porcelain like git-diff won't). Nobody noticed that either, probably because very few people disable color. One could argue that the plumbing should _always_ disable color unless an explicit --color option is given on the command line. But in practice, this creates a lot of complications for scripts which do want plumbing to show user-visible output. They can't just pass "--color" blindly; they need to check the user's config and decide what to send. Given that nobody has complained about the current behavior, let's assume it's a good path, and follow it to its conclusion: supporting color.ui everywhere. Note that you can create havoc by setting color.ui=always in your config, but that's more or less already the case. We could disallow it entirely, but it is handy for one-offs like: git -c color.ui=always foo >not-a-tty when "foo" does not take a --color option itself. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- builtin/clean.c | 3 +-- builtin/grep.c | 2 +- builtin/show-branch.c | 2 +- color.c | 8 -------- config.c | 4 ++++ diff.c | 3 --- 7 files changed, 8 insertions(+), 16 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index 036fdc9298..d852ded497 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -92,7 +92,7 @@ static int git_branch_config(const char *var, const char *value, void *cb) return config_error_nonbool(var); return color_parse(value, branch_colors[slot]); } - return git_color_default_config(var, value, cb); + return git_default_config(var, value, cb); } static const char *branch_get_color(enum color_branch ix) diff --git a/builtin/clean.c b/builtin/clean.c index 057fc97fe4..c1bafda5b6 100644 --- a/builtin/clean.c +++ b/builtin/clean.c @@ -125,8 +125,7 @@ static int git_clean_config(const char *var, const char *value, void *cb) return 0; } - /* inspect the color.ui config variable and others */ - return git_color_default_config(var, value, cb); + return git_default_config(var, value, cb); } static const char *clean_get_color(enum color_clean ix) diff --git a/builtin/grep.c b/builtin/grep.c index 0d6e669732..a7157f5632 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -284,7 +284,7 @@ static int wait_all(void) static int grep_cmd_config(const char *var, const char *value, void *cb) { int st = grep_config(var, value, cb); - if (git_color_default_config(var, value, cb) < 0) + if (git_default_config(var, value, cb) < 0) st = -1; if (!strcmp(var, "grep.threads")) { diff --git a/builtin/show-branch.c b/builtin/show-branch.c index 7073a3eb97..28f245c8cc 100644 --- a/builtin/show-branch.c +++ b/builtin/show-branch.c @@ -554,7 +554,7 @@ static int git_show_branch_config(const char *var, const char *value, void *cb) return 0; } - return git_color_default_config(var, value, cb); + return git_default_config(var, value, cb); } static int omit_in_dense(struct commit *commit, struct commit **rev, int n) diff --git a/color.c b/color.c index 31b6207a00..7aa8b076f0 100644 --- a/color.c +++ b/color.c @@ -361,14 +361,6 @@ int git_color_config(const char *var, const char *value, void *cb) return 0; } -int git_color_default_config(const char *var, const char *value, void *cb) -{ - if (git_color_config(var, value, cb) < 0) - return -1; - - return git_default_config(var, value, cb); -} - void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb) { if (*color) diff --git a/config.c b/config.c index a9356c1383..bc290e7563 100644 --- a/config.c +++ b/config.c @@ -16,6 +16,7 @@ #include "string-list.h" #include "utf8.h" #include "dir.h" +#include "color.h" struct config_source { struct config_source *prev; @@ -1350,6 +1351,9 @@ int git_default_config(const char *var, const char *value, void *dummy) if (starts_with(var, "advice.")) return git_default_advice_config(var, value); + if (git_color_config(var, value, dummy) < 0) + return -1; + if (!strcmp(var, "pager.color") || !strcmp(var, "color.pager")) { pager_use_color = git_config_bool(var,value); return 0; diff --git a/diff.c b/diff.c index 85e714f6c6..9c38258030 100644 --- a/diff.c +++ b/diff.c @@ -299,9 +299,6 @@ int git_diff_ui_config(const char *var, const char *value, void *cb) return 0; } - if (git_color_config(var, value, cb) < 0) - return -1; - return git_diff_basic_config(var, value, cb); } -- cgit v1.2.3 From d8b68686a4cc8a486823dd356ed4dba0e51536ea Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:07:14 -0400 Subject: for-each-ref: load config earlier In most commands we load config before parsing command line options, since it lets the latter override the former with a simple variable assignment. In the case of for-each-ref, though, we do it in the reverse order. This is OK with the current code, since there's no interaction between the config and command-line options. However, as the ref-filter code starts to care about config during verify_ref_format(), we'll want to make sure the config is loaded. Let's bump the config to the usual spot near the top of the function. We can drop the comment there; it's impossible to keep a "why we load the config" comment like this up to date with every config option we might be interested in. And indeed, it's already stale; we'd care about core.abbrev, for instance, when %(objectname:short) is used. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/for-each-ref.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c index f47066b42a..5d7c921a77 100644 --- a/builtin/for-each-ref.c +++ b/builtin/for-each-ref.c @@ -54,6 +54,8 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) format.format = "%(objectname) %(objecttype)\t%(refname)"; + git_config(git_default_config, NULL); + parse_options(argc, argv, prefix, opts, for_each_ref_usage, 0); if (maxcount < 0) { error("invalid --count argument: `%d'", maxcount); @@ -71,9 +73,6 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix) sorting->ignore_case = icase; filter.ignore_case = icase; - /* for warn_ambiguous_refs */ - git_config(git_default_config, NULL); - filter.name_patterns = argv; filter.match_as_path = 1; filter_refs(&array, &filter, FILTER_REFS_ALL | FILTER_REFS_INCLUDE_BROKEN); -- cgit v1.2.3 From d75dfb1089d471501f6042341bed647525f6e293 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:07:30 -0400 Subject: rev-list: pass diffopt->use_colors through to pretty-print When rev-list pretty-prints a commit, it creates a new pretty_print_context and copies items from the rev_info struct. We don't currently copy the "use_color" field, though. Nobody seems to have noticed because the only part of pretty.c that cares is the %C(auto,...) placeholder, and presumably not many people use that with the rev-list plumbing (as opposed to with git-log). It will become more noticeable in a future patch, though, when we start treating all user-format colors as auto-colors (in which case it would become impossible to format colors with rev-list, even with --color=always). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/rev-list.c | 1 + t/t6006-rev-list-format.sh | 11 +++++++++++ 2 files changed, 12 insertions(+) diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 95d84d5cda..fee10d8567 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -122,6 +122,7 @@ static void show_commit(struct commit *commit, void *data) ctx.date_mode_explicit = revs->date_mode_explicit; ctx.fmt = revs->commit_format; ctx.output_encoding = get_log_output_encoding(); + ctx.color = revs->diffopt.use_color; pretty_print_commit(&ctx, commit, &buf); if (buf.len) { if (revs->commit_format != CMIT_FMT_ONELINE) diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 647218b4ef..7b97a90bae 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -245,6 +245,17 @@ test_expect_success '%C(auto) respects --no-color' ' test_cmp expect actual ' +test_expect_success 'rev-list %C(auto,...) respects --color' ' + git rev-list --color --format="%C(auto,green)foo%C(auto,reset)" \ + -1 HEAD >actual.raw && + test_decode_color actual && + cat >expect <<-EOF && + commit $(git rev-parse HEAD) + foo + EOF + test_cmp expect actual +' + iconv -f utf-8 -t $test_encoding > commit-msg < Date: Thu, 13 Jul 2017 11:08:46 -0400 Subject: pretty: respect color settings for %C placeholders The color placeholders have traditionally been unconditional, showing colors even when git is not otherwise configured to do so. This was not so bad for their original use, which was on the command-line (and the user could decide at that moment whether to add colors or not). But these days we have configured formats via pretty.*, and those should operate correctly in multiple contexts. In 3082517 (log --format: teach %C(auto,black) to respect color config, 2012-12-17), we gave an extended placeholder that could be used to accomplish this. But it's rather clunky to use, because you have to specify it individually for each color (and their matching resets) in the format. We shied away from just switching the default to auto, because it is technically breaking backwards compatibility. However, there's not really a use case for unconditional colors. The most plausible reason you would want them is to redirect "git log" output to a file. But there, the right answer is --color=always, as it does the right thing both with custom user-format colors and git-generated colors. So let's switch to the more useful default. In the off-chance that somebody really does find a use for unconditional colors without wanting to enable the rest of git's colors, we provide a new %C(always,...) to enable the old behavior. And we can remind them of --color=always in the documentation. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- Documentation/pretty-formats.txt | 18 +++++---- pretty.c | 27 +++++++++++-- t/t6006-rev-list-format.sh | 84 ++++++++++++++++++++++++---------------- 3 files changed, 84 insertions(+), 45 deletions(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 4d6dac5770..973d19606b 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -173,13 +173,17 @@ endif::git-rev-list[] - '%Cblue': switch color to blue - '%Creset': reset color - '%C(...)': color specification, as described under Values in the - "CONFIGURATION FILE" section of linkgit:git-config[1]; - adding `auto,` at the beginning (e.g. `%C(auto,red)`) will emit - color only when colors are enabled for log output (by `color.diff`, - `color.ui`, or `--color`, and respecting the `auto` settings of the - former if we are going to a terminal). `auto` alone (i.e. - `%C(auto)`) will turn on auto coloring on the next placeholders - until the color is switched again. + "CONFIGURATION FILE" section of linkgit:git-config[1]. + By default, colors are shown only when enabled for log output (by + `color.diff`, `color.ui`, or `--color`, and respecting the `auto` + settings of the former if we are going to a terminal). `%C(auto,...)` + is accepted as a historical synonym for the default (e.g., + `%C(auto,red)`). Specifying `%C(always,...) will show the colors + even when color is not otherwise enabled (though consider + just using `--color=always` to enable color for the whole output, + including this format and anything else git might color). `auto` + alone (i.e. `%C(auto)`) will turn on auto coloring on the next + placeholders until the color is switched again. - '%m': left (`<`), right (`>`) or boundary (`-`) mark - '%n': newline - '%%': a raw '%' diff --git a/pretty.c b/pretty.c index e4b561c582..39cad5112b 100644 --- a/pretty.c +++ b/pretty.c @@ -947,6 +947,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ struct format_commit_context *c) { const char *rest = placeholder; + const char *basic_color = NULL; if (placeholder[1] == '(') { const char *begin = placeholder + 2; @@ -955,23 +956,41 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */ if (!end) return 0; + if (skip_prefix(begin, "auto,", &begin)) { if (!want_color(c->pretty_ctx->color)) return end - placeholder + 1; + } else if (skip_prefix(begin, "always,", &begin)) { + /* nothing to do; we do not respect want_color at all */ + } else { + /* the default is the same as "auto" */ + if (!want_color(c->pretty_ctx->color)) + return end - placeholder + 1; } + if (color_parse_mem(begin, end - begin, color) < 0) die(_("unable to parse --pretty format")); strbuf_addstr(sb, color); return end - placeholder + 1; } + + /* + * We handle things like "%C(red)" above; for historical reasons, there + * are a few colors that can be specified without parentheses (and + * they cannot support things like "auto" or "always" at all). + */ if (skip_prefix(placeholder + 1, "red", &rest)) - strbuf_addstr(sb, GIT_COLOR_RED); + basic_color = GIT_COLOR_RED; else if (skip_prefix(placeholder + 1, "green", &rest)) - strbuf_addstr(sb, GIT_COLOR_GREEN); + basic_color = GIT_COLOR_GREEN; else if (skip_prefix(placeholder + 1, "blue", &rest)) - strbuf_addstr(sb, GIT_COLOR_BLUE); + basic_color = GIT_COLOR_BLUE; else if (skip_prefix(placeholder + 1, "reset", &rest)) - strbuf_addstr(sb, GIT_COLOR_RESET); + basic_color = GIT_COLOR_RESET; + + if (basic_color && want_color(c->pretty_ctx->color)) + strbuf_addstr(sb, basic_color); + return rest - placeholder; } diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 7b97a90bae..b326d550f3 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -59,7 +59,10 @@ test_format () { } # Feed to --format to provide predictable colored sequences. +BASIC_COLOR='%Credfoo%Creset' +COLOR='%C(red)foo%C(reset)' AUTO_COLOR='%C(auto,red)foo%C(auto,reset)' +ALWAYS_COLOR='%C(always,red)foo%C(always,reset)' has_color () { test_decode_color <"$1" >decoded && echo "foo" >expect && @@ -177,7 +180,7 @@ test_expect_success 'basic colors' ' foobarbazxyzzy EOF format="%Credfoo%Cgreenbar%Cbluebaz%Cresetxyzzy" && - git rev-list --format="$format" -1 master >actual.raw && + git rev-list --color --format="$format" -1 master >actual.raw && test_decode_color actual && test_cmp expect actual ' @@ -188,48 +191,61 @@ test_expect_success 'advanced colors' ' foo EOF format="%C(red yellow bold)foo%C(reset)" && - git rev-list --format="$format" -1 master >actual.raw && + git rev-list --color --format="$format" -1 master >actual.raw && test_decode_color actual && test_cmp expect actual ' -test_expect_success '%C(auto,...) does not enable color by default' ' - git log --format=$AUTO_COLOR -1 >actual && - has_no_color actual -' - -test_expect_success '%C(auto,...) enables colors for color.diff' ' - git -c color.diff=always log --format=$AUTO_COLOR -1 >actual && - has_color actual -' - -test_expect_success '%C(auto,...) enables colors for color.ui' ' - git -c color.ui=always log --format=$AUTO_COLOR -1 >actual && - has_color actual -' +for spec in \ + "%Cred:$BASIC_COLOR" \ + "%C(...):$COLOR" \ + "%C(auto,...):$AUTO_COLOR" +do + desc=${spec%%:*} + color=${spec#*:} + test_expect_success "$desc does not enable color by default" ' + git log --format=$color -1 >actual && + has_no_color actual + ' -test_expect_success '%C(auto,...) respects --color' ' - git log --format=$AUTO_COLOR -1 --color >actual && - has_color actual -' + test_expect_success "$desc enables colors for color.diff" ' + git -c color.diff=always log --format=$color -1 >actual && + has_color actual + ' -test_expect_success '%C(auto,...) respects --no-color' ' - git -c color.ui=always log --format=$AUTO_COLOR -1 --no-color >actual && - has_no_color actual -' + test_expect_success "$desc enables colors for color.ui" ' + git -c color.ui=always log --format=$color -1 >actual && + has_color actual + ' -test_expect_success TTY '%C(auto,...) respects --color=auto (stdout is tty)' ' - test_terminal env TERM=vt100 \ - git log --format=$AUTO_COLOR -1 --color=auto >actual && - has_color actual -' + test_expect_success "$desc respects --color" ' + git log --format=$color -1 --color >actual && + has_color actual + ' -test_expect_success '%C(auto,...) respects --color=auto (stdout not tty)' ' - ( - TERM=vt100 && export TERM && - git log --format=$AUTO_COLOR -1 --color=auto >actual && + test_expect_success "$desc respects --no-color" ' + git -c color.ui=always log --format=$color -1 --no-color >actual && has_no_color actual - ) + ' + + test_expect_success TTY "$desc respects --color=auto (stdout is tty)" ' + test_terminal env TERM=vt100 \ + git log --format=$color -1 --color=auto >actual && + has_color actual + ' + + test_expect_success "$desc respects --color=auto (stdout not tty)" ' + ( + TERM=vt100 && export TERM && + git log --format=$color -1 --color=auto >actual && + has_no_color actual + ) + ' +done + +test_expect_success '%C(always,...) enables color even without tty' ' + git log --format=$ALWAYS_COLOR -1 >actual && + has_color actual ' test_expect_success '%C(auto) respects --color' ' -- cgit v1.2.3 From 11b087adfd469ca597f1d269314f8cad32d0d72f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 13 Jul 2017 11:09:32 -0400 Subject: ref-filter: consult want_color() before emitting colors When color placeholders like %(color:red) are used in a ref-filter format, we unconditionally output the colors, even if the user has asked us for no colors. This usually isn't a problem when the user is constructing a --format on the command line, but it means we may do the wrong thing when the format is fed from a script or alias. For example: $ git config alias.b 'branch --format=%(color:green)%(refname)' $ git b --no-color should probably omit the green color. Likewise, running: $ git b >branches should probably also omit the color, just as we would for all baked-in coloring (and as we recently started to do for user-specified colors in --pretty formats). This commit makes both of those cases work by teaching the ref-filter code to consult want_color() before outputting any color. The color flag in ref_format defaults to "-1", which means we'll consult color.ui, which in turn defaults to the usual isatty() check on stdout. However, callers like git-branch which support their own color config (and command-line options) can override that. The new tests independently cover all three of the callers of ref-filter (for-each-ref, tag, and branch). Even though these seem redundant, it confirms that we've correctly plumbed through all of the necessary config to make colors work by default. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/branch.c | 1 + ref-filter.c | 8 ++++++++ ref-filter.h | 3 ++- t/t3203-branch-output.sh | 31 +++++++++++++++++++++++++++++++ t/t6300-for-each-ref.sh | 37 ++++++++++++++++++++++++++----------- t/t7004-tag.sh | 25 +++++++++++++++++++++++++ 6 files changed, 93 insertions(+), 12 deletions(-) diff --git a/builtin/branch.c b/builtin/branch.c index d852ded497..16d391b407 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -409,6 +409,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin if (!format->format) format->format = to_free = build_format(filter, maxwidth, remote_prefix); + format->use_color = branch_use_color; if (verify_ref_format(format)) die(_("unable to parse format string")); diff --git a/ref-filter.c b/ref-filter.c index 129a636771..bc591f4f3d 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -104,6 +104,12 @@ static void color_atom_parser(const struct ref_format *format, struct used_atom die(_("expected format: %%(color:)")); if (color_parse(color_value, atom->u.color) < 0) die(_("unrecognized color: %%(color:%s)"), color_value); + /* + * We check this after we've parsed the color, which lets us complain + * about syntactically bogus color names even if they won't be used. + */ + if (!want_color(format->use_color)) + color_parse("", atom->u.color); } static void refname_atom_parser_internal(struct refname_atom *atom, @@ -675,6 +681,8 @@ int verify_ref_format(struct ref_format *format) if (skip_prefix(used_atom[at].name, "color:", &color)) format->need_color_reset_at_eol = !!strcmp(color, "reset"); } + if (format->need_color_reset_at_eol && !want_color(format->use_color)) + format->need_color_reset_at_eol = 0; return 0; } diff --git a/ref-filter.h b/ref-filter.h index 1ffc3ca816..0d98342b34 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -79,12 +79,13 @@ struct ref_format { */ const char *format; int quote_style; + int use_color; /* Internal state to ref-filter */ int need_color_reset_at_eol; }; -#define REF_FORMAT_INIT { NULL, 0 } +#define REF_FORMAT_INIT { NULL, 0, -1 } /* Macros for checking --merged and --no-merged options */ #define _OPT_MERGED_NO_MERGED(option, filter, h) \ diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh index a428ae6703..d2aec0f38b 100755 --- a/t/t3203-branch-output.sh +++ b/t/t3203-branch-output.sh @@ -2,6 +2,7 @@ test_description='git branch display tests' . ./test-lib.sh +. "$TEST_DIRECTORY"/lib-terminal.sh test_expect_success 'make commits' ' echo content >file && @@ -239,4 +240,34 @@ test_expect_success 'git branch --format option' ' test_i18ncmp expect actual ' +test_expect_success "set up color tests" ' + echo "master" >expect.color && + echo "master" >expect.bare && + color_args="--format=%(color:red)%(refname:short) --list master" +' + +test_expect_success '%(color) omitted without tty' ' + TERM=vt100 git branch $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.bare actual +' + +test_expect_success TTY '%(color) present with tty' ' + test_terminal env TERM=vt100 git branch $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.color actual +' + +test_expect_success 'color.branch=always overrides auto-color' ' + git -c color.branch=always branch $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.color actual +' + +test_expect_success '--color overrides auto-color' ' + git branch --color $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.color actual +' + test_done diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index 7872a2f544..2274a4b733 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -7,6 +7,7 @@ test_description='for-each-ref test' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh +. "$TEST_DIRECTORY"/lib-terminal.sh # Mon Jul 3 23:18:43 2006 +0000 datestamp=1151968723 @@ -412,19 +413,33 @@ test_expect_success 'Check for invalid refname format' ' test_must_fail git for-each-ref --format="%(refname:INVALID)" ' -cat >expected <master -$(git rev-parse --short refs/remotes/origin/master) origin/master -$(git rev-parse --short refs/tags/testtag) testtag -$(git rev-parse --short refs/tags/two) two -EOF +test_expect_success 'set up color tests' ' + cat >expected.color <<-EOF && + $(git rev-parse --short refs/heads/master) master + $(git rev-parse --short refs/remotes/origin/master) origin/master + $(git rev-parse --short refs/tags/testtag) testtag + $(git rev-parse --short refs/tags/two) two + EOF + sed "s/<[^>]*>//g" expected.bare && + color_format="%(objectname:short) %(color:green)%(refname:short)" +' -test_expect_success 'Check %(color:...) ' ' - git for-each-ref \ - --format="%(objectname:short) %(color:green)%(refname:short)" \ - >actual.raw && +test_expect_success TTY '%(color) shows color with a tty' ' + test_terminal env TERM=vt100 \ + git for-each-ref --format="$color_format" >actual.raw && test_decode_color actual && - test_cmp expected actual + test_cmp expected.color actual +' + +test_expect_success '%(color) does not show color without tty' ' + TERM=vt100 git for-each-ref --format="$color_format" >actual && + test_cmp expected.bare actual +' + +test_expect_success 'color.ui=always can override tty check' ' + git -c color.ui=always for-each-ref --format="$color_format" >actual.raw && + test_decode_color actual && + test_cmp expected.color actual ' cat >expected <<\EOF diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 0ef7b94394..dd5ba450ee 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -9,6 +9,7 @@ Tests for operations with tags.' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-gpg.sh +. "$TEST_DIRECTORY"/lib-terminal.sh # creating and listing lightweight tags: @@ -1900,6 +1901,30 @@ test_expect_success '--format should list tags as per format given' ' test_cmp expect actual ' +test_expect_success "set up color tests" ' + echo "v1.0" >expect.color && + echo "v1.0" >expect.bare && + color_args="--format=%(color:red)%(refname:short) --list v1.0" +' + +test_expect_success '%(color) omitted without tty' ' + TERM=vt100 git tag $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.bare actual +' + +test_expect_success TTY '%(color) present with tty' ' + test_terminal env TERM=vt100 git tag $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.color actual +' + +test_expect_success 'color.ui=always overrides auto-color' ' + git -c color.ui=always tag $color_args >actual.raw && + test_decode_color actual && + test_cmp expect.color actual +' + test_expect_success 'setup --merged test tags' ' git tag mergetest-1 HEAD~2 && git tag mergetest-2 HEAD~1 && -- cgit v1.2.3