Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Jeff King  writes:

> I see you ended up with a test that uses test_terminal, which is much
> better (and your patch looks good to me).
>
> But I was concerned that there might be a bug in pager_in_use(), so I
> dug into it a little. I think the code there is correct; it's just
> relaying the environment variable's value. Likewise, on the setting
> side, I think the code is correct. We set the environment variable
> whenever we start the pager in setup_pager().
>
> I think what is confusing is that "git -p" does _not_ mean
> "unconditionally use a pager". It means "start a pager if stdout is a
> terminal, even if this command is not usually paged". So something like
> "git -p log >actual" is correct not to trigger pager_in_use().
>
> I also double-checked the documentation, which covers this case
> accurately. So I think all is well, and there's nothing to fix. You
> might want an option for "even though stdout is not a tty pretend like a
> pager is in use", but that is exactly what GIT_PAGER_IN_USE=1 is for.

Thanks ;-).

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Jeff King]

On Thu, Mar 23, 2017 at 10:52:34AM -0600, Alex Henrie wrote:

> Unfortunately, I think I found a bug. Even when using `git -p`, the
> function pager_in_use() always returns false if the output is not a
> TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||
> (pager_in_use() && pager_use_color)` are redundant.
> 
> If we want to use `git -p log` in a test, we'll have to change the
> behavior of pager_in_use(). Alternatively, we could use
> `GIT_PAGER_IN_USE=1 git log` instead.

I see you ended up with a test that uses test_terminal, which is much
better (and your patch looks good to me).

But I was concerned that there might be a bug in pager_in_use(), so I
dug into it a little. I think the code there is correct; it's just
relaying the environment variable's value. Likewise, on the setting
side, I think the code is correct. We set the environment variable
whenever we start the pager in setup_pager().

I think what is confusing is that "git -p" does _not_ mean
"unconditionally use a pager". It means "start a pager if stdout is a
terminal, even if this command is not usually paged". So something like
"git -p log >actual" is correct not to trigger pager_in_use().

I also double-checked the documentation, which covers this case
accurately. So I think all is well, and there's nothing to fix. You
might want an option for "even though stdout is not a tty pretend like a
pager is in use", but that is exactly what GIT_PAGER_IN_USE=1 is for.

-Peff

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

2017-03-23 12:03 GMT-06:00 Junio C Hamano :
> Alex Henrie  writes:
>
>> Yes, that makes sense. I assume that when you talk about 'next', you
>> mean 'master'?
>
> No, I do mean 'next'.  See "A note from the maintainer" post that
> are sent to the list every once in a while (i.e. after a new release
> is tagged) for the project structure.
>
> https://public-inbox.org/git/xmqqy3vztypi@gitster.mtv.corp.google.com/

That document was very helpful, thanks.

>> If we want to use `git -p log` in a test, we'll have to change the
>> behavior of pager_in_use(). Alternatively, we could use
>> `GIT_PAGER_IN_USE=1 git log` instead.
>
> Testing "git -p" is not the goal; testing that decorate defaults to
> auto during an interactive session is.  We could run tests under pty
> like t7006 does using lib-terminal.sh if we really want to emulate
> an interactive session.
>
> Exporting GIT_PAGER_IN_USE may be sufficient for the purpose of
> convincing the command to be in an interactive session for this
> test, even though it feels a bit too brittle to depend on the
> internal implementation detail.

Okay, I've now written a lib-terminal test for git log. I'll send the
revised patch momentarily.

-Alex

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Alex Henrie  writes:

> Yes, that makes sense. I assume that when you talk about 'next', you
> mean 'master'?

No, I do mean 'next'.  See "A note from the maintainer" post that
are sent to the list every once in a while (i.e. after a new release
is tagged) for the project structure.

https://public-inbox.org/git/xmqqy3vztypi@gitster.mtv.corp.google.com/

> If we want to use `git -p log` in a test, we'll have to change the
> behavior of pager_in_use(). Alternatively, we could use
> `GIT_PAGER_IN_USE=1 git log` instead.

Testing "git -p" is not the goal; testing that decorate defaults to
auto during an interactive session is.  We could run tests under pty
like t7006 does using lib-terminal.sh if we really want to emulate
an interactive session.  

Exporting GIT_PAGER_IN_USE may be sufficient for the purpose of
convincing the command to be in an interactive session for this
test, even though it feels a bit too brittle to depend on the
internal implementation detail.

Thanks.

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

2017-03-23 9:54 GMT-06:00 Junio C Hamano :
> Alex Henrie  writes:
>
>> 2017-03-22 10:54 GMT-06:00 Junio C Hamano :
>>> Alex Henrie  writes:
 No problem. Do I need to submit a second version of the patch with a
 test for `git -p log`?
>>>
>>> You do want to protect this "without an option, we default to
>>> 'auto'" feature from future breakage, no?
>>
>> Yes, but I need to know whether you want a v2 of this patch with all
>> of the changes including the new test, or a second patch that depends
>> on the first patch and only adds the new test.
>
> Sorry, I misunderstood the question.
>
> In general, we prefer to have tests that protects the updated
> behaviour in the same patch that makes code changes that brings in
> the new behaviour, i.e. a single v2 patch with new test would be
> more appropriate in this case.
>
> When people work on a large bugfix, especially one that needs
> multiple steps, we sometimes see a patch that adds new tests that
> describe the desired behaviour as failing tests first, and then
> subsequent patches to the code to update the behaviour flip
> "test_expect_failure" to "test_expect_success" as they fix the
> behaviour.  But for a small change like this one, that approach is
> inappropriate.
>
> When a patch that was reviewed, deemed good enough and has been
> already merged to the 'next' branch later turns out that it needs
> further work (like "we do need some tests"), we do such necessary
> updates as separate follow-up patches, simply because we promise
> that 'next' won't be rewound and are not allowed to replace patches.
> But this one is not yet in 'next', so we can freely take a
> replacement patch.
>
> Hope this message makes it clear enough?

Yes, that makes sense. I assume that when you talk about 'next', you
mean 'master'?

Unfortunately, I think I found a bug. Even when using `git -p`, the
function pager_in_use() always returns false if the output is not a
TTY. So, `isatty(1) || pager_in_use()` and `color_stdout_is_tty ||
(pager_in_use() && pager_use_color)` are redundant.

If we want to use `git -p log` in a test, we'll have to change the
behavior of pager_in_use(). Alternatively, we could use
`GIT_PAGER_IN_USE=1 git log` instead.

-Alex

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Alex Henrie  writes:

> 2017-03-22 10:54 GMT-06:00 Junio C Hamano :
>> Alex Henrie  writes:
>>> No problem. Do I need to submit a second version of the patch with a
>>> test for `git -p log`?
>>
>> You do want to protect this "without an option, we default to
>> 'auto'" feature from future breakage, no?
>
> Yes, but I need to know whether you want a v2 of this patch with all
> of the changes including the new test, or a second patch that depends
> on the first patch and only adds the new test.

Sorry, I misunderstood the question.

In general, we prefer to have tests that protects the updated
behaviour in the same patch that makes code changes that brings in
the new behaviour, i.e. a single v2 patch with new test would be
more appropriate in this case.

When people work on a large bugfix, especially one that needs
multiple steps, we sometimes see a patch that adds new tests that
describe the desired behaviour as failing tests first, and then
subsequent patches to the code to update the behaviour flip
"test_expect_failure" to "test_expect_success" as they fix the
behaviour.  But for a small change like this one, that approach is
inappropriate.

When a patch that was reviewed, deemed good enough and has been
already merged to the 'next' branch later turns out that it needs
further work (like "we do need some tests"), we do such necessary
updates as separate follow-up patches, simply because we promise
that 'next' won't be rewound and are not allowed to replace patches.
But this one is not yet in 'next', so we can freely take a
replacement patch.

Hope this message makes it clear enough?

Thanks.

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

2017-03-22 10:54 GMT-06:00 Junio C Hamano :
> Alex Henrie  writes:
>> No problem. Do I need to submit a second version of the patch with a
>> test for `git -p log`?
>
> You do want to protect this "without an option, we default to
> 'auto'" feature from future breakage, no?

Yes, but I need to know whether you want a v2 of this patch with all
of the changes including the new test, or a second patch that depends
on the first patch and only adds the new test.

-Alex

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Alex Henrie  writes:

> 2017-03-21 16:28 GMT-06:00 Junio C Hamano :
>> Junio C Hamano  writes:
>>
  test_expect_success 'log.decorate configuration' '
 -git log --oneline >expect.none &&
 +git log --oneline --no-decorate >expect.none &&
  git log --oneline --decorate >expect.short &&
  git log --oneline --decorate=full >expect.full &&
>>>
>>> This ensures that an explicit --no-decorate from the command line
>>> does give "none" output, which we failed to do so far, and is a good
>>> change.  Don't we also need a _new_ test to ensure that "auto" kicks
>>> in without any explicit request?  Knowing the implementation that
>>> pager-in-use triggers the "auto" behaviour, perhaps testing the
>>> output from "git -p log" would be sufficient?
>>
>> BTW,
>>
>>>
>>> +static int auto_decoration_style()
>>> +{
>>> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
>>> +}
>>
>> FYI, I fixed this to
>>
>> static int auto_decoration_style(void)
>>
>> while queuing to make it compile.
>
> No problem. Do I need to submit a second version of the patch with a
> test for `git -p log`?

You do want to protect this "without an option, we default to
'auto'" feature from future breakage, no?

Thanks.

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

2017-03-21 16:28 GMT-06:00 Junio C Hamano :
> Junio C Hamano  writes:
>
>>>  test_expect_success 'log.decorate configuration' '
>>> -git log --oneline >expect.none &&
>>> +git log --oneline --no-decorate >expect.none &&
>>>  git log --oneline --decorate >expect.short &&
>>>  git log --oneline --decorate=full >expect.full &&
>>
>> This ensures that an explicit --no-decorate from the command line
>> does give "none" output, which we failed to do so far, and is a good
>> change.  Don't we also need a _new_ test to ensure that "auto" kicks
>> in without any explicit request?  Knowing the implementation that
>> pager-in-use triggers the "auto" behaviour, perhaps testing the
>> output from "git -p log" would be sufficient?
>
> BTW,
>
>>
>> +static int auto_decoration_style()
>> +{
>> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
>> +}
>
> FYI, I fixed this to
>
> static int auto_decoration_style(void)
>
> while queuing to make it compile.

No problem. Do I need to submit a second version of the patch with a
test for `git -p log`?

-Alex

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Junio C Hamano  writes:

>>  test_expect_success 'log.decorate configuration' '
>> -git log --oneline >expect.none &&
>> +git log --oneline --no-decorate >expect.none &&
>>  git log --oneline --decorate >expect.short &&
>>  git log --oneline --decorate=full >expect.full &&
>
> This ensures that an explicit --no-decorate from the command line
> does give "none" output, which we failed to do so far, and is a good
> change.  Don't we also need a _new_ test to ensure that "auto" kicks
> in without any explicit request?  Knowing the implementation that
> pager-in-use triggers the "auto" behaviour, perhaps testing the
> output from "git -p log" would be sufficient?

BTW,

>  
> +static int auto_decoration_style()
> +{
> + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
> +}

FYI, I fixed this to

static int auto_decoration_style(void)

while queuing to make it compile.

Re: [PATCH] log: if --decorate is not given, default to --decorate=auto

[Junio C Hamano]

Alex Henrie  writes:

> Git's branching and merging system can be confusing, especially to new
> users. When teaching people Git, I tell them to set log.decorate=auto.
> This preference greatly improves the user's awareness of the local and
> remote branches. So for the sake of user friendliness, I'd like to
> change the default from log.decorate=no to log.decorate=auto.
>
> Signed-off-by: Alex Henrie 

I guess this is an analog of making color.ui default to "auto" we
did earlier. After having shipped with a more conservative default
while letting willing users experiment/experience to help us get the
kinks out of the "auto" setting, it probably is about time to flip
the default.

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 48b55bfd2..3aa8daba3 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '
>  '
>  
>  test_expect_success 'log.decorate configuration' '
> - git log --oneline >expect.none &&
> + git log --oneline --no-decorate >expect.none &&
>   git log --oneline --decorate >expect.short &&
>   git log --oneline --decorate=full >expect.full &&

This ensures that an explicit --no-decorate from the command line
does give "none" output, which we failed to do so far, and is a good
change.  Don't we also need a _new_ test to ensure that "auto" kicks
in without any explicit request?  Knowing the implementation that
pager-in-use triggers the "auto" behaviour, perhaps testing the
output from "git -p log" would be sufficient?

[PATCH] log: if --decorate is not given, default to --decorate=auto

[Alex Henrie]

Git's branching and merging system can be confusing, especially to new
users. When teaching people Git, I tell them to set log.decorate=auto.
This preference greatly improves the user's awareness of the local and
remote branches. So for the sake of user friendliness, I'd like to
change the default from log.decorate=no to log.decorate=auto.

Signed-off-by: Alex Henrie 
---
 builtin/log.c  | 9 -
 t/t4202-log.sh | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 281af8c1e..ddb4515dc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -52,6 +52,11 @@ struct line_opt_callback_data {
struct string_list args;
 };
 
+static int auto_decoration_style()
+{
+   return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+}
+
 static int parse_decoration_style(const char *var, const char *value)
 {
switch (git_config_maybe_bool(var, value)) {
@@ -67,7 +72,7 @@ static int parse_decoration_style(const char *var, const char 
*value)
else if (!strcmp(value, "short"))
return DECORATE_SHORT_REFS;
else if (!strcmp(value, "auto"))
-   return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0;
+   return auto_decoration_style();
return -1;
 }
 
@@ -405,6 +410,8 @@ 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 48b55bfd2..3aa8daba3 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -520,7 +520,7 @@ test_expect_success 'log --graph with merge' '
 '
 
 test_expect_success 'log.decorate configuration' '
-   git log --oneline >expect.none &&
+   git log --oneline --no-decorate >expect.none &&
git log --oneline --decorate >expect.short &&
git log --oneline --decorate=full >expect.full &&
 
-- 
2.12.0