{"thread":{"id":"54194","subject":"[PATCH] pretty: allow to override the built-in formats","startedAt":"2020-09-05T19:24:32Z","lastAt":"2020-09-09T18:28:06Z","messageCount":10,"participants":["Beat Bolli","Denton Liu","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"405079","messageId":"20200905192406.74411-1-dev+git@drbeat.li","threadId":"54194","inReplyTo":null,"subject":"[PATCH] pretty: allow to override the built-in formats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2020-09-05T19:24:06Z","receivedAt":"2020-09-05T19:24:32Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"In 1f0fc1db8599 (pretty: implement 'reference' format, 2019-11-19), the\n\"reference\" format was added. As a built-in format, it cannot be\noverridden, although different projects may have divergent conventions\non how to format a commit reference. E.g., Git uses\n\n (, ) [1]\n\nwhile Linux uses\n\n (\"\") [2]\n\nTeach pretty to look at a different set of config variables, all\nstarting with \"override\" (e.g. \"pretty.overrideReference\"), to override\nthe built-in formats. Note that a format called \"override\" by itself is\nnot affected. The prefix was chosen to make it clear to the user that\nthis should not be done without thought, as it may cause issues with\nother tools that expect the built-in formats to be immutable.\n\n[1] https://github.com/git/git/blob/3a238e539bcdfe3f9eb5010fd218640c1b499f7a/Documentation/SubmittingPatches#L144\n[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v5.9-rc3#n167\n\nSigned-off-by: Beat Bolli \n---\nI intend to also submit a patch to gitk that will use \"git show -s\n--pretty=reference\" if it is available, with a fallback to reading\n\"pretty.overrideReference\", so there's a single point of configuration\nfor the reference format.\n\n Documentation/config/pretty.txt | 6 ++++--\n pretty.c | 20 +++++++++++++++-----\n t/t4205-log-pretty-formats.sh | 8 ++++++++\n 3 files changed, 27 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config/pretty.txt b/Documentation/config/pretty.txt\nindex 063c6b63d9..d9dac7b3ee 100644\n--- a/Documentation/config/pretty.txt\n+++ b/Documentation/config/pretty.txt\n@@ -5,5 +5,7 @@ pretty.::\n \trunning `git config pretty.changelog \"format:* %H %s\"`\n \twould cause the invocation `git log --pretty=changelog`\n \tto be equivalent to running `git log \"--pretty=format:* %H %s\"`.\n-\tNote that an alias with the same name as a built-in format\n-\twill be silently ignored.\n+\tNote that you can override a built-in format by prefixing its\n+\tname with `override`, e.g. `pretty.overrideReference` to override\n+\tthe built-in reference format. Doing so can cause interoperability\n+\tissues with tools that expect a built-in format to be immutable.\ndiff --git a/pretty.c b/pretty.c\nindex 2a3d46bf42..a8f8ade470 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -46,18 +46,28 @@ static int git_pretty_formats_config(const char *var, const char *value, void *c\n {\n \tstruct cmt_fmt_map *commit_format = NULL;\n \tconst char *name;\n+\tconst char *suffix;\n \tconst char *fmt;\n \tint i;\n \n \tif (!skip_prefix(var, \"pretty.\", &name))\n \t\treturn 0;\n-\n-\tfor (i = 0; i < builtin_formats_len; i++) {\n-\t\tif (!strcmp(commit_formats[i].name, name))\n-\t\t\treturn 0;\n+\tif (skip_prefix(name, \"override\", &suffix) && *suffix) {\n+\t\tname = suffix;\n+\t\t/* also search the built-in formats */\n+\t\ti = 0;\n+\t} else {\n+\t\tfor (i = 0; i < builtin_formats_len; i++) {\n+\t\t\tif (!strcmp(commit_formats[i].name, name))\n+\t\t\t\treturn 0;\n+\t\t}\n+\t\t/*\n+\t\t * Here, i == builtin_formats_len, so we only search the\n+\t\t * user-defined formats\n+\t\t */\n \t}\n \n-\tfor (i = builtin_formats_len; i < commit_formats_len; i++) {\n+\tfor (; i < commit_formats_len; i++) {\n \t\tif (!strcmp(commit_formats[i].name, name)) {\n \t\t\tcommit_format = &commit_formats[i];\n \t\t\tbreak;\ndiff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh\nindex 204c149d5a..55c37be392 100755\n--- a/t/t4205-log-pretty-formats.sh\n+++ b/t/t4205-log-pretty-formats.sh\n@@ -52,6 +52,14 @@ test_expect_success 'alias masking builtin format' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'overriding builtin format' '\n+\tgit log --pretty=%H >expected &&\n+\tgit config pretty.overrideOneline \"%H\" &&\n+\tgit log --pretty=oneline >actual &&\n+\tgit config --unset pretty.overrideOneline &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'alias user-defined format' '\n \tgit log --pretty=\"format:%h\" >expected &&\n \tgit config pretty.test-alias \"format:%h\" &&\n-- \n2.26.0.277.gb8618d28a9\n\n"},{"id":"405084","messageId":"20200905195218.GA892287@generichostname","threadId":"54194","inReplyTo":"20200905192406.74411-1-dev+git@drbeat.li","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-09-05T19:52:18Z","receivedAt":"2020-09-05T19:52:24Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Beat,\n\nThanks for doing this. It was on my todo list but I've been quite busy\nrecently.\n\nOn Sat, Sep 05, 2020 at 09:24:06PM +0200, Beat Bolli wrote:\n> In 1f0fc1db8599 (pretty: implement 'reference' format, 2019-11-19), the\n> \"reference\" format was added. As a built-in format, it cannot be\n> overridden, although different projects may have divergent conventions\n> on how to format a commit reference. E.g., Git uses\n> \n> (, ) [1]\n> \n> while Linux uses\n> \n> (\"\") [2]\n> \n> Teach pretty to look at a different set of config variables, all\n> starting with \"override\" (e.g. \"pretty.overrideReference\"), to override\n> the built-in formats. Note that a format called \"override\" by itself is\n> not affected. The prefix was chosen to make it clear to the user that\n> this should not be done without thought, as it may cause issues with\n> other tools that expect the built-in formats to be immutable.\n\nHmm, I'm not sure how I feel about being able to override formats other\nthan \"reference\". Perhaps we could special-case \"reference\" instead of\nproviding users with a possible foot-gun?\n\n> [1] https://github.com/git/git/blob/3a238e539bcdfe3f9eb5010fd218640c1b499f7a/Documentation/SubmittingPatches#L144\n> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v5.9-rc3#n167\n> \n> Signed-off-by: Beat Bolli \n> ---\n> I intend to also submit a patch to gitk that will use \"git show -s\n> --pretty=reference\" if it is available, with a fallback to reading\n> \"pretty.overrideReference\", so there's a single point of configuration\n> for the reference format.\n\nVery good, I'm in favour of this.\n"},{"id":"405105","messageId":"xmqqeene36t7.fsf@gitster.c.googlers.com","threadId":"54194","inReplyTo":"20200905195218.GA892287@generichostname","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-06T21:59:32Z","receivedAt":"2020-09-06T21:59:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Hmm, I'm not sure how I feel about being able to override formats other\n> than \"reference\".\n\nIs the idea to introduce a parallel namespace to pretty.? I\nam not sure why that is a good idea than, say a single variable that\nsays \"to me, pretty. would override even the built-in names\".\n\nI am not sure how I feel about being able to override built-in\nformats in the first place, though.\n\nAfter all, pretty. were introduced so that user-defined ones\ncan be invoked with an equal ease as the built-in ones, but\noverriding common understanding among the users of the tool is a\ndifferent story.\n"},{"id":"405113","messageId":"8bb68268-8e4c-749e-b2e0-21b38b70c8bf@drbeat.li","threadId":"54194","inReplyTo":"xmqqeene36t7.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2020-09-07T05:36:47Z","receivedAt":"2020-09-07T05:36:59Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 06.09.20 23:59, Junio C Hamano wrote:\n> Denton Liu writes:\n> \n>> Hmm, I'm not sure how I feel about being able to override formats other\n>> than \"reference\".\n> \n> Is the idea to introduce a parallel namespace to pretty.? I\n> am not sure why that is a good idea than, say a single variable that\n> says \"to me, pretty. would override even the built-in names\".\n> \n> I am not sure how I feel about being able to override built-in\n> formats in the first place, though.\n> \n> After all, pretty. were introduced so that user-defined ones\n> can be invoked with an equal ease as the built-in ones, but\n> overriding common understanding among the users of the tool is a\n> different story.\n\nI gave a reason for the reference format, at least.\n\nWould you be fine with a patch that just allows to override the\nreference format (for the stated reasons)?\n"},{"id":"405115","messageId":"xmqqtuwa13gt.fsf@gitster.c.googlers.com","threadId":"54194","inReplyTo":"8bb68268-8e4c-749e-b2e0-21b38b70c8bf@drbeat.li","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-07T06:54:42Z","receivedAt":"2020-09-07T06:54:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Beat Bolli writes:\n\n> On 06.09.20 23:59, Junio C Hamano wrote:\n>> Denton Liu writes:\n>> \n>>> Hmm, I'm not sure how I feel about being able to override formats other\n>>> than \"reference\".\n>> \n>> Is the idea to introduce a parallel namespace to pretty.? I\n>> am not sure why that is a good idea than, say a single variable that\n>> says \"to me, pretty. would override even the built-in names\".\n>> \n>> I am not sure how I feel about being able to override built-in\n>> formats in the first place, though.\n>> \n>> After all, pretty. were introduced so that user-defined ones\n>> can be invoked with an equal ease as the built-in ones, but\n>> overriding common understanding among the users of the tool is a\n>> different story.\n>\n> I gave a reason for the reference format, at least.\n>\n> Would you be fine with a patch that just allows to override the\n> reference format (for the stated reasons)?\n\nYour \"reason\" read pretty much the same as \"I want reference to do\nsomething else\", but that leads to \"depending on the configuration,\neven built-in names that are well known to all Git users behave\ndifferently---the users lose common reference (no pun intended)\".\n\nAlso I am not sure how your reason applies specifically to the\nreference format. It would be widely applicable to other formats\nlike 'short' and 'oneline' in that depending on projects' and\npersonal preference, people may want \"something like X but not\nexactly X\" for all the built-in formats.\n\nIOW I still do not see why your \"stated reasons\" justify overriding\nany built-in format, and/or overriding only the reference format. I\ncan understand (but not necessarily agree with) the position \"We'll\nlet any built-in format to be overridden\", but I do not see what\nmakes \"reference\" so special. Even though I think it would confuse\nthe users to make any built-in format overridable and therefore I do\nnot think it is such a good idea, if we were to allow it, I do not\nsee any point in limiting the damage only to the reference format.\n\nFinally, a non-built-in name to express the format specific to a\nproject can already be defined and used pretty easily; e.g. the\n\"pretty.kernel\" format may say %h (\"%s\") and can be used like\n\n $ git show -s --pretty=kernel HEAD\n\nwith the same ease as the 'reference' format.\n\n $ git show -s --pretty=reference HEAD\n\nSo, I dunno.\n\n"},{"id":"405116","messageId":"259819c3-4d6c-e9ed-80c9-15ffa8feb6ad@drbeat.li","threadId":"54194","inReplyTo":"xmqqtuwa13gt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Beat Bolli","fromEmail":"dev+git@drbeat.li","sentAt":"2020-09-07T07:06:05Z","receivedAt":"2020-09-07T07:06:13Z","isPatch":true,"sender":{"key":"dev+git@drbeat.li","avatar":"https://avatars.githubusercontent.com/u/21444?v=4"},"body":"On 07.09.20 08:54, Junio C Hamano wrote:\n> Finally, a non-built-in name to express the format specific to a\n> project can already be defined and used pretty easily; e.g. the\n> \"pretty.kernel\" format may say %h (\"%s\") and can be used like\n> \n> $ git show -s --pretty=kernel HEAD\n> \n> with the same ease as the 'reference' format.\n> \n> $ git show -s --pretty=reference HEAD\n\nThis would work fine if there wasn't also gitk, which has a \"Copy commit\nreference\" function. Without a common name for the format in Git and\ngitk, we lose the single point of truth.\n\nThe one alternative I can see is to make the format name configurable in\ngitk.\n"},{"id":"405158","messageId":"20200908135303.GA2448968@coredump.intra.peff.net","threadId":"54194","inReplyTo":"xmqqtuwa13gt.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-08T13:53:03Z","receivedAt":"2020-09-08T16:40:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 06, 2020 at 11:54:42PM -0700, Junio C Hamano wrote:\n\n> > I gave a reason for the reference format, at least.\n> >\n> > Would you be fine with a patch that just allows to override the\n> > reference format (for the stated reasons)?\n> \n> Your \"reason\" read pretty much the same as \"I want reference to do\n> something else\", but that leads to \"depending on the configuration,\n> even built-in names that are well known to all Git users behave\n> differently---the users lose common reference (no pun intended)\".\n\nI think there is some value in having names and rough semantics that are\nwell-known to all Git users (and scripts), but whose exact output is not\nset in stone. So a name like \"reference\" becomes a rendezvous point\nbetween a script like gitk and the user. It is a shorthand for\n\"reference a commit in a human-readable way according to the user or\nproject preferences\". The script wants to read that value, and the user\nwants to specify it.\n\nYou could accomplish something similar by having gitk look up\npretty.userReference, and defaulting to something sensible if it's not\ndefined. For a big script like gitk that's not too much of an\nimposition. But it's awfully convenient to be able to just say\n--format=reference in any script and get the user's preferred format.\n\nThat's where I think your pretty.kernel example falls down; both the\nrepo and the script have to agree that the name \"kernel\" exists.\n\n> Also I am not sure how your reason applies specifically to the\n> reference format. It would be widely applicable to other formats\n> like 'short' and 'oneline' in that depending on projects' and\n> personal preference, people may want \"something like X but not\n> exactly X\" for all the built-in formats.\n\nThe things that may make \"reference\" different are:\n\n - it's new-ish, so there's less chance of historical dependencies on it\n (this is a bit hand-wavy, of course; it has been in released\n versions. On the other hand, people may well have been using\n pretty.reference for this already, and we made it stop working when\n we added \"reference\").\n\n - from the start, the point was for it to be a human-readable format\n (it's not even unambiguously parseable anyway).\n\nSo of any of the formats, it seems like the most likely candidate for\nsuch a feature (setting \"pretty.raw\" would be a pretty big foot-gun, for\ninstance). I don't like the inconsistency it introduces between formats,\nthough.\n\nHere's a slightly different proposal. I'm not sure if I like it or not,\nbut just thinking out loud for a moment. The issue is that we're worried\nthe consumer of the output may be surprised by a user-configured pretty\nformat. Can we give them a way to say \"I don't care about the exact\noutput; pick what the user configured for this name, or some sane\ndefault\". I.e., something like:\n\n git log --format=loose:reference\n\n? That would let pretty.reference override the built-in name, but the\nbehavior of plain \"--format=reference\" would continue to ignore it. It's\na little more annoying for a script to specify, but not nearly as\nannoying as:\n\n format=$(git config pretty.customReference || echo \"%h (%s, %d)\")\n git log --format=\"%format\"\n\n-Peff\n"},{"id":"405168","messageId":"xmqqzh60xhms.fsf@gitster.c.googlers.com","threadId":"54194","inReplyTo":"20200908135303.GA2448968@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-08T18:12:11Z","receivedAt":"2020-09-08T18:12:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You could accomplish something similar by having gitk look up\n> pretty.userReference, and defaulting to something sensible if it's not\n> defined. For a big script like gitk that's not too much of an\n> imposition. But it's awfully convenient to be able to just say\n> --format=reference in any script and get the user's preferred format.\n\nOr --format=userReference in any script, and then allow it to fall\nback to pretty.reference that is otherwise ignored? Ah, that indeed\nis what you suggested with --format=loose:reference already.\n\n> So of any of the formats, it seems like the most likely candidate for\n> such a feature (setting \"pretty.raw\" would be a pretty big foot-gun, for\n> instance). I don't like the inconsistency it introduces between formats,\n> though.\n\nYes, the inconsistency was what primarily disturbed me.\n\n> Here's a slightly different proposal. I'm not sure if I like it or not,\n> but just thinking out loud for a moment. The issue is that we're worried\n> the consumer of the output may be surprised by a user-configured pretty\n> format. Can we give them a way to say \"I don't care about the exact\n> output; pick what the user configured for this name, or some sane\n> default\". I.e., something like:\n>\n> git log --format=loose:reference\n\nYeah, that, or with s/loose/user/ or something.\n"},{"id":"405232","messageId":"20200909090842.GA2496536@coredump.intra.peff.net","threadId":"54194","inReplyTo":"xmqqzh60xhms.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-09-09T09:08:42Z","receivedAt":"2020-09-09T09:08:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 08, 2020 at 11:12:11AM -0700, Junio C Hamano wrote:\n\n> > Here's a slightly different proposal. I'm not sure if I like it or not,\n> > but just thinking out loud for a moment. The issue is that we're worried\n> > the consumer of the output may be surprised by a user-configured pretty\n> > format. Can we give them a way to say \"I don't care about the exact\n> > output; pick what the user configured for this name, or some sane\n> > default\". I.e., something like:\n> >\n> > git log --format=loose:reference\n> \n> Yeah, that, or with s/loose/user/ or something.\n\nHeh, I actually called it \"user:\" initially but wasn't sure if that was\nsufficiently descriptive, so I groped around for another word. But if\nboth of us thought of \"user\", maybe it's better.\n\nAt any rate, this was mostly just thinking out loud, and isn't something\nI'm planning to follow up on with a patch. But maybe it inspires\nsomebody to run with it.\n\n-Peff\n"},{"id":"405268","messageId":"xmqqtuw6vm8w.fsf@gitster.c.googlers.com","threadId":"54194","inReplyTo":"20200909090842.GA2496536@coredump.intra.peff.net","subject":"Re: [PATCH] pretty: allow to override the built-in formats","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-09T18:27:43Z","receivedAt":"2020-09-09T18:28:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Sep 08, 2020 at 11:12:11AM -0700, Junio C Hamano wrote:\n>\n>> > Here's a slightly different proposal. I'm not sure if I like it or not,\n>> > but just thinking out loud for a moment. The issue is that we're worried\n>> > the consumer of the output may be surprised by a user-configured pretty\n>> > format. Can we give them a way to say \"I don't care about the exact\n>> > output; pick what the user configured for this name, or some sane\n>> > default\". I.e., something like:\n>> >\n>> > git log --format=loose:reference\n>> \n>> Yeah, that, or with s/loose/user/ or something.\n>\n> Heh, I actually called it \"user:\" initially but wasn't sure if that was\n> sufficiently descriptive, so I groped around for another word. But if\n> both of us thought of \"user\", maybe it's better.\n>\n> At any rate, this was mostly just thinking out loud, and isn't something\n> I'm planning to follow up on with a patch. But maybe it inspires\n> somebody to run with it.\n\nOf course, we could go the other way and follow the same approach as\nthe \"--literal-pathspecs\" feature (and what bash does with the alias\nand uses \"command\" keyword to work around the confusion it causes).\n\nIOW, we could force those scripts that want to be strict to pay the\nprice and be explicit (e.g. \"--format=builtin:\") and allow\nothers that want to be affected by end user customization can keep\nsaying \"--format=\". It unfortunately breaks our long standing\nstance against backward compatibility breaking changes, so I would\nsay it is not likely to happen. \"--format=loose:reference\" does not\nshare the problem, and it is much safer.\n\nIn any case, I do not think \"pretty.override\" configuration\nvariable, which warns with the 'override' and makes those who tweak\nbuiltin format think twice, is a good idea. Those who add the\ncustom configuration are not in the position to decide if a\nparticular use of --format= in a script (like gitk[*1*]) should\nor should not be affected by customization. It is up to the script\nwriters [*2*].\n\n\n[Footnote]\n\n*1* We've been using gitk as an example but it is not the best one,\n since it was made crystal clear that gitk will not be accepting a\n single liner patch that uses --pretty=reference anyway, it is a\n moot point.\n\n cf. https://lore.kernel.org/git/20191211215826.GA31614@blackberry/\n\n*2* In general, a script wants strict/builtin output if it captures\n and parses, and customizable output if it just lets the git\n command it calls directly talk to the end user.\n"}]}