Some #leftoverbits for anyone looking for little projects

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

Hi, Ævar

Quoted 5 lines

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

I have the same problem, so I wanted to try solving this. The patch bellow creates a "rebase.verboseCommit" configuration that includes a diff when rewording or squashing. I'd appreciate knowing your thoughts on it.

As Christian wisely pointed out to me, though, we can also achieve this behavior by setting "commit.verbose" to true. The only "downside" of it is that users cannot choose to see the diff only when rebasing. Despite of that, if we decide not to go with this patch, what do you think of adding a "commit.verbose" entry at git-rebase's man page?

Documentation/config/rebase.txt +5 -0

index d98e32d812..ae50b3e05d 100644
@@ -62,3 +62,8 @@ rebase.rescheduleFailedExec::
 	Automatically reschedule `exec` commands that failed. This only makes
 	sense in interactive mode (or when an `--exec` option was provided).
 	This is the same as specifying the `--reschedule-failed-exec` option.
+
+rebase.verboseCommit::
+	When rewording or squashing commits, during an interactive rebase, show
+	the commits' diff to help describe the modifications they bring. False
+	by default.

sequencer.c +15 -2

index f88a97fb10..1596fc4cd0 100644
@@ -914,6 +914,7 @@ N_("you have staged changes in your working tree\n"
 #define CLEANUP_MSG (1<<3)
 #define VERIFY_MSG  (1<<4)
 #define CREATE_ROOT_COMMIT (1<<5)
+#define VERBOSE_COMMIT (1<<6)
 
 static int run_command_silent_on_success(struct child_process *cmd)
 {

@@ -1007,6 +1008,8 @@ static int run_git_commit(struct repository *r,
 		argv_array_push(&cmd.args, "-n");
 	if ((flags & AMEND_MSG))
 		argv_array_push(&cmd.args, "--amend");
+	if ((flags & VERBOSE_COMMIT))
+		argv_array_push(&cmd.args, "-v");
 	if (opts->gpg_sign)
 		argv_array_pushf(&cmd.args, "-S%s", opts->gpg_sign);
 	if (defmsg)

@@ -1782,7 +1785,7 @@ static int do_pick_commit(struct repository *r,
 	char *author = NULL;
 	struct commit_message msg = { NULL, NULL, NULL, NULL };
 	struct strbuf msgbuf = STRBUF_INIT;
-	int res, unborn = 0, allow;
+	int res, unborn = 0, allow, verbose_commit = 0;
 
 	if (opts->no_commit) {
 		/*

@@ -1843,6 +1846,9 @@ static int do_pick_commit(struct repository *r,
 		return error(_("cannot get commit message for %s"),
 			oid_to_hex(&commit->object.oid));
 
+	if (git_config_get_maybe_bool("rebase.verbosecommit", &verbose_commit) < 0)
+		warning("Invalid value for rebase.verboseCommit. Using 'false' instead.");
+
 	if (opts->allow_ff && !is_fixup(command) &&
 	    ((parent && oideq(&parent->object.oid, &head)) ||
 	     (!parent && unborn))) {

@@ -1853,6 +1859,8 @@ static int do_pick_commit(struct repository *r,
 		if (res || command != TODO_REWORD)
 			goto leave;
 		flags |= EDIT_MSG | AMEND_MSG | VERIFY_MSG;
+		if (verbose_commit)
+			flags |= VERBOSE_COMMIT;
 		msg_file = NULL;
 		goto fast_forward_edit;
 	}

@@ -1909,12 +1917,17 @@ static int do_pick_commit(struct repository *r,
 			author = get_author(msg.message);
 	}
 
-	if (command == TODO_REWORD)
+	if (command == TODO_REWORD) {
 		flags |= EDIT_MSG | VERIFY_MSG;
+		if (verbose_commit)
+			flags |= VERBOSE_COMMIT;
+	}
 	else if (is_fixup(command)) {
 		if (update_squash_messages(r, command, commit, opts))
 			return -1;
 		flags |= AMEND_MSG;
+		if (verbose_commit)
+			flags |= VERBOSE_COMMIT;
 		if (!final_fixup)
 			msg_file = rebase_path_squash_msg();
 		else if (file_exists(rebase_path_fixup_msg())) {

t/t3404-rebase-interactive.sh +56 -0

index 1723e1a858..9b410d31e2 100755
@@ -1477,4 +1477,60 @@ test_expect_success 'valid author header when author contains single quote' '
 	test_cmp expected actual
 '
 
+write_script "reword-and-check-for-diff" <<\EOF &&
+case "$1" in
+*/git-rebase-todo)
+	sed s/pick/reword/ "$1" > "$1.tmp"
+	mv -f "$1.tmp" "$1"
+	;;
+*)
+	grep '^diff --git' "$1" >has-diff
+	;;
+esac
+exit 0
+EOF
+
+test_expect_success 'rebase -i does not show diff by default when rewording' '
+	rebase_setup_and_clean no-verbose-commit-reword &&
+	test_set_editor "$PWD/reword-and-check-for-diff" &&
+	git rebase -i HEAD~1 &&
+	test_line_count = 0 has-diff
+'
+
+test_expect_success 'rebase -i respects rebase.verboseCommit when rewording' '
+	rebase_setup_and_clean verbose-commit-reword &&
+	test_config rebase.verboseCommit true &&
+	test_set_editor "$PWD/reword-and-check-for-diff" &&
+	git rebase -i HEAD~1 &&
+	test_line_count -gt 0 has-diff
+'
+
+write_script "squash-and-check-for-diff" <<\EOF &&
+case "$1" in
+*/git-rebase-todo)
+	sed "s/pick \([0-9a-f]*\) E/squash \1 E/" "$1" > "$1.tmp"
+	mv -f "$1.tmp" "$1"
+	;;
+*)
+	grep '^diff --git' "$1" >has-diff
+	;;
+esac
+exit 0
+EOF
+
+test_expect_success 'rebase -i does not show diff by default when squashing' '
+	rebase_setup_and_clean no-verbose-commit-squash &&
+	test_set_editor "$PWD/squash-and-check-for-diff" &&
+	git rebase -i HEAD~2 &&
+	test_line_count = 0 has-diff
+'
+
+test_expect_success 'rebase -i respects rebase.verboseCommit when squashing' '
+	rebase_setup_and_clean verbose-commit-squash &&
+	test_config rebase.verboseCommit true &&
+	test_set_editor "$PWD/squash-and-check-for-diff" &&
+	git rebase -i HEAD~2 &&
+	test_line_count -gt 0 has-diff
+'
+
 test_done

Quoted 13 lines · Matheus Tavares

Hi, Ævar

Quoted 5 lines

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

I have the same problem, so I wanted to try solving this. The patch bellow creates a "rebase.verboseCommit" configuration that includes a diff when rewording or squashing. I'd appreciate knowing your thoughts on it.

Quoted 5 lines

As Christian wisely pointed out to me, though, we can also achieve this behavior by setting "commit.verbose" to true. The only "downside" of it is that users cannot choose to see the diff only when rebasing. Despite of that, if we decide not to go with this patch, what do you think of adding a "commit.verbose" entry at git-rebase's man page?

Thanks for working on this. I'd somehow missed the addition of the commit.verbose option, so the problem I had is 100% solved by it (and I've turned it on).

I think it's better to just document it with rebase, perhaps rather than mention that option specifically (but that would also be fine) promise that we support "commit" options in general.

Do we promise anywhere that interactive rebase is going to run the "normal" git-commit command. From a quick skimming of the docs it doesn't seem so, perhaps we should explicitly promise that, and then test for it if we don't (e.g. by stealing the tests you added).

Quoted 5 lines

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

I have the same problem, so I wanted to try solving this. The patch bellow creates a "rebase.verboseCommit" configuration that includes a diff when rewording or squashing. I'd appreciate knowing your thoughts on it.

As Christian wisely pointed out to me, though, we can also achieve this behavior by setting "commit.verbose" to true. The only "downside" of it is that users cannot choose to see the diff only when rebasing.

Hi Matheus,

Quoted 11 lines · Matheus Tavares

Quoted 5 lines

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

I have the same problem, so I wanted to try solving this. The patch bellow creates a "rebase.verboseCommit" configuration that includes a diff when rewording or squashing. I'd appreciate knowing your thoughts on it.

Quoted 3 lines

As Christian wisely pointed out to me, though, we can also achieve this behavior by setting "commit.verbose" to true. The only "downside" of it is that users cannot choose to see the diff only when rebasing.

You could of course add an alias like

[alias]
myrebase = -c commit.verbose=true rebase

which *should* work.

However, I am actually slightly in favor of your patch because it *does* make it more convenient to have this on during rebases only.

Ciao,
Dscho

Quoted 14 lines · Johannes Schindelin

Quoted 11 lines · Matheus Tavares

Quoted 5 lines

Give "rebase -i" some option so when you "reword" the patch is included in the message.

I keep going to the shell because I have no idea what change I'm describing.

I have the same problem, so I wanted to try solving this. The patch bellow creates a "rebase.verboseCommit" configuration that includes a diff when rewording or squashing. I'd appreciate knowing your thoughts on it.

Quoted 8 lines

Quoted 3 lines

As Christian wisely pointed out to me, though, we can also achieve this behavior by setting "commit.verbose" to true. The only "downside" of it is that users cannot choose to see the diff only when rebasing.

You could of course add an alias like

[alias]
myrebase = -c commit.verbose=true rebase

Hmm, I didn't know about alias. Thanks for the information.

Quoted 2 lines

which *should* work.

Quoted 2 lines

However, I am actually slightly in favor of your patch because it *does* make it more convenient to have this on during rebases only.

Another option we were discussing is to document that rebase obeys all commit.* options, instead of adding the rebase.verboseCommit config. Yes, this way we won't be able to toggle diff for rebase only, but I'm not sure if that's something users would want to do...

In lieu of sending a PR to https://git.github.io/SoC-2018-Microprojects/
I thought I'd list a few more suggestions, and hopefully others will chime in.

Quoted 3 lines · Ævar Arnfjörð Bjarmason

In lieu of sending a PR to https://git.github.io/SoC-2018-Microprojects/
I thought I'd list a few more suggestions, and hopefully others will chime in.

I am in the same camp, and figured that GitGitGadget (which *already* augments the Git mailing list-centric workflow via GitHub's convenient UI) would make for a fine location for these small left-over bits. So I added them to https://github.com/gitgitgadget/git/issues/234 (except the

"git-unpack-*" idea, as I think that should be done as a test helper instead, and it should be done in the context of a new test case that actually needs this).

Ciao,
Dscho

One thing I'd *really* like to see is the bits of %(if) %(then) etc. extracted from ref-filter.c into some general API other commands could use with strbuf_expand() and friends.

I.e. if you could in addition to the strbuf_expand() callback define what verbs you support for "if" and the like, or have callbacks for their comparison functions.

Then have that machinery drive the whole format expansion, which eventually would expand your %(some-custom-thing) via a callback.

I.e. the whole "valid_atom" state machine in ref-filter.c.

Quoted 10 lines · Ævar Arnfjörð Bjarmason

One thing I'd *really* like to see is the bits of %(if) %(then) etc. extracted from ref-filter.c into some general API other commands could use with strbuf_expand() and friends.

I.e. if you could in addition to the strbuf_expand() callback define what verbs you support for "if" and the like, or have callbacks for their comparison functions.

Quoted 4 lines

Then have that machinery drive the whole format expansion, which eventually would expand your %(some-custom-thing) via a callback.

I.e. the whole "valid_atom" state machine in ref-filter.c.

So, the end goal is to design a formatting API that can be used by any command that takes --format option?

Previous interns worked on
unifying pretty.c and cat-file.c with ref-filter.c and so the next task can be to extend "valid_atom" state machine to work with more commands.

Do you have any suggestions for new atoms/verbs I could add support for or any such similar small exercise to start with?

Quoted 16 lines · Jayati Shrivastava

Quoted 13 lines · Ævar Arnfjörð Bjarmason

One thing I'd *really* like to see is the bits of %(if) %(then) etc. extracted from ref-filter.c into some general API other commands could use with strbuf_expand() and friends.

I.e. if you could in addition to the strbuf_expand() callback define what verbs you support for "if" and the like, or have callbacks for their comparison functions.

Then have that machinery drive the whole format expansion, which eventually would expand your %(some-custom-thing) via a callback.

Quoted 4 lines

Quoted 1 line

I.e. the whole "valid_atom" state machine in ref-filter.c.

So, the end goal is to design a formatting API that can be used by any command that takes --format option?

It might be nice if we get closer to this after your GSoC project, but I don't think it should become the main goal of the GSoC.

Quoted 17 lines · Christian Couder

Quoted 16 lines · Jayati Shrivastava

Quoted 13 lines · Ævar Arnfjörð Bjarmason

One thing I'd *really* like to see is the bits of %(if) %(then) etc. extracted from ref-filter.c into some general API other commands could use with strbuf_expand() and friends.

I.e. if you could in addition to the strbuf_expand() callback define what verbs you support for "if" and the like, or have callbacks for their comparison functions.

Then have that machinery drive the whole format expansion, which eventually would expand your %(some-custom-thing) via a callback.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

I.e. the whole "valid_atom" state machine in ref-filter.c.

So, the end goal is to design a formatting API that can be used by any command that takes --format option?

It might be nice if we get closer to this after your GSoC project, but I don't think it should become the main goal of the GSoC.

Agreed. FWIW this is off-list discussion between Jayati and myself which started with her asking (and this was omitted in the context that made it on list:

[...] Infact, I'll be excited to work on anything you suggest even if its not related to the project, as it will help me get familiar with the codebase and the contribution process at Git.

So that suggestion of mine of generalizing ref-filter.c wasn't meant to distract from the GSoC project, I understood her to be looking for suggestions for things to work on that were *not* part of the GSoC project.

So I suggested that ref-filter.c task, which is orthagonal, but relates to some of the same code.