| Message ID | 20201008162015.23898-1-avoidr@posteo.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] log: add log.showStat configuration variable | expand |
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.
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.
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!
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
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.
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,