Re: [PATCH v2] reflog: specify default pretty format in config
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).
> 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.
Show 11 quoted lines
> 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.
Show 10 quoted lines
> 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.
Show 23 quoted lines
> 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.
Show 19 quoted lines
> 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".
Show 11 quoted lines
> + 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(-)
Show changes to builtin/log.c +4 −4
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);
Roland Hieber
