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

[Phillip Wood <phillip.wood123@gmail.com>]

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@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 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@gmail.com>
>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
>> [...]

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