[RFC PATCH] log: add log.showStat configuration variable

[RFC PATCH] log: add log.showStat configuration variable

[Robert Karszniewicz]

Changes default behaviour of `git log` and `git show` when no
command-line options are given. Doesn't affect behaviour otherwise (same
behaviour as with stash.showStat).
---
I've wanted to have `show` and `log` show --stat by default, and I
couldn't find any better solution for it. And I've discovered that there
is stash.showStat, which is exactly what I want. So I wanted to bring
stash.showStat to `show` and `log`.

So far, setting log.showStat affects behaviour as described in the
commit message.
But it does so for `show` and `log` at the same time. I think they
should be configurable separately. (log.showStat and show.showStat)

Before I do all the work, please tell me if this is the right approach
so far, and if the feature - when ready - would be accepted. (I'm aware
that documentation and tests are missing.)

 builtin/log.c | 22 ++++++++++++++++++----
 revision.h    |  1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 0a7ed4bef9..225252f0b4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -45,6 +45,7 @@ static int default_abbrev_commit;
 static int default_show_root = 1;
 static int default_follow;
 static int default_show_signature;
+static int default_show_stat;
 static int default_encode_email_headers = 1;
 static int decoration_style;
 static int decoration_given;
@@ -151,6 +152,7 @@ static void cmd_log_init_defaults(struct rev_info *rev)
 	rev->show_root_diff = default_show_root;
 	rev->subject_prefix = fmt_patch_subject_prefix;
 	rev->show_signature = default_show_signature;
+	rev->show_stat = default_show_stat;
 	rev->encode_email_headers = default_encode_email_headers;
 	rev->diffopt.flags.allow_textconv = 1;
 
@@ -488,6 +490,10 @@ static int git_log_config(const char *var, const char *value, void *cb)
 		default_show_signature = git_config_bool(var, value);
 		return 0;
 	}
+	if (!strcmp(var, "log.showstat")) {
+		default_show_stat = git_config_bool(var, value);
+		return 0;
+	}
 
 	if (grep_config(var, value, cb) < 0)
 		return -1;
@@ -607,8 +613,11 @@ static void show_setup_revisions_tweak(struct rev_info *rev,
 			rev->dense_combined_merges = 1;
 		}
 	}
-	if (!rev->diffopt.output_format)
+	if (!rev->diffopt.output_format) {
 		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
+		if (rev->show_stat)
+			rev->diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
+	}
 }
 
 int cmd_show(int argc, const char **argv, const char *prefix)
@@ -727,9 +736,14 @@ static void log_setup_revisions_tweak(struct rev_info *rev,
 	    rev->prune_data.nr == 1)
 		rev->diffopt.flags.follow_renames = 1;
 
-	/* Turn --cc/-c into -p --cc/-c when -p was not given */
-	if (!rev->diffopt.output_format && rev->combine_merges)
-		rev->diffopt.output_format = DIFF_FORMAT_PATCH;
+	if (!rev->diffopt.output_format) {
+		/* Turn --cc/-c into -p --cc/-c when -p was not given */
+		if (rev->combine_merges)
+			rev->diffopt.output_format = DIFF_FORMAT_PATCH;
+
+		if (rev->show_stat)
+			rev->diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
+	}
 
 	if (rev->first_parent_only && rev->ignore_merges < 0)
 		rev->ignore_merges = 0;
diff --git a/revision.h b/revision.h
index f6bf860d19..e402c519d8 100644
--- a/revision.h
+++ b/revision.h
@@ -204,6 +204,7 @@ struct rev_info {
 			show_merge:1,
 			show_notes_given:1,
 			show_signature:1,
+			show_stat:1,
 			pretty_given:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
-- 
2.28.0

Re: [RFC PATCH] log: add log.showStat configuration variable

[Junio C Hamano]

Robert Karszniewicz <avoidr@posteo.de> writes:

> Changes default behaviour of `git log` and `git show` when no
> command-line options are given. Doesn't affect behaviour otherwise (same
> behaviour as with stash.showStat).
> ---
> I've wanted to have `show` and `log` show --stat by default, and I
> couldn't find any better solution for it. And I've discovered that there
> is stash.showStat, which is exactly what I want. So I wanted to bring
> stash.showStat to `show` and `log`.

I would be happy if I can configure my "git show" to 

 - show not just patch but stat by default;
 - keep showing nothing when told to be silent with "git show -s"

independently what happens to my "git log".  Specifically, I do not
want to see a configuration that I use to tweak "git show" the way I
want (see above) to make my "git log" to become "git log --stat".

And why is "stat" so special?  I am sure there are people who want
to do --numstat or --summary or combinations of these by default,
so I doubt that a new bit in rev_info structure is a good way to go.

> So far, setting log.showStat affects behaviour as described in the
> commit message.
> But it does so for `show` and `log` at the same time. I think they
> should be configurable separately. (log.showStat and show.showStat)

Absolutely.

> Before I do all the work, please tell me if this is the right approach
> so far, and if the feature - when ready - would be accepted. (I'm aware
> that documentation and tests are missing.)

Nobody will get such a guarantee.  A good test to see if a topic is
worth spending the reviewers' time on is if the authors are willing
to spend their time whether it will be in the official relesae or it
will have to be kept in a private fork for the authors' own use.  If
it is not good enough that the authors won't keep a fork of Git just
to use it for themselves, it is hard to imagine that it would be
good enough for public consumption.

In short, make it so useful that we'd come to you begging for it ;-)

> diff --git a/revision.h b/revision.h
> index f6bf860d19..e402c519d8 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -204,6 +204,7 @@ struct rev_info {
>  			show_merge:1,
>  			show_notes_given:1,
>  			show_signature:1,
> +			show_stat:1,
>  			pretty_given:1,
>  			abbrev_commit:1,
>  			abbrev_commit_given:1,

The change to the code we saw in builtin/log.c, e.g.

> +	if (!rev->diffopt.output_format) {
> +		/* Turn --cc/-c into -p --cc/-c when -p was not given */
> +		if (rev->combine_merges)
> +			rev->diffopt.output_format = DIFF_FORMAT_PATCH;
> +
> +		if (rev->show_stat)
> +			rev->diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
> +	}

hints us that this new bit belongs to the group that the
combine_merges bit belongs to, not here, no?

But again, I am not sure if a new bit in rev_info structure is a
good way to proceed---after all, when a diff (in various forms, like
"patch", "stat only", "patch and stat", "patch, stat, and summary")
is shown, how exactly they are shown is not controlled by bits in this
structure (rather, that comes from the diffopt field).

Thanks.

Re: [RFC PATCH] log: add log.showStat configuration variable

[Derrick Stolee]

On 10/8/2020 12:20 PM, Robert Karszniewicz wrote:
> Changes default behaviour of `git log` and `git show` when no
> command-line options are given. Doesn't affect behaviour otherwise (same
> behaviour as with stash.showStat).
> ---
> I've wanted to have `show` and `log` show --stat by default, and I
> couldn't find any better solution for it. And I've discovered that there
> is stash.showStat, which is exactly what I want. So I wanted to bring
> stash.showStat to `show` and `log`.

I'm wondering: why should this be a config setting instead of just
a configure alias?

	git config --global alias.logs "log --stat"
 
My personal preference is to use "--graph --oneline" by default, so
I use

	git config --global alias.lg "log --graph --oneline"

and then type "git lg ..." whenever I'm looking at history. I also
have an easy way to turn off the graph by using just "git log" when
I want that disabled.

> So far, setting log.showStat affects behaviour as described in the
> commit message.
> But it does so for `show` and `log` at the same time. I think they
> should be configurable separately. (log.showStat and show.showStat)
> 
> Before I do all the work, please tell me if this is the right approach
> so far, and if the feature - when ready - would be accepted. (I'm aware
> that documentation and tests are missing.)

If this is something we want to do as a config instead of alias,
I'm wondering if it is worth expanding the scope and thinking about
these other arguments (like --graph, --oneline, etc.) and how they
could be incorporated into a coherent config system.

I worry that this initial step leads us down a road of slowly adding
one-off config settings for each option when:

 1. aliases exist, and
 2. it becomes unclear which arguments have configured defaults.

Thanks,
-Stolee

Re: [RFC PATCH] log: add log.showStat configuration variable

[Robert Karszniewicz]

On Thu, Oct 08, 2020 at 10:58:22AM -0700, Junio C Hamano wrote:
> Robert Karszniewicz <avoidr@posteo.de> writes:
> 
> > Changes default behaviour of `git log` and `git show` when no
> > command-line options are given. Doesn't affect behaviour otherwise (same
> > behaviour as with stash.showStat).
> > ---
> > I've wanted to have `show` and `log` show --stat by default, and I
> > couldn't find any better solution for it. And I've discovered that there
> > is stash.showStat, which is exactly what I want. So I wanted to bring
> > stash.showStat to `show` and `log`.
> 
> I would be happy if I can configure my "git show" to 
> 
>  - show not just patch but stat by default;
>  - keep showing nothing when told to be silent with "git show -s"
> 
> independently what happens to my "git log".  Specifically, I do not
> want to see a configuration that I use to tweak "git show" the way I
> want (see above) to make my "git log" to become "git log --stat".
> 
> And why is "stat" so special?  I am sure there are people who want
> to do --numstat or --summary or combinations of these by default,

I think --stat is "special" because it is the most prominent one,
popularized by the format-patch format. I've personally come to like
--stat very much, to me it serves as a TOC of a commit, an extension of
the commit message, an essential description of a commit/patch.

It makes sense for format-patch, but it does not make less sense for
`show`. (Or does it? I mean, if it is a good idea for distributable
patch files, why is it less of a good idea for local commits/patches?)

Then we also have `stash-show`, which shows nothing /but/ --stat by
default. Here again: what's the difference between `show` and
`stash-show`? One might say "different contexts", but I don't see them
being that different, really. It just seems inconsistent to me. 

And that was what I wanted to achieve with my patch - to make it
possible to make the three formats consistent with each other.

That's how I think --stat is special. For other "unknown"/"custom"
options I would use an alias, as I already do for variations of `log`
options.
Then why did I still add log.showStat? Because it seemed like too close
of a relative not to do it. Also because I personally use it and I
believe that commit message and stat belong together and it's an
injustice to separate them. And still only because "--stat is special".

> > diff --git a/revision.h b/revision.h
> > index f6bf860d19..e402c519d8 100644
> > --- a/revision.h
> > +++ b/revision.h
> > @@ -204,6 +204,7 @@ struct rev_info {
> >  			show_merge:1,
> >  			show_notes_given:1,
> >  			show_signature:1,
> > +			show_stat:1,
> >  			pretty_given:1,
> >  			abbrev_commit:1,
> >  			abbrev_commit_given:1,
> 
> The change to the code we saw in builtin/log.c, e.g.
> 
> > +	if (!rev->diffopt.output_format) {
> > +		/* Turn --cc/-c into -p --cc/-c when -p was not given */
> > +		if (rev->combine_merges)
> > +			rev->diffopt.output_format = DIFF_FORMAT_PATCH;
> > +
> > +		if (rev->show_stat)
> > +			rev->diffopt.output_format |= DIFF_FORMAT_DIFFSTAT;
> > +	}
> 
> hints us that this new bit belongs to the group that the
> combine_merges bit belongs to, not here, no?

Right! I remember being unsure about it, but then the peer pressure of
all the show* variables made me group it to them.

> 
> But again, I am not sure if a new bit in rev_info structure is a
> good way to proceed---after all, when a diff (in various forms, like
> "patch", "stat only", "patch and stat", "patch, stat, and summary")
> is shown, how exactly they are shown is not controlled by bits in this
> structure (rather, that comes from the diffopt field).

I will try to find a better way.

> 
> Thanks.

Thank you for your comments.

Re: [RFC PATCH] log: add log.showStat configuration variable

[Robert Karszniewicz]

On Thu, Oct 08, 2020 at 02:12:50PM -0400, Derrick Stolee wrote:
> On 10/8/2020 12:20 PM, Robert Karszniewicz wrote:
> > Changes default behaviour of `git log` and `git show` when no
> > command-line options are given. Doesn't affect behaviour otherwise (same
> > behaviour as with stash.showStat).
> > ---
> > I've wanted to have `show` and `log` show --stat by default, and I
> > couldn't find any better solution for it. And I've discovered that there
> > is stash.showStat, which is exactly what I want. So I wanted to bring
> > stash.showStat to `show` and `log`.
> 
> I'm wondering: why should this be a config setting instead of just
> a configure alias?

I answered this in the reply to Junio C Hamano.

Actually, the first thing I tried, was make an alias named after the git
command, like so:

  git config --global alias.show "show --stat"
  git config --global alias.log "log --stat"

But that didn't work. Why, actually? We're used to it from our POSIX
shells, and other places I can't think of, but it feels familiar.
Perhaps this would be a good way to enable changing default behaviour of
each git command without having to change anything about config
handling? Would this be difficult to do?

> If this is something we want to do as a config instead of alias,
> I'm wondering if it is worth expanding the scope and thinking about
> these other arguments (like --graph, --oneline, etc.) and how they
> could be incorporated into a coherent config system.
> 
> I worry that this initial step leads us down a road of slowly adding
> one-off config settings for each option when:

I worried about that, too. But I think the initial step was already in
2015, when stash.showStat and stash.showPatch were added. No flood of
options happened since then? I was actually surprised about it, too,
that it took so long until someone wanted to have showStat for show and
log, too.

> 
>  1. aliases exist, and
>  2. it becomes unclear which arguments have configured defaults.
> 
> Thanks,
> -Stolee
> 

Thank you!

Re: [RFC PATCH] log: add log.showStat configuration variable

[Derrick Stolee]

On 10/11/2020 5:59 AM, Robert Karszniewicz wrote:
> On Thu, Oct 08, 2020 at 02:12:50PM -0400, Derrick Stolee wrote:
>> On 10/8/2020 12:20 PM, Robert Karszniewicz wrote:
>>> Changes default behaviour of `git log` and `git show` when no
>>> command-line options are given. Doesn't affect behaviour otherwise (same
>>> behaviour as with stash.showStat).
>>> ---
>>> I've wanted to have `show` and `log` show --stat by default, and I
>>> couldn't find any better solution for it. And I've discovered that there
>>> is stash.showStat, which is exactly what I want. So I wanted to bring
>>> stash.showStat to `show` and `log`.
>>
>> I'm wondering: why should this be a config setting instead of just
>> a configure alias?
> 
> I answered this in the reply to Junio C Hamano.
> 
> Actually, the first thing I tried, was make an alias named after the git
> command, like so:
> 
>   git config --global alias.show "show --stat"
>   git config --global alias.log "log --stat"
> 
> But that didn't work. Why, actually? We're used to it from our POSIX
> shells, and other places I can't think of, but it feels familiar.
> Perhaps this would be a good way to enable changing default behaviour of
> each git command without having to change anything about config
> handling? Would this be difficult to do?

You can't replace a builtin with an alias, because that creates a
recursive loop. Note that I changed the name to "slog" for my example.

If you are going to customize it, then you need to remember your new
name. But this is something you can do right now without needing to
patch Git.

>> If this is something we want to do as a config instead of alias,
>> I'm wondering if it is worth expanding the scope and thinking about
>> these other arguments (like --graph, --oneline, etc.) and how they
>> could be incorporated into a coherent config system.
>>
>> I worry that this initial step leads us down a road of slowly adding
>> one-off config settings for each option when:
> 
> I worried about that, too. But I think the initial step was already in
> 2015, when stash.showStat and stash.showPatch were added. No flood of
> options happened since then? I was actually surprised about it, too,
> that it took so long until someone wanted to have showStat for show and
> log, too.

I'm not sure these examples will help your case.

Does 'stash' have more things that would be beneficial to show
every time? If no, then 'stash' is much more specialized than
'show' and 'log' which have many more options. If yes, then this
is exactly what we want to avoid happening: an incomplete set of
config options that are tailored to a small subset of options.

While my stance is still "an alias should suffice," perhaps it is
worth investigating the "status.*" config options, which include
this kind of behavior:

* status.aheadBehind can disable some output normally there by
  default. (This was created for performance implications.)

* status.showStash enables --show-stash

* status.showUntrackedFiles enables --untracked-files

* status.submoduleSummary interacts with --ignore-submodules

Thanks,
-Stolee

Re: [RFC PATCH] log: add log.showStat configuration variable

[Junio C Hamano]

Derrick Stolee <stolee@gmail.com> writes:

>> I worried about that, too. But I think the initial step was already in
>> 2015, when stash.showStat and stash.showPatch were added. No flood of
>> options happened since then? I was actually surprised about it, too,
>> that it took so long until someone wanted to have showStat for show and
>> log, too.
>
> I'm not sure these examples will help your case.
>
> Does 'stash' have more things that would be beneficial to show
> every time? If no, then 'stash' is much more specialized than
> 'show' and 'log' which have many more options. If yes, then this
> is exactly what we want to avoid happening: an incomplete set of
> config options that are tailored to a small subset of options.

Well said---I do not have anything more to add to that point that
'stash' is not a very good example to mimic.