{"thread":{"id":"45949","subject":"[PATCH] builtin/log: honor log.decorate","startedAt":"2017-05-12T21:34:40Z","lastAt":"2017-05-15T16:26:45Z","messageCount":15,"participants":["brian m. carlson","Alex Henrie","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"319569","messageId":"20170512213407.46251-1-sandals@crustytoothpaste.net","threadId":"45949","inReplyTo":null,"subject":"[PATCH] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-12T21:34:07Z","receivedAt":"2017-05-12T21:34:40Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The recent change that introduced autodecorating of refs accidentally\nbroke the ability of users to set log.decorate = false to override it.\nWhen the git_log_config was traversed a second time with an option other\nthan log.decorate, the decoration style would be set to the automatic\nstyle, even if the user had already overridden it. Only set the option\nto its default value if we haven't set it already.\n\nSigned-off-by: brian m. carlson \n---\n builtin/log.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1e..304923836 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -34,7 +34,7 @@ static int default_abbrev_commit;\n static int default_show_root = 1;\n static int default_follow;\n static int default_show_signature;\n-static int decoration_style;\n+static int decoration_style = -1;\n static int decoration_given;\n static int use_mailmap_config;\n static const char *fmt_patch_subject_prefix = \"PATCH\";\n@@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tif (decoration_style < 0)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n-\t} else {\n+\t} else if (decoration_style == -1) {\n \t\tdecoration_style = auto_decoration_style();\n \t}\n \tif (!strcmp(var, \"log.showroot\")) {\n"},{"id":"319570","messageId":"CAMMLpeTYvK+yow8R83Yf-kOFOZngFEfYsoQeXuTOL0tZOE3XTA@mail.gmail.com","threadId":"45949","inReplyTo":"20170512213407.46251-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH] builtin/log: honor log.decorate","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-05-12T22:03:04Z","receivedAt":"2017-05-12T22:03:31Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-05-12 15:34 GMT-06:00 brian m. carlson :\n> The recent change that introduced autodecorating of refs accidentally\n> broke the ability of users to set log.decorate = false to override it.\n> When the git_log_config was traversed a second time with an option other\n> than log.decorate, the decoration style would be set to the automatic\n> style, even if the user had already overridden it. Only set the option\n> to its default value if we haven't set it already.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> builtin/log.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1e..304923836 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -34,7 +34,7 @@ static int default_abbrev_commit;\n> static int default_show_root = 1;\n> static int default_follow;\n> static int default_show_signature;\n> -static int decoration_style;\n> +static int decoration_style = -1;\n> static int decoration_given;\n> static int use_mailmap_config;\n> static const char *fmt_patch_subject_prefix = \"PATCH\";\n> @@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> if (decoration_style < 0)\n> decoration_style = 0; /* maybe warn? */\n> return 0;\n> - } else {\n> + } else if (decoration_style == -1) {\n> decoration_style = auto_decoration_style();\n> }\n> if (!strcmp(var, \"log.showroot\")) {\n\nSorry for the mistake. On second thought, I think we should set\ndecoration_style = auto_decoration_style() in init_log_defaults.\n\n-Alex\n"},{"id":"319571","messageId":"20170512220704.txmdk45s66aqhexi@genre.crustytoothpaste.net","threadId":"45949","inReplyTo":"CAMMLpeTYvK+yow8R83Yf-kOFOZngFEfYsoQeXuTOL0tZOE3XTA@mail.gmail.com","subject":"Re: [PATCH] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-12T22:07:04Z","receivedAt":"2017-05-12T22:07:13Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, May 12, 2017 at 04:03:04PM -0600, Alex Henrie wrote:\n> > diff --git a/builtin/log.c b/builtin/log.c\n> > index b3b10cc1e..304923836 100644\n> > --- a/builtin/log.c\n> > +++ b/builtin/log.c\n> > @@ -34,7 +34,7 @@ static int default_abbrev_commit;\n> > static int default_show_root = 1;\n> > static int default_follow;\n> > static int default_show_signature;\n> > -static int decoration_style;\n> > +static int decoration_style = -1;\n> > static int decoration_given;\n> > static int use_mailmap_config;\n> > static const char *fmt_patch_subject_prefix = \"PATCH\";\n> > @@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> > if (decoration_style < 0)\n> > decoration_style = 0; /* maybe warn? */\n> > return 0;\n> > - } else {\n> > + } else if (decoration_style == -1) {\n> > decoration_style = auto_decoration_style();\n> > }\n> > if (!strcmp(var, \"log.showroot\")) {\n> \n> Sorry for the mistake. On second thought, I think we should set\n> decoration_style = auto_decoration_style() in init_log_defaults.\n\nI can do that. Patch forthcoming shortly.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"319572","messageId":"20170512221221.406645-1-sandals@crustytoothpaste.net","threadId":"45949","inReplyTo":"20170512220704.txmdk45s66aqhexi@genre.crustytoothpaste.net","subject":"[PATCH v2] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-12T22:12:21Z","receivedAt":"2017-05-12T22:12:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The recent change that introduced autodecorating of refs accidentally\nbroke the ability of users to set log.decorate = false to override it.\nWhen the git_log_config was traversed a second time with an option other\nthan log.decorate, the decoration style would be set to the automatic\nstyle, even if the user had already overridden it. Instead of setting\nthe option in config parsing, set it in init_log_defaults instead.\n\nSigned-off-by: brian m. carlson \n---\n builtin/log.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1e..ec3258368 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -110,6 +110,8 @@ static void init_log_defaults(void)\n {\n \tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n+\n+\tdecoration_style = auto_decoration_style();\n }\n \n static void cmd_log_init_defaults(struct rev_info *rev)\n@@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tif (decoration_style < 0)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n-\t} else {\n-\t\tdecoration_style = auto_decoration_style();\n \t}\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\n"},{"id":"319573","messageId":"CAMMLpeSOesfJPtoj4vGK1iQB+YDRge8TYy2fnL94brheRU3dYA@mail.gmail.com","threadId":"45949","inReplyTo":"20170512221221.406645-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-05-12T22:22:09Z","receivedAt":"2017-05-12T22:22:36Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-05-12 16:12 GMT-06:00 brian m. carlson :\n> The recent change that introduced autodecorating of refs accidentally\n> broke the ability of users to set log.decorate = false to override it.\n> When the git_log_config was traversed a second time with an option other\n> than log.decorate, the decoration style would be set to the automatic\n> style, even if the user had already overridden it. Instead of setting\n> the option in config parsing, set it in init_log_defaults instead.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> builtin/log.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1e..ec3258368 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -110,6 +110,8 @@ static void init_log_defaults(void)\n> {\n> init_grep_defaults();\n> init_diff_ui_defaults();\n> +\n> + decoration_style = auto_decoration_style();\n> }\n>\n> static void cmd_log_init_defaults(struct rev_info *rev)\n> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> if (decoration_style < 0)\n> decoration_style = 0; /* maybe warn? */\n> return 0;\n> - } else {\n> - decoration_style = auto_decoration_style();\n> }\n> if (!strcmp(var, \"log.showroot\")) {\n> default_show_root = git_config_bool(var, value);\n\nSigned-off-by: Alex Henrie \n"},{"id":"319582","messageId":"20170512233214.GE27400@aiede.svl.corp.google.com","threadId":"45949","inReplyTo":"20170512221221.406645-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-12T23:32:14Z","receivedAt":"2017-05-12T23:32:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"brian m. carlson wrote:\n\n> The recent change that introduced autodecorating of refs accidentally\n> broke the ability of users to set log.decorate = false to override it.\n\nYikes. It sounds to me like we need a test to ensure we don't regress\nit again later.\n\n> When the git_log_config was traversed a second time with an option other\n> than log.decorate, the decoration style would be set to the automatic\n> style, even if the user had already overridden it. Instead of setting\n> the option in config parsing, set it in init_log_defaults instead.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> builtin/log.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1e..ec3258368 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -110,6 +110,8 @@ static void init_log_defaults(void)\n> {\n> \tinit_grep_defaults();\n> \tinit_diff_ui_defaults();\n> +\n> +\tdecoration_style = auto_decoration_style();\n> }\n> \n> static void cmd_log_init_defaults(struct rev_info *rev)\n> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> \t\tif (decoration_style < 0)\n> \t\t\tdecoration_style = 0; /* maybe warn? */\n> \t\treturn 0;\n> -\t} else {\n> -\t\tdecoration_style = auto_decoration_style();\n> \t}\n\nMakese sense.\n\nThanks,\nJonathan\n"},{"id":"319583","messageId":"20170512233753.rz2g7quews4ny5iq@genre.crustytoothpaste.net","threadId":"45949","inReplyTo":"20170512233214.GE27400@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-12T23:37:53Z","receivedAt":"2017-05-12T23:38:07Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:\n> brian m. carlson wrote:\n> \n> > The recent change that introduced autodecorating of refs accidentally\n> > broke the ability of users to set log.decorate = false to override it.\n> \n> Yikes. It sounds to me like we need a test to ensure we don't regress\n> it again later.\n\nI can add one, but it's going to be a bit odd. The issue is that as far\nas I can tell, the option is honored only if it's the last one read, so\nit necessarily has to be in the per-repository configuration.\n\nI'm not sure it makes that much sense to add a test for this case. Do\nwe generally want to write such tests for all config options? I don't\nsuppose it's that common a mistake to make.\n\nDoes anyone else have views on whether this is good thing to test for?\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"319585","messageId":"20170512234814.GG27400@aiede.svl.corp.google.com","threadId":"45949","inReplyTo":"20170512233753.rz2g7quews4ny5iq@genre.crustytoothpaste.net","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-12T23:48:14Z","receivedAt":"2017-05-12T23:48:23Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n\n> Does anyone else have views on whether this is good thing to test for?\n\nI know you don't mean to be rude, but this comes across as a bit of\na dismissive question.\n\n> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:\n>> brian m. carlson wrote:\n\n>>> The recent change that introduced autodecorating of refs accidentally\n>>> broke the ability of users to set log.decorate = false to override it.\n>>\n>> Yikes. It sounds to me like we need a test to ensure we don't regress\n>> it again later.\n>\n> I can add one, but it's going to be a bit odd. The issue is that as far\n> as I can tell, the option is honored only if it's the last one read, so\n> it necessarily has to be in the per-repository configuration.\n>\n> I'm not sure it makes that much sense to add a test for this case. Do\n> we generally want to write such tests for all config options? I don't\n> suppose it's that common a mistake to make.\n\nIn my humble opinion, the bug being subtle makes it especially useful\nto have a test that describes that bug. That way, if someone is\nrefactoring this code later then they know what to watch out for not\nreintroducing.\n\nI'm happy to hear other opinions, especially if they come with data or\na principle attached that I can use when writing future patches of my\nown.\n\nThanks,\nJonathan\n"},{"id":"319587","messageId":"CAMMLpeRkVsvp_fzt+bHSQ_F9gsMhYqej_oyqrjhAXsY1xs7=Qw@mail.gmail.com","threadId":"45949","inReplyTo":"20170512234814.GG27400@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-05-13T01:23:04Z","receivedAt":"2017-05-13T01:23:33Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-05-12 17:48 GMT-06:00 Jonathan Nieder :\n> Hi,\n>\n> brian m. carlson wrote:\n>\n>> Does anyone else have views on whether this is good thing to test for?\n>\n> I know you don't mean to be rude, but this comes across as a bit of\n> a dismissive question.\n\nThe question sounded neutral to me.\n\n>> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:\n>>> brian m. carlson wrote:\n>\n>>>> The recent change that introduced autodecorating of refs accidentally\n>>>> broke the ability of users to set log.decorate = false to override it.\n>>>\n>>> Yikes. It sounds to me like we need a test to ensure we don't regress\n>>> it again later.\n>>\n>> I can add one, but it's going to be a bit odd. The issue is that as far\n>> as I can tell, the option is honored only if it's the last one read, so\n>> it necessarily has to be in the per-repository configuration.\n>>\n>> I'm not sure it makes that much sense to add a test for this case. Do\n>> we generally want to write such tests for all config options? I don't\n>> suppose it's that common a mistake to make.\n>\n> In my humble opinion, the bug being subtle makes it especially useful\n> to have a test that describes that bug. That way, if someone is\n> refactoring this code later then they know what to watch out for not\n> reintroducing.\n>\n> I'm happy to hear other opinions, especially if they come with data or\n> a principle attached that I can use when writing future patches of my\n> own.\n\nWhen I saw Brian's email today, my first thought was \"What was I\nthinking?\" My mistake was pretty obvious. Then I remembered that when\nI wrote the original patch, I wasn't sure where to set the default\nvalue, because there were no clear examples in this file. Now that\nwe've established a clear precedent for setting the log.decorate\ndefault (and other defaults like it) in init_log_defaults, I don't\nexpect any more problems with log.decorate. And since it's not\npractical to add tests for similar bugs for every command and\nconfiguration option in Git, we'll just have to be a little more\nvigilant about code review.\n\nAgain, I apologize for the trouble.\n\n-Alex\n"},{"id":"319591","messageId":"20170513024141.52nazwvnqsvj2af7@genre.crustytoothpaste.net","threadId":"45949","inReplyTo":"20170512234814.GG27400@aiede.svl.corp.google.com","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-13T02:41:41Z","receivedAt":"2017-05-13T02:41:54Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, May 12, 2017 at 04:48:14PM -0700, Jonathan Nieder wrote:\n> Hi,\n> \n> brian m. carlson wrote:\n> \n> > Does anyone else have views on whether this is good thing to test for?\n> \n> I know you don't mean to be rude, but this comes across as a bit of\n> a dismissive question.\n\nSorry, that wasn't my intention, but I see how it could have come off\nthat way. I was trying to solicit other opinions, but did a bad job and\nimplied yours wasn't valuable. I apologize.\n\n> > On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:\n> \n> In my humble opinion, the bug being subtle makes it especially useful\n> to have a test that describes that bug. That way, if someone is\n> refactoring this code later then they know what to watch out for not\n> reintroducing.\n\nOkay. I'll reroll with a patch to the tests. I agree that this is\nrather subtle.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"319592","messageId":"20170513024950.qlk5vrdwwsquceyn@genre.crustytoothpaste.net","threadId":"45949","inReplyTo":"CAMMLpeRkVsvp_fzt+bHSQ_F9gsMhYqej_oyqrjhAXsY1xs7=Qw@mail.gmail.com","subject":"Re: [PATCH v2] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-13T02:49:50Z","receivedAt":"2017-05-13T02:49:59Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, May 12, 2017 at 07:23:04PM -0600, Alex Henrie wrote:\n> When I saw Brian's email today, my first thought was \"What was I\n> thinking?\" My mistake was pretty obvious. Then I remembered that when\n> I wrote the original patch, I wasn't sure where to set the default\n> value, because there were no clear examples in this file. Now that\n> we've established a clear precedent for setting the log.decorate\n> default (and other defaults like it) in init_log_defaults, I don't\n> expect any more problems with log.decorate. And since it's not\n> practical to add tests for similar bugs for every command and\n> configuration option in Git, we'll just have to be a little more\n> vigilant about code review.\n> \n> Again, I apologize for the trouble.\n\nEveryone sends a patch that breaks sometimes (I've broken kernel.org's\ninfrastructure). We try hard to avoid it, but we don't always succeed.\nI appreciate your review on my patch and suggestion on how to improve\nit, and I hope this won't put you off from further contributions.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"319729","messageId":"20170514180058.842315-1-sandals@crustytoothpaste.net","threadId":"45949","inReplyTo":"20170512221221.406645-1-sandals@crustytoothpaste.net","subject":"[PATCH v3] builtin/log: honor log.decorate","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-05-14T18:00:58Z","receivedAt":"2017-05-14T18:01:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"The recent change that introduced autodecorating of refs accidentally\nbroke the ability of users to set log.decorate = false to override it.\nWhen the git_log_config was traversed a second time with an option other\nthan log.decorate, the decoration style would be set to the automatic\nstyle, even if the user had already overridden it. Instead of setting\nthe option in config parsing, set it in init_log_defaults instead.\n\nAdd a test for this case. The actual additional config option doesn't\nmatter, but it needs to be something not already set in the\nconfiguration file.\n\nSigned-off-by: brian m. carlson \n---\nChanges from v2:\n* Add a test. I tested that the config parsing both works with\n additional options and also can be overridden from the command line.\n\n builtin/log.c | 4 ++--\n t/t4202-log.sh | 12 ++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex b3b10cc1e..ec3258368 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -110,6 +110,8 @@ static void init_log_defaults(void)\n {\n \tinit_grep_defaults();\n \tinit_diff_ui_defaults();\n+\n+\tdecoration_style = auto_decoration_style();\n }\n \n static void cmd_log_init_defaults(struct rev_info *rev)\n@@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n \t\tif (decoration_style < 0)\n \t\t\tdecoration_style = 0; /* maybe warn? */\n \t\treturn 0;\n-\t} else {\n-\t\tdecoration_style = auto_decoration_style();\n \t}\n \tif (!strcmp(var, \"log.showroot\")) {\n \t\tdefault_show_root = git_config_bool(var, value);\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex f57799071..1c7d6729c 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '\n \n '\n \n+test_expect_success 'log.decorate config parsing' '\n+\tgit log --oneline --decorate=full >expect.full &&\n+\tgit log --oneline --decorate=short >expect.short &&\n+\n+\ttest_config log.decorate full &&\n+\ttest_config log.mailmap true &&\n+\tgit log --oneline >actual &&\n+\ttest_cmp expect.full actual &&\n+\tgit log --oneline --decorate=short >actual &&\n+\ttest_cmp expect.short actual\n+'\n+\n test_expect_success TTY 'log output on a TTY' '\n \tgit log --oneline --decorate >expect.short &&\n \n"},{"id":"319743","messageId":"xmqqy3tyx2x5.fsf@gitster.mtv.corp.google.com","threadId":"45949","inReplyTo":"20170514180058.842315-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v3] builtin/log: honor log.decorate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-15T02:35:34Z","receivedAt":"2017-05-15T02:35:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> The recent change that introduced autodecorating of refs accidentally\n> broke the ability of users to set log.decorate = false to override it.\n> When the git_log_config was traversed a second time with an option other\n> than log.decorate, the decoration style would be set to the automatic\n> style, even if the user had already overridden it. Instead of setting\n> the option in config parsing, set it in init_log_defaults instead.\n>\n> Add a test for this case. The actual additional config option doesn't\n> matter, but it needs to be something not already set in the\n> configuration file.\n>\n> Signed-off-by: brian m. carlson \n> ---\n> Changes from v2:\n> * Add a test. I tested that the config parsing both works with\n> additional options and also can be overridden from the command line.\n\nThanks, all.\n\nWill queue with Acked-by by Alex and Reviewed-by by Jonathan.\n\n\n> builtin/log.c | 4 ++--\n> t/t4202-log.sh | 12 ++++++++++++\n> 2 files changed, 14 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index b3b10cc1e..ec3258368 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -110,6 +110,8 @@ static void init_log_defaults(void)\n> {\n> \tinit_grep_defaults();\n> \tinit_diff_ui_defaults();\n> +\n> +\tdecoration_style = auto_decoration_style();\n> }\n> \n> static void cmd_log_init_defaults(struct rev_info *rev)\n> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)\n> \t\tif (decoration_style < 0)\n> \t\t\tdecoration_style = 0; /* maybe warn? */\n> \t\treturn 0;\n> -\t} else {\n> -\t\tdecoration_style = auto_decoration_style();\n> \t}\n> \tif (!strcmp(var, \"log.showroot\")) {\n> \t\tdefault_show_root = git_config_bool(var, value);\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index f57799071..1c7d6729c 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '\n> \n> '\n> \n> +test_expect_success 'log.decorate config parsing' '\n> +\tgit log --oneline --decorate=full >expect.full &&\n> +\tgit log --oneline --decorate=short >expect.short &&\n> +\n> +\ttest_config log.decorate full &&\n> +\ttest_config log.mailmap true &&\n> +\tgit log --oneline >actual &&\n> +\ttest_cmp expect.full actual &&\n> +\tgit log --oneline --decorate=short >actual &&\n> +\ttest_cmp expect.short actual\n> +'\n> +\n> test_expect_success TTY 'log output on a TTY' '\n> \tgit log --oneline --decorate >expect.short &&\n> \n"},{"id":"319750","messageId":"CAMMLpeSpgfOFLV_WE6yQzp2aKta9DEt+Z5gDGDhwrEDzfqtwBg@mail.gmail.com","threadId":"45949","inReplyTo":"xmqqy3tyx2x5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] builtin/log: honor log.decorate","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2017-05-15T03:48:33Z","receivedAt":"2017-05-15T03:49:15Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"2017-05-14 20:35 GMT-06:00 Junio C Hamano :\n> \"brian m. carlson\" writes:\n>\n>> The recent change that introduced autodecorating of refs accidentally\n>> broke the ability of users to set log.decorate = false to override it.\n>> When the git_log_config was traversed a second time with an option other\n>> than log.decorate, the decoration style would be set to the automatic\n>> style, even if the user had already overridden it. Instead of setting\n>> the option in config parsing, set it in init_log_defaults instead.\n>>\n>> Add a test for this case. The actual additional config option doesn't\n>> matter, but it needs to be something not already set in the\n>> configuration file.\n>>\n>> Signed-off-by: brian m. carlson \n>> ---\n>> Changes from v2:\n>> * Add a test. I tested that the config parsing both works with\n>> additional options and also can be overridden from the command line.\n>\n> Thanks, all.\n>\n> Will queue with Acked-by by Alex and Reviewed-by by Jonathan.\n\nThat sounds great, thank you.\n\n-Alex\n"},{"id":"319796","messageId":"20170515162635.GI27400@aiede.svl.corp.google.com","threadId":"45949","inReplyTo":"xmqqy3tyx2x5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3] builtin/log: honor log.decorate","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-05-15T16:26:35Z","receivedAt":"2017-05-15T16:26:45Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> \"brian m. carlson\" writes:\n\n>> The recent change that introduced autodecorating of refs accidentally\n>> broke the ability of users to set log.decorate = false to override it.\n>> When the git_log_config was traversed a second time with an option other\n>> than log.decorate, the decoration style would be set to the automatic\n>> style, even if the user had already overridden it. Instead of setting\n>> the option in config parsing, set it in init_log_defaults instead.\n>>\n>> Add a test for this case. The actual additional config option doesn't\n>> matter, but it needs to be something not already set in the\n>> configuration file.\n>>\n>> Signed-off-by: brian m. carlson \n>> ---\n>> Changes from v2:\n>> * Add a test. I tested that the config parsing both works with\n>> additional options and also can be overridden from the command line.\n>\n> Thanks, all.\n>\n> Will queue with Acked-by by Alex and Reviewed-by by Jonathan.\n\nFor completeness: yeah, this version is Reviewed-by: me. Thanks, all.\n"}]}