On 13/05/2020 04:54, Elijah Newren wrote:
> Sorry for taking so long to get back to you, and thanks for pushing
> this forward.
Thanks for taking a look at this series
>
> On Wed, Apr 29, 2020 at 3:26 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>>
>> From: Rohit Ashiwal <rohit.ashiwal265@xxxxxxxxx>
>>
>> Rebase is implemented with two different backends - 'apply' and 'merge'
>> each of which support a different set of options. In particuar the apply
>> backend supports a number of options implemented by 'git am' that are
>> not available to the merge backend. As part of an on going effort to
>> remove the apply backend 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@xxxxxxxxx>
>> Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
>> [...]
>> ---
>> Documentation/git-rebase.txt | 12 +++-
>> builtin/rebase.c | 19 ++++--
>> t/t3422-rebase-incompatible-options.sh | 1 -
>> t/t3436-rebase-more-options.sh | 86 ++++++++++++++++++++++++++
>> 4 files changed, 111 insertions(+), 7 deletions(-)
>> create mode 100755 t/t3436-rebase-more-options.sh
>>
>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>> index f7a6033607..d060c143e6 100644
>> --- a/Documentation/git-rebase.txt
>> +++ b/Documentation/git-rebase.txt
>> @@ -422,8 +422,16 @@ 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::
>> + Behaves differently depending on which backend is selected.
>
> I still don't like this wording; it defers answering the question,
> implies that the difference is intentional, and most importantly
> provides no context about *intended* behavior. I tried to communicate
> this to Rohit multiple times, but he seemed to fixate on and highlight
> the differences in a way that made them sound like they were by
> design, rather than highlighting the intent we want to move towards
> and mentioning that this patch gets us most the way there.
>
> As far as I can tell, the --ignore-whitespace and
> -Xignore-space-change were always meant to do the same thing: ignore
> differences in whitespace when doing so can avoid conflicts.
>
> In case anyone isn't sure about my assertion that these were always
> meant to do the same thing:
> * apply aliases --ignore-whitespace and --ignore-space-change; they
> meant the same thing
> * commit f008cef4ab ("Merge branch 'jc/apply-ignore-whitespace'",
> 2014-06-03) says that apply's --ignore-space-change wasn't behaving
> consistently with diff's --ignore-space-change
> * diff's --ignore-space-change goes through xdiff's XDL opts, much
> like merge-recursive does.
> Further, the original commit that introduced these xdiff options to
> merge-recursive, 4e5dd044c6 ("merge-recursive: options to ignore
> whitespace changes", 2010-08-26), it is clear that:
> * he only cared about ignore-space-at-eol and implemented
> ignore-space-change at the same time only for completeness
> * it wouldn't matter to his usecase if whitespace-only changes were
> stripped, thus he wouldn't have spotted the bug it has
> * the wording also suggests these options were picked to match
> options of the same name elsewhere in git
>
> I would rather we said something like:
> 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.
>
> Maybe add something like:
> (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.
>
> Maybe add something like:
> (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.)
Thanks for the suggestions, I'll incorporate them into the re-roll.
Best Wishes
Phillip
>> +
>> --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 +580,6 @@ The following options:
>> * --apply
>> * --committer-date-is-author-date
>> * --ignore-date
>> - * --ignore-whitespace
>> * --whitespace
>> * -C
>>
>> @@ -598,6 +605,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..5d8e117276 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -86,6 +86,7 @@ struct rebase_options {
>> int signoff;
>> int allow_rerere_autoupdate;
>> int autosquash;
>> + int ignore_whitespace;
>> char *gpg_sign_opt;
>> int autostash;
>> char *cmd;
>> @@ -108,6 +109,7 @@ struct rebase_options {
>>
>> static struct replay_opts get_replay_opts(const struct rebase_options *opts)
>> {
>> + struct strbuf strategy_buf = STRBUF_INIT;
>> struct replay_opts replay = REPLAY_OPTS_INIT;
>>
>> replay.action = REPLAY_INTERACTIVE_REBASE;
>> @@ -126,14 +128,20 @@ 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);
>> + strbuf_addstr(&strategy_buf, opts->strategy_opts);
>> + if (opts->ignore_whitespace)
>> + strbuf_addstr(&strategy_buf, " --ignore-space-change");
>> + if (strategy_buf.len)
>> + parse_strategy_opts(&replay, strategy_buf.buf);
>>
>> if (opts->squash_onto) {
>> oidcpy(&replay.squash_onto, opts->squash_onto);
>> replay.have_squash_onto = 1;
>> }
>>
>> + strbuf_release(&strategy_buf);
>> return replay;
>> }
>>
>> @@ -539,6 +547,8 @@ int cmd_rebase__interactive(int argc, const char **argv, const char *prefix)
>> argc = parse_options(argc, argv, prefix, 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;
>>
>> @@ -991,6 +1001,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);
>> @@ -1495,16 +1507,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", &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,
>> 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..fb5e747e86
>> --- /dev/null
>> +++ b/t/t3436-rebase-more-options.sh
>> @@ -0,0 +1,86 @@
>> +#!/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 &&
>> + q_to_tab >file <<-\EOF &&
>> + line 1
>> + Qline 2
>> + line 3
>> + EOF
>> + git add file &&
>> + git commit -m "add file" &&
>> + cat >file <<-\EOF &&
>> + line 1
>> + new line 2
>> + line 3
>> + EOF
>> + git commit -am "update file" &&
>> + git tag side &&
>> +
>> + git checkout --orphan master &&
>> + sed -e "s/^|//" >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 apply backend' '
>> + cat >expect <<-\EOF &&
>> + line 1
>> + new line 2
>> + line 3
>> + EOF
>> + test_must_fail git rebase --apply main side &&
>> + git rebase --abort &&
>> + git rebase --apply --ignore-whitespace main side &&
>> + test_cmp expect file
>> +'
>> +
>> +test_expect_success '--ignore-whitespace works with merge 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_expect_success '--ignore-whitespace is remembered when continuing' '
>> + cat >expect <<-\EOF &&
>> + line 1
>> + new line 2
>> + line 3
>> + EOF
>> + (
>> + set_fake_editor &&
>> + FAKE_LINES="break 1" git rebase -i --ignore-whitespace main side
>> + ) &&
>> + git rebase --continue &&
>> + test_cmp expect file
>> +'
>> +
>> +# This must be the last test in this file
>> +test_expect_success '$EDITOR and friends are unchanged' '
>> + test_editor_unchanged
>> +'
>> +
>> +test_done
>> --
>> 2.26.2
>
> The rest looks good to me.
>
