diff mbox series

[GSoC,1/1] rebase -i: add --ignore-whitespace flag

Message ID 20190712185015.20585-2-rohit.ashiwal265@gmail.com (mailing list archive)
State New, archived
Headers show
Series rebase -i: support --ignore-whitespace | expand

Commit Message

Rohit Ashiwal July 12, 2019, 6:50 p.m. UTC 
There are two backends available for rebasing, viz, the am and the
interactive. Naturally, there shall be some features that are
implemented in one but not in the other. One such flag is
--ignore-whitespace which indicates merge mechanism to treat lines
with only whitespace changes as unchanged. Wire the interactive
rebase to also understand the --ignore-whitespace flag by
translating it to -Xignore-space-change.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
---
 Documentation/git-rebase.txt            |  9 +++-
 builtin/rebase.c                        | 24 +++++++--
 sequencer.h                             |  1 +
 t/t3422-rebase-incompatible-options.sh  |  1 -
 t/t3431-rebase-options-compatibility.sh | 66 +++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 6 deletions(-)
 create mode 100755 t/t3431-rebase-options-compatibility.sh

Comments

Junio C Hamano July 15, 2019, 5:57 p.m. UTC | #1 
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> +	if (opts->ignore_whitespace) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (opts->strategy_opts)
> +			strbuf_addstr(&buf, opts->strategy_opts);
> +
> +		strbuf_addstr(&buf, " --ignore-space-change");
> +		free(opts->strategy_opts);

Is this call to free() safe?

> +		opts->strategy_opts = strbuf_detach(&buf, NULL);
> +	}
> +
> @@ -1821,6 +1836,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	if (options.rebase_merges) {
> +		if (options.ignore_whitespace)
> +			die(_("cannot combine '--rebase-merges' with "
> +			      "'--ignore-whitespace'"));

Hmph, this is unfortunate.  The patch is not making things worse, though.
Rohit Ashiwal July 15, 2019, 10 p.m. UTC | #2 
Hi Junio

On Mon, 15 Jul 2019 10:57:18 -0700 Junio C Hamano <gitster@pobox.com> wrote:
> 
> Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:
> 
> > +	if (opts->ignore_whitespace) {
> > +		struct strbuf buf = STRBUF_INIT;
> > +
> > +		if (opts->strategy_opts)
> > +			strbuf_addstr(&buf, opts->strategy_opts);
> > +
> > +		strbuf_addstr(&buf, " --ignore-space-change");
> > +		free(opts->strategy_opts);
> 
> Is this call to free() safe?

Yes, as far as I can tell. Since up till now, `strategy_opts` is
either "NULL" or xstrdup()'d string. There is no double free involved.

> > +		opts->strategy_opts = strbuf_detach(&buf, NULL);
> > +	}
> > +
> > @@ -1821,6 +1836,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >  	}
> >
> >  	if (options.rebase_merges) {
> > +		if (options.ignore_whitespace)
> > +			die(_("cannot combine '--rebase-merges' with "
> > +			      "'--ignore-whitespace'"));
> 
> Hmph, this is unfortunate.  The patch is not making things worse, though.

Actually, there is no need for --rebase-merges and --ignore-whitespace
to be "incompatible". My mentors also agree on this one. Making them
work in harmony will be handled in another patch series.

Best
Rohit
Junio C Hamano July 15, 2019, 10:08 p.m. UTC | #3 
Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:

> Hi Junio
>
> On Mon, 15 Jul 2019 10:57:18 -0700 Junio C Hamano <gitster@pobox.com> wrote:
>> 
>> Rohit Ashiwal <rohit.ashiwal265@gmail.com> writes:
>> 
>> > +	if (opts->ignore_whitespace) {
>> > +		struct strbuf buf = STRBUF_INIT;
>> > +
>> > +		if (opts->strategy_opts)
>> > +			strbuf_addstr(&buf, opts->strategy_opts);
>> > +
>> > +		strbuf_addstr(&buf, " --ignore-space-change");
>> > +		free(opts->strategy_opts);
>> 
>> Is this call to free() safe?
>
> Yes, as far as I can tell. Since up till now, `strategy_opts` is
> either "NULL" or xstrdup()'d string. There is no double free involved.

Doesn't it also come from handling OPT_STRING() via parse_options(),
which gives a pointer from argv[], freeing something that is not
allocated in the first place?
Rohit Ashiwal July 15, 2019, 10:42 p.m. UTC | #4 
Hi Junio

On Mon, 15 Jul 2019 15:08:30 -0700 Junio C Hamano <gitster@pobox.com> wrote:
>
> Doesn't it also come from handling OPT_STRING() via parse_options(),
> which gives a pointer from argv[], freeing something that is not
> allocated in the first place?

Ah! The path from cmd_rebase__interactive! A simple workaround could be:

diff --git a/builtin/rebase.c b/builtin/rebase.c
index c55957c31f..afe376c3fe 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -525,6 +525,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
        argc = parse_options(argc, argv, NULL, options,
                        builtin_rebase_interactive_usage, PARSE_OPT_KEEP_ARGV0);

+       opts.strategy_opts = xstrdup_or_null(opts.strategy_opts);
+
        if (!is_null_oid(&squash_onto))
                opts.squash_onto = &squash_onto;
diff mbox series

Patch

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 5e4e92764..eda52ed82 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -371,8 +371,13 @@  If either <upstream> or --root is given on the command line, then the
 default is `--no-fork-point`, otherwise the default is `--fork-point`.
 
 --ignore-whitespace::
+	This flag is either passed to the 'git apply' program
+	(see linkgit:git-apply[1]), or to 'git merge' program
+	(see linkgit:git-merge[1]) as `-Xignore-space-change`,
+	depending on which backend is selected by other options.
+
 --whitespace=<option>::
-	These flag are passed to the 'git apply' program
+	This flag is passed to the 'git apply' program
 	(see linkgit:git-apply[1]) that applies the patch.
 +
 See also INCOMPATIBLE OPTIONS below.
@@ -520,7 +525,6 @@  The following options:
  * --committer-date-is-author-date
  * --ignore-date
  * --whitespace
- * --ignore-whitespace
  * -C
 
 are incompatible with the following options:
@@ -543,6 +547,7 @@  In addition, the following pairs of options are incompatible:
  * --preserve-merges and --interactive
  * --preserve-merges and --signoff
  * --preserve-merges and --rebase-merges
+ * --rebase-merges and --ignore-whitespace
  * --rebase-merges and --strategy
  * --rebase-merges and --strategy-option
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index db6ca9bd7..c55957c31 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -79,6 +79,7 @@  struct rebase_options {
 	int allow_rerere_autoupdate;
 	int keep_empty;
 	int autosquash;
+	int ignore_whitespace;
 	char *gpg_sign_opt;
 	int autostash;
 	char *cmd;
@@ -376,6 +377,17 @@  static int run_rebase_interactive(struct rebase_options *opts,
 	flags |= opts->rebase_cousins > 0 ? TODO_LIST_REBASE_COUSINS : 0;
 	flags |= command == ACTION_SHORTEN_OIDS ? TODO_LIST_SHORTEN_IDS : 0;
 
+	if (opts->ignore_whitespace) {
+		struct strbuf buf = STRBUF_INIT;
+
+		if (opts->strategy_opts)
+			strbuf_addstr(&buf, opts->strategy_opts);
+
+		strbuf_addstr(&buf, " --ignore-space-change");
+		free(opts->strategy_opts);
+		opts->strategy_opts = strbuf_detach(&buf, NULL);
+	}
+
 	switch (command) {
 	case ACTION_NONE: {
 		if (!opts->onto && !opts->upstream)
@@ -489,6 +501,8 @@  int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
 		{ OPTION_STRING, 'S', "gpg-sign", &opts.gpg_sign_opt, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
+		OPT_BOOL(0, "ignore-whitespace", &opts.ignore_whitespace,
+			 N_("ignore changes in whitespace")),
 		OPT_STRING(0, "strategy", &opts.strategy, N_("strategy"),
 			   N_("rebase strategy")),
 		OPT_STRING(0, "strategy-opts", &opts.strategy_opts, N_("strategy-opts"),
@@ -954,6 +968,8 @@  static int run_am(struct rebase_options *opts)
 	am.git_cmd = 1;
 	argv_array_push(&am.args, "am");
 
+	if (opts->ignore_whitespace)
+		argv_array_push(&am.args, "--ignore-whitespace");
 	if (opts->action && !strcmp("continue", opts->action)) {
 		argv_array_push(&am.args, "--resolved");
 		argv_array_pushf(&am.args, "--resolvemsg=%s", resolvemsg);
@@ -1401,9 +1417,6 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG, NULL, REBASE_DIFFSTAT },
 		OPT_BOOL(0, "signoff", &options.signoff,
 			 N_("add a Signed-off-by: line to each commit")),
-		OPT_PASSTHRU_ARGV(0, "ignore-whitespace", &options.git_am_opts,
-				  NULL, N_("passed to 'git am'"),
-				  PARSE_OPT_NOARG),
 		OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date",
 				  &options.git_am_opts, NULL,
 				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
@@ -1411,6 +1424,8 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 				  N_("passed to 'git am'"), PARSE_OPT_NOARG),
 		OPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_("n"),
 				  N_("passed to 'git apply'"), 0),
+		OPT_BOOL(0, "ignore-whitespace", &options.ignore_whitespace,
+			 N_("ignore changes in whitespace")),
 		OPT_PASSTHRU_ARGV(0, "whitespace", &options.git_am_opts,
 				  N_("action"), N_("passed to 'git apply'"), 0),
 		OPT_BIT('f', "force-rebase", &options.flags,
@@ -1821,6 +1836,9 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	}
 
 	if (options.rebase_merges) {
+		if (options.ignore_whitespace)
+			die(_("cannot combine '--rebase-merges' with "
+			      "'--ignore-whitespace'"));
 		if (strategy_options.nr)
 			die(_("cannot combine '--rebase-merges' with "
 			      "'--strategy-option'"));
diff --git a/sequencer.h b/sequencer.h
index 0c494b83d..303047a13 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -43,6 +43,7 @@  struct replay_opts {
 	int verbose;
 	int quiet;
 	int reschedule_failed_exec;
+	int ignore_whitespace;
 
 	int mainline;
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index a5868ea15..4342f79ee 100755
--- a/t/t3422-rebase-incompatible-options.sh
+++ b/t/t3422-rebase-incompatible-options.sh
@@ -61,7 +61,6 @@  test_rebase_am_only () {
 }
 
 test_rebase_am_only --whitespace=fix
-test_rebase_am_only --ignore-whitespace
 test_rebase_am_only --committer-date-is-author-date
 test_rebase_am_only -C4
 
diff --git a/t/t3431-rebase-options-compatibility.sh b/t/t3431-rebase-options-compatibility.sh
new file mode 100755
index 000000000..f38ae6f5f
--- /dev/null
+++ b/t/t3431-rebase-options-compatibility.sh
@@ -0,0 +1,66 @@ 
+#!/bin/sh
+#
+# Copyright (c) 2019 Rohit Ashiwal
+#
+
+test_description='tests to ensure compatibility between am and interactive backends'
+
+. ./test-lib.sh
+
+# This is a special case in which both am and interactive backends
+# provide the same outputs. It was done intentionally because
+# --ignore-whitespace both the backends fall short of optimal
+# behaviour.
+test_expect_success 'setup' '
+	git checkout -b topic &&
+	q_to_tab >file <<-EOF &&
+	line 1
+	Qline 2
+	line 3
+	EOF
+	git add file &&
+	git commit -m "add file" &&
+	q_to_tab >file <<-EOF &&
+	line 1
+	new line 2
+	line 3
+	EOF
+	git commit -am "update file" &&
+	git tag side &&
+
+	git checkout --orphan master &&
+	q_to_tab >file <<-EOF &&
+	line 1
+	        line 2
+	line 3
+	EOF
+	git add file &&
+	git commit -m "add file" &&
+	git tag main
+'
+
+test_expect_success '--ignore-whitespace works with am backend' '
+	cat >expect <<-EOF &&
+	line 1
+	new line 2
+	line 3
+	EOF
+	test_must_fail git rebase main side &&
+	git rebase --abort &&
+	git rebase --ignore-whitespace main side &&
+	test_cmp expect file
+'
+
+test_expect_success '--ignore-whitespace works with interactive backend' '
+	cat >expect <<-EOF &&
+	line 1
+	new line 2
+	line 3
+	EOF
+	test_must_fail git rebase --merge main side &&
+	git rebase --abort &&
+	git rebase --merge --ignore-whitespace main side &&
+	test_cmp expect file
+'
+
+test_done