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

Re: [RFC PATCH 2/3] rebase: Implement --merge via git-rebase--interactive

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Jun 9, 2018, 22:04 UTC
Message-ID
<nycvar.QRO.7.76.6.1806092348390.77@tvgsbejvaqbjf.bet>
In-Reply-To
<20180607171344.23331-3-newren@gmail.com>
Hi Elijah,
On Thu, 7 Jun 2018, Elijah Newren wrote:
Show 12 quoted lines
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 451252c173..28d1658d7a 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -275,6 +275,10 @@ branch on top of the <upstream> branch.  Because of this, when a merge
>  conflict happens, the side reported as 'ours' is the so-far rebased
>  series, starting with <upstream>, and 'theirs' is the working branch.  In
>  other words, the sides are swapped.
> ++
> +This uses the `--interactive` machinery internally, and as such,
> +anything that is incompatible with --interactive is incompatible
> +with this option.

I am not sure I like this change, as it describes an implementation detail that users should neither know, nor care about, nor rely on.

Show 31 quoted lines
> @@ -328,8 +332,8 @@ which makes little sense.
>  	and after each change.  When fewer lines of surrounding
>  	context exist they all must match.  By default no context is
>  	ever ignored.
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  -f::
>  --force-rebase::
> @@ -361,15 +365,15 @@ default is `--no-fork-point`, otherwise the default is `--fork-point`.
>  --whitespace=<option>::
>  	These flag are passed to the 'git apply' program
>  	(see linkgit:git-apply[1]) that applies the patch.
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  --committer-date-is-author-date::
>  --ignore-date::
>  	These flags are passed to 'git am' to easily change the dates
>  	of the rebased commits (see linkgit:git-am[1]).
> -	Incompatible with the --merge and --interactive options, or
> -	anything that implies those options or their machinery.
> +	Incompatible with the --interactive option, or anything that
> +	uses the `--interactive` machinery.
>  
>  --signoff::
>  	Add a Signed-off-by: trailer to all the rebased commits. Note
For above-mentioned reason, I'd suggest dropping these three hunks.
Show 13 quoted lines
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 1f2401f702..dcc4a26a78 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -885,7 +885,10 @@ init_basic_state () {
>  	mkdir -p "$state_dir" || die "$(eval_gettext "Could not create temporary \$state_dir")"
>  	rm -f "$(git rev-parse --git-path REBASE_HEAD)"
>  
> -	: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
> +	if test -n "$actually_interactive"
> +	then
> +		: > "$state_dir"/interactive || die "$(gettext "Could not mark as interactive")"
> +	fi

Do we really care at this stage? IOW what breaks if we still write that file, even in --merge mode?

Show 18 quoted lines
> diff --git a/git-rebase.sh b/git-rebase.sh
> index b639c0d4fe..5ac1dee30b 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -239,12 +239,10 @@ then
>  	state_dir="$apply_dir"
>  elif test -d "$merge_dir"
>  then
> +	type=interactive
>  	if test -f "$merge_dir"/interactive
>  	then
> -		type=interactive
>  		interactive_rebase=explicit
> -	else
> -		type=merge
>  	fi
>  	state_dir="$merge_dir"
>  fi
This makes me think even more that we don't care...
Show 24 quoted lines
> @@ -481,13 +479,16 @@ then
>  	test -z "$interactive_rebase" && interactive_rebase=implied
>  fi
>  
> +actually_interactive=
>  if test -n "$interactive_rebase"
>  then
>  	type=interactive
> +	actually_interactive=t
>  	state_dir="$merge_dir"
>  elif test -n "$do_merge"
>  then
> -	type=merge
> +	interactive_rebase=implied
> +	type=interactive
>  	state_dir="$merge_dir"
>  else
>  	type=am
> @@ -501,17 +502,11 @@ fi
>  
>  if test -n "$git_am_opt"; then
>  	incompatible_opts=`echo "$git_am_opt" | sed -e 's/ -q//'`
> -	if test -n "$interactive_rebase"
> +	if test -n "$incompatible_opts"

Did you not mean to turn this into a test for actually_interactve || do_merge?

>  	then
> -		if test -n "$incompatible_opts"
> +		if test -n "$actually_interactive" || test "$do_merge"

This could now be combined with the previous if (and the `-n` could be added to the latter test):

	if test -n "$actually_interactive" -o -n "$do_merge" &&
		test -n "$incompatible_opts"

The indentation would change, but this hunk is already confusing to read, anyway, so...

Show 31 quoted lines
>  		then
> -			die "$(gettext "error: cannot combine interactive options (--interactive, --exec, --rebase-merges, --preserve-merges, --keep-empty, --root + --onto) with am options ($incompatible_opts)")"
> -		fi
> -	fi
> -	if test -n "$do_merge"; then
> -		if test -n "$incompatible_opts"
> -		then
> -			die "$(gettext "error: cannot combine merge options (--merge, --strategy, --strategy-option) with am options ($incompatible_opts)")"
> +			die "$(gettext "error: cannot combine am options ($incompatible_opts) with either interactive or merge options")"
>  		fi
>  	fi
>  fi
> @@ -660,7 +655,7 @@ require_clean_work_tree "rebase" "$(gettext "Please commit or stash them.")"
>  # but this should be done only when upstream and onto are the same
>  # and if this is not an interactive rebase.
>  mb=$(git merge-base "$onto" "$orig_head")
> -if test "$type" != interactive && test "$upstream" = "$onto" &&
> +if test -z "$actually_interactive" && test "$upstream" = "$onto" &&
>  	test "$mb" = "$onto" && test -z "$restrict_revision" &&
>  	# linear history?
>  	! (git rev-list --parents "$onto".."$orig_head" | sane_grep " .* ") > /dev/null
> @@ -704,6 +699,22 @@ then
>  	GIT_PAGER='' git diff --stat --summary "$mb" "$onto"
>  fi
>  
> +if test -z "$actually_interactive"
> +then
> +	# If the $onto is a proper descendant of the tip of the branch, then
> +	# we just fast-forwarded.
> +	if test "$mb" = "$orig_head"
> +	then
Likewise, this would be easier to read as
	if test -z "$actually_interactive" && test "$mb" = "$orig_head"
Also: how certain are we that "$mb" does not start with a dash? We may
have to use the `test a"$mb" = a"$orig_head"` trick here... But I guess
this is moved code, so if it is buggy, it was buggy before.
Show 50 quoted lines
> +		say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> +		GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
> +			git checkout -q "$onto^0" || die "could not detach HEAD"
> +		git update-ref ORIG_HEAD $orig_head
> +		move_to_original_branch
> +		finish_rebase
> +		exit 0
> +	fi
> +fi
> +
>  test "$type" = interactive && run_specific_rebase
>  
>  # Detach HEAD and reset the tree
> @@ -713,16 +724,6 @@ GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name" \
>  	git checkout -q "$onto^0" || die "could not detach HEAD"
>  git update-ref ORIG_HEAD $orig_head
>  
> -# If the $onto is a proper descendant of the tip of the branch, then
> -# we just fast-forwarded.
> -if test "$mb" = "$orig_head"
> -then
> -	say "$(eval_gettext "Fast-forwarded \$branch_name to \$onto_name.")"
> -	move_to_original_branch
> -	finish_rebase
> -	exit 0
> -fi
> -
>  if test -n "$rebase_root"
>  then
>  	revisions="$onto..$orig_head"
> diff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh
> index 0392e36d23..04d6c71899 100755
> --- a/t/t3406-rebase-message.sh
> +++ b/t/t3406-rebase-message.sh
> @@ -17,14 +17,9 @@ test_expect_success 'setup' '
>  	git tag start
>  '
>  
> -cat >expect <<\EOF
> -Already applied: 0001 A
> -Already applied: 0002 B
> -Committed: 0003 Z
> -EOF
> -
>  test_expect_success 'rebase -m' '
>  	git rebase -m master >report &&
> +	>expect &&
>  	sed -n -e "/^Already applied: /p" \
>  		-e "/^Committed: /p" report >actual &&
>  	test_cmp expect actual

This might be called a regression... I don't know any user of `git rebase -m`, but I guess if I was one, I would like to keep seeing those messages.

It *should* be relatively easy to tell the sequencer.c to issue these messages, I think. But it would be more work.

Show 48 quoted lines
> diff --git a/t/t3421-rebase-topology-linear.sh b/t/t3421-rebase-topology-linear.sh
> index 99b2aac921..911ef49f70 100755
> --- a/t/t3421-rebase-topology-linear.sh
> +++ b/t/t3421-rebase-topology-linear.sh
> @@ -111,7 +111,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -126,7 +126,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -141,7 +141,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -284,7 +284,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase success -p
>  
> @@ -315,7 +315,7 @@ test_run_rebase () {
>  	"
>  }
>  test_run_rebase success ''
> -test_run_rebase failure -m
> +test_run_rebase success -m
>  test_run_rebase success -i
>  test_run_rebase failure -p
Neat!
Show 29 quoted lines
> diff --git a/t/t3425-rebase-topology-merges.sh b/t/t3425-rebase-topology-merges.sh
> index 846f85c27e..cd505c0711 100755
> --- a/t/t3425-rebase-topology-merges.sh
> +++ b/t/t3425-rebase-topology-merges.sh
> @@ -72,7 +72,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'e n o' ''
> -test_run_rebase success 'e n o' -m
> +test_run_rebase success 'n o e' -m
>  test_run_rebase success 'n o e' -i
>  
>  test_run_rebase () {
> @@ -89,7 +89,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'd e n o' ''
> -test_run_rebase success 'd e n o' -m
> +test_run_rebase success 'd n o e' -m
>  test_run_rebase success 'd n o e' -i
>  
>  test_run_rebase () {
> @@ -106,7 +106,7 @@ test_run_rebase () {
>  }
>  #TODO: make order consistent across all flavors of rebase
>  test_run_rebase success 'd e n o' ''
> -test_run_rebase success 'd e n o' -m
> +test_run_rebase success 'd n o e' -m
>  test_run_rebase success 'd n o e' -i
Nice!
Show 13 quoted lines
>  test_expect_success "rebase -p is no-op in non-linear history" "
> diff --git a/t/t5407-post-rewrite-hook.sh b/t/t5407-post-rewrite-hook.sh
> index 9b2a274c71..145c61251d 100755
> --- a/t/t5407-post-rewrite-hook.sh
> +++ b/t/t5407-post-rewrite-hook.sh
> @@ -120,6 +120,7 @@ test_expect_success 'git rebase -m --skip' '
>  	git rebase --continue &&
>  	echo rebase >expected.args &&
>  	cat >expected.data <<-EOF &&
> +	$(git rev-parse C) $(git rev-parse HEAD^)
>  	$(git rev-parse D) $(git rev-parse HEAD)
>  	EOF
>  	verify_hook_input

This is a bit sad, because it seems to suggest that we now do more unnecessary work here, or is my reading incorrect?

Thanks, Dscho

Previous: Elijah NewrenNext: Elijah Newren
Message 126 of 130 in “RFC: rebase inconsistency in 2.18 -- course of action?”
  1. Elijah NewrenJun 7, 2018
  2. t3401: add directory rename testcases for rebase and amElijah Newren, Jun 7, 2018
  3. Elijah NewrenJun 25, 2018
  4. apply: fix grammar error in commentElijah Newren, Jun 7, 2018
  5. t5407: fix test to cover intended argumentsElijah Newren, Jun 7, 2018
  6. git-rebase--merge: modernize "git-$cmd" to "git $cmd"Elijah Newren, Jun 7, 2018
  7. Elijah NewrenJun 7, 2018
  8. git-rebase--merge: modernize "git-$cmd" to "git $cmd"Elijah Newren, Jun 27, 2018
  9. Johannes SchindelinJul 12, 2018
  10. Elijah NewrenJul 13, 2018
  11. Johannes SchindelinJul 14, 2018
  12. 1/2 t3422: new testcases for checking when incompatible options passedElijah Newren, Jun 7, 2018
  13. 2/2 git-rebase: error out when incompatible options passedElijah Newren, Jun 7, 2018
  14. Phillip WoodJun 10, 2018
  15. Elijah NewrenJun 11, 2018
  16. Phillip WoodJun 11, 2018
  17. Elijah NewrenJun 11, 2018
  18. Phillip WoodJun 12, 2018
  19. 0/7 Document/fix/warn about rebase incompatibilities and inconsistencesElijah Newren, Jun 17, 2018
  20. 1/7 git-rebase.txt: document incompatible optionsElijah Newren, Jun 17, 2018
  21. Phillip WoodJun 17, 2018
  22. Elijah NewrenJun 20, 2018
  23. Eric SunshineJun 17, 2018
  24. Elijah NewrenJun 20, 2018
  25. 2/7 git-rebase.sh: update help messages a bitElijah Newren, Jun 17, 2018
  26. 3/7 t3422: new testcases for checking when incompatible options passedElijah Newren, Jun 17, 2018
  27. 4/7 git-rebase: error out when incompatible options passedElijah Newren, Jun 17, 2018
  28. 7/7 git-rebase: make --allow-empty-message the defaultElijah Newren, Jun 17, 2018
  29. Phillip WoodJun 17, 2018
  30. Elijah NewrenJun 20, 2018
  31. 6/7 git-rebase.txt: address confusion between --no-ff vs --force-rebaseElijah Newren, Jun 17, 2018
  32. 5/7 git-rebase.txt: document behavioral inconsistencies between modesElijah Newren, Jun 17, 2018
  33. Phillip WoodJun 17, 2018
  34. 0/7 Document/fix/warn about rebase incompatibilities and inconsistenciesElijah Newren, Jun 21, 2018
  35. 7/7 git-rebase: make --allow-empty-message the defaultElijah Newren, Jun 21, 2018
  36. 3/7 t3422: new testcases for checking when incompatible options passedElijah Newren, Jun 21, 2018
  37. Junio C HamanoJun 21, 2018
  38. Elijah NewrenJun 21, 2018
  39. Eric SunshineJun 21, 2018
  40. 2/7 git-rebase.sh: update help messages a bitElijah Newren, Jun 21, 2018
  41. Junio C HamanoJun 21, 2018
  42. Elijah NewrenJun 21, 2018
  43. 5/7 git-rebase.txt: document behavioral inconsistencies between modesElijah Newren, Jun 21, 2018
  44. Junio C HamanoJun 21, 2018
  45. Elijah NewrenJun 21, 2018
  46. 1/7 git-rebase.txt: document incompatible optionsElijah Newren, Jun 21, 2018
  47. Junio C HamanoJun 21, 2018
  48. Elijah NewrenJun 21, 2018
  49. SZEDER GáborJun 22, 2018
  50. 4/7 git-rebase: error out when incompatible options passedElijah Newren, Jun 21, 2018
  51. Junio C HamanoJun 21, 2018
  52. Elijah NewrenJun 21, 2018
  53. 6/7 git-rebase.txt: address confusion between --no-ff vs --force-rebaseElijah Newren, Jun 21, 2018
  54. Junio C HamanoJun 21, 2018
  55. Elijah NewrenJun 21, 2018
  56. 0/9 Document/fix/warn about rebase incompatibilities and inconsistenciesElijah Newren, Jun 25, 2018
  57. 3/9 t3422: new testcases for checking when incompatible options passedElijah Newren, Jun 25, 2018
  58. Junio C HamanoJun 26, 2018
  59. Elijah NewrenJun 27, 2018
  60. 2/9 git-rebase.sh: update help messages a bitElijah Newren, Jun 25, 2018
  61. 5/9 git-rebase.txt: address confusion between --no-ff vs --force-rebaseElijah Newren, Jun 25, 2018
  62. 9/9 git-rebase: make --allow-empty-message the defaultElijah Newren, Jun 25, 2018
  63. 8/9 t3401: add directory rename testcases for rebase and amElijah Newren, Jun 25, 2018
  64. 7/9 git-rebase.txt: document behavioral differences between modesElijah Newren, Jun 25, 2018
  65. 1/9 git-rebase.txt: document incompatible optionsElijah Newren, Jun 25, 2018
  66. 6/9 directory-rename-detection.txt: technical docs on abilities and limitationsElijah Newren, Jun 25, 2018
  67. 4/9 git-rebase: error out when incompatible options passedElijah Newren, Jun 25, 2018
  68. 0/9 Document/fix/warn about rebase incompatibilities and inconsistenciesElijah Newren, Jun 27, 2018
  69. 2/9 git-rebase.sh: update help messages a bitElijah Newren, Jun 27, 2018
  70. 1/9 git-rebase.txt: document incompatible optionsElijah Newren, Jun 27, 2018
  71. 7/9 git-rebase.txt: document behavioral differences between modesElijah Newren, Jun 27, 2018
  72. 9/9 git-rebase: make --allow-empty-message the defaultElijah Newren, Jun 27, 2018
  73. 2.19.0 regression: leaving editor with empty commit message doesn't stop rebase [was: Re: [RFC PATCH v5 9/9] git-rebase: make --allow-empty-message the default]SZEDER Gábor, Sep 12, 2018
  74. Elijah NewrenSep 12, 2018
  75. sequencer: fix --allow-empty-message behavior, make it smarterElijah Newren, Sep 12, 2018
  76. Junio C HamanoSep 13, 2018
  77. 6/9 directory-rename-detection.txt: technical docs on abilities and limitationsElijah Newren, Jun 27, 2018
  78. 8/9 t3401: add directory rename testcases for rebase and amElijah Newren, Jun 27, 2018
  79. 4/9 git-rebase: error out when incompatible options passedElijah Newren, Jun 27, 2018
  80. 5/9 git-rebase.txt: address confusion between --no-ff vs --force-rebaseElijah Newren, Jun 27, 2018
  81. 3/9 t3422: new testcases for checking when incompatible options passedElijah Newren, Jun 27, 2018
  82. Phillip WoodJun 29, 2018
  83. git-rebase.sh: handle keep-empty like all other optionsElijah Newren, Jun 7, 2018
  84. Phillip WoodJun 10, 2018
  85. Elijah NewrenJun 11, 2018
  86. Phillip WoodJun 11, 2018
  87. Elijah NewrenJun 11, 2018
  88. Phillip WoodJun 12, 2018
  89. 1/2 t3418: add testcase showing problems with rebase -i and strategy optionsElijah Newren, Jun 7, 2018
  90. 2/2 Fix use of strategy options with interactive rebasesElijah Newren, Jun 7, 2018
  91. 0/2 Fix use of strategy options with interactive rebasesElijah Newren, Jun 27, 2018
  92. 1/2 t3418: add testcase showing problems with rebase -i and strategy optionsElijah Newren, Jun 27, 2018
  93. Eric SunshineJun 27, 2018
  94. Elijah NewrenJun 27, 2018
  95. Junio C HamanoJun 27, 2018
  96. Elijah NewrenJun 27, 2018
  97. Johannes SixtJun 27, 2018
  98. Eric SunshineJun 27, 2018
  99. SZEDER GáborJun 27, 2018
  100. Junio C HamanoJun 27, 2018
  101. Junio C HamanoJun 27, 2018
  102. 2/2 Fix use of strategy options with interactive rebasesElijah Newren, Jun 27, 2018
  103. 0/2 Fix use of strategy options with interactive rebasesElijah Newren, Jun 27, 2018
  104. 2/2 Fix use of strategy options with interactive rebasesElijah Newren, Jun 27, 2018
  105. Johannes SchindelinJul 12, 2018
  106. Elijah NewrenJul 13, 2018
  107. 1/2 t3418: add testcase showing problems with rebase -i and strategy optionsElijah Newren, Jun 27, 2018
  108. SZEDER GáborJun 27, 2018
  109. SZEDER GáborJul 11, 2018
  110. Elijah NewrenJul 11, 2018
  111. 0/3 Send more rebases through interactive machineryElijah Newren, Jun 7, 2018
  112. 1/3 git-rebase, sequencer: add a --quiet option for the interactive machineryElijah Newren, Jun 7, 2018
  113. Johannes SchindelinJun 9, 2018
  114. Elijah NewrenJun 9, 2018
  115. 3/3 git-rebase.sh: make git-rebase--interactive the defaultElijah Newren, Jun 7, 2018
  116. Johannes SchindelinJun 9, 2018
  117. Elijah NewrenJun 10, 2018
  118. Johannes SchindelinJun 17, 2018
  119. Elijah NewrenJun 20, 2018
  120. Johannes SchindelinJun 21, 2018
  121. Elijah NewrenJun 21, 2018
  122. Johannes SchindelinJan 21, 2019
  123. Elijah NewrenJan 21, 2019
  124. Johannes SchindelinJan 22, 2019
  125. 2/3 rebase: Implement --merge via git-rebase--interactiveElijah Newren, Jun 7, 2018
  126. Johannes SchindelinJun 9, 2018
  127. Elijah NewrenJun 10, 2018
  128. Phillip WoodJun 11, 2018
  129. Junio C HamanoJun 11, 2018
  130. Elijah NewrenJun 11, 2018

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.