| Message ID | 20200527173356.47364-2-phillip.wood123@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cleanup ra/rebase-i-more-options | expand |
Hi Phillip, sorry to be _so_ late in the game. (And sorry for sending this to you twice, I managed to skip all the Cc:s due to the Reply-To: header the first time round.) On Wed, 27 May 2020, Phillip Wood 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 As a non-native speaker, I am thrown off when reading "available to" instead of the grammatically correct (I believe) "available in". Likewise, "on going" instead of "ongoing" just disrupts my workflow. Maybe these can be fixed? > 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 | 19 +++++- > builtin/rebase.c | 19 ++++-- > t/t3422-rebase-incompatible-options.sh | 1 - > t/t3436-rebase-more-options.sh | 86 ++++++++++++++++++++++++++ > 4 files changed, 118 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..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..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); Quite honestly, this is very, very ugly. I would have expected this at a way earlier layer, namely in `cmd__rebase()`. Something along these lines: -- snip -- diff --git a/builtin/rebase.c b/builtin/rebase.c index 37ba76ac3d26..748e08aee2f2 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1289,6 +1289,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; @@ -1318,9 +1319,8 @@ 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_BOOL(0, "ignore-whitespace", &ignore_whitespace, + N_("passed to 'git am'")), OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date", &options.git_am_opts, NULL, N_("passed to 'git am'"), PARSE_OPT_NOARG), @@ -1682,6 +1682,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) imply_merge(&options, "--rebase-merges"); } + if (ignore_whitespace) { + if (options.type == REBASE_APPLY) + argv_array_push(&options.git_am_opts, + "--ignore-whitespace"); + else + string_list_append(&stragey_options, + "--ignore-space-change"); + } + if (strategy_options.nr) { int i; -- snap -- > > 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); I am not sure what this is about: `opts.strategy_opts` is of type `char *`, i.e. it is supposed to be already allocated. Not that `cmd_rebase__interactive()` matters _all_ that much anymore, of course: it is only used by the --preserve-merges backend, which will hopefully be retired soon. > + > 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 > +' The file contents are repeated in an awfully repetitive manner. That not only makes things a bit hard to read, it also makes it all too easy to slip in bugs by mistake. How about something like this instead? test_commit file && test_write_lines line1 Qline2 line3 >templ && q_to_tab <templ >file.t && git commit -m tab file.t && sed "s/Q/new /" <templ >file.t && git commit -m new file.t && git tag side && git checkout file -- && sed "s/Q/ /" <templ >file.t && git commit -m spaces file.t ... and then... > + > +test_expect_success '--ignore-whitespace works with apply backend' ' > + cat >expect <<-\EOF && > + line 1 > + new line 2 > + line 3 > + EOF sed "s/Q/new /" <templ >expect > + test_must_fail git rebase --apply main side && > + git rebase --abort && > + git rebase --apply --ignore-whitespace main side && > + test_cmp expect file Personally, I prefer to read the contents of `expect` directly before the `test_cmp expect file.t` > +' > + > +test_expect_success '--ignore-whitespace works with merge backend' ' > + cat >expect <<-\EOF && > + line 1 > + new line 2 > + line 3 > + EOF Isn't this totally identical to the `expect` constructed earlier? And in any case, isn't this identical to `git show main:file.t`, which is what we _actually_ expect: for the file contents to be identical to the tagged `main`? I.e. git diff --exit-code main > + 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 It is a bit funny to see these two invocations _specifically_ pulled out from the subshell, that's not how we do things in other test scripts: instead, we run all the Git commands _inside_ the subshell, and all the verifications after the subshell. I believe that with my suggestions, this test script will be a ton easier to read and to maintain. At least it will be a lot DRYer. Ciao, Dscho > +' > + > +# 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 > >
As a non-native speaker, I am thrown off when reading "available to" instead of the grammatically correct (I believe) "available in". Likewise, "on going" instead of "ongoing" just disrupts my workflow. Maybe these can be fixed? [RK] There is a distinct difference between "available to" and "available in". [RK] "service A is available to entity B" does not mean the same as "service A is available in entity B". The former indicates that service A is implemented within entity B, the latter indicates merely that entity B uses service A. [RK] As a real world example consider "The internet banking service of Bank X is available to me", which makes complete sense. But "The internet banking service of Bank X is available in me" would be nonsense. I don't know what the OP intended but the difference between these is not purely grammatical, there is a difference in meaning which I must leave the OP to clarify. [RK] As for the other issue, I too would suggest "ongoing". [RK] Regards, [RK] Richard.
Hi dscho Thanks for taking a look at this On 29/05/2020 03:38, Johannes Schindelin wrote: > Hi Phillip, > > sorry to be _so_ late in the game. (And sorry for sending this to you > twice, I managed to skip all the Cc:s due to the Reply-To: header the > first time round.) > > On Wed, 27 May 2020, Phillip Wood 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 > > As a non-native speaker, I am thrown off when reading "available to" > instead of the grammatically correct (I believe) "available in". Likewise, > "on going" instead of "ongoing" just disrupts my workflow. > > Maybe these can be fixed? Sure >> 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); > > Quite honestly, this is very, very ugly. > > I would have expected this at a way earlier layer, namely in > `cmd__rebase()`. Something along these lines: Yes your version is definitely better, I'll update with that (I had left this patch mostly alone as unlike the later ones the original actually worked) > > -- snip -- > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 37ba76ac3d26..748e08aee2f2 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1289,6 +1289,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; > @@ -1318,9 +1319,8 @@ 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_BOOL(0, "ignore-whitespace", &ignore_whitespace, > + N_("passed to 'git am'")), > OPT_PASSTHRU_ARGV(0, "committer-date-is-author-date", > &options.git_am_opts, NULL, > N_("passed to 'git am'"), PARSE_OPT_NOARG), > @@ -1682,6 +1682,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > imply_merge(&options, "--rebase-merges"); > } > > + if (ignore_whitespace) { > + if (options.type == REBASE_APPLY) > + argv_array_push(&options.git_am_opts, > + "--ignore-whitespace"); > + else > + string_list_append(&stragey_options, > + "--ignore-space-change"); > + } > + > if (strategy_options.nr) { > int i; > > -- snap -- > > >> >> 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); > > I am not sure what this is about: `opts.strategy_opts` is of type `char > *`, i.e. it is supposed to be already allocated. > > Not that `cmd_rebase__interactive()` matters _all_ that much anymore, of > course: it is only used by the --preserve-merges backend, which will > hopefully be retired soon. > >> + >> 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 >> +' > > The file contents are repeated in an awfully repetitive manner. That not > only makes things a bit hard to read, it also makes it all too easy to > slip in bugs by mistake. How about something like this instead? > > test_commit file && > > test_write_lines line1 Qline2 line3 >templ && > > q_to_tab <templ >file.t && > git commit -m tab file.t && > > sed "s/Q/new /" <templ >file.t && > git commit -m new file.t && > git tag side && > > git checkout file -- && > sed "s/Q/ /" <templ >file.t && > git commit -m spaces file.t I'm not totally convinced by this, I'd prefer to be able to read the contents rather than having to work out what sed is doing. What about test_write_lines "line 1" " line 2" "line 3" >file && add and commit test_write_lines "line 1" "new line 2" "line 3" >file && commit and tag test_write_lines "line 1" " line 2" "line 3" >file && commit and tag It does not get rid of the repetition but it does dispense with having the work out what sed and q_to_tab are doing > ... and then... > >> + >> +test_expect_success '--ignore-whitespace works with apply backend' ' >> + cat >expect <<-\EOF && >> + line 1 >> + new line 2 >> + line 3 >> + EOF > > sed "s/Q/new /" <templ >expect > >> + test_must_fail git rebase --apply main side && >> + git rebase --abort && >> + git rebase --apply --ignore-whitespace main side && >> + test_cmp expect file > > Personally, I prefer to read the contents of `expect` directly before the > `test_cmp expect file.t` I can move it but if we use sed you cannot see the contents >> +' >> + >> +test_expect_success '--ignore-whitespace works with merge backend' ' >> + cat >expect <<-\EOF && >> + line 1 >> + new line 2 >> + line 3 >> + EOF > > Isn't this totally identical to the `expect` constructed earlier? And in > any case, isn't this identical to `git show main:file.t`, which is what we > _actually_ expect: for the file contents to be identical to the tagged > `main`? I.e. Well spotted I'll update (I think it's actually the same as side) > git diff --exit-code main > >> + 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 > > It is a bit funny to see these two invocations _specifically_ pulled out > from the subshell, that's not how we do things in other test scripts: > instead, we run all the Git commands _inside_ the subshell, and all the > verifications after the subshell. The idea was to only set the variable where it is used. Best Wishes Phillip > I believe that with my suggestions, this test script will be a ton easier > to read and to maintain. At least it will be a lot DRYer. > > Ciao, > Dscho > >> +' >> + >> +# 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 >> >>
Hi Phillip, On Mon, 1 Jun 2020, Phillip Wood wrote: > On 29/05/2020 03:38, Johannes Schindelin wrote: > > > > On Wed, 27 May 2020, Phillip Wood wrote: > > > >> From: Rohit Ashiwal <rohit.ashiwal265@gmail.com> > >> > >> [...] > >> 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 > >> +' > > > > The file contents are repeated in an awfully repetitive manner. That not > > only makes things a bit hard to read, it also makes it all too easy to > > slip in bugs by mistake. How about something like this instead? > > > > test_commit file && > > > > test_write_lines line1 Qline2 line3 >templ && > > > > q_to_tab <templ >file.t && > > git commit -m tab file.t && > > > > sed "s/Q/new /" <templ >file.t && > > git commit -m new file.t && > > git tag side && > > > > git checkout file -- && > > sed "s/Q/ /" <templ >file.t && > > git commit -m spaces file.t > > I'm not totally convinced by this, I'd prefer to be able to read the > contents rather than having to work out what sed is doing. What about > > test_write_lines "line 1" " line 2" "line 3" >file && > add and commit > test_write_lines "line 1" "new line 2" "line 3" >file && > commit and tag > test_write_lines "line 1" " line 2" "line 3" >file && > commit and tag > > It does not get rid of the repetition but it does dispense with having > the work out what sed and q_to_tab are doing Sure. Your version is still tons easier to parse for a human being. > > git diff --exit-code main > > > >> + 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 > > > > It is a bit funny to see these two invocations _specifically_ pulled out > > from the subshell, that's not how we do things in other test scripts: > > instead, we run all the Git commands _inside_ the subshell, and all the > > verifications after the subshell. > > The idea was to only set the variable where it is used. I understand that, but I think that it would be better to follow the existing pattern rather than introducing an inconsistent second one. Thanks, Dscho
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..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