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

[brian m. carlson]

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


signature.asc
Description: PGP signature

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

[brian m. carlson]

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


signature.asc
Description: PGP signature

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

[Alex Henrie]

2017-05-12 17:48 GMT-06:00 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.

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

[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

[brian m. carlson]

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


signature.asc
Description: PGP signature

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 
> ---
>  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

[Alex Henrie]

2017-05-12 16:12 GMT-06:00 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 
> ---
>  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 

[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 
---
 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);