Re: [PATCH v5 1/5] rebase -i: add --ignore-whitespace flag

[Junio C Hamano <gitster@pobox.com>]

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.