threads / patch / 45452

patchlog: if --decorate is not given, default to --decorate=auto

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

## tl;dr

13 messages between Mar 21, 2017 and Mar 24, 2017. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Alex Henrie· Mar 21, 2017, 05:52 UTC · lore

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 <alexhenrie24@gmail.com>
---
 builtin/log.c  | 9 ++++++++-
 t/t4202-log.sh | 2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)
Show changes to 2 files +9 −2

builtin/log.c, t/t4202-log.sh

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
Junio C Hamano· Mar 21, 2017, 17:34 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 7 quoted lines
> 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 <alexhenrie24@gmail.com>

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.

Show 12 quoted lines
> 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?

Junio C Hamano· Mar 21, 2017, 22:28 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>>  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,
Show 5 quoted lines
>  
> +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.
Alex Henrie· Mar 22, 2017, 05:47 UTC · re: Junio C Hamano · lore

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

2017-03-21 16:28 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
Show 28 quoted lines
> Junio C Hamano <gitster@pobox.com> 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
Junio C Hamano· Mar 22, 2017, 16:54 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 32 quoted lines
> 2017-03-21 16:28 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
>> Junio C Hamano <gitster@pobox.com> 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.
Alex Henrie· Mar 23, 2017, 15:07 UTC · re: Junio C Hamano · lore

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

2017-03-22 10:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
Show 6 quoted lines
> Alex Henrie <alexhenrie24@gmail.com> 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
Junio C Hamano· Mar 23, 2017, 15:54 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> writes:
Show 11 quoted lines
> 2017-03-22 10:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
>> Alex Henrie <alexhenrie24@gmail.com> 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.
Alex Henrie· Mar 23, 2017, 16:52 UTC · re: Junio C Hamano · lore

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

2017-03-23 9:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
Show 38 quoted lines
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> 2017-03-22 10:54 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
>>> Alex Henrie <alexhenrie24@gmail.com> 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
Junio C Hamano· Mar 23, 2017, 18:03 UTC · re: Alex Henrie · lore

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

Alex Henrie <alexhenrie24@gmail.com> 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.fsf@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.
Alex Henrie· Mar 24, 2017, 05:45 UTC · re: Junio C Hamano · lore

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

2017-03-23 12:03 GMT-06:00 Junio C Hamano <gitster@pobox.com>:
Show 10 quoted lines
> Alex Henrie <alexhenrie24@gmail.com> 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.fsf@gitster.mtv.corp.google.com/
That document was very helpful, thanks.
Show 13 quoted lines
>> 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
Jeff King· Mar 24, 2017, 18:55 UTC · re: Alex Henrie · lore

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

On Thu, Mar 23, 2017 at 10:52:34AM -0600, Alex Henrie wrote:
Show 8 quoted lines
> 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
Jeff King· Mar 24, 2017, 18:59 UTC · re: Jeff King · lore

[PATCH] pager_in_use: use git_env_bool()

On Fri, Mar 24, 2017 at 02:55:43PM -0400, Jeff King wrote:
> 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; [...]
I did see this small cleanup opportunity, though.
-- >8 --
Subject: [PATCH] pager_in_use: use git_env_bool()

The pager_in_use() function predates git_env_bool(), but ends up doing the same thing. Let's make use of the latter, which is shorter and less repetitive.

Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
Show changes to pager.c +1 −3
diff --git a/pager.c b/pager.c
index 73ca8bc3b..c113d898a 100644
--- a/pager.c
+++ b/pager.c
@@ -135,9 +135,7 @@ void setup_pager(void)
 
 int pager_in_use(void)
 {
-	const char *env;
-	env = getenv("GIT_PAGER_IN_USE");
-	return env ? git_config_bool("GIT_PAGER_IN_USE", env) : 0;
+	return git_env_bool("GIT_PAGER_IN_USE", 0);
 }
 
 /*

-- 
2.12.1.843.g1937c56c2
Junio C Hamano· Mar 24, 2017, 19:13 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> 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 ;-).

← back to recent threads