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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 8, 2020, 17:58 UTC
Message-ID: <xmqq1ri8y4zl.fsf@gitster.c.googlers.com>
In-Reply-To: <20201008162015.23898-1-avoidr@posteo.de>

Robert Karszniewicz <avoidr@posteo.de> writes:

Show 8 quoted lines

> 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 ;-)

Show 12 quoted lines

> 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.

Show 8 quoted lines

> +	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.