Re: [PATCH v2] reflog: specify default pretty format in config

[Junio C Hamano <gitster@pobox.com>]

Roland Hieber <rhi@pengutronix.de> writes:

> The output of git-reflog(1) is currently only customisable by calling
> reflog with --pretty=... or overriding the "pretty.oneline" format in
> the config, which is currently the default fallback.

It is correct to point out that "git reflog --pretty=..." is a valid
way to get customized output, but I do not think the pretty.<name>
configuration allows you to modify built-in formats line oneline.

Don't overuse 'currently'.  The present tense is about describing
the current behaviour by default anyway.

	The output from "git reflog" can be customized with
	"--pretty=..." option from the command line.

is sufficient.

> To enhance flexibility and save typing, teach reflog to fall back to a
> default format specified in the config option "reflog.pretty" unless a
> different pretty format is given on the command line. This behaviour is
> similar to the "format.pretty" option for git-log(1) and git-show(1).

OK.

> When "reflog.pretty" is not set, fall back to the old default of
> --pretty=oneline --abbrev-hash.

I've never heard of the "--abbrev-hash" option.  Perhaps you meant
to say --abbrev-commit?

Make it a habit to try out any program example you write in your log
message or documentation and you'll do much better.

> diff --git a/Documentation/config/reflog.txt b/Documentation/config/reflog.txt
> new file mode 100644
> index 000000000..637cd852a
> --- /dev/null
> +++ b/Documentation/config/reflog.txt
> @@ -0,0 +1,5 @@
> +reflog.pretty::
> +	The default pretty format used by linkgit:git-reflog[1] if nothing
> +	else is specified via the `--pretty=` option.  If both are unset,
> +	git-reflog falls back to `--pretty=oneline --abbrev-hash`.  See the
> +	section Pretty Formats in linkgit:git-log[1] for possible values.

Likewise.

> diff --git a/Documentation/git-reflog.txt b/Documentation/git-reflog.txt
> index ff487ff77..9dccd4bcf 100644
> --- a/Documentation/git-reflog.txt
> +++ b/Documentation/git-reflog.txt
> @@ -41,6 +41,8 @@ command-line (or `HEAD`, by default). The reflog covers all recent
>  actions, and in addition the `HEAD` reflog records branch switching.
>  `git reflog show` is an alias for `git log -g --abbrev-commit
>  --pretty=oneline`; see linkgit:git-log[1] for more information.
> +The config option `reflog.pretty` is used as the default pretty
> +format if nothing else is specified.

The existing text in the pre-context hints a possible pitfall in
implementing this new feature.  When we defeat the built-in default
via the configuration or an explicit --pretty=... option, what would
(and more importantly, what should) happen to the --abbrev-commit
option given by the default?

Let's read on.

> diff --git a/builtin/log.c b/builtin/log.c
> index a479642eb..0592e5076 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -667,6 +667,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
>  {
>  	struct rev_info rev;
>  	struct setup_revision_opt opt;
> +	int cfg_have_pretty;
>  
>  	init_log_defaults();
>  	git_config(git_log_config, NULL);
> @@ -676,10 +677,14 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
>  	rev.verbose_header = 1;
>  	memset(&opt, 0, sizeof(opt));
>  	opt.def = "HEAD";
> +
> +	cfg_have_pretty = git_config_get_string_const("reflog.pretty", &fmt_pretty);
>  	cmd_log_init_defaults(&rev);
> -	rev.abbrev_commit = 1;
> -	rev.commit_format = CMIT_FMT_ONELINE;
> -	rev.use_terminator = 1;
> +	if (cfg_have_pretty != 0) {

This is misleading.  A natural thinking would go "unless we have
config, we use the default", so the following three lines that makes
the command use the default setting must be triggered when config
does *NOT* have reflog.pretty, and in C, a non-zero integer is
generally TRUE.

If you are using the fact that the result of git_config_get_X() is
non-zero if the configuration is not found, then fixing the polarity
of the value of the variable to match its name (or fixing the name
to match what it means), perhaps like:

	int needs_default;

	...
	needs_default = git_config_get_string_const(...);
	cmd_log_init_defaults(&rev);
	if (needs_default) {
		... set the --oneline --abbrev-commit ...
	}

would make the logic easier to follow.  Or if you choose to keep the
name, then

	cfg_have_pretty = !git_config_get_string_const(...);
	...
	if (!cfg_have_pretty) {
		...

I think "needs_default" would be a better solution that avoids
unnecessary negation.

> +		rev.abbrev_commit = 1;
> +		rev.commit_format = CMIT_FMT_ONELINE;
> +		rev.use_terminator = 1;
> +	}

And this answers my earlier question.  In the original code,
rev.abbrev_commit is always set by default and is not turned off, so
unlike "git log --oneline" that never gets .abbrev_commit=1 (hence
shows a full object name in the oneline output, unless shortened by
giving --abbrev-commit at the same time), "git reflog --oneline" would
still shorten the object names.

Even with this change, "git reflog --oneline" without reflog.pretty
configuration would behave the same as before.  Which is nice.

What interests us is how the new feature, i.e.

	git -c reflog.pretty=oneline reflog

would behave.  As we have the config, abbrev_commit is not set.  So
my guess is that it would give the full object names in the output?

In other words,

	git reflog --oneline
	git -c reflog.pretty=oneline reflog

would behave differently.

That is easily "fix"-able by moving the assignment of .abbrev_commit
outside the conditional.  I am not sure what the best way to justify
that behaviour, though.

> diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
> index ae8a448e3..74abf3d0f 100755
> --- a/t/t1410-reflog.sh
> +++ b/t/t1410-reflog.sh
> @@ -383,4 +383,25 @@ test_expect_success 'expire with multiple worktrees' '
>  	)
>  '
>  
> +test_expect_success '--pretty format' '
> +	git init pretty-formats &&
> +	(
> +		cd pretty-formats &&
> +		cat >expect <<-\EOF &&
> +		HEAD@{0} commit (initial): foobar
> +		EOF
> +		test_tick &&
> +		test_commit foobar &&
> +
> +		git reflog -n1 --pretty="%gD %gs" > output &&

Style: do not write SP after redirection operator '>' and its target
filename.

Style: our convention is, unless there is a good reason to deviate,
call the expected output "expect" and actual output "actual".

> +		test_cmp expect output &&
> +
> +		git -c reflog.pretty="%gD %gs" reflog -n1 > output &&
> +		test_cmp expect output &&
> +
> +		git -c reflog.pretty="%h" reflog -n1 --pretty="%gD %gs" > output &&
> +		test_cmp expect output
> +	)
> +'
> +
>  test_done

If I apply the attached patch on top of yours to restore the
behaviour of the command that forces .abbrev_commit to be always on,
these tests still pass.  Which means that this test is not covering
enough.  You would want to make sure "git -c reflog.pretty=oneline",
"git reflog --pretty=oneline", and "git reflog" behaves the same
way, at least, I think.


[Sample patch---do not blindly squash]

 builtin/log.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 619bb06686..00e29409bf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -670,7 +670,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
-	int cfg_have_pretty;
+	int needs_default;
 
 	init_log_defaults();
 	git_config(git_log_config, NULL);
@@ -681,13 +681,13 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
 	memset(&opt, 0, sizeof(opt));
 	opt.def = "HEAD";
 
-	cfg_have_pretty = git_config_get_string_const("reflog.pretty", &fmt_pretty);
+	needs_default = git_config_get_string_const("reflog.pretty", &fmt_pretty);
 	cmd_log_init_defaults(&rev);
-	if (cfg_have_pretty != 0) {
-		rev.abbrev_commit = 1;
+	if (needs_default) {
 		rev.commit_format = CMIT_FMT_ONELINE;
 		rev.use_terminator = 1;
 	}
+	rev.abbrev_commit = 1;
 	rev.always_show_header = 1;
 	cmd_log_init_finish(argc, argv, prefix, &rev, &opt);