git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: Matheus Tavares <matheus.bernardino@usp.br>
Cc: git@vger.kernel.org,
	"Christian Couder" <christian.couder@gmail.com>,
	"Оля Тележная" <olyatelezhnaya@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [GSoC] Some #leftoverbits for anyone looking for little projects
Date: Tue, 21 May 2019 01:49:28 +0200	[thread overview]
Message-ID: <87d0kcvg8n.fsf@evledraar.gmail.com> (raw)
In-Reply-To: <20190520182353.22221-1-matheus.bernardino@usp.br>


On Mon, May 20 2019, Matheus Tavares wrote:

> Hi, Ævar
>
>> 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?

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

Aside from that, if this patch is kept I see commit.verbose is a
bool-or-int option, but yours is maybe-bool, so there's no way with
rebase.verboseCommit to turn on the higher level of verbosity. Perhaps
if this option is kept some implementation that just grabs whatever "X"
rebase.verboseCommit=X is set to and passes it as commit.verbase=X down
to git-commit is better, letting it deal with the validation?

> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index d98e32d812..ae50b3e05d 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -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.
> diff --git a/sequencer.c b/sequencer.c
> index f88a97fb10..1596fc4cd0 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -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())) {
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 1723e1a858..9b410d31e2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -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

  reply	other threads:[~2019-05-20 23:49 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-17 21:20 [GSoC] Some #leftoverbits for anyone looking for little projects Ævar Arnfjörð Bjarmason
2019-05-20 18:23 ` Matheus Tavares
2019-05-20 23:49   ` Ævar Arnfjörð Bjarmason [this message]
2019-05-21  4:38     ` Matheus Tavares Bernardino
2019-05-28 10:37   ` Johannes Schindelin
2019-05-28 17:37     ` Matheus Tavares Bernardino
2019-05-28 18:16       ` Johannes Schindelin
2019-05-29  9:38 ` Johannes Schindelin
2019-05-29  9:40   ` Johannes Schindelin
     [not found] ` <20220318220623.50528-1-gaurijove@gmail.com>
     [not found]   ` <CANsrJQdNKiX93GnVXztmvYQQBxr6-HsYNx5UvYXSFg32Op3ZPQ@mail.gmail.com>
     [not found]     ` <CANsrJQe1YuggxdBHdSdukXRj3myVCTNwLiiWNLrAzPpzA4FOOA@mail.gmail.com>
     [not found]       ` <220319.86ee2yds2f.gmgdl@evledraar.gmail.com>
     [not found]         ` <CANsrJQdJ1wBThUyJ=QSt6NwU8HzQY2VaWc11UfZQ+ktRQs_YTQ@mail.gmail.com>
     [not found]           ` <220319.86a6dlewyj.gmgdl@evledraar.gmail.com>
2022-03-20 19:14             ` Having grep support the -o option Jayati Shrivastava
2022-03-22  6:08               ` Christian Couder
2022-03-22 10:50                 ` Ævar Arnfjörð Bjarmason
2022-03-23 17:45                   ` Jayati Shrivastava

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87d0kcvg8n.fsf@evledraar.gmail.com \
    --to=avarab@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=matheus.bernardino@usp.br \
    --cc=olyatelezhnaya@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).