{"thread":{"id":"57454","subject":"Pretty format color bug","startedAt":"2022-02-21T22:44:51Z","lastAt":"2022-04-08T23:40:40Z","messageCount":11,"participants":["Raphael Bauer","Ævar Arnfjörð Bjarmason","Raphael","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"449061","messageId":"b5b7cb21-30a9-32b8-cadd-1a768f5b98b2@pdev.de","threadId":"57454","inReplyTo":null,"subject":"Pretty format color bug","fromName":"Raphael Bauer","fromEmail":"raphael@pdev.de","sentAt":"2022-02-21T22:38:30Z","receivedAt":"2022-02-21T22:44:51Z","isPatch":false,"sender":{"key":"raphael@pdev.de","avatar":null},"body":"Hello,\n\nI think I found a minor formatting bug when using the custom pretty format:\n\n* What did you do before the bug happened? (Steps to reproduce your issue)\ngit log --pretty=format:'%h%Cred%+d test'\n\n* What did you expect to happen? (Expected behavior)\nA listing of commit hashes and, if ref names for this commit exist, a \nsecond line.\nThis line should be colored in red and contain the ref names as well as \nthe string 'test'.\nIn the case of no refs, the string 'test' should appear in line with the \ncommit hash, also in red.\n\n* What happened instead? (Actual behavior)\nIn case of ref names / a second line, the color is missing completely.\nThe colors work correctly for the single line case (when no ref names \nare available).\n\n* What's different between what you expected and what actually happened?\nThe %+d placeholder inserts newlines if the string is non-empty, but in \ndoing so, resets any coloring information.\nThis is demonstrated by the string 'test' which should always show in \nred, but does so only if %+d is not expanded.\nThis makes it currently impossible to color anything with the %+ \nplaceholder.\n\n[System Info]\ngit version:\ngit version 2.35.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.16.10-arch1-1 #1 SMP PREEMPT Wed, 16 Feb 2022 19:35:18 \n+0000 x86_64\ncompiler info: gnuc: 11.1\nlibc info: glibc: 2.35\n$SHELL (typically, interactive shell): /bin/bash\n\n[Enabled Hooks]\n"},{"id":"449065","messageId":"220222.865yp7aj6z.gmgdl@evledraar.gmail.com","threadId":"57454","inReplyTo":"b5b7cb21-30a9-32b8-cadd-1a768f5b98b2@pdev.de","subject":"Re: Pretty format color bug","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T23:29:48Z","receivedAt":"2022-02-21T23:33:45Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n> I think I found a minor formatting bug when using the custom pretty format:\n>\n> * What did you do before the bug happened? (Steps to reproduce your issue)\n> git log --pretty=format:'%h%Cred%+d test'\n> [...]\n> In case of ref names / a second line, the color is missing completely.\n\nI think you've found a \"bug\" or unexpected feature, but it's not in git,\nbut in your pager.\n\nTry:\n\n    PAGER=cat git log --pretty=format:'%h%Cred%+d test'\n\nI'm assuming you're using \"less\" and are about to read about its\nbehavior of resetting colors that are still in effect when it hits a\nnewline.\n\nI hope that helps.\n"},{"id":"449066","messageId":"6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de","threadId":"57454","inReplyTo":"220222.865yp7aj6z.gmgdl@evledraar.gmail.com","subject":"Re: Pretty format color bug","fromName":"Raphael Bauer","fromEmail":"raphael@pdev.de","sentAt":"2022-02-22T00:15:47Z","receivedAt":"2022-02-22T00:16:14Z","isPatch":false,"sender":{"key":"raphael@pdev.de","avatar":null},"body":"> I think you've found a \"bug\" or unexpected feature, but it's not in git,\n> but in your pager.\n> \n> Try:\n>      PAGER=cat git log --pretty=format:'%h%Cred%+d test'\n\nInteresting, cat works correctly.\nBut this still fails for me:\nPAGER=cat git log --pretty=format:'%h%Cred%+d test' --graph\n\nThe pipe symbol printed (because of --graph) for the second line is \ncolored and resets the color of the refnames and 'test'.\nNow this looks like a bug in git?\n\n> I'm assuming you're using \"less\" and are about to read about its\n> behavior of resetting colors that are still in effect when it hits a\n> newline.\n\nThere is a section (-R) that mentions less assumes every line starts \nnormal / uncolored. Shouldn't git follow this assumption when printing \ncolorized to less?\n"},{"id":"451934","messageId":"600744a4-8d5a-0ef3-6e75-e3637e7070aa@pdev.de","threadId":"57454","inReplyTo":"6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de","subject":"BUG: %+ format placeholder and colors","fromName":"Raphael Bauer","fromEmail":"raphael@pdev.de","sentAt":"2022-03-22T23:42:51Z","receivedAt":"2022-03-22T23:46:17Z","isPatch":false,"sender":{"key":"raphael@pdev.de","avatar":null},"body":"Hello,\n\nI am not familiar with using mailing list for bug tracking. I already \nreported this bug, but got no answer, so I wanted to mention it again.\n\n* What did you do before the bug happened? (Steps to reproduce your issue)\ngit log --pretty=format:'%h%Cred%+d test' --graph\n\nThis also happens with a PAGER=cat environment, just to preclude this is \nrelated to the pager.\n\n* What did you expect to happen? (Expected behavior)\nA graph listing of commit hashes and, if ref names for this commit \nexist, a second line.\nThis line should be colored in red and contain the ref names as well as \nthe string 'test'.\nIn the case of no refs, the string 'test' should appear in line with the \ncommit hash, also in red.\n\n* What happened instead? (Actual behavior)\nIn case of ref names / a second line, the color is missing completely.\nThe colors work correctly for the single line case (when no ref names \nare available).\n\n* What's different between what you expected and what actually happened?\nThe %+d placeholder inserts newlines if the string is non-empty, but in \ndoing so, resets any coloring information.\nThis is demonstrated by the string 'test' which should always show in \nred, but does so only if %+d is not expanded.\nThis makes it currently impossible to color anything with the %+ \nplaceholder.\n\n\nIf this is already tracked, I apologize.\n"},{"id":"451938","messageId":"220323.86wnglnzrq.gmgdl@evledraar.gmail.com","threadId":"57454","inReplyTo":"600744a4-8d5a-0ef3-6e75-e3637e7070aa@pdev.de","subject":"Re: BUG: %+ format placeholder and colors","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-23T02:49:00Z","receivedAt":"2022-03-23T02:55:43Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 23 2022, Raphael Bauer wrote:\n\n> Hello,\n>\n> I am not familiar with using mailing list for bug tracking. I already\n> reported this bug, but got no answer, so I wanted to mention it again.\n\nThere was my answer at\nhttps://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/\nand your reply at\nhttps://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/;\nPerhaps not the answer you wanted, but not \"no answer\"...\n\n> * What did you do before the bug happened? (Steps to reproduce your issue)\n> git log --pretty=format:'%h%Cred%+d test' --graph\n>\n> This also happens with a PAGER=cat environment, just to preclude this\n> is related to the pager.\n\nThis example also renders differently under PAGER=cat for me. I.e. it's\nthe pager that's resetting the colors, but maybe I'm not understanding\nthe subtlety here...\n\n> * What did you expect to happen? (Expected behavior)\n> A graph listing of commit hashes and, if ref names for this commit\n> exist, a second line.\n> This line should be colored in red and contain the ref names as well\n> as the string 'test'.\n> In the case of no refs, the string 'test' should appear in line with\n> the commit hash, also in red.\n>\n> * What happened instead? (Actual behavior)\n> In case of ref names / a second line, the color is missing completely.\n> The colors work correctly for the single line case (when no ref names\n> are available).\n>\n> * What's different between what you expected and what actually happened?\n> The %+d placeholder inserts newlines if the string is non-empty, but\n> in doing so, resets any coloring information.\n> This is demonstrated by the string 'test' which should always show in\n> red, but does so only if %+d is not expanded.\n> This makes it currently impossible to color anything with the %+\n> placeholder.\n\nAt least for me a screenshot & a link to a repo you expect to run this\non would really help, since the output is very different depending on\nbranch topology, ref names etc.\n"},{"id":"452009","messageId":"e8417ad5-f9f2-c1de-75f6-45be49f0011b@pdev.de","threadId":"57454","inReplyTo":"220323.86wnglnzrq.gmgdl@evledraar.gmail.com","subject":"Re: BUG: %+ format placeholder and colors","fromName":"Raphael","fromEmail":"raphael@pdev.de","sentAt":"2022-03-23T18:12:03Z","receivedAt":"2022-03-23T18:12:31Z","isPatch":false,"sender":{"key":"raphael@pdev.de","avatar":null},"body":" > At least for me a screenshot & a link to a repo you expect to run this\n > on would really help, since the output is very different depending on\n > branch topology, ref names etc.\n\nI believe the branch topology is not relevant to reproduce this bug, it \nhappens in my small demo repo with two commits as well as big repos.\n\nI uploaded a screenshot at https://qu.ax/Mzuv.png\nThe key is that lines starting with the pipe symbol should have their \ntext printed in red, which does not happen.\n\nThe general idea is to highlight when a commit has refnames, which I \nwant to print in an optional second line and colorized. The latter seems \nnot possible currently.\n\nThe exact command used was:\ngit log --pretty=format:'%h %s%Cred%+d test' --graph\n"},{"id":"453138","messageId":"20220405154529.966434-1-raphael@pdev.de","threadId":"57454","inReplyTo":"e8417ad5-f9f2-c1de-75f6-45be49f0011b@pdev.de","subject":"[PATCH] pretty format: fix colors on %+ placeholder newline","fromName":"Raphael Bauer","fromEmail":"raphael@pdev.de","sentAt":"2022-04-05T15:45:29Z","receivedAt":"2022-04-05T21:50:09Z","isPatch":true,"sender":{"key":"raphael@pdev.de","avatar":null},"body":"Previously, the color escape codes were not printed again when a %+\nplaceholder was expanded, which could lead to missing colors.\n\nIn particular, the following command on any commit history exercised the\nproblem:\n\ngit log --pretty=format:'%h%Cred%+d test' --graph\n\nThe string 'test' should always be in red, but is not when commits have\nref names associated and the %+d placeholder is expanded.\nThis is also not a problem of any pager since the --graph option adds a\ncolored pipe symbol at the beginning of the line, which makes re-setting\nthe color again afterwards necessary.\n---\n pretty.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex ee6114e3f0..0aabba89ca 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -863,6 +863,7 @@ struct format_commit_context {\n \tsize_t width, indent1, indent2;\n \tint auto_color;\n \tint padding;\n+\tchar curr_color_escape[COLOR_MAXLEN];\n \n \t/* These offsets are relative to the start of the commit message. */\n \tstruct chunk author;\n@@ -1050,6 +1051,7 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \t\tif (color_parse_mem(begin, end - begin, color) < 0)\n \t\t\tdie(_(\"unable to parse --pretty format\"));\n \t\tstrbuf_addstr(sb, color);\n+\t\tstrlcpy(c->curr_color_escape, color, COLOR_MAXLEN);\n \t\treturn end - placeholder + 1;\n \t}\n \n@@ -1067,8 +1069,10 @@ static size_t parse_color(struct strbuf *sb, /* in UTF-8 */\n \telse if (skip_prefix(placeholder + 1, \"reset\", &rest))\n \t\tbasic_color = GIT_COLOR_RESET;\n \n-\tif (basic_color && want_color(c->pretty_ctx->color))\n+\tif (basic_color && want_color(c->pretty_ctx->color)) {\n \t\tstrbuf_addstr(sb, basic_color);\n+\t\tstrlcpy(c->curr_color_escape, basic_color, COLOR_MAXLEN);\n+\t}\n \n \treturn rest - placeholder;\n }\n@@ -1360,8 +1364,10 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */\n \tcase 'C':\n \t\tif (starts_with(placeholder + 1, \"(auto)\")) {\n \t\t\tc->auto_color = want_color(c->pretty_ctx->color);\n-\t\t\tif (c->auto_color && sb->len)\n+\t\t\tif (c->auto_color && sb->len) {\n \t\t\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n+\t\t\t\tc->curr_color_escape[0] = 0;\n+\t\t\t}\n \t\t\treturn 7; /* consumed 7 bytes, \"C(auto)\" */\n \t\t} else {\n \t\t\tint ret = parse_color(sb, placeholder, c);\n@@ -1813,9 +1819,11 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\twhile (sb->len && sb->buf[sb->len - 1] == '\\n')\n \t\t\tstrbuf_setlen(sb, sb->len - 1);\n \t} else if (orig_len != sb->len) {\n-\t\tif (magic == ADD_LF_BEFORE_NON_EMPTY)\n+\t\tif (magic == ADD_LF_BEFORE_NON_EMPTY) {\n \t\t\tstrbuf_insertstr(sb, orig_len, \"\\n\");\n-\t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n+\t\t\tstrbuf_insertstr(sb, orig_len + 1,\n+\t\t\t\t((struct format_commit_context *)context)->curr_color_escape);\n+\t\t} else if (magic == ADD_SP_BEFORE_NON_EMPTY)\n \t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n \t}\n \treturn consumed + 1;\n-- \n2.35.1\n\n"},{"id":"453232","messageId":"xmqq8rshx687.fsf@gitster.g","threadId":"57454","inReplyTo":"20220405154529.966434-1-raphael@pdev.de","subject":"Re: [PATCH] pretty format: fix colors on %+ placeholder newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-06T21:16:40Z","receivedAt":"2022-04-06T21:38:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raphael Bauer <raphael@pdev.de> writes:\n\n> Previously, the color escape codes were not printed again when a %+\n> placeholder was expanded, which could lead to missing colors.\n\nIt is very good to start the proposed log message by describing the\ncurrent behaviour, highlighting what problem it has.  Because the\nmessage is merely _proposing_ to make this change, which may or may\nnot be accepted into the codebase, however, please describe the\nstatus quo in the present tense.  \"We behave this way\", not \"we used\nto behave this way\".  There is no need to say \"Currently\" (and it is\nsimply misleading to say \"Previously\").\n\n> In particular, the following command on any commit history exercised the\n> problem:\n>\n> git log --pretty=format:'%h%Cred%+d test' --graph\n>\n> The string 'test' should always be in red, but is not when commits have\n> ref names associated and the %+d placeholder is expanded.\n> This is also not a problem of any pager since the --graph option adds a\n> colored pipe symbol at the beginning of the line, which makes re-setting\n> the color again afterwards necessary.\n\nIsn't it the problem with the \"--graph\" codepath, then?\n\nIt is unclear from the proposed log message why it is a good idea to\nadd the new behaviour to the code that handles \"%+\" unconditionally.\nIt also is unclear why the new behaviour to save only one \"color\nescape\" is sufficient.  For example, if we used\n\n    git log --pretty='format:%h%C(green)%C(reverse)%+d test' --graph\n\nwouldn't the effects of these two add up?  Would it be sufficient to\nremember the last one we saw and re-enable it?\n\nCombining the above two together, it makes me wonder if the right\napproach is instead to (1) stop resetting at the end of --graph, and\n(2) instead \"save\" the attribute before starting to draw --graph,\ndraw the --graph in color, and then \"restore\" the attribute.\n\nWhatever approach we decide to take to solve this issue, let's have\na test case or two added to the test suite to better document the\nissue.\n\nThanks.\n"},{"id":"453333","messageId":"978e7684-2b91-379b-2fdf-bf0453bff30c@pdev.de","threadId":"57454","inReplyTo":"xmqq8rshx687.fsf@gitster.g","subject":"Re: [PATCH] pretty format: fix colors on %+ placeholder newline","fromName":"Raphael","fromEmail":"raphael@pdev.de","sentAt":"2022-04-08T13:18:39Z","receivedAt":"2022-04-08T13:22:08Z","isPatch":true,"sender":{"key":"raphael@pdev.de","avatar":null},"body":"On 06.04.22 23:16, Junio C Hamano wrote:\n> It is very good to start the proposed log message by describing the\n> current behaviour, highlighting what problem it has.  Because the\n> message is merely _proposing_ to make this change, which may or may\n> not be accepted into the codebase, however, please describe the\n> status quo in the present tense. \nI understand that for proposing changes this makes sense.\nBut if the change would be accepted into the code, wouldn't the text \nbecome the commit message? I assumed it is sensible to write the message \nfor that case: Using the present tense for the state of the code at this \ncommit / after the patch.\n\n> Isn't it the problem with the \"--graph\" codepath, then?\n> \n> It is unclear from the proposed log message why it is a good idea to\n> add the new behaviour to the code that handles \"%+\" unconditionally.\n\nGood point, let me explain my thinking:\nI first reported this bug without the --graph option where the color on \nthe second line is missing as well.\nIt was pointed out that this is a problem with the pager \"less\" and not \na bug in git:\nhttps://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/\nhttps://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/\n\nUsing \"cat\" as a pager produces the correct coloring, but since \"less\" \nis the default pager I find it useful to follow its conventions: Namely \nthat lines are started non-colored and escape sequences must be repeated \nat the beginning of each colored line.\nThis is achieved as well by my implementation.\n\n> It also is unclear why the new behaviour to save only one \"color\n> escape\" is sufficient.  For example, if we used\n> \n>      git log --pretty='format:%h%C(green)%C(reverse)%+d test' --graph\n> \n> wouldn't the effects of these two add up?\n\nYou are right, I forgot about this case.\nA naive solution would be to concatenate the format escapes and clearing \nthe string when the color is reset.\nIs there already existing code for keeping track of which format strings \noverride each other, so that only the required escape sequences must be \nstored/printed?\nOr do you see a different, more elegant solution?\n\n> Whatever approach we decide to take to solve this issue, let's have\n> a test case or two added to the test suite to better document the\n> issue.\n\nSure, I will take a look after solving the core issue.\n"},{"id":"453336","messageId":"220408.86ee27bmws.gmgdl@evledraar.gmail.com","threadId":"57454","inReplyTo":"978e7684-2b91-379b-2fdf-bf0453bff30c@pdev.de","subject":"Re: [PATCH] pretty format: fix colors on %+ placeholder newline","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-04-08T15:14:36Z","receivedAt":"2022-04-08T15:44:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Apr 08 2022, Raphael wrote:\n\n> On 06.04.22 23:16, Junio C Hamano wrote:\n> Good point, let me explain my thinking:\n> I first reported this bug without the --graph option where the color\n> on the second line is missing as well.\n> It was pointed out that this is a problem with the pager \"less\" and\n> not a bug in git:\n> https://lore.kernel.org/git/220222.865yp7aj6z.gmgdl@evledraar.gmail.com/\n> https://lore.kernel.org/git/6f0d1c12-4cf8-bbdd-96d4-eae99317e2e4@pdev.de/\n\nTo be clear I meant there that at least 1/2 of what you were proposing\nwas really a feature request. I.e. that we pro-actively work around a\ndetected pager when coloring is still in effect when we hit a \\n.\n\n> Using \"cat\" as a pager produces the correct coloring, but since \"less\"\n> is the default pager I find it useful to follow its conventions:\n> Namely that lines are started non-colored and escape sequences must be\n> repeated at the beginning of each colored line.\n> This is achieved as well by my implementation.\n\n*nod*, do we forsee any fallout from doing that where ANSI escapes now\nreach \"across\" lines for people who were relying on the previous\nbehavior?\n\nI.e. you're changing how %Cred works, which is a synonym for\n%C(red). Perhaps this should be %C(red:across-nl).\n\n>> It also is unclear why the new behaviour to save only one \"color\n>> escape\" is sufficient.  For example, if we used\n>>      git log --pretty='format:%h%C(green)%C(reverse)%+d test'\n>> --graph\n>> wouldn't the effects of these two add up?\n>\n> You are right, I forgot about this case.\n> A naive solution would be to concatenate the format escapes and\n> clearing the string when the color is reset.\n> Is there already existing code for keeping track of which format\n> strings override each other, so that only the required escape\n> sequences must be stored/printed?\n> Or do you see a different, more elegant solution?\n\nRight now when we emit any color we do e.g.:\n\n    %C(red)<thing>%C(reset)\n\nWhere as what you're really asking for, if I'm understandng it\ncorrectly, is:\n\n    %C(red)%C(save)<thing>%C(reset)%C(restore)\n\nOr equivalent, and then you'd like to have the vertical pipe (and\nanything else) that's printed at the beginning of a line to do the\n\"restore\". Is that correct?\n\n>> Whatever approach we decide to take to solve this issue, let's have\n>> a test case or two added to the test suite to better document the\n>> issue.\n>\n> Sure, I will take a look after solving the core issue.\n\nSee \"test_decode_color\" for numerous examples.\n\nAnyway, just take the above as suggestions. I really haven't looked\ndeeply enough into this to form any sort of strong opinion.\n\nExcept that I really think it would be useful to split up these logical\nchanges, and have a smal series that:\n\n 1. Tests for the current behavior of both\n 2. Does just the \"across lines\" care, perhaps only if we detect less as a pager?\n 3. Does the \"don't just reset, but reset back to what was the state one\n    before the coloring preceding the reset\"\n\nBut now as I'm finishing up this E-Mail & testing your patch I was\nexpecting that this would \"keep\" the color such that my %s would always\nbe red, i.e. across the vertical bars:\n\n\tgit log --oneline --graph --pretty=format:\"%s => %Cred%aN <%aE>\"\n\nWhich it's not, but it is what we do before this patch with:\n\n    git -c core.pager=cat -c color.ui=always log --oneline --pretty=format:\"%s => %Cred%aN <%aE>\" | head -n 3\n\nBut if I do it with your patch it does it for some things (the branch\nnames) if I put %+d in there:\n\n    git -c color.ui=always log --oneline --pretty=format:\"%s => %Cred%aN%+d<%aE>\"\n\nBut still not the subjects? I'm also confused by:\n\t\n\tThis is also not a problem of any pager since the --graph option adds a\n\tcolored pipe symbol at the beginning of the line, which makes re-setting\n\tthe color again afterwards necessary.\n\nSince I can't find any way to do this with --graph that'll emit coloring\nacross the various colored bars it emits with your patch.\n\nHrm, I partially take that back, it'll do it for the ref names, but not\nthe subjects, so I suppose it's the same.\n\n:)\n\nAnyway, I think all of the above might make a good case that it would be\nreally good to do this more incrementally, and with tests before/after\nfor the various formats/resets etc.\n\n"},{"id":"453360","messageId":"xmqqmtgvjg9a.fsf@gitster.g","threadId":"57454","inReplyTo":"978e7684-2b91-379b-2fdf-bf0453bff30c@pdev.de","subject":"Re: [PATCH] pretty format: fix colors on %+ placeholder newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-04-08T23:40:33Z","receivedAt":"2022-04-08T23:40:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Raphael <raphael@pdev.de> writes:\n\n> On 06.04.22 23:16, Junio C Hamano wrote:\n>> It is very good to start the proposed log message by describing the\n>> current behaviour, highlighting what problem it has.  Because the\n>> message is merely _proposing_ to make this change, which may or may\n>> not be accepted into the codebase, however, please describe the\n>> status quo in the present tense. \n> I understand that for proposing changes this makes sense.\n\nOK\n\n> But if the change would be accepted into the code, wouldn't the text\n> become the commit message?\n\nYes.  Our history records what proposals were made in the past, and\n\"git log\" is the way to learn about them.\n\n> I assumed it is sensible to write the\n> message for that case: Using the present tense for the state of the\n> code at this commit / after the patch.\n\nI was not saying what you did was unconditionally wrong.  I\nunderstand that you wrote in the past tense with good intentions.\nThe conventions are different from project and project, and I was\nmerely telling you what the project convention around here is.\n\n> Using \"cat\" as a pager produces the correct coloring, but since \"less\"\n> is the default pager I find it useful to follow its conventions:\n> Namely that lines are started non-colored and escape sequences must be\n> repeated at the beginning of each colored line.\n\nEven if it may be a problem with the pagers, we are OK to help such\npagers if the workaround we need to make is reasonable and does not\ninvolve too much 'bending over backwards' work.  That much we have\nalready established before seeing this patch in the previous\ndiscussion thread on this, I thought ;-)\n\nBut this is a behaviour change, if not limited to the --graph thing,\nthat probably would affect other usecases negatively.\n\n>> It also is unclear why the new behaviour to save only one \"color\n>> escape\" is sufficient.  For example, if we used\n>>      git log --pretty='format:%h%C(green)%C(reverse)%+d test'\n>> --graph\n>> wouldn't the effects of these two add up?\n>\n> You are right, I forgot about this case.\n> A naive solution would be to concatenate the format escapes and\n> clearing the string when the color is reset.\n\nThere is vt100/ansi sequence to save/restore attributes, no?  I am\nnot sure if it passes the \"reasonable and not too much\" bar if we\nhave to save all the previous attribute requests ourselves.\n"}]}