Support --verbose in git history
cover Support --verbose in git history
The new git history subcommands like reword do not support showing the full git diff when editing the git commit message. This RFC adds support for git history reword behave similiar to git commit --amend --verbose except I choose to leave out support the "commit.verbose" configuration option.
Other git history subcommands like split, squash and fixup could also support --verbose under some circumstances (without --no-edit for fixup or only with --reedit-message for fixup)
I would be happy to work start with landing support for --verbose for git history reword, ideally with it also supporting the "commit.verbose" configuration option. Further patches could then add support for other subcommands where it makes sense.
Jelle van der Waa (2):
history: drop redundant strbuf_stripspace()
history: add --verbose option for reword
Documentation/git-history.adoc | 11 +++- builtin/history.c | 61 ++++++++++++++++++--- t/t3451-history-reword.sh | 97 ++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 9 deletions(-)
Signature · 1 line
2.56.0
RFC PATCH 1/2 history: drop redundant strbuf_stripspace()
cleanup_message() with COMMIT_MSG_CLEANUP_ALL already calls strbuf_stripspace() with the same arguments.
--- builtin/history.c | 1 - 1 file changed, 1 deletion(-)
builtin/history.c +0 -1
@@ -94,7 +94,6 @@ static int fill_commit_message(struct repository *repo,fprintf (stderr,_ ("Aborting commit as launching the editor failed.\n" ));return -1 ; } -strbuf_stripspace (out, comment_line_str);cleanup_message (out,COMMIT_MSG_CLEANUP_ALL ,0 );
Signature · 1 line
2.56.0
RFC PATCH 2/2 history: add --verbose option for reword
In reply to Jelle van der WaaWhen rewording a commit message it is often useful to have context of the changes. This commit implements the --verbose option of git commit in git history reword with the only difference in that it does not folllow the commit.verbose configuration setting.
--- Documentation/git-history.adoc | 11 +++- builtin/history.c | 60 ++++++++++++++++++--- t/t3451-history-reword.sh | 97 ++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 8 deletions(-)
Documentation/git-history.adoc +10 -1
@@ -10,7 +10,7 @@ SYNOPSIS [synopsis] git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]] git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]] -git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]] +git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [-v | --verbose] [--[no-]gpg-sign[=<key-id>]] git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]] [--] [<pathspec>...] git history squash [--dry-run] [--update-refs=(branches|head)] [--[no-]edit] [--[no-]gpg-sign[=<key-id>]] <revision-range>
@@ -180,6 +180,15 @@ OPTIONS `--reedit-message`:: Open an editor to modify the target commit's message. +`-v`:: +`--verbose`:: +`--no-verbose`:: + For `reword`, show the diff of the commit being reworded at the bottom + of the commit message to help the user describe the commit by reminding + what changes the commit has. + Note that this diff output doesn't have its lines prefixed with `#`. + This diff will not be a part of the commit message. + `-S[<key-id>]`:: `--gpg-sign[=<key-id>]`:: `--no-gpg-sign`::
builtin/history.c +53 -7
152 lines not on this page · raw message
t/t3451-history-reword.sh +97 -0
109 lines not on this page · raw message
Signature · 1 line
2.56.0
Re: [RFC PATCH 2/2] history: add --verbose option for reword
Hi Jelle
Quoted 2 lines · Jelle van der Waa
When rewording a commit message it is often useful to have context of the changes.
Indeed - I'm still using a wrapper around "git rebase -i" to reword commits because "git history reword" does not show the diff. Thanks for working on this.
Quoted 3 lines
This commit implements the --verbose option of git commit in git history reword with the only difference in that it does not folllow the commit.verbose configuration setting.
As it is easy to support commit.verbose, I think we should. In my ideal world we'd default to showing the diff when rewording but that's not really compatible with the way "commit.verbose" works.
Documentation/git-history.adoc SYNOPSIS · line 13
Quoted 10 lines
Documentation/git-history.adoc +1
-1@@ -10,7 +10,7 @@ SYNOPSIS [synopsis] git history drop <commit> [--dry-run] [--update-refs=(branches|head)] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]] git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]] -git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]] +git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [-v | --verbose] [--[no-]gpg-sign[=<key-id>]]
It is not a new problem, but these lines are getting very long.
builtin/history.c line 39
Quoted 10 lines
builtin/history.c +1
-1@@ -34,7 +36,7 @@#define GIT_HISTORY_FIXUP_USAGE \ N_("git history fixup <commit> [--dry-run] [--update-refs=(branches|head)] [--reedit-message] [--empty=(drop|keep|abort)] [--[no-]gpg-sign[=<key-id>]]")#define GIT_HISTORY_REWORD_USAGE \ - N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]]") + N_("git history reword <commit> [--dry-run] [--update-refs=(branches|head)] [-v | --verbose] [--[no-]gpg-sign[=<key-id>]]")
As above, it is not a new problem, but these lines are very long.
builtin/history.c
Quoted 10 lines
builtin/history.c +0
-0#define GIT_HISTORY_SPLIT_USAGE \ N_("git history split <commit> [--dry-run] [--update-refs=(branches|head)] [--[no-]gpg-sign[=<key-id>]] [--] [<pathspec>...]")#define GIT_HISTORY_SQUASH_USAGE \static int fill_commit_message(struct repository *repo,
const structobject_id*old_tree, const structobject_id*new_tree, const char*default_message, const char *action,builtin/history.c +1
-0+ int verbose,
If this was a bool, we could get rid of the "!!()" in the caller.
builtin/history.c static int fill_commit_message(struct repository *repo, · line 112
Quoted 10 lines
struct strbuf *out) { const char *path = git_path_commit_editmsg();builtin/history.c +3
-0@@ -84,6 +107,10 @@ static int fill_commit_message(struct repository *repo,wt_status_collect_changes_trees (&s, old_tree, new_tree);wt_status_print (&s); +if (verbose) { +wt_status_add_cut_line (&s); +print_verbose_diff (repo, old_tree, new_tree, s.fp);
We already reuse some of the code in wt-status.c that "git commit" uses to generate the template message, how hard would it be to teach the status code to generate the diff for us as it does for "git commit"? "git commit" also respects status.relativePaths, status.renames and status.renameLimit in addition to the equivalent diff settings - should we be doing the same here?
builtin/history.c static int history_config(const char *var, const char *value, · line 164
Quoted 12 lines
builtin/history.c +12
-0+static int reword_config (const char *var,const char *value, +const struct config_context *ctx,void *data) +{ +const char **sign_commit = data; + +if (!strcmp (var,"commit.gpgsign" )) { + *sign_commit =git_config_bool (var, value) ?"" :NULL ; +return 0 ; + } + +return git_diff_ui_config (var, value, ctx,NULL ); +}
I'm in two minds about this - it would be simpler to share the same config callback between all subcommands, even if it means we read a bit more config that we really need to for some commands.
Quoted 5 lines
t/t3451-history-reword.sh +0
-0
t/t3451-history-reword.sh reword_with_message () { · line 24
Quoted 9 lines
t/t3451-history-reword.sh +6
-0@@ -16,6 +16,19 @@ reword_with_message () { rm fake-editor.sh message } +reword_and_capture () { + write_script fake-editor.sh <<-\EOF && + cp "$1" captured && + { + echo reworded && + sed -n "/^# -* >8 -*$/,\$p" "$1"
We preserve the scissors line and the diff to check the message is cleaned up as we expect - good.
t/t3451-history-reword.sh test_expect_success 'aborts with empty commit message' ' · line 461
Quoted 12 lines
t/t3451-history-reword.sh +12
-0+test_expect_success '--verbose shows the diff of the reworded commit' ' + test_when_finished "rm -rf repo" && + git init repo && + ( + cd repo && + test_commit first && + test_commit second && + test_commit third && + echo unstaged >first.t && + echo staged >third.t && + git add third.t && + git show --format= HEAD~ >expect &&
This looks a bit strange - if we just want a diff why don't we create it by running "git diff"?
git diff HEAD~^- >expect
t/t3451-history-reword.sh test_expect_success 'aborts with empty commit message' ' · line 465
Quoted 4 lines
t/t3451-history-reword.sh +4
-0+ + reword_and_capture --verbose HEAD~ && + + test_grep "^# -* >8 -*\$" captured &&
If this line is not in the output then the sed command in the fake editor wont have printed the diff so I'm not sure this actually adds anything.
t/t3451-history-reword.sh test_expect_success 'aborts with empty commit message' ' · line 471
Quoted 6 lines
t/t3451-history-reword.sh +6
-0+ sed -n "/^diff --git/,\$p" captured >actual && + test_cmp expect actual && + + test_commit_message HEAD~ <<-\EOF + reworded + EOF
We check the diff and the reworded message - good. We do not check that the user sees the original message though - perhaps we'd be better to just compare captured against what we expect the user to see.
Quoted 1 line
+test_expect_success 'no diff is shown without --verbose' '
Why do we need a separate test to do this, rather than just checking there is no diff in one of the existing tests?
Quoted 1 line
+test_expect_success '--verbose works in a bare repository' '
I'm not sure this adds much value - we already have a test to check reword works in a bare repository and there is nothing in the implementation of verbose that is conditional on the whether the repository is bare or not. What failure mode do we expect this test to detect that the existing test wont?
Quoted 1 line
+test_expect_success '--verbose on a merge diffs against the first parent' '
Can we extend the existing test for rewording a merge commit instead of adding a new one? Our test suite is slow and the way the history tests are written makes it even slower because they initialize a new repository and create a bunch of new commits for every single test.
Thanks for working on this - it is a really useful improvement
Phillip
Quoted 3 lines
test_expect_success'retains changes in the worktree and index' '
test_when_finished"rm -rf repo" &&
git init repo &&
Quoted 6 lines · Jelle van der Waa
The new git history subcommands like reword do not support showing the full git diff when editing the git commit message. This RFC adds support for git history reword behave similiar to git commit --amend --verbose except I choose to leave out support the "commit.verbose" configuration option.
Just as a first thought, while it is nice to keep the interfaces consistent with each other, I wonder if we want to reserve a different meaning for "verbose" in the context of git-history? For example, I've sometimes wished git-history would show the rewritten commits after execution:
commit abc => def
…
Anyway, my point is, there's a lot of possible meanings for "verbose", so maybe we could use "--commit-verbose" (blech), "--diff-in-editor", or some other thing that smarter folks than I will come up with which clearly signals what we're about?
Quoted 13 lines
Other git history subcommands like split, squash and fixup could also support --verbose under some circumstances (without --no-edit for fixup or only with --reedit-message for fixup)
I would be happy to work start with landing support for --verbose for git history reword, ideally with it also supporting the "commit.verbose" configuration option. Further patches could then add support for other subcommands where it makes sense.
Jelle van der Waa (2):
history: drop redundantstrbuf_stripspace()
history: add --verbose option for reword
Quoted 7 lines
Documentation/git-history.adoc | 11 +++- builtin/history.c | 61 ++++++++++++++++++--- t/t3451-history-reword.sh | 97 ++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 9 deletions(-)--
2.56.0
Thanks for an interesting idea.
Signature · 1 line
D. Ben Knoble
Quoted 4 lines · D. Ben Knoble
Quoted 1 line · Jelle van der Waa
Anyway, my point is, there's a lot of possible meanings for "verbose",
That's a good point
Quoted 3 lines
so maybe we could use "--commit-verbose" (blech), "--diff-in-editor", or some other thing that smarter folks than I will come up with which clearly signals what we're about?
Off the top of my head --show-diff, though I'm not sure that's any better than your suggestions
Quoted 3 lines
Quoted 3 lines
Other git history subcommands like split, squash and fixup could also support --verbose under some circumstances (without --no-edit for fixup or only with --reedit-message for fixup)
That, and supporting "commit.verbose" is a good idea
Thanks
Phillip
Quoted 19 lines
Quoted 9 lines
I would be happy to work start with landing support for --verbose for git history reword, ideally with it also supporting the "commit.verbose" configuration option. Further patches could then add support for other subcommands where it makes sense.
Jelle van der Waa (2):
history: drop redundantstrbuf_stripspace()
history: add --verbose option for rewordQuoted 7 lines
Documentation/git-history.adoc | 11 +++- builtin/history.c | 61 ++++++++++++++++++--- t/t3451-history-reword.sh | 97 ++++++++++++++++++++++++++++++++++ 3 files changed, 160 insertions(+), 9 deletions(-)--
2.56.0Thanks for an interesting idea.
Quoted 6 lines · Phillip Wood
Quoted 4 lines · D. Ben Knoble
Quoted 1 line · Jelle van der Waa
Anyway, my point is, there's a lot of possible meanings for "verbose",
Quoted 9 lines
That's a good point
Quoted 3 lines
so maybe we could use "--commit-verbose" (blech), "--diff-in-editor", or some other thing that smarter folks than I will come up with which clearly signals what we're about?
Off the top of my head --show-diff, though I'm not sure that's any better than your suggestions
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
Quoted 15 lines · Karthik Nayak
Quoted 12 lines · Phillip Wood
Quoted 4 lines · D. Ben Knoble
Quoted 1 line · Jelle van der Waa
Anyway, my point is, there's a lot of possible meanings for "verbose",
That's a good point
Quoted 3 lines
so maybe we could use "--commit-verbose" (blech), "--diff-in-editor", or some other thing that smarter folks than I will come up with which clearly signals what we're about?
Quoted 7 lines
Quoted 3 lines
Off the top of my head --show-diff, though I'm not sure that's any better than your suggestions
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
When the diff is extremely long, or if the user has other means of obtaining it and wants the editor to open quickly, it may not be desired. So I'd prefer to still have it optional, personally.
Signature · 1 line
D. Ben Knoble
Quoted 9 lines · D. Ben Knoble
Quoted 4 lines · Karthik Nayak
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
When the diff is extremely long, or if the user has other means of obtaining it and wants the editor to open quickly, it may not be desired. So I'd prefer to still have it optional, personally.
Another case where we might not want the diff is scripted editing as it is much easier if the script does not have to worry about the diff getting in the way of its edits. Personally I'd be happy to see this on by default - the history command is marked as experimental so if having it on by default rather than following "commit.verbose" proves to be problematic we can always change it in the future. If we were staring again I'd also make "commit.verbose" on by default as well so maybe I'm biased.
Thanks
Phillip
Quoted 20 lines · Phillip Wood
Quoted 9 lines · D. Ben Knoble
Quoted 4 lines · Karthik Nayak
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
When the diff is extremely long, or if the user has other means of obtaining it and wants the editor to open quickly, it may not be desired. So I'd prefer to still have it optional, personally.
Another case where we might not want the diff is scripted editing as it is much easier if the script does not have to worry about the diff getting in the way of its edits. Personally I'd be happy to see this on by default - the history command is marked as experimental so if having it on by default rather than following "commit.verbose" proves to be problematic we can always change it in the future. If we were staring again I'd also make "commit.verbose" on by default as well so maybe I'm biased.
Quoted 3 lines
Thanks
Phillip
I'm on board with this suggestion. Plus, I see 'git history' to be more user facing and it's always nicer to have better defaults for users. Showing diffs and verbose information seems to be inline with that. We could add an option to disable such output for scripts (but that can be done later as you suggested).
Quoted 28 lines · Karthik Nayak
Quoted 22 lines · Phillip Wood
Quoted 10 lines · D. Ben Knoble
Quoted 4 lines · Karthik Nayak
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
When the diff is extremely long, or if the user has other means of obtaining it and wants the editor to open quickly, it may not be desired. So I'd prefer to still have it optional, personally.
Another case where we might not want the diff is scripted editing as it is much easier if the script does not have to worry about the diff getting in the way of its edits. Personally I'd be happy to see this on by default - the history command is marked as experimental so if having it on by default rather than following "commit.verbose" proves to be problematic we can always change it in the future. If we were staring again I'd also make "commit.verbose" on by default as well so maybe I'm biased.
Thanks
Quoted 8 lines
Quoted 1 line
Phillip
I'm on board with this suggestion. Plus, I see 'git history' to be more user facing and it's always nicer to have better defaults for users. Showing diffs and verbose information seems to be inline with that. We could add an option to disable such output for scripts (but that can be done later as you suggested).
<signature.asc>
I can live with opt-out as long as I can turn it off via config.
Quoted 30 lines · Ben Knoble
Quoted 30 lines · Karthik Nayak
Quoted 23 lines · Phillip Wood
Quoted 10 lines · D. Ben Knoble
Quoted 4 lines · Karthik Nayak
My preference would be to make this (current patch) default behavior and not even have an option. I don't see a disadvantage of having the diff always being shown and it definitely is a better default to have.
When the diff is extremely long, or if the user has other means of obtaining it and wants the editor to open quickly, it may not be desired. So I'd prefer to still have it optional, personally.
Another case where we might not want the diff is scripted editing as it is much easier if the script does not have to worry about the diff getting in the way of its edits. Personally I'd be happy to see this on by default - the history command is marked as experimental so if having it on by default rather than following "commit.verbose" proves to be problematic we can always change it in the future. If we were staring again I'd also make "commit.verbose" on by default as well so maybe I'm biased.
Thanks
Phillip
Quoted 8 lines
Quoted 6 lines
I'm on board with this suggestion. Plus, I see 'git history' to be more user facing and it's always nicer to have better defaults for users. Showing diffs and verbose information seems to be inline with that. We could add an option to disable such output for scripts (but that can be done later as you suggested).
<signature.asc>I can live with opt-out as long as I can turn it off via config.
Yeah, agreed. For me there's two important parts, where being able to disable this is the first one. The second part is that we need to make sure that such a format can work for every subcommand of git-history(1) so that we don't have to have multiple different formats for it.
Patrick