[v5,1/5] rebase -i: add --ignore-whitespace flag
diff mbox series

Message ID 20200626095528.117524-2-phillip.wood123@gmail.com
State New
Headers show
Series
  • cleanup ra/rebase-i-more-options
Related show

Commit Message

Phillip Wood June 26, 2020, 9:55 a.m. UTC
From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

Rebase is implemented with two different backends - 'apply' and
'merge' each of which support a different set of options. In
particular the apply backend supports a number of options implemented
by 'git am' that are not implemented in the merge backend. This means
that the available options are different depending on which backend is
used which is confusing. This patch adds support for the
--ignore-whitespace option to the merge backend. This option treats
lines with only whitespace changes as unchanged and is implemented in
the merge backend by translating it to -Xignore-space-change.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
 Documentation/git-rebase.txt           | 19 +++++++-
 builtin/rebase.c                       | 16 +++++--
 t/t3422-rebase-incompatible-options.sh |  1 -
 t/t3436-rebase-more-options.sh         | 60 ++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 6 deletions(-)
 create mode 100755 t/t3436-rebase-more-options.sh

Comments

Đoàn Trần Công Danh June 26, 2020, 1:37 p.m. UTC | #1
On 2020-06-26 10:55:24+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> 
> Rebase is implemented with two different backends - 'apply' and
> 'merge' each of which support a different set of options. In
> particular the apply backend supports a number of options implemented
> by 'git am' that are not implemented in the merge backend. This means
> that the available options are different depending on which backend is
> used which is confusing. This patch adds support for the
> --ignore-whitespace option to the merge backend. This option treats
> lines with only whitespace changes as unchanged and is implemented in
> the merge backend by translating it to -Xignore-space-change.
> 
> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  Documentation/git-rebase.txt           | 19 +++++++-
>  builtin/rebase.c                       | 16 +++++--
>  t/t3422-rebase-incompatible-options.sh |  1 -
>  t/t3436-rebase-more-options.sh         | 60 ++++++++++++++++++++++++++
>  4 files changed, 90 insertions(+), 6 deletions(-)
>  create mode 100755 t/t3436-rebase-more-options.sh
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f7a6033607..b003784f01 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -422,8 +422,23 @@ your branch contains commits which were dropped, this option can be used
>  with `--keep-base` in order to drop those commits from your branch.
>  
>  --ignore-whitespace::
> +	Ignore whitespace differences when trying to reconcile
> +differences. Currently, each backend implements an approximation of
> +this behavior:
> ++
> +apply backend: When applying a patch, ignore changes in whitespace in
> +context lines. Unfortunately, this means that if the "old" lines being
> +replaced by the patch differ only in whitespace from the existing
> +file, you will get a merge conflict instead of a successful patch
> +application.
> ++
> +merge backend: Treat lines with only whitespace changes as unchanged
> +when merging. Unfortunately, this means that any patch hunks that were
> +intended to modify whitespace and nothing else will be dropped, even
> +if the other side had no changes that conflicted.
> +
>  --whitespace=<option>::
> -	These flags 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.
>  	Implies --apply.
>  +
> @@ -572,7 +587,6 @@ The following options:
>   * --apply
>   * --committer-date-is-author-date
>   * --ignore-date
> - * --ignore-whitespace
>   * --whitespace
>   * -C
>  
> @@ -598,6 +612,7 @@ In addition, the following pairs of options are incompatible:
>   * --preserve-merges and --signoff
>   * --preserve-merges and --rebase-merges
>   * --preserve-merges and --empty=
> + * --preserve-merges and --ignore-whitespace
>   * --keep-base and --onto
>   * --keep-base and --root
>  
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 27a07d4e78..810c9b7779 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -126,6 +126,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>  	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>  	replay.strategy = opts->strategy;
> +
>  	if (opts->strategy_opts)
>  		parse_strategy_opts(&replay, opts->strategy_opts);
>  
> @@ -1466,6 +1467,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	struct strbuf revisions = STRBUF_INIT;
>  	struct strbuf buf = STRBUF_INIT;
>  	struct object_id merge_base;
> +	int ignore_whitespace = 0;
>  	enum action action = ACTION_NONE;
>  	const char *gpg_sign = NULL;
>  	struct string_list exec = STRING_LIST_INIT_NODUP;
> @@ -1495,16 +1497,15 @@ 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),
>  		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
>  				  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", &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,
> @@ -1850,6 +1851,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		imply_merge(&options, "--rebase-merges");
>  	}
>  
> +	if (options.type == REBASE_APPLY) {
> +		if (ignore_whitespace)
> +			argv_array_push (&options.git_am_opts,
> +					 "--ignore-whitespace");
> +	} else if (ignore_whitespace) {
> +			string_list_append (&strategy_options,
> +					    "ignore-space-change");
> +	}
> +

Hm, I've just noticed this by now.
Would it's better if we rewrite it as:

	if (ignore_whitespace) {
		if (options.type == REBASE_APPLY)
			argv_array_push(&options.git_am_opts,
					"--ignore-whitespace");
		else
			string_list_append(&strategy_options,
					   "ignore-space-change");
	}

Ah, the incoming patches will add more conditions into the:

	if (options.type == REBASE_APPLY)

I'm still not convinced, though.

Anyway, IIRC, --ignore-whitespace and --ignore-space-change has the
same meaning, I think it's better to use the same option for both
legs, no?

I can understand the decision to use --ignore-whitespace as keeping
the pass-through behavior of old code, but I think future maintenance
is more important than that.

I've tried changing ignore-whitespace to ignore-space-change and run
make test

It looks good to me (aka nothing failed _in my machine_),
4/5 and 5/5 is not applied, though.

>  	if (strategy_options.nr) {
>  		int i;
>  
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 50e7960702..55ca46786d 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/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> new file mode 100755
> index 0000000000..4f8a6e51c9
> --- /dev/null
> +++ b/t/t3436-rebase-more-options.sh
> @@ -0,0 +1,60 @@
> +#!/bin/sh
> +#
> +# Copyright (c) 2019 Rohit Ashiwal
> +#
> +
> +test_description='tests to ensure compatibility between am and interactive backends'
> +
> +. ./test-lib.sh
> +
> +. "$TEST_DIRECTORY"/lib-rebase.sh
> +
> +# This is a special case in which both am and interactive backends
> +# provide the same output. It was done intentionally because
> +# both the backends fall short of optimal behaviour.
> +test_expect_success 'setup' '
> +	git checkout -b topic &&
> +	test_write_lines "line 1" "	line 2" "line 3" >file &&
> +	git add file &&
> +	git commit -m "add file" &&
> +
> +	test_write_lines "line 1" "new line 2" "line 3" >file &&
> +	git commit -am "update file" &&
> +	git tag side &&
> +
> +	git checkout --orphan master &&
> +	test_write_lines "line 1" "        line 2" "line 3" >file &&
> +	git commit -am "add file" &&
> +	git tag main
> +'
> +
> +test_expect_success '--ignore-whitespace works with apply backend' '
> +	test_must_fail git rebase --apply main side &&
> +	git rebase --abort &&
> +	git rebase --apply --ignore-whitespace main side &&
> +	git diff --exit-code side
> +'
> +
> +test_expect_success '--ignore-whitespace works with merge backend' '
> +	test_must_fail git rebase --merge main side &&
> +	git rebase --abort &&
> +	git rebase --merge --ignore-whitespace main side &&
> +	git diff --exit-code side
> +'
> +
> +test_expect_success '--ignore-whitespace is remembered when continuing' '
> +	(
> +		set_fake_editor &&
> +		FAKE_LINES="break 1" git rebase -i --ignore-whitespace \
> +			main side &&
> +		git rebase --continue
> +	) &&
> +	git diff --exit-code side
> +'
> +
> +# This must be the last test in this file
> +test_expect_success '$EDITOR and friends are unchanged' '
> +	test_editor_unchanged
> +'
> +
> +test_done
> -- 
> 2.27.0
>
Phillip Wood June 26, 2020, 2:43 p.m. UTC | #2
Hi Danh

On 26/06/2020 14:37, Đoàn Trần Công Danh wrote:
> On 2020-06-26 10:55:24+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>>
>> Rebase is implemented with two different backends - 'apply' and
>> 'merge' each of which support a different set of options. In
>> particular the apply backend supports a number of options implemented
>> by 'git am' that are not implemented in the merge backend. This means
>> that the available options are different depending on which backend is
>> used which is confusing. This patch adds support for the
>> --ignore-whitespace option to the merge backend. This option treats
>> lines with only whitespace changes as unchanged and is implemented in
>> the merge backend by translating it to -Xignore-space-change.
>>
>> Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> ---
>>   Documentation/git-rebase.txt           | 19 +++++++-
>>   builtin/rebase.c                       | 16 +++++--
>>   t/t3422-rebase-incompatible-options.sh |  1 -
>>   t/t3436-rebase-more-options.sh         | 60 ++++++++++++++++++++++++++
>>   4 files changed, 90 insertions(+), 6 deletions(-)
>>   create mode 100755 t/t3436-rebase-more-options.sh
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index f7a6033607..b003784f01 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -422,8 +422,23 @@ your branch contains commits which were dropped, this option can be used
>>   with `--keep-base` in order to drop those commits from your branch.
>>   
>>   --ignore-whitespace::
>> +	Ignore whitespace differences when trying to reconcile
>> +differences. Currently, each backend implements an approximation of
>> +this behavior:
>> ++
>> +apply backend: When applying a patch, ignore changes in whitespace in
>> +context lines. Unfortunately, this means that if the "old" lines being
>> +replaced by the patch differ only in whitespace from the existing
>> +file, you will get a merge conflict instead of a successful patch
>> +application.
>> ++
>> +merge backend: Treat lines with only whitespace changes as unchanged
>> +when merging. Unfortunately, this means that any patch hunks that were
>> +intended to modify whitespace and nothing else will be dropped, even
>> +if the other side had no changes that conflicted.
>> +
>>   --whitespace=<option>::
>> -	These flags 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.
>>   	Implies --apply.
>>   +
>> @@ -572,7 +587,6 @@ The following options:
>>    * --apply
>>    * --committer-date-is-author-date
>>    * --ignore-date
>> - * --ignore-whitespace
>>    * --whitespace
>>    * -C
>>   
>> @@ -598,6 +612,7 @@ In addition, the following pairs of options are incompatible:
>>    * --preserve-merges and --signoff
>>    * --preserve-merges and --rebase-merges
>>    * --preserve-merges and --empty=
>> + * --preserve-merges and --ignore-whitespace
>>    * --keep-base and --onto
>>    * --keep-base and --root
>>   
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 27a07d4e78..810c9b7779 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -126,6 +126,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>>   	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>>   	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>>   	replay.strategy = opts->strategy;
>> +
>>   	if (opts->strategy_opts)
>>   		parse_strategy_opts(&replay, opts->strategy_opts);
>>   
>> @@ -1466,6 +1467,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   	struct strbuf revisions = STRBUF_INIT;
>>   	struct strbuf buf = STRBUF_INIT;
>>   	struct object_id merge_base;
>> +	int ignore_whitespace = 0;
>>   	enum action action = ACTION_NONE;
>>   	const char *gpg_sign = NULL;
>>   	struct string_list exec = STRING_LIST_INIT_NODUP;
>> @@ -1495,16 +1497,15 @@ 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),
>>   		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
>>   				  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", &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,
>> @@ -1850,6 +1851,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		imply_merge(&options, "--rebase-merges");
>>   	}
>>   
>> +	if (options.type == REBASE_APPLY) {
>> +		if (ignore_whitespace)
>> +			argv_array_push (&options.git_am_opts,
>> +					 "--ignore-whitespace");
>> +	} else if (ignore_whitespace) {
>> +			string_list_append (&strategy_options,
>> +					    "ignore-space-change");
>> +	}
>> +
> 
> Hm, I've just noticed this by now.
> Would it's better if we rewrite it as:
> 
> 	if (ignore_whitespace) {
> 		if (options.type == REBASE_APPLY)
> 			argv_array_push(&options.git_am_opts,
> 					"--ignore-whitespace");
> 		else
> 			string_list_append(&strategy_options,
> 					   "ignore-space-change");
> 	}
> 
> Ah, the incoming patches will add more conditions into the:
> 
> 	if (options.type == REBASE_APPLY)
> 
> I'm still not convinced, though.

I wanted to keep the subsequent patches as simple as possible. Having to 
rewrite the if statement in the next patch just clutters it up and makes 
the real changes introduced by that patch less obvious

> Anyway, IIRC, --ignore-whitespace and --ignore-space-change has the
> same meaning, I think it's better to use the same option for both
> legs, no?
> 
> I can understand the decision to use --ignore-whitespace as keeping
> the pass-through behavior of old code, but I think future maintenance
> is more important than that.

I'm not sure how it affects future maintenance. The two different 
options are for two different commands so I'm not sure it is worth the 
effort

Best Wishes

Phillip

> I've tried changing ignore-whitespace to ignore-space-change and run
> make test
> 
> It looks good to me (aka nothing failed _in my machine_),
> 4/5 and 5/5 is not applied, though.
> 
>>   	if (strategy_options.nr) {
>>   		int i;
>>   
>> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
>> index 50e7960702..55ca46786d 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/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
>> new file mode 100755
>> index 0000000000..4f8a6e51c9
>> --- /dev/null
>> +++ b/t/t3436-rebase-more-options.sh
>> @@ -0,0 +1,60 @@
>> +#!/bin/sh
>> +#
>> +# Copyright (c) 2019 Rohit Ashiwal
>> +#
>> +
>> +test_description='tests to ensure compatibility between am and interactive backends'
>> +
>> +. ./test-lib.sh
>> +
>> +. "$TEST_DIRECTORY"/lib-rebase.sh
>> +
>> +# This is a special case in which both am and interactive backends
>> +# provide the same output. It was done intentionally because
>> +# both the backends fall short of optimal behaviour.
>> +test_expect_success 'setup' '
>> +	git checkout -b topic &&
>> +	test_write_lines "line 1" "	line 2" "line 3" >file &&
>> +	git add file &&
>> +	git commit -m "add file" &&
>> +
>> +	test_write_lines "line 1" "new line 2" "line 3" >file &&
>> +	git commit -am "update file" &&
>> +	git tag side &&
>> +
>> +	git checkout --orphan master &&
>> +	test_write_lines "line 1" "        line 2" "line 3" >file &&
>> +	git commit -am "add file" &&
>> +	git tag main
>> +'
>> +
>> +test_expect_success '--ignore-whitespace works with apply backend' '
>> +	test_must_fail git rebase --apply main side &&
>> +	git rebase --abort &&
>> +	git rebase --apply --ignore-whitespace main side &&
>> +	git diff --exit-code side
>> +'
>> +
>> +test_expect_success '--ignore-whitespace works with merge backend' '
>> +	test_must_fail git rebase --merge main side &&
>> +	git rebase --abort &&
>> +	git rebase --merge --ignore-whitespace main side &&
>> +	git diff --exit-code side
>> +'
>> +
>> +test_expect_success '--ignore-whitespace is remembered when continuing' '
>> +	(
>> +		set_fake_editor &&
>> +		FAKE_LINES="break 1" git rebase -i --ignore-whitespace \
>> +			main side &&
>> +		git rebase --continue
>> +	) &&
>> +	git diff --exit-code side
>> +'
>> +
>> +# This must be the last test in this file
>> +test_expect_success '$EDITOR and friends are unchanged' '
>> +	test_editor_unchanged
>> +'
>> +
>> +test_done
>> -- 
>> 2.27.0
>>
>
Junio C Hamano June 26, 2020, 3:43 p.m. UTC | #3
Phillip Wood <phillip.wood123@gmail.com> writes:

> ... This option treats
> lines with only whitespace changes as unchanged and is implemented in
> the merge backend by translating it to -Xignore-space-change.

OK.  Hiding the subtle difference is good.

> @@ -598,6 +612,7 @@ In addition, the following pairs of options are incompatible:
>   * --preserve-merges and --signoff
>   * --preserve-merges and --rebase-merges
>   * --preserve-merges and --empty=
> + * --preserve-merges and --ignore-whitespace
>   * --keep-base and --onto
>   * --keep-base and --root

Hmph...

> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 27a07d4e78..810c9b7779 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -126,6 +126,7 @@ static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>  	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
>  	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
>  	replay.strategy = opts->strategy;
> +
>  	if (opts->strategy_opts)
>  		parse_strategy_opts(&replay, opts->strategy_opts);

Usually I would complain about whitespace-only changes to places
where there is no real change, but I am OK with this one.  It does
make sense to have a blank here.

> @@ -1850,6 +1851,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		imply_merge(&options, "--rebase-merges");
>  	}
>  
> +	if (options.type == REBASE_APPLY) {
> +		if (ignore_whitespace)
> +			argv_array_push (&options.git_am_opts,
> +					 "--ignore-whitespace");
> +	} else if (ignore_whitespace) {
> +			string_list_append (&strategy_options,
> +					    "ignore-space-change");
> +	}

I agree with the other reviewer that "when --ignore-whitespace is
given, do these different things depending on the .type" is easier
to follow.  Also pay attention to the style.  There is no SP between
the name of the function and the opening '(' for its parameter list.

	if (ignore_whitespace) {
        	if (options.type == REBASE_APPLY)
			argv_array_push(&options.git_am_opts, "--ignore-whitespace");
		else
			string_list_append(&strategy_options, "ignore-space-change");
	}


> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 50e7960702..55ca46786d 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

OK.

Thanks.
Junio C Hamano June 26, 2020, 4:03 p.m. UTC | #4
Phillip Wood <phillip.wood123@gmail.com> writes:

>>>   +	if (options.type == REBASE_APPLY) {
>>> +		if (ignore_whitespace)
>>> +			argv_array_push (&options.git_am_opts,
>>> +					 "--ignore-whitespace");
>>> +	} else if (ignore_whitespace) {
>>> +			string_list_append (&strategy_options,
>>> +					    "ignore-space-change");
>>> +	}
>>> +
>> ...
> I wanted to keep the subsequent patches as simple as possible. Having
> to rewrite the if statement in the next patch just clutters it up and
> makes the real changes introduced by that patch less obvious

A set of different behaviour depending on .type is OK, but then at
least the above should be more like this:

	if (options.type == REBASE_APPLY) {
		if (ignore_whitespace)
			argv_array_push(...);
	} else {
		/* REBASE_MERGE and PRESERVE_MERGES */
		if (ignore_whitespace)
			string_list_append(...);
	}

or even

	switch (options.type) {
	case REBASE_APPLY:
		...
		break;
	case REBASE_MERGE:
	case REBASE_PRESERVE_MERGES:
		...
		break;
	default:
		BUG("unhandled rebase type %d", options.type);
	}

That would clarify the flow of the logic better.

Thanks.
Đoàn Trần Công Danh June 29, 2020, 2:14 p.m. UTC | #5
Hi Phillip,

On 2020-06-26 15:43:00+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
> > > +	if (options.type == REBASE_APPLY) {
> > > +		if (ignore_whitespace)
> > > +			argv_array_push (&options.git_am_opts,
> > > +					 "--ignore-whitespace");
> > > +	} else if (ignore_whitespace) {
> > > +			string_list_append (&strategy_options,
> > > +					    "ignore-space-change");
> > > +	}
> > > +
> > 
> > Hm, I've just noticed this by now.
> > Would it's better if we rewrite it as:
> > 
> > 	if (ignore_whitespace) {
> > 		if (options.type == REBASE_APPLY)
> > 			argv_array_push(&options.git_am_opts,
> > 					"--ignore-whitespace");
> > 		else
> > 			string_list_append(&strategy_options,
> > 					   "ignore-space-change");
> > 	}
> > 
> > Ah, the incoming patches will add more conditions into the:
> > 
> > 	if (options.type == REBASE_APPLY)
> > 
> > I'm still not convinced, though.
> 
> I wanted to keep the subsequent patches as simple as possible. Having to
> rewrite the if statement in the next patch just clutters it up and makes the
> real changes introduced by that patch less obvious

I think the code suggested by Junio may be cleaner ;)
I may write as:

	if (ignore_whitespace) {
		if (options.type == REBASE_APPLY)
			argv_array_push(...)
		else
			string_list_append(...)
	}

	if (other_condition)
		do_something_else(...)

I don't know if it's cleaner or not.
I haven't tried applied it into real code.

> > Anyway, IIRC, --ignore-whitespace and --ignore-space-change has the
> > same meaning, I think it's better to use the same option for both
> > legs, no?
> > 
> > I can understand the decision to use --ignore-whitespace as keeping
> > the pass-through behavior of old code, but I think future maintenance
> > is more important than that.
> 
> I'm not sure how it affects future maintenance. The two different options
> are for two different commands so I'm not sure it is worth the effort

I vaguely remember I was thinking about same option text in both leg
would make the code easier for reasoning. And we can unify the
recommendation for both backend. We will never strip
--ignore-whitespace from git-apply, but it would be easier to always
answer: "--ignore-space-change should be used to ignore space" when
someone asks about it.

Typing this now makes me wonder if we should teach --ignore-space-change to
git-rebase?

Thanks,

-- Danh

> 
> Best Wishes
> 
> Phillip
> 
> > I've tried changing ignore-whitespace to ignore-space-change and run
> > make test
> > 
> > It looks good to me (aka nothing failed _in my machine_),
> > 4/5 and 5/5 is not applied, though.
> > 
> > >   	if (strategy_options.nr) {
> > >   		int i;
> > > diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> > > index 50e7960702..55ca46786d 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/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
> > > new file mode 100755
> > > index 0000000000..4f8a6e51c9
> > > --- /dev/null
> > > +++ b/t/t3436-rebase-more-options.sh
> > > @@ -0,0 +1,60 @@
> > > +#!/bin/sh
> > > +#
> > > +# Copyright (c) 2019 Rohit Ashiwal
> > > +#
> > > +
> > > +test_description='tests to ensure compatibility between am and interactive backends'
> > > +
> > > +. ./test-lib.sh
> > > +
> > > +. "$TEST_DIRECTORY"/lib-rebase.sh
> > > +
> > > +# This is a special case in which both am and interactive backends
> > > +# provide the same output. It was done intentionally because
> > > +# both the backends fall short of optimal behaviour.
> > > +test_expect_success 'setup' '
> > > +	git checkout -b topic &&
> > > +	test_write_lines "line 1" "	line 2" "line 3" >file &&
> > > +	git add file &&
> > > +	git commit -m "add file" &&
> > > +
> > > +	test_write_lines "line 1" "new line 2" "line 3" >file &&
> > > +	git commit -am "update file" &&
> > > +	git tag side &&
> > > +
> > > +	git checkout --orphan master &&
> > > +	test_write_lines "line 1" "        line 2" "line 3" >file &&
> > > +	git commit -am "add file" &&
> > > +	git tag main
> > > +'
> > > +
> > > +test_expect_success '--ignore-whitespace works with apply backend' '
> > > +	test_must_fail git rebase --apply main side &&
> > > +	git rebase --abort &&
> > > +	git rebase --apply --ignore-whitespace main side &&
> > > +	git diff --exit-code side
> > > +'
> > > +
> > > +test_expect_success '--ignore-whitespace works with merge backend' '
> > > +	test_must_fail git rebase --merge main side &&
> > > +	git rebase --abort &&
> > > +	git rebase --merge --ignore-whitespace main side &&
> > > +	git diff --exit-code side
> > > +'
> > > +
> > > +test_expect_success '--ignore-whitespace is remembered when continuing' '
> > > +	(
> > > +		set_fake_editor &&
> > > +		FAKE_LINES="break 1" git rebase -i --ignore-whitespace \
> > > +			main side &&
> > > +		git rebase --continue
> > > +	) &&
> > > +	git diff --exit-code side
> > > +'
> > > +
> > > +# This must be the last test in this file
> > > +test_expect_success '$EDITOR and friends are unchanged' '
> > > +	test_editor_unchanged
> > > +'
> > > +
> > > +test_done
> > > -- 
> > > 2.27.0
> > > 
> >
Phillip Wood July 13, 2020, 10:02 a.m. UTC | #6
Hi Danh

On 29/06/2020 15:14, Đoàn Trần Công Danh wrote:
> Hi Phillip,
> 
> On 2020-06-26 15:43:00+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>> +	if (options.type == REBASE_APPLY) {
>>>> +		if (ignore_whitespace)
>>>> +			argv_array_push (&options.git_am_opts,
>>>> +					 "--ignore-whitespace");
>>>> +	} else if (ignore_whitespace) {
>>>> +			string_list_append (&strategy_options,
>>>> +					    "ignore-space-change");
>>>> +	}
>>>> +
>>>
>>> Hm, I've just noticed this by now.
>>> Would it's better if we rewrite it as:
>>>
>>> 	if (ignore_whitespace) {
>>> 		if (options.type == REBASE_APPLY)
>>> 			argv_array_push(&options.git_am_opts,
>>> 					"--ignore-whitespace");
>>> 		else
>>> 			string_list_append(&strategy_options,
>>> 					   "ignore-space-change");
>>> 	}
>>>
>>> Ah, the incoming patches will add more conditions into the:
>>>
>>> 	if (options.type == REBASE_APPLY)
>>>
>>> I'm still not convinced, though.
>>
>> I wanted to keep the subsequent patches as simple as possible. Having to
>> rewrite the if statement in the next patch just clutters it up and makes the
>> real changes introduced by that patch less obvious
> 
> I think the code suggested by Junio may be cleaner ;)

Yes, I'll reroll with Junio's suggestion

> I may write as:
> 
> 	if (ignore_whitespace) {
> 		if (options.type == REBASE_APPLY)
> 			argv_array_push(...)
> 		else
> 			string_list_append(...)
> 	}
> 
> 	if (other_condition)
> 		do_something_else(...)
> 
> I don't know if it's cleaner or not.

It leads to a lot of repetition of ' && options.type == REBASE_APPLY' as
all the other conditions apart from --ignore-whitespace only need
handling for the apply backend here.

> I haven't tried applied it into real code.
> 
>>> Anyway, IIRC, --ignore-whitespace and --ignore-space-change has the
>>> same meaning, I think it's better to use the same option for both
>>> legs, no?
>>>
>>> I can understand the decision to use --ignore-whitespace as keeping
>>> the pass-through behavior of old code, but I think future maintenance
>>> is more important than that.
>>
>> I'm not sure how it affects future maintenance. The two different options
>> are for two different commands so I'm not sure it is worth the effort
> 
> I vaguely remember I was thinking about same option text in both leg
> would make the code easier for reasoning. And we can unify the
> recommendation for both backend. We will never strip
> --ignore-whitespace from git-apply, but it would be easier to always
> answer: "--ignore-space-change should be used to ignore space" when
> someone asks about it.
> 
> Typing this now makes me wonder if we should teach --ignore-space-change to
> git-rebase?

Possibly but not as part of this series which is about improving the
merge backend rather than adding new options to rebase itself

Best Wishes

Phillip

> 
> Thanks,
> 
> -- Danh
> 
>>
>> Best Wishes
>>
>> Phillip
>>
>>> I've tried changing ignore-whitespace to ignore-space-change and run
>>> make test
>>>
>>> It looks good to me (aka nothing failed _in my machine_),
>>> 4/5 and 5/5 is not applied, though.
>>>
>>>>   	if (strategy_options.nr) {
>>>>   		int i;
>>>> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
>>>> index 50e7960702..55ca46786d 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/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
>>>> new file mode 100755
>>>> index 0000000000..4f8a6e51c9
>>>> --- /dev/null
>>>> +++ b/t/t3436-rebase-more-options.sh
>>>> @@ -0,0 +1,60 @@
>>>> +#!/bin/sh
>>>> +#
>>>> +# Copyright (c) 2019 Rohit Ashiwal
>>>> +#
>>>> +
>>>> +test_description='tests to ensure compatibility between am and interactive backends'
>>>> +
>>>> +. ./test-lib.sh
>>>> +
>>>> +. "$TEST_DIRECTORY"/lib-rebase.sh
>>>> +
>>>> +# This is a special case in which both am and interactive backends
>>>> +# provide the same output. It was done intentionally because
>>>> +# both the backends fall short of optimal behaviour.
>>>> +test_expect_success 'setup' '
>>>> +	git checkout -b topic &&
>>>> +	test_write_lines "line 1" "	line 2" "line 3" >file &&
>>>> +	git add file &&
>>>> +	git commit -m "add file" &&
>>>> +
>>>> +	test_write_lines "line 1" "new line 2" "line 3" >file &&
>>>> +	git commit -am "update file" &&
>>>> +	git tag side &&
>>>> +
>>>> +	git checkout --orphan master &&
>>>> +	test_write_lines "line 1" "        line 2" "line 3" >file &&
>>>> +	git commit -am "add file" &&
>>>> +	git tag main
>>>> +'
>>>> +
>>>> +test_expect_success '--ignore-whitespace works with apply backend' '
>>>> +	test_must_fail git rebase --apply main side &&
>>>> +	git rebase --abort &&
>>>> +	git rebase --apply --ignore-whitespace main side &&
>>>> +	git diff --exit-code side
>>>> +'
>>>> +
>>>> +test_expect_success '--ignore-whitespace works with merge backend' '
>>>> +	test_must_fail git rebase --merge main side &&
>>>> +	git rebase --abort &&
>>>> +	git rebase --merge --ignore-whitespace main side &&
>>>> +	git diff --exit-code side
>>>> +'
>>>> +
>>>> +test_expect_success '--ignore-whitespace is remembered when continuing' '
>>>> +	(
>>>> +		set_fake_editor &&
>>>> +		FAKE_LINES="break 1" git rebase -i --ignore-whitespace \
>>>> +			main side &&
>>>> +		git rebase --continue
>>>> +	) &&
>>>> +	git diff --exit-code side
>>>> +'
>>>> +
>>>> +# This must be the last test in this file
>>>> +test_expect_success '$EDITOR and friends are unchanged' '
>>>> +	test_editor_unchanged
>>>> +'
>>>> +
>>>> +test_done
>>>> -- 
>>>> 2.27.0
>>>>
>>>
>

Patch
diff mbox series

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f7a6033607..b003784f01 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -422,8 +422,23 @@  your branch contains commits which were dropped, this option can be used
 with `--keep-base` in order to drop those commits from your branch.
 
 --ignore-whitespace::
+	Ignore whitespace differences when trying to reconcile
+differences. Currently, each backend implements an approximation of
+this behavior:
++
+apply backend: When applying a patch, ignore changes in whitespace in
+context lines. Unfortunately, this means that if the "old" lines being
+replaced by the patch differ only in whitespace from the existing
+file, you will get a merge conflict instead of a successful patch
+application.
++
+merge backend: Treat lines with only whitespace changes as unchanged
+when merging. Unfortunately, this means that any patch hunks that were
+intended to modify whitespace and nothing else will be dropped, even
+if the other side had no changes that conflicted.
+
 --whitespace=<option>::
-	These flags 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.
 	Implies --apply.
 +
@@ -572,7 +587,6 @@  The following options:
  * --apply
  * --committer-date-is-author-date
  * --ignore-date
- * --ignore-whitespace
  * --whitespace
  * -C
 
@@ -598,6 +612,7 @@  In addition, the following pairs of options are incompatible:
  * --preserve-merges and --signoff
  * --preserve-merges and --rebase-merges
  * --preserve-merges and --empty=
+ * --preserve-merges and --ignore-whitespace
  * --keep-base and --onto
  * --keep-base and --root
 
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 27a07d4e78..810c9b7779 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -126,6 +126,7 @@  static struct replay_opts get_replay_opts(const struct rebase_options *opts)
 	replay.reschedule_failed_exec = opts->reschedule_failed_exec;
 	replay.gpg_sign = xstrdup_or_null(opts->gpg_sign_opt);
 	replay.strategy = opts->strategy;
+
 	if (opts->strategy_opts)
 		parse_strategy_opts(&replay, opts->strategy_opts);
 
@@ -1466,6 +1467,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	struct strbuf revisions = STRBUF_INIT;
 	struct strbuf buf = STRBUF_INIT;
 	struct object_id merge_base;
+	int ignore_whitespace = 0;
 	enum action action = ACTION_NONE;
 	const char *gpg_sign = NULL;
 	struct string_list exec = STRING_LIST_INIT_NODUP;
@@ -1495,16 +1497,15 @@  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),
 		OPT_PASSTHRU_ARGV(0, "ignore-date", &options.git_am_opts, NULL,
 				  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", &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,
@@ -1850,6 +1851,15 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 		imply_merge(&options, "--rebase-merges");
 	}
 
+	if (options.type == REBASE_APPLY) {
+		if (ignore_whitespace)
+			argv_array_push (&options.git_am_opts,
+					 "--ignore-whitespace");
+	} else if (ignore_whitespace) {
+			string_list_append (&strategy_options,
+					    "ignore-space-change");
+	}
+
 	if (strategy_options.nr) {
 		int i;
 
diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
index 50e7960702..55ca46786d 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/t3436-rebase-more-options.sh b/t/t3436-rebase-more-options.sh
new file mode 100755
index 0000000000..4f8a6e51c9
--- /dev/null
+++ b/t/t3436-rebase-more-options.sh
@@ -0,0 +1,60 @@ 
+#!/bin/sh
+#
+# Copyright (c) 2019 Rohit Ashiwal
+#
+
+test_description='tests to ensure compatibility between am and interactive backends'
+
+. ./test-lib.sh
+
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+# This is a special case in which both am and interactive backends
+# provide the same output. It was done intentionally because
+# both the backends fall short of optimal behaviour.
+test_expect_success 'setup' '
+	git checkout -b topic &&
+	test_write_lines "line 1" "	line 2" "line 3" >file &&
+	git add file &&
+	git commit -m "add file" &&
+
+	test_write_lines "line 1" "new line 2" "line 3" >file &&
+	git commit -am "update file" &&
+	git tag side &&
+
+	git checkout --orphan master &&
+	test_write_lines "line 1" "        line 2" "line 3" >file &&
+	git commit -am "add file" &&
+	git tag main
+'
+
+test_expect_success '--ignore-whitespace works with apply backend' '
+	test_must_fail git rebase --apply main side &&
+	git rebase --abort &&
+	git rebase --apply --ignore-whitespace main side &&
+	git diff --exit-code side
+'
+
+test_expect_success '--ignore-whitespace works with merge backend' '
+	test_must_fail git rebase --merge main side &&
+	git rebase --abort &&
+	git rebase --merge --ignore-whitespace main side &&
+	git diff --exit-code side
+'
+
+test_expect_success '--ignore-whitespace is remembered when continuing' '
+	(
+		set_fake_editor &&
+		FAKE_LINES="break 1" git rebase -i --ignore-whitespace \
+			main side &&
+		git rebase --continue
+	) &&
+	git diff --exit-code side
+'
+
+# This must be the last test in this file
+test_expect_success '$EDITOR and friends are unchanged' '
+	test_editor_unchanged
+'
+
+test_done