git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
May 29, 2020, 02:38 UTC
Message-ID
<nycvar.QRO.7.76.6.2005290437350.56@tvgsbejvaqbjf.bet>
In-Reply-To
<20200527173356.47364-2-phillip.wood123@gmail.com>
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:
Show 6 quoted lines
> 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?
Show 92 quoted lines
> 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
>
>
Previous: Phillip WoodNext: Kerry, Richard
Message 58 of 130 in “fixup ra/rebase-i-more-options”
  1. 0/6 fixup ra/rebase-i-more-optionsPhillip Wood, Apr 7, 2020
  2. 2/6 t3433: remove loops from testsPhillip Wood, Apr 7, 2020
  3. Elijah NewrenApr 7, 2020
  4. 1/6 Revert "Revert "Merge branch 'ra/rebase-i-more-options'""Phillip Wood, Apr 7, 2020
  5. Elijah NewrenApr 7, 2020
  6. Phillip WoodApr 7, 2020
  7. Junio C HamanoApr 7, 2020
  8. Junio C HamanoApr 7, 2020
  9. Johannes SchindelinApr 12, 2020
  10. Phillip WoodApr 13, 2020
  11. Junio C HamanoApr 13, 2020
  12. 3/6 t3433: only compare commit datesPhillip Wood, Apr 7, 2020
  13. 4/6 rebase -i: fix --committer-date-is-author-datePhillip Wood, Apr 7, 2020
  14. Elijah NewrenApr 7, 2020
  15. Phillip WoodApr 7, 2020
  16. 5/6 Revert "sequencer: allow callers of read_author_script() to ignore fields"Phillip Wood, Apr 7, 2020
  17. Elijah NewrenApr 7, 2020
  18. 6/6 t3433: improve coveragePhillip Wood, Apr 7, 2020
  19. Elijah NewrenApr 7, 2020
  20. Phillip WoodApr 7, 2020
  21. Elijah NewrenApr 7, 2020
  22. Phillip WoodApr 7, 2020
  23. Junio C HamanoApr 7, 2020
  24. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, Apr 29, 2020
  25. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, Apr 29, 2020
  26. Elijah NewrenMay 13, 2020
  27. Phillip WoodMay 14, 2020
  28. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, Apr 29, 2020
  29. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, Apr 29, 2020
  30. Alban GruinMay 10, 2020
  31. Elijah NewrenMay 13, 2020
  32. 5/5 rebase: add --reset-author-datePhillip Wood, Apr 29, 2020
  33. 4/5 rebase -i: support --ignore-datePhillip Wood, Apr 29, 2020
  34. Alban GruinMay 10, 2020
  35. Phillip WoodMay 12, 2020
  36. Junio C HamanoMay 13, 2020
  37. Elijah NewrenMay 13, 2020
  38. Junio C HamanoApr 29, 2020
  39. Elijah NewrenMay 13, 2020
  40. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, May 21, 2020
  41. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, May 21, 2020
  42. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, May 21, 2020
  43. 5/5 rebase: add --reset-author-datePhillip Wood, May 21, 2020
  44. 4/5 rebase -i: support --ignore-datePhillip Wood, May 21, 2020
  45. Đoàn Trần Công DanhMay 23, 2020
  46. Phillip WoodMay 23, 2020
  47. Đoàn Trần Công DanhMay 23, 2020
  48. Phillip WoodMay 23, 2020
  49. Đoàn Trần Công DanhMay 23, 2020
  50. Phillip WoodMay 27, 2020
  51. Junio C HamanoMay 24, 2020
  52. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, May 21, 2020
  53. Elijah NewrenMay 22, 2020
  54. Phillip WoodMay 23, 2020
  55. Johannes SchindelinMay 23, 2020
  56. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, May 27, 2020
  57. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, May 27, 2020
  58. Johannes SchindelinMay 29, 2020
  59. Kerry, RichardJun 1, 2020
  60. Phillip WoodJun 1, 2020
  61. Johannes SchindelinJun 1, 2020
  62. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, May 27, 2020
  63. Johannes SchindelinMay 29, 2020
  64. Phillip WoodJun 1, 2020
  65. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, May 27, 2020
  66. 4/5 rebase -i: support --ignore-datePhillip Wood, May 27, 2020
  67. 5/5 rebase: add --reset-author-datePhillip Wood, May 27, 2020
  68. 6/5 fixup! rebase: add --reset-author-datePhillip Wood, May 27, 2020
  69. Đoàn Trần Công DanhMay 28, 2020
  70. Johannes SchindelinMay 29, 2020
  71. Phillip WoodJun 1, 2020
  72. Junio C HamanoMay 27, 2020
  73. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, Jun 26, 2020
  74. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, Jun 26, 2020
  75. Đoàn Trần Công DanhJun 26, 2020
  76. Phillip WoodJun 26, 2020
  77. Junio C HamanoJun 26, 2020
  78. Đoàn Trần Công DanhJun 29, 2020
  79. Phillip WoodJul 13, 2020
  80. Junio C HamanoJun 26, 2020
  81. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, Jun 26, 2020
  82. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, Jun 26, 2020
  83. 4/5 rebase -i: support --ignore-datePhillip Wood, Jun 26, 2020
  84. Đoàn Trần Công DanhJun 26, 2020
  85. Phillip WoodJun 26, 2020
  86. Junio C HamanoJun 26, 2020
  87. 5/5 rebase: add --reset-author-datePhillip Wood, Jun 26, 2020
  88. Junio C HamanoJun 26, 2020
  89. Phillip WoodJun 26, 2020
  90. Junio C HamanoJun 26, 2020
  91. 0/5 fixup ra/rebase-i-more-optionsPhillip Wood, Jul 13, 2020
  92. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, Jul 13, 2020
  93. Đoàn Trần Công DanhJul 15, 2020
  94. Phillip WoodJul 16, 2020
  95. Đoàn Trần Công DanhJul 16, 2020
  96. Phillip WoodJul 16, 2020
  97. Phillip WoodJul 16, 2020
  98. Đoàn Trần Công DanhJul 17, 2020
  99. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, Jul 13, 2020
  100. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, Jul 13, 2020
  101. 4/5 rebase -i: support --ignore-datePhillip Wood, Jul 13, 2020
  102. 5/5 rebase: add --reset-author-datePhillip Wood, Jul 13, 2020
  103. Junio C HamanoJul 13, 2020
  104. Phillip WoodJul 15, 2020
  105. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, Jul 16, 2020
  106. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, Jul 16, 2020
  107. 2/5 rebase -i: support --committer-date-is-author-datePhillip Wood, Jul 16, 2020
  108. Johannes SchindelinAug 13, 2020
  109. 3/5 sequencer: rename amend_author to author_to_freePhillip Wood, Jul 16, 2020
  110. 5/5 rebase: add --reset-author-datePhillip Wood, Jul 16, 2020
  111. 4/5 rebase -i: support --ignore-datePhillip Wood, Jul 16, 2020
  112. Johannes SchindelinAug 13, 2020
  113. Junio C HamanoJul 16, 2020
  114. Johannes SchindelinAug 13, 2020
  115. Junio C HamanoAug 13, 2020
  116. Phillip WoodAug 13, 2020
  117. 0/5 cleanup ra/rebase-i-more-optionsPhillip Wood, Aug 17, 2020
  118. 2/5 am: stop exporting GIT_COMMITTER_DATEPhillip Wood, Aug 17, 2020
  119. Junio C HamanoAug 17, 2020
  120. Phillip WoodAug 19, 2020
  121. Junio C HamanoAug 19, 2020
  122. Phillip WoodAug 20, 2020
  123. 4/5 rebase -i: support --ignore-datePhillip Wood, Aug 17, 2020
  124. Phillip WoodAug 19, 2020
  125. Junio C HamanoAug 19, 2020
  126. Phillip WoodAug 20, 2020
  127. 5/5 rebase: add --reset-author-datePhillip Wood, Aug 17, 2020
  128. 3/5 rebase -i: support --committer-date-is-author-datePhillip Wood, Aug 17, 2020
  129. Junio C HamanoAug 26, 2020
  130. 1/5 rebase -i: add --ignore-whitespace flagPhillip Wood, Aug 17, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.