[PATCH] builtin/log: honor log.decorate

[PATCH] builtin/log: honor log.decorate

[brian m. carlson]

The recent change that introduced autodecorating of refs accidentally
broke the ability of users to set log.decorate = false to override it.
When the git_log_config was traversed a second time with an option other
than log.decorate, the decoration style would be set to the automatic
style, even if the user had already overridden it.  Only set the option
to its default value if we haven't set it already.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1e..304923836 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -34,7 +34,7 @@ static int default_abbrev_commit;
 static int default_show_root = 1;
 static int default_follow;
 static int default_show_signature;
-static int decoration_style;
+static int decoration_style = -1;
 static int decoration_given;
 static int use_mailmap_config;
 static const char *fmt_patch_subject_prefix = "PATCH";
@@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		if (decoration_style < 0)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
-	} else {
+	} else if (decoration_style == -1) {
 		decoration_style = auto_decoration_style();
 	}
 	if (!strcmp(var, "log.showroot")) {

Re: [PATCH] builtin/log: honor log.decorate

[Alex Henrie]

2017-05-12 15:34 GMT-06:00 brian m. carlson <sandals@crustytoothpaste.net>:
> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it.  Only set the option
> to its default value if we haven't set it already.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..304923836 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -34,7 +34,7 @@ static int default_abbrev_commit;
>  static int default_show_root = 1;
>  static int default_follow;
>  static int default_show_signature;
> -static int decoration_style;
> +static int decoration_style = -1;
>  static int decoration_given;
>  static int use_mailmap_config;
>  static const char *fmt_patch_subject_prefix = "PATCH";
> @@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
>                 if (decoration_style < 0)
>                         decoration_style = 0; /* maybe warn? */
>                 return 0;
> -       } else {
> +       } else if (decoration_style == -1) {
>                 decoration_style = auto_decoration_style();
>         }
>         if (!strcmp(var, "log.showroot")) {

Sorry for the mistake. On second thought, I think we should set
decoration_style = auto_decoration_style() in init_log_defaults.

-Alex

Re: [PATCH] builtin/log: honor log.decorate

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1383 bytes --]

On Fri, May 12, 2017 at 04:03:04PM -0600, Alex Henrie wrote:
> > diff --git a/builtin/log.c b/builtin/log.c
> > index b3b10cc1e..304923836 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -34,7 +34,7 @@ static int default_abbrev_commit;
> >  static int default_show_root = 1;
> >  static int default_follow;
> >  static int default_show_signature;
> > -static int decoration_style;
> > +static int decoration_style = -1;
> >  static int decoration_given;
> >  static int use_mailmap_config;
> >  static const char *fmt_patch_subject_prefix = "PATCH";
> > @@ -410,7 +410,7 @@ static int git_log_config(const char *var, const char *value, void *cb)
> >                 if (decoration_style < 0)
> >                         decoration_style = 0; /* maybe warn? */
> >                 return 0;
> > -       } else {
> > +       } else if (decoration_style == -1) {
> >                 decoration_style = auto_decoration_style();
> >         }
> >         if (!strcmp(var, "log.showroot")) {
> 
> Sorry for the mistake. On second thought, I think we should set
> decoration_style = auto_decoration_style() in init_log_defaults.

I can do that.  Patch forthcoming shortly.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

[PATCH v2] builtin/log: honor log.decorate

[brian m. carlson]

The recent change that introduced autodecorating of refs accidentally
broke the ability of users to set log.decorate = false to override it.
When the git_log_config was traversed a second time with an option other
than log.decorate, the decoration style would be set to the automatic
style, even if the user had already overridden it.  Instead of setting
the option in config parsing, set it in init_log_defaults instead.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/log.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1e..ec3258368 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -110,6 +110,8 @@ static void init_log_defaults(void)
 {
 	init_grep_defaults();
 	init_diff_ui_defaults();
+
+	decoration_style = auto_decoration_style();
 }
 
 static void cmd_log_init_defaults(struct rev_info *rev)
@@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		if (decoration_style < 0)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
-	} else {
-		decoration_style = auto_decoration_style();
 	}
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);

Re: [PATCH v2] builtin/log: honor log.decorate

[Alex Henrie]

2017-05-12 16:12 GMT-06:00 brian m. carlson <sandals@crustytoothpaste.net>:
> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it.  Instead of setting
> the option in config parsing, set it in init_log_defaults instead.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..ec3258368 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -110,6 +110,8 @@ static void init_log_defaults(void)
>  {
>         init_grep_defaults();
>         init_diff_ui_defaults();
> +
> +       decoration_style = auto_decoration_style();
>  }
>
>  static void cmd_log_init_defaults(struct rev_info *rev)
> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
>                 if (decoration_style < 0)
>                         decoration_style = 0; /* maybe warn? */
>                 return 0;
> -       } else {
> -               decoration_style = auto_decoration_style();
>         }
>         if (!strcmp(var, "log.showroot")) {
>                 default_show_root = git_config_bool(var, value);

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>

Re: [PATCH v2] builtin/log: honor log.decorate

[Jonathan Nieder]

brian m. carlson wrote:

> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.

Yikes.  It sounds to me like we need a test to ensure we don't regress
it again later.

> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it.  Instead of setting
> the option in config parsing, set it in init_log_defaults instead.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  builtin/log.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..ec3258368 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -110,6 +110,8 @@ static void init_log_defaults(void)
>  {
>  	init_grep_defaults();
>  	init_diff_ui_defaults();
> +
> +	decoration_style = auto_decoration_style();
>  }
>  
>  static void cmd_log_init_defaults(struct rev_info *rev)
> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
>  		if (decoration_style < 0)
>  			decoration_style = 0; /* maybe warn? */
>  		return 0;
> -	} else {
> -		decoration_style = auto_decoration_style();
>  	}

Makese sense.

Thanks,
Jonathan

Re: [PATCH v2] builtin/log: honor log.decorate

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:
> brian m. carlson wrote:
> 
> > The recent change that introduced autodecorating of refs accidentally
> > broke the ability of users to set log.decorate = false to override it.
> 
> Yikes.  It sounds to me like we need a test to ensure we don't regress
> it again later.

I can add one, but it's going to be a bit odd.  The issue is that as far
as I can tell, the option is honored only if it's the last one read, so
it necessarily has to be in the per-repository configuration.

I'm not sure it makes that much sense to add a test for this case.  Do
we generally want to write such tests for all config options?  I don't
suppose it's that common a mistake to make.

Does anyone else have views on whether this is good thing to test for?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2] builtin/log: honor log.decorate

[Jonathan Nieder]

Hi,

brian m. carlson wrote:

> Does anyone else have views on whether this is good thing to test for?

I know you don't mean to be rude, but this comes across as a bit of
a dismissive question.

> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:

>>> The recent change that introduced autodecorating of refs accidentally
>>> broke the ability of users to set log.decorate = false to override it.
>>
>> Yikes.  It sounds to me like we need a test to ensure we don't regress
>> it again later.
>
> I can add one, but it's going to be a bit odd.  The issue is that as far
> as I can tell, the option is honored only if it's the last one read, so
> it necessarily has to be in the per-repository configuration.
>
> I'm not sure it makes that much sense to add a test for this case.  Do
> we generally want to write such tests for all config options?  I don't
> suppose it's that common a mistake to make.

In my humble opinion, the bug being subtle makes it especially useful
to have a test that describes that bug.  That way, if someone is
refactoring this code later then they know what to watch out for not
reintroducing.

I'm happy to hear other opinions, especially if they come with data or
a principle attached that I can use when writing future patches of my
own.

Thanks,
Jonathan

Re: [PATCH v2] builtin/log: honor log.decorate

[Alex Henrie]

2017-05-12 17:48 GMT-06:00 Jonathan Nieder <jrnieder@gmail.com>:
> Hi,
>
> brian m. carlson wrote:
>
>> Does anyone else have views on whether this is good thing to test for?
>
> I know you don't mean to be rude, but this comes across as a bit of
> a dismissive question.

The question sounded neutral to me.

>> On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:
>>> brian m. carlson wrote:
>
>>>> The recent change that introduced autodecorating of refs accidentally
>>>> broke the ability of users to set log.decorate = false to override it.
>>>
>>> Yikes.  It sounds to me like we need a test to ensure we don't regress
>>> it again later.
>>
>> I can add one, but it's going to be a bit odd.  The issue is that as far
>> as I can tell, the option is honored only if it's the last one read, so
>> it necessarily has to be in the per-repository configuration.
>>
>> I'm not sure it makes that much sense to add a test for this case.  Do
>> we generally want to write such tests for all config options?  I don't
>> suppose it's that common a mistake to make.
>
> In my humble opinion, the bug being subtle makes it especially useful
> to have a test that describes that bug.  That way, if someone is
> refactoring this code later then they know what to watch out for not
> reintroducing.
>
> I'm happy to hear other opinions, especially if they come with data or
> a principle attached that I can use when writing future patches of my
> own.

When I saw Brian's email today, my first thought was "What was I
thinking?" My mistake was pretty obvious. Then I remembered that when
I wrote the original patch, I wasn't sure where to set the default
value, because there were no clear examples in this file. Now that
we've established a clear precedent for setting the log.decorate
default (and other defaults like it) in init_log_defaults, I don't
expect any more problems with log.decorate. And since it's not
practical to add tests for similar bugs for every command and
configuration option in Git, we'll just have to be a little more
vigilant about code review.

Again, I apologize for the trouble.

-Alex

Re: [PATCH v2] builtin/log: honor log.decorate

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1047 bytes --]

On Fri, May 12, 2017 at 04:48:14PM -0700, Jonathan Nieder wrote:
> Hi,
> 
> brian m. carlson wrote:
> 
> > Does anyone else have views on whether this is good thing to test for?
> 
> I know you don't mean to be rude, but this comes across as a bit of
> a dismissive question.

Sorry, that wasn't my intention, but I see how it could have come off
that way.  I was trying to solicit other opinions, but did a bad job and
implied yours wasn't valuable.  I apologize.

> > On Fri, May 12, 2017 at 04:32:14PM -0700, Jonathan Nieder wrote:
> 
> In my humble opinion, the bug being subtle makes it especially useful
> to have a test that describes that bug.  That way, if someone is
> refactoring this code later then they know what to watch out for not
> reintroducing.

Okay.  I'll reroll with a patch to the tests.  I agree that this is
rather subtle.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH v2] builtin/log: honor log.decorate

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]

On Fri, May 12, 2017 at 07:23:04PM -0600, Alex Henrie wrote:
> When I saw Brian's email today, my first thought was "What was I
> thinking?" My mistake was pretty obvious. Then I remembered that when
> I wrote the original patch, I wasn't sure where to set the default
> value, because there were no clear examples in this file. Now that
> we've established a clear precedent for setting the log.decorate
> default (and other defaults like it) in init_log_defaults, I don't
> expect any more problems with log.decorate. And since it's not
> practical to add tests for similar bugs for every command and
> configuration option in Git, we'll just have to be a little more
> vigilant about code review.
> 
> Again, I apologize for the trouble.

Everyone sends a patch that breaks sometimes (I've broken kernel.org's
infrastructure).  We try hard to avoid it, but we don't always succeed.
I appreciate your review on my patch and suggestion on how to improve
it, and I hope this won't put you off from further contributions.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

[PATCH v3] builtin/log: honor log.decorate

[brian m. carlson]

The recent change that introduced autodecorating of refs accidentally
broke the ability of users to set log.decorate = false to override it.
When the git_log_config was traversed a second time with an option other
than log.decorate, the decoration style would be set to the automatic
style, even if the user had already overridden it.  Instead of setting
the option in config parsing, set it in init_log_defaults instead.

Add a test for this case.  The actual additional config option doesn't
matter, but it needs to be something not already set in the
configuration file.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
Changes from v2:
* Add a test.  I tested that the config parsing both works with
  additional options and also can be overridden from the command line.

 builtin/log.c  |  4 ++--
 t/t4202-log.sh | 12 ++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1e..ec3258368 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -110,6 +110,8 @@ static void init_log_defaults(void)
 {
 	init_grep_defaults();
 	init_diff_ui_defaults();
+
+	decoration_style = auto_decoration_style();
 }
 
 static void cmd_log_init_defaults(struct rev_info *rev)
@@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		if (decoration_style < 0)
 			decoration_style = 0; /* maybe warn? */
 		return 0;
-	} else {
-		decoration_style = auto_decoration_style();
 	}
 	if (!strcmp(var, "log.showroot")) {
 		default_show_root = git_config_bool(var, value);
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f57799071..1c7d6729c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '
 
 '
 
+test_expect_success 'log.decorate config parsing' '
+	git log --oneline --decorate=full >expect.full &&
+	git log --oneline --decorate=short >expect.short &&
+
+	test_config log.decorate full &&
+	test_config log.mailmap true &&
+	git log --oneline >actual &&
+	test_cmp expect.full actual &&
+	git log --oneline --decorate=short >actual &&
+	test_cmp expect.short actual
+'
+
 test_expect_success TTY 'log output on a TTY' '
 	git log --oneline --decorate >expect.short &&
 

Re: [PATCH v3] builtin/log: honor log.decorate

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it.  Instead of setting
> the option in config parsing, set it in init_log_defaults instead.
>
> Add a test for this case.  The actual additional config option doesn't
> matter, but it needs to be something not already set in the
> configuration file.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> Changes from v2:
> * Add a test.  I tested that the config parsing both works with
>   additional options and also can be overridden from the command line.

Thanks, all.

Will queue with Acked-by by Alex and Reviewed-by by Jonathan.


>  builtin/log.c  |  4 ++--
>  t/t4202-log.sh | 12 ++++++++++++
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..ec3258368 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -110,6 +110,8 @@ static void init_log_defaults(void)
>  {
>  	init_grep_defaults();
>  	init_diff_ui_defaults();
> +
> +	decoration_style = auto_decoration_style();
>  }
>  
>  static void cmd_log_init_defaults(struct rev_info *rev)
> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char *value, void *cb)
>  		if (decoration_style < 0)
>  			decoration_style = 0; /* maybe warn? */
>  		return 0;
> -	} else {
> -		decoration_style = auto_decoration_style();
>  	}
>  	if (!strcmp(var, "log.showroot")) {
>  		default_show_root = git_config_bool(var, value);
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index f57799071..1c7d6729c 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '
>  
>  '
>  
> +test_expect_success 'log.decorate config parsing' '
> +	git log --oneline --decorate=full >expect.full &&
> +	git log --oneline --decorate=short >expect.short &&
> +
> +	test_config log.decorate full &&
> +	test_config log.mailmap true &&
> +	git log --oneline >actual &&
> +	test_cmp expect.full actual &&
> +	git log --oneline --decorate=short >actual &&
> +	test_cmp expect.short actual
> +'
> +
>  test_expect_success TTY 'log output on a TTY' '
>  	git log --oneline --decorate >expect.short &&
>  

Re: [PATCH v3] builtin/log: honor log.decorate

[Alex Henrie]

2017-05-14 20:35 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> The recent change that introduced autodecorating of refs accidentally
>> broke the ability of users to set log.decorate = false to override it.
>> When the git_log_config was traversed a second time with an option other
>> than log.decorate, the decoration style would be set to the automatic
>> style, even if the user had already overridden it.  Instead of setting
>> the option in config parsing, set it in init_log_defaults instead.
>>
>> Add a test for this case.  The actual additional config option doesn't
>> matter, but it needs to be something not already set in the
>> configuration file.
>>
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>> Changes from v2:
>> * Add a test.  I tested that the config parsing both works with
>>   additional options and also can be overridden from the command line.
>
> Thanks, all.
>
> Will queue with Acked-by by Alex and Reviewed-by by Jonathan.

That sounds great, thank you.

-Alex

Re: [PATCH v3] builtin/log: honor log.decorate

[Jonathan Nieder]

Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:

>> The recent change that introduced autodecorating of refs accidentally
>> broke the ability of users to set log.decorate = false to override it.
>> When the git_log_config was traversed a second time with an option other
>> than log.decorate, the decoration style would be set to the automatic
>> style, even if the user had already overridden it.  Instead of setting
>> the option in config parsing, set it in init_log_defaults instead.
>>
>> Add a test for this case.  The actual additional config option doesn't
>> matter, but it needs to be something not already set in the
>> configuration file.
>>
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>> Changes from v2:
>> * Add a test.  I tested that the config parsing both works with
>>   additional options and also can be overridden from the command line.
>
> Thanks, all.
>
> Will queue with Acked-by by Alex and Reviewed-by by Jonathan.

For completeness: yeah, this version is Reviewed-by: me.  Thanks, all.