| Message ID | 20230715160730.4046-1-andy.koppe@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] pretty: add %(decorate[:<options>]) format | expand |
Andy Koppe <andy.koppe@gmail.com> writes: > This lists ref names in the same way as the %d decoration format, but Please replace "This" with "%(descorate[:<options>]", i.e. a more concrete form, so that people do not have to go back to the title in order to understand the body of the proposed log message. > -'%(describe[:options])':: human-readable name, like > +'%(describe[:<options>])':: human-readable name, like > -'%(trailers[:options])':: display the trailers of the body as > +'%(trailers[:<options>])':: display the trailers of the body as It is a very good idea to signal that <options> is a placeholder by enclosing it inside <angle bracket> like this patch wants to do with %(decorate), and to make sure that other existing ones consistently follow the same convention. But the latter, being very small, can be buried in the noise. It may be a good idea to have small "preliminary clean-up" patches that do not add anything related to %(decorate) at the beginning of the series. [PATCH 1/3] can be %(token[:<options>]) clean-up, [PATCH 2/3] can be "what is literal formatting code" clarification, and [PATCH 3/3] can be the rest of this patch, for example. > +'%(decorate[:<options>])':: ref names with custom decorations. > + The `decorate` string may be followed by a colon > + and zero or more comma-separated options. > + Option values may contain literal formatting codes. > + These must be used for commas (`%x2C`) and closing > + parentheses (`%x29`), due to their role in the option > + syntax. OK. I fear that "literal formatting codes" may not be understood by readers without having any cross references here. Perhaps something like the patch attached at the end of this message would help. > +** 'arrow=<value>': Shown between HEAD and the branch it points to, if any. > + Defaults to " \-> ". It feels a bit strange that this feature is limited only to "HEAD" and other symbolic refs are not annotated with an arrow, but it is not the fault of this patch. We might want to see if it is worth to extend this to other symbolic refs but not while this patch is being discussed and polished. > diff --git a/log-tree.c b/log-tree.c > index f4b22a60cc..4b46884ef6 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -301,27 +301,34 @@ static void show_name(struct strbuf *sb, const struct name_decoration *decoratio > > /* > * The caller makes sure there is no funny color before calling. > - * format_decorations_extended makes sure the same after return. > + * format_decorations ensures the same after return. > */ > -void format_decorations_extended(struct strbuf *sb, > +void format_decorations(struct strbuf *sb, > const struct commit *commit, > int use_color, > - const char *prefix, > - const char *separator, > - const char *suffix) > + const struct decoration_options *opts) Hmph, presumably the idea is to collect these parameters in a struct and pass it around, which would be easier to extend, and then teach the hardcoded default to the callee, instead of the macro. OK. It may have made the change easier to review if such a change that can cleanly separable into a single step into a single preliminary clean-up patch before we start adding the customization, but let's read on to see if I can keep everything in my head---I'll complain at the end if I can't ;-). > { > - const struct name_decoration *decoration; > - const struct name_decoration *current_and_HEAD; > - const char *color_commit = > - diff_get_color(use_color, DIFF_COMMIT); > - const char *color_reset = > - decorate_get_color(use_color, DECORATION_NONE); Getting rid of the computation of the initialization value from the above decl block, ... > + const char *color_commit, *color_reset; > + const char *prefix, *suffix, *separator, *arrow, *tag; > + > + const struct name_decoration *current_and_HEAD; ... like this, so we will return early without wasting extra cycles whose result we will not use, is a very good idea. But then, ... > + const struct name_decoration *decoration = > + get_name_decoration(&commit->object); > > - decoration = get_name_decoration(&commit->object); > if (!decoration) > return; ... I think the original is easier to follow than the updated form for the "decoration" variable, simply because the declaration part will become absolutely free, and it becomes easier to see that the computation of "decoration" is the very first thing the cycles are spent on. > + color_commit = diff_get_color(use_color, DIFF_COMMIT); > + color_reset = decorate_get_color(use_color, DECORATION_NONE); > + > + prefix = (opts && opts->prefix) ? opts->prefix : " ("; > + suffix = (opts && opts->suffix) ? opts->suffix : ")"; > + separator = (opts && opts->separator) ? opts->separator : ", "; Knowing these hardcoded values were the responsibility of the format_decorations() C preprocessor macro; now it is written here. It is a moral no-op change from the caller's point of view. > + arrow = (opts && opts->arrow) ? opts->arrow : " -> "; > + tag = (opts && opts->tag) ? opts->tag : "tag: "; These two are new. That is one thing why I wondered above if it is a good idea to separate the "refactor to introduce decoration_options structure that has three members and replace three parameters to this function with it, so that we can get rid of the format_decorations() macro" into a single preliminary step. Then the reviewers can go on, after being convinced that such a moral no-op refactoring is correct, to review the next step that would presumably add these two members to the option struct and make these customizable. > current_and_HEAD = current_pointed_by_HEAD(decoration); > + > while (decoration) { > /* > * When both current and HEAD are there, only Unrelated noise. > @@ -329,20 +336,29 @@ void format_decorations_extended(struct strbuf *sb, > * appeared, skipping the entry for current. > */ > if (decoration != current_and_HEAD) { > - strbuf_addstr(sb, color_commit); > - strbuf_addstr(sb, prefix); > - strbuf_addstr(sb, color_reset); > - strbuf_addstr(sb, decorate_get_color(use_color, decoration->type)); > - if (decoration->type == DECORATION_REF_TAG) > - strbuf_addstr(sb, "tag: "); > + const char *color = > + decorate_get_color(use_color, decoration->type); > > + if (*prefix) { > + strbuf_addstr(sb, color_commit); > + strbuf_addstr(sb, prefix); > + strbuf_addstr(sb, color_reset); > + } > + > + if (*tag && decoration->type == DECORATION_REF_TAG) { > + strbuf_addstr(sb, color); > + strbuf_addstr(sb, tag); > + strbuf_addstr(sb, color_reset); > + } > + strbuf_addstr(sb, color); > show_name(sb, decoration); Again, this mixes adding new things (i.e. customizeable "tag:" string, and "->" we see below) and improving existing things (i.e. "<color><reset>" that is presumably pointless when prefix is an empty string is shown as an empty string). Ideally, the step to move the three existing parameters to three members of the new struct should be done first WITHOUT the empty-string improvement, then another step should do the empty-string improvement (at which time, presumably existing test script may have to be adjusted), and then new features to costumize "tag:" and "->" should be added on top. > - if (current_and_HEAD && > + if (*arrow && current_and_HEAD && > decoration->type == DECORATION_REF_HEAD) { Because arrow is never allowed to be NULL, remove the above change, and ... > - strbuf_addstr(sb, " -> "); > + strbuf_addstr(sb, arrow); ... let the program crash to catch a future bug at runtime. > @@ -351,9 +367,12 @@ void format_decorations_extended(struct strbuf *sb, > } > decoration = decoration->next; > } > - strbuf_addstr(sb, color_commit); > - strbuf_addstr(sb, suffix); > - strbuf_addstr(sb, color_reset); > + > + if (*suffix) { > + strbuf_addstr(sb, color_commit); > + strbuf_addstr(sb, suffix); > + strbuf_addstr(sb, color_reset); > + } Ditto about "improving the <color><reset> empty sequence is a separate change from making various fields customizable". I'll stop here. After skimming the changes to the test, I think this single patch should be split into separate steps. Perhaps the split should go like this: * documentation clean-up %(token[:options]) -> %(token[:<options>]) plus clarification of what a "literal formatting code" is. * introduction of "struct decoration_option", removing the format_decorations() macro, renaming format_decorations_extended() to format_decorations(), replacing the three parameters with a single struct pointer. * improving <color><reset> string when the meat of the string is empty. This step will be the FIRST step that changes the externally visible behaviour, and presumably will have adjustment to existing tests. * making "tag:" and "->" customizable, if values are passed in the struct. This step does not have UI changes, and the existing tests should serve as a safety net to catch mistakes in this step. * read the %(describe[:<option>]) and fill "struct describe_option". This will be accompanied by additional tests for the new feature. Thanks. ---------------------- >8 ---------------------- Subject: pretty-formats: define "literal formatting code" The description for %(trailer) option already uses this term without having definition anywhere in the document, and we are about to add another one %(decorate) that uses it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff --git c/Documentation/pretty-formats.txt w/Documentation/pretty-formats.txt index c08aba15af..b7a3a150ae 100644 --- c/Documentation/pretty-formats.txt +++ w/Documentation/pretty-formats.txt @@ -122,7 +122,9 @@ The placeholders are: - Placeholders that expand to a single literal character: '%n':: newline '%%':: a raw '%' -'%x00':: print a byte from a hex code +'%x00':: '%x' followed by two hexadecimal digits is replaced with a + byte with the hexdecimal digits' value (we will call this + "literal formatting code" in the rest of this document). - Placeholders that affect formatting of later placeholders: '%Cred':: switch color to red
Junio C Hamano <gitster@pobox.com> writes: > I'll stop here. After skimming the changes to the test, I think this > single patch should be split into separate steps. Perhaps the split > should go like this: > ... > Thanks. Oh, sorry that I forgot to add one thing. Overall, the patch seems to be done very well when viewed as a whole. Thanks for working on it. It is just I cannot be as confident as I would like to be in my review when the single patch does several different things at once. If it were split in steps, each step focusing on doing a single thing well and describing well what it does and why, reviewers can be more confident that they did not miss something important in the patch(es).
Hi Andy! We picked up this series at Review Club. Reviewers will leave their thoughts on the mailing list, but if you like, you can find the notes at: https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit Andy Koppe <andy.koppe@gmail.com> writes: > This lists ref names in the same way as the %d decoration format, but > allows all the otherwise fixed strings printed around the ref names to > be customized, namely prefix, suffix, separator, the "tag:" annotation > and the arrow used to show where HEAD points. > > Examples: > - %(decorate) is equivalent to %d. > - %(decorate:prefix=,suffix=) is equivalent to %D. > - %(decorate:prefix=,suffix=,separator= ,tag=,arrow=->) produces a > space-separated list without wrapping, tag annotations or spaces > around the arrow. > - %(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=) produces > a comma-separated list enclosed in square brackets where the arrow is > replaced by a comma as well. I think giving the user this level of customization makes sense, especially since we do this for other format options. Importantly, this design also fits the existing conventions we have, so this looks like a good proposal. As a micro-nit: there's some useful context behind your chosen design in [1]. It would have been useful to link to it in the `---` context, or perhaps send this series as v3 and v4 to [1]. [1] https://lore.kernel.org/git/20230712110732.8274-1-andy.koppe@gmail.com/ > Add functions parse_decoration_option(), parse_decoration_options() and > free_decoration_options() to help implement the format. Test it in > t4205-log-pretty-formats.sh and document it in pretty-formats.txt. This commit adds the new feature... > Refactor format_decorations() to take a struct decoration_options > argument specifying those strings, whereby NULL entries select the > default. Avoid emitting color sequences for empty strings. does some refactoring to support the new feature + existing use cases... > Wrap tag annotations in separate color sequences from tag names, because > otherwise tag names can end up uncolored when %w width formatting breaks > lines between annotation and name. Amend t4207-log-decoration-colors.sh > accordingly. and fixes a bug with coloring that is easier to run into as a result of the new feature. As others have mentioned, I think this would be easier to follow as separate commits. This commit isn't so big that the refactor needs to be its own commit, though I don't feel strongly either way. > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt > index 3b71334459..c08aba15af 100644 > --- a/Documentation/pretty-formats.txt > +++ b/Documentation/pretty-formats.txt > @@ -222,7 +222,22 @@ The placeholders are: > linkgit:git-rev-list[1]) > '%d':: ref names, like the --decorate option of linkgit:git-log[1] > '%D':: ref names without the " (", ")" wrapping. > -'%(describe[:options])':: human-readable name, like > +'%(decorate[:<options>])':: ref names with custom decorations. > + The `decorate` string may be followed by a colon > + and zero or more comma-separated options. > + Option values may contain literal formatting codes. > + These must be used for commas (`%x2C`) and closing > + parentheses (`%x29`), due to their role in the option > + syntax. To make this easier to visualize, it would be useful to include the examples from your commit message (%d, %D, etc.). > +'%(describe[:<options>])':: human-readable name, like Ah, adding the <> is a good fix. I think it doesn't warrant its own patch, but it should be called out in the commit message. > /* > * The caller makes sure there is no funny color before calling. > - * format_decorations_extended makes sure the same after return. > + * format_decorations ensures the same after return. > */ > -void format_decorations_extended(struct strbuf *sb, > +void format_decorations(struct strbuf *sb, > const struct commit *commit, > int use_color, > - const char *prefix, > - const char *separator, > - const char *suffix) > + const struct decoration_options *opts) > { > - const struct name_decoration *decoration; > - const struct name_decoration *current_and_HEAD; > - const char *color_commit = > - diff_get_color(use_color, DIFF_COMMIT); > - const char *color_reset = > - decorate_get_color(use_color, DECORATION_NONE); > + const char *color_commit, *color_reset; > + const char *prefix, *suffix, *separator, *arrow, *tag; > + > + const struct name_decoration *current_and_HEAD; > + const struct name_decoration *decoration = > + get_name_decoration(&commit->object); > > - decoration = get_name_decoration(&commit->object); > if (!decoration) > return; > > + color_commit = diff_get_color(use_color, DIFF_COMMIT); > + color_reset = decorate_get_color(use_color, DECORATION_NONE); I'm guessing that you shuffled these lines to make use of an early return? If so, both versions are not different enough to warrant the churn IMO. It would be worth pointing out the reshuffling in the commit message, especially if you had another rationale in mind. > + prefix = (opts && opts->prefix) ? opts->prefix : " ("; > + suffix = (opts && opts->suffix) ? opts->suffix : ")"; > + separator = (opts && opts->separator) ? opts->separator : ", "; > + arrow = (opts && opts->arrow) ? opts->arrow : " -> "; > + tag = (opts && opts->tag) ? opts->tag : "tag: "; So NULL means "use the default"... > +struct decoration_options { > + char *prefix; > + char *suffix; > + char *separator; > + char *arrow; > + char *tag; > +}; > + > int parse_decorate_color_config(const char *var, const char *slot_name, const char *value); > int log_tree_diff_flush(struct rev_info *); > int log_tree_commit(struct rev_info *, struct commit *); > void show_log(struct rev_info *opt); > -void format_decorations_extended(struct strbuf *sb, const struct commit *commit, > - int use_color, > - const char *prefix, > - const char *separator, > - const char *suffix); > -#define format_decorations(strbuf, commit, color) \ > - format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")") > +void format_decorations(struct strbuf *sb, const struct commit *commit, > + int use_color, const struct decoration_options *opts); Which lets us unify these two functions. Makes sense. > +static int parse_decoration_option(const char **arg, > + const char *name, > + char **opt) > +{ > + const char *argval; > + size_t arglen; > + > + if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) { > + char *val = xstrndup(argval, arglen); > + struct strbuf sb = STRBUF_INIT; > + > + strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL); strbuf_expand() got removed in 'master' recently, so this should be rebased. > static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > const char *placeholder, > void *context) > @@ -1526,10 +1566,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ > strbuf_addstr(sb, get_revision_mark(NULL, commit)); > return 1; > case 'd': > - format_decorations(sb, commit, c->auto_color); > + format_decorations(sb, commit, c->auto_color, NULL); > return 1; > case 'D': > - format_decorations_extended(sb, commit, c->auto_color, "", ", ", ""); > + format_decorations(sb, commit, c->auto_color, > + &(struct decoration_options){"", ""}); I don't remember if C99 lets you name .prefix and .suffix here, but if so, it would be good to name them. Otherwise it's easy to get the order wrong, e.g. if someone reorders the fields in struct decoration_options. > +test_expect_success 'pretty format %decorate' ' > + git checkout -b foo && > + git commit --allow-empty -m "new commit" && > + git tag bar && > + git branch qux && > + echo " (HEAD -> foo, tag: bar, qux)" >expect1 && > + git log --format="%(decorate)" -1 >actual1 && > + test_cmp expect1 actual1 && > + echo "HEAD -> foo, tag: bar, qux" >expect2 && > + git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 && > + test_cmp expect2 actual2 && > + echo "HEAD->foo bar qux" >expect3 && > + git log --format="%(decorate:prefix=,suffix=,separator= ,arrow=->,tag=)" \ > + -1 >actual3 && > + test_cmp expect3 actual3 && > + echo "[HEAD,foo,bar,qux]" >expect4 && > + git log --format="%(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=)" \ > + -1 >actual4 && > + test_cmp expect4 actual4 > +' > + It would be useful to get some "bad" inputs to %(decorate:) to check that we handle them correctly, especially since it's implemented with while() loops. Overall, I thought this patch looks really good. Thanks!
Hi Andy On 19/07/2023 19:16, Glen Choo wrote: >> case 'D': >> - format_decorations_extended(sb, commit, c->auto_color, "", ", ", ""); >> + format_decorations(sb, commit, c->auto_color, >> + &(struct decoration_options){"", ""}); > > I don't remember if C99 lets you name .prefix and .suffix here, but if > so, it would be good to name them. Otherwise it's easy to get the order > wrong, e.g. if someone reorders the fields in struct decoration_options. That's a good suggestion. I think this would be the first use of a compound literal in the code base so it would be helpful to mention that in the commit message. We've been depending on C99 for a while now so I'd support adding this compound literal as a test balloon for compiler support. Ævar reported a while back that they are supported by IBM xlc, Oracle SunCC and HP/UX's aCC[1] and back then I looked at NonStop which seemed to offer support with the right compiler flag. Overall this is a well written, well motivated patch with a good commit message. Best Wishes Phillip [1] https://lore.kernel.org/git/87h7e61duk.fsf@evledraar.gmail.com/
Junio C Hamano <gitster@pobox.com> wrote: > Overall, the patch seems to be done very well when viewed as a > whole. Thanks for working on it. > > It is just I cannot be as confident as I would like to be in my > review when the single patch does several different things at once. > If it were split in steps, each step focusing on doing a single > thing well and describing well what it does and why, reviewers can > be more confident that they did not miss something important in the > patch(es). Thanks to Junio, Glen, Phil and the Review Club for the helpful reviews, especially the guidance on commit granularity. Sorry for not getting back to this sooner, but the v3 patch series addressing the review comments is now here: https://lore.kernel.org/git/20230715160730.4046-1-andy.koppe@gmail.com/T/#m46ad3ebbe3163821f649f7122edcabd619fc5837 Kind regards, Andy
On Wed, 19 Jul 2023 at 19:16, Glen Choo wrote: > As a micro-nit: there's some useful context behind your chosen design in > [1]. It would have been useful to link to it in the `---` context, or > perhaps send this series as v3 and v4 to [1]. > > [1] https://lore.kernel.org/git/20230712110732.8274-1-andy.koppe@gmail.com/ Point taken. > > + strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL); > > strbuf_expand() got removed in 'master' recently, so this should be > rebased. Done. I think I had started off main, wrongly assuming that it's the same as master. Thanks again, Andy
On Sun, 23 Jul 2023 at 17:26, Phillip Wood wrote: > > On 19/07/2023 19:16, Glen Choo wrote: > >> case 'D': > >> - format_decorations_extended(sb, commit, c->auto_color, "", ", ", ""); > >> + format_decorations(sb, commit, c->auto_color, > >> + &(struct decoration_options){"", ""}); > > > > I don't remember if C99 lets you name .prefix and .suffix here, but if > > so, it would be good to name them. Otherwise it's easy to get the order > > wrong, e.g. if someone reorders the fields in struct decoration_options. > > That's a good suggestion. I think this would be the first use of a > compound literal in the code base so it would be helpful to mention that > in the commit message. I've taken the suggestion, but then forgot to mention it in the commit message. Will do in the next round. > We've been depending on C99 for a while now so I'd support adding this > compound literal as a test balloon for compiler support. Ævar reported a > while back that they are supported by IBM xlc, Oracle SunCC and HP/UX's > aCC[1] and back then I looked at NonStop which seemed to offer support > with the right compiler flag. There are a number of uses of designated initializers already, so hopefully compound literals aren't too much of an extra challenge. Thanks, Andy
Andy Koppe <andy.koppe@gmail.com> writes: > There are a number of uses of designated initializers already, so > hopefully compound literals aren't too much of an extra challenge. I do not see how one leads to the other here. I'd prefer not to see use of a new construct we do not currently use mixed in a new code, even if it is mentioned in the proposed log message. If we want to use compound literals in our codebase in the longer term, we should first add a weatherballoon use to a very stable part of the codebase that rarely changes, in a single patch that is trivial to revert when a platform that matters is found to have problem with the language construct, just like what we did when we adopted the use of designated initializers. Thanks.
On 11/08/2023 21:38, Junio C Hamano wrote: > Andy Koppe <andy.koppe@gmail.com> writes: > >> There are a number of uses of designated initializers already, so >> hopefully compound literals aren't too much of an extra challenge. > > I do not see how one leads to the other here. I'd prefer not to see > use of a new construct we do not currently use mixed in a new code, > even if it is mentioned in the proposed log message. Okay. Would this style be acceptable to fulfil Glen's request to name the fields? case 'D': { const struct decoration_options opts = { .prefix = "", .suffix = "" }; format_decorations(sb, commit, c->auto_color, &opts); } return 1; Andy
Andy Koppe <andy.koppe@gmail.com> writes: > On 11/08/2023 21:38, Junio C Hamano wrote: >> Andy Koppe <andy.koppe@gmail.com> writes: >> >>> There are a number of uses of designated initializers already, so >>> hopefully compound literals aren't too much of an extra challenge. >> >> I do not see how one leads to the other here. I'd prefer not to see >> use of a new construct we do not currently use mixed in a new code, >> even if it is mentioned in the proposed log message. > > Okay. > > Would this style be acceptable to fulfil Glen's request to name the > fields? > > case 'D': > { > const struct decoration_options opts = { > .prefix = "", > .suffix = "" > }; > > format_decorations(sb, commit, c->auto_color, &opts); > } > return 1; > > Andy Sounds good to me.
Andy Koppe <andy.koppe@gmail.com> writes: > Done. I think I had started off main, wrongly assuming that it's the > same as master. If there is 'main' that is different from 'master', that sounds like a problem to me. This project predates the newer convention that allows the primary branch to be named 'main', but many new folks of course expect to see 'main', so while my primary working areas all call the primary branch 'master', it is pushed out to both names. Or at least I thought I arranged that to happen. Thanks.
On 15/08/2023 19:13, Junio C Hamano wrote: > If there is 'main' that is different from 'master', that sounds like > a problem to me. This project predates the newer convention that > allows the primary branch to be named 'main', but many new folks of > course expect to see 'main', so while my primary working areas all > call the primary branch 'master', it is pushed out to both names. > > Or at least I thought I arranged that to happen. See [1], where main currently is at v2.41.0. Regards, Andy [1] https://github.com/git/git/tree/main
Andy Koppe <andy.koppe@gmail.com> writes: > On 15/08/2023 19:13, Junio C Hamano wrote: >> If there is 'main' that is different from 'master', that sounds like >> a problem to me. This project predates the newer convention that >> allows the primary branch to be named 'main', but many new folks of >> course expect to see 'main', so while my primary working areas all >> call the primary branch 'master', it is pushed out to both names. >> Or at least I thought I arranged that to happen. > > See [1], where main currently is at v2.41.0. > > Regards, > Andy > > [1] https://github.com/git/git/tree/main Ah, that one. The CI job is unfortunately attached to that tree and updating 'master' and 'main' with the same commit at the same time wastes CI cycles, so I had to tentatively stop updating it. It used to be that 'main' was set to lag behind 'master' by 24 hours or so to prevent the problem---CI notices that the commit updated 'main' has been already dealt with 24 hours ago at 'master' and refrains from wasting time on it. But resurrecting it would still make folks confused about how 'main' is different from 'master'. Perhaps it is a good time to remove stale 'main' and keep only 'master' there?
On 15/08/2023 20:01, Junio C Hamano wrote: >> See [1], where main currently is at v2.41.0. >> >> [1] https://github.com/git/git/tree/main > > Ah, that one. The CI job is unfortunately attached to that tree and > updating 'master' and 'main' with the same commit at the same time > wastes CI cycles, so I had to tentatively stop updating it. > > It used to be that 'main' was set to lag behind 'master' by 24 hours > or so to prevent the problem---CI notices that the commit updated > 'main' has been already dealt with 24 hours ago at 'master' and > refrains from wasting time on it. But resurrecting it would still > make folks confused about how 'main' is different from 'master'. > Perhaps it is a good time to remove stale 'main' and keep only > 'master' there? An alternative might be to exclude one of the branches in the workflow file, as per [1]. Andy [1] https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-excluding-branches
On Tue, Aug 15, 2023 at 08:29:43PM +0100, Andy Koppe wrote: > On 15/08/2023 20:01, Junio C Hamano wrote: > > > See [1], where main currently is at v2.41.0. > > > > > > [1] https://github.com/git/git/tree/main > > > > Ah, that one. The CI job is unfortunately attached to that tree and > > updating 'master' and 'main' with the same commit at the same time > > wastes CI cycles, so I had to tentatively stop updating it. > > > > It used to be that 'main' was set to lag behind 'master' by 24 hours > > or so to prevent the problem---CI notices that the commit updated > > 'main' has been already dealt with 24 hours ago at 'master' and > > refrains from wasting time on it. But resurrecting it would still > > make folks confused about how 'main' is different from 'master'. > > Perhaps it is a good time to remove stale 'main' and keep only > > 'master' there? > > An alternative might be to exclude one of the branches in the workflow file, > as per [1]. I think that this should be relatively straightforward to do, and would be preferable to dropping 'main'. Here's an (untested) patch that should do the trick: --- >8 --- diff --git a/.github/workflows/check-whitespace.yml b/.github/workflows/check-whitespace.yml index a58e2dc8ad..764f46b21f 100644 --- a/.github/workflows/check-whitespace.yml +++ b/.github/workflows/check-whitespace.yml @@ -8,6 +8,8 @@ name: check-whitespace on: pull_request: types: [opened, synchronize] + branches_ignore: + - main # Avoid unnecessary builds. Unlike the main CI jobs, these are not # ci-configurable (but could be). diff --git a/.github/workflows/l10n.yml b/.github/workflows/l10n.yml index 6c3849658a..f6767a73d2 100644 --- a/.github/workflows/l10n.yml +++ b/.github/workflows/l10n.yml @@ -1,6 +1,12 @@ name: git-l10n -on: [push, pull_request_target] +on: + push: + branches_ignore: + - main + pull_request_target: + branches_ignore: + - main # Avoid unnecessary builds. Unlike the main CI jobs, these are not # ci-configurable (but could be). diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 079645b776..eaaf6a9151 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -1,6 +1,12 @@ name: CI -on: [push, pull_request] +on: + push: + branches-ignore: + - main + pull_request: + branches-ignore: + - main env: DEVELOPER: 1 --- 8< --- > [1] https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-excluding-branches Thanks, Taylor
On Tue, Aug 15, 2023 at 06:16:29PM -0400, Taylor Blau wrote: > > An alternative might be to exclude one of the branches in the workflow file, > > as per [1]. > > I think that this should be relatively straightforward to do, and would > be preferable to dropping 'main'. That was my inclination, too, though I wonder if that might cause hassles for Git for Windows: $ git ls-remote --symref https://github.com/git-for-windows/git HEAD ref: refs/heads/main HEAD a67b85bf88ddbccae96714edb64d741ddfc3a1c9 HEAD I'm not sure how big a deal it would be in practice. Obviously they carry patches that are not in upstream git and could adjust the file themselves that way. But it might introduce extra friction, and in my experience changes to "meta" files like this can be a hassle, because you often want them independently on every branch (though in theory this one only matters for the "main" branch itself). So I won't say it's obviously a bad idea, but it might bear some thinking on what the ramifications would be for downstream. -Peff
On Tuesday, August 15, 2023 10:24 PM, Jeff King wrote: >On Tue, Aug 15, 2023 at 06:16:29PM -0400, Taylor Blau wrote: > >> > An alternative might be to exclude one of the branches in the >> > workflow file, as per [1]. >> >> I think that this should be relatively straightforward to do, and >> would be preferable to dropping 'main'. > >That was my inclination, too, though I wonder if that might cause hassles for Git for >Windows: > > $ git ls-remote --symref https://github.com/git-for-windows/git HEAD > ref: refs/heads/main HEAD > a67b85bf88ddbccae96714edb64d741ddfc3a1c9 HEAD > >I'm not sure how big a deal it would be in practice. Obviously they carry patches that >are not in upstream git and could adjust the file themselves that way. But it might >introduce extra friction, and in my experience changes to "meta" files like this can be >a hassle, because you often want them independently on every branch (though in >theory this one only matters for the "main" branch itself). > >So I won't say it's obviously a bad idea, but it might bear some thinking on what the >ramifications would be for downstream. Would it not be more convenient just to add a GitHub action that set main = master for each push?
<rsbecker@nexbridge.com> writes: > Would it not be more convenient just to add a GitHub action that > set main = master for each push? If "my private working area calls the primary integration branch 'master', but for publishing repositories, I have to push it twice, once to 'master' and then to 'main'" were the problem, the solution I would rather want to see implemented is to an ability for the repository owners to set a symref that makes 'main' refer to 'master', so that I do not have to worry about the aliasing. But it is not a problem (the push refspec can be set up to send the same commit to two different branches just fine). In any case, I am not sure if it would solve the problem being discussed: when CI runner sees branches updated to commit that hasn't been worked on, a new job is created to work on that commit, and updating two branches with the same commit at the same time unfortunately means two independent CI jobs work on the same commit in parallel. The 'lagging behind by 24 hours' hack I mentioned earlier was one way to work it around, but it would confuse folks. I'd really prefer not to special case 'main' (or 'master' for that matter), primarily because some downstreams rely more heavily on 'main' as Peff pointed out, but also because the problem is not 'master' vs 'main'. If 'next' happens to become empty soon after a new cycle starts and points at the same commit as 'master', we will see the same waste of cycles between 'master' and 'next'. Thanks.
Hi Junio, On Thu, 17 Aug 2023, Junio C Hamano wrote: > <rsbecker@nexbridge.com> writes: > > [...] when CI runner sees branches updated to commit that hasn't been > worked on, a new job is created to work on that commit, and updating two > branches with the same commit at the same time unfortunately means two > independent CI jobs work on the same commit in parallel. My understanding is that the recommended way to handle this via the `concurrency` key [*1*]. That is, if we changed concurrency: group: windows-build-${{ github.ref }} cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} to concurrency: group: windows-build-${{ github.sha }} then pushing both `master` and `next` pointing at the same commit would start only one of the workflow runs immediately, keeping the second one pending until the first run is done. If the first run succeeds, the second run will pick up that status and avoid running everything all over again, via `skip-if-redundant`. Ciao, Johannes Footnote *1*: https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#concurrency
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > My understanding is that the recommended way to handle this via the > `concurrency` key [*1*]. That is, if we changed > > concurrency: > group: windows-build-${{ github.ref }} > cancel-in-progress: ${{ needs.ci-config.outputs.skip_concurrent == 'yes' }} > > to > > concurrency: > group: windows-build-${{ github.sha }} > > then pushing both `master` and `next` pointing at the same commit would > start only one of the workflow runs immediately, keeping the second one > pending until the first run is done. Perfect. It is much better than pushing 'master@{24.hours.ago} to 'main' which was what I used to do avoid the problem between these two, which I stopped doing because it did not work well. > If the first run succeeds, the second > run will pick up that status and avoid running everything all over again, > via `skip-if-redundant`. Nice. I understand that this would kick in regardless, but the right use of the concurrency key would make it far more effective. Very nice.
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index 3b71334459..c08aba15af 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -222,7 +222,22 @@ The placeholders are: linkgit:git-rev-list[1]) '%d':: ref names, like the --decorate option of linkgit:git-log[1] '%D':: ref names without the " (", ")" wrapping. -'%(describe[:options])':: human-readable name, like +'%(decorate[:<options>])':: ref names with custom decorations. + The `decorate` string may be followed by a colon + and zero or more comma-separated options. + Option values may contain literal formatting codes. + These must be used for commas (`%x2C`) and closing + parentheses (`%x29`), due to their role in the option + syntax. ++ +** 'prefix=<value>': Shown before the list of ref names. Defaults to " (". +** 'suffix=<value>': Shown after the list of ref names. Defaults to ")". +** 'separator=<value>': Shown between ref names. Defaults to ", ". +** 'arrow=<value>': Shown between HEAD and the branch it points to, if any. + Defaults to " \-> ". +** 'tag=<value>': Shown before tag names. Defaults to "tag: ". + +'%(describe[:<options>])':: human-readable name, like linkgit:git-describe[1]; empty string for undescribable commits. The `describe` string may be followed by a colon and zero or more @@ -281,7 +296,7 @@ endif::git-rev-list[] '%gE':: reflog identity email (respecting .mailmap, see linkgit:git-shortlog[1] or linkgit:git-blame[1]) '%gs':: reflog subject -'%(trailers[:options])':: display the trailers of the body as +'%(trailers[:<options>])':: display the trailers of the body as interpreted by linkgit:git-interpret-trailers[1]. The `trailers` string may be followed by a colon diff --git a/log-tree.c b/log-tree.c index f4b22a60cc..4b46884ef6 100644 --- a/log-tree.c +++ b/log-tree.c @@ -301,27 +301,34 @@ static void show_name(struct strbuf *sb, const struct name_decoration *decoratio /* * The caller makes sure there is no funny color before calling. - * format_decorations_extended makes sure the same after return. + * format_decorations ensures the same after return. */ -void format_decorations_extended(struct strbuf *sb, +void format_decorations(struct strbuf *sb, const struct commit *commit, int use_color, - const char *prefix, - const char *separator, - const char *suffix) + const struct decoration_options *opts) { - const struct name_decoration *decoration; - const struct name_decoration *current_and_HEAD; - const char *color_commit = - diff_get_color(use_color, DIFF_COMMIT); - const char *color_reset = - decorate_get_color(use_color, DECORATION_NONE); + const char *color_commit, *color_reset; + const char *prefix, *suffix, *separator, *arrow, *tag; + + const struct name_decoration *current_and_HEAD; + const struct name_decoration *decoration = + get_name_decoration(&commit->object); - decoration = get_name_decoration(&commit->object); if (!decoration) return; + color_commit = diff_get_color(use_color, DIFF_COMMIT); + color_reset = decorate_get_color(use_color, DECORATION_NONE); + + prefix = (opts && opts->prefix) ? opts->prefix : " ("; + suffix = (opts && opts->suffix) ? opts->suffix : ")"; + separator = (opts && opts->separator) ? opts->separator : ", "; + arrow = (opts && opts->arrow) ? opts->arrow : " -> "; + tag = (opts && opts->tag) ? opts->tag : "tag: "; + current_and_HEAD = current_pointed_by_HEAD(decoration); + while (decoration) { /* * When both current and HEAD are there, only @@ -329,20 +336,29 @@ void format_decorations_extended(struct strbuf *sb, * appeared, skipping the entry for current. */ if (decoration != current_and_HEAD) { - strbuf_addstr(sb, color_commit); - strbuf_addstr(sb, prefix); - strbuf_addstr(sb, color_reset); - strbuf_addstr(sb, decorate_get_color(use_color, decoration->type)); - if (decoration->type == DECORATION_REF_TAG) - strbuf_addstr(sb, "tag: "); + const char *color = + decorate_get_color(use_color, decoration->type); + if (*prefix) { + strbuf_addstr(sb, color_commit); + strbuf_addstr(sb, prefix); + strbuf_addstr(sb, color_reset); + } + + if (*tag && decoration->type == DECORATION_REF_TAG) { + strbuf_addstr(sb, color); + strbuf_addstr(sb, tag); + strbuf_addstr(sb, color_reset); + } + strbuf_addstr(sb, color); show_name(sb, decoration); - if (current_and_HEAD && + if (*arrow && current_and_HEAD && decoration->type == DECORATION_REF_HEAD) { - strbuf_addstr(sb, " -> "); + strbuf_addstr(sb, arrow); strbuf_addstr(sb, color_reset); - strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type)); + strbuf_addstr(sb, decorate_get_color( + use_color, current_and_HEAD->type)); show_name(sb, current_and_HEAD); } strbuf_addstr(sb, color_reset); @@ -351,9 +367,12 @@ void format_decorations_extended(struct strbuf *sb, } decoration = decoration->next; } - strbuf_addstr(sb, color_commit); - strbuf_addstr(sb, suffix); - strbuf_addstr(sb, color_reset); + + if (*suffix) { + strbuf_addstr(sb, color_commit); + strbuf_addstr(sb, suffix); + strbuf_addstr(sb, color_reset); + } } void show_decorations(struct rev_info *opt, struct commit *commit) @@ -368,7 +387,7 @@ void show_decorations(struct rev_info *opt, struct commit *commit) } if (!opt->show_decorations) return; - format_decorations(&sb, commit, opt->diffopt.use_color); + format_decorations(&sb, commit, opt->diffopt.use_color, NULL); fputs(sb.buf, opt->diffopt.file); strbuf_release(&sb); } diff --git a/log-tree.h b/log-tree.h index e7e4641cf8..39ab06a3ca 100644 --- a/log-tree.h +++ b/log-tree.h @@ -13,17 +13,20 @@ struct decoration_filter { struct string_list *exclude_ref_config_pattern; }; +struct decoration_options { + char *prefix; + char *suffix; + char *separator; + char *arrow; + char *tag; +}; + int parse_decorate_color_config(const char *var, const char *slot_name, const char *value); int log_tree_diff_flush(struct rev_info *); int log_tree_commit(struct rev_info *, struct commit *); void show_log(struct rev_info *opt); -void format_decorations_extended(struct strbuf *sb, const struct commit *commit, - int use_color, - const char *prefix, - const char *separator, - const char *suffix); -#define format_decorations(strbuf, commit, color) \ - format_decorations_extended((strbuf), (commit), (color), " (", ", ", ")") +void format_decorations(struct strbuf *sb, const struct commit *commit, + int use_color, const struct decoration_options *opts); void show_decorations(struct rev_info *opt, struct commit *commit); void log_write_email_headers(struct rev_info *opt, struct commit *commit, const char **extra_headers_p, diff --git a/pretty.c b/pretty.c index 0bb938021b..a59b7f0dbc 100644 --- a/pretty.c +++ b/pretty.c @@ -1373,6 +1373,46 @@ static size_t parse_describe_args(const char *start, struct strvec *args) return arg - start; } + +static int parse_decoration_option(const char **arg, + const char *name, + char **opt) +{ + const char *argval; + size_t arglen; + + if (match_placeholder_arg_value(*arg, name, arg, &argval, &arglen)) { + char *val = xstrndup(argval, arglen); + struct strbuf sb = STRBUF_INIT; + + strbuf_expand(&sb, val, strbuf_expand_literal_cb, NULL); + free(val); + *opt = strbuf_detach(&sb, NULL); + return 1; + } + return 0; +} + +static void parse_decoration_options(const char **arg, + struct decoration_options *opts) +{ + while (parse_decoration_option(arg, "prefix", &opts->prefix) || + parse_decoration_option(arg, "suffix", &opts->suffix) || + parse_decoration_option(arg, "separator", &opts->separator) || + parse_decoration_option(arg, "arrow", &opts->arrow) || + parse_decoration_option(arg, "tag", &opts->tag)) + ; +} + +static void free_decoration_options(const struct decoration_options *opts) +{ + free(opts->prefix); + free(opts->suffix); + free(opts->separator); + free(opts->arrow); + free(opts->tag); +} + static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ const char *placeholder, void *context) @@ -1526,10 +1566,11 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ strbuf_addstr(sb, get_revision_mark(NULL, commit)); return 1; case 'd': - format_decorations(sb, commit, c->auto_color); + format_decorations(sb, commit, c->auto_color, NULL); return 1; case 'D': - format_decorations_extended(sb, commit, c->auto_color, "", ", ", ""); + format_decorations(sb, commit, c->auto_color, + &(struct decoration_options){"", ""}); return 1; case 'S': /* tag/branch like --source */ if (!(c->pretty_ctx->rev && c->pretty_ctx->rev->sources)) @@ -1627,6 +1668,23 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */ return 2; } + if (skip_prefix(placeholder, "(decorate", &arg)) { + struct decoration_options opts = { NULL }; + size_t ret = 0; + + if (*arg == ':') { + arg++; + parse_decoration_options(&arg, &opts); + } + if (*arg == ')') { + format_decorations(sb, commit, c->auto_color, &opts); + ret = arg - placeholder + 1; + } + + free_decoration_options(&opts); + return ret; + } + /* For the rest we have to parse the commit header. */ if (!c->commit_header_parsed) { msg = c->message = diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh index 4cf8a77667..5ea937648a 100755 --- a/t/t4205-log-pretty-formats.sh +++ b/t/t4205-log-pretty-formats.sh @@ -576,6 +576,27 @@ test_expect_success 'clean log decoration' ' test_cmp expected actual1 ' +test_expect_success 'pretty format %decorate' ' + git checkout -b foo && + git commit --allow-empty -m "new commit" && + git tag bar && + git branch qux && + echo " (HEAD -> foo, tag: bar, qux)" >expect1 && + git log --format="%(decorate)" -1 >actual1 && + test_cmp expect1 actual1 && + echo "HEAD -> foo, tag: bar, qux" >expect2 && + git log --format="%(decorate:prefix=,suffix=)" -1 >actual2 && + test_cmp expect2 actual2 && + echo "HEAD->foo bar qux" >expect3 && + git log --format="%(decorate:prefix=,suffix=,separator= ,arrow=->,tag=)" \ + -1 >actual3 && + test_cmp expect3 actual3 && + echo "[HEAD,foo,bar,qux]" >expect4 && + git log --format="%(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=)" \ + -1 >actual4 && + test_cmp expect4 actual4 +' + cat >trailers <<EOF Signed-off-by: A U Thor <author@example.com> Acked-by: A U Thor <author@example.com> diff --git a/t/t4207-log-decoration-colors.sh b/t/t4207-log-decoration-colors.sh index ded33a82e2..3a4eedc494 100755 --- a/t/t4207-log-decoration-colors.sh +++ b/t/t4207-log-decoration-colors.sh @@ -55,13 +55,15 @@ test_expect_success 'commit decorations colored correctly' ' cat >expect <<-EOF && ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \ ${c_reset}${c_branch}main${c_reset}${c_commit}, \ -${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit}, \ -${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B -${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A1${c_reset}${c_commit}, \ +${c_reset}${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \ +${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B +${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}A1${c_reset}${c_commit}, \ ${c_reset}${c_remoteBranch}other/main${c_reset}${c_commit})${c_reset} A1 - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_stash}refs/stash${c_reset}${c_commit})${c_reset} \ -On main: Changes to A.t - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_stash}refs/stash${c_reset}${c_commit})${c_reset} On main: Changes to A.t + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A EOF git log --first-parent --no-abbrev --decorate --oneline --color=always --all >actual && @@ -78,10 +80,12 @@ test_expect_success 'test coloring with replace-objects' ' cat >expect <<-EOF && ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \ ${c_reset}${c_branch}main${c_reset}${c_commit}, \ -${c_reset}${c_tag}tag: D${c_reset}${c_commit})${c_reset} D - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: C${c_reset}${c_commit}, \ +${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit})${c_reset} D + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}C${c_reset}${c_commit}, \ ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} B - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A EOF git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual && @@ -102,11 +106,13 @@ test_expect_success 'test coloring with grafted commit' ' cat >expect <<-EOF && ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_HEAD}HEAD -> \ ${c_reset}${c_branch}main${c_reset}${c_commit}, \ -${c_reset}${c_tag}tag: D${c_reset}${c_commit}, \ +${c_reset}${c_tag}tag: ${c_reset}${c_tag}D${c_reset}${c_commit}, \ ${c_reset}${c_grafted}replaced${c_reset}${c_commit})${c_reset} D - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: v1.0${c_reset}${c_commit}, \ -${c_reset}${c_tag}tag: B${c_reset}${c_commit})${c_reset} B - ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}${c_tag}tag: A${c_reset}${c_commit})${c_reset} A + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}v1.0${c_reset}${c_commit}, \ +${c_reset}${c_tag}tag: ${c_reset}${c_tag}B${c_reset}${c_commit})${c_reset} B + ${c_commit}COMMIT_ID${c_reset}${c_commit} (${c_reset}\ +${c_tag}tag: ${c_reset}${c_tag}A${c_reset}${c_commit})${c_reset} A EOF git log --first-parent --no-abbrev --decorate --oneline --color=always HEAD >actual &&
This lists ref names in the same way as the %d decoration format, but allows all the otherwise fixed strings printed around the ref names to be customized, namely prefix, suffix, separator, the "tag:" annotation and the arrow used to show where HEAD points. Examples: - %(decorate) is equivalent to %d. - %(decorate:prefix=,suffix=) is equivalent to %D. - %(decorate:prefix=,suffix=,separator= ,tag=,arrow=->) produces a space-separated list without wrapping, tag annotations or spaces around the arrow. - %(decorate:prefix=[,suffix=],separator=%x2C,arrow=%x2C,tag=) produces a comma-separated list enclosed in square brackets where the arrow is replaced by a comma as well. Add functions parse_decoration_option(), parse_decoration_options() and free_decoration_options() to help implement the format. Test it in t4205-log-pretty-formats.sh and document it in pretty-formats.txt. Refactor format_decorations() to take a struct decoration_options argument specifying those strings, whereby NULL entries select the default. Avoid emitting color sequences for empty strings. Wrap tag annotations in separate color sequences from tag names, because otherwise tag names can end up uncolored when %w width formatting breaks lines between annotation and name. Amend t4207-log-decoration-colors.sh accordingly. Signed-off-by: Andy Koppe <andy.koppe@gmail.com> --- Corrected mistakes in commit description. Documentation/pretty-formats.txt | 19 ++++++++- log-tree.c | 69 ++++++++++++++++++++------------ log-tree.h | 17 ++++---- pretty.c | 62 +++++++++++++++++++++++++++- t/t4205-log-pretty-formats.sh | 21 ++++++++++ t/t4207-log-decoration-colors.sh | 32 +++++++++------ 6 files changed, 171 insertions(+), 49 deletions(-)